Add the Windows.IsPinnedToTaskbar metric.

Add the Windows.IsPinnedToTaskbar metric.

This metric records the taskbar pinned state of Chrome for the current
user. Because the code loads shell extensions into the process, this
is done on a utility process.

BUG=621662
Committed: https://crrev.com/5057ca3b14b295bfb632538ab4e7e681790e5d9b
Cr-Commit-Position: refs/heads/master@{#403695}

[Patrick Monette, 2016-06-21 15:09:11]

Patchset #2 (id:20001) has been deleted

[Patrick Monette, 2016-06-21 15:09:28]

Patchset #1 (id:1) has been deleted

[Patrick Monette, 2016-06-21 15:19:27]

Patchset #1 (id:40001) has been deleted

[Patrick Monette, 2016-06-21 15:20:12]

Description was changed from

==========
Add the Windows.IsPinnedToTaskbar metric.

This metric records the taskbar pinned state of Chrome for the current
user.

BUG=621662
==========

to

==========
Add the Windows.IsPinnedToTaskbar metric.

This metric records the taskbar pinned state of Chrome for the current
user. Because the code loads shell extensions into the process, this is done on
a utility process.

BUG=621662
==========

[Patrick Monette, 2016-06-21 15:20:22]

Description was changed from

==========
Add the Windows.IsPinnedToTaskbar metric.

This metric records the taskbar pinned state of Chrome for the current
user. Because the code loads shell extensions into the process, this is done on
a utility process.

BUG=621662
==========

to

==========
Add the Windows.IsPinnedToTaskbar metric.

This metric records the taskbar pinned state of Chrome for the current
user. Because the code loads shell extensions into the process, this
is done on a utility process.

BUG=621662
==========

[Patrick Monette, 2016-06-21 15:25:17]

pmonette@chromium.org changed reviewers:
+ grt@chromium.org, mark@chromium.org

[Patrick Monette, 2016-06-21 15:25:18]

grt@ shell_integration_win.
mark@ base/
sky@ everything else under chrome/     
___chrome_browser_main_extra_parts_metrics.cc
___chrome/common/
___chrome/utility/

PTAL.

[Mark Mentovai, 2016-06-21 16:07:32]

LGTM in base

[grt (UTC plus 2), 2016-06-22 13:58:50]

https://codereview.chromium.org/2079233004/diff/60001/chrome/app/chromium_str...
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/2079233004/diff/60001/chrome/app/chromium_str...
chrome/app/chromium_strings.grd:1218: Shell Handler
since this string doesn't contain the product name, can you put it in
generated_resources.grd instead of these two files?

https://codereview.chromium.org/2079233004/diff/60001/chrome/browser/shell_in...
File chrome/browser/shell_integration_win.cc (right):

https://codereview.chromium.org/2079233004/diff/60001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:448:
UMA_HISTOGRAM_COUNTS("Windows.ShellHandlerProcessError", 1);
is it always the case that Chrome will either record IsPinnedToTaskbar OR
ShellHandlerProcessError? does it make sense to lump these into one tri-state so
they can be viewed together?

https://codereview.chromium.org/2079233004/diff/60001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:455: // executed on a utility process.
nit: on -> in

https://codereview.chromium.org/2079233004/diff/60001/chrome/utility/shell_ha...
File chrome/utility/shell_handler_impl_win.cc (right):

https://codereview.chromium.org/2079233004/diff/60001/chrome/utility/shell_ha...
chrome/utility/shell_handler_impl_win.cc:26: base::ScopedNativeLibrary
scoped_native_library(
is it possible to load it as a data file (LOAD_LIBRARY_AS_DATAFILE |
LOAD_LIBRARY_AS_IMAGE_RESOURCE) since no functions are used? this may be more
efficient; see msdn for more:
https://msdn.microsoft.com/library/windows/desktop/ms684179.aspx.

https://codereview.chromium.org/2079233004/diff/60001/chrome/utility/shell_ha...
chrome/utility/shell_handler_impl_win.cc:47: return false;
it seems that we won't be able to distinguish "not pinned" from "unexpected
error while checking for pin." i think it would be good to split these out.
wdyt?

[Patrick Monette, 2016-06-23 22:57:11]

Patchset #2 (id:80001) has been deleted

[Patrick Monette, 2016-06-23 22:58:52]

pmonette@chromium.org changed reviewers:
+ gab@chromium.org

[Patrick Monette, 2016-06-23 22:58:53]

Hey gab@, can you PTAL while grt@ is OOO?

https://codereview.chromium.org/2079233004/diff/60001/chrome/app/chromium_str...
File chrome/app/chromium_strings.grd (right):

https://codereview.chromium.org/2079233004/diff/60001/chrome/app/chromium_str...
chrome/app/chromium_strings.grd:1218: Shell Handler
On 2016/06/22 13:58:50, grt (no reviews Jun 23-Jul 11) wrote:
> since this string doesn't contain the product name, can you put it in
> generated_resources.grd instead of these two files?

Done.

https://codereview.chromium.org/2079233004/diff/60001/chrome/browser/shell_in...
File chrome/browser/shell_integration_win.cc (right):

https://codereview.chromium.org/2079233004/diff/60001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:448:
UMA_HISTOGRAM_COUNTS("Windows.ShellHandlerProcessError", 1);
On 2016/06/22 13:58:50, grt (no reviews Jun 23-Jul 11) wrote:
> is it always the case that Chrome will either record IsPinnedToTaskbar OR
> ShellHandlerProcessError? does it make sense to lump these into one tri-state
so
> they can be viewed together?

I can easily imagine that we'll add an histogram with the reason the process
crashed (with the name of the ShellHandler function called). I think it's
best left separated.

https://codereview.chromium.org/2079233004/diff/60001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:455: // executed on a utility process.
On 2016/06/22 13:58:50, grt (no reviews Jun 23-Jul 11) wrote:
> nit: on -> in

Done.

https://codereview.chromium.org/2079233004/diff/60001/chrome/utility/shell_ha...
File chrome/utility/shell_handler_impl_win.cc (right):

https://codereview.chromium.org/2079233004/diff/60001/chrome/utility/shell_ha...
chrome/utility/shell_handler_impl_win.cc:26: base::ScopedNativeLibrary
scoped_native_library(
On 2016/06/22 13:58:50, grt (no reviews Jun 23-Jul 11) wrote:
> is it possible to load it as a data file (LOAD_LIBRARY_AS_DATAFILE |
> LOAD_LIBRARY_AS_IMAGE_RESOURCE) since no functions are used? this may be more
> efficient; see msdn for more:
> https://msdn.microsoft.com/library/windows/desktop/ms684179.aspx.

Done. I am not sure of the difference between LOAD_LIBRARY_AS_DATAFILE and
LOAD_LIBRARY_AS_IMAGE_RESOURCE, but it works if I only specify the former.

https://codereview.chromium.org/2079233004/diff/60001/chrome/utility/shell_ha...
chrome/utility/shell_handler_impl_win.cc:47: return false;
On 2016/06/22 13:58:50, grt (no reviews Jun 23-Jul 11) wrote:
> it seems that we won't be able to distinguish "not pinned" from "unexpected
> error while checking for pin." i think it would be good to split these out.
> wdyt?

Done.

[gab, 2016-06-28 20:59:19]

lg but I'm not clear on the whole mojo thing (i.e. why ShellHandler doesn't use
mojo for its thing but ShellHandlerImpl does -- and how they are related?)

https://codereview.chromium.org/2079233004/diff/100001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2079233004/diff/100001/chrome/app/generated_r...
chrome/app/generated_resources.grd:15106: +      <message
name="IDS_UTILITY_PROCESS_SHELL_HANDLER_NAME" desc="The name of the utility
process used to handle shell operations.">
Why does this need to be localized?

https://codereview.chromium.org/2079233004/diff/100001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right):

https://codereview.chromium.org/2079233004/diff/100001/chrome/browser/metrics...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:154:
shell_integration::win::RecordIsPinnedToTaskbarHistogram();
Feels overkill to bounce to the BlockingPool only to have this post a task to
the IO thread. I think PostBrowserStart() below should post a delayed task
directly on the IO thread.

https://codereview.chromium.org/2079233004/diff/100001/chrome/browser/shell_i...
File chrome/browser/shell_integration_win.cc (right):

https://codereview.chromium.org/2079233004/diff/100001/chrome/browser/shell_i...
chrome/browser/shell_integration_win.cc:452:
UMA_HISTOGRAM_COUNTS("Windows.ShellHandlerProcessError", 1);
I think UMA_HISTOGRAM_BOOLEAN with enum="BooleanHit" is more appropriate for
this.

Or turn "Windows.IsPinnedToTaskbar.Succeeded" into
"Windows.IsPinnedToTaskbar.RemoteCallResult" with enum { CALL_SUCCEEEDED,
CALL_FAILED, CONNECTION_FAILED }

https://codereview.chromium.org/2079233004/diff/100001/chrome/browser/shell_i...
chrome/browser/shell_integration_win.cc:458: // The code to check if Chrome is
pinned to the taskbar is crashy so it is
s/is crashy/brings in shell extensions which can hinder stability/

https://codereview.chromium.org/2079233004/diff/100001/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

https://codereview.chromium.org/2079233004/diff/100001/chrome/chrome_common.g...
chrome/chrome_common.gypi:653: 'common/shell_handler_win.mojom',
Why does this need to be in common/? I would expect only browser/ stuff to ever
do shell operations.

https://codereview.chromium.org/2079233004/diff/100001/chrome/utility/shell_h...
File chrome/utility/shell_handler_impl_win.cc (right):

https://codereview.chromium.org/2079233004/diff/100001/chrome/utility/shell_h...
chrome/utility/shell_handler_impl_win.cc:32: bool error_occured() { return
error_occured_; }
nit: Empty line above

https://codereview.chromium.org/2079233004/diff/100001/chrome/utility/shell_h...
chrome/utility/shell_handler_impl_win.cc:58: base::string16
IsPinnedToTaskbarHelper::LoadShellResourceString(
Note: glanced over the code below but didn't fully re-review as I recall
reviewing in detail in earlier patch sets before it moved here and now assume
everything's been addressed (and more under grt's scrutiny :-)).

https://codereview.chromium.org/2079233004/diff/100001/chrome/utility/shell_h...
File chrome/utility/shell_handler_impl_win.h (right):

https://codereview.chromium.org/2079233004/diff/100001/chrome/utility/shell_h...
chrome/utility/shell_handler_impl_win.h:13: class ShellHandlerImpl : public
mojom::ShellHandler {
How is this related to "class ShellHandler"? I'm not clear on the relationship
between the two (why does one use mojo and not the other?) and therefore find it
weird that this is named as the "Impl" of the other?

https://codereview.chromium.org/2079233004/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2079233004/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:64126: +    This boolean indicates
whether Chrome is pinned to the Windows taskbar for
s/This boolean indicates/Indicates/ (type is already declared above)

(same below)

https://codereview.chromium.org/2079233004/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:64159: +    The count of connection
errors that happened between the browser and the
As mentioned above, this always reports, as such it's only a count on the
server-side, but this description aims to describe what it means for a given
user and as such BooleanHit is more appropriate.

[Patrick Monette, 2016-06-28 23:22:24]

pmonette@chromium.org changed reviewers:
+ asvitkine@chromium.org, thakis@chromium.org

[Patrick Monette, 2016-06-28 23:22:25]

Thanks gab, Another look?

thakis@, I need owners review for chrome/common/ and chrome/utility. Can you
take a look?
asvitkine@ PTAL at chrome_browser_main_extra_parts_metrics.cc and histograms.xml

https://codereview.chromium.org/2079233004/diff/100001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2079233004/diff/100001/chrome/app/generated_r...
chrome/app/generated_resources.grd:15106: +      <message
name="IDS_UTILITY_PROCESS_SHELL_HANDLER_NAME" desc="The name of the utility
process used to handle shell operations.">
On 2016/06/28 20:59:18, gab wrote:
> Why does this need to be localized?

The Chrome task manager displays this string.

https://codereview.chromium.org/2079233004/diff/100001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right):

https://codereview.chromium.org/2079233004/diff/100001/chrome/browser/metrics...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:154:
shell_integration::win::RecordIsPinnedToTaskbarHistogram();
On 2016/06/28 20:59:18, gab wrote:
> Feels overkill to bounce to the BlockingPool only to have this post a task to
> the IO thread. I think PostBrowserStart() below should post a delayed task
> directly on the IO thread.

Done.

https://codereview.chromium.org/2079233004/diff/100001/chrome/browser/shell_i...
File chrome/browser/shell_integration_win.cc (right):

https://codereview.chromium.org/2079233004/diff/100001/chrome/browser/shell_i...
chrome/browser/shell_integration_win.cc:452:
UMA_HISTOGRAM_COUNTS("Windows.ShellHandlerProcessError", 1);
On 2016/06/28 20:59:18, gab wrote:
> I think UMA_HISTOGRAM_BOOLEAN with enum="BooleanHit" is more appropriate for
> this.
> 
> Or turn "Windows.IsPinnedToTaskbar.Succeeded" into
> "Windows.IsPinnedToTaskbar.RemoteCallResult" with enum { CALL_SUCCEEEDED,
> CALL_FAILED, CONNECTION_FAILED }

Done.

https://codereview.chromium.org/2079233004/diff/100001/chrome/browser/shell_i...
chrome/browser/shell_integration_win.cc:458: // The code to check if Chrome is
pinned to the taskbar is crashy so it is
On 2016/06/28 20:59:18, gab wrote:
> s/is crashy/brings in shell extensions which can hinder stability/

Done.

https://codereview.chromium.org/2079233004/diff/100001/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

https://codereview.chromium.org/2079233004/diff/100001/chrome/chrome_common.g...
chrome/chrome_common.gypi:653: 'common/shell_handler_win.mojom',
On 2016/06/28 20:59:18, gab wrote:
> Why does this need to be in common/? I would expect only browser/ stuff to
ever
> do shell operations.

The mojom file is used by chrome/browser and chrome/utility (One implementer and
one caller).

https://codereview.chromium.org/2079233004/diff/100001/chrome/utility/shell_h...
File chrome/utility/shell_handler_impl_win.cc (right):

https://codereview.chromium.org/2079233004/diff/100001/chrome/utility/shell_h...
chrome/utility/shell_handler_impl_win.cc:32: bool error_occured() { return
error_occured_; }
On 2016/06/28 20:59:18, gab wrote:
> nit: Empty line above

Done.

https://codereview.chromium.org/2079233004/diff/100001/chrome/utility/shell_h...
chrome/utility/shell_handler_impl_win.cc:58: base::string16
IsPinnedToTaskbarHelper::LoadShellResourceString(
On 2016/06/28 20:59:18, gab wrote:
> Note: glanced over the code below but didn't fully re-review as I recall
> reviewing in detail in earlier patch sets before it moved here and now assume
> everything's been addressed (and more under grt's scrutiny :-)).

Sounds good but maybe you could review everything that touches "error_occured_"
that I added in the last patch set.

https://codereview.chromium.org/2079233004/diff/100001/chrome/utility/shell_h...
File chrome/utility/shell_handler_impl_win.h (right):

https://codereview.chromium.org/2079233004/diff/100001/chrome/utility/shell_h...
chrome/utility/shell_handler_impl_win.h:13: class ShellHandlerImpl : public
mojom::ShellHandler {
On 2016/06/28 20:59:18, gab wrote:
> How is this related to "class ShellHandler"? I'm not clear on the relationship
> between the two (why does one use mojo and not the other?) and therefore find
it
> weird that this is named as the "Impl" of the other?

ShellHandler is the deprecated IPC version that needs to be migrated to Mojo. It
won't exist anymore once the migration is finished. I agree the names are
confusing for now.

The "Impl" is a just a convention for classes that implement Mojo interfaces.

https://codereview.chromium.org/2079233004/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2079233004/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:64126: +    This boolean indicates
whether Chrome is pinned to the Windows taskbar for
On 2016/06/28 20:59:18, gab wrote:
> s/This boolean indicates/Indicates/ (type is already declared above)
> 
> (same below)

Done.

https://codereview.chromium.org/2079233004/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:64159: +    The count of connection
errors that happened between the browser and the
On 2016/06/28 20:59:18, gab wrote:
> As mentioned above, this always reports, as such it's only a count on the
> server-side, but this description aims to describe what it means for a given
> user and as such BooleanHit is more appropriate.

Done.

[Alexei Svitkine (slow), 2016-06-29 14:37:45]

https://codereview.chromium.org/2079233004/diff/120001/chrome/browser/shell_i...
File chrome/browser/shell_integration_win.cc (right):

https://codereview.chromium.org/2079233004/diff/120001/chrome/browser/shell_i...
chrome/browser/shell_integration_win.cc:439:
UMA_HISTOGRAM_BOOLEAN("Windows.IsPinnedToTaskbar.Succeeded", succeeded);
I don't see why you need multiple histograms. Just use a single histogram -
either logging true or false. Or you can make it an enum if you want more cases.

https://codereview.chromium.org/2079233004/diff/120001/chrome/browser/shell_i...
chrome/browser/shell_integration_win.cc:451: delete client;
Can the param be a scoped_ptr so you don't need to explicitly delete?

I guess it's not possible because of both success and error callback existing
and each needing to free it but only one will be called? That seems kind of
sub-optimal. Is it a limitation of Mojo?

Is there a way to use the same callback that owns the object with a
error/success param instead?

https://codereview.chromium.org/2079233004/diff/120001/chrome/browser/shell_i...
chrome/browser/shell_integration_win.cc:453:
UMA_HISTOGRAM_BOOLEAN("Windows.IsPinnedToTaskbar.ProcessError", true);
A UMA macro expands to a lot of code. Please make a helper function for a
histogram you plan to log from multiple places, rather than repeating the macro.

https://codereview.chromium.org/2079233004/diff/120001/chrome/utility/shell_h...
File chrome/utility/shell_handler_impl_win.cc (right):

https://codereview.chromium.org/2079233004/diff/120001/chrome/utility/shell_h...
chrome/utility/shell_handler_impl_win.cc:57: };
DISALLOW_COPY_AND_ASSIGN()?

https://codereview.chromium.org/2079233004/diff/120001/chrome/utility/shell_h...
chrome/utility/shell_handler_impl_win.cc:121: error_occured_ = true;
I would recommend making an enum with different error types so your histogram
has more info about what happened if an error occurred.

https://codereview.chromium.org/2079233004/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2079233004/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:64453: +    user.
Mention when this is logged. On start up?

[Patrick Monette, 2016-06-29 18:28:42]

https://codereview.chromium.org/2079233004/diff/120001/chrome/browser/shell_i...
File chrome/browser/shell_integration_win.cc (right):

https://codereview.chromium.org/2079233004/diff/120001/chrome/browser/shell_i...
chrome/browser/shell_integration_win.cc:439:
UMA_HISTOGRAM_BOOLEAN("Windows.IsPinnedToTaskbar.Succeeded", succeeded);
On 2016/06/29 14:37:44, Alexei Svitkine (very slow) wrote:
> I don't see why you need multiple histograms. Just use a single histogram -
> either logging true or false. Or you can make it an enum if you want more
cases.

Done. The process error is still separate because it will move somewhere else
when more users of the ShellHandler Mojo interface are added.

https://codereview.chromium.org/2079233004/diff/120001/chrome/browser/shell_i...
chrome/browser/shell_integration_win.cc:451: delete client;
On 2016/06/29 14:37:44, Alexei Svitkine (very slow) wrote:
> Can the param be a scoped_ptr so you don't need to explicitly delete?
> 
> I guess it's not possible because of both success and error callback existing
> and each needing to free it but only one will be called? That seems kind of
> sub-optimal. Is it a limitation of Mojo?
> 
> Is there a way to use the same callback that owns the object with a
> error/success param instead?

It needs to be 2 callbacks. In the general case, a Mojo interface can have
multiple functions with different parameters yet they share the same connection
error callback.

https://codereview.chromium.org/2079233004/diff/120001/chrome/browser/shell_i...
chrome/browser/shell_integration_win.cc:453:
UMA_HISTOGRAM_BOOLEAN("Windows.IsPinnedToTaskbar.ProcessError", true);
On 2016/06/29 14:37:44, Alexei Svitkine (very slow) wrote:
> A UMA macro expands to a lot of code. Please make a helper function for a
> histogram you plan to log from multiple places, rather than repeating the
macro.

Done.

https://codereview.chromium.org/2079233004/diff/120001/chrome/utility/shell_h...
File chrome/utility/shell_handler_impl_win.cc (right):

https://codereview.chromium.org/2079233004/diff/120001/chrome/utility/shell_h...
chrome/utility/shell_handler_impl_win.cc:57: };
On 2016/06/29 14:37:44, Alexei Svitkine (very slow) wrote:
> DISALLOW_COPY_AND_ASSIGN()?

