Created attachment 268555 [details] [review]
Alternative HTML-based implementation for the overview

I like the idea. Will you also take bug 697318 (keyboard accessibility) into consideration? Thanks :)

With the provided patch, the keyboard accessibility problem is already managed.

As far as the proposed overview has a quite simple HTML, consisting of a list of links, the default tabindex allows moving between the elements of the overview. The focused element is shown changing the opacity of the thumbnail in the same way as on the :hover event, so the page can be opened by pressing 'enter' on the focused element (it would be possible to attach too a listener to the 'del' key to enable removing elements just with the keyboard). 

It would be easy to modify how explicitly the focused elements are displayed (in case just changing the opacity is too subtle) just by changing a CSS rule.

I have tested it quickly and I really like it. It's a great improvement, the animations and layouting is superb. Great work. 

Now from a quick usability review, these are the things I think need to be worked before we can merge this:

1. It needs to be able to react to thumbnail updates.
2. It needs to react to relevant changes in the history (page title updates, position in the history, new items appearing in an empty history, etc).
3. When a item is hidden, a new one should pop up.
4. The background color needs to be fixed.
5. The position of the "close" icon doesn't seem right. I'd say it needs to be similar to the one in the current overview widget.

(In reply to comment #4)
> I have tested it quickly and I really like it. It's a great improvement, the
> animations and layouting is superb. Great work. 
> 
> Now from a quick usability review, these are the things I think need to be
> worked before we can merge this:
> 
> 1. It needs to be able to react to thumbnail updates.
> 2. It needs to react to relevant changes in the history (page title updates,
> position in the history, new items appearing in an empty history, etc).
> 3. When a item is hidden, a new one should pop up.
> 4. The background color needs to be fixed.

Agree with everything.

> 5. The position of the "close" icon doesn't seem right. I'd say it needs to be
> similar to the one in the current overview widget.

I think this is not a bug, but a feature :-) For me the close icon is a lot easier to see now.

(In reply to comment #5)

> I think this is not a bug, but a feature :-) For me the close icon is a lot
> easier to see now.

If there are visibility issues, we can address them somehow (using gradients or something else), but a floating button outside the snapshot is rather odd to me.

I don't think it's out the snapshot, it's like the title, and you see it on hover, so it's pretty obvious that's to close that snapshot specifically.

Review of attachment 268555 [details] [review]:

I had a quick look. I wonder if the approach to updating the overview will scale well to doing other operations that relate to staying up to date to changes in the model.

