Switch FaviconTabHelper to use WebContentsUserData.

Switch FaviconTabHelper to use WebContentsUserData.

BUG=107201
TEST=no visible change


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=159606

[Avi (use Gerrit), 2012-09-29 20:43:59]

Jenn: panels
Steven: all else

[jennb, 2012-10-01 17:28:27]

LGTM for panels.

[stevenjb, 2012-10-01 17:56:50]

https://codereview.chromium.org/11011002/diff/13001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/11011002/diff/13001/chrome/browser/ui/browser...
chrome/browser/ui/browser.cc:553: : NULL;
nit: does : NULL; fit on the previous line?

https://codereview.chromium.org/11011002/diff/13001/chrome/browser/ui/cocoa/t...
File chrome/browser/ui/cocoa/tab_contents/favicon_util_mac.mm (right):

https://codereview.chromium.org/11011002/diff/13001/chrome/browser/ui/cocoa/t...
chrome/browser/ui/cocoa/tab_contents/favicon_util_mac.mm:16: contents ?
FaviconTabHelper::FromWebContents(contents->web_contents())
This seems like a common enough pattern that it would make sense to have a
::FromTabContents. Or would that be difficult/awkward?

https://codereview.chromium.org/11011002/diff/13001/chrome/browser/ui/content...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/11011002/diff/13001/chrome/browser/ui/content...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:480:
TabContents* tab_contents = TabContents::FromWebContents(*i);
nit: Move this above where it's needed (or eliminate the local variable
entirely)

https://codereview.chromium.org/11011002/diff/13001/chrome/browser/ui/content...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:482:
FaviconTabHelper::FromWebContents(*i);
nit: Don't really need a local here either.

https://codereview.chromium.org/11011002/diff/13001/chrome/browser/ui/views/h...
File chrome/browser/ui/views/hung_renderer_view.h (right):

https://codereview.chromium.org/11011002/diff/13001/chrome/browser/ui/views/h...
chrome/browser/ui/views/hung_renderer_view.h:62: using
content::WebContentsObserver::web_contents;
My understanding is that we avoid 'using' in headers? This doesn't seem too
problematic since it's in a private Impl class, but it still might be better to
eliminate it from here and be explicit (or move the 'using' to the .cc file)

[Avi (use Gerrit), 2012-10-01 19:23:56]

https://codereview.chromium.org/11011002/diff/13001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/11011002/diff/13001/chrome/browser/ui/browser...
chrome/browser/ui/browser.cc:553: : NULL;
On 2012/10/01 17:56:51, stevenjb (chromium) wrote:
> nit: does : NULL; fit on the previous line?

Done.

https://codereview.chromium.org/11011002/diff/13001/chrome/browser/ui/cocoa/t...
File chrome/browser/ui/cocoa/tab_contents/favicon_util_mac.mm (right):

https://codereview.chromium.org/11011002/diff/13001/chrome/browser/ui/cocoa/t...
chrome/browser/ui/cocoa/tab_contents/favicon_util_mac.mm:16: contents ?
FaviconTabHelper::FromWebContents(contents->web_contents())
On 2012/10/01 17:56:51, stevenjb (chromium) wrote:
> This seems like a common enough pattern that it would make sense to have a
> ::FromTabContents. Or would that be difficult/awkward?

I'm on a quest to kill TabContents (see the bug linked from this CL). Adding any
more use of TabContents would be counterproductive.

https://codereview.chromium.org/11011002/diff/13001/chrome/browser/ui/content...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/11011002/diff/13001/chrome/browser/ui/content...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:480:
TabContents* tab_contents = TabContents::FromWebContents(*i);
On 2012/10/01 17:56:51, stevenjb (chromium) wrote:
> nit: Move this above where it's needed (or eliminate the local variable
> entirely)

Done.

https://codereview.chromium.org/11011002/diff/13001/chrome/browser/ui/content...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:482:
FaviconTabHelper::FromWebContents(*i);
On 2012/10/01 17:56:51, stevenjb (chromium) wrote:
> nit: Don't really need a local here either.

Done.

https://codereview.chromium.org/11011002/diff/13001/chrome/browser/ui/views/h...
File chrome/browser/ui/views/hung_renderer_view.h (right):

https://codereview.chromium.org/11011002/diff/13001/chrome/browser/ui/views/h...
chrome/browser/ui/views/hung_renderer_view.h:62: using
content::WebContentsObserver::web_contents;
On 2012/10/01 17:56:51, stevenjb (chromium) wrote:
> My understanding is that we avoid 'using' in headers? This doesn't seem too
> problematic since it's in a private Impl class, but it still might be better
to
> eliminate it from here and be explicit (or move the 'using' to the .cc file)

Done.

[stevenjb, 2012-10-01 19:31:46]

LGTM

https://codereview.chromium.org/11011002/diff/13001/chrome/browser/ui/cocoa/t...
File chrome/browser/ui/cocoa/tab_contents/favicon_util_mac.mm (right):

https://codereview.chromium.org/11011002/diff/13001/chrome/browser/ui/cocoa/t...
chrome/browser/ui/cocoa/tab_contents/favicon_util_mac.mm:16: contents ?
FaviconTabHelper::FromWebContents(contents->web_contents())
On 2012/10/01 19:23:57, Avi wrote:
> On 2012/10/01 17:56:51, stevenjb (chromium) wrote:
> > This seems like a common enough pattern that it would make sense to have a
> > ::FromTabContents. Or would that be difficult/awkward?
> 
> I'm on a quest to kill TabContents (see the bug linked from this CL). Adding
any
> more use of TabContents would be counterproductive.
Fair enough.

[commit-bot: I haz the power, 2012-10-01 19:34:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/11011002/22001

[commit-bot: I haz the power, 2012-10-01 19:35:20]

Presubmit check for 11011002-22001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/task_manager
    chrome/browser/ui/gtk
    chrome/browser/ui/content_settings
    chrome/browser/ui/ash/launcher
    chrome/browser/prerender
    chrome/browser/ui/webui
    chrome/browser/ui/views/tabs
    chrome/browser/ui
    chrome/browser/ui/views

Presubmit checks took 3.1s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Avi (use Gerrit), 2012-10-01 19:38:10]

Ben: OWNER

[Ben Goodger (Google), 2012-10-01 21:09:27]

lgtm

[commit-bot: I haz the power, 2012-10-01 21:15:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/11011002/22001

[commit-bot: I haz the power, 2012-10-02 01:33:16]

Change committed as 159606