Done.

https://codereview.chromium.org/2079233004/diff/120001/chrome/utility/shell_h...
chrome/utility/shell_handler_impl_win.cc:121: error_occured_ = true;
On 2016/06/29 14:37:44, Alexei Svitkine (very slow) wrote:
> I would recommend making an enum with different error types so your histogram
> has more info about what happened if an error occurred.

I don't expect this to ever fail so this is probably fine. If it ever starts
failing I'll add more metrics.

https://codereview.chromium.org/2079233004/diff/120001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2079233004/diff/120001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:64453: +    user.
On 2016/06/29 14:37:44, Alexei Svitkine (very slow) wrote:
> Mention when this is logged. On start up?

Done.

[Alexei Svitkine (slow), 2016-06-29 18:32:03]

lgtm

https://codereview.chromium.org/2079233004/diff/140001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2079233004/diff/140001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:64462: +    process.
Nit: Mention that this is logged every time the state is queried.

[Nico, 2016-06-29 20:02:32]

I'm not familiar with mojom stuff either; maybe someone who is should take a
look at the mojom bits. Should this have test coverage?

Generally looks good except the deps thing.

https://codereview.chromium.org/2079233004/diff/140001/chrome/utility/DEPS
File chrome/utility/DEPS (right):

https://codereview.chromium.org/2079233004/diff/140001/chrome/utility/DEPS#ne...
chrome/utility/DEPS:6: "+content/public/browser",
it seems like a dependency violation that chrome's utility process code depends
on content's browser process code

