[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
bugzilla-daemon at webkit.org
Mon May 9 20:24:22 PDT 2011
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?
More information about the webkit-reviews
mailing list