Modify fileAtPath stat to track if the call was redirected by chrome_elf.

Modify fileAtPath stat to track if the call was redirected by chrome_elf.

TEST=Manual: Start chrome.exe, wait ~60s and check chrome://histograms, the Stability.FileAtPath metric should have run (at least) once, and the resulting value should be 6, assuming the check succeeded.

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

[Cait (Slow), 2014-02-18 18:47:18]

Robert: PTAL at chrome_elf changes.
Gab: PTAL at file_path_verifier_win changes. Does adding the 3 new values like
this make sense (and do I need to add them elsewhere as well?)

Thanks!

[robertshield, 2014-02-18 19:36:03]

Looks awesome, minor suggestion re bool -> count

https://codereview.chromium.org/169093007/diff/30001/chrome_elf/create_file/c...
File chrome_elf/create_file/chrome_create_file.cc (right):

https://codereview.chromium.org/169093007/diff/30001/chrome_elf/create_file/c...
chrome_elf/create_file/chrome_create_file.cc:15: bool g_last_call_was_redirected
= false;
I think this should be a monotonically increasing count instead.

https://codereview.chromium.org/169093007/diff/30001/chrome_elf/create_file/c...
File chrome_elf/create_file/chrome_create_file.h (right):

https://codereview.chromium.org/169093007/diff/30001/chrome_elf/create_file/c...
chrome_elf/create_file/chrome_create_file.h:23: // Returns if true if the last
call to CreateFile was redirected.
s/Returns if true/Returns true

[gab, 2014-02-18 19:43:10]

https://codereview.chromium.org/169093007/diff/30001/chrome/browser/profiles/...
File chrome/browser/profiles/file_path_verifier_win.cc (right):

https://codereview.chromium.org/169093007/diff/30001/chrome/browser/profiles/...
chrome/browser/profiles/file_path_verifier_win.cc:43: bool was_redirected =
LastCallWasRedirected();
LastCallWasRedirected() is a bit vague out-of-chrome-elf-context here.

Should chrome_elf get a namespace?

(adding thoughts as I read more!) Ah nvm this is actually using a DLL import;
then add a "::" prefix as we do for Windows API calls to show that this is
calling an external method.

https://codereview.chromium.org/169093007/diff/30001/chrome/browser/profiles/...
chrome/browser/profiles/file_path_verifier_win.cc:90: bool was_redirected =
LastCallWasRedirected();
unused?

https://codereview.chromium.org/169093007/diff/30001/chrome_elf/create_file/c...
File chrome_elf/create_file/chrome_create_file.cc (right):

https://codereview.chromium.org/169093007/diff/30001/chrome_elf/create_file/c...
chrome_elf/create_file/chrome_create_file.cc:15: bool g_last_call_was_redirected
= false;
On 2014/02/18 19:36:03, robertshield wrote:
> I think this should be a monotonically increasing count instead.

And if you want to go deeper, you could have a periodic task in Chrome looking
for that count to tick and reporting UMA for it.

[robertshield, 2014-02-18 19:46:22]

https://codereview.chromium.org/169093007/diff/30001/chrome_elf/create_file/c...
File chrome_elf/create_file/chrome_create_file.cc (right):

https://codereview.chromium.org/169093007/diff/30001/chrome_elf/create_file/c...
chrome_elf/create_file/chrome_create_file.cc:15: bool g_last_call_was_redirected
= false;
On 2014/02/18 19:43:11, gab wrote:
> On 2014/02/18 19:36:03, robertshield wrote:
> > I think this should be a monotonically increasing count instead.
> 
> And if you want to go deeper, you could have a periodic task in Chrome looking
> for that count to tick and reporting UMA for it.

We could, but we're really interested in seeing this increment across specific
events.

[Cait (Slow), 2014-02-18 23:17:17]

Thanks! Switched to a count as discussed.

https://codereview.chromium.org/169093007/diff/30001/chrome/browser/profiles/...
File chrome/browser/profiles/file_path_verifier_win.cc (right):