https://codereview.chromium.org/2079233004/diff/140001/chrome/utility/shell_h...
File chrome/utility/shell_handler_impl_win.cc (right):

https://codereview.chromium.org/2079233004/diff/140001/chrome/utility/shell_h...
chrome/utility/shell_handler_impl_win.cc:28: ~IsPinnedToTaskbarHelper() =
default;
why these two lines? that's what happens if you omit them as well, no?

[Patrick Monette, 2016-06-29 21:14:54]

Thanks Nico. I'll add someone familiar with Mojo.

About tests.. I don't think it's needed. I know it's not possible to pin a
shortcut on Windows 10. Maybe I could create a test that pins the unittest
executable that only runs on win7 and win8.

https://codereview.chromium.org/2079233004/diff/140001/chrome/utility/DEPS
File chrome/utility/DEPS (right):

https://codereview.chromium.org/2079233004/diff/140001/chrome/utility/DEPS#ne...
chrome/utility/DEPS:6: "+content/public/browser",
On 2016/06/29 20:02:31, Nico wrote:
> it seems like a dependency violation that chrome's utility process code
depends
> on content's browser process code

Removed. I don't remember why I added this but it's not needed.

https://codereview.chromium.org/2079233004/diff/140001/chrome/utility/shell_h...
File chrome/utility/shell_handler_impl_win.cc (right):

