RESOLVED FIXED 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
Summary Return empty Favicon URL instead of default one when the frame isn't top leve...
michaelbai
Reported 2011-05-09 19:08:34 PDT
Attachments
Fix (2.34 KB, patch)
2011-05-09 19:11 PDT, michaelbai
darin: review-
Add more explaination in change log (11.60 KB, patch)
2011-05-11 08:55 PDT, michaelbai
no flags
Add more explaination in change log (2.71 KB, patch)
2011-05-11 08:59 PDT, michaelbai
no flags
Address the comment (2.71 KB, patch)
2011-05-11 10:05 PDT, michaelbai
no flags
michaelbai
Comment 1 2011-05-09 19:11:02 PDT
Darin Adler
Comment 2 2011-05-09 20:24:23 PDT
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?
michaelbai
Comment 3 2011-05-09 21:26:10 PDT
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?
michaelbai
Comment 4 2011-05-09 21:29:13 PDT
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
Comment 5 2011-05-11 01:39:20 PDT
(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?
michaelbai
Comment 6 2011-05-11 08:55:22 PDT
Created attachment 93126 [details] Add more explaination in change log
michaelbai
Comment 7 2011-05-11 08:59:03 PDT
Created attachment 93127 [details] Add more explaination in change log
Tony Gentilcore
Comment 8 2011-05-11 09:10:02 PDT
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".
michaelbai
Comment 9 2011-05-11 10:04:44 PDT
(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.
michaelbai
Comment 10 2011-05-11 10:05:05 PDT
Created attachment 93139 [details] Address the comment
David Kilzer (:ddkilzer)
Comment 11 2011-05-11 12:34:10 PDT
Comment on attachment 93139 [details] Address the comment r=me
David Kilzer (:ddkilzer)
Comment 12 2011-05-11 12:35:03 PDT
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 13 2011-05-11 15:58:51 PDT
Comment on attachment 93139 [details] Address the comment Clearing flags on attachment: 93139 Committed r86279: <http://trac.webkit.org/changeset/86279>
WebKit Commit Bot
Comment 14 2011-05-11 15:58:57 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.