https://codereview.chromium.org/169093007/diff/30001/chrome/browser/profiles/...
chrome/browser/profiles/file_path_verifier_win.cc:43: bool was_redirected =
LastCallWasRedirected();
On 2014/02/18 19:43:11, gab wrote:
> LastCallWasRedirected() is a bit vague out-of-chrome-elf-context here.
> 
> Should chrome_elf get a namespace?
> 
> (adding thoughts as I read more!) Ah nvm this is actually using a DLL import;
> then add a "::" prefix as we do for Windows API calls to show that this is
> calling an external method.

Done.

https://codereview.chromium.org/169093007/diff/30001/chrome/browser/profiles/...
chrome/browser/profiles/file_path_verifier_win.cc:90: bool was_redirected =
LastCallWasRedirected();
On 2014/02/18 19:43:11, gab wrote:
> unused?

Done.

https://codereview.chromium.org/169093007/diff/30001/chrome_elf/create_file/c...
File chrome_elf/create_file/chrome_create_file.cc (right):

https://codereview.chromium.org/169093007/diff/30001/chrome_elf/create_file/c...
chrome_elf/create_file/chrome_create_file.cc:15: bool g_last_call_was_redirected
= false;
On 2014/02/18 19:46:22, robertshield wrote:
> On 2014/02/18 19:43:11, gab wrote:
> > On 2014/02/18 19:36:03, robertshield wrote:
> > > I think this should be a monotonically increasing count instead.
> > 
> > And if you want to go deeper, you could have a periodic task in Chrome
looking
> > for that count to tick and reporting UMA for it.
> 
> We could, but we're really interested in seeing this increment across specific
> events.
I'll switch it to a count, when we can use to check for specific events, that
way if we ever need a running tally it will only require adding an UMA stat,
chrome-side.

https://codereview.chromium.org/169093007/diff/30001/chrome_elf/create_file/c...
File chrome_elf/create_file/chrome_create_file.h (right):

https://codereview.chromium.org/169093007/diff/30001/chrome_elf/create_file/c...
chrome_elf/create_file/chrome_create_file.h:23: // Returns if true if the last
call to CreateFile was redirected.
On 2014/02/18 19:36:03, robertshield wrote:
> s/Returns if true/Returns true

Done.

[robertshield, 2014-02-18 23:30:20]

https://codereview.chromium.org/169093007/diff/110001/chrome/browser/profiles...
File chrome/browser/profiles/file_path_verifier_win.cc (right):

https://codereview.chromium.org/169093007/diff/110001/chrome/browser/profiles...
chrome/browser/profiles/file_path_verifier_win.cc:35: int redirect_count_before
= ::GetRedirectCount();
Sorry for not noticing this earlier, this won't call the version of
GetRedirectCount in chrome_elf, rather it will call the one statically linked
into chrome.dll. 

See
https://codereview.chromium.org/166953002/diff/160001/chrome/renderer/chrome_...
for an example of invoking the one exposed by chrome_elf.

https://codereview.chromium.org/169093007/diff/110001/chrome_elf/create_file/...
File chrome_elf/create_file/chrome_create_file.cc (right):

https://codereview.chromium.org/169093007/diff/110001/chrome_elf/create_file/...
chrome_elf/create_file/chrome_create_file.cc:15: int g_redirect_count = 0;
This is only used in this file, it can be moved into the anonymous namespace.

[Cait (Slow), 2014-02-19 15:43:18]

https://codereview.chromium.org/169093007/diff/110001/chrome/browser/profiles...
File chrome/browser/profiles/file_path_verifier_win.cc (right):

https://codereview.chromium.org/169093007/diff/110001/chrome/browser/profiles...
chrome/browser/profiles/file_path_verifier_win.cc:35: int redirect_count_before
= ::GetRedirectCount();
On 2014/02/18 23:30:21, robertshield wrote:
> Sorry for not noticing this earlier, this won't call the version of
> GetRedirectCount in chrome_elf, rather it will call the one statically linked
> into chrome.dll. 
> 
> See
>
https://codereview.chromium.org/166953002/diff/160001/chrome/renderer/chrome_...
> for an example of invoking the one exposed by chrome_elf.

As discussed w/Robert -- I think we're ok here since this f'n is exported via
the .def file.

https://codereview.chromium.org/169093007/diff/110001/chrome_elf/create_file/...
File chrome_elf/create_file/chrome_create_file.cc (right):