https://codereview.chromium.org/2079233004/diff/140001/chrome/utility/shell_h...
chrome/utility/shell_handler_impl_win.cc:28: ~IsPinnedToTaskbarHelper() =
default;
On 2016/06/29 20:02:31, Nico wrote:
> why these two lines? that's what happens if you omit them as well, no?

Removed the destructor. The constructor is needed because the
DISALLOW_COPY_AND_ASSIGN() prevents it from being implicitly defined.

https://codereview.chromium.org/2079233004/diff/140001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2079233004/diff/140001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:64462: +    process.
On 2016/06/29 18:32:02, Alexei Svitkine (very slow) wrote:
> Nit: Mention that this is logged every time the state is queried.

Done.

[Patrick Monette, 2016-06-29 21:17:04]

pmonette@chromium.org changed reviewers:
+ amistry@chromium.org

[Patrick Monette, 2016-06-29 21:17:05]

amistry@ Can you please take a look at the Mojo changes?
files:
shell_handler_win.mojom
shell_integration_win.cc
chrome_content_utility_client.cc
shell_handler_impl_win.cc
shell_handler_impl_win.h

[Nico, 2016-06-29 23:42:54]

lgtm once amistry likes it

[Anand Mistry (off Chromium), 2016-06-30 07:38:26]

