Removes usage of NavigationEntry from favicon_handler.*

Removes usage of NavigationEntry from favicon_handler.*

BUG=359598
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269871

[blundell, 2014-05-06 15:03:59]

I like this direction.

https://codereview.chromium.org/261403003/diff/1/chrome/browser/favicon/favic...
File chrome/browser/favicon/favicon_handler.cc (right):

https://codereview.chromium.org/261403003/diff/1/chrome/browser/favicon/favic...
chrome/browser/favicon/favicon_handler.cc:403:
driver_->GetActiveFaviconValidity() &&
Hmm, maybe these would all read better as "FooForCurrentPage"?

GetURLForCurrentPage()
GetFaviconValidityForCurrentPage()
SetFaviconImageForCurrentPage()

etc. WDYT?

https://codereview.chromium.org/261403003/diff/1/chrome/browser/favicon/favic...
chrome/browser/favicon/favicon_handler.cc:494: bool
FaviconHandler::NavigationEntryConcernsFavicon() {
I think this is just something like "PageHasChangedSinceFoo", where "Foo" is the
event that causes url_ to be set.

https://codereview.chromium.org/261403003/diff/1/chrome/browser/favicon/favic...
chrome/browser/favicon/favicon_handler.cc:495: NavigationEntry* entry =
driver_->GetActiveEntry();
Could you get rid of all mentions of NavigationEntry in this file?

https://codereview.chromium.org/261403003/diff/1/chrome/browser/favicon/favic...
File chrome/browser/favicon/favicon_tab_helper.cc (right):

https://codereview.chromium.org/261403003/diff/1/chrome/browser/favicon/favic...
chrome/browser/favicon/favicon_tab_helper.cc:178: return GURL();
could you implement these so we can see the full CL?

https://codereview.chromium.org/261403003/diff/1/chrome/browser/favicon/favic...
chrome/browser/favicon/favicon_tab_helper.cc:186:
web_contents()->GetController().GetActiveEntry()->GetFavicon().valid =
the active entry can be NULL. You probably want a helper method that gets the
entry and then just short-circuit out if it's NULL (or alternatively DCHECK
not-NULL if some of these have a contract that they should only be called when
there's an active entry).

[blundell, 2014-05-06 15:39:48]

Hi John,

This CL is a work in progress, but I would just like to get
confirmation/correction of my understanding on one crucial point that the CL
hinges on (the question in the comment on favicon_handler.cc:573).

Thanks,

Colin

https://codereview.chromium.org/261403003/diff/1/chrome/browser/favicon/favic...
File chrome/browser/favicon/favicon_handler.cc (left):

https://codereview.chromium.org/261403003/diff/1/chrome/browser/favicon/favic...
chrome/browser/favicon/favicon_handler.cc:573: NavigationEntry* entry =
GetEntry();
This code was previously getting the active NavigationEntry and then
getting/setting state from it through the rest of this method body. In the CL
under development here, the state getters and setters would each get the active
NavigationEntry in their own method bodies. My understanding is that this change
is OK to make because the active NavigationEntry won't change while this code is
executing. Is that correct?

[jam, 2014-05-06 18:18:28]

https://codereview.chromium.org/261403003/diff/1/chrome/browser/favicon/favic...
File chrome/browser/favicon/favicon_handler.cc (left):

https://codereview.chromium.org/261403003/diff/1/chrome/browser/favicon/favic...
chrome/browser/favicon/favicon_handler.cc:573: NavigationEntry* entry =
GetEntry();
On 2014/05/06 15:39:48, blundell wrote:
> This code was previously getting the active NavigationEntry and then
> getting/setting state from it through the rest of this method body. In the CL
> under development here, the state getters and setters would each get the
active
> NavigationEntry in their own method bodies. My understanding is that this
change
> is OK to make because the active NavigationEntry won't change while this code
is
> executing. Is that correct?

yep I don't see how that would change. the nav entry would be committed based on
IPCs from the renderer, which aren't going to be dispatched while this code is
running

[blundell, 2014-05-07 09:01:06]

Thanks! -jam from reviewers to avoid spam

[jif, 2014-05-09 14:17:18]

PTAL!

https://codereview.chromium.org/261403003/diff/1/chrome/browser/favicon/favic...
File chrome/browser/favicon/favicon_handler.cc (right):

https://codereview.chromium.org/261403003/diff/1/chrome/browser/favicon/favic...
chrome/browser/favicon/favicon_handler.cc:403:
driver_->GetActiveFaviconValidity() &&
On 2014/05/06 15:04:00, blundell wrote:
> Hmm, maybe these would all read better as "FooForCurrentPage"?
> 
> GetURLForCurrentPage()
> GetFaviconValidityForCurrentPage()
> SetFaviconImageForCurrentPage()
> 
> etc. WDYT?

Done.

https://codereview.chromium.org/261403003/diff/1/chrome/browser/favicon/favic...
chrome/browser/favicon/favicon_handler.cc:494: bool
FaviconHandler::NavigationEntryConcernsFavicon() {
On 2014/05/06 15:04:00, blundell wrote:
> I think this is just something like "PageHasChangedSinceFoo", where "Foo" is
the
> event that causes url_ to be set.

Done.

https://codereview.chromium.org/261403003/diff/1/chrome/browser/favicon/favic...
chrome/browser/favicon/favicon_handler.cc:495: NavigationEntry* entry =
driver_->GetActiveEntry();
On 2014/05/06 15:04:00, blundell wrote:
> Could you get rid of all mentions of NavigationEntry in this file?

Yeah, just wanted to get to a state were it was building.

https://codereview.chromium.org/261403003/diff/1/chrome/browser/favicon/favic...
File chrome/browser/favicon/favicon_tab_helper.cc (right):

https://codereview.chromium.org/261403003/diff/1/chrome/browser/favicon/favic...
chrome/browser/favicon/favicon_tab_helper.cc:178: return GURL();
On 2014/05/06 15:04:00, blundell wrote:
> could you implement these so we can see the full CL?

Done.

https://codereview.chromium.org/261403003/diff/1/chrome/browser/favicon/favic...
chrome/browser/favicon/favicon_tab_helper.cc:186:
web_contents()->GetController().GetActiveEntry()->GetFavicon().valid =
On 2014/05/06 15:04:00, blundell wrote:
> the active entry can be NULL. You probably want a helper method that gets the
> entry and then just short-circuit out if it's NULL (or alternatively DCHECK
> not-NULL if some of these have a contract that they should only be called when
> there's an active entry).

Done.

[blundell, 2014-05-09 14:30:33]

Mostly nits :).

https://codereview.chromium.org/261403003/diff/170001/chrome/browser/favicon/...
File chrome/browser/favicon/favicon_handler.h (right):

https://codereview.chromium.org/261403003/diff/170001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_handler.h:228: void SetFaviconOnNavigationEntry(
s/OnNavigationEntry// ?
Is line 227 obsolete now?

https://codereview.chromium.org/261403003/diff/170001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_handler.h:239: // Returns wheter the page's url
changed since requesting the favicon.
s/requesting the favicon/the favicon was requested

https://codereview.chromium.org/261403003/diff/170001/chrome/browser/favicon/...
File chrome/browser/favicon/favicon_tab_helper.cc (right):

https://codereview.chromium.org/261403003/diff/170001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_tab_helper.cc:198:
CHECK(web_contents()->GetController().GetActiveEntry());
s/CHECK/DCHECK

https://codereview.chromium.org/261403003/diff/170001/components/favicon/core...
File components/favicon/core/favicon_driver.h (right):

https://codereview.chromium.org/261403003/diff/170001/components/favicon/core...
components/favicon/core/favicon_driver.h:39: virtual const gfx::Image
GetActiveFaviconImage() = 0;
It looks like all of these getters/setters expected that GetActiveURL() is valid
when they're called? Document.

https://codereview.chromium.org/261403003/diff/170001/components/favicon/core...
components/favicon/core/favicon_driver.h:48: // Returns the URL of the current
page.
Returns an invalid GURL if there is no current page, right? Document.

https://codereview.chromium.org/261403003/diff/170001/components/favicon/core...
components/favicon/core/favicon_driver.h:54: // Sets whether we've got the
current page's favicon, or just using the
This comment is wrong.

https://codereview.chromium.org/261403003/diff/170001/components/favicon/core...
components/favicon/core/favicon_driver.h:58: // Set whether we've got the page's
favicon, or just using the default
Sets whether the page's favicon is valid (if false, the default favicon is being
used).

