Fix tab discarding reload count metric.

Fix tab discarding reload count metric.

BUG=514340
Committed: https://crrev.com/9339aa5dc5758d59ce6688a7df23b1b18bda9f44
Cr-Commit-Position: refs/heads/master@{#351131}

[Georges Khalil, 2015-09-25 16:48:19]

georgesak@chromium.org changed reviewers:
+ chrisha@chromium.org

[Georges Khalil, 2015-09-25 16:48:40]

Chris, PTAL.

[chrisha, 2015-09-25 17:11:53]

https://codereview.chromium.org/1366203003/diff/1/chrome/browser/ui/tabs/tab_...
File chrome/browser/ui/tabs/tab_discard_state.cc (right):

https://codereview.chromium.org/1366203003/diff/1/chrome/browser/ui/tabs/tab_...
chrome/browser/ui/tabs/tab_discard_state.cc:46: TabDiscardState* discard_state =
TabDiscardState::Get(web_contents);
Does this object only get called from a single thread?

Maybe add a thread-checker to make sure? Otherwise there's still a race
condition.

[Georges Khalil, 2015-09-25 18:36:25]

Chris, PTAnL.

https://codereview.chromium.org/1366203003/diff/1/chrome/browser/ui/tabs/tab_...
File chrome/browser/ui/tabs/tab_discard_state.cc (right):

https://codereview.chromium.org/1366203003/diff/1/chrome/browser/ui/tabs/tab_...
chrome/browser/ui/tabs/tab_discard_state.cc:46: TabDiscardState* discard_state =
TabDiscardState::Get(web_contents);
On 2015/09/25 17:11:52, chrisha wrote:
> Does this object only get called from a single thread?
> 
> Maybe add a thread-checker to make sure? Otherwise there's still a race
> condition.

Should all be on the UI thread.

Added thread checker just in case.

[chrisha, 2015-09-28 15:07:49]

https://codereview.chromium.org/1366203003/diff/20001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_discard_state.cc (right):

https://codereview.chromium.org/1366203003/diff/20001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_discard_state.cc:18: base::ThreadChecker
thread_checker;
The thread check remembers the thread on which it was constructed. This is a
static so it'll happen on the main thread in the C++ initializers. The general
pattern when using a thread_checker is to make it a member variable, but I just
realized that won't work for you, as the whole class has a 'static' interface.

Maybe just use BrowserThreads::CurrentlyOn and the associated DCHECKs?

https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...

[Georges Khalil, 2015-09-28 18:30:42]

Patchset #3 (id:40001) has been deleted

[Georges Khalil, 2015-09-28 18:41:13]

georgesak@chromium.org changed reviewers:
+ sky@chromium.org

[Georges Khalil, 2015-09-28 18:41:13]

chrisha@, PTAnL.

sky@, PTAL for owner approval.

https://codereview.chromium.org/1366203003/diff/20001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_discard_state.cc (right):

https://codereview.chromium.org/1366203003/diff/20001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_discard_state.cc:18: base::ThreadChecker
thread_checker;
On 2015/09/28 15:07:49, chrisha wrote:
> The thread check remembers the thread on which it was constructed. This is a
> static so it'll happen on the main thread in the C++ initializers. The general
> pattern when using a thread_checker is to make it a member variable, but I
just
> realized that won't work for you, as the whole class has a 'static' interface.
> 
> Maybe just use BrowserThreads::CurrentlyOn and the associated DCHECKs?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...

I was under the (wrong) impression that it remembers the thread on the first
call to CalledOnValidThread, my bad.

Changed to DCHECK_CURRENTLY_ON(content::BrowserThread::UI) instead.

[chrisha, 2015-09-28 18:50:13]

lgtm

[sky, 2015-09-28 19:13:00]

LGTM

[Georges Khalil, 2015-09-28 19:13:30]

The CQ bit was checked by georgesak@chromium.org

[commit-bot: I haz the power, 2015-09-28 19:14:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1366203003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1366203003/60001

[commit-bot: I haz the power, 2015-09-28 20:50:38]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-09-28 20:51:33]

Patchset 3 (id:??) landed as
https://crrev.com/9339aa5dc5758d59ce6688a7df23b1b18bda9f44
Cr-Commit-Position: refs/heads/master@{#351131}