DefaultBrowserWorker now fully supports opening the settings for Win10

DefaultBrowserWorker now fully supports opening the settings for Win10

The worker uses a new SetAsDefaultBrowser style function that manages
the user interaction with the system settings.

BUG=576490
Committed: https://crrev.com/32a5cfb4e2c2db333e007fbba67d35bd9fa664bd
Cr-Commit-Position: refs/heads/master@{#386487}

[Patrick Monette, 2016-03-24 18:11:22]

Description was changed from

==========
DefaultBrowserWorker now fully supports opening the settings for Win10

The worker uses a new SetAsDefaultBrowser style function that manages
the user interaction with the system settingts.

BUG=576490
==========

to

==========
DefaultBrowserWorker now fully supports opening the settings for Win10

The worker uses a new SetAsDefaultBrowser style function that manages
the user interaction with the system settings.

BUG=576490
==========

[Patrick Monette, 2016-03-24 18:11:38]

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

[Patrick Monette, 2016-03-24 18:11:38]

PTAL

[grt (UTC plus 2), 2016-03-29 16:45:03]

https://codereview.chromium.org/1832853003/diff/1/chrome/browser/shell_integr...
File chrome/browser/shell_integration.cc (right):

https://codereview.chromium.org/1832853003/diff/1/chrome/browser/shell_integr...
chrome/browser/shell_integration.cc:51: }
should this also:
  on_finished_callback.Run();
since the fn is described as unconditionally running the callback once?

https://codereview.chromium.org/1832853003/diff/1/chrome/browser/shell_integr...
chrome/browser/shell_integration.cc:260: #if defined(OS_WIN)
can this not be removed? i think it's nice to reduce the compile-time
conditionals to the extent possible.

https://codereview.chromium.org/1832853003/diff/1/chrome/browser/shell_integr...
File chrome/browser/shell_integration.h (right):

https://codereview.chromium.org/1832853003/diff/1/chrome/browser/shell_integr...
chrome/browser/shell_integration.h:40: void
SetAsDefaultBrowserUsingSystemSettings(base::Closure on_finished_callback);
nit: pass callbacks by const-ref

https://codereview.chromium.org/1832853003/diff/1/chrome/browser/shell_integr...
File chrome/browser/shell_integration_win.cc (right):

https://codereview.chromium.org/1832853003/diff/1/chrome/browser/shell_integr...
chrome/browser/shell_integration_win.cc:230: // This helper class manages both
the timer and the registry watchers and make
nit: make -> makes

