GNOME Bugzilla – Bug 722194
Find is disabled in some regular pages
Last modified: 2014-01-16 10:51:41 UTC
For example in the following page "Find" is disabled: http://www.w3.org/TR/shadow-dom/ The problem is that after the HTML page is loading 4 SVG images are loaded and Epiphany thinks it's an image and disables "Find" command. The problem is in EphyWebView::decide_policy_cb() as the notification of the document-type is done before checking if it's or not the main resource.
Created attachment 266274 [details] [review] Patch
Review of attachment 266274 [details] [review]: It took me a while to understand the patch, so I don't think this is very clear. I think you should explicitly do all the type checking only when the main resource is being handled. I don't see why the big if-else clause with document types is necessary for resources other than the main one.
(In reply to comment #2) > Review of attachment 266274 [details] [review]: > > It took me a while to understand the patch, so I don't think this is very > clear. I think you should explicitly do all the type checking only when the > main resource is being handled. I don't see why the big if-else clause with > document types is necessary for resources other than the main one. The patch is quite weird, but what I really did was pretty simple. I just moved the big if-else after: if (g_strcmp0 (webkit_web_resource_get_uri (main_resource), request_uri) != 0) return FALSE; I understand that that's the place where it's checked if it's the main resource, otherwise it returns. So if I'm not wrong the big if-else will only be used in the case of the main resource. The problem is that in the generated patch, it seems that I moved the other part above the big if-else. That's what it's weird to understand I guess.
I understand what you did. I am saying as it is written now (or after your patch) it is unclear that the document type needs only to be checked for the main resurce. Either rewriting the method in a clearer way or just adding a comment explaining this before the resource URI comparison would help.
Review of attachment 266274 [details] [review]: Also the patch one-line summary is not explaining what was fixed but how. You should write there what the bug fixed is, not what you did.
Created attachment 266435 [details] [review] Patch New patch addressing review comments.
Review of attachment 266435 [details] [review]: Better.
Review of attachment 266435 [details] [review]: Thanks for the review! Committed now: https://git.gnome.org/browse/epiphany/commit/?id=c6e2cc5f55a637c2e9b254e763b2779a01d6dea4