Adding a favicon may trigger an invalid didChangedIcon event

• Component: New Bugs
• Status: NEW ---

[Rafael Brandao]

Adding a favicon may trigger an invalid didChangedIcon event

[Rafael Brandao]

Created attachment 124030 [details]
Patch

[Rafael Brandao]

Comment on attachment 124030 [details]
Patch

We've figured out this bug in the discussion at bug #75877. We may have the same url already in the list of favicons for a page and then add a new one with the same url. Even if we are going to use it, the icon url wouldn't have changed. I'll try to add a test for this, but if I remember correctly, we have a problem to test favicon loads on layout tests as we can't hold the end of it until the icon load happens (there is no event we could use to listen).

[Eric Seidel (no email)]

Comment on attachment 124030 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124030&action=review

How do we test this?

> Source/WebCore/ChangeLog:3
> +        Adding a favicon may trigger an invalid didChangedIcon event

I think you mean "didChangeIcon"?  "didChanged" doesn't make much sense.

[Rafael Brandao]

Comment on attachment 124030 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124030&action=review

Thanks for reviewing!

Back in time there was no way to retrieve which favicon was resolved by the ui client. This is the reason which most of tests involving favicons are flaky or skipped. I'm not sure of how things are now, but maybe adding a way to access it via Internals would be the way to use it on a layout test.

>> Source/WebCore/ChangeLog:3
>> +        Adding a favicon may trigger an invalid didChangedIcon event
> 
> I think you mean "didChangeIcon"?  "didChanged" doesn't make much sense.

Oops, typo. :)

[Rafael Brandao]

It looks like the results of this test will be port-specific. On Qt, if we have set dumpFrameLoadCallbacks, then we will print something when we emit the iconChanged signal. But for Chromium, it doesn't happen. Gtk, doesn't have it implemented, and so on. Do you think the way to go here is to implement the same output for each one of them or try to expose things through Internals, it it's possible?

The idea of the test would be to check whether the iconChanged signal is emitted. It should not emit when we "change" the icon to the same previous icon.

[Eric Seidel (no email)]

Comment on attachment 124030 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124030&action=review

>>> Source/WebCore/ChangeLog:3
>>> +        Adding a favicon may trigger an invalid didChangedIcon event
>> 
>> I think you mean "didChangeIcon"?  "didChanged" doesn't make much sense.
> 
> Oops, typo. :)

So I'm confsued.  Is this actually an event (in teh DOM sense)?  Or is this a signal in the Qt sense?

> Source/WebCore/dom/Document.cpp:4594
> +    if (f)
> +        previousIconURL = f->loader()->icon()->resolvedIconURL(iconType);

Why does this need to be moved earlier like this?

[Eric Seidel (no email)]

Comment on attachment 124030 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124030&action=review

> Source/WebCore/loader/icon/IconController.h:52
> +    IconURL resolvedIconURL(IconType) const;

Why are you changing this method name?

[Rafael Brandao]

(In reply to comment #6)
> (From update of attachment 124030 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124030&action=review
> 
> >>> Source/WebCore/ChangeLog:3
> >>> +        Adding a favicon may trigger an invalid didChangedIcon event
> >> 
> >> I think you mean "didChangeIcon"?  "didChanged" doesn't make much sense.
> > 
> > Oops, typo. :)
> 
> So I'm confsued.  Is this actually an event (in teh DOM sense)?  Or is this a signal in the Qt sense?

This is not an event in DOM sense, but in UI client sense (a signal in Qt). It's emitted through FrameLoaderClient.

> 
> > Source/WebCore/dom/Document.cpp:4594
> > +    if (f)
> > +        previousIconURL = f->loader()->icon()->resolvedIconURL(iconType);
> 
> Why does this need to be moved earlier like this?

Because I need to know which icon was chosen before we add the new icon, and we don't keep that info anywhere.

The name was modified to "resolvedIconUrl" to make more sense to what it is actually doing. It has a list of currently avaliable icons of that page and then we must choose which would we would pick at that given point in time.