[webkit-reviews] review denied: [Bug 60527] Return empty Favicon URL instead of default one when the frame isn't top level one. : [Attachment 92907] Fix

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied michaelbai at chromium.org's request for
review:
Bug 60527: Return empty Favicon URL instead of default one when the frame isn't
top level one.
https://bugs.webkit.org/show_bug.cgi?id=60527

Attachment 92907: Fix
https://bugs.webkit.org/attachment.cgi?id=92907&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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?