Summary: | [EFL][DRT] EFL's DRT needs to support LayoutTestController.dumpIconChanges() | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||
Component: | WebKit EFL | Assignee: | Chris Dumez <cdumez> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | gyuyoung, gyuyoung.kim, gyuyoung.kim, lucas.de.marchi, rakuco, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Chris Dumez
2012-04-20 04:51:08 PDT
Created attachment 138071 [details]
Patch
Comment on attachment 138071 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138071&action=review Almost looks good to me except for trivial comments. > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:602 > + /* report received favicon only for main frame. */ Though this is trivial nits, it looks WebKit coding style tends to use '//' instead of '/*..*/' basically. > Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:110 > + evas_object_smart_callback_add(mainFrame, "icon,changed", onFrameIconChanged, 0); Though I'm not sure this is correct in this case, is it better to register this callback by alphabetical order ? Created attachment 138291 [details]
Patch
Updated patch to take feedback into consideration.
Comment on attachment 138291 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138291&action=review > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:600 > void FrameLoaderClientEfl::dispatchDidChangeIcons(WebCore::IconType) Shouldn't the parameter for this method be used now so the signal can be more useful? Right now it looks like only DRT has a good use for this feature. > Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:604 > + // report received favicon only for main frame. > + if (ewk_view_frame_main_get(m_view) != m_frame) > + return; What's the reason for emitting this signal only for the main frame? Created attachment 138495 [details]
Patch
Update patch to pass the icon type with the "icon,changed" signal. Note that this signal is emitted only on the main frame for consistency.
Indeed, the "icon,received" signal was already existing and is only emitted on the main frame.
Comment on attachment 138291 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138291&action=review >> Source/WebKit/efl/WebCoreSupport/FrameLoaderClientEfl.cpp:604 >> + return; > > What's the reason for emitting this signal only for the main frame? If icon means favicon for main frame, it seems to me that this is reasonable. Basically, main frame only has favicon. Comment on attachment 138495 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=138495&action=review > Source/WebKit/efl/ewk/ewk_view.cpp:2914 > + evas_object_smart_callback_call(frame, "icon,changed", &iconType); Don't you need to change from WebCore::IconType to Ewk_Icon_Type ? Created attachment 138965 [details]
Patch
Take feedback into consideration.
(In reply to comment #6) > If icon means favicon for main frame, it seems to me that this is reasonable. Basically, main frame only has favicon. My point is that from the method name there's no mention that this is reported only for the main frame, so I'd like to understand the reason for adding a restriction in our layer. If you check GTK port for example: void FrameLoaderClient::dispatchDidReceiveIcon() { ASSERT(m_frame == webkit_web_view_get_main_frame(webView)); ... } But the EFL port is doing: void FrameLoaderClientEfl::dispatchDidChangeIcons(WebCore::IconType iconType) { // report received favicon only for main frame. if (ewk_view_frame_main_get(m_view) != m_frame) return; ... } So I believe that even if we removed the main_frame check in the EFL port, we would still send the "icon,received" signal for the main frame only. Now, I don't know if dispatchDidChangeIcons() is called for the main frame only or not but does it really make sense to emit the "icon,changed" signal potentially for all frame when we know for sure that we send the "icon,received" signal only for the main frame? Any update on this? (In reply to comment #10) > So I believe that even if we removed the main_frame check in the EFL port, we would still send the "icon,received" signal for the main frame only. > > Now, I don't know if dispatchDidChangeIcons() is called for the main frame only or not but does it really make sense to emit the "icon,changed" signal potentially for all frame when we know for sure that we send the "icon,received" signal only for the main frame? Nope, we should either send both signals in all cases or only for the main frame. It'd be nice if you could check whether the WebCore machinery that ends up in our FrameLoaderClient also makes this distinction between main frames and other frames or not -- from an API user point of view, it makes sense to provide as much information as possible (ie. if all frames have favicons, they should all be available for the user to query). (In reply to comment #12) > (In reply to comment #10) > > So I believe that even if we removed the main_frame check in the EFL port, we would still send the "icon,received" signal for the main frame only. > > > > Now, I don't know if dispatchDidChangeIcons() is called for the main frame only or not but does it really make sense to emit the "icon,changed" signal potentially for all frame when we know for sure that we send the "icon,received" signal only for the main frame? > > Nope, we should either send both signals in all cases or only for the main frame. It'd be nice if you could check whether the WebCore machinery that ends up in our FrameLoaderClient also makes this distinction between main frames and other frames or not -- from an API user point of view, it makes sense to provide as much information as possible (ie. if all frames have favicons, they should all be available for the user to query). AFAIK, favicon is added in <head>...</head> section. http://en.wikipedia.org/wiki/Favicon#cite_note-7 So, it seems to me that a main frame is only able to have favicon. However, I take a look other port implementations to check how to process this signal in other ports. It looks that is implemented differently by each port. Some ports send a signal to all frames, gtk port sends a signal for main frame. If all frames can have favicon, I also think we should send signal for all frames. If not so, in my opinion, we can send icon,changed for only main frame. Created attachment 140487 [details]
Patch
Now emitting the "icon,changed" signal on all frames, not just the main one. This is consistent with what the Chromium port is doing.
I also converted the main frame runtime check in FrameLoaderClientEfl::dispatchDidReceiveIcon() into an assertion since IconController loads icons only for the main frame (consistent with other ports such as Mac and GTK).
Comment on attachment 140487 [details]
Patch
Looks good overall, but please add some documentation to Ewk_Icon_Type.
Comment on attachment 140487 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140487&action=review > Source/WebKit/efl/ChangeLog:8 > + Emit a "icon,changed" signal when the frame when one of its icons has Though I'm not sure where my English is correct, I don't understand below sentence well. * when the frame when one of its icons has changed.* Created attachment 140676 [details] Patch Ok, back to the beginning :) As it turns out, the icon type is only relevant on apple (only apple has touch-icons, other ports only have favicons: Bug 59143). Therefore, I removed the iconType parameter from the "icon,changed" signal and added an assert to make sure the iconType is always WebCore::Favicon. Looks fine on my side. Comment on attachment 140676 [details]
Patch
rs=me
Comment on attachment 140676 [details] Patch Clearing flags on attachment: 140676 Committed r116547: <http://trac.webkit.org/changeset/116547> All reviewed patches have been landed. Closing bug. |