[jif, 2014-05-11 13:41:22]

Thanks!
ptal.

https://codereview.chromium.org/261403003/diff/170001/chrome/browser/favicon/...
File chrome/browser/favicon/favicon_handler.h (right):

https://codereview.chromium.org/261403003/diff/170001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_handler.h:228: void SetFaviconOnNavigationEntry(
On 2014/05/09 14:30:34, blundell wrote:
> s/OnNavigationEntry// ?
> Is line 227 obsolete now?

Correct.

https://codereview.chromium.org/261403003/diff/170001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_handler.h:239: // Returns wheter the page's url
changed since requesting the favicon.
On 2014/05/09 14:30:34, blundell wrote:
> s/requesting the favicon/the favicon was requested

Done.

https://codereview.chromium.org/261403003/diff/170001/chrome/browser/favicon/...
File chrome/browser/favicon/favicon_tab_helper.cc (right):

https://codereview.chromium.org/261403003/diff/170001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_tab_helper.cc:198:
CHECK(web_contents()->GetController().GetActiveEntry());
On 2014/05/09 14:30:34, blundell wrote:
> s/CHECK/DCHECK

Done.

https://codereview.chromium.org/261403003/diff/170001/components/favicon/core...
File components/favicon/core/favicon_driver.h (right):

https://codereview.chromium.org/261403003/diff/170001/components/favicon/core...
components/favicon/core/favicon_driver.h:39: virtual const gfx::Image
GetActiveFaviconImage() = 0;
On 2014/05/09 14:30:34, blundell wrote:
> It looks like all of these getters/setters expected that GetActiveURL() is
valid
> when they're called? Document.

