Adding a favicon may trigger an invalid didChangedIcon event
Created attachment 124030 [details] Patch
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).
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.
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. :)
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.
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?
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?
(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.