Move histograms and supporting code that don't belong in content out.

Move histograms and supporting code that don't belong in content out.

R=avi@chromium.org

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

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

[Avi (use Gerrit), 2013-06-18 21:20:32]

https://codereview.chromium.org/17151010/diff/1/chrome/browser/ui/cocoa/apple...
File chrome/browser/ui/cocoa/applescript/window_applescript.mm (right):

https://codereview.chromium.org/17151010/diff/1/chrome/browser/ui/cocoa/apple...
chrome/browser/ui/cocoa/applescript/window_applescript.mm:184:
core_tab_helper->set_new_tab_start_time(newTabStartTime);
Kinda bummed that we have to paste this code everywhere that we create a tab
with kChromeUINewTabURL. Are these two files the only places? Can we catch it
elsewhere, perhaps in one of the browser_* files?

https://codereview.chromium.org/17151010/diff/1/chrome/browser/ui/webui/metri...
File chrome/browser/ui/webui/metrics_handler.cc (right):

https://codereview.chromium.org/17151010/diff/1/chrome/browser/ui/webui/metri...
chrome/browser/ui/webui/metrics_handler.cc:87: if
(core_tab_helper->new_tab_start_time().is_null())
Same comment repeatedly: when pulling stuff out into a tab helper, are we sure
that CoreTabHelper::FromWebContents won't be null?

[jam, 2013-06-18 21:28:43]

https://codereview.chromium.org/17151010/diff/1/chrome/browser/ui/cocoa/apple...
File chrome/browser/ui/cocoa/applescript/window_applescript.mm (right):

https://codereview.chromium.org/17151010/diff/1/chrome/browser/ui/cocoa/apple...
chrome/browser/ui/cocoa/applescript/window_applescript.mm:184:
core_tab_helper->set_new_tab_start_time(newTabStartTime);
On 2013/06/18 21:20:33, Avi wrote:
> Kinda bummed that we have to paste this code everywhere that we create a tab
> with kChromeUINewTabURL. Are these two files the only places? Can we catch it
> elsewhere, perhaps in one of the browser_* files?

well, this is translating calls that were to content to be instead to other code
in chrome, so I prefer if you want that changed, that's another cl. i just want
to clean out the content interfaces at this point

https://codereview.chromium.org/17151010/diff/1/chrome/browser/ui/webui/metri...
File chrome/browser/ui/webui/metrics_handler.cc (right):

https://codereview.chromium.org/17151010/diff/1/chrome/browser/ui/webui/metri...
chrome/browser/ui/webui/metrics_handler.cc:87: if
(core_tab_helper->new_tab_start_time().is_null())
On 2013/06/18 21:20:33, Avi wrote:
> Same comment repeatedly: when pulling stuff out into a tab helper, are we sure
> that CoreTabHelper::FromWebContents won't be null?

when would WebContents exist without a CoreTabHelper?

[Avi (use Gerrit), 2013-06-18 21:32:47]

LGTM

https://codereview.chromium.org/17151010/diff/1/chrome/browser/ui/cocoa/apple...
File chrome/browser/ui/cocoa/applescript/window_applescript.mm (right):

https://codereview.chromium.org/17151010/diff/1/chrome/browser/ui/cocoa/apple...
chrome/browser/ui/cocoa/applescript/window_applescript.mm:184:
core_tab_helper->set_new_tab_start_time(newTabStartTime);
On 2013/06/18 21:28:43, jam wrote:
> On 2013/06/18 21:20:33, Avi wrote:
> > Kinda bummed that we have to paste this code everywhere that we create a tab
> > with kChromeUINewTabURL. Are these two files the only places? Can we catch
it
> > elsewhere, perhaps in one of the browser_* files?
> 
> well, this is translating calls that were to content to be instead to other
code
> in chrome, so I prefer if you want that changed, that's another cl. i just
want
> to clean out the content interfaces at this point

SG.