https://codereview.chromium.org/169093007/diff/110001/chrome_elf/create_file/...
chrome_elf/create_file/chrome_create_file.cc:15: int g_redirect_count = 0;
On 2014/02/18 23:30:21, robertshield wrote:
> This is only used in this file, it can be moved into the anonymous namespace.

Done.

[gab, 2014-02-19 15:43:40]

https://codereview.chromium.org/169093007/diff/110001/chrome/browser/profiles...
File chrome/browser/profiles/file_path_verifier_win.cc (right):

https://codereview.chromium.org/169093007/diff/110001/chrome/browser/profiles...
chrome/browser/profiles/file_path_verifier_win.cc:35: int redirect_count_before
= ::GetRedirectCount();
On 2014/02/18 23:30:21, robertshield wrote:
> Sorry for not noticing this earlier, this won't call the version of
> GetRedirectCount in chrome_elf, rather it will call the one statically linked
> into chrome.dll. 
> 
> See
>
https://codereview.chromium.org/166953002/diff/160001/chrome/renderer/chrome_...
> for an example of invoking the one exposed by chrome_elf.

chrome_elf methods are statically linked in chrome.dll? I'm confused, please
enlighten me :)!

https://codereview.chromium.org/169093007/diff/170001/chrome/browser/profiles...
File chrome/browser/profiles/file_path_verifier_win.cc (right):

https://codereview.chromium.org/169093007/diff/170001/chrome/browser/profiles...
chrome/browser/profiles/file_path_verifier_win.cc:26:
FILE_VERIFICATION_FAILED_SAMEDIR_REDIRECT,
Also update the enum in histograms.xml to include these (and tell csharp to also
update his histograms to display these values).

https://codereview.chromium.org/169093007/diff/170001/chrome_elf/create_file/...
File chrome_elf/create_file/chrome_create_file.cc (right):

https://codereview.chromium.org/169093007/diff/170001/chrome_elf/create_file/...
chrome_elf/create_file/chrome_create_file.cc:98: g_redirect_count++;
nit: pre-increment

[robertshield, 2014-02-19 16:17:01]

lgtm

https://codereview.chromium.org/169093007/diff/110001/chrome/browser/profiles...
File chrome/browser/profiles/file_path_verifier_win.cc (right):

https://codereview.chromium.org/169093007/diff/110001/chrome/browser/profiles...
chrome/browser/profiles/file_path_verifier_win.cc:35: int redirect_count_before
= ::GetRedirectCount();
On 2014/02/19 15:43:40, gab wrote:
> On 2014/02/18 23:30:21, robertshield wrote:
> > Sorry for not noticing this earlier, this won't call the version of
> > GetRedirectCount in chrome_elf, rather it will call the one statically
linked
> > into chrome.dll. 
> > 
> > See
> >
>
https://codereview.chromium.org/166953002/diff/160001/chrome/renderer/chrome_...
> > for an example of invoking the one exposed by chrome_elf.
> 
> chrome_elf methods are statically linked in chrome.dll? I'm confused, please
> enlighten me :)!

This was a brain fart on my part. For some reason I'd thought that we were
linking this in by depending on a .lib which would cause this method to modify
the linked in copy of g_redirect_count. 

This belief is incorrect as the dependency in the gyp file is on a .dll target
and since the method is found in the exports section of the target's .def file,
the call here actually invokes the function in the dll. So all is well.

[Cait (Slow), 2014-02-19 16:28:05]

[+noms]

noms: can I haz OWNERS approval for c/b/profiles/file_path_verifier_win.cc
changes?

Thanks!

https://codereview.chromium.org/169093007/diff/170001/chrome/browser/profiles...
File chrome/browser/profiles/file_path_verifier_win.cc (right):

https://codereview.chromium.org/169093007/diff/170001/chrome/browser/profiles...
chrome/browser/profiles/file_path_verifier_win.cc:26:
FILE_VERIFICATION_FAILED_SAMEDIR_REDIRECT,
On 2014/02/19 15:43:40, gab wrote:
> Also update the enum in histograms.xml to include these (and tell csharp to
also
> update his histograms to display these values).

Done.

https://codereview.chromium.org/169093007/diff/170001/chrome_elf/create_file/...
File chrome_elf/create_file/chrome_create_file.cc (right):