Done.

https://codereview.chromium.org/261403003/diff/170001/components/favicon/core...
components/favicon/core/favicon_driver.h:48: // Returns the URL of the current
page.
On 2014/05/09 14:30:34, blundell wrote:
> Returns an invalid GURL if there is no current page, right? Document.

Done.

https://codereview.chromium.org/261403003/diff/170001/components/favicon/core...
components/favicon/core/favicon_driver.h:54: // Sets whether we've got the
current page's favicon, or just using the
On 2014/05/09 14:30:34, blundell wrote:
> This comment is wrong.

Done.

https://codereview.chromium.org/261403003/diff/170001/components/favicon/core...
components/favicon/core/favicon_driver.h:58: // Set whether we've got the page's
favicon, or just using the default
On 2014/05/09 14:30:34, blundell wrote:
> Sets whether the page's favicon is valid (if false, the default favicon is
being
> used).

Done.

[blundell, 2014-05-12 07:52:19]

LGTM with nits

Clean approach to a tricky refactoring

https://codereview.chromium.org/261403003/diff/190001/chrome/browser/favicon/...
File chrome/browser/favicon/favicon_handler.h (right):

https://codereview.chromium.org/261403003/diff/190001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_handler.h:47: //   the NavigationEntry and use
the favicon.
s/NavigationEntry/current page

https://codereview.chromium.org/261403003/diff/190001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_handler.h:236: // Returns wheter the page's url
changed since the favicon was requested.
s/wheter/whether

https://codereview.chromium.org/261403003/diff/190001/chrome/browser/favicon/...
File chrome/browser/favicon/favicon_tab_helper.h (right):

https://codereview.chromium.org/261403003/diff/190001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_tab_helper.h:110: // FaviconDriver helper method.
I would comment what this function does.

https://codereview.chromium.org/261403003/diff/190001/components/favicon/core...
File components/favicon/core/favicon_driver.h (right):

https://codereview.chromium.org/261403003/diff/190001/components/favicon/core...
components/favicon/core/favicon_driver.h:46: // Returns whether we've got the
current page's favicon, or just using the
Update this comment to match the comment at line 61.

[jif, 2014-05-12 18:07:41]

done

https://codereview.chromium.org/261403003/diff/190001/chrome/browser/favicon/...
File chrome/browser/favicon/favicon_handler.h (right):

https://codereview.chromium.org/261403003/diff/190001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_handler.h:47: //   the NavigationEntry and use
the favicon.
On 2014/05/12 07:52:19, blundell wrote:
> s/NavigationEntry/current page

Done.

https://codereview.chromium.org/261403003/diff/190001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_handler.h:236: // Returns wheter the page's url
changed since the favicon was requested.
On 2014/05/12 07:52:19, blundell wrote:
> s/wheter/whether

Done.

https://codereview.chromium.org/261403003/diff/190001/chrome/browser/favicon/...
File chrome/browser/favicon/favicon_tab_helper.h (right):

https://codereview.chromium.org/261403003/diff/190001/chrome/browser/favicon/...
chrome/browser/favicon/favicon_tab_helper.h:110: // FaviconDriver helper method.
On 2014/05/12 07:52:19, blundell wrote:
> I would comment what this function does.

Done.

https://codereview.chromium.org/261403003/diff/190001/components/favicon/core...
File components/favicon/core/favicon_driver.h (right):

https://codereview.chromium.org/261403003/diff/190001/components/favicon/core...
components/favicon/core/favicon_driver.h:46: // Returns whether we've got the
current page's favicon, or just using the
On 2014/05/12 07:52:19, blundell wrote:
> Update this comment to match the comment at line 61.

Done.

[jif, 2014-05-12 18:07:46]

The CQ bit was checked by jif@chromium.org

[commit-bot: I haz the power, 2014-05-12 18:11:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jif@chromium.org/261403003/210001

[commit-bot: I haz the power, 2014-05-12 20:57:24]

Change committed as 269871