https://codereview.chromium.org/17151010/diff/1/chrome/browser/ui/webui/metri...
File chrome/browser/ui/webui/metrics_handler.cc (right):

https://codereview.chromium.org/17151010/diff/1/chrome/browser/ui/webui/metri...
chrome/browser/ui/webui/metrics_handler.cc:87: if
(core_tab_helper->new_tab_start_time().is_null())
On 2013/06/18 21:28:43, jam wrote:
> On 2013/06/18 21:20:33, Avi wrote:
> > Same comment repeatedly: when pulling stuff out into a tab helper, are we
sure
> > that CoreTabHelper::FromWebContents won't be null?
> 
> when would WebContents exist without a CoreTabHelper?

Beats me; that's the lesson I learned.

You seem pretty sure that _these_ WebContents will have a CoreTabHelper. That's
probably true. Just keep an eye on crashes :)

[jam, 2013-06-18 22:44:26]

https://codereview.chromium.org/17151010/diff/1/chrome/browser/ui/webui/metri...
File chrome/browser/ui/webui/metrics_handler.cc (right):

https://codereview.chromium.org/17151010/diff/1/chrome/browser/ui/webui/metri...
chrome/browser/ui/webui/metrics_handler.cc:87: if
(core_tab_helper->new_tab_start_time().is_null())
On 2013/06/18 21:32:47, Avi wrote:
> On 2013/06/18 21:28:43, jam wrote:
> > On 2013/06/18 21:20:33, Avi wrote:
> > > Same comment repeatedly: when pulling stuff out into a tab helper, are we
> sure
> > > that CoreTabHelper::FromWebContents won't be null?
> > 
> > when would WebContents exist without a CoreTabHelper?
> 
> Beats me; that's the lesson I learned.
> 
> You seem pretty sure that _these_ WebContents will have a CoreTabHelper.
That's
> probably true. Just keep an eye on crashes :)

to be clear: I was asking you, not making a statement :) I have no idea if it'll
be null, I'm depending on tests + canary as I prefer not to add null checks if
I'm not sure they're needed.

[Avi (use Gerrit), 2013-06-19 04:43:36]

https://codereview.chromium.org/17151010/diff/1/chrome/browser/ui/webui/metri...
File chrome/browser/ui/webui/metrics_handler.cc (right):

https://codereview.chromium.org/17151010/diff/1/chrome/browser/ui/webui/metri...
chrome/browser/ui/webui/metrics_handler.cc:87: if
(core_tab_helper->new_tab_start_time().is_null())
On 2013/06/18 22:44:27, jam wrote:
> to be clear: I was asking you, not making a statement :) I have no idea if
it'll
> be null

My method was assuming it was non-null, and fixing crashes as they showed up. :O

[jam, 2013-06-19 04:56:20]

On 2013/06/19 04:43:36, Avi wrote:
>
https://codereview.chromium.org/17151010/diff/1/chrome/browser/ui/webui/metri...
> File chrome/browser/ui/webui/metrics_handler.cc (right):
> 
>
https://codereview.chromium.org/17151010/diff/1/chrome/browser/ui/webui/metri...
> chrome/browser/ui/webui/metrics_handler.cc:87: if
> (core_tab_helper->new_tab_start_time().is_null())
> On 2013/06/18 22:44:27, jam wrote:
> > to be clear: I was asking you, not making a statement :) I have no idea if
> it'll
> > be null
> 
> My method was assuming it was non-null, and fixing crashes as they showed up.
:O

ditto

btw this got reverted because of an asan failure (use after free). patchset 3 to
2 has the small fix of moving the observer call.

[Avi (use Gerrit), 2013-06-19 05:00:01]

SLGTM

https://codereview.chromium.org/17151010/diff/11004/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/17151010/diff/11004/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:3467: // Note: |this| can be
deleted at this point.
"can be deleted" sounds like permission. Perhaps "might be deleted"?