After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 686376 - Port to WebKit2
Port to WebKit2
Status: RESOLVED FIXED
Product: yelp
Classification: Applications
Component: General
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Yelp maintainers
Yelp maintainers
Depends on: 683100
Blocks: 686373
 
 
Reported: 2012-10-18 12:50 UTC by Carlos Garcia Campos
Modified: 2015-06-23 09:22 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
--with-webkit2 (12.26 KB, patch)
2014-05-01 11:24 UTC, Jarek Czekalski
none Details | Review
autotools: Adapt autotools for using WebKit2 (686 bytes, patch)
2014-10-31 12:37 UTC, Marcos Chavarria Teijeiro
none Details | Review
yelp: Change headers files (1.54 KB, patch)
2014-10-31 12:39 UTC, Marcos Chavarria Teijeiro
reviewed Details | Review
yelp-view: Replace "navigation-policy-decision-requested" signal by "decide-policy" signal (4.19 KB, patch)
2014-10-31 12:56 UTC, Marcos Chavarria Teijeiro
reviewed Details | Review
yelp-view: Replace "script-alert" signal by "script-dialog" signal (2.88 KB, patch)
2014-10-31 12:56 UTC, Marcos Chavarria Teijeiro
reviewed Details | Review
yelp-view: Replace "resource-request-starting" signal by "resource-load-started" signal (3.76 KB, patch)
2014-10-31 12:57 UTC, Marcos Chavarria Teijeiro
none Details | Review
yelp-view: Replace "document-load-finished" signal by "load-changed" signal (3.03 KB, patch)
2014-10-31 12:57 UTC, Marcos Chavarria Teijeiro
none Details | Review
yelp-view: Rename WebKitWebSettings to WebKitSettings (1.23 KB, patch)
2014-10-31 12:57 UTC, Marcos Chavarria Teijeiro
reviewed Details | Review
yelp-view: Implement view_print_action using WebKit2 API (1.26 KB, patch)
2014-10-31 12:58 UTC, Marcos Chavarria Teijeiro
reviewed Details | Review
yelp-view: Use WKFindController instead of deprecated search functions in WKWebView (4.01 KB, patch)
2014-10-31 12:58 UTC, Marcos Chavarria Teijeiro
reviewed Details | Review
yelp-view: Adapt properties of WebSettings to WK2 Settings (1.88 KB, patch)
2014-10-31 12:58 UTC, Marcos Chavarria Teijeiro
reviewed Details | Review
yelp-document: Avoid GLib warning on referencing null object (1.10 KB, patch)
2014-10-31 12:59 UTC, Marcos Chavarria Teijeiro
reviewed Details | Review
yelp-view: Replace "populate-popup" signal by "context-menu" signal (20.17 KB, patch)
2014-10-31 12:59 UTC, Marcos Chavarria Teijeiro
reviewed Details | Review
yelp-view: Implement web extension to deal with DOM tree (44.45 KB, patch)
2014-10-31 12:59 UTC, Marcos Chavarria Teijeiro
reviewed Details | Review
yelp-view: Fix view_show_error_page (2.77 KB, patch)
2014-10-31 12:59 UTC, Marcos Chavarria Teijeiro
reviewed Details | Review
yelp-view: Implement custom uri schemes (41.50 KB, patch)
2014-10-31 13:00 UTC, Marcos Chavarria Teijeiro
reviewed Details | Review
yelp-view: Implement web extension to load resources (7.95 KB, patch)
2014-10-31 13:00 UTC, Marcos Chavarria Teijeiro
reviewed Details | Review
yelp: Change headers files to use WebKit2 (1.56 KB, patch)
2014-11-03 09:41 UTC, Marcos Chavarria Teijeiro
none Details | Review
yelp-view: Replace "navigation-policy-decision-requested" signal by "decide-policy" signal (4.16 KB, patch)
2014-11-03 09:45 UTC, Marcos Chavarria Teijeiro
none Details | Review
yelp-view: Replace "script-alert" signal by "script-dialog" signal (2.79 KB, patch)
2014-11-03 09:47 UTC, Marcos Chavarria Teijeiro
none Details | Review
yelp-view: Rename WKWebSettings to WKSettings and adapt properties (2.56 KB, patch)
2014-11-03 10:16 UTC, Marcos Chavarria Teijeiro
none Details | Review
yelp-view: Implement view_print_action using WebKit2 API (1.02 KB, patch)
2014-11-03 10:18 UTC, Marcos Chavarria Teijeiro
reviewed Details | Review
yelp-view: Use WKFindController instead of deprecated search functions in WKWebView (4.01 KB, patch)
2014-11-03 10:20 UTC, Marcos Chavarria Teijeiro
none Details | Review
yelp-view: Rename WKWebSettings to WKSettings and adapt properties (2.56 KB, patch)
2014-11-03 12:01 UTC, Marcos Chavarria Teijeiro
none Details | Review
yelp-view: Replace "populate-popup" signal by "context-menu" signal (19.86 KB, patch)
2014-11-05 09:59 UTC, Marcos Chavarria Teijeiro
reviewed Details | Review
yelp-view: Implement web extension to deal with DOM tree (17.69 KB, patch)
2014-11-05 10:21 UTC, Marcos Chavarria Teijeiro
reviewed Details | Review
yelp-view: Implement pages load in WebKit2 (54.95 KB, patch)
2014-11-05 12:44 UTC, Marcos Chavarria Teijeiro
none Details | Review
yelp-view: Implement pages load in WebKit2 (54.91 KB, patch)
2014-11-05 16:44 UTC, Marcos Chavarria Teijeiro
none Details | Review
Fix leak. (1.06 KB, patch)
2014-11-06 10:17 UTC, Marcos Chavarria Teijeiro
none Details | Review
yelp-view: Replace "populate-popup" signal by "context-menu" signal (20.25 KB, patch)
2014-11-06 10:28 UTC, Marcos Chavarria Teijeiro
none Details | Review
yelp-view: Implement web extension to deal with DOM tree (16.99 KB, patch)
2014-11-06 10:48 UTC, Marcos Chavarria Teijeiro
reviewed Details | Review
yelp-view: Replace "populate-popup" signal by "context-menu" signal (20.23 KB, patch)
2014-11-06 11:17 UTC, Marcos Chavarria Teijeiro
none Details | Review
yelp-view: Implement web extension to load resources (5.08 KB, patch)
2014-11-06 11:47 UTC, Marcos Chavarria Teijeiro
reviewed Details | Review
yelp-view: Implement web extension to deal with DOM tree (17.07 KB, patch)
2014-11-07 08:25 UTC, Marcos Chavarria Teijeiro
reviewed Details | Review
yelp-view: Implement web extension to load resources (24.34 KB, patch)
2014-11-07 11:14 UTC, Marcos Chavarria Teijeiro
none Details | Review
yelp-view: Implement web extension to load resources (13.96 KB, patch)
2014-11-12 09:33 UTC, Marcos Chavarria Teijeiro
none Details | Review
yelp-view: Implement web extension to deal with DOM tree (17.19 KB, patch)
2014-11-12 09:42 UTC, Marcos Chavarria Teijeiro
none Details | Review
yelp-window: Remove ScrolledWindow to hold WebView. (1.90 KB, patch)
2014-11-19 09:44 UTC, Marcos Chavarria Teijeiro
none Details | Review
Debug build backtrace of UI process crash when loading man page (35.08 KB, text/plain)
2015-06-13 18:21 UTC, Michael Catanzaro
  Details
screenshot when running 'LANG=pl_PL.utf8 jhbuild run yelp info:/usr/share/info/assuan.info.bz2' (16.86 KB, image/png)
2015-06-13 23:16 UTC, Michael Catanzaro
  Details
my counterscrenshot (11.35 KB, image/png)
2015-06-13 23:58 UTC, Rafał Mużyło
  Details
Handle info uris (2.96 KB, patch)
2015-06-15 18:24 UTC, Carlos Garcia Campos
none Details | Review
gdb backtrace (2.79 KB, text/plain)
2015-06-15 20:27 UTC, Rafał Mużyło
  Details

Description Carlos Garcia Campos 2012-10-18 12:50:18 UTC
Yelp can be ported to WebKit2, although bug #683100 needs to be fixed first.
Comment 1 Jarek Czekalski 2014-04-30 13:17:41 UTC
Do we have any branch or patch that does the port?
Comment 2 Jarek Czekalski 2014-05-01 11:24:30 UTC
Created attachment 275530 [details] [review]
--with-webkit2

This is a very brute approach to porting to webkit2. Only obvious build breaks were hackarounded. But it builds and displays a page. Warnings present at runtime.
Comment 3 Jarek Czekalski 2014-05-04 07:25:34 UTC
Carlos, is this wiki page [1] up-to-date? It says:
>This can useful when access to the DOM is required. [Planned, but not currently implemented]

I saw yelp does access DOM. Doesn't it make a problem in webkit2?

[1] http://trac.webkit.org/wiki/WebKit2#CAPI
Comment 4 Carlos Garcia Campos 2014-05-04 08:42:46 UTC
GObject DOM bindings API is accessible in WebKit2 using WebProcess extensions API. See http://blogs.igalia.com/carlosgc/2013/09/10/webkit2gtk-web-process-extensions/ for more information about web extensions in WebKit2
Comment 5 Marcos Chavarria Teijeiro 2014-10-31 12:37:33 UTC
Created attachment 289726 [details] [review]
autotools: Adapt autotools for using WebKit2
Comment 6 Marcos Chavarria Teijeiro 2014-10-31 12:39:29 UTC
Created attachment 289728 [details] [review]
yelp: Change headers files
Comment 7 Marcos Chavarria Teijeiro 2014-10-31 12:56:20 UTC
Created attachment 289729 [details] [review]
yelp-view: Replace "navigation-policy-decision-requested" signal by "decide-policy" signal

WK2 only has a signal for policy decisions.
Comment 8 Marcos Chavarria Teijeiro 2014-10-31 12:56:49 UTC
Created attachment 289730 [details] [review]
yelp-view: Replace "script-alert" signal by "script-dialog" signal
Comment 9 Marcos Chavarria Teijeiro 2014-10-31 12:57:17 UTC
Created attachment 289731 [details] [review]
yelp-view: Replace "resource-request-starting" signal by "resource-load-started" signal

I have to use new class WebKitURIRequest methods.
Comment 10 Marcos Chavarria Teijeiro 2014-10-31 12:57:35 UTC
Created attachment 289732 [details] [review]
yelp-view: Replace "document-load-finished" signal by "load-changed" signal

"load-changed" is a generic signal that receives a type parameter. I have add if
condition to return any execution of the handler that doesn't correspond to a
LOAD_FINISHED event.
In addition we have a problem (again) with DOM modification that should be FIXED
someway. Maybe WebExtensions is a good choice this time.
Comment 11 Marcos Chavarria Teijeiro 2014-10-31 12:57:50 UTC
Created attachment 289733 [details] [review]
yelp-view: Rename WebKitWebSettings to WebKitSettings
Comment 12 Marcos Chavarria Teijeiro 2014-10-31 12:58:07 UTC
Created attachment 289734 [details] [review]
yelp-view: Implement view_print_action using WebKit2 API
Comment 13 Marcos Chavarria Teijeiro 2014-10-31 12:58:26 UTC
Created attachment 289735 [details] [review]
yelp-view: Use WKFindController instead of deprecated search functions in WKWebView
Comment 14 Marcos Chavarria Teijeiro 2014-10-31 12:58:51 UTC
Created attachment 289736 [details] [review]
yelp-view: Adapt properties of WebSettings to WK2 Settings