::: embed/ephy-about-handler.c
@@ +459,3 @@
+      "<li>" \
+      "  <a class=\"overview-item\" title=\"%s\" href=\"%s\">" \
+  while (valid) {

Instead of doing this wouldn't it be possible to handle this by manipulating the DOM tree after the page is loaded? Having some kind of DOM/treemodel controller that keeps track of the overview model and the view and keeps them up to date, all happening on one place?

::: src/resources/overview.html
@@ +120,3 @@
+
+    event.preventDefault();
+    listItemNode.className +=" removed ";

This basically means that the item is not really removed from the view, just hidden, right?

(In reply to comment #8)
> 
> ::: src/resources/overview.html
> @@ +120,3 @@
> +
> +    event.preventDefault();
> +    listItemNode.className +=" removed ";
> 
> This basically means that the item is not really removed from the view, just
> hidden, right?

The reason for not directly removing the element from the DOM and changing it's status to a hidden class instead, is to be able to create animated transitions on the process of having the item deleted from the overview.

As just after removing the element the list has to be refreshed to get another  new item, either by a location.reload() or through DOM bindings, the element is actually removed from the HTML.

Created attachment 268678 [details] [review]
Alternative HTML-based implementation for the overview

Forced a js reload of the overview after removing an item to include another new one.

Created attachment 268715 [details] [review]
Remove the overview widget and use the html based one instead

I think it's a lot more convenient to try the new html based overview by actually replacing the current overview.

28 files changed, 67 insertions(+), 3396 deletions(-)

I'm really looking forward to getting rid of all those lines of code :-)

Review of attachment 268678 [details] [review]:

::: embed/ephy-about-handler.c
@@ +451,3 @@
+      -1);
+
+    // FIXME: use a more efficient way to get the base64 snapshot

Do not C++ style comments, use /* */ instead.

@@ +452,3 @@
+
+    // FIXME: use a more efficient way to get the base64 snapshot
+    gdk_pixbuf_save_to_buffer(row_snapshot, &buffer, &buffersize, "png", NULL, NULL);

gdk_pixbuf_save_to_buffer expects gchar, you need a cast here.

@@ +453,3 @@
+    // FIXME: use a more efficient way to get the base64 snapshot
+    gdk_pixbuf_save_to_buffer(row_snapshot, &buffer, &buffersize, "png", NULL, NULL);
+    row_snapshot_base64 = g_base64_encode(buffer, buffersize);

g_base64_encode(buffer -> g_base64_encode (buffer

@@ +454,3 @@
+    gdk_pixbuf_save_to_buffer(row_snapshot, &buffer, &buffersize, "png", NULL, NULL);
+    row_snapshot_base64 = g_base64_encode(buffer, buffersize);
+    g_free(buffer);

g_free(buffer) -> g_free (buffer)

@@ +460,3 @@
+      "  <a class=\"overview-item\" title=\"%s\" href=\"%s\">" \
+      "    <div class=\"close-button\" onclick=\"removeFromOverview(this,event)\" title=\"%s\">&#10006;</div>" \
+      "    <span class=\"thumbnail\" style=\"background-image: url(data:image/png;base64,%s);\"></span>" \

This is convenient but also a bit unfortunate, because we are sending the snapshot from the web process to the UI process and now back to the web process again. It's ok for now, but I wonder if we could do everything in the web process side eventually.

@@ +500,3 @@
+                   ((gtk_widget_get_default_direction () == GTK_TEXT_DIR_RTL) ? "rtl" : "ltr"),
+                   _("Most visited"),
+                   g_string_free(data_str,FALSE));

You are leaking the string contained in data_str here. You should use data_str->str directly and then save the string passing TRUE instead of FALSE.

@@ +502,3 @@
+                   g_string_free(data_str,FALSE));
+
+  data_length = html ->len;

html ->len -> html->len

::: embed/web-extension/ephy-web-extension.c
@@ +36,3 @@
 #include <libsoup/soup.h>
 #include <webkit2/webkit-web-extension.h>
 

Remove this empty line.

@@ +256,3 @@
+                                 "RemoveItemFromOverview",
+                                 g_variant_new ("(s)",
+                                                url),

This could be one line.

@@ +1179,3 @@
 
+
+static JSValueRef remove_from_overview_cb (JSContextRef context,

Move the emove_from_overview_cb (JSContextRef context, to a new line.

@@ +1187,3 @@
+{
+
+  char resultString[1024];

This 1024 looks a bit arbitrary. I think you can use JSStringGetMaximumUTF8CStringSize to get the bugger size and allocate it in the heap. It should be result_str or result_string, you are mixing coding styles here.

@@ +1189,3 @@
+  char resultString[1024];
+
+  JSStringRef resultStringJS = JSValueToStringCopy(context, arguments[0], NULL);

Move the declaration at the beginning of the function in a single block.
Same comment about the coding style.

@@ +1192,3 @@
+  JSStringGetUTF8CString(resultStringJS, resultString, 1024);
+
+  remove_from_overview_emit_signal(resultString);

You are laking the resultStringJS, use JSStringRelease();

@@ +1194,3 @@
+  remove_from_overview_emit_signal(resultString);
+
+  return  JSValueMakeUndefined(context);

return  JSValueMakeUndefined(context); -> return JSValueMakeUndefined(context);

@@ +1203,3 @@
+};
+
+static const JSClassDefinition notification_def =

This is specific to the overview js, so it should be called something less generic. Think that we might add more custom js here in the future.

@@ +1205,3 @@
+static const JSClassDefinition notification_def =
+{
+  0,                     // version

Do not use C++ style comments

@@ +1235,3 @@
+  context   = webkit_frame_get_javascript_context_for_script_world (frame, world);
+  globalObj = JSContextGetGlobalObject (context);
+

I think we could use a helper function like:
ephy_overview_js_initialize(context, global_object) or something like that.

@@ +1238,3 @@
+  JSClassRef  classDef = JSClassCreate (&notification_def);
+  JSObjectRef classObj = JSObjectMake (context, classDef, context);
+  JSStringRef str = JSStringCreateWithUTF8CString("Overview");

Same comments about the coding style and the variable declared inline

Review of attachment 268678 [details] [review]:

::: embed/ephy-about-handler.c
@@ +460,3 @@
+      "  <a class=\"overview-item\" title=\"%s\" href=\"%s\">" \
+      "    <div class=\"close-button\" onclick=\"removeFromOverview(this,event)\" title=\"%s\">&#10006;</div>" \
+  while (valid) {

Right now the overview model loads the snapshot as a GdKPixbuf, because this way it can be used by the iconview we use. Since we're removing the GTK+-based implementation we don't need to load the snapshot to a pixbuf, we could store the local URI instead and use that.

This is definitely doable but it will need a bit of work.

@@ +500,3 @@
+                   ((gtk_widget_get_default_direction () == GTK_TEXT_DIR_RTL) ? "rtl" : "ltr"),
+                   _("Most visited"),
+      g_markup_escape_text(row_title,-1), row_url, _("Remove from overview"),row_snapshot_base64, row_title);

Also, space after the function name.

Created attachment 268806 [details] [review]
overview: a few adjustments to the CSS style

Created attachment 268811 [details] [review]
Alternative HTML-based implementation for the overview

Thanks for the reviews. I have fixed the small code issues on this patch, still pending work on the improvements suggested.

Created attachment 268821 [details] [review]
Update the overview when the model changes

This solution is far from optimal, but it works, the overview is updated when a new page is added or when the snapshot is available. I'll improve it after the ui freeze.

(In reply to comment #15)
> Created an attachment (id=268806) [details] [review]
> overview: a few adjustments to the CSS style

I still prefer the close icon out of the snapshot, like the title

Comment on attachment 268821 [details] [review]
Update the overview when the model changes

We agreed not to do this for now.

Created attachment 269134 [details] [review]
Alternative HTML-based implementation for the overview

Added support to remove elements from the overview using the delete key

Created attachment 269141 [details]
screenshot

Finally got a chance to try this. Sorry for the delay!

Looks nice. A few comments:

 * It would be nice to be able to always show the same number of items per line. Right now when it reflows for size there may be an uneven number of items in the last row.
 * It would be nice to either center the grid in the width or make it adapt to size a bit better in order to fit things into a row without a lot of space on the right side.
 * It doesn't look like the close buttons on the items are using our standard window-close-symbolic icon.
 * I think there may be a little too much padding space at the top of the grid.

Created attachment 269392 [details] [review]
Add snapshot path to EphyOverviewStore

Created attachment 269393 [details] [review]
overview: Do not send decoded snapshots to the web process

Created attachment 269394 [details] [review]
Notify the web extension about history changes to update the overview

More cleanup is still needed after these patches, I'll do it once we have the new overview in master.

Created attachment 269409 [details] [review]
Style modifications to the html overview

Some adjustments on the placement of the grid elements on the
html overview, to get them always centered, as well as extracting
to the CSS the symbol for the close button.

Created attachment 269412 [details] [review]
Notify the web extension about history changes to update the overview

Updated patch, fixed the weak reference handling in EphyWebOverview.

Review of attachment 269393 [details] [review]:

Looks good, with comments.

::: embed/ephy-about-handler.c
@@ +447,3 @@
       EPHY_OVERVIEW_STORE_URI, &row_url,
       EPHY_OVERVIEW_STORE_TITLE, &row_title,
+      EPHY_OVERVIEW_STORE_SNAPSHOT_PATH, &row_snapshot,

Probably leaking the row_snapshot? It was a GObject but now it's a char pointer.

Review of attachment 269392 [details] [review]:

You should probably split this patch into several different ones.

Review of attachment 269409 [details] [review]:

This should probably be merged to the patches where those files were created or initially modified.

(In reply to comment #28)
> Review of attachment 269393 [details] [review]:
> 
> Looks good, with comments.
> 
> ::: embed/ephy-about-handler.c
> @@ +447,3 @@
>        EPHY_OVERVIEW_STORE_URI, &row_url,
>        EPHY_OVERVIEW_STORE_TITLE, &row_title,
> +      EPHY_OVERVIEW_STORE_SNAPSHOT_PATH, &row_snapshot,
> 
> Probably leaking the row_snapshot? It was a GObject but now it's a char
> pointer.

You are right

Comment on attachment 268806 [details] [review]
overview: a few adjustments to the CSS style

let's track the style adjustments in a different bug

Comment on attachment 269409 [details] [review]
Style modifications to the html overview

Ditto.

Created attachment 269437 [details] [review]
Alternative HTML-based implementation for the overview

Created attachment 269438 [details] [review]
Remove the overview widget and use the html based one instead

Created attachment 269439 [details] [review]
Add snapshot path to EphyOverviewStore

Created attachment 269440 [details] [review]
overview: Do not send decoded snapshots to the web process

Created attachment 269441 [details] [review]
Notify the web extension about history changes to update the overview

Created attachment 269508 [details] [review]
overview: Do not send decoded snapshots to the web process

Fix debug build

Created attachment 269518 [details] [review]
Notify the web extension about history changes to update the overview

Fixed several issues in multiprocess mode

Review of attachment 269437 [details] [review]:

Looks fine, but I still don't like the style much.

::: embed/web-extension/ephy-web-extension.c
@@ +245,3 @@
+{
+  char *username_field_value = NULL;
+  guint request_id;

Unused.

Review of attachment 269438 [details] [review]:

Good.

::: embed/ephy-web-view.c
@@ +739,3 @@
   else if (!strcmp (address, EPHY_ABOUT_SCHEME":overview") ||
            !strcmp (address, "about:overview"))
+    return g_strdup (_("Most Visited"));

Do we really need this? Now that the overview is html we could just use a <title> tag, no?

(In reply to comment #42)
> Review of attachment 269438 [details] [review]:
> 
> Good.
> 
> ::: embed/ephy-web-view.c
> @@ +739,3 @@
>    else if (!strcmp (address, EPHY_ABOUT_SCHEME":overview") ||
>             !strcmp (address, "about:overview"))
> +    return g_strdup (_("Most Visited"));
> 
> Do we really need this? Now that the overview is html we could just use a
> <title> tag, no?

I'm not sure, that's called to get the title based on the address, I guess it's because we still don't have the title, to show it as early as possible. But I'm just guessing.

Created attachment 269526 [details] [review]
Alternative HTML-based implementation for the overview

Removed unused variables

Review of attachment 269439 [details] [review]:

I'm not a big fan of this patch changing the API of the overview store and adding signals, etc. and at the same time using these changes. I'd split them for clarity.

::: lib/widgets/ephy-overview-store.c
@@ +242,2 @@
   gtk_list_store_set (GTK_LIST_STORE (store), iter,
                       EPHY_OVERVIEW_STORE_SNAPSHOT, framed,

Is there a reason why we still need to store the snapshot itself?

Review of attachment 269508 [details] [review]:

Looks fine.

Review of attachment 269508 [details] [review]:

Looks fine.

(In reply to comment #45)
> Review of attachment 269439 [details] [review]:
> 
> I'm not a big fan of this patch changing the API of the overview store and
> adding signals, etc. and at the same time using these changes. I'd split them
> for clarity.
>

Tried to split the patch, but changes depend on each other, it's a pain to split it now

> ::: lib/widgets/ephy-overview-store.c
> @@ +242,2 @@
>    gtk_list_store_set (GTK_LIST_STORE (store), iter,
>                        EPHY_OVERVIEW_STORE_SNAPSHOT, framed,
> 
> Is there a reason why we still need to store the snapshot itself?

No there isn't, and I plan to continue cleaning this up once these patches land.

Review of attachment 269518 [details] [review]:

Patch makes sense in general, but I won't lie, I probably need a lot of time to review it properly, but I think we can land it as is.

Review of attachment 269526 [details] [review]:

OK

Created attachment 269692 [details] [review]
overview thumbnails: improve stylesheet

- replace hard baked frames with css properties
- adjust position of close buttons and reassure action with
  specific :hover state for the close button
- avoid using alpha for normal state (feels washed out). Raise
  the thumbail on :hover and sunk it for :active

The thing missing is thumbnails still having the shadow baked in.
For this to work, the web thumnails need to remain flat.

Created attachment 269712 [details] [review]
overview: further improve layout

- center the thumbnails, keeping a decent narrow view
- using table-cells to top align (yuck)

Created attachment 269715 [details] [review]
overview: further improve layout

- center the thumbnails, keeping a decent narrow view
- using table-cells to top align (yuck)

Review of attachment 269715 [details] [review]:

Looks better. There's some spacing garbage in the patch, please fix that. Not sure if we're affected by the UI freeze at this point though.

For further patches please file new bugs.