Detect when chrome is shutting down and don't SendFinacialPing

Detect when chrome is shutting down and don't SendFinacialPing

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

[bcwhite, 2013-04-25 23:03:41]

This is basically the same as

https://chromiumcodereview.appspot.com/11419137

but using "post" hooks.

[jam, 2013-04-26 16:31:42]

On 2013/04/25 23:03:41, bcwhite wrote:
> This is basically the same as
> 
> https://chromiumcodereview.appspot.com/11419137
> 
> but using "post" hooks.

please see chrome/OWNERS. i'm too overloaded to do reviews in src/chrome, so i'm
not in the reviewer section

[bcwhite, 2013-04-26 16:56:20]

Will do.  I chose you because "git cl upload" suggested you and because you
requested the revert of the original patch and gave information on how it should
be done.  (You probably don't remember...  it was a few months back.)

[bcwhite, 2013-04-26 16:57:56]

Brett, would you mind taking a look at this?

[brettw, 2013-04-28 03:48:11]

lgtm

[commit-bot: I haz the power, 2013-04-29 16:53:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bcwhite@chromium.org/13868016/1

[Roger Tawa OOO till Jul 10th, 2013-04-29 16:56:45]

Hi Brian,

Some comments below.

https://codereview.chromium.org/13868016/diff/1/chrome/browser/chrome_browser...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/13868016/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_main.cc:1622:
RLZTracker::PostMainMessageLoopRun();
Browser shutdown already calls RLZTracker::CleanupRlz().  Instead of adding this
new method can we do the work in there?

https://codereview.chromium.org/13868016/diff/1/chrome/browser/rlz/rlz.cc
File chrome/browser/rlz/rlz.cc (right):

https://codereview.chromium.org/13868016/diff/1/chrome/browser/rlz/rlz.cc#new...
chrome/browser/rlz/rlz.cc:600: chrome_is_exiting = true;
Instead of a new global variable, I think we should be able to call
rlz_lib::SetURLRequestContext(NULL).

https://codereview.chromium.org/13868016/diff/1/chrome/browser/rlz/rlz.h
File chrome/browser/rlz/rlz.h (right):

https://codereview.chromium.org/13868016/diff/1/chrome/browser/rlz/rlz.h#newc...
chrome/browser/rlz/rlz.h:91: static void PostMainMessageLoopRun();
If we do need this method, better to rename this something like Shutdown(). 
PostMainMessageLoopRun() is descriptive of where this is currently called from
the chrome code today, not really what the function does.

https://codereview.chromium.org/13868016/diff/1/chrome/browser/rlz/rlz.h#newc...
chrome/browser/rlz/rlz.h:208: net::URLRequestContextGetter*
url_request_context_;
This member is unused.  Lets delete it?

[bcwhite, 2013-04-29 19:21:34]

https://codereview.chromium.org/13868016/diff/1/chrome/browser/chrome_browser...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/13868016/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_main.cc:1622:
RLZTracker::PostMainMessageLoopRun();
On 2013/04/29 16:56:45, Roger Tawa wrote:
> Browser shutdown already calls RLZTracker::CleanupRlz().  Instead of adding
this
> new method can we do the work in there?

Done.

https://codereview.chromium.org/13868016/diff/1/chrome/browser/rlz/rlz.cc
File chrome/browser/rlz/rlz.cc (right):

https://codereview.chromium.org/13868016/diff/1/chrome/browser/rlz/rlz.cc#new...
chrome/browser/rlz/rlz.cc:600: chrome_is_exiting = true;
The financial_ping code uses this value without checking if it is valid or not. 
I'll add the "abort" code there, instead.

https://codereview.chromium.org/13868016/diff/1/chrome/browser/rlz/rlz.h
File chrome/browser/rlz/rlz.h (right):

https://codereview.chromium.org/13868016/diff/1/chrome/browser/rlz/rlz.h#newc...
chrome/browser/rlz/rlz.h:91: static void PostMainMessageLoopRun();
I was trying to keep the name consistent with every other shutdown method, but
it doesn't matter as I've gone with the existing method.

https://codereview.chromium.org/13868016/diff/1/chrome/browser/rlz/rlz.h#newc...
chrome/browser/rlz/rlz.h:208: net::URLRequestContextGetter*
url_request_context_;
On 2013/04/29 16:56:45, Roger Tawa wrote:
> This member is unused.  Lets delete it?

Done.

[Roger Tawa OOO till Jul 10th, 2013-04-29 20:05:35]

lgtm

Thanks Brian!

[commit-bot: I haz the power, 2013-04-29 20:07:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bcwhite@chromium.org/13868016/28001

[commit-bot: I haz the power, 2013-04-30 01:30:36]

Change committed as 197214