Bug 84435 - [EFL][DRT] EFL's DRT needs to support LayoutTestController.dumpIconChanges()
: [EFL][DRT] EFL's DRT needs to support LayoutTestController.dumpIconChanges()
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit EFL
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Chris Dumez
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-20 04:51 PDT by Chris Dumez
Modified: 2012-05-09 11:39 PDT (History)
6 users (show)

See Also:


Attachments
Patch (7.71 KB, patch)
2012-04-20 04:59 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (7.81 KB, patch)
2012-04-22 22:33 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (8.79 KB, patch)
2012-04-23 22:38 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (10.11 KB, patch)
2012-04-26 03:37 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (10.95 KB, patch)
2012-05-07 00:51 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (9.07 KB, patch)
2012-05-07 22:41 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2012-04-20 04:51:08 PDT
EFL's DRT needs to print information when the main frame icon is changed and LayoutTestController.dumpIconChanges() returns true.

This is allow unskipping fast/dom/icon-url-property.html.
Comment 1 Chris Dumez 2012-04-20 04:59:22 PDT
Created attachment 138071 [details]
Patch
Comment 2 Gyuyoung Kim 2012-04-21 00:58:31 PDT
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 ?
Comment 3 Chris Dumez 2012-04-22 22:33:39 PDT
Created attachment 138291 [details]
Patch

Updated patch to take feedback into consideration.
Comment 4 Raphael Kubo da Costa (:rakuco) 2012-04-23 19:33:23 PDT
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?
Comment 5 Chris Dumez 2012-04-23 22:38:06 PDT
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 6 Gyuyoung Kim 2012-04-26 02:09:56 PDT
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 7 Gyuyoung Kim 2012-04-26 02:11:21 PDT
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 ?
Comment 8 Chris Dumez 2012-04-26 03:37:08 PDT
Created attachment 138965 [details]
Patch

Take feedback into consideration.
Comment 9 Raphael Kubo da Costa (:rakuco) 2012-04-26 07:04:37 PDT
(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.
Comment 10 Chris Dumez 2012-04-26 22:57:11 PDT
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?
Comment 11 Chris Dumez 2012-05-04 00:28:20 PDT
Any update on this?
Comment 12 Raphael Kubo da Costa (:rakuco) 2012-05-06 18:35:50 PDT
(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).
Comment 13 Gyuyoung Kim 2012-05-07 00:29:25 PDT
(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.
Comment 14 Chris Dumez 2012-05-07 00:51:04 PDT
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 15 Raphael Kubo da Costa (:rakuco) 2012-05-07 07:58:10 PDT
Comment on attachment 140487 [details]
Patch

Looks good overall, but please add some documentation to Ewk_Icon_Type.
Comment 16 Gyuyoung Kim 2012-05-07 18:02:59 PDT
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.*
Comment 17 Chris Dumez 2012-05-07 22:41:34 PDT
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.
Comment 18 Gyuyoung Kim 2012-05-07 22:56:29 PDT
Looks fine on my side.
Comment 19 Antonio Gomes 2012-05-09 11:27:28 PDT
Comment on attachment 140676 [details]
Patch

rs=me
Comment 20 WebKit Review Bot 2012-05-09 11:39:40 PDT
Comment on attachment 140676 [details]
Patch

Clearing flags on attachment: 140676

Committed r116547: <http://trac.webkit.org/changeset/116547>
Comment 21 WebKit Review Bot 2012-05-09 11:39:46 PDT
All reviewed patches have been landed.  Closing bug.