Bug 84435 – [EFL][DRT] EFL's DRT needs to support LayoutTestController.dumpIconChanges()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 138071 [details]
Patch

[Gyuyoung Kim]

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 ?

[Chris Dumez]

Created attachment 138291 [details]
Patch

Updated patch to take feedback into consideration.

[Raphael Kubo da Costa (:rakuco)]

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?

[Chris Dumez]

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.

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

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 ?

[Chris Dumez]

Created attachment 138965 [details]
Patch

Take feedback into consideration.

[Raphael Kubo da Costa (:rakuco)]

(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.

[Chris Dumez]

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?

[Chris Dumez]

Any update on this?

[Raphael Kubo da Costa (:rakuco)]

(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).

[Gyuyoung Kim]

(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.

[Chris Dumez]

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).

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 140487 [details]
Patch

Looks good overall, but please add some documentation to Ewk_Icon_Type.

[Gyuyoung Kim]

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.*

[Chris Dumez]

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.

[Gyuyoung Kim]

Looks fine on my side.

[Antonio Gomes]

Comment on attachment 140676 [details]
Patch

rs=me

[WebKit Review Bot]

Comment on attachment 140676 [details]
Patch

Clearing flags on attachment: 140676

Committed r116547: <http://trac.webkit.org/changeset/116547>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.