WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
This bug was introduced by
https://bugs.webkit.org/show_bug.cgi?id=59143
Attachments
Fix
(2.34 KB, patch)
2011-05-09 19:11 PDT
,
michaelbai
darin
: review-
Details
Formatted Diff
Diff
Add more explaination in change log
(11.60 KB, patch)
2011-05-11 08:55 PDT
,
michaelbai
no flags
Details
Formatted Diff
Diff
Add more explaination in change log
(2.71 KB, patch)
2011-05-11 08:59 PDT
,
michaelbai
no flags
Details
Formatted Diff
Diff
Address the comment
(2.71 KB, patch)
2011-05-11 10:05 PDT
,
michaelbai
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
michaelbai
Comment 1
2011-05-09 19:11:02 PDT
Created
attachment 92907
[details]
Fix
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.
Top of Page
Format For Printing
XML
Clone This Bug