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
: WebKit
WebKit EFL
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-04-20 04:51 PST by
Modified: 2012-05-09 11:39 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-04-20 04:51:08 PST
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 From 2012-04-20 04:59:22 PST -------
Created an attachment (id=138071) [details]
Patch
------- Comment #2 From 2012-04-21 00:58:31 PST -------
(From update of attachment 138071 [details])
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 From 2012-04-22 22:33:39 PST -------
Created an attachment (id=138291) [details]
Patch

Updated patch to take feedback into consideration.
------- Comment #4 From 2012-04-23 19:33:23 PST -------
(From update of attachment 138291 [details])
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 From 2012-04-23 22:38:06 PST -------
Created an attachment (id=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 From 2012-04-26 02:09:56 PST -------
(From update of attachment 138291 [details])
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 From 2012-04-26 02:11:21 PST -------
(From update of attachment 138495 [details])
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 From 2012-04-26 03:37:08 PST -------
Created an attachment (id=138965) [details]
Patch

Take feedback into consideration.
------- Comment #9 From 2012-04-26 07:04:37 PST -------
(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 From 2012-04-26 22:57:11 PST -------
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 From 2012-05-04 00:28:20 PST -------
Any update on this?
------- Comment #12 From 2012-05-06 18:35:50 PST -------
(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 From 2012-05-07 00:29:25 PST -------
(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 From 2012-05-07 00:51:04 PST -------
Created an attachment (id=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 From 2012-05-07 07:58:10 PST -------
(From update of attachment 140487 [details])
Looks good overall, but please add some documentation to Ewk_Icon_Type.
------- Comment #16 From 2012-05-07 18:02:59 PST -------
(From update of attachment 140487 [details])
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 From 2012-05-07 22:41:34 PST -------
Created an attachment (id=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 From 2012-05-07 22:56:29 PST -------
Looks fine on my side.
------- Comment #19 From 2012-05-09 11:27:28 PST -------
(From update of attachment 140676 [details])
rs=me
------- Comment #20 From 2012-05-09 11:39:40 PST -------
(From update of attachment 140676 [details])
Clearing flags on attachment: 140676

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