Remove enable-universal-access-from-file-uris that doesn't
exist any more and rename default-encoding to default-charset.
Comment 15 Marcos Chavarria Teijeiro 2014-10-31 12:59:06 UTC
Created attachment 289737 [details] [review]
yelp-document: Avoid GLib warning on referencing null object
Comment 16 Marcos Chavarria Teijeiro 2014-10-31 12:59:18 UTC
Created attachment 289738 [details] [review]
yelp-view: Replace "populate-popup" signal by "context-menu" signal

New WK2 API HitTestResutl class doesnt have a reference to the DOM
node so we cant know using this property if we are inside a code block
or the name of a link.
Comment 17 Marcos Chavarria Teijeiro 2014-10-31 12:59:34 UTC
Created attachment 289739 [details] [review]
yelp-view: Implement web extension to deal with DOM tree
Comment 18 Marcos Chavarria Teijeiro 2014-10-31 12:59:51 UTC
Created attachment 289740 [details] [review]
yelp-view: Fix view_show_error_page

Substitute webkit_web_view_load_string call for webkit_web_view_load_html
and remove navigation requested signal unblock.
Comment 19 Marcos Chavarria Teijeiro 2014-10-31 13:00:11 UTC
Created attachment 289741 [details] [review]
yelp-view: Implement custom uri schemes

We should deal with the uris that Yelp undestand and the uris that Webkit undestand so we have created functions to convert from one to the others. In addition, we should add a hack to be able to load absolute uris. When we have a help:gnome-help/... uri on our document and the current page has the same scheme (help) WebKit interprets this uri as relative and it builds a different uri. To fix this instead of use a help scheme we use a bogus-help schme so WebKit interprets the uri as absolute.
Comment 20 Marcos Chavarria Teijeiro 2014-10-31 13:00:34 UTC
Created attachment 289742 [details] [review]
yelp-view: Implement web extension to load resources
Comment 21 Marcos Chavarria Teijeiro 2014-10-31 13:04:12 UTC
Hi! I have been working on porting Yelp to WebKit2 during the last two months. These are the patches with the changes I have made :)

In addition I have a GitHub repository with the changes applyed in a separate branch. https://github.com/chavaone/yelp/tree/wip/webkit2-port
Comment 22 David King 2014-10-31 14:10:02 UTC
Review of attachment 289729 [details] [review]:

::: libyelp/yelp-view.c
@@ +1444,3 @@
 }
 
+

Stray extra line here.

@@ +1456,3 @@
+    WebKitNavigationPolicyDecision *navigation_decision = WEBKIT_NAVIGATION_POLICY_DECISION (decision);
+    WebKitURIRequest *request = webkit_navigation_policy_decision_get_request (navigation_decision);
+    const gchar *requri = webkit_uri_request_get_uri (request);

I would move these code lines to be below the assignment to the private pointer, but this is just nitpicking.
Comment 23 David King 2014-10-31 14:19:32 UTC
Review of attachment 289742 [details] [review]:

::: libyelp/Makefile.am
@@ -1,1 @@
-SUBDIRS = web-extensions

You added this directory to SUBDIRS only a couple of patches earlier. Is there a reason to remove it again? (I see that you added the subdirectories to the toplevel Makefile.am in this patch, but I am curious what the reasoning is.)

::: libyelp/web-extensions/change-resources-uri/change_resources_uri.c
@@ +68,3 @@
+    gchar * wk_uri;
+    gchar * yelp_str_uri;
+    gchar * file_path;

You seem to have some extra spaces here.

@@ +74,3 @@
+
+    wk_uri = webkit_uri_request_get_uri (request);
+    yelp_str_uri = build_yelp_uri (strdup (wk_uri));

I think that it is better to try to use only a single memory allocator if possible (in other words, to always use g_strdup() rather than strdup()), which means that free() should also be replaced with g_free(). I think that, in practice, g_free() calls free() anyway, but free() does not seem be used in Yelp, so it is better to be consistent with the rest of the codebase.

@@ +104,3 @@
+
+    if (current_uri)
+        free(current_uri);

Similar argument to above about g_free() versus free().

@@ +109,3 @@
+    current_yelp_uri = yelp_uri_new (uri);
+
+    if (! yelp_uri_is_resolved (current_yelp_uri))

Yelp style seems to be to omit the space after the exclamation mark.

::: libyelp/yelp-view.c
@@ +339,3 @@
     priv->popup_send_media_action = gtk_action_new ("yelp-view-popup-send-media",
                                                     NULL, NULL, NULL);
+     g_signal_connect (priv->popup_send_media_action,

Unwanted indentation change?
Comment 24 David King 2014-10-31 14:28:37 UTC
Review of attachment 289730 [details] [review]:

::: libyelp/yelp-view.c
@@ +1447,3 @@
+        message = webkit_script_dialog_get_message (dialog);
+        printf ("\n\n===ALERT===\n%s\n\n", message);
+        g_free (message);

You should not be calling g_free() on a const char *, should you?
Comment 25 Marcos Chavarria Teijeiro 2014-10-31 14:29:44 UTC
(In reply to comment #23)
> Review of attachment 289742 [details] [review]:
> 
> ::: libyelp/Makefile.am
> @@ -1,1 @@
> -SUBDIRS = web-extensions
> 
> You added this directory to SUBDIRS only a couple of patches earlier. Is there
> a reason to remove it again? (I see that you added the subdirectories to the
> toplevel Makefile.am in this patch, but I am curious what the reasoning is.)

The reason is that this web extension uses som libyelp classes so I need to have them compiled before compile the extension, the directory that I've removed from there has been added to the root Makefile.am.

Another option would be to move the entire to the root directory but I didnt do it because I think that the extensions are part of the library and so they should be a subdirectory of libyelp.
Comment 26 David King 2014-10-31 14:32:32 UTC
Review of attachment 289731 [details] [review]:

Seems fine.
Comment 27 David King 2014-10-31 14:35:39 UTC
Review of attachment 289733 [details] [review]:

Seems fine, although I would probably squash this patch into "yelp-view: Adapt properties of WebSettings to WK2 Settings"
Comment 28 David King 2014-10-31 14:40:22 UTC
Review of attachment 289736 [details] [review]:

::: libyelp/yelp-view.c
@@ +570,3 @@
 {
+    GtkWidget *obj = (GtkWidget *) g_object_new (YELP_TYPE_VIEW, NULL);
+    webkit_web_view_set_settings (WEBKIT_WEB_VIEW(obj), websettings);

It is better to do this in the init() method if you can. As "settings" is a construct property, I guess that you would have to do the initialization in constructed(), come to think of it.
Comment 29 David King 2014-10-31 14:41:26 UTC
Review of attachment 289728 [details] [review]:

Seems fine, although it would be nice to mention WebKit2 in the commit message.
Comment 30 David King 2014-10-31 14:41:55 UTC
Review of attachment 289726 [details] [review]:

Seems fine.
Comment 31 David King 2014-10-31 14:44:04 UTC
Review of attachment 289734 [details] [review]:

::: libyelp/yelp-view.c
@@ +1570,3 @@
+         window = gtk_widget_get_parent (window));
+
+    print_operation = webkit_print_operation_new (WEBKIT_WEB_VIEW(view));

Missing a space after WEBKIT_WEB_VIEW.
Comment 32 David King 2014-10-31 14:47:58 UTC
Review of attachment 289735 [details] [review]:

::: src/yelp-window.c
@@ +999,3 @@
 {
     YelpWindowPrivate *priv = GET_PRIV (window);
+    WebKitFindController * find_controller;

You have an extra space here.

@@ +1001,3 @@
+    WebKitFindController * find_controller;
+
+    find_controller = webkit_web_view_get_find_controller (WEBKIT_WEB_VIEW(priv->view));

Extra space needed after WEBKIT_WEB_VIEW.

@@ +1025,2 @@
     YelpWindowPrivate *priv = GET_PRIV (window);
+    WebKitFindController * find_controller;

You have an extra space here.

@@ +1044,3 @@
+    WebKitFindController * find_controller;
+
+    find_controller = webkit_web_view_get_find_controller (WEBKIT_WEB_VIEW(priv->view));

Same comments about spaces as earlier.

@@ +1055,3 @@
+    WebKitFindController * find_controller;
+
+    find_controller = webkit_web_view_get_find_controller (WEBKIT_WEB_VIEW(priv->view));

Same comments about spaces as earlier.
Comment 33 David King 2014-10-31 14:49:33 UTC
Comment on attachment 275530 [details] [review]
--with-webkit2

Obsoleted by the more recent patches.
Comment 34 David King 2014-10-31 15:02:04 UTC
Review of attachment 289738 [details] [review]:

Typo in the commit message: HitTestResutl.

::: libyelp/yelp-view.c
@@ +276,3 @@
                       G_CALLBACK (view_script_dialog), NULL);
 
+    priv->popup_copy_code_action = gtk_action_new ("yelp-view-popup-copy-code",

As GtkAction is a GObject and is created with a reference count of 1, do the actions ever need to be unreffed?

@@ +1016,1 @@
                  YelpView    *view)

Might be too annoying to fix every instance of this, but it the two parameters should line up.

@@ +1330,3 @@
         g_free (priv->popup_link_text);
         priv->popup_link_text = NULL;
+

Whitespace change? (Difficult to see in Splinter.)
Comment 35 David King 2014-10-31 15:03:20 UTC
Review of attachment 289741 [details] [review]:

::: libyelp/yelp-uri-builder.c
@@ +96,3 @@
+  }
+
+  g_memmove (uri, uri + BOGUS_PREFIX_LEN, strlen(uri) - BOGUS_PREFIX_LEN + 1);

g_memmove() is deprecated as of GLib 2.40, and you should use memmove() instead.
Comment 36 David King 2014-10-31 15:19:02 UTC
Review of attachment 289726 [details] [review]:

On second thoughts, does this use any API added in 2.7.1, or is 2.6 sufficient?
Comment 37 David King 2014-10-31 15:22:38 UTC
Review of attachment 289741 [details] [review]:

::: libyelp/yelp-view.c
@@ +866,3 @@
+
+    for (i = 0; help_schemes[i] != NULL; i++) {
+        network_scheme = build_network_scheme (help_schemes[i]);

help_schemes is const gchar *, but build_network_scheme() accepts a gchar *, which discards the const.
Comment 38 David King 2014-10-31 15:24:03 UTC
Review of attachment 289742 [details] [review]:

::: libyelp/web-extensions/change-resources-uri/change_resources_uri.c
@@ +73,3 @@
+        return FALSE;
+
+    wk_uri = webkit_uri_request_get_uri (request);

webkit_uri_request_get_uri() returns a const gchar *, but wk_uri is a gchar *, which discards the const.

@@ +106,3 @@
+        free(current_uri);
+
+    current_uri = uri;

This also discards the const.
Comment 39 David King 2014-10-31 15:32:56 UTC
Review of attachment 289739 [details] [review]:

::: libyelp/web-extensions/current-dom-node-extension/current-dom-node-web-extension.c
@@ +213,3 @@
+    web_page = webkit_web_extension_get_page (extension, page_id);
+
+    if (! web_page) {

Yelp style is to have no space after the exclamation mark.

@@ +220,3 @@
+
+    document = webkit_web_page_get_dom_document (web_page);
+    saved_element = WEBKIT_DOM_NODE(webkit_dom_document_element_from_point (document, x, y));

Missing a space after WEBKIT_DOM_NODE.

@@ +234,3 @@
+                    GVariant *parameters,
+                    GDBusMethodInvocation *invocation,
+                    WebKitWebExtension * web_extension)

You have an extra space here.

@@ +269,3 @@
+
+static const GDBusInterfaceVTable interface_vtable = {
+    handle_method_call,

This gives a compiler warning about an incompatible pointer type. A cast would seem to be appropriate.

::: libyelp/yelp-view.c
@@ +832,3 @@
+                                      NULL);
+
+    if (! dbus_result)

Yelp style is for there to be no space after the exclamation mark.

@@ +856,3 @@
+                                      NULL);
+
+    if (! dbus_result)

Same as above about the exclamation mark.

@@ +880,3 @@
+                                          NULL);
+
+    if (! dbus_result)