lgtm

[gab, 2016-06-30 15:28:11]

lgtm w/ rename of old ShellHandler and one more comment

https://codereview.chromium.org/2079233004/diff/100001/chrome/utility/shell_h...
File chrome/utility/shell_handler_impl_win.cc (right):

https://codereview.chromium.org/2079233004/diff/100001/chrome/utility/shell_h...
chrome/utility/shell_handler_impl_win.cc:58: base::string16
IsPinnedToTaskbarHelper::LoadShellResourceString(
On 2016/06/28 23:22:25, Patrick Monette wrote:
> On 2016/06/28 20:59:18, gab wrote:
> > Note: glanced over the code below but didn't fully re-review as I recall
> > reviewing in detail in earlier patch sets before it moved here and now
assume
> > everything's been addressed (and more under grt's scrutiny :-)).
> 
> Sounds good but maybe you could review everything that touches
"error_occured_"
> that I added in the last patch set. 

error_occurred_ lgtm

https://codereview.chromium.org/2079233004/diff/100001/chrome/utility/shell_h...
File chrome/utility/shell_handler_impl_win.h (right):

https://codereview.chromium.org/2079233004/diff/100001/chrome/utility/shell_h...
chrome/utility/shell_handler_impl_win.h:13: class ShellHandlerImpl : public
mojom::ShellHandler {
On 2016/06/28 23:22:25, Patrick Monette wrote:
> On 2016/06/28 20:59:18, gab wrote:
> > How is this related to "class ShellHandler"? I'm not clear on the
relationship
> > between the two (why does one use mojo and not the other?) and therefore
find
> it
> > weird that this is named as the "Impl" of the other?
> 
> ShellHandler is the deprecated IPC version that needs to be migrated to Mojo.
It
> won't exist anymore once the migration is finished. I agree the names are
> confusing for now.
> 
> The "Impl" is a just a convention for classes that implement Mojo interfaces.

Got it, can you rename the old one then? IPCShellHandler or something with a
TODO to migrate it?

https://codereview.chromium.org/2079233004/diff/160001/chrome/browser/shell_i...
File chrome/browser/shell_integration_win.h (right):

https://codereview.chromium.org/2079233004/diff/160001/chrome/browser/shell_i...
chrome/browser/shell_integration_win.h:60: // Records the pinned state of the
current executable into a histogram.
Document that this must be called on the IO thread.

[Patrick Monette, 2016-06-30 15:55:58]

Thanks!

[Patrick Monette, 2016-06-30 15:56:05]

The CQ bit was checked by pmonette@chromium.org

[Patrick Monette, 2016-06-30 15:56:06]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org,
asvitkine@chromium.org, gab@chromium.org, thakis@chromium.org,
amistry@chromium.org
Link to the patchset: https://codereview.chromium.org/2079233004/#ps180001
(title: "Renamed ShellHandler to IPCShellHandler")

[commit-bot: I haz the power, 2016-06-30 15:56:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-30 16:03:43]

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

[commit-bot: I haz the power, 2016-06-30 16:03:45]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Patrick Monette, 2016-06-30 17:29:05]

The CQ bit was checked by pmonette@chromium.org

[Patrick Monette, 2016-06-30 17:29:06]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
mark@chromium.org, thakis@chromium.org, asvitkine@chromium.org,
amistry@chromium.org
Link to the patchset: https://codereview.chromium.org/2079233004/#ps200001
(title: "Fix compilation")

[commit-bot: I haz the power, 2016-06-30 17:29:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-30 17:38:42]

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

[commit-bot: I haz the power, 2016-06-30 17:38:44]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Patrick Monette, 2016-06-30 17:52:34]

pmonette@chromium.org changed reviewers:
+ dcheng@chromium.org

[Patrick Monette, 2016-06-30 17:52:35]

dcheng@ Can you review shell_handler_win.mojom for IPC security?

[dcheng, 2016-07-01 05:46:56]

mojom lgtm

https://codereview.chromium.org/2079233004/diff/200001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right):

https://codereview.chromium.org/2079233004/diff/200001/chrome/browser/metrics...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:362:
base::TimeDelta::FromSeconds(kStartupMetricsGatheringDelaySeconds));
Nit: I think kStartupMetricsGatheringDelaySeconds can just be a base::TimeDelta
directly now: https://codereview.chromium.org/1976703005

https://codereview.chromium.org/2079233004/diff/200001/chrome/browser/shell_i...
File chrome/browser/shell_integration_win.cc (right):

https://codereview.chromium.org/2079233004/diff/200001/chrome/browser/shell_i...
chrome/browser/shell_integration_win.cc:440: delete client;
Hmm… this feels a bit sketchy.

I don't think it should block this CL, but we should figure out a better way to
do ownership for this that doesn't depend on manually deletes like this.

[Patrick Monette, 2016-07-04 15:36:15]

Thanks!

https://codereview.chromium.org/2079233004/diff/200001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right):

https://codereview.chromium.org/2079233004/diff/200001/chrome/browser/metrics...
chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:362:
base::TimeDelta::FromSeconds(kStartupMetricsGatheringDelaySeconds));
On 2016/07/01 05:46:55, dcheng wrote:
> Nit: I think kStartupMetricsGatheringDelaySeconds can just be a
base::TimeDelta
> directly now: https://codereview.chromium.org/1976703005

Done.

https://codereview.chromium.org/2079233004/diff/200001/chrome/browser/shell_i...
File chrome/browser/shell_integration_win.cc (right):

https://codereview.chromium.org/2079233004/diff/200001/chrome/browser/shell_i...
chrome/browser/shell_integration_win.cc:440: delete client;
On 2016/07/01 05:46:55, dcheng wrote:
> Hmm… this feels a bit sketchy.
> 
> I don't think it should block this CL, but we should figure out a better way
to
> do ownership for this that doesn't depend on manually deletes like this.