https://codereview.chromium.org/169093007/diff/170001/chrome_elf/create_file/...
chrome_elf/create_file/chrome_create_file.cc:98: g_redirect_count++;
On 2014/02/19 15:43:40, gab wrote:
> nit: pre-increment

Done.

[noms (inactive), 2014-02-19 16:48:13]

You can has lgtm with a nit (which you can ignore if you want).

https://codereview.chromium.org/169093007/diff/240001/chrome/browser/profiles...
File chrome/browser/profiles/file_path_verifier_win.cc (right):

https://codereview.chromium.org/169093007/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/file_path_verifier_win.cc:44: bool was_redirected =
::GetRedirectCount() > redirect_count_before;
nit: If possible, I would add a small comment here about why GetRedirectCount()
might return a different value from the last time it was called.

[gab, 2014-02-19 18:11:28]

lgtm

Remove BUG= line if you don't have a bug# and a TEST= line specifying how you
verified this change works locally.

Cheers,
Gab

[Cait (Slow), 2014-02-19 18:58:09]

Thanks all!

https://codereview.chromium.org/169093007/diff/240001/chrome/browser/profiles...
File chrome/browser/profiles/file_path_verifier_win.cc (right):

https://codereview.chromium.org/169093007/diff/240001/chrome/browser/profiles...
chrome/browser/profiles/file_path_verifier_win.cc:44: bool was_redirected =
::GetRedirectCount() > redirect_count_before;
On 2014/02/19 16:48:13, Monica Dinculescu wrote:
> nit: If possible, I would add a small comment here about why
GetRedirectCount()
> might return a different value from the last time it was called.

Done.

[Cait (Slow), 2014-02-19 18:58:14]

The CQ bit was checked by caitkp@chromium.org

[commit-bot: I haz the power, 2014-02-19 19:04:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caitkp@chromium.org/169093007/320001

[commit-bot: I haz the power, 2014-02-19 19:41:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-19 19:41:22]

Try jobs failed on following builders: linux_chromium_rel, mac_chromium_rel

[Cait (Slow), 2014-02-19 20:24:43]

+sky for chrome/browser/DEPS change -- thanks!

[sky, 2014-02-19 21:25:40]

DEPS LGTM

[Cait (Slow), 2014-02-19 21:26:48]

The CQ bit was checked by caitkp@chromium.org

[commit-bot: I haz the power, 2014-02-19 21:40:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caitkp@chromium.org/169093007/320001

[commit-bot: I haz the power, 2014-02-19 22:11:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-19 22:12:00]

Try jobs failed on following builders: linux_chromium_rel, mac_chromium_rel

[Cait (Slow), 2014-02-19 22:14:13]

The CQ bit was checked by caitkp@chromium.org

[commit-bot: I haz the power, 2014-02-19 22:50:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caitkp@chromium.org/169093007/320001

[commit-bot: I haz the power, 2014-02-19 23:15:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-19 23:15:41]

Try jobs failed on following builders: linux_chromium_rel, mac_chromium_rel

[robertshield, 2014-02-24 18:19:18]

still lgtm

[Cait (Slow), 2014-02-24 22:06:34]

The CQ bit was checked by caitkp@chromium.org

[commit-bot: I haz the power, 2014-02-24 22:07:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caitkp@chromium.org/169093007/720001

[commit-bot: I haz the power, 2014-02-25 01:32:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caitkp@chromium.org/169093007/720001

[commit-bot: I haz the power, 2014-02-25 05:06:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-25 05:06:31]

Try jobs failed on following builders: mac_chromium_rel

[Cait (Slow), 2014-02-25 13:45:55]

The CQ bit was checked by caitkp@chromium.org

[commit-bot: I haz the power, 2014-02-25 13:49:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caitkp@chromium.org/169093007/720001

[commit-bot: I haz the power, 2014-02-25 13:52:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-25 13:52:17]

Try jobs failed on following builders: mac_chromium_rel

[Cait (Slow), 2014-02-25 21:19:07]

The CQ bit was checked by caitkp@chromium.org

[commit-bot: I haz the power, 2014-02-25 21:19:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caitkp@chromium.org/169093007/720001

[commit-bot: I haz the power, 2014-02-25 21:22:23]

Change committed as 253251