Same as above about the exclamation mark.

@@ +904,3 @@
+                                          NULL);
+
+    if (! dbus_result)

Same as above about the exclamation mark.

@@ +1406,3 @@
+    content = view_current_dom_node_web_extension_code_get_code (view);
+
+    if (! content)

Same as above about the exclamation mark.

@@ +1412,2 @@
     gtk_clipboard_set_text (clipboard, content, -1);
+

Whitespace change?

@@ +1422,3 @@
     GtkWidget *dialog, *window;
     gint res;
+    gchar * filename;

No need for the extra space after *.

@@ +1428,3 @@
+    priv->popup_code_text = view_current_dom_node_web_extension_code_get_code (view);
+
+    if (! priv->popup_code_text)

Same as above about the exclamation mark.

@@ +1454,2 @@
         gtk_file_chooser_set_current_name (GTK_FILE_CHOOSER (dialog), filename);
+

Whitespace?

@@ +1545,3 @@
         }
+
+        if (! priv->popup_link_text)

Same as above about the exclamation mark.
Comment 40 David King 2014-10-31 15:35:15 UTC
Review of attachment 289740 [details] [review]:

Seems fine.
Comment 41 David King 2014-10-31 15:39:27 UTC
Review of attachment 289737 [details] [review]:

As this change seems to be solely related to your change in "yelp-view: Implement custom uri schemes", where you call yelp_document_request_page() with a NULL cancellable, I would squash it into that patch.
Comment 42 David King 2014-10-31 15:42:40 UTC
Review of attachment 289732 [details] [review]:

Seems fine.
Comment 43 Carlos Garcia Campos 2014-10-31 16:33:47 UTC
Review of attachment 289730 [details] [review]:

::: libyelp/yelp-view.c
@@ +1444,3 @@
+
+    if (type == WEBKIT_SCRIPT_DIALOG_ALERT)
+    {

Move the brace to the previous line. I would use an early return, though

@@ +1447,3 @@
+        message = webkit_script_dialog_get_message (dialog);
+        printf ("\n\n===ALERT===\n%s\n\n", message);
+        g_free (message);

Right, you could use webkit_script_dialog_get_message directly

printf ("\n\n===ALERT===\n%s\n\n", webkit_script_dialog_get_message (dialog));
Comment 44 Carlos Garcia Campos 2014-10-31 16:38:33 UTC
Review of attachment 289731 [details] [review]:

This doesn't make sense. I know it's confusing, but resource-load-started is not equivalent to resource-request-starting. Changing the URI of the request in resource-load-started doesn't have any effect, since this signal is only a notification that a new resource load has started. The equivalent is WebKitWebPage::send-request, that allows you to change the URI of the request that is about to be sent, or even block the resource load by returning NULL (instead of the old code that set the uri to about:blank).
Comment 45 Carlos Garcia Campos 2014-10-31 16:50:12 UTC
Review of attachment 289734 [details] [review]:

::: libyelp/yelp-view.c
@@ +1562,3 @@
 view_print_action (GAction *action, GVariant *parameter, YelpView *view)
 {
+    WebKitPrintOperation * print_operation;

WebKitPrintOperation *print_operation;

@@ +1568,3 @@
+    for (window = gtk_widget_get_parent (GTK_WIDGET (view));
+         window && !GTK_IS_WINDOW (window);
+         window = gtk_widget_get_parent (window));

gtk_widget_get_toplevel()?

@@ +1575,3 @@
+
+    if (res == WEBKIT_PRINT_OPERATION_RESPONSE_PRINT)
+        webkit_print_operation_print (print_operation);

This is not correct. webkit_print_operation_run_dialog() will show the print dialog, and will start the print if the user clicks the print button. webkit_print_operation_print() is to print without showing the dialog, when you already have the print settings. In this particular case you can simply ignore the return value of webkit_print_operation_run_dialog.

@@ +1578,1 @@
 }

You are leaking the print operation, you should g_object_unref the WebKitPrintOperation
Comment 46 Carlos Garcia Campos 2014-10-31 16:55:49 UTC
Review of attachment 289733 [details] [review]:

::: libyelp/yelp-view.c
@@ +389,3 @@
     nautilus_sendto = g_find_program_in_path ("nautilus-sendto");
 
+    websettings = webkit_settings_new ();

This is not enough, since the g_object_set below will set a setting that doesn't exist. Also, in WebKit2, the settings is a construct property of the view, so I would remove the g_object_get in yelp_view_init, and would add websettings as construct parameter of g_object_new in yelp_view_new.
Comment 47 Marcos Chavarria Teijeiro 2014-10-31 17:01:23 UTC
(In reply to comment #44)
> Review of attachment 289731 [details] [review]:
> 
> This doesn't make sense. I know it's confusing, but resource-load-started is
> not equivalent to resource-request-starting. Changing the URI of the request in
> resource-load-started doesn't have any effect, since this signal is only a
> notification that a new resource load has started. The equivalent is
> WebKitWebPage::send-request, that allows you to change the URI of the request
> that is about to be sent, or even block the resource load by returning NULL
> (instead of the old code that set the uri to about:blank).

In addition I remove this function on other patch :S I should remove that patch...
Comment 48 Marcos Chavarria Teijeiro 2014-10-31 17:03:53 UTC
Review of attachment 289731 [details] [review]:

I need to remove that patch because it applies changes that are removed on a later commit.
Comment 49 Carlos Garcia Campos 2014-10-31 17:06:39 UTC
Review of attachment 289736 [details] [review]:

I see now, I agree with David, this should be squashed in the previous commit that updated the settings API.

::: libyelp/yelp-view.c
@@ +570,3 @@
 {
+    GtkWidget *obj = (GtkWidget *) g_object_new (YELP_TYPE_VIEW, NULL);
+    webkit_web_view_set_settings (WEBKIT_WEB_VIEW(obj), websettings);

Or add it as construct property here:

g_object_new (YELP_TYPE_VIEW, "settings", websettings, NULL);
Comment 50 Carlos Garcia Campos 2014-10-31 17:09:48 UTC
Review of attachment 289736 [details] [review]:

::: libyelp/yelp-view.c
@@ +1869,2 @@
     g_object_set (websettings,
+                  "default-charset", "utf-8",

These settings that are hardcoded could be set only once when the settings object is created. You could use webkit_settings_new_with_settings() and pass these.
Comment 51 Carlos Garcia Campos 2014-10-31 17:10:57 UTC
Review of attachment 289732 [details] [review]:

This is not equivalent either, the document-loaded signal is in WebKitWebPage now, load-finished is not equivalent to document-loaded.
Comment 52 Carlos Garcia Campos 2014-10-31 17:34:50 UTC
Review of attachment 289738 [details] [review]:

::: libyelp/yelp-view.c
@@ +239,3 @@
+    GtkAction  *popup_copy_text_action;
+    GtkAction  *popup_send_email_action;
+    GtkAction  *popup_install_packages_action;

You should probably use a GtkActionGroup

@@ +276,3 @@
                       G_CALLBACK (view_script_dialog), NULL);
 
+    priv->popup_copy_code_action = gtk_action_new ("yelp-view-popup-copy-code",

Yes, all those actions are indeed leaked.

@@ +344,3 @@
+              "activate",
+              G_CALLBACK (popup_copy_clipboard),
+              view);

You can define your actions using an array of GtkActionEntry, and pass it to gtk_action_group_add_actions() that already connects the activate signal to the given callbacks.

@@ +1295,1 @@
                   "context", &context,

I would use webkit_hit_test_result_is_* functions instead of getting the context, I think it makes the code eaier to read, but this is just my personal opinion :-)

@@ +1324,3 @@
     if (context & WEBKIT_HIT_TEST_RESULT_CONTEXT_LINK) {
         gchar *uri;
+        g_object_get (hit_test_result, "link-uri", &uri, NULL);

Same here, webkit_hit_test_result_get_link_uri), it's also more efficient than g_object_get
Comment 53 Carlos Garcia Campos 2014-10-31 18:06:23 UTC
Review of attachment 289739 [details] [review]:

::: configure.ac
@@ +45,3 @@
+PKG_CHECK_MODULES(YELP_EXTENSION,
+[
+	webkit2gtk-4.0 >= 2.5

Use webkit2gtk-web-extension-4.0, it's currently the same as webkit2gtk-4.0, but if might change in the future

@@ +245,3 @@
 tests/Makefile
+libyelp/web-extensions/Makefile
+libyelp/web-extensions/current-dom-node-extension/Makefile

I don't think we need an extra subdirectory inside web-extension

::: libyelp/Makefile.am
@@ +1,1 @@
+SUBDIRS = web-extensions

I don't think yelp will have more than one web extensions, call this web-extension instead.

::: libyelp/web-extensions/current-dom-node-extension/Makefile.am
@@ +1,2 @@
+
+webextension_LTLIBRARIES = libyelpdomwebextension.la

The extension will be used for more thing than dom, I would call this simply libyelpwebextension.la

@@ +3,3 @@
+webextensiondir = $(pkglibdir)/web-extensions
+
+libyelpdomwebextension_la_SOURCES = current-dom-node-web-extension.c

yelp-web-extension.c

::: libyelp/web-extensions/current-dom-node-extension/current-dom-node-web-extension.c
@@ +24,3 @@
+
+#include <webkit2/webkit-web-extension.h>
+#include <webkitdom/WebKitDOMHTMLElementUnstable.h>

This is not enough to use the unstable DOM API, you should also define WEBKIT_DOM_USE_UNSTABLE_API before including any unstable header.

@@ +30,3 @@
+
+static GDBusConnection *dbus_connection;
+static WebKitDOMNode *saved_element = NULL;

Don't initialize globals to NULL

@@ +55,3 @@
+"      <arg type='s' name='code' direction='out'/>"
+"    </method>"
+"  </interface>"

I think we should add API to wk to customize the context menu from the web extensions, instead of adding this custom dbus API.

@@ +67,3 @@
+        if (WEBKIT_DOM_IS_ELEMENT (cur) &&
+            g_strcmp0 (webkit_dom_element_get_tag_name ((WebKitDOMElement *) cur), "div") == 0 &&
+            g_strcmp0 (webkit_dom_element_get_class_name ((WebKitDOMElement *) cur), "code") == 0)

This is leaking both the tag and class names, also use WEBKIT_DOM_ELEMENT instead of C casts for all DOM objects.

@@ +80,3 @@
+    for (cur = saved_element; cur != NULL; cur = webkit_dom_node_get_parent_node (cur))
+        if (WEBKIT_DOM_IS_ELEMENT (cur) &&
+            g_strcmp0 (webkit_dom_element_get_tag_name ((WebKitDOMElement *) cur), "a") == 0)

Ditto.

@@ +103,3 @@
+                "There is no link node.");
+
+    child = (WebKitDOMNode *)

WEBKIT_DOM_NODE

@@ +132,3 @@
+
+    g_dbus_method_invocation_return_value (
+                invocation, g_variant_new ("(s)", tmp));

I think you are leaking tmp here too

@@ +148,3 @@
+
+    g_dbus_method_invocation_return_value (
+                invocation, g_variant_new ("(s)", webkit_dom_node_get_text_content (code_node)));

This leaks the text content

@@ +166,3 @@
+    if (title != NULL && WEBKIT_DOM_IS_ELEMENT (title) &&
+        g_strcmp0 (webkit_dom_element_get_tag_name ((WebKitDOMElement *) title), "div") == 0 &&
+        g_strcmp0 (webkit_dom_element_get_class_name ((WebKitDOMElement *) title), "contents") == 0) {

More leaks here.

@@ +171,3 @@
+        if (title != NULL && WEBKIT_DOM_IS_ELEMENT (title) &&
+            g_strcmp0 (webkit_dom_element_get_tag_name ((WebKitDOMElement *) title), "div") == 0 &&
+            g_strcmp0 (webkit_dom_element_get_class_name ((WebKitDOMElement *) title), "title") == 0) {

And here, why don't you use the exactly same code that was used in WebKit1? The DOM API is the same.

@@ +265,3 @@
+
+        dbus_method_code_get_code (invocation);
+    }

Too many empty lines in this if block

::: libyelp/yelp-view.c
@@ +40,3 @@
 #include "yelp-types.h"
 #include "yelp-view.h"
+#include "web-extensions/current-dom-node-extension/current-dom-node-web-extension.h"

You should probably add the web extension dir to the include dir.

@@ +248,3 @@
+
+    GDBusProxy     *current_dom_node_web_extension;
+    gint            current_dom_node_web_extension_watch_name_id;

Simply this, and call it web_extension.

@@ +793,3 @@
 /******************************************************************************/
 
+static gchar*

gchar *

@@ +800,3 @@
+    gchar* result;
+
+    dbus_result = g_dbus_proxy_call_sync (priv->current_dom_node_web_extension,

Never use DBus sync API to communicate with the web extension.

@@ +824,3 @@
+    gchar* result;
+
+    dbus_result = g_dbus_proxy_call_sync (priv->current_dom_node_web_extension,

Ditto.

@@ +848,3 @@
+    gchar* result;
+
+    dbus_result = g_dbus_proxy_call_sync (priv->current_dom_node_web_extension,

Ditto.

@@ +872,3 @@
+    gboolean result;
+
+    dbus_result = g_dbus_proxy_call_sync (priv->current_dom_node_web_extension,

Ditto.

@@ +896,3 @@
+    gboolean result;
+
+    dbus_result = g_dbus_proxy_call_sync (priv->current_dom_node_web_extension,

Ditto.

@@ +921,3 @@
+    arguments = g_variant_new ("(tii)", page_id, x, y);
+
+    g_dbus_proxy_call_sync (priv->current_dom_node_web_extension,

Ditto.

@@ +1542,3 @@
+        if (is_link) {
+            priv->popup_link_text =
+                view_current_dom_node_web_extension_link_get_title (view);

As I said, we should add API to WebKit for this, so that you don't need to use DBus at all, but if you do this using DBus, you should use a single DBus call that returns all the information you need, instead of doing muliple calls here, and always suing the async API.
Comment 54 Carlos Garcia Campos 2014-11-02 08:49:30 UTC
Review of attachment 289740 [details] [review]:

::: libyelp/yelp-view.c
@@ +1796,3 @@
     }
+
+    g_signal_handler_unblock (view, priv->navigation_requested);

Note that this might be unbalanced. This is called for every single main resource that finishes loading, not only error pages. Also, you might want to unblock this earlier, in load-start I would say. But a better way to implement this could be to check in navigation_requested callabck if we are loading an error page and simply return FALSE to continue with the load normally.
Comment 55 Carlos Garcia Campos 2014-11-02 09:46:35 UTC
Review of attachment 289741 [details] [review]:

::: libyelp/yelp-uri-builder.c
@@ +18,3 @@
+ * Boston, MA 02111-1307, USA.
+ *
+ * Author: Marcos Chavarría Teijeiro <chavarria1991@gmail.com>

Really?

@@ +33,3 @@
+
+    canonical = yelp_uri_get_canonical_uri (uri);
+    soup_uri = soup_uri_new (canonical);

You are leaking the canonical uri, in my original code there was g_free here :-)

@@ +75,3 @@
+    }
+
+    bogus_scheme = g_strdup_printf (BOGUS_PREFIX "%s", soup_uri->scheme);

I don't understand this, the idea of using custom uri schemes was precisely to get rid of the bogus trick

@@ +76,3 @@
+
+    bogus_scheme = g_strdup_printf (BOGUS_PREFIX "%s", soup_uri->scheme);
+    soup_uri_set_scheme (soup_uri, bogus_scheme);

You are leaking the bogus_scheme

@@ +85,3 @@
+
+gchar *
+build_yelp_uri    (gchar* uri)

This is a bit weird, I think this should receive a const char and return a new allocated string, or NULL if it's not a known uri. That way you don't always allocate a new string.

@@ +92,3 @@
+  if (!g_str_has_prefix (uri, BOGUS_PREFIX "ghelp:/") &&
+      !g_str_has_prefix (uri, BOGUS_PREFIX "gnome-help:/") &&
+      !g_str_has_prefix (uri, BOGUS_PREFIX "help:/")) {

Same comment here, why do we still need to keep the bogus trick?

@@ +112,3 @@
+
+gchar *
+build_network_scheme (gchar *scheme)

const gchar *scheme

@@ +118,3 @@
+	bogus_scheme = g_strdup_printf ( BOGUS_PREFIX "%s", scheme);
+
+	return bogus_scheme;

you don't need the local variable, you could simply return g_strdup_printf (BOGUS_PREFIX "%s", scheme);

::: libyelp/yelp-view.c
@@ +126,3 @@
+                                                              WebKitLoadEvent           load_event,
+                                                              gchar                    *failing_uri,
+                                                              gpointer                  error,

GError *error, I guess

@@ +760,3 @@
+    data->request = g_object_ref (request);
+    data->page_id = NULL;
+    data->resource_file = NULL;

If you are using g_slice_new0 you don't need to initialize the member to 0 again. so either use g_slice_new() and keep the explicit initialization, or keep g_slice_new0 and remove the explicit initializations.

@@ +770,3 @@
+
+    if (data->page_id)
+      g_free (data->page_id);

Use g_clear_pointer

@@ +773,3 @@
+
+    if (data->resource_file)
+      g_clear_object (&data->resource_file);

Don't check if resource_file is NULL before calling g_clear_object, it's already checked by g_clear_object,

@@ +803,3 @@
+    content_length = strlen (contents);
+
+    stream = g_memory_input_stream_new_from_data (strdup (contents), -1, g_free);

You already know the size, use content_length instead of -1. You are mixing strdup with g_free. Instead of duplicating the contents, use a different destroy notify callback and call yelp_document_finish_read there to clean up the resources.

@@ +811,3 @@
+                                      mime_type);
+    request_async_data_free (data);
+    g_free (mime_type);

You should also unref the stream.

@@ +821,3 @@
+
+
+    document = yelp_document_get_for_uri (uri);

You are leaking the returned document, yelp_document_get_for_uri returns a new reference.

@@ +832,3 @@
+    }
+
+    webkit_uri_scheme_request_finish (data->request, g_memory_input_stream_new (), -1, NULL);

This is leaking the stream, and pointless I would say. I think this should never happen, requests of resources that are not documents should be blocked earlier, either by send-request in the web extension or navigation_request in the UI process.

@@ +834,3 @@
+    webkit_uri_scheme_request_finish (data->request, g_memory_input_stream_new (), -1, NULL);
+
+    request_async_data_free (data);

If you pass a ref to the request, you can create the RequestAsyncData here only if needed.

@@ +847,3 @@
+    data = request_async_data_new (request);
+
+    uri_str = build_yelp_uri (strdup (webkit_uri_scheme_request_get_uri (request)));

You are leaking the uri_str.

@@ +849,3 @@
+    uri_str = build_yelp_uri (strdup (webkit_uri_scheme_request_get_uri (request)));
+
+    uri = yelp_uri_new (uri_str);

And this uri, as well.

::: libyelp/yelp-view.h
@@ +66,3 @@
+struct _RequestAsyncData {
+    WebKitURISchemeRequest *request;
+    GFile *resource_file;

Where's this resource_file used?

@@ +68,3 @@
+    GFile *resource_file;
+    gchar *page_id;
+};

Why does this need to be public?
Comment 56 Carlos Garcia Campos 2014-11-02 10:14:57 UTC
Review of attachment 289742 [details] [review]:

::: Makefile.am
@@ +1,3 @@
 ACLOCAL_AMFLAGS = -I m4 ${ACLOCAL_FLAGS}
 
+SUBDIRS = libyelp libyelp/web-extensions src po data tests docs

Use a single web extension for everything

::: libyelp/web-extensions/change-resources-uri/change_resources_uri.c
@@ +27,3 @@
+#include "../../yelp-uri.h"
+#include "../../yelp-document.h"
+#include "../../yelp-uri-builder.h"

I'm not sure it's a good idea that the web extension links to libyelp, maybe we could split libyelp and move all the document related classes, which is what we need here, to a libyelpdocument library. Although, maybe the only thing we need is YelpURI.

@@ +54,3 @@
+
+        document_uri = yelp_document_get_uri (document);
+        resource_path = yelp_uri_locate_file_uri (document_uri, resource);

I wonder if we really need the document here just to resolve paths, since all the logic seems to be in YelpURI. The document URI is mainly yelp_uri_get_document_uri() for all document types except man that uses yelp_uri_get_canonical_uri.

@@ +84,3 @@
+        return FALSE;
+
+    webkit_uri_request_set_uri (request, file_path);

You are leaking the file_path. Since you are going to return FALSE anyway you could remove previous of and do:

if (file_path) {
        webkit_uri_request_set_uri (request, file_path);
        g_free (file_path);
}

return FALSE;

@@ +101,3 @@
+
+    if (g_strcmp0 (current_uri, uri) == 0)
+        return;

the uri is leaked here.

@@ +119,3 @@
+        document_type == YELP_URI_DOCUMENT_TYPE_ERROR)
+    {
+        current_document = NULL;

Why do we need to check this? yelp_document_get_for_uri will return NULL anyway for those cases.

@@ +134,3 @@
+    g_signal_connect_object (web_page, "send-request",
+                             G_CALLBACK (web_page_send_request),
+                             NULL, 0);

Using g_signal_connect_object and passing NULL as the GObject doesn't make sense to me.

@@ +137,3 @@
+    g_signal_connect_object (web_page, "notify::uri",
+                             G_CALLBACK (web_page_notify_uri),
+                             NULL, 0);

Ditto.
Comment 57 Marcos Chavarria Teijeiro 2014-11-03 08:20:29 UTC
(In reply to comment #36)
> Review of attachment 289726 [details] [review]:
> 
> On second thoughts, does this use any API added in 2.7.1, or is 2.6 sufficient?

Yes, we need 2.7.1 because we use webkit_hit_test_result_context_is_selection().
Comment 58 Marcos Chavarria Teijeiro 2014-11-03 09:41:41 UTC
Created attachment 289878 [details] [review]
yelp: Change headers files to use WebKit2
Comment 59 Marcos Chavarria Teijeiro 2014-11-03 09:45:51 UTC
Created attachment 289879 [details] [review]
yelp-view: Replace "navigation-policy-decision-requested" signal by "decide-policy" signal

WK2 only has a signal for policy decisions.
Comment 60 Marcos Chavarria Teijeiro 2014-11-03 09:47:14 UTC
Created attachment 289880 [details] [review]
yelp-view: Replace "script-alert" signal by "script-dialog" signal
Comment 61 Marcos Chavarria Teijeiro 2014-11-03 10:16:02 UTC
Created attachment 289881 [details] [review]
yelp-view: Rename WKWebSettings to WKSettings and adapt properties

Remove enable-universal-access-from-file-uris that doesn't
exist any more and rename default-encoding to default-charset.
Comment 62 Marcos Chavarria Teijeiro 2014-11-03 10:18:12 UTC
Created attachment 289882 [details] [review]
yelp-view: Implement view_print_action using WebKit2 API
Comment 63 Marcos Chavarria Teijeiro 2014-11-03 10:20:45 UTC
Created attachment 289883 [details] [review]
yelp-view: Use WKFindController instead of deprecated search functions in WKWebView
Comment 64 Marcos Chavarria Teijeiro 2014-11-03 12:01:30 UTC
Created attachment 289890 [details] [review]
yelp-view: Rename WKWebSettings to WKSettings and adapt properties

Remove enable-universal-access-from-file-uris that doesn't
exist any more and rename default-encoding to default-charset.
Comment 65 Marcos Chavarria Teijeiro 2014-11-05 09:59:00 UTC
Created attachment 290008 [details] [review]
yelp-view: Replace "populate-popup" signal by "context-menu" signal

New WK2 API HitTestResutl class doesnt have a reference to the DOM
node so we cant know using this property if we are inside a code block
or the name of a link.
Comment 66 Marcos Chavarria Teijeiro 2014-11-05 10:21:56 UTC
Created attachment 290009 [details] [review]
yelp-view: Implement web extension to deal with DOM tree
Comment 67 Marcos Chavarria Teijeiro 2014-11-05 12:44:54 UTC
Created attachment 290015 [details] [review]
yelp-view: Implement pages load in WebKit2

Substitute webkit_web_view_load_string call for webkit_web_view_load_html for showing error pages.
Implement custom uri schemes for loading the normal pages.

We should deal with the uris that Yelp undestand and the uris that Webkit undestand so we have created functions to convert from one to the others. In addition, we should add a hack to be able to load absolute uris. When we have a help:gnome-help/... uri on our document and the current page has the same scheme (help) WebKit interprets this uri as relative and it builds a different uri. To fix this instead of use a help scheme we use a bogus-help schme so WebKit interprets the uri as absolute.
Comment 68 Marcos Chavarria Teijeiro 2014-11-05 16:44:08 UTC
Created attachment 290038 [details] [review]
yelp-view: Implement pages load in WebKit2

Substitute webkit_web_view_load_string call for webkit_web_view_load_html for showing error pages.
Implement custom uri schemes for loading the normal pages.

We should deal with the uris that Yelp undestand and the uris that Webkit undestand so we have created functions to convert from one to the others. In addition, we should add a hack to be able to load absolute uris. When we have a help:gnome-help/... uri on our document and the current page has the same scheme (help) WebKit interprets this uri as relative and it builds a different uri. To fix this instead of use a help scheme we use a bogus-help schme so WebKit interprets the uri as absolute.
Comment 69 Carlos Garcia Campos 2014-11-06 07:42:47 UTC
Review of attachment 289882 [details] [review]:

::: libyelp/yelp-view.c
@@ +1562,3 @@
+
+    print_operation = webkit_print_operation_new (WEBKIT_WEB_VIEW (view));
+    webkit_print_operation_run_dialog (print_operation, GTK_WINDOW (window));

You are still leaking the print operation, call g_object_unref here
Comment 70 Carlos Garcia Campos 2014-11-06 07:53:33 UTC
Review of attachment 290008 [details] [review]:

You are still leaking the actions, you should unref the action group on view dispose or finalize.

::: libyelp/yelp-view.c
@@ +269,3 @@
                       G_CALLBACK (view_script_dialog), NULL);
 
+    const GtkActionEntry popup_action_entries[] = {

Make this static.

@@ +271,3 @@
+    const GtkActionEntry popup_action_entries[] = {
+      {
+        "yelp-view-popup-copy-code", NULL,

Too large and redundant name for the actions, IMO, also we normally use camel case for actions names, I would use something simpler like "CopyCode"

@@ +324,3 @@
+    priv->popup_actions = gtk_action_group_new ("popup-actions");
+    gtk_action_group_add_actions (priv->popup_actions, popup_action_entries,
+                                  10, view);

Use G_N_ELEMENTS instead of hardcoding the size here, that way you won't have to update this value if you add/remove entries.
Comment 71 Carlos Garcia Campos 2014-11-06 08:31:56 UTC
Review of attachment 290009 [details] [review]:

I'm happy to see the new WebKit API makes things a lot easier with the context menu

::: configure.ac
@@ +45,3 @@
+PKG_CHECK_MODULES(YELP_EXTENSION,
+[
+	webkit2gtk-web-extension-4.0 >= 2.5

2.7.2, since this will depend on new context menu api that hasn't been released yet.

::: libyelp/web-extension/yelp-web-extension.c
@@ +22,3 @@
+#include <webkitdom/webkitdom.h>
+
+static void

This should return a gboolean, and you should return always FALSE because the proposed context menu hasn't been changed, we only add user data.

@@ +36,3 @@
+    for (cur = node; cur != NULL; cur = webkit_dom_node_get_parent_node (cur)) {
+        if (WEBKIT_DOM_IS_ELEMENT (cur) &&
+            webkit_dom_element_webkit_matches_selector (WEBKIT_DOM_ELEMENT (cur),

I wonder how this can build. This is now in the DOM unstable API. To use this method you need to explicitly include <webkitdom/WebKitDOMElementUnstable.h> and define WEBKIT_DOM_USE_UNSTABLE_API before the include.

@@ +48,3 @@
+                                                   "pre.contents", NULL));
+            title = webkit_dom_node_get_parent_node (cur);
+            if (title != NULL && WEBKIT_DOM_IS_ELEMENT (title) &&

This is redundant, WEBKIT_DOM_IS_ELEMENT evaluates to false if the pointer is NULL.

@@ +52,3 @@
+                                                            "div.contents", NULL)) {
+                title = webkit_dom_node_get_previous_sibling (title);
+                if (title != NULL && WEBKIT_DOM_IS_ELEMENT (title) &&

Ditto.

@@ +67,3 @@
+        gboolean ws;
+
+        child = (WebKitDOMNode *)

WEBKIT_DOM_NODE

@@ +97,3 @@
+    }
+
+    user_data_dict = g_variant_dict_new (NULL);

You could probably use a stack allocated GVariantDict to avoid memory allocations.

@@ +99,3 @@
+    user_data_dict = g_variant_dict_new (NULL);
+
+    g_variant_dict_insert (user_data_dict, "is_link", "b", link_node ? TRUE : FALSE);

Do we really need this? I think we can add to the dict the values directly, then in the UI process if the lookup for link-title fails, we know it's not a link.

@@ +102,3 @@
+    if (popup_link_text) {
+        g_variant_dict_insert (user_data_dict, "link_title", "s", popup_link_text);
+        g_free (popup_link_text);

I would use link-title instead of link_title. Also, I think we can avoid data copies by using g_variant_dict_insert_value and g_variant_new_take_string

@@ +105,3 @@
+    }
+
+    g_variant_dict_insert (user_data_dict, "is_code", "b", code_node ? TRUE : FALSE);

Same here, if don't find code entry in the dict, it's because it's not a code.

@@ +109,3 @@
+        gchar *code_code = webkit_dom_node_get_text_content (code_node);
+        g_variant_dict_insert (user_data_dict, "code_code", "s", code_code);
+        g_free (code_code);

why code_code? I would use simply code or code-text. And same comment here about data copies.

@@ +115,3 @@
+        gchar *code_title = webkit_dom_node_get_text_content (code_title_node);
+        g_variant_dict_insert (user_data_dict, "code_title", "s", code_title);
+        g_free (code_title);

And here.

@@ +120,3 @@
+    user_data = g_variant_dict_end (user_data_dict);
+
+    webkit_context_menu_set_user_data (context_menu, user_data);

You can use g_variant_dict_end directly here, without any local variable.

@@ +123,3 @@
+
+    g_variant_dict_unref (user_data_dict);
+}

return FALSE;

::: libyelp/yelp-view.c
@@ +1287,3 @@
     webkit_context_menu_remove_all (context_menu);
 
+    // We extract the info about the dom tree that we build in the web-extension.

Do not use C++ style comments

@@ +1289,3 @@
+    // We extract the info about the dom tree that we build in the web-extension.
+    dom_info_variant = webkit_context_menu_get_user_data (context_menu);
+    dom_info_dict = g_variant_dict_new (dom_info_variant);

You could also use a stack allocated dict here as well. Also note that user_data might be NULL if it's not a link nor code.

@@ +1306,3 @@
+                                   &(priv->popup_link_text));
+
+        if (!priv->popup_link_text)

So, you don't really need the is_link, because this will be NULL anyway if link-title entry is not present in the dict.

@@ +1432,3 @@
+    if (is_code) {
+        g_free (priv->popup_code_title);
+        priv->popup_code_title = NULL;

Use g_clear_pointer

@@ +1437,3 @@
+
+        g_free (priv->popup_code_text);
+        priv->popup_code_text = NULL;

Ditto.
Comment 72 Marcos Chavarria Teijeiro 2014-11-06 09:31:00 UTC
Review of attachment 290009 [details] [review]:

::: libyelp/yelp-view.c
@@ +1289,3 @@
+    // We extract the info about the dom tree that we build in the web-extension.
+    dom_info_variant = webkit_context_menu_get_user_data (context_menu);
+    dom_info_dict = g_variant_dict_new (dom_info_variant);

If the context-menu signal is captured in the extension we always return a user_data, it could be an empty dictionary (if it's not link or code) but it's not NULL.

Anyway if user_data is NULL we create an empty dictionary in the UI so there isn't still any problem with that I think.
Comment 73 Marcos Chavarria Teijeiro 2014-11-06 10:17:50 UTC
Created attachment 290084 [details] [review]
Fix leak.
Comment 74 Marcos Chavarria Teijeiro 2014-11-06 10:28:54 UTC
Created attachment 290086 [details] [review]
yelp-view: Replace "populate-popup" signal by "context-menu" signal

Fix leak, make array static, rename action names and use G_N_ELEMENTS.
Comment 75 Marcos Chavarria Teijeiro 2014-11-06 10:48:15 UTC
Created attachment 290088 [details] [review]
yelp-view: Implement web extension to deal with DOM tree

Change needed wk version, change header file, use stack dict to return results, remove useless dict variables and other minor fixes.
Comment 76 Marcos Chavarria Teijeiro 2014-11-06 11:17:02 UTC
Created attachment 290089 [details] [review]
yelp-view: Replace "populate-popup" signal by "context-menu" signal

Rename an action that I forgot in the last patch.
Comment 77 Marcos Chavarria Teijeiro 2014-11-06 11:47:23 UTC
Created attachment 290095 [details] [review]
yelp-view: Implement web extension to load resources

Remove use of yelp-document, use a single web extension, fix leaks and fix some style problems.
Comment 78 Marcos Chavarria Teijeiro 2014-11-06 11:48:32 UTC
Review of attachment 289732 [details] [review]:

We remove this in a later patch, so this is not needed.
Comment 79 Marcos Chavarria Teijeiro 2014-11-06 11:56:40 UTC
Review of attachment 289726 [details] [review]:

We need 2.7.1 because we use webkit_hit_test_result_context_is_selection() that has been added in 2.7.1
Comment 80 Carlos Garcia Campos 2014-11-06 16:18:09 UTC
(In reply to comment #72)
> Review of attachment 290009 [details] [review]:
> 
> ::: libyelp/yelp-view.c
> @@ +1289,3 @@
> +    // We extract the info about the dom tree that we build in the
> web-extension.
> +    dom_info_variant = webkit_context_menu_get_user_data (context_menu);
> +    dom_info_dict = g_variant_dict_new (dom_info_variant);
> 
> If the context-menu signal is captured in the extension we always return a
> user_data, it could be an empty dictionary (if it's not link or code) but it's
> not NULL.
> 
> Anyway if user_data is NULL we create an empty dictionary in the UI so there
> isn't still any problem with that I think.

If we don't have any user data to pass to the UI process, then simply return FALSE without calling webkit_context_menu_set_user_data().
Comment 81 Carlos Garcia Campos 2014-11-06 16:22:59 UTC
Review of attachment 290088 [details] [review]:

::: libyelp/web-extension/yelp-web-extension.c
@@ +98,3 @@
+    }
+
+    g_variant_dict_init (&user_data_dict, NULL);

Before creating the dict, check if we really need it and return early otherwise.
Comment 82 Carlos Garcia Campos 2014-11-06 17:08:17 UTC
Review of attachment 290095 [details] [review]:

::: libyelp/Makefile.am
@@ -1,1 @@
-SUBDIRS = web-extension

I don't understand this change.

::: libyelp/web-extension/Makefile.am
@@ +5,3 @@
 libyelpwebextension_la_SOURCES = yelp-web-extension.c
+libyelpwebextension_la_CFLAGS = $(YELP_EXTENSION_CFLAGS) \
+								  -I$(top_srcdir)/libyelp

This looks bad indented.

@@ +8,2 @@
 libyelpwebextension_la_LIBADD = $(YELP_EXTENSION_LIBS)
+libyelpwebextension_la_LDFLAGS = -module -avoid-version -no-undefined ../libyelp.la
 No newline at end of file

This is not correct. libyelp.la should be added to LIBADD not to LDFLAGS. And yhis will probably break distcehck, you should use $(top_builddir)/libyelp/libyelp.la to handle the case when distdir != builddir. 
But I still think the extension shouldn't link to libyelp. We have two options here, build yelp-uri twice, or create a small lib for the shared code.

::: libyelp/web-extension/yelp-web-extension.c
@@ +23,3 @@
+#include <stdlib.h>
+#include "../yelp-uri.h"
+#include "../yelp-uri-builder.h"

You shouldn't need to use ../ since you added libyelp dir to the include dirs

@@ +69,3 @@
+    current_uri_canonical = yelp_uri_get_canonical_uri (current_uri);
+
+    if (g_strcmp0 (yelp_uri, current_uri_canonical) != 0) {

I'm not sure I understand this. Looking at the current yelp code again, all paths are located and build based on priv->uri in YelpView. So, I think we could simplify all this by just saving the web page uri as a YelpURI when it changes and resolving it, and using that for everything else, like the ui process seems to do.

@@ +97,3 @@
+        yelp_uri_get_canonical_uri (current_uri) : NULL;
+
+    if (g_strcmp0 (current_uri_canonical, yelp_uri) != 0) {

Why are we checking this here?

@@ +98,3 @@
+
+    if (g_strcmp0 (current_uri_canonical, yelp_uri) != 0) {
+        current_uri = yelp_uri_new (yelp_uri);

This is leaking any previous current_uri every time the uri changes.
Comment 83 Marcos Chavarria Teijeiro 2014-11-07 08:18:33 UTC
Review of attachment 290095 [details] [review]:

::: libyelp/web-extension/yelp-web-extension.c
@@ +69,3 @@
+    current_uri_canonical = yelp_uri_get_canonical_uri (current_uri);
+
+    if (resource && resource[0] != '\0')

We are saving only the current YelpUri in this implementation. This "if" checks if the request uri is different from the saved one. The idea was to avoid to modify the url of the main page but probably this is checked with get_resource_path too. So we may remove this if statement.

@@ +97,3 @@
+        yelp_uri_get_canonical_uri (current_uri) : NULL;
+
+

To check if the uri has changed and not resolving it again if is the same uri.
Comment 84 Marcos Chavarria Teijeiro 2014-11-07 08:25:26 UTC
Created attachment 290135 [details] [review]
yelp-view: Implement web extension to deal with DOM tree

Don't set a empty dictionary as context-menu user data.
Comment 85 Marcos Chavarria Teijeiro 2014-11-07 11:14:07 UTC
Created attachment 290151 [details] [review]
yelp-view: Implement web extension to load resources

I have split the libyelp library into two parts in order to avoid linking the webextension against libyelp. So now we have a libyelpuri with the yelp-uri, yelp-build-uri and their dependecies and the old libyelp with the remaining files. I have modified the yelp-build-uri logic to not have to use yelp-document because it implies to have to add to the new library most of libyelp classes.
Comment 86 Carlos Garcia Campos 2014-11-11 16:02:06 UTC
Review of attachment 290135 [details] [review]:

::: libyelp/yelp-view.c
@@ +1289,3 @@
+    /* We extract the info about the dom tree that we build in the web-extension.*/
+    dom_info_variant = webkit_context_menu_get_user_data (context_menu);
+    g_variant_dict_init (&dom_info_dict, dom_info_variant);

Do not init the GVariantDict when dom_info_variant is NULL.
Comment 87 Carlos Garcia Campos 2014-11-11 16:33:23 UTC
Review of attachment 290151 [details] [review]:

::: configure.ac
@@ +183,3 @@
 docs/libyelp/version.xml
 libyelp/Makefile
+libyelpuri/Makefile

I don't think you need to move the code, you could just create the libyelpuri inside libyelp, it's a private lib after all.

::: libyelp/yelp-docbook-document.c
@@ +35,3 @@
 #include "yelp-storage.h"
 #include "yelp-transform.h"
+#include <libyelpuri/yelp-debug.h>

You shouldn't need to change the includes, since it's a private lib, add the needed -I when building and keep using include "foo.h"

::: libyelpuri/Makefile.am
@@ +5,3 @@
+	yelp-uri-builder.c          \
+	yelp-settings.c             \
+	yelp-debug.c

hmm, it's a bit weird that a lib called libyelpuri includes also the debug and settings, if we really need those, I would use a more generic name like libnyelpshared or libyelpcommon. But I don't think building settings is going to work anyway. The only setting used by yelp-uri is the editor-mode. The settings object created in the web extension will always have the default value, and changes made to settings in the UI process will not have any effect in the web process (except for gsettings, but not for editor-mode in particular). Regarding debug, I see it's included in yelp-uri, but I don't see where it's used, so maybe it's not even needed.
Comment 88 Marcos Chavarria Teijeiro 2014-11-12 09:33:25 UTC
Created attachment 290481 [details] [review]
yelp-view: Implement web extension to load resources

Do not move new library files to a new directory. Remove yelp-debug dependency and rename the library to libyelpcommon.
Comment 89 Marcos Chavarria Teijeiro 2014-11-12 09:42:18 UTC
Created attachment 290482 [details] [review]
yelp-view: Implement web extension to deal with DOM tree

Do not initialize dict if user data is NULL.
Comment 90 Marcos Chavarria Teijeiro 2014-11-19 09:44:24 UTC
Created attachment 290968 [details] [review]
yelp-window: Remove ScrolledWindow to hold WebView.

WebKit2 WebView has its own scrollbars.
Comment 91 Carlos Garcia Campos 2015-01-29 09:24:00 UTC
Shaun, any chance this could be reviewed before the freeze?
Comment 92 Marcos Chavarria Teijeiro 2015-01-29 10:26:06 UTC
He (Shaun) told me a couple of days ago that he could not apply the patches so if I could provide him a branch with them applied like the one I had in GitHub (that was outdated). I couldn't do that until now couse I was formatting and reinstalling all soft in my laptop. But now the github repo is updated (https://github.com/chavaone/yelp/commits/wip/webkit2-port) :)
Comment 93 David King 2015-02-14 10:42:51 UTC
I rebased Marcos' branch against current master, as there had been many changes due to the recent warning fixes. I pushed the rebased branch to git.gnome.org:

https://git.gnome.org/browse/yelp/log/?h=wip/webkit2-port

I have yet to build a recent version of webkit-gtk, which is broken with JavaScript at the moment in Rawhide, but should be able to look again at this over the weekend.
Comment 94 David King 2015-02-14 17:07:44 UTC
I just tested this, after a fixed webkit-gtk landed in Rawhide.

The font size of the content seems to be much too big, so maybe some custom CSS is not being correctly injected or applied. Also, it seems that a handler is unblocked when it is not already blocked:

(yelp:17959): GLib-GObject-WARNING **: gsignal.c:2530: handler '185' of instance '0xa00470' is not blocked

I set G_DEBUG=fatal-warnings and found that this was at the start of view_history_action() (I triggered it when navigating to a page and then going forward or back).
Comment 95 Carlos Garcia Campos 2015-02-15 09:16:07 UTC
(In reply to David King from comment #94)
> I just tested this, after a fixed webkit-gtk landed in Rawhide.

Thanks!

> The font size of the content seems to be much too big, so maybe some custom
> CSS is not being correctly injected or applied.

Ah, no. This is because the fonts sizes in WebKit2 are in pixels, and not in points. See what devhelp does to convert points to pixels:

https://git.gnome.org/browse/devhelp/tree/src/dh-util.c?id=3.14.0#n241

We could copy paste that.

> Also, it seems that a
> handler is unblocked when it is not already blocked:
> 
> (yelp:17959): GLib-GObject-WARNING **: gsignal.c:2530: handler '185' of
> instance '0xa00470' is not blocked
> 
> I set G_DEBUG=fatal-warnings and found that this was at the start of
> view_history_action() (I triggered it when navigating to a page and then
> going forward or back).

Never liked that block/unlock approach, see comment #54 :-)
Comment 96 David King 2015-02-16 11:57:31 UTC
I have applied a few fixes on top of Marcos' branch, and pushed them to:

https://git.gnome.org/browse/yelp/log/?h=wip/amigadave/webkit2-port

Thanks for the hint about font sizes; I copied the Devhelp technique over. Remaining problems are the signal handler blocking, and that viewing the special "All Help" page (window menu→"All Help") leads to a page load error.

I will try to get those fixed for later today, unless anyone beats me to it.
Comment 97 David King 2015-02-16 23:21:31 UTC
I wasn't able to fix the problems that I mentioned in comment #96, so this will have to be postponed to 3.17, unfortunately.
Comment 98 Carlos Garcia Campos 2015-02-17 13:56:30 UTC
(In reply to David King from comment #97)
> I wasn't able to fix the problems that I mentioned in comment #96, so this
> will have to be postponed to 3.17, unfortunately.

I'll try to help when I have some time. Anyway, I think it's better to land this at the very beginning of the release cycle so that we have the whole cycle to fix regressions, than right before the freeze, so I agree with postponing it. Thanks for your help.
Comment 99 Michael Catanzaro 2015-02-26 15:42:02 UTC
(In reply to Carlos Garcia Campos from comment #95)
> Ah, no. This is because the fonts sizes in WebKit2 are in pixels, and not in
> points. See what devhelp does to convert points to pixels:
> 
> https://git.gnome.org/browse/devhelp/tree/src/dh-util.c?id=3.14.0#n241
> 
> We could copy paste that.

We have that in ephy too, and the code is about to diverge since we have a patch to make it use the GtkSetting gtk-xft-dpi instead. Why do we make each application copypaste this same code? We should deprecate the default-font-size setting and add a new setting default-font-size-points instead.
Comment 100 Michael Catanzaro 2015-02-26 20:46:30 UTC
(In reply to Michael Catanzaro from comment #99)
> We have that in ephy too, and the code is about to diverge since we have a
> patch to make it use the GtkSetting gtk-xft-dpi instead. 

And that aside, the code is still wrong: see bug #680659
Comment 101 Carlos Garcia Campos 2015-04-10 10:52:03 UTC
I've just pushed a new branch https://git.gnome.org/browse/yelp/log/?h=carlosgc/webkit2-port that fixes all the problems I found on top of David's branch. 
There's a problem with that branch, though. A clean build doesn't work, I had to revert the last commit and build to get the marshallers generated (then fails to link), and then apply it again and build to fix the link issues.
Comment 102 Rafał Mużyło 2015-06-09 08:55:33 UTC
At this time, two questions about that branch:

1. were the seemingly less used by Gnome people parts tested ? 'info:' addresses don't seem to work (the names supposed to be used should be the same as in terminal 'info', right ?)

2. loading timeout seems wrong -that is to say that the busy cursor is displayed for quite awhile after the page seems fully loaded and sometimes while going forward/back the page isn't getting displayed, but there's no busy cursor

Also, somewhat, but not fully independent of this patch, the error page seems to need encoding meta tag - utf8 error messages seem to be interpreted as latin1.
Comment 103 Carlos Garcia Campos 2015-06-13 08:24:30 UTC
(In reply to Rafał Mużyło from comment #102)
> At this time, two questions about that branch:

Thanks for trying it out!

> 1. were the seemingly less used by Gnome people parts tested ? 'info:'
> addresses don't seem to work (the names supposed to be used should be the
> same as in terminal 'info', right ?)

I don't remember, how could I test that?

> 2. loading timeout seems wrong -that is to say that the busy cursor is
> displayed for quite awhile after the page seems fully loaded and sometimes
> while going forward/back the page isn't getting displayed, but there's no
> busy cursor

I didn't notice that, but I think it's normal that there are some problems when a large change like this happens. That's why I think it's important to push this as early as possible in the release cycle, and then you have the rest of the cycle to fix bugs and try to ensure there are no regressions when the new stable release is out.

> Also, somewhat, but not fully independent of this patch, the error page
> seems to need encoding meta tag - utf8 error messages seem to be interpreted
> as latin1.

How can I force an error page?
Comment 104 Rafał Mużyło 2015-06-13 09:55:58 UTC
(In reply to Carlos Garcia Campos from comment #103)
> (In reply to Rafał Mużyło from comment #102)
> > 1. were the seemingly less used by Gnome people parts tested ? 'info:'
> > addresses don't seem to work (the names supposed to be used should be the
> > same as in terminal 'info', right ?)
> 
> I don't remember, how could I test that?
> 

Well, as long as you've got some info pages installed, the syntax is shown in libyelp/yelp-uri.c in 'static void resolve_info_uri(YelpUri *uri)'.

> > Also, somewhat, but not fully independent of this patch, the error page
> > seems to need encoding meta tag - utf8 error messages seem to be interpreted
> > as latin1.
> 
> How can I force an error page?

Well, there are many ways, but the problem is actually getting the right error page, as most look fine.
So, while 'yelp info:assuan' just displays 'Unknown Error.', 'yelp info:/usr/share/info/assuan.info.bz2' displays (for me) a localized error message, that's utf-8 encoded, but webkit (as per html4 default) displays it as latin1 (or at least that's how I interpret the output on screen). That's probably cause with these changes yelp expects 'yelp info:file:///usr/share/info/assuan.info.bz2' (which isn't working either; on the other hand, maybe not, as the error displayed for such address shows it's getting transformed to 'man:' address), still that's no reason for mojibake.
Comment 105 Michael Catanzaro 2015-06-13 14:46:17 UTC
(In reply to Carlos Garcia Campos from comment #103)
> > 1. were the seemingly less used by Gnome people parts tested ? 'info:'
> > addresses don't seem to work (the names supposed to be used should be the
> > same as in terminal 'info', right ?)
> 
> I don't remember, how could I test that?

Run from the command line:

$ yelp info:gcc

Also:

$ yelp man:gcc
 
> > Also, somewhat, but not fully independent of this patch, the error page
> > seems to need encoding meta tag - utf8 error messages seem to be interpreted
> > as latin1.
> 
> How can I force an error page?

$ LANG=he_IL.utf8 yelp info:/usr/share/info/gcc.info.bz2
Comment 106 Michael Catanzaro 2015-06-13 18:19:21 UTC
> > 2. loading timeout seems wrong -that is to say that the busy cursor is
> > displayed for quite awhile after the page seems fully loaded and sometimes
> > while going forward/back the page isn't getting displayed, but there's no
> > busy cursor

I haven't noticed any of this; it seems normal to me.

> Well, there are many ways, but the problem is actually getting the right
> error page, as most look fine.
> So, while 'yelp info:assuan' just displays 'Unknown Error.', 'yelp
> info:/usr/share/info/assuan.info.bz2' displays (for me) a localized error
> message, that's utf-8 encoded, but webkit (as per html4 default) displays it
> as latin1 (or at least that's how I interpret the output on screen). 

When I run 'jhbuild run yelp info:/usr/share/info/assuan.info.bz2' I get English, but if I run 'LANG=pl_PL.utf8 jhbuild run yelp info:/usr/share/info/assuan.info.bz2' then I get real UTF-8 characters, so I haven't seen any problem. Exactly what locale are you using: what do you get when you 'echo $LANG'?

Regardless, we could easily change the default character set to UTF-8. latin1 is the right default for displaying web pages, not files on your Linux computer.
Comment 107 Michael Catanzaro 2015-06-13 18:21:05 UTC
Created attachment 305216 [details]
Debug build backtrace of UI process crash when loading man page

Loading a man page or an info page causes yelp to crash for me (with a debug build).
Comment 108 Rafał Mużyło 2015-06-13 20:12:07 UTC
LANG=pl_PL.utf8

The message in en_US.utf8 is "URL cannot be shown".

Funny thing - if yelp isn't already running, that's all that's displayed, if - however - it's running in the background, it's displayed within yellow 'Unknown Error' div (or whatever that page element is). That's somewhat inconsistent - might be related to those cursor problems.
Comment 109 Michael Catanzaro 2015-06-13 23:16:31 UTC
Created attachment 305219 [details]
screenshot when running 'LANG=pl_PL.utf8 jhbuild run yelp info:/usr/share/info/assuan.info.bz2'

(In reply to Rafał Mużyło from comment #108)
> LANG=pl_PL.utf8
> 
> The message in en_US.utf8 is "URL cannot be shown".
> 
> Funny thing - if yelp isn't already running, that's all that's displayed, if
> - however - it's running in the background, it's displayed within yellow
> 'Unknown Error' div (or whatever that page element is). That's somewhat
> inconsistent - might be related to those cursor problems.

Hm, I can't reproduce this. I'm attaching a screenshot of what I see when I run 'LANG=pl_PL.utf8 jhbuild run yelp info:/usr/share/info/assuan.info.bz2' regardless of whether yelp is running in the background or not. The default character set is already set to UTF-8 in yelp_view_class_init().

Note that I'm using a newer build of WebKit than is in jhbuild, and it's possible that could be related, but it's not very likely.
Comment 110 Rafał Mużyło 2015-06-13 23:58:49 UTC
Created attachment 305220 [details]
my counterscrenshot

Screenshots are weak arguments, but...
Comment 111 Rafał Mużyło 2015-06-14 00:19:19 UTC
...though a thought: does /usr/share/info/assuan.info.bz2 actually exist on your machine ?
Comment 112 Michael Catanzaro 2015-06-14 01:09:42 UTC
(In reply to Rafał Mużyło from comment #111)
> ...though a thought: does /usr/share/info/assuan.info.bz2 actually exist on
> your machine ?

Nope, I assumed you were intentionally giving a file that doesn't exist to create an error message. I have no clue what assuan is (still don't after searching the web :) and all my info files are .gz.

Trying again:

$ LANG=pl_PL.utf8 jhbuild run yelp info:/usr/share/info/gcc.info.gz

Causes the same error as in the screenshot you posted above. OK, so we got to the same screenshot, that's a good step. So we need to figure out why the wrong encoding is being used there.

Next weird thing: that error only occurs in the Polish locale. If I remove LANG=pl_PL.utf8 from the command, then Yelp crashes with the same backtrace that I posted above. (You won't be able to reproduce that crash since you're surely using a release build of WebKit.)
Comment 113 Rafał Mużyło 2015-06-14 09:14:06 UTC
...well, you know, libassuan, one of the mandatory dependencies of gnupg and gpgme...

As for the other thing, building webkit-gtk with debugging symbols is at best a "No, thank you" proposition (but more often than not, a stronger reply is required).


Though the way you describe it, perhaps one of the objects is getting destroyed too early or not getting reffed enough times ?
Comment 114 Carlos Garcia Campos 2015-06-15 16:03:37 UTC
There are several things broken with info and man. First of all, I don't know why (I don't remember) we handle the URIs differently when they are ghelp/gnome-help or help/help-list. This means that we register bogus-info and bogus-man schemes, but we don't convert info and man uris to the "bogus" form. So, we would need to consider those in build_network_uri and build_yelp_uri. But that's not enough, there's a bug in WebKit that prevents uris with fragments from working when using custom uri schemes, so info:foo works, but info:foo#bar, loads info:foo. See https://bugs.webkit.org/show_bug.cgi?id=145969
Comment 115 Carlos Garcia Campos 2015-06-15 17:47:45 UTC
(In reply to Carlos Garcia Campos from comment #114)
> There are several things broken with info and man. First of all, I don't
> know why (I don't remember) we handle the URIs differently when they are
> ghelp/gnome-help or help/help-list. This means that we register bogus-info
> and bogus-man schemes, but we don't convert info and man uris to the "bogus"
> form. So, we would need to consider those in build_network_uri and
> build_yelp_uri. But that's not enough, there's a bug in WebKit that prevents
> uris with fragments from working when using custom uri schemes, so info:foo
> works, but info:foo#bar, loads info:foo. See
> https://bugs.webkit.org/show_bug.cgi?id=145969

This is still not enough, since when navigating from info:foo to info:foo#bar, the page is not expected to be loaded. The problem is that we are using fragments to handle links to different pages. We would need to handle that differently somehow.
Comment 116 Carlos Garcia Campos 2015-06-15 18:24:36 UTC
Created attachment 305334 [details] [review]
Handle info uris

This should fix info. I haven't tested man yet, but I would like to know if someone is going to review these patches, or if there's any interest to fix this bug for 3.18, because otherwise I won't waste more time on this.
Comment 117 Rafał Mużyło 2015-06-15 20:27:42 UTC
Created attachment 305341 [details]
gdb backtrace

Well, the offsets will be somewhat off, but this was G_DEBUG=fatal-warnings gdb output (done because 'g_str_has_prefix: assertion 'str != NULL' failed').

That's for 'yelp info:assuan' case, which displays blank site.

'yelp info:/usr/share/info/assuan.info.bz2' is unchanged (and doesn't display the assertion).
Comment 118 Michael Catanzaro 2015-06-15 23:51:34 UTC
(In reply to Carlos Garcia Campos from comment #116)
> Created attachment 305334 [details] [review] [review]
> Handle info uris
> 
> This should fix info. I haven't tested man yet, but I would like to know if
> someone is going to review these patches, or if there's any interest to fix
> this bug for 3.18, because otherwise I won't waste more time on this.

Pretty sure David will review if you ask him; he might not be reading every comment here.
Comment 119 Carlos Garcia Campos 2015-06-16 06:38:46 UTC
(In reply to Rafał Mużyło from comment #117)
> Created attachment 305341 [details]
> gdb backtrace
> 
> Well, the offsets will be somewhat off, but this was G_DEBUG=fatal-warnings
> gdb output (done because 'g_str_has_prefix: assertion 'str != NULL' failed').
> 
> That's for 'yelp info:assuan' case, which displays blank site.
> 
> 'yelp info:/usr/share/info/assuan.info.bz2' is unchanged (and doesn't
> display the assertion).

I think there's a point in which page_id, that can be NULL, is not checked in the info implementation.
Comment 120 Carlos Garcia Campos 2015-06-16 10:51:19 UTC
Ok, I have just pushed some more patches to my branch to fix info and man documents. Fixed also the runtime critical, and the loading cursor inconsistencies. Encoding of error pages is correct in Spanish locale, so I couldn't reproduce any issue there.
Comment 121 Rafał Mużyło 2015-06-16 13:19:17 UTC
(In reply to Carlos Garcia Campos from comment #120)
> Ok, I have just pushed some more patches to my branch to fix info and man
> documents. Fixed also the runtime critical, and the loading cursor
> inconsistencies. 

The cursor inconsistencies are still there...

> Encoding of error pages is correct in Spanish locale, so I
> couldn't reproduce any issue there.

If the relevant message in your case seems to have been "No se pueden mostrar el URL"...well, no surprise that it was correctly encoded...

Now the error I sometimes get is "Load request cancelled"...which will also be `correctly` encoded in the Spanish locale (and isn't in pl_PL.utf8).

I'm saying sometimes, as it takes entering for example 'info:assuan' or 'info gcc' into location bar. If just ran as 'yelp info:assuan' or 'yelp info:/usr/share/info/assuan.info.bz2', the result is quite consistent...it's a blank page.

I'm becoming more convinced, that that cursor problem is just a symptom of the request not getting processed at the right stage.
Comment 122 Carlos Garcia Campos 2015-06-16 13:37:09 UTC
(In reply to Rafał Mużyło from comment #121)
> (In reply to Carlos Garcia Campos from comment #120)
> > Ok, I have just pushed some more patches to my branch to fix info and man
> > documents. Fixed also the runtime critical, and the loading cursor
> > inconsistencies. 
> 
> The cursor inconsistencies are still there...

Then, I guess something keeps loading forever, or something like that.

> > Encoding of error pages is correct in Spanish locale, so I
> > couldn't reproduce any issue there.
> 
> If the relevant message in your case seems to have been "No se pueden
> mostrar el URL"...well, no surprise that it was correctly encoded...

No se encontró el documento
El URI «info:gcc» no apunta a una página válida.

for example.

> Now the error I sometimes get is "Load request cancelled"...which will also
> be `correctly` encoded in the Spanish locale (and isn't in pl_PL.utf8).

Weird, why is the load cancelled?

> I'm saying sometimes, as it takes entering for example 'info:assuan' or
> 'info gcc' into location bar. If just ran as 'yelp info:assuan' or 'yelp
> info:/usr/share/info/assuan.info.bz2', the result is quite consistent...it's
> a blank page.

It works for me.

> I'm becoming more convinced, that that cursor problem is just a symptom of
> the request not getting processed at the right stage.

Are you running make install? You might be using an old installed version of the web extension with a newer yelp. I can't reproduce any of the issues.
Comment 123 Rafał Mużyło 2015-06-16 15:40:33 UTC
(In reply to Carlos Garcia Campos from comment #122)
> (In reply to Rafał Mużyło from comment #121)
> > (In reply to Carlos Garcia Campos from comment #120)
> > > Ok, I have just pushed some more patches to my branch to fix info and man
> > > documents. Fixed also the runtime critical, and the loading cursor
> > > inconsistencies. 
> > 
> > The cursor inconsistencies are still there...
> 
> Then, I guess something keeps loading forever, or something like that.
> 

Well '"Load request cancelled"' might suggest something like that...

A funny thing - when I get that blank page while cursor is not busy, CPU usage is quite significant till yelp is closed.

> > > Encoding of error pages is correct in Spanish locale, so I
> > > couldn't reproduce any issue there.
> > 
> > If the relevant message in your case seems to have been "No se pueden
> > mostrar el URL"...well, no surprise that it was correctly encoded...
> 
> No se encontró el documento
> El URI «info:gcc» no apunta a una página válida.
> 
> for example.
> 

That's the wrong error.
That's for a page that doesn't exist - that one is translated in yelp.

Both '"Load request cancelled"' and 'URL cannot be shown' are passed via GError from webkit.

> > Now the error I sometimes get is "Load request cancelled"...which will also
> > be `correctly` encoded in the Spanish locale (and isn't in pl_PL.utf8).
> 
> Weird, why is the load cancelled?
> 

Well, I need to explain more how exactly I get it.
When I get that blank page (at that time cursor is not busy), I press Ctrl-L to get the location bar, which shows the expected, correct value. Then I press Enter and I get the error. Sometimes afterwards cursor stays busy.

> > I'm becoming more convinced, that that cursor problem is just a symptom of
> > the request not getting processed at the right stage.
> 
> Are you running make install? You might be using an old installed version of
> the web extension with a newer yelp. I can't reproduce any of the issues.

I'm running it clean - no other copy is present on the system - at least not within $PATH.
Comment 124 Carlos Garcia Campos 2015-06-17 15:24:36 UTC
I've noticed another regression, the scroll position is not remembered when navigating back/forward. For some reason yelp has its own back-forward list implementation. I've just pushed a patch to my branch to use the WebKit back-forward list, that fixes the regression and simplifies the code a lot.
Comment 125 David King 2015-06-22 14:01:56 UTC
Thanks for all the patches Carlos! They look fine to me. I rebased your branch on top of master and pushed to wip/amigadave/webkit2-port. Can you do a quick check to see that the rebase is OK (specifically the xft DPI bits)?
Comment 126 David King 2015-06-22 15:29:04 UTC
Hmm, actually the info support doesn't work. If I run "yelp info:gcc", then hit Ctrl+L, the URI is "man:gcc." (and a the GCC man page is shown). This seems to happen for all info URIs (might be a problem in resolve_info_uri() in libyelp/yelp-uri.c).
Comment 127 Rafał Mużyło 2015-06-22 20:32:12 UTC
(In reply to David King from comment #126)
> Hmm, actually the info support doesn't work. If I run "yelp info:gcc", then
> hit Ctrl+L, the URI is "man:gcc." (and a the GCC man page is shown). This
> seems to happen for all info URIs (might be a problem in resolve_info_uri()
> in libyelp/yelp-uri.c).

...you know, this might actually be in line with my findings...

That's cause I've got legacy man, not man-db installed and this one just doesn't work with yelp. So that blank page might just be yelp trying and failing to use man fallback for info pages.
Comment 128 Rafał Mużyło 2015-06-22 20:35:09 UTC
One more thing: wrt. those messages with encoding not set: does webkit call 'setlocale(LC_ALL, NULL)' for plugins it launches ?

Just an idea.
Comment 129 Carlos Garcia Campos 2015-06-23 07:04:46 UTC
(In reply to David King from comment #126)
> Hmm, actually the info support doesn't work. If I run "yelp info:gcc", then
> hit Ctrl+L, the URI is "man:gcc." (and a the GCC man page is shown). This
> seems to happen for all info URIs (might be a problem in resolve_info_uri()
> in libyelp/yelp-uri.c).

This is because the info page was not found and it's falling back to man. This also happens to me, since I don't have info documents in my jhbuild prefix, but it works when using INFOPATH envvar, for example:

$ INFOPATH=/usr/share/info jhbuild run yelp info:coreutils

I'll check the code, but I think this is not because of WebKit2 port, but the existing behaviour
Comment 130 Carlos Garcia Campos 2015-06-23 07:13:21 UTC
(In reply to Carlos Garcia Campos from comment #129)
> (In reply to David King from comment #126)
> > Hmm, actually the info support doesn't work. If I run "yelp info:gcc", then
> > hit Ctrl+L, the URI is "man:gcc." (and a the GCC man page is shown). This
> > seems to happen for all info URIs (might be a problem in resolve_info_uri()
> > in libyelp/yelp-uri.c).
> 
> This is because the info page was not found and it's falling back to man.
> This also happens to me, since I don't have info documents in my jhbuild
> prefix, but it works when using INFOPATH envvar, for example:
> 
> $ INFOPATH=/usr/share/info jhbuild run yelp info:coreutils
> 
> I'll check the code, but I think this is not because of WebKit2 port, but
> the existing behaviour

Yes, this is because jhbuild sets INFOPATH to your prefix. 

https://git.gnome.org/browse/jhbuild/commit/?id=1972ff2a405444dfe61049539185fbdcb6e216f9

And the env var always takes precedence over the default paths. I'm pretty sure this happens with current master, we haven't changed anything there.
Comment 131 David King 2015-06-23 08:30:29 UTC
(In reply to Carlos Garcia Campos from comment #130)
> And the env var always takes precedence over the default paths. I'm pretty
> sure this happens with current master, we haven't changed anything there.

Yep, tested and confirmed. Looks like the branch is ready to be merged then, and we can deal with remaining problems before 3.18. I will try to get this into a 3.17.3 release today. Thanks for all the patches, reviews and patience.
Comment 132 Carlos Garcia Campos 2015-06-23 08:36:31 UTC
(In reply to David King from comment #131)
> (In reply to Carlos Garcia Campos from comment #130)
> > And the env var always takes precedence over the default paths. I'm pretty
> > sure this happens with current master, we haven't changed anything there.
> 
> Yep, tested and confirmed. Looks like the branch is ready to be merged then,
> and we can deal with remaining problems before 3.18. I will try to get this
> into a 3.17.3 release today. Thanks for all the patches, reviews and
> patience.

Cool, feel free to add me to the CC of any new bug, I'm sure there will be more regressions, but we have time to fix them before 3.18. Thanks!
Comment 133 David King 2015-06-23 09:22:28 UTC
Now merged to master and included in the 3.17.3 release.