This bug was introduced by https://bugs.webkit.org/show_bug.cgi?id=59143
Created attachment 92907 [details] Fix
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?
(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
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
Comment on attachment 93139 [details] Address the comment r=me
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.
Comment on attachment 93139 [details] Address the comment Clearing flags on attachment: 93139 Committed r86279: <http://trac.webkit.org/changeset/86279>
All reviewed patches have been landed. Closing bug.