Bug 60527 - Return empty Favicon URL instead of default one when the frame isn't top level one.
Summary: Return empty Favicon URL instead of default one when the frame isn't top leve...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-09 19:08 PDT by michaelbai
Modified: 2011-05-11 15:58 PDT (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description michaelbai 2011-05-09 19:08:34 PDT
This bug was introduced by https://bugs.webkit.org/show_bug.cgi?id=59143
Comment 1 michaelbai 2011-05-09 19:11:02 PDT
Created attachment 92907 [details]
Fix
Comment 2 Darin Adler 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?
Comment 3 michaelbai 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?
Comment 4 michaelbai 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?
Comment 5 Tony Gentilcore 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?
Comment 6 michaelbai 2011-05-11 08:55:22 PDT
Created attachment 93126 [details]
Add more explaination in change log
Comment 7 michaelbai 2011-05-11 08:59:03 PDT
Created attachment 93127 [details]
Add more explaination in change log
Comment 8 Tony Gentilcore 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".
Comment 9 michaelbai 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.
Comment 10 michaelbai 2011-05-11 10:05:05 PDT
Created attachment 93139 [details]
Address the comment
Comment 11 David Kilzer (:ddkilzer) 2011-05-11 12:34:10 PDT
Comment on attachment 93139 [details]
Address the comment

r=me
Comment 12 David Kilzer (:ddkilzer) 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.
Comment 13 WebKit Commit Bot 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>
Comment 14 WebKit Commit Bot 2011-05-11 15:58:57 PDT
All reviewed patches have been landed.  Closing bug.