Bug 60527 – Return empty Favicon URL instead of default one when the frame isn't top level one.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

This bug was introduced by https://bugs.webkit.org/show_bug.cgi?id=59143

Created attachment 92907 [details]
Fix

[Darin Adler]

Comment on attachment 92907 [details]
Fix

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

This patch is not well explained. Even if the code change is correct, there is no rationale in the change log for the code changes.

> Source/WebCore/dom/Document.cpp:4437
> -    if (!iconURL(iconType).m_iconURL.isEmpty())
> +    if (iconURL(iconType).m_iconURL.isEmpty())

What’s the reasoning behind reversing the logic here?

> Source/WebCore/loader/FrameLoader.cpp:480
> +    // If this isn't a top level frame, return
> +    if (m_frame->tree() && m_frame->tree()->parent())
> +        return iconURLs;

This is the only change that matches the change log comment. But even this is not explained.

> Source/WebCore/loader/FrameLoader.cpp:-503
> -    // If this isn't a top level frame, return
> -    if (m_frame->tree() && m_frame->tree()->parent())
> -        return false;

Why is this a good change?

If you checked patch https://bugs.webkit.org/show_bug.cgi?id=59143, you will find the 59143 patch changed the orignal logic. This patch will correct it.


(In reply to comment #2)
> (From update of attachment 92907 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92907&action=review
> 
> This patch is not well explained. Even if the code change is correct, there is no rationale in the change log for the code changes.
> 
> > Source/WebCore/dom/Document.cpp:4437
> > -    if (!iconURL(iconType).m_iconURL.isEmpty())
> > +    if (iconURL(iconType).m_iconURL.isEmpty())
> 
> What’s the reasoning behind reversing the logic here?
> 
> > Source/WebCore/loader/FrameLoader.cpp:480
> > +    // If this isn't a top level frame, return
> > +    if (m_frame->tree() && m_frame->tree()->parent())
> > +        return iconURLs;
> 
> This is the only change that matches the change log comment. But even this is not explained.
> 
> > Source/WebCore/loader/FrameLoader.cpp:-503
> > -    // If this isn't a top level frame, return
> > -    if (m_frame->tree() && m_frame->tree()->parent())
> > -        return false;
> 
> Why is this a good change?

Let me know the additional information you want me provide.

Thanks


(In reply to comment #3)
> If you checked patch https://bugs.webkit.org/show_bug.cgi?id=59143, you will find the 59143 patch changed the orignal logic. This patch will correct it.
> 
> 
> (In reply to comment #2)
> > (From update of attachment 92907 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=92907&action=review
> > 
> > This patch is not well explained. Even if the code change is correct, there is no rationale in the change log for the code changes.
> > 
> > > Source/WebCore/dom/Document.cpp:4437
> > > -    if (!iconURL(iconType).m_iconURL.isEmpty())
> > > +    if (iconURL(iconType).m_iconURL.isEmpty())
> > 
> > What’s the reasoning behind reversing the logic here?
> > 
> > > Source/WebCore/loader/FrameLoader.cpp:480
> > > +    // If this isn't a top level frame, return
> > > +    if (m_frame->tree() && m_frame->tree()->parent())
> > > +        return iconURLs;
> > 
> > This is the only change that matches the change log comment. But even this is not explained.
> > 
> > > Source/WebCore/loader/FrameLoader.cpp:-503
> > > -    // If this isn't a top level frame, return
> > > -    if (m_frame->tree() && m_frame->tree()->parent())
> > > -        return false;
> > 
> > Why is this a good change?

[Tony Gentilcore]

(In reply to comment #4)
> Let me know the additional information you want me provide.
> 
> Thanks
> 
> 
> (In reply to comment #3)
> > If you checked patch https://bugs.webkit.org/show_bug.cgi?id=59143, you will find the 59143 patch changed the orignal logic. This patch will correct it.

Rather than pointing to another bug, I think Darin is asking that the changes be described in the ChangeLog. Good ChangeLog explanations are critical for future developers to understand why a change was made (they won't always look through the bug).

When possible, it is best to describe each function individually. For example, I don't know if this comment is true, but:

...
* dom/Document.cpp:
(WebCore::Document::setIconURL): Restored original logic which was inadvertently flipped by http://trac.webkit.org/changeset/85785.
...

Poke around in trac to see some examples.

Also, since this silently regressed, I guess it isn't tested. Is it possible to test this behavior?

Created attachment 93126 [details]
Add more explaination in change log

Created attachment 93127 [details]
Add more explaination in change log

[Tony Gentilcore]

Comment on attachment 93127 [details]
Add more explaination in change log

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

> Source/WebCore/ChangeLog:16
> +        This issue was discussed by chromium browser test.

Maybe "discovered" instead of "discussed"?

So I guess that means there isn't a good way to test this in WebKit, but at least some ports might have coverage?

> Source/WebCore/loader/FrameLoader.cpp:478
> +    // If this isn't a top level frame, return

I'd just remove this comment now. It basically restates the code below without answering the question "why".

(In reply to comment #8)
> (From update of attachment 93127 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93127&action=review
> 
> > Source/WebCore/ChangeLog:16
> > +        This issue was discussed by chromium browser test.
> 
> Maybe "discovered" instead of "discussed"?
> 
> So I guess that means there isn't a good way to test this in WebKit, but at least some ports might have coverage?
>

Yes, Chromium BrowserTest covered this case.
 
> > Source/WebCore/loader/FrameLoader.cpp:478
> > +    // If this isn't a top level frame, return
> 
> I'd just remove this comment now. It basically restates the code below without answering the question "why".

I didn't add this comment, and it helped me to understand the code, I think we may leave it there.

Created attachment 93139 [details]
Address the comment

[David Kilzer (:ddkilzer)]

Comment on attachment 93139 [details]
Address the comment

r=me

[David Kilzer (:ddkilzer)]

Comment on attachment 93139 [details]
Address the comment

BTW, if you want to use the commit-queue, you should set the cq? flag on the patch.

[WebKit Commit Bot]

Comment on attachment 93139 [details]
Address the comment

Clearing flags on attachment: 93139

Committed r86279: <http://trac.webkit.org/changeset/86279>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.