https://codereview.chromium.org/1832853003/diff/1/chrome/browser/shell_integr...
chrome/browser/shell_integration_win.cc:275: if (reg_key && reg_key->Valid()) {
no need to test reg_key alone since it will always be valid -- the process will
crash on out-of-memory.

https://codereview.chromium.org/1832853003/diff/1/chrome/browser/shell_integr...
chrome/browser/shell_integration_win.cc:296: void
ConcludeOpenSystemSettingsToDefaultBrowser(
this function is tightly coupled to the helper. can it be a private member
function of it?

https://codereview.chromium.org/1832853003/diff/1/chrome/browser/shell_integr...
chrome/browser/shell_integration_win.cc:298: base::Callback<void()>
on_finished_callback,
const base::Closure&

[Patrick Monette, 2016-03-29 17:43:08]

Thanks for the comments PTAnL

https://codereview.chromium.org/1832853003/diff/1/chrome/browser/shell_integr...
File chrome/browser/shell_integration.cc (right):

https://codereview.chromium.org/1832853003/diff/1/chrome/browser/shell_integr...
chrome/browser/shell_integration.cc:51: }
On 2016/03/29 16:45:02, grt (very slow) wrote:
> should this also:
>   on_finished_callback.Run();
> since the fn is described as unconditionally running the callback once?

Done.

https://codereview.chromium.org/1832853003/diff/1/chrome/browser/shell_integr...
chrome/browser/shell_integration.cc:260: #if defined(OS_WIN)
On 2016/03/29 16:45:02, grt (very slow) wrote:
> can this not be removed? i think it's nice to reduce the compile-time
> conditionals to the extent possible.

Done.

https://codereview.chromium.org/1832853003/diff/1/chrome/browser/shell_integr...
File chrome/browser/shell_integration.h (right):

https://codereview.chromium.org/1832853003/diff/1/chrome/browser/shell_integr...
chrome/browser/shell_integration.h:40: void
SetAsDefaultBrowserUsingSystemSettings(base::Closure on_finished_callback);
On 2016/03/29 16:45:02, grt (very slow) wrote:
> nit: pass callbacks by const-ref

Done.

https://codereview.chromium.org/1832853003/diff/1/chrome/browser/shell_integr...
File chrome/browser/shell_integration_win.cc (right):

https://codereview.chromium.org/1832853003/diff/1/chrome/browser/shell_integr...
chrome/browser/shell_integration_win.cc:230: // This helper class manages both
the timer and the registry watchers and make
On 2016/03/29 16:45:03, grt (very slow) wrote:
> nit: make -> makes

Done.

https://codereview.chromium.org/1832853003/diff/1/chrome/browser/shell_integr...
chrome/browser/shell_integration_win.cc:275: if (reg_key && reg_key->Valid()) {
On 2016/03/29 16:45:03, grt (very slow) wrote:
> no need to test reg_key alone since it will always be valid -- the process
will
> crash on out-of-memory.

Done.

https://codereview.chromium.org/1832853003/diff/1/chrome/browser/shell_integr...
chrome/browser/shell_integration_win.cc:296: void
ConcludeOpenSystemSettingsToDefaultBrowser(
On 2016/03/29 16:45:03, grt (very slow) wrote:
> this function is tightly coupled to the helper. can it be a private member
> function of it?

Done.

https://codereview.chromium.org/1832853003/diff/1/chrome/browser/shell_integr...
chrome/browser/shell_integration_win.cc:298: base::Callback<void()>
on_finished_callback,
On 2016/03/29 16:45:03, grt (very slow) wrote:
> const base::Closure&

Done.

[grt (UTC plus 2), 2016-03-30 15:55:10]

okay, the real review has started. :-)

https://codereview.chromium.org/1832853003/diff/20001/chrome/browser/shell_in...
File chrome/browser/shell_integration.h (right):

https://codereview.chromium.org/1832853003/diff/20001/chrome/browser/shell_in...
chrome/browser/shell_integration.h:244: // Subclasses MUST make sure the
callback is executed.
consider adding a note that the callback may be run reentrantly or at an
arbitrary time later on this thread.

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

https://codereview.chromium.org/1832853003/diff/20001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:238: void Initialize(const
base::Closure& on_finished_callback) {
is there a reason not to move this into the ctor and get rid of Initialize()?

https://codereview.chromium.org/1832853003/diff/20001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:239: const wchar_t
url_association_format[] =
nit: static const wchar_t kUrlAssociationFormat[]

https://codereview.chromium.org/1832853003/diff/20001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:246: this, on_finished_callback);
what's the advantage of binding on_finished_callback into the callbacks passed
to the watcher and timer? could you put it in a member variable in the helper
instead?

https://codereview.chromium.org/1832853003/diff/20001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:271: bool is_registry_watcher) {
i think the code would be a little more readable if the "a reg key was modified"
callback and the "we timed out" callback were handled by two different
(well-named) functions.

https://codereview.chromium.org/1832853003/diff/20001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:283: if (!run_once_)
you can do away with |run_once_| if you keep on_finished_callback as a member
variable by clearing out on_finished_callback after it is first invoked:

    if (on_finished_callback_.is_null())
      return;
    on_finished_callback_.Run();
    Reset();

(i like Reset more than Clear since that's what happens under the covers.)

and in Reset():
    on_finished_callback_ = base::Closure();

better yet: does this class need to be refcounted? i think you get what you need
by binding via WeakPtrs and deleting the instance after invoking the callback:

      on_finished_callback_.Run();
      delete this;

then you don't need Clear/Reset at all since the dtor will take care of
everything for you.

https://codereview.chromium.org/1832853003/diff/20001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:290: scoped_ptr<base::win::RegKey>
CreateRegistryKeyWatcher(
wdyt of getting rid of the scoped_ptrs and changing this to something like:
  static void InstallRegistryKeyWatcher(
      const wchar_t* key_path,
      const base::Closure& change_callback,
      base::win::RegKey* key) {
    if (key->Open(...) == ERROR_SUCCESS &&
        !key->StartWatching(change_callback)) {
      key->Close();
    }
  }

[Patrick Monette, 2016-03-30 22:09:47]

On 2016/03/30 15:55:10, grt (very slow) wrote:
> okay, the real review has started. :-)
You mean the fun has started!
> 
>
https://codereview.chromium.org/1832853003/diff/20001/chrome/browser/shell_in...
> File chrome/browser/shell_integration.h (right):
> 
>
https://codereview.chromium.org/1832853003/diff/20001/chrome/browser/shell_in...
> chrome/browser/shell_integration.h:244: // Subclasses MUST make sure the
> callback is executed.
> consider adding a note that the callback may be run reentrantly or at an
> arbitrary time later on this thread.
> 
>
https://codereview.chromium.org/1832853003/diff/20001/chrome/browser/shell_in...
> File chrome/browser/shell_integration_win.cc (right):
> 
>
https://codereview.chromium.org/1832853003/diff/20001/chrome/browser/shell_in...
> chrome/browser/shell_integration_win.cc:238: void Initialize(const
> base::Closure& on_finished_callback) {
> is there a reason not to move this into the ctor and get rid of Initialize()?
> 
>
https://codereview.chromium.org/1832853003/diff/20001/chrome/browser/shell_in...
> chrome/browser/shell_integration_win.cc:239: const wchar_t
> url_association_format[] =
> nit: static const wchar_t kUrlAssociationFormat[]
> 
>
https://codereview.chromium.org/1832853003/diff/20001/chrome/browser/shell_in...
> chrome/browser/shell_integration_win.cc:246: this, on_finished_callback);
> what's the advantage of binding on_finished_callback into the callbacks passed
> to the watcher and timer? could you put it in a member variable in the helper
> instead?
> 
>
https://codereview.chromium.org/1832853003/diff/20001/chrome/browser/shell_in...
> chrome/browser/shell_integration_win.cc:271: bool is_registry_watcher) {
> i think the code would be a little more readable if the "a reg key was
modified"
> callback and the "we timed out" callback were handled by two different
> (well-named) functions.
> 
>
https://codereview.chromium.org/1832853003/diff/20001/chrome/browser/shell_in...
> chrome/browser/shell_integration_win.cc:283: if (!run_once_)
> you can do away with |run_once_| if you keep on_finished_callback as a member
> variable by clearing out on_finished_callback after it is first invoked:
> 
>     if (on_finished_callback_.is_null())
>       return;
>     on_finished_callback_.Run();
>     Reset();
> 
> (i like Reset more than Clear since that's what happens under the covers.)
> 
> and in Reset():
>     on_finished_callback_ = base::Closure();
> 
> better yet: does this class need to be refcounted? i think you get what you
need
> by binding via WeakPtrs and deleting the instance after invoking the callback:
> 
>       on_finished_callback_.Run();
>       delete this;
> 
> then you don't need Clear/Reset at all since the dtor will take care of
> everything for you.
> 
>
https://codereview.chromium.org/1832853003/diff/20001/chrome/browser/shell_in...
> chrome/browser/shell_integration_win.cc:290: scoped_ptr<base::win::RegKey>
> CreateRegistryKeyWatcher(
> wdyt of getting rid of the scoped_ptrs and changing this to something like:
>   static void InstallRegistryKeyWatcher(
>       const wchar_t* key_path,
>       const base::Closure& change_callback,
>       base::win::RegKey* key) {
>     if (key->Open(...) == ERROR_SUCCESS &&
>         !key->StartWatching(change_callback)) {
>       key->Close();
>     }
>   }

[Patrick Monette, 2016-03-30 22:21:55]

Thanks for the comments. PTAL.

https://codereview.chromium.org/1832853003/diff/20001/chrome/browser/shell_in...
File chrome/browser/shell_integration.h (right):

https://codereview.chromium.org/1832853003/diff/20001/chrome/browser/shell_in...
chrome/browser/shell_integration.h:244: // Subclasses MUST make sure the
callback is executed.
On 2016/03/30 15:55:09, grt (very slow) wrote:
> consider adding a note that the callback may be run reentrantly or at an
> arbitrary time later on this thread.

Done.

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

https://codereview.chromium.org/1832853003/diff/20001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:238: void Initialize(const
base::Closure& on_finished_callback) {
On 2016/03/30 15:55:09, grt (very slow) wrote:
> is there a reason not to move this into the ctor and get rid of Initialize()?

No reason. Changed it!

https://codereview.chromium.org/1832853003/diff/20001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:239: const wchar_t
url_association_format[] =
On 2016/03/30 15:55:10, grt (very slow) wrote:
> nit: static const wchar_t kUrlAssociationFormat[]

Done.

https://codereview.chromium.org/1832853003/diff/20001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:246: this, on_finished_callback);
On 2016/03/30 15:55:10, grt (very slow) wrote:
> what's the advantage of binding on_finished_callback into the callbacks passed
> to the watcher and timer? could you put it in a member variable in the helper
> instead?

Done.

https://codereview.chromium.org/1832853003/diff/20001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:271: bool is_registry_watcher) {
On 2016/03/30 15:55:09, grt (very slow) wrote:
> i think the code would be a little more readable if the "a reg key was
modified"
> callback and the "we timed out" callback were handled by two different
> (well-named) functions.

Done for "a reg key was modified" and kept
ConcludeOpenSystemSettingsToDefaultBrowser().

https://codereview.chromium.org/1832853003/diff/20001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:283: if (!run_once_)
On 2016/03/30 15:55:10, grt (very slow) wrote:
> you can do away with |run_once_| if you keep on_finished_callback as a member
> variable by clearing out on_finished_callback after it is first invoked:
> 
>     if (on_finished_callback_.is_null())
>       return;
>     on_finished_callback_.Run();
>     Reset();
> 
> (i like Reset more than Clear since that's what happens under the covers.)
> 
> and in Reset():
>     on_finished_callback_ = base::Closure();
> 
> better yet: does this class need to be refcounted? i think you get what you
need
> by binding via WeakPtrs and deleting the instance after invoking the callback:
> 
>       on_finished_callback_.Run();
>       delete this;
> 
> then you don't need Clear/Reset at all since the dtor will take care of
> everything for you.

Done. The naked new in SetAsDefaultBrowserUsingSystemSettings is weird? But I
think you know best.

https://codereview.chromium.org/1832853003/diff/20001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:290: scoped_ptr<base::win::RegKey>
CreateRegistryKeyWatcher(
On 2016/03/30 15:55:09, grt (very slow) wrote:
> wdyt of getting rid of the scoped_ptrs and changing this to something like:
>   static void InstallRegistryKeyWatcher(
>       const wchar_t* key_path,
>       const base::Closure& change_callback,
>       base::win::RegKey* key) {
>     if (key->Open(...) == ERROR_SUCCESS &&
>         !key->StartWatching(change_callback)) {
>       key->Close();
>     }
>   }

Kept this as-is to use with the new vector of scoped ptrs.

[grt (UTC plus 2), 2016-03-31 13:23:37]

looking good!

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

https://codereview.chromium.org/1832853003/diff/40001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:241:
registry_watcher_count_(protocols.size()),
should this count be the # of protocols provided, or the # of watchers
successfully created? in other words, if the helper is asked to monitor two
protocols but can only monitor one, should it complete the interaction after the
one is notified, or wait until the timeout?

https://codereview.chromium.org/1832853003/diff/40001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:260: void OnRegistryValueChanged() {
nit: OnRegistryKeyChanged since it monitors a key not a value. also worth noting
somewhere in a comment that this helper only waits for the first modification of
any type to a key (value changes, attribute changes, ACL changes, etc). this is
likely "good enough" for now, but i think it's worth noting.

https://codereview.chromium.org/1832853003/diff/40001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:263: // Make sure all the registry
watchers fired.
nit: "have fired"

https://codereview.chromium.org/1832853003/diff/40001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:266: if (registry_watcher_count_ == 0)
nit: if (--registry_watcher_count_ == 0)

https://codereview.chromium.org/1832853003/diff/40001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:270: void
ConcludeOpenSystemSettingsToDefaultBrowser() {
since this will be re-used for other protocols, consider generalizing this
method name ("DefaultBrowser"). maybe just "ConcludeInteraction"?

https://codereview.chromium.org/1832853003/diff/40001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:270: void
ConcludeOpenSystemSettingsToDefaultBrowser() {
nit: add a doc comment

https://codereview.chromium.org/1832853003/diff/40001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:277: scoped_ptr<base::win::RegKey>
CreateRegistryKeyWatcher(
nit: add a doc comment

https://codereview.chromium.org/1832853003/diff/40001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:278: const wchar_t* registry_value) {
nit: key_path since it's not naming a value in the registry

https://codereview.chromium.org/1832853003/diff/40001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:282: if (reg_key->Valid()) {
only return the key if it's really being watched?

if (reg_key->Valid() && StartWatching(...))
  return reg_key;
return scoped_ptr<base::win::RegKey>();

otherwise the helper will always wait for the timeout. or is that what you
really want?

https://codereview.chromium.org/1832853003/diff/40001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:298: // multiple protocol associations.
e.g. Changing the default browser modify
nit: "modifies"

https://codereview.chromium.org/1832853003/diff/40001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:355: new
OpenSystemSettingsHelper({L"http", L"https"}, on_finished_callback);
note: uniform initialization is banned
(http://chromium-cpp.appspot.com/#core-blacklist)

https://codereview.chromium.org/1832853003/diff/40001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:355: new
OpenSystemSettingsHelper({L"http", L"https"}, on_finished_callback);
rather than the naked new, i suggest a static function on the helper class to
kick things off:

  static const wchar_t* const kProtocols[] = {L"http", L"https", nullptr};
  OpenSystemSettingsHelper::Begin(kProtocols, on_finished_callback);

the ctor can then be a private method

[Patrick Monette, 2016-03-31 18:26:04]

How about now!? PTanL

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

https://codereview.chromium.org/1832853003/diff/40001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:241:
registry_watcher_count_(protocols.size()),
On 2016/03/31 13:23:37, grt (very slow) wrote:
> should this count be the # of protocols provided, or the # of watchers
> successfully created? in other words, if the helper is asked to monitor two
> protocols but can only monitor one, should it complete the interaction after
the
> one is notified, or wait until the timeout?

Okay I changed it to the # of watchers successfully created. I don't think the
differences between the 2 are important but a watcher is essentially useless if
another one failed and we're waiting on the 2.

https://codereview.chromium.org/1832853003/diff/40001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:260: void OnRegistryValueChanged() {
On 2016/03/31 13:23:37, grt (very slow) wrote:
> nit: OnRegistryKeyChanged since it monitors a key not a value. also worth
noting
> somewhere in a comment that this helper only waits for the first modification
of
> any type to a key (value changes, attribute changes, ACL changes, etc). this
is
> likely "good enough" for now, but i think it's worth noting.

Done.

https://codereview.chromium.org/1832853003/diff/40001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:263: // Make sure all the registry
watchers fired.
On 2016/03/31 13:23:37, grt (very slow) wrote:
> nit: "have fired"

Done.

https://codereview.chromium.org/1832853003/diff/40001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:266: if (registry_watcher_count_ == 0)
On 2016/03/31 13:23:37, grt (very slow) wrote:
> nit: if (--registry_watcher_count_ == 0)

Done.

https://codereview.chromium.org/1832853003/diff/40001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:270: void
ConcludeOpenSystemSettingsToDefaultBrowser() {
On 2016/03/31 13:23:37, grt (very slow) wrote:
> nit: add a doc comment

Done.

https://codereview.chromium.org/1832853003/diff/40001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:270: void
ConcludeOpenSystemSettingsToDefaultBrowser() {
On 2016/03/31 13:23:37, grt (very slow) wrote:
> since this will be re-used for other protocols, consider generalizing this
> method name ("DefaultBrowser"). maybe just "ConcludeInteraction"?

Done.

https://codereview.chromium.org/1832853003/diff/40001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:277: scoped_ptr<base::win::RegKey>
CreateRegistryKeyWatcher(
On 2016/03/31 13:23:37, grt (very slow) wrote:
> nit: add a doc comment

Done.

https://codereview.chromium.org/1832853003/diff/40001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:278: const wchar_t* registry_value) {
On 2016/03/31 13:23:37, grt (very slow) wrote:
> nit: key_path since it's not naming a value in the registry

Done.

https://codereview.chromium.org/1832853003/diff/40001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:282: if (reg_key->Valid()) {
On 2016/03/31 13:23:37, grt (very slow) wrote:
> only return the key if it's really being watched?
> 
> if (reg_key->Valid() && StartWatching(...))
>   return reg_key;
> return scoped_ptr<base::win::RegKey>();
> 
> otherwise the helper will always wait for the timeout. or is that what you
> really want?

I changed it to AddRegistryKeyWatcher() so the vector only keep tracks of
initialized watchers.

https://codereview.chromium.org/1832853003/diff/40001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:298: // multiple protocol associations.
e.g. Changing the default browser modify
On 2016/03/31 13:23:37, grt (very slow) wrote:
> nit: "modifies"

Done.

https://codereview.chromium.org/1832853003/diff/40001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:355: new
OpenSystemSettingsHelper({L"http", L"https"}, on_finished_callback);
On 2016/03/31 13:23:37, grt (very slow) wrote:
> note: uniform initialization is banned
> (http://chromium-cpp.appspot.com/#core-blacklist)

Done.

https://codereview.chromium.org/1832853003/diff/40001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:355: new
OpenSystemSettingsHelper({L"http", L"https"}, on_finished_callback);
On 2016/03/31 13:23:37, grt (very slow) wrote:
> rather than the naked new, i suggest a static function on the helper class to
> kick things off:
> 
>   static const wchar_t* const kProtocols[] = {L"http", L"https", nullptr};
>   OpenSystemSettingsHelper::Begin(kProtocols, on_finished_callback);
> 
> the ctor can then be a private method

Done.

[grt (UTC plus 2), 2016-03-31 19:08:12]

looking good.

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

https://codereview.chromium.org/1832853003/diff/60001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:255: int i = 0;
you could get rid of |i| by scanning the array:
  for (const wchar_t* const* scan = &protocols[0]; *scan != nullptr; ++scan) {
    AddRegistryKeyWatcher(..., *scan, ...);
  }

https://codereview.chromium.org/1832853003/diff/60001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:286: 
how about UMA here so we know if the interaction is concluded due to a timeout
vs due to keys being changed? might also be interesting to know how long it took
for the final OnRegistryKeyChanged to arrive. wdyt?

https://codereview.chromium.org/1832853003/diff/60001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:376: return;
wdyt of running the callback before returning here, and only starting the helper
down below? there's no need to wait two minutes before running the callback if
this happens. then again, i don't think this could ever fail, so maybe the
return value should just be ignored?

[Patrick Monette, 2016-04-05 20:04:55]

Can you please take yet another look grt@

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

https://codereview.chromium.org/1832853003/diff/60001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:255: int i = 0;
On 2016/03/31 19:08:12, grt (very slow) wrote:
> you could get rid of |i| by scanning the array:
>   for (const wchar_t* const* scan = &protocols[0]; *scan != nullptr; ++scan) {
>     AddRegistryKeyWatcher(..., *scan, ...);
>   }

Done.

https://codereview.chromium.org/1832853003/diff/60001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:286: 
On 2016/03/31 19:08:12, grt (very slow) wrote:
> how about UMA here so we know if the interaction is concluded due to a timeout
> vs due to keys being changed? might also be interesting to know how long it
took
> for the final OnRegistryKeyChanged to arrive. wdyt?

Done.

https://codereview.chromium.org/1832853003/diff/60001/chrome/browser/shell_in...
chrome/browser/shell_integration_win.cc:376: return;
On 2016/03/31 19:08:12, grt (very slow) wrote:
> wdyt of running the callback before returning here, and only starting the
helper
> down below? there's no need to wait two minutes before running the callback if
> this happens. then again, i don't think this could ever fail, so maybe the
> return value should just be ignored?

Done. The logging is consistent with the other SetASDefault() functions.

[grt (UTC plus 2), 2016-04-06 19:41:51]

nice! lgtm.

[Patrick Monette, 2016-04-06 20:08:55]

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

[Patrick Monette, 2016-04-06 20:08:55]

sky@ PTAL at shell_integration*
asvitkine@ PTAL at histograms related changes.

[Alexei Svitkine (slow), 2016-04-06 20:13:06]

lgtm

https://codereview.chromium.org/1832853003/diff/80001/chrome/browser/shell_in...
File chrome/browser/shell_integration.cc (right):

https://codereview.chromium.org/1832853003/diff/80001/chrome/browser/shell_in...
chrome/browser/shell_integration.cc:50: NOTREACHED();
Why is this NOTREACHED()? Should have a comment.

https://codereview.chromium.org/1832853003/diff/80001/chrome/browser/shell_in...
chrome/browser/shell_integration.cc:265: }
Nit: Add a break outside the if so that if a new case is added, it doesn't
accidentally fall-through.

https://codereview.chromium.org/1832853003/diff/80001/chrome/browser/shell_in...
File chrome/browser/shell_integration.h (right):

https://codereview.chromium.org/1832853003/diff/80001/chrome/browser/shell_in...
chrome/browser/shell_integration.h:246: // Note : Subclasses MUST make sure
|on_finished_callback| is executed.
Nit: No space before :

[Patrick Monette, 2016-04-06 21:54:34]

https://codereview.chromium.org/1832853003/diff/80001/chrome/browser/shell_in...
File chrome/browser/shell_integration.cc (right):

https://codereview.chromium.org/1832853003/diff/80001/chrome/browser/shell_in...
chrome/browser/shell_integration.cc:50: NOTREACHED();
On 2016/04/06 20:13:06, Alexei Svitkine wrote:
> Why is this NOTREACHED()? Should have a comment.

Done.

https://codereview.chromium.org/1832853003/diff/80001/chrome/browser/shell_in...
chrome/browser/shell_integration.cc:265: }
On 2016/04/06 20:13:06, Alexei Svitkine wrote:
> Nit: Add a break outside the if so that if a new case is added, it doesn't
> accidentally fall-through.

Done.

https://codereview.chromium.org/1832853003/diff/80001/chrome/browser/shell_in...
File chrome/browser/shell_integration.h (right):

https://codereview.chromium.org/1832853003/diff/80001/chrome/browser/shell_in...
chrome/browser/shell_integration.h:246: // Note : Subclasses MUST make sure
|on_finished_callback| is executed.
On 2016/04/06 20:13:06, Alexei Svitkine wrote:
> Nit: No space before :

Done.

[sky, 2016-04-07 18:18:47]

I started look at shell_integration_win.cc, then stopped when I realized grt is
an owner. Only a couple of comments.

https://codereview.chromium.org/1832853003/diff/100001/chrome/browser/shell_i...
File chrome/browser/shell_integration.h (right):

https://codereview.chromium.org/1832853003/diff/100001/chrome/browser/shell_i...
chrome/browser/shell_integration.h:40: void
SetAsDefaultBrowserUsingSystemSettings(
If this is win specific can it be in an ifdef?

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

https://codereview.chromium.org/1832853003/diff/100001/chrome/browser/shell_i...
chrome/browser/shell_integration_win.cc:225: bool
IsSetAsDefaultUsingSystemSettings() {
This name is rather confusing as well. I would move the version check into the
single place that calls it and document what it means there.

https://codereview.chromium.org/1832853003/diff/100001/chrome/browser/shell_i...
chrome/browser/shell_integration_win.cc:445: DefaultWebClientSetPermission
CanSetAsDefaultBrowser() {
The name of this function is now rather confusing. How about
GetDefaultWebClientSetPermission()?

[Patrick Monette, 2016-04-08 22:25:48]

Patchset #7 (id:120001) has been deleted

[Patrick Monette, 2016-04-08 22:26:08]

sky@ WDYT now?

https://codereview.chromium.org/1832853003/diff/100001/chrome/browser/shell_i...
File chrome/browser/shell_integration.h (right):

https://codereview.chromium.org/1832853003/diff/100001/chrome/browser/shell_i...
chrome/browser/shell_integration.h:40: void
SetAsDefaultBrowserUsingSystemSettings(
On 2016/04/07 18:18:47, sky wrote:
> If this is win specific can it be in an ifdef?

I don't like #ifdef for windows here. Moved win specific function for set as
default to shell_integration_win.h

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

https://codereview.chromium.org/1832853003/diff/100001/chrome/browser/shell_i...
chrome/browser/shell_integration_win.cc:225: bool
IsSetAsDefaultUsingSystemSettings() {
On 2016/04/07 18:18:47, sky wrote:
> This name is rather confusing as well. I would move the version check into the
> single place that calls it and document what it means there.

Done.

https://codereview.chromium.org/1832853003/diff/100001/chrome/browser/shell_i...
chrome/browser/shell_integration_win.cc:445: DefaultWebClientSetPermission
CanSetAsDefaultBrowser() {
On 2016/04/07 18:18:47, sky wrote:
> The name of this function is now rather confusing. How about
> GetDefaultWebClientSetPermission()?

Done. I actually renamed this function GetDefaultWebClientSetPermission() but
also created a CanSetAsDefaultBrowser() function for convenience.

[sky, 2016-04-11 15:06:55]

Much improved, thanks. LGTM

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

https://codereview.chromium.org/1832853003/diff/140001/chrome/browser/shell_i...
chrome/browser/shell_integration_win.h:10: #include "base/callback.h"
nit: callback_forward.h

https://codereview.chromium.org/1832853003/diff/140001/chrome/browser/shell_i...
chrome/browser/shell_integration_win.h:11: #include "base/files/file_path.h"
nit: this include and the next aren't needed.

[Patrick Monette, 2016-04-11 15:48:22]

Thanks!

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

https://codereview.chromium.org/1832853003/diff/140001/chrome/browser/shell_i...
chrome/browser/shell_integration_win.h:10: #include "base/callback.h"
On 2016/04/11 15:06:55, sky wrote:
> nit: callback_forward.h

Done.

https://codereview.chromium.org/1832853003/diff/140001/chrome/browser/shell_i...
chrome/browser/shell_integration_win.h:11: #include "base/files/file_path.h"
On 2016/04/11 15:06:55, sky wrote:
> nit: this include and the next aren't needed.

Done.

[Patrick Monette, 2016-04-11 15:48:34]

The CQ bit was checked by pmonette@chromium.org

[Patrick Monette, 2016-04-11 15:48:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, grt@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1832853003/#ps160001
(title: "Fix last nits")

[commit-bot: I haz the power, 2016-04-11 15:48:49]

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

[commit-bot: I haz the power, 2016-04-11 16:02:51]

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

[commit-bot: I haz the power, 2016-04-11 16:02:52]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Patrick Monette, 2016-04-11 16:37:59]

The CQ bit was checked by pmonette@chromium.org

[Patrick Monette, 2016-04-11 16:37:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, sky@chromium.org, grt@chromium.org
Link to the patchset: https://codereview.chromium.org/1832853003/#ps180001
(title: "Rebasing + fix compilation error")

[commit-bot: I haz the power, 2016-04-11 16:38:15]

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

[commit-bot: I haz the power, 2016-04-11 17:03:44]

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

[commit-bot: I haz the power, 2016-04-11 17:03:45]

Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[Patrick Monette, 2016-04-11 18:16:29]

The CQ bit was checked by pmonette@chromium.org

[Patrick Monette, 2016-04-11 18:16:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, sky@chromium.org, grt@chromium.org
Link to the patchset: https://codereview.chromium.org/1832853003/#ps200001
(title: "Fix mac compilation")

[commit-bot: I haz the power, 2016-04-11 18:17:27]

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

[commit-bot: I haz the power, 2016-04-11 22:04:59]

Description was changed from

==========
DefaultBrowserWorker now fully supports opening the settings for Win10

The worker uses a new SetAsDefaultBrowser style function that manages
the user interaction with the system settings.

BUG=576490
==========

to

==========
DefaultBrowserWorker now fully supports opening the settings for Win10

The worker uses a new SetAsDefaultBrowser style function that manages
the user interaction with the system settings.

BUG=576490
==========

[commit-bot: I haz the power, 2016-04-11 22:05:00]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2016-04-11 22:07:23]

Description was changed from

==========
DefaultBrowserWorker now fully supports opening the settings for Win10

The worker uses a new SetAsDefaultBrowser style function that manages
the user interaction with the system settings.

BUG=576490
==========

to

==========
DefaultBrowserWorker now fully supports opening the settings for Win10

The worker uses a new SetAsDefaultBrowser style function that manages
the user interaction with the system settings.

BUG=576490

Committed: https://crrev.com/32a5cfb4e2c2db333e007fbba67d35bd9fa664bd
Cr-Commit-Position: refs/heads/master@{#386487}
==========

[commit-bot: I haz the power, 2016-04-11 22:07:27]

Patchset 10 (id:??) landed as
https://crrev.com/32a5cfb4e2c2db333e007fbba67d35bd9fa664bd
Cr-Commit-Position: refs/heads/master@{#386487}

[grt (UTC plus 2), 2016-04-12 13:36:44]

nice refactor. lgtm w/ one nit for the future.

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

https://codereview.chromium.org/1832853003/diff/200001/chrome/browser/shell_i...
chrome/browser/shell_integration.cc:252: } else {
nit for the future: omit "else" when the "if" clause ends with "return;":
  if (foo) {
    blorf();
    return;
  }
  // don't put this in an "else" clause:
  spam();