Acknowledged. I'm working on it on another CL where I add more functions to
shell_handler_win.mojom.

[Patrick Monette, 2016-07-04 15:36:40]

The CQ bit was checked by pmonette@chromium.org

[Patrick Monette, 2016-07-04 15:36:41]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
dcheng@chromium.org, mark@chromium.org, thakis@chromium.org,
asvitkine@chromium.org, amistry@chromium.org
Link to the patchset: https://codereview.chromium.org/2079233004/#ps220001
(title: "Constexpr TimeDelta")

[commit-bot: I haz the power, 2016-07-04 15:36:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-04 16:48:55]

Description was changed from

==========
Add the Windows.IsPinnedToTaskbar metric.

This metric records the taskbar pinned state of Chrome for the current
user. Because the code loads shell extensions into the process, this
is done on a utility process.

BUG=621662
==========

to

==========
Add the Windows.IsPinnedToTaskbar metric.

This metric records the taskbar pinned state of Chrome for the current
user. Because the code loads shell extensions into the process, this
is done on a utility process.

BUG=621662
==========

[commit-bot: I haz the power, 2016-07-04 16:48:57]

Committed patchset #8 (id:220001)

[commit-bot: I haz the power, 2016-07-04 16:49:02]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-04 16:51:09]

Description was changed from

==========
Add the Windows.IsPinnedToTaskbar metric.

This metric records the taskbar pinned state of Chrome for the current
user. Because the code loads shell extensions into the process, this
is done on a utility process.

BUG=621662
==========

to

==========
Add the Windows.IsPinnedToTaskbar metric.

This metric records the taskbar pinned state of Chrome for the current
user. Because the code loads shell extensions into the process, this
is done on a utility process.

BUG=621662

Committed: https://crrev.com/5057ca3b14b295bfb632538ab4e7e681790e5d9b
Cr-Commit-Position: refs/heads/master@{#403695}
==========

[commit-bot: I haz the power, 2016-07-04 16:51:10]

Patchset 8 (id:??) landed as
https://crrev.com/5057ca3b14b295bfb632538ab4e7e681790e5d9b
Cr-Commit-Position: refs/heads/master@{#403695}

[grt (UTC plus 2), 2016-07-14 07:39:35]

post-commit fyi

https://codereview.chromium.org/2079233004/diff/60001/chrome/utility/shell_ha...
File chrome/utility/shell_handler_impl_win.cc (right):

https://codereview.chromium.org/2079233004/diff/60001/chrome/utility/shell_ha...
chrome/utility/shell_handler_impl_win.cc:26: base::ScopedNativeLibrary
scoped_native_library(
On 2016/06/23 22:58:53, Patrick Monette wrote:
> On 2016/06/22 13:58:50, grt (no reviews Jun 23-Jul 11) wrote:
> > is it possible to load it as a data file (LOAD_LIBRARY_AS_DATAFILE |
> > LOAD_LIBRARY_AS_IMAGE_RESOURCE) since no functions are used? this may be
more
> > efficient; see msdn for more:
> > https://msdn.microsoft.com/library/windows/desktop/ms684179.aspx.
> 
> Done. I am not sure of the difference between LOAD_LIBRARY_AS_DATAFILE and
> LOAD_LIBRARY_AS_IMAGE_RESOURCE, but it works if I only specify the former.

Apologies for not being more clear: my suggestion was to use the two flags
together as per MSDN: "Unless an application depends on specific image mapping
characteristics, the LOAD_LIBRARY_AS_IMAGE_RESOURCE value should be used with
either LOAD_LIBRARY_AS_DATAFILE_EXCLUSIVE or LOAD_LIBRARY_AS_DATAFILE." Also
"When the LOAD_LIBRARY_AS_IMAGE_RESOURCE value is used...resources can be more
quickly retrieved from the module." The performance is a non-issue since only
one resource is read, but I think it makes sense to follow the recommendation
anyway.