Prototype version of the first-run dialog for Windows 8

Prototype version of the first-run dialog for Windows 8.

The new UI supports both dialog and in-tab versions. Its purpose is to
set Chrome as default browser when running first time and to re-launch it
in Metro mode.


R=grt,gab,jhawkins,sky
BUG=131838
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=143729

[gab, 2012-06-16 19:23:56]

On 2012/06/14 22:07:16, motek. wrote:

You'll likely want another reviewer for the UI stuff (unless Greg is on fire and
willing to review that as well; I don't have sufficient knowledge to throughly
review the UI part myself).

[gab, 2012-06-16 19:32:51]

lgtm, but i haven't looked at chrome/browser/*, I can do a readbility review of
it if you really need it, but other than that I never touched that code so am
not the right person for it.

Cheers,
Gab

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/shel...
File chrome/browser/shell_integration.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/shel...
chrome/browser/shell_integration.cc:271: return
RESULT_SET_DEFAULT_FAILED_OR_CANCELLED;
I prefer the way you have it in the function above this one, i.e.:

return ShellIntegration::SetAsDefaultProtocolClient(protocol_) ?
    RESULT_SET_DEFAULT_OK : RESULT_SET_DEFAULT_FAILED_OR_CANCELLED;

[grt (UTC plus 2), 2012-06-18 19:19:13]

general comments:
- document all new classes and functions
- chromium style note: "When calling a function, however, feel free to collapse
things to take less space."  as such, resist your urge to put each arg to a
function on its own line.

i'll re-review once you have a new patch set up.  cheers.

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/exte...
File chrome/browser/external_protocol/external_protocol_handler_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/exte...
chrome/browser/external_protocol/external_protocol_handler_unittest.cc:31:
SetAsDefault(bool interactive_permitted) {
add OVERRIDE while you're here

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/shel...
File chrome/browser/shell_integration.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/shel...
chrome/browser/shell_integration.cc:122:
base::Bind(&DefaultWebClientWorker::ExecuteCheckIsDefault,
chromium prefers to conserve vertical space in function calls.  please move
"this" up to this line.

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/shel...
chrome/browser/shell_integration.cc:155:
base::Bind(&DefaultWebClientWorker::CompleteCheckIsDefault,
same note about condensing args

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/shel...
chrome/browser/shell_integration.cc:187: StartCheckIsDefault(call_result);
is there any reason to do this when call_result isn't RESULT_SET_DEFAULT_OK?

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/shel...
File chrome/browser/shell_integration.h (right):

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/shel...
chrome/browser/shell_integration.h:161: enum SetDefaultWebClientResult {
Please document this.

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/shel...
chrome/browser/shell_integration.h:171: virtual void SetDefaultWebClientUIState(
i don't really like this as-is, since it confuses things for users of the
observe that only care about whether chrome is or isn't the default.

would the addition of STATE_REFUSED_DEFAULT to DefaultWebClientUIState be
sufficient?

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/s...
File chrome/browser/ui/startup/default_browser_prompt.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/s...
chrome/browser/ui/startup/default_browser_prompt.cc:155:
base::Bind(&SetChromeAsDefaultBrowser,
condense

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/s...
chrome/browser/ui/startup/default_browser_prompt.cc:187:
MetroizerUI::Show(profile, browser, dialog);
de-indent two spaces

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/s...
chrome/browser/ui/startup/default_browser_prompt.cc:222:
content::Source<content::WebContents>(source).ptr());
nit: indent two more spaces

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/s...
chrome/browser/ui/startup/default_browser_prompt.cc:227: // Unregister and
delete.
i don't see the delete.  is it handled by the registrar?  if it were, the
profile_ access on line 237 would be bad.  i think you need an explicit delete
this; at the very end of this function.

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/s...
chrome/browser/ui/startup/default_browser_prompt.cc:233:
BrowserThread::PostTask(
save the whitespace!  (args can share lines)

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/w...
File chrome/browser/ui/webui/metroizer_ui.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/w...
chrome/browser/ui/webui/metroizer_ui.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
why is this file present on non-windows platforms?

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/w...
chrome/browser/ui/webui/metroizer_ui.cc:21: #ifdef OS_WIN
move platform-specific includes down below the all-platforms includes

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/w...
chrome/browser/ui/webui/metroizer_ui.cc:63: explicit MetroizeHandler();
no explicit

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/w...
chrome/browser/ui/webui/metroizer_ui.cc:143: // If this is per-user install, we
will have to remove the sentinel file
this removes the sentinel from desktop chrome.  if your intent is to force
first-run in metro, you don't need to do anything here: if the user has never
been in metro chrome, they'll automatically get the first-run flow.  if they
have been in metro chrome before, i'm not sure why we'd want them to see
first-run again.

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/w...
chrome/browser/ui/webui/metroizer_ui.cc:149: // If Metro Chrome has been
activated, we should close this process.
indentation here on down

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/w...
chrome/browser/ui/webui/metroizer_ui.cc:170:
ShellIntegration::DefaultWebClientUIState state,
indentation

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/w...
File chrome/browser/ui/webui/metroizer_ui.h (right):

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/w...
chrome/browser/ui/webui/metroizer_ui.h:13: class MetroizerUI : public
content::WebUIController {
newline above this

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/w...
chrome/browser/ui/webui/metroizer_ui.h:19: private:
newline above this

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/chrome_brows...
File chrome/chrome_browser.gypi (right):

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/chrome_brows...
chrome/chrome_browser.gypi:3815: 'browser/ui/webui/metroizer_ui.cc',
why aren't these metroizer_ui_win?  the metroizer doesn't need to impose a cost
on all other platforms, does it?

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/installer/ut...
File chrome/installer/util/shell_util.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/installer/ut...
chrome/installer/util/shell_util.cc:1061: return
LaunchSelectDefaultProtocolHandlerDialog(L"http") == S_OK;
use SUCCEEDED() macro rather than comparing to S_OK

[motek., 2012-06-19 18:06:44]

I have addressed all trivial remarks, but I have some doubts regarding the
interface in shell_integration.h and regarding the sentinel file. I'd like to
discuss one and get clarification on the other. 

Meanwhile, I'll work on making metroizer_ui.* windows only.

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/exte...
File chrome/browser/external_protocol/external_protocol_handler_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/exte...
chrome/browser/external_protocol/external_protocol_handler_unittest.cc:31:
SetAsDefault(bool interactive_permitted) {
On 2012/06/18 19:19:13, grt wrote:
> add OVERRIDE while you're here

Done.

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/shel...
File chrome/browser/shell_integration.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/shel...
chrome/browser/shell_integration.cc:122:
base::Bind(&DefaultWebClientWorker::ExecuteCheckIsDefault,
On 2012/06/18 19:19:13, grt wrote:
> chromium prefers to conserve vertical space in function calls.  please move
> "this" up to this line.

Done.

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/shel...
chrome/browser/shell_integration.cc:155:
base::Bind(&DefaultWebClientWorker::CompleteCheckIsDefault,
On 2012/06/18 19:19:13, grt wrote:
> same note about condensing args

Done.

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/shel...
chrome/browser/shell_integration.cc:187: StartCheckIsDefault(call_result);
On 2012/06/18 19:19:13, grt wrote:
> is there any reason to do this when call_result isn't RESULT_SET_DEFAULT_OK?
I think so. It may be a debug blindness, but I invoke it all the time while
*being* the default.

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/shel...
chrome/browser/shell_integration.cc:271: return
RESULT_SET_DEFAULT_FAILED_OR_CANCELLED;
On 2012/06/16 19:32:51, gab wrote:
> I prefer the way you have it in the function above this one, i.e.:
> 
> return ShellIntegration::SetAsDefaultProtocolClient(protocol_) ?
>     RESULT_SET_DEFAULT_OK : RESULT_SET_DEFAULT_FAILED_OR_CANCELLED;
>                             

Done.

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/shel...
File chrome/browser/shell_integration.h (right):

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/shel...
chrome/browser/shell_integration.h:161: enum SetDefaultWebClientResult {
On 2012/06/18 19:19:13, grt wrote:
> Please document this.

Done.

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/shel...
chrome/browser/shell_integration.h:171: virtual void SetDefaultWebClientUIState(
On 2012/06/18 19:19:13, grt wrote:
> i don't really like this as-is, since it confuses things for users of the
> observe that only care about whether chrome is or isn't the default.
> 
> would the addition of STATE_REFUSED_DEFAULT to DefaultWebClientUIState be
> sufficient?
I did consider a similar idea. The trouble is that 'refused default' is not a
state, but a result. At the end of the sequence, refused or not, we can still be
default (if invoked when we are default already), not default or unknown. 
Moreover, this observer is used both in context of setting the default (which is
then followed by verification) and when we just want to check the state. I
thought it would be less surprising (albeit a bit more cumbersome indeed) to
pass both state and result as separate variables, with a specific result of
value to indicate no 'set' operation was undertaken.

I've also considered following options:
A. Turning DefaultWebClientUIState into a flag and composing state with result. 
B. Leaving this stuff as is and making a one-off for metroizer, where I would  
invoke both operations as wanted them. 
C. Adding a separate callback function to DefaultWebClientObserver to handle set
step (this would require a bit of synchronization work, though). 
D. Short-circuiting the whole thing on failure of the set operation. The trouble
I have with that is that this would effectively change the behaviour of this
operation in all other contexts (now after _PROCESSING you are bound to get
something else that makes sense, after the change - not so much; so, to display
the real state, say in options, I would need to ask again). 

If you really dislike this operation as is (personally, I think it makes sense),
I could do A or B (C is rather unappealing, because it wouldn't really buy as
much). What is your take?

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/s...
File chrome/browser/ui/startup/default_browser_prompt.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/s...
chrome/browser/ui/startup/default_browser_prompt.cc:155:
base::Bind(&SetChromeAsDefaultBrowser,
On 2012/06/18 19:19:13, grt wrote:
> condense

Done.

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/s...
chrome/browser/ui/startup/default_browser_prompt.cc:187:
MetroizerUI::Show(profile, browser, dialog);
On 2012/06/18 19:19:13, grt wrote:
> de-indent two spaces

Done.

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/s...
chrome/browser/ui/startup/default_browser_prompt.cc:222:
content::Source<content::WebContents>(source).ptr());
On 2012/06/18 19:19:13, grt wrote:
> nit: indent two more spaces

Done.

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/s...
chrome/browser/ui/startup/default_browser_prompt.cc:227: // Unregister and
delete.
On 2012/06/18 19:19:13, grt wrote:
> i don't see the delete.  is it handled by the registrar?  if it were, the
> profile_ access on line 237 would be bad.  i think you need an explicit delete
> this; at the very end of this function.
You are right, obviously. I must have clipped it in editing, it was there when I
lifted this code from FirstRunBubbleLauncher.

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/s...
chrome/browser/ui/startup/default_browser_prompt.cc:233:
BrowserThread::PostTask(
On 2012/06/18 19:19:13, grt wrote:
> save the whitespace!  (args can share lines)

Done.

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/w...
File chrome/browser/ui/webui/metroizer_ui.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/w...
chrome/browser/ui/webui/metroizer_ui.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/06/18 19:19:13, grt wrote:
> why is this file present on non-windows platforms?
Good question. I sort-of made it so by placing the start code in
default_browser_prompt.cc. There is not much reason for it being present
elsewhere. Although it is not actively harmful, just useless. Do we have
platform-specific files at this level?
If you think I should exclude this from non-Windows builds I could certainly do
that.

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/w...
chrome/browser/ui/webui/metroizer_ui.cc:21: #ifdef OS_WIN
On 2012/06/18 19:19:13, grt wrote:
> move platform-specific includes down below the all-platforms includes

Done.

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/w...
chrome/browser/ui/webui/metroizer_ui.cc:63: explicit MetroizeHandler();
On 2012/06/18 19:19:13, grt wrote:
> no explicit

Done.

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/w...
chrome/browser/ui/webui/metroizer_ui.cc:143: // If this is per-user install, we
will have to remove the sentinel file
On 2012/06/18 19:19:13, grt wrote:
> this removes the sentinel from desktop chrome.  if your intent is to force
> first-run in metro, you don't need to do anything here: if the user has never
> been in metro chrome, they'll automatically get the first-run flow.  if they
> have been in metro chrome before, i'm not sure why we'd want them to see
> first-run again.
I am not sure I understand this. This is not the behaviour I observed while
testing this on Win8 with per-user install. There, binaries were placed in
user/appdata directory and the sentinel file would be with the executable. Do I
miss something? Or has it changed somehow? It is not clear to me what behaviour
I should expect. Would you have time to discuss that?

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/w...
chrome/browser/ui/webui/metroizer_ui.cc:149: // If Metro Chrome has been
activated, we should close this process.
On 2012/06/18 19:19:13, grt wrote:
> indentation here on down

Done.

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/w...
chrome/browser/ui/webui/metroizer_ui.cc:170:
ShellIntegration::DefaultWebClientUIState state,
On 2012/06/18 19:19:13, grt wrote:
> indentation

Done.

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/w...
File chrome/browser/ui/webui/metroizer_ui.h (right):

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/w...
chrome/browser/ui/webui/metroizer_ui.h:13: class MetroizerUI : public
content::WebUIController {
On 2012/06/18 19:19:13, grt wrote:
> newline above this

Done.

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/w...
chrome/browser/ui/webui/metroizer_ui.h:19: private:
On 2012/06/18 19:19:13, grt wrote:
> newline above this

Done.

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/chrome_brows...
File chrome/chrome_browser.gypi (right):

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/chrome_brows...
chrome/chrome_browser.gypi:3815: 'browser/ui/webui/metroizer_ui.cc',
On 2012/06/18 19:19:13, grt wrote:
> why aren't these metroizer_ui_win?  the metroizer doesn't need to impose a
cost
> on all other platforms, does it?
It doesn't. Ack & will do.

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/installer/ut...
File chrome/installer/util/shell_util.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/installer/ut...
chrome/installer/util/shell_util.cc:1061: return
LaunchSelectDefaultProtocolHandlerDialog(L"http") == S_OK;
On 2012/06/18 19:19:13, grt wrote:
> use SUCCEEDED() macro rather than comparing to S_OK

Yeah, obviously. I am not sure what made me believe that cancelling/off-clicking
would result in S_FALSE. In fact, it is a negative code (0x8...).
I've reverted to the previous state.

[grt (UTC plus 2), 2012-06-19 20:43:09]

more comments

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/shel...
File chrome/browser/shell_integration.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/shel...
chrome/browser/shell_integration.cc:187: StartCheckIsDefault(call_result);
On 2012/06/19 18:06:44, motek. wrote:
> On 2012/06/18 19:19:13, grt wrote:
> > is there any reason to do this when call_result isn't RESULT_SET_DEFAULT_OK?
> I think so. It may be a debug blindness, but I invoke it all the time while
> *being* the default. 

do you mean that the browser choice dialog pops up when chrome is already the
default browser?  that seems bad.  we don't want to drive users to accidentally
clicking to make something other than chrome the default.

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/shel...
File chrome/browser/shell_integration.h (right):

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/shel...
chrome/browser/shell_integration.h:171: virtual void SetDefaultWebClientUIState(
On 2012/06/19 18:06:44, motek. wrote:
> On 2012/06/18 19:19:13, grt wrote:
> > i don't really like this as-is, since it confuses things for users of the
> > observe that only care about whether chrome is or isn't the default.
> > 
> > would the addition of STATE_REFUSED_DEFAULT to DefaultWebClientUIState be
> > sufficient?
> I did consider a similar idea. The trouble is that 'refused default' is not a
> state, but a result.

I was interpreting "state" in the sense of a FSM. "PROCESSING" is an
intermediate state, and "tried to set default but was rejected" is a terminal
state (this appears to be the what MetroizeHandler::SetDefaultWebClientUIState
cares about).  Does this seem any more palatable to you?

> At the end of the sequence, refused or not, we can still be
> default (if invoked when we are default already), not default or unknown.

As I mentioned in another comment, I think we shouldn't bring up the choice
dialog if Chrome is already the default browser.  Are we?  Maybe I'm missing
something and it's not so bad after all, but it seems bad.

> Moreover, this observer is used both in context of setting the default (which
is
> then followed by verification) and when we just want to check the state. I
> thought it would be less surprising (albeit a bit more cumbersome indeed) to
> pass both state and result as separate variables, with a specific result of
> value to indicate no 'set' operation was undertaken.
> 
> I've also considered following options:
> A. Turning DefaultWebClientUIState into a flag and composing state with
result. 
> B. Leaving this stuff as is and making a one-off for metroizer, where I would 

> invoke both operations as wanted them. 
> C. Adding a separate callback function to DefaultWebClientObserver to handle
set
> step (this would require a bit of synchronization work, though).

Would it?  The observer is notified on the UI thread, so you could add something
like this to CompleteSetAsDefault:

if (observer_)
  observer_->WhateverNewFunction(call_result);

> D. Short-circuiting the whole thing on failure of the set operation. The
trouble
> I have with that is that this would effectively change the behaviour of this
> operation in all other contexts (now after _PROCESSING you are bound to get
> something else that makes sense, after the change - not so much; so, to
display
> the real state, say in options, I would need to ask again). 
>
> If you really dislike this operation as is (personally, I think it makes
sense),
> I could do A or B (C is rather unappealing, because it wouldn't really buy as
> much). What is your take?

C would avoid changing all other consumers of this API for the one that cares.

Would you mind tracking down the author of this WebClientWorker stuff and asking
them for their thoughts on the matter?

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/w...
File chrome/browser/ui/webui/metroizer_ui.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/w...
chrome/browser/ui/webui/metroizer_ui.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/06/19 18:06:44, motek. wrote:
> On 2012/06/18 19:19:13, grt wrote:
> > why is this file present on non-windows platforms?
> Good question. I sort-of made it so by placing the start code in
> default_browser_prompt.cc. There is not much reason for it being present
> elsewhere. Although it is not actively harmful, just useless. Do we have
> platform-specific files at this level?
> If you think I should exclude this from non-Windows builds I could certainly
do
> that. 

renaming it to metroizer_ui_win will cause it to be omitted from non-windows
builds.  thanks for doing that.

https://chromiumcodereview.appspot.com/10539169/diff/6001/chrome/browser/ui/w...
chrome/browser/ui/webui/metroizer_ui.cc:143: // If this is per-user install, we
will have to remove the sentinel file
On 2012/06/19 18:06:44, motek. wrote:
> On 2012/06/18 19:19:13, grt wrote:
> > this removes the sentinel from desktop chrome.  if your intent is to force
> > first-run in metro, you don't need to do anything here: if the user has
never
> > been in metro chrome, they'll automatically get the first-run flow.  if they
> > have been in metro chrome before, i'm not sure why we'd want them to see
> > first-run again.
> I am not sure I understand this. This is not the behaviour I observed while
> testing this on Win8 with per-user install. There, binaries were placed in
> user/appdata directory and the sentinel file would be with the executable. Do
I
> miss something? Or has it changed somehow? It is not clear to me what
behaviour
> I should expect. Would you have time to discuss that?

Ah!  You're right: in-per user there's only one sentinel in the Application dir.
 In per-machine the sentinel is in the user data dir.  Would you add to the
comment explaining why the sentinel isn't removed for system-level installs?

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/ext...
File chrome/browser/external_protocol/external_protocol_handler.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/ext...
chrome/browser/external_protocol/external_protocol_handler.cc:1: // Copyright
(c) 2011 The Chromium Authors. All rights reserved.
2012

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/res...
File chrome/browser/resources/metroize/metroize.html (right):

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/res...
chrome/browser/resources/metroize/metroize.html:1: <!-- Copyright (c) 2011 The
Chromium Authors. All rights reserved.
2012

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/she...
File chrome/browser/shell_integration.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/she...
chrome/browser/shell_integration.cc:173: observer_ &&
observer_->IsInteractiveSetDefaultPermitted());
apologies for not noticing this on the previous review, but this call to a
virtual function on the observer is taking place on the FILE thread.  all others
are on the UI thread.  would you move this call into StartSetAsDefault (which is
on the UI thread) and pass the bool into ExecuteSetAsDefault?

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/she...
chrome/browser/shell_integration.cc:182:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
how about making a member variable to hold call_result and setting it here (on
the UI thread) so it doesn't need to be passed to StartCheckIsDefault,
ExecuteCheckIsDefault, and CompleteCheckIsDefault?

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/she...
File chrome/browser/shell_integration.h (right):

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/she...
chrome/browser/shell_integration.h:167: // cannot really infere if Chrome indeed
become the default, because
infere -> infer

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/ui/...
File chrome/browser/ui/startup/default_browser_prompt.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/ui/...
chrome/browser/ui/startup/default_browser_prompt.cc:182: Profile* profile,
Browser* browser, bool dialog) {
style: each arg goes on its own line if the args don't all fit on the same line
as the function name for delarations and definitions (the rule is different at
callsites); see
http://www.chromium.org/developers/coding-style#TOC-Code-formatting.

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/ui/...
chrome/browser/ui/startup/default_browser_prompt.cc:190: class
SetMetroBrowserFlowLauncher : public content::NotificationObserver {
please document.  also, i think class definitions maybe belong up at the top of
the file rather than sprinkled about in the middle, but i'm not so certain of
this.  could you poke around and see if this is done elsewhere?

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/ui/...
File chrome/browser/ui/webui/metroizer_ui.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui.cc:202: virtual ui::ModalType
GetDialogModalType() const OVERRIDE {
chromium style: do not inline virtual methods; see
http://www.chromium.org/developers/coding-style#TOC-Inline-functions

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/ui/...
File chrome/browser/ui/webui/metroizer_ui.h (right):

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui.h:14: 
delete this blank line and add one before "class MetroizerUI"

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui.h:21: 
delete this blank line and add one before "private:"

[motek., 2012-06-20 16:02:15]

Split windows code out (sending to trybots).
Addressed the interface issue by adding another function there.

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/ext...
File chrome/browser/external_protocol/external_protocol_handler.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/ext...
chrome/browser/external_protocol/external_protocol_handler.cc:1: // Copyright
(c) 2011 The Chromium Authors. All rights reserved.
On 2012/06/19 20:43:09, grt wrote:
> 2012

Done.

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/res...
File chrome/browser/resources/metroize/metroize.html (right):

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/res...
chrome/browser/resources/metroize/metroize.html:1: <!-- Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/06/19 20:43:09, grt wrote:
> 2012

Done.

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/she...
File chrome/browser/shell_integration.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/she...
chrome/browser/shell_integration.cc:173: observer_ &&
observer_->IsInteractiveSetDefaultPermitted());
On 2012/06/19 20:43:09, grt wrote:
> apologies for not noticing this on the previous review, but this call to a
> virtual function on the observer is taking place on the FILE thread.  all
others
> are on the UI thread.  would you move this call into StartSetAsDefault (which
is
> on the UI thread) and pass the bool into ExecuteSetAsDefault?

Done.

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/she...
chrome/browser/shell_integration.cc:182:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2012/06/19 20:43:09, grt wrote:
> how about making a member variable to hold call_result and setting it here (on
> the UI thread) so it doesn't need to be passed to StartCheckIsDefault,
> ExecuteCheckIsDefault, and CompleteCheckIsDefault?
I went a different way after all (with a separate callback). This way client
worries about everything, seems simplest.

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/she...
File chrome/browser/shell_integration.h (right):

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/she...
chrome/browser/shell_integration.h:167: // cannot really infere if Chrome indeed
become the default, because
On 2012/06/19 20:43:09, grt wrote:
> infere -> infer
This is gone.

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/ui/...
File chrome/browser/ui/startup/default_browser_prompt.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/ui/...
chrome/browser/ui/startup/default_browser_prompt.cc:182: Profile* profile,
Browser* browser, bool dialog) {
On 2012/06/19 20:43:09, grt wrote:
> style: each arg goes on its own line if the args don't all fit on the same
line
> as the function name for delarations and definitions (the rule is different at
> callsites); see
> http://www.chromium.org/developers/coding-style#TOC-Code-formatting.

Done. (moved to another file, too).

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/ui/...
chrome/browser/ui/startup/default_browser_prompt.cc:190: class
SetMetroBrowserFlowLauncher : public content::NotificationObserver {
On 2012/06/19 20:43:09, grt wrote:
> please document.  also, i think class definitions maybe belong up at the top
of
> the file rather than sprinkled about in the middle, but i'm not so certain of
> this.  could you poke around and see if this is done elsewhere?
Documented. Placement of the class declaration does not seem consistent. For
instance chromium/src/chrome/browser/ui/webui/feedback_ui.cc has it somewhere in
the middle. That said, I moved this one to the top of a new file. Just because I
can.

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/ui/...
File chrome/browser/ui/webui/metroizer_ui.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui.cc:202: virtual ui::ModalType
GetDialogModalType() const OVERRIDE {
On 2012/06/19 20:43:09, grt wrote:
> chromium style: do not inline virtual methods; see
> http://www.chromium.org/developers/coding-style#TOC-Inline-functions

Done.

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/ui/...
File chrome/browser/ui/webui/metroizer_ui.h (right):

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui.h:14: 
On 2012/06/19 20:43:09, grt wrote:
> delete this blank line and add one before "class MetroizerUI"
That did seem strange to me, but... Done.

https://chromiumcodereview.appspot.com/10539169/diff/17001/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui.h:21: 
On 2012/06/19 20:43:09, grt wrote:
> delete this blank line and add one before "private:"

Done.

[Cait (Slow), 2012-06-20 19:10:10]

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/res...
File chrome/browser/resources/metroize/metroize.html (right):

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/res...
chrome/browser/resources/metroize/metroize.html:36: </div>
Should this be a </span>?

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/res...
File chrome/browser/resources/metroize/metroize.js (right):

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/res...
chrome/browser/resources/metroize/metroize.js:5: <include
src="../shared/js/util.js"/>
From looking at other chromium js files, it seems like this include should
probably be specified in metroize.html (unless I'm missing something)?

[grt (UTC plus 2), 2012-06-20 19:42:12]

looking really good

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/she...
File chrome/browser/shell_integration.h (right):

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/she...
chrome/browser/shell_integration.h:214: virtual bool SetAsDefault(bool
interactive_permitted) = 0;
document meaning of return value

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/she...
chrome/browser/shell_integration.h:228: void ExecuteSetAsDefault(bool
interactive_permitted);
document interactive_permitted

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/she...
chrome/browser/shell_integration.h:240: void CompleteSetAsDefault(bool
call_result);
call_result -> succeeded?
also, document

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
File chrome/browser/ui/startup/default_browser_prompt_win.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
chrome/browser/ui/startup/default_browser_prompt_win.cc:7: #include
"base/memory/weak_ptr.h"
remove this

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
chrome/browser/ui/startup/default_browser_prompt_win.cc:8: #include
"chrome/browser/prefs/pref_service.h"
and this

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
chrome/browser/ui/startup/default_browser_prompt_win.cc:52: static void
CheckDefaultAndShowMetroizerCallback(Profile* profile,
no static within unnamed namespace

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
chrome/browser/ui/startup/default_browser_prompt_win.cc:55: if
(!ShellIntegration::IsDefaultBrowser() ||
|| -> &&

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
chrome/browser/ui/startup/default_browser_prompt_win.cc:91: if
(ShellIntegration::SET_DEFAULT_INTERACTIVE ==
nit: chromium would prefer that you write CanSetAsDefaultBrowser() ==
SET_DEFAULT_INTERACTIVE.

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
File chrome/browser/ui/webui/metroizer_ui_win.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui_win.cc:39: ChromeWebUIDataSource *
CreateMetroizerUIHTMLSource() {
no space between ChromeWebUIDataSource and *

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui_win.cc:56: class MetroizeHandler : public
WebUIMessageHandler,
please add doc comments to the classes in this file, as well as to the
definitions of all new (not overridden) methods (although don't bother for dtors
or no-arg/obvious ctors).

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui_win.cc:87: default_browser_worker_ = new
ShellIntegration::DefaultBrowserWorker(this);
please move this into the initalizer list:
: ALLOW_THIS_IN_INITIALIZER_LIST(default_browser_worker_(new
ShellIntegration::DefaultBrowserWorker(this))), ...

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui_win.cc:91: if
(default_browser_worker_.get())
why this check?

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui_win.cc:107: if
(default_browser_worker_.get()) {
why is this check needed?  default_browser_worker_ will always have an instance
as long as |this| is alive.

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui_win.cc:167: void
MetroizeHandler::SetDefaultWebClientUIState(
the order of method definitions should match the order of the declarations in
the class.  please move this and the next two methods up to just after
RegisterMessages.

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui_win.cc:282: static void
ShowMetroizerInDialog(Profile* profile, Browser* browser) {
no static

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui_win.cc:288: static void
AddMetroizerTab(Browser* browser) {
no static

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
File chrome/browser/ui/webui/metroizer_ui_win.h (right):

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui_win.h:26: 
nit: remove empty line

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/installer/u...
File chrome/installer/util/shell_util.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/installer/u...
chrome/installer/util/shell_util.cc:130: // TODO (gab): Extract the Windows 8
only registrations out of this function.
has it been a long time since you rebased?

[motek., 2012-06-20 22:02:28]

All done(I hope).

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/res...
File chrome/browser/resources/metroize/metroize.html (right):

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/res...
chrome/browser/resources/metroize/metroize.html:36: </div>
On 2012/06/20 19:10:11, caitkp wrote:
> Should this be a </span>?
Actually, div both places. Thank you.

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/res...
File chrome/browser/resources/metroize/metroize.js (right):

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/res...
chrome/browser/resources/metroize/metroize.js:5: <include
src="../shared/js/util.js"/>
On 2012/06/20 19:10:11, caitkp wrote:
> From looking at other chromium js files, it seems like this include should
> probably be specified in metroize.html (unless I'm missing something)?

I am not sure. I saw it both ways (see for instance ../sync_promo/sync_promo.js,
which served me as the example). But one thing is sure: it doesn't need to
appear in both places. I've removed it from here.

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/she...
File chrome/browser/shell_integration.h (right):

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/she...
chrome/browser/shell_integration.h:214: virtual bool SetAsDefault(bool
interactive_permitted) = 0;
On 2012/06/20 19:42:12, grt wrote:
> document meaning of return value

Done.

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/she...
chrome/browser/shell_integration.h:228: void ExecuteSetAsDefault(bool
interactive_permitted);
On 2012/06/20 19:42:12, grt wrote:
> document interactive_permitted

Done.

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/she...
chrome/browser/shell_integration.h:240: void CompleteSetAsDefault(bool
call_result);
On 2012/06/20 19:42:12, grt wrote:
> call_result -> succeeded?
> also, document

Done.

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
File chrome/browser/ui/startup/default_browser_prompt_win.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
chrome/browser/ui/startup/default_browser_prompt_win.cc:7: #include
"base/memory/weak_ptr.h"
On 2012/06/20 19:42:12, grt wrote:
> remove this

Done.

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
chrome/browser/ui/startup/default_browser_prompt_win.cc:8: #include
"chrome/browser/prefs/pref_service.h"
On 2012/06/20 19:42:12, grt wrote:
> and this
Can't do. This is used (profile_->GetPrefs())

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
chrome/browser/ui/startup/default_browser_prompt_win.cc:52: static void
CheckDefaultAndShowMetroizerCallback(Profile* profile,
On 2012/06/20 19:42:12, grt wrote:
> no static within unnamed namespace

Done.

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
chrome/browser/ui/startup/default_browser_prompt_win.cc:55: if
(!ShellIntegration::IsDefaultBrowser() ||
On 2012/06/20 19:42:12, grt wrote:
> || -> &&

Done.

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
chrome/browser/ui/startup/default_browser_prompt_win.cc:91: if
(ShellIntegration::SET_DEFAULT_INTERACTIVE ==
On 2012/06/20 19:42:12, grt wrote:
> nit: chromium would prefer that you write CanSetAsDefaultBrowser() ==
> SET_DEFAULT_INTERACTIVE.

Done.

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
File chrome/browser/ui/webui/metroizer_ui_win.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui_win.cc:39: ChromeWebUIDataSource *
CreateMetroizerUIHTMLSource() {
On 2012/06/20 19:42:12, grt wrote:
> no space between ChromeWebUIDataSource and *

Done.

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui_win.cc:56: class MetroizeHandler : public
WebUIMessageHandler,
On 2012/06/20 19:42:12, grt wrote:
> please add doc comments to the classes in this file, as well as to the
> definitions of all new (not overridden) methods (although don't bother for
dtors
> or no-arg/obvious ctors).

Done.

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui_win.cc:87: default_browser_worker_ = new
ShellIntegration::DefaultBrowserWorker(this);
On 2012/06/20 19:42:12, grt wrote:
> please move this into the initalizer list:
> : ALLOW_THIS_IN_INITIALIZER_LIST(default_browser_worker_(new
> ShellIntegration::DefaultBrowserWorker(this))), ...

Done.

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui_win.cc:91: if
(default_browser_worker_.get())
On 2012/06/20 19:42:12, grt wrote:
> why this check?
Not needed anymore. Done.

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui_win.cc:107: if
(default_browser_worker_.get()) {
On 2012/06/20 19:42:12, grt wrote:
> why is this check needed?  default_browser_worker_ will always have an
instance
> as long as |this| is alive.

Done.

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui_win.cc:167: void
MetroizeHandler::SetDefaultWebClientUIState(
On 2012/06/20 19:42:12, grt wrote:
> the order of method definitions should match the order of the declarations in
> the class.  please move this and the next two methods up to just after
> RegisterMessages.

Done.

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui_win.cc:282: static void
ShowMetroizerInDialog(Profile* profile, Browser* browser) {
On 2012/06/20 19:42:12, grt wrote:
> no static

Done.

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui_win.cc:288: static void
AddMetroizerTab(Browser* browser) {
On 2012/06/20 19:42:12, grt wrote:
> no static

Done.

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
File chrome/browser/ui/webui/metroizer_ui_win.h (right):

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui_win.h:26: 
On 2012/06/20 19:42:12, grt wrote:
> nit: remove empty line

Done.

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/installer/u...
File chrome/installer/util/shell_util.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/21004/chrome/installer/u...
chrome/installer/util/shell_util.cc:130: // TODO (gab): Extract the Windows 8
only registrations out of this function.
On 2012/06/20 19:42:12, grt wrote:
> has it been a long time since you rebased?

Not really. This is some git accident, gab@ says likely because I yanked a
branch out. This should be fixed now.

[motek., 2012-06-20 22:17:04]

jhawkins: OWNERS chrome/browser/ui/webui, chrome/browser/resources
sky: OWNERS chrome/browser/ui

It does seem we are now down to real nits with original review and grt@
suggested I bug you guys today since this is somehow urgent (not desperately so,
but still).

[sky, 2012-06-20 22:34:45]

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/browser/ui/...
File chrome/browser/ui/startup/default_browser_prompt_win.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/browser/ui/...
chrome/browser/ui/startup/default_browser_prompt_win.cc:51: void
CheckDefaultAndShowMetroizerCallback(Profile* profile,
nit: keep SetMetroBrowserFlowLauncher definition and declaration together. Move
this after Observe.

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/browser/ui/...
chrome/browser/ui/startup/default_browser_prompt_win.cc:57:
MetroizerUI::Show(profile, browser, dialog);
MetroizerUI is an awful name. How about something more meaningful.

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/browser/ui/...
chrome/browser/ui/startup/default_browser_prompt_win.cc:78:
BrowserThread::PostTask(
Why is this delayed? Its entirely possible the browser and/or profile is deleted
out from under you.

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/browser/ui/...
File chrome/browser/ui/webui/metroizer_ui_win.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui_win.cc:306:
MetroizerUI::MetroizerUI(content::WebUI* web_ui) : WebUIController(web_ui) {
Does anyone actually create an instance of this?

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui_win.cc:318:
base::Bind(&ShowMetroizerInDialog, profile, browser));
As mentioned, there are timing problems with posting a delayed task.

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/browser/ui/...
File chrome/browser/ui/webui/metroizer_ui_win.h (right):

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui_win.h:5: #ifndef
CHROME_BROWSER_UI_WEBUI_METROIZER_UI_WIN_H_
This file should be metroizer_ui. Although as mentioned, rename this class,
MetroizerUI is totally unclear. Additionally add a description.

[Cait (Slow), 2012-06-21 00:37:01]

lgtm

js/html/css LGTM

[grt (UTC plus 2), 2012-06-21 15:18:43]

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/browser/ext...
File chrome/browser/external_protocol/external_protocol_handler.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/browser/ext...
chrome/browser/external_protocol/external_protocol_handler.cc:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
you can remove this file from the CL if this is the only change left in it

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/browser/ui/...
File chrome/browser/ui/webui/metroizer_ui_win.h (right):

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui_win.h:5: #ifndef
CHROME_BROWSER_UI_WEBUI_METROIZER_UI_WIN_H_
On 2012/06/20 22:34:45, sky wrote:
> This file should be metroizer_ui. Although as mentioned, rename this class,
> MetroizerUI is totally unclear. Additionally add a description.

i had suggested having it be _win.{cc,h} so that it would be excluded from
non-windows builds via filename_rules.gypi.  was this bad advice?  my apologies
if so.

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/installer/u...
File chrome/installer/util/shell_util.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/installer/u...
chrome/installer/util/shell_util.cc:616: return SUCCEEDED(hr);
SUCCEEDED(hr) -> true

[motek., 2012-06-21 18:28:10]

All issues but one addressed as suggested. Clarification requested re: naming of
a platform-specific h file.

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/browser/ext...
File chrome/browser/external_protocol/external_protocol_handler.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/browser/ext...
chrome/browser/external_protocol/external_protocol_handler.cc:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
On 2012/06/21 15:18:43, grt wrote:
> you can remove this file from the CL if this is the only change left in it

Done.

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/browser/ui/...
File chrome/browser/ui/startup/default_browser_prompt_win.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/browser/ui/...
chrome/browser/ui/startup/default_browser_prompt_win.cc:51: void
CheckDefaultAndShowMetroizerCallback(Profile* profile,
On 2012/06/20 22:34:45, sky wrote:
> nit: keep SetMetroBrowserFlowLauncher definition and declaration together.
Move
> this after Observe.
Right. We don't really need this check anymore. It has been collapsed into
'Observe'.

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/browser/ui/...
chrome/browser/ui/startup/default_browser_prompt_win.cc:57:
MetroizerUI::Show(profile, browser, dialog);
On 2012/06/20 22:34:45, sky wrote:
> MetroizerUI is an awful name. How about something more meaningful.
Oh, bother. This was my favourite part. Gone now.

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/browser/ui/...
chrome/browser/ui/startup/default_browser_prompt_win.cc:78:
BrowserThread::PostTask(
On 2012/06/20 22:34:45, sky wrote:
> Why is this delayed? Its entirely possible the browser and/or profile is
deleted
> out from under you.
I think that these 'is default' checks should be undertaken from FILE thread. On
the other hand, the rest (Observe and showing stuff) happens in the UI thread.
That was the immediate reason the call was spun off like that. 
However, I think the check itself is spurious, because this is verified right
before creating the launcher. I collapsed it into this function here. This does
simplify things a bit.

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/browser/ui/...
File chrome/browser/ui/webui/metroizer_ui_win.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui_win.cc:306:
MetroizerUI::MetroizerUI(content::WebUI* web_ui) : WebUIController(web_ui) {
On 2012/06/20 22:34:45, sky wrote:
> Does anyone actually create an instance of this?
This is left in in the dev for user studies. Display mode (in-tab vs. dialog)
can be fiddled with through preferences. I intend to remove one of these paths
once this is settled 100%. I left a TODO to this effect in
chrome/common/pref_names.cc.

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui_win.cc:318:
base::Bind(&ShowMetroizerInDialog, profile, browser));
On 2012/06/20 22:34:45, sky wrote:
> As mentioned, there are timing problems with posting a delayed task.
Right. Now that this is bound to be invoked from UI thread, there is no point in
doing this. Rolled into a single function.

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/browser/ui/...
File chrome/browser/ui/webui/metroizer_ui_win.h (right):

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui_win.h:5: #ifndef
CHROME_BROWSER_UI_WEBUI_METROIZER_UI_WIN_H_
On 2012/06/21 15:18:43, grt wrote:
> On 2012/06/20 22:34:45, sky wrote:
> > This file should be metroizer_ui. Although as mentioned, rename this class,
> > MetroizerUI is totally unclear. Additionally add a description.
> 
> i had suggested having it be _win.{cc,h} so that it would be excluded from
> non-windows builds via filename_rules.gypi.  was this bad advice?  my
apologies
> if so.
I renamed the class and the file (and pretty much anything that has 'metroize'
in it. I'd appreciate a definite clarification about this _win part here.

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/installer/u...
File chrome/installer/util/shell_util.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/installer/u...
chrome/installer/util/shell_util.cc:616: return SUCCEEDED(hr);
On 2012/06/21 15:18:43, grt wrote:
> SUCCEEDED(hr) -> true

Done.

[sky, 2012-06-21 19:39:36]

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/browser/ui/...
File chrome/browser/ui/webui/metroizer_ui_win.h (right):

https://chromiumcodereview.appspot.com/10539169/diff/33002/chrome/browser/ui/...
chrome/browser/ui/webui/metroizer_ui_win.h:5: #ifndef
CHROME_BROWSER_UI_WEBUI_METROIZER_UI_WIN_H_
On 2012/06/21 18:28:10, motek. wrote:
> On 2012/06/21 15:18:43, grt wrote:
> > On 2012/06/20 22:34:45, sky wrote:
> > > This file should be metroizer_ui. Although as mentioned, rename this
class,
> > > MetroizerUI is totally unclear. Additionally add a description.
> > 
> > i had suggested having it be _win.{cc,h} so that it would be excluded from
> > non-windows builds via filename_rules.gypi.  was this bad advice?  my
> apologies
> > if so.
> I renamed the class and the file (and pretty much anything that has 'metroize'
> in it. I'd appreciate a definite clarification about this _win part here.

You don't need _win in the name, you just need to exclude the file where
appropriate in the gyp file.

[arv (Not doing code reviews), 2012-06-21 20:11:35]

The CSS needs to be RTL aware. Any time you use left/right you need to take care
of rtl

https://chromiumcodereview.appspot.com/10539169/diff/45001/chrome/browser/res...
File chrome/browser/resources/set_as_metro_default.css (right):

https://chromiumcodereview.appspot.com/10539169/diff/45001/chrome/browser/res...
chrome/browser/resources/set_as_metro_default.css:77: #metro-setup-overlay *
.content-area {
Skip the * ?

https://chromiumcodereview.appspot.com/10539169/diff/45001/chrome/browser/res...
chrome/browser/resources/set_as_metro_default.css:79: text-align: right;
RTL

text-align: end;

https://chromiumcodereview.appspot.com/10539169/diff/45001/chrome/browser/res...
chrome/browser/resources/set_as_metro_default.css:121: -webkit-box-shadow: none;
Are these the standard look and feel? If so can you just include the shared css?

https://chromiumcodereview.appspot.com/10539169/diff/45001/chrome/browser/res...
chrome/browser/resources/set_as_metro_default.css:151: margin-left: 30px;
Use -webkit-margin-start and end

https://chromiumcodereview.appspot.com/10539169/diff/45001/chrome/browser/res...
File chrome/browser/resources/set_as_metro_default.html (right):

https://chromiumcodereview.appspot.com/10539169/diff/45001/chrome/browser/res...
chrome/browser/resources/set_as_metro_default.html:2: Use of this source code is
governed by a BSD-style license that can be
DOCTYPE must be on the first line

https://chromiumcodereview.appspot.com/10539169/diff/45001/chrome/browser/res...
chrome/browser/resources/set_as_metro_default.html:23: <div
id="metro-setup-outer-container">
Too many ids? Do you really need an id on every element?

[motek., 2012-06-21 21:34:41]

https://chromiumcodereview.appspot.com/10539169/diff/45001/chrome/browser/res...
File chrome/browser/resources/set_as_metro_default.css (right):

https://chromiumcodereview.appspot.com/10539169/diff/45001/chrome/browser/res...
chrome/browser/resources/set_as_metro_default.css:77: #metro-setup-overlay *
.content-area {
On 2012/06/21 20:11:35, arv wrote:
> Skip the * ?

Done.

https://chromiumcodereview.appspot.com/10539169/diff/45001/chrome/browser/res...
chrome/browser/resources/set_as_metro_default.css:79: text-align: right;
On 2012/06/21 20:11:35, arv wrote:
> RTL
> 
> text-align: end;

Done.

https://chromiumcodereview.appspot.com/10539169/diff/45001/chrome/browser/res...
chrome/browser/resources/set_as_metro_default.css:121: -webkit-box-shadow: none;
On 2012/06/21 20:11:35, arv wrote:
> Are these the standard look and feel? If so can you just include the shared
css?
I am not sure. I am *really* new to CSS, I learned it to write this. Help!
Anyway, I lifted this design from
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/resources/sync...

The file itself was not general enough for me to include it and I am not sure
enough for myself to take up on generalizing it, especially not for a prototype.

If required, I'd rather do it in a separate CL to avoid touching unrelated stuff
here.

https://chromiumcodereview.appspot.com/10539169/diff/45001/chrome/browser/res...
chrome/browser/resources/set_as_metro_default.css:151: margin-left: 30px;
On 2012/06/21 20:11:35, arv wrote:
> Use -webkit-margin-start and end

Done.

https://chromiumcodereview.appspot.com/10539169/diff/45001/chrome/browser/res...
File chrome/browser/resources/set_as_metro_default.html (right):

https://chromiumcodereview.appspot.com/10539169/diff/45001/chrome/browser/res...
chrome/browser/resources/set_as_metro_default.html:2: Use of this source code is
governed by a BSD-style license that can be
On 2012/06/21 20:11:35, arv wrote:
> DOCTYPE must be on the first line

Done.

https://chromiumcodereview.appspot.com/10539169/diff/45001/chrome/browser/res...
chrome/browser/resources/set_as_metro_default.html:23: <div
id="metro-setup-outer-container">
On 2012/06/21 20:11:35, arv wrote:
> Too many ids? Do you really need an id on every element?

Indeed, some of them were spurious. Removed.

[arv (Not doing code reviews), 2012-06-21 22:08:24]

https://chromiumcodereview.appspot.com/10539169/diff/52001/chrome/browser/res...
File chrome/browser/resources/set_as_metro_default.html (right):

https://chromiumcodereview.appspot.com/10539169/diff/52001/chrome/browser/res...
chrome/browser/resources/set_as_metro_default.html:34: i18n-content="flowNext"/>
Invalid HTML. HTML does not support /> and buttons require a closing tag.

https://chromiumcodereview.appspot.com/10539169/diff/52001/chrome/browser/res...
chrome/browser/resources/set_as_metro_default.html:35: <button
id="return-button" i18n-content="flowCancel"/>
here too

[motek., 2012-06-21 22:51:21]

https://chromiumcodereview.appspot.com/10539169/diff/52001/chrome/browser/res...
File chrome/browser/resources/set_as_metro_default.html (right):

https://chromiumcodereview.appspot.com/10539169/diff/52001/chrome/browser/res...
chrome/browser/resources/set_as_metro_default.html:34: i18n-content="flowNext"/>
On 2012/06/21 22:08:24, arv wrote:
> Invalid HTML. HTML does not support /> and buttons require a closing tag.
Oops. We live and learn. Thanks.

https://chromiumcodereview.appspot.com/10539169/diff/52001/chrome/browser/res...
chrome/browser/resources/set_as_metro_default.html:35: <button
id="return-button" i18n-content="flowCancel"/>
On 2012/06/21 22:08:24, arv wrote:
> here too

Done.

[grt (UTC plus 2), 2012-06-21 23:12:08]

https://chromiumcodereview.appspot.com/10539169/diff/59001/chrome/browser/ui/...
File chrome/browser/ui/startup/default_browser_prompt_win.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/59001/chrome/browser/ui/...
chrome/browser/ui/startup/default_browser_prompt_win.cc:79:
ShellIntegration::NOT_DEFAULT_WEB_CLIENT) &&
nit: indent one more space

https://chromiumcodereview.appspot.com/10539169/diff/59001/chrome/browser/ui/...
chrome/browser/ui/startup/default_browser_prompt_win.cc:81:
ShellIntegration::SET_DEFAULT_INTERACTIVE)) {
nit: indent one less space

https://chromiumcodereview.appspot.com/10539169/diff/59001/chrome/browser/ui/...
File chrome/browser/ui/webui/set_as_metro_default_ui.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/59001/chrome/browser/ui/...
chrome/browser/ui/webui/set_as_metro_default_ui.cc:80: // Handler for the 'No,
thanks' (Cancel) button.
please add blank lines between each method and the comment for the following
method.  like:

  // Spam the floozit.
  void SpamTheThang();

  // Barf the toodongle
  bool BarfEverywhere();

https://chromiumcodereview.appspot.com/10539169/diff/59001/chrome/browser/ui/...
File chrome/browser/ui/webui/set_as_metro_default_ui.h (right):

https://chromiumcodereview.appspot.com/10539169/diff/59001/chrome/browser/ui/...
chrome/browser/ui/webui/set_as_metro_default_ui.h:5: #ifndef
CHROME_BROWSER_UI_WEBUI_SET_AS_METRO_DEFAULT_UI_H_
SetAsMetroDefaultUI -> SetAsDefaultBrowserUI or something like that

Chrome being available for metro-mode is a side effect of being the default
browser on Windows 8.  The motivation here is that a program can't become the
default browser on Windows 8+ without involving an interactive Windows Shell
dialog.  True, one reason we're jumping through these hoops is that we think the
user will enjoy using Chrome in metro mode, but technically all we're doing is
prompting the user to make Chrome their default browser.

[motek., 2012-06-22 16:42:13]

https://chromiumcodereview.appspot.com/10539169/diff/59001/chrome/browser/ui/...
File chrome/browser/ui/startup/default_browser_prompt_win.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/59001/chrome/browser/ui/...
chrome/browser/ui/startup/default_browser_prompt_win.cc:79:
ShellIntegration::NOT_DEFAULT_WEB_CLIENT) &&
On 2012/06/21 23:12:08, grt wrote:
> nit: indent one more space
I hate Visual Studio with all my hart! Done.

https://chromiumcodereview.appspot.com/10539169/diff/59001/chrome/browser/ui/...
chrome/browser/ui/startup/default_browser_prompt_win.cc:81:
ShellIntegration::SET_DEFAULT_INTERACTIVE)) {
On 2012/06/21 23:12:08, grt wrote:
> nit: indent one less space

Done.

https://chromiumcodereview.appspot.com/10539169/diff/59001/chrome/browser/ui/...
File chrome/browser/ui/webui/set_as_metro_default_ui.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/59001/chrome/browser/ui/...
chrome/browser/ui/webui/set_as_metro_default_ui.cc:80: // Handler for the 'No,
thanks' (Cancel) button.
On 2012/06/21 23:12:08, grt wrote:
> please add blank lines between each method and the comment for the following
> method.  like:
> 
>   // Spam the floozit.
>   void SpamTheThang();
> 
>   // Barf the toodongle
>   bool BarfEverywhere();

Done.

https://chromiumcodereview.appspot.com/10539169/diff/59001/chrome/browser/ui/...
File chrome/browser/ui/webui/set_as_metro_default_ui.h (right):

https://chromiumcodereview.appspot.com/10539169/diff/59001/chrome/browser/ui/...
chrome/browser/ui/webui/set_as_metro_default_ui.h:5: #ifndef
CHROME_BROWSER_UI_WEBUI_SET_AS_METRO_DEFAULT_UI_H_
On 2012/06/21 23:12:08, grt wrote:
> SetAsMetroDefaultUI -> SetAsDefaultBrowserUI or something like that
> 
> Chrome being available for metro-mode is a side effect of being the default
> browser on Windows 8.  The motivation here is that a program can't become the
> default browser on Windows 8+ without involving an interactive Windows Shell
> dialog.  True, one reason we're jumping through these hoops is that we think
the
> user will enjoy using Chrome in metro mode, but technically all we're doing is
> prompting the user to make Chrome their default browser.
We also explicitly restart the program in Metro mode upon success, so I thought
putting 'metro' in there would be perfectly appropriate. But...
Done.

[grt (UTC plus 2), 2012-06-22 17:02:00]

lgtm.  thanks for your patience.

[sky, 2012-06-22 19:19:32]

LGTM

https://chromiumcodereview.appspot.com/10539169/diff/42026/chrome/browser/she...
File chrome/browser/shell_integration.h (right):

https://chromiumcodereview.appspot.com/10539169/diff/42026/chrome/browser/she...
chrome/browser/shell_integration.h:174: virtual void
OnSetAsDefaultConcluded(bool succeeded) { }
{ } -> {}

https://chromiumcodereview.appspot.com/10539169/diff/42026/chrome/browser/she...
chrome/browser/shell_integration.h:177: virtual bool IsOwnedByWorker() { return
false; }
This and 180 should not be inlined.

https://chromiumcodereview.appspot.com/10539169/diff/42026/chrome/browser/she...
chrome/browser/shell_integration.h:232: //     in context where user interaction
is required (CanSetAsDefault*
No extra indentation (same comment on 247).

https://chromiumcodereview.appspot.com/10539169/diff/42026/chrome/browser/ui/...
File chrome/browser/ui/startup/default_browser_prompt_win.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/42026/chrome/browser/ui/...
chrome/browser/ui/startup/default_browser_prompt_win.cc:63:
registrar_.Remove(this, content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME,
registrar_.RemoveAll(). Although since you're about to 'delete this', do you
need to do anything?

https://chromiumcodereview.appspot.com/10539169/diff/42026/chrome/browser/ui/...
chrome/browser/ui/startup/default_browser_prompt_win.cc:84: // this action as a
delayed task (including the check).
The second sentence here is confusing. In particular the 'delayed task'. Please
update it, or just remove it.

[James Hawkins, 2012-06-22 20:04:52]

resources LGTM with nits.

https://chromiumcodereview.appspot.com/10539169/diff/42026/chrome/browser/res...
File chrome/browser/resources/set_as_default_browser.css (right):

https://chromiumcodereview.appspot.com/10539169/diff/42026/chrome/browser/res...
chrome/browser/resources/set_as_default_browser.css:88: 
nit: Remove blank line.

https://chromiumcodereview.appspot.com/10539169/diff/42026/chrome/browser/res...
File chrome/browser/resources/set_as_default_browser.html (right):

https://chromiumcodereview.appspot.com/10539169/diff/42026/chrome/browser/res...
chrome/browser/resources/set_as_default_browser.html:15: 
nit: Remove all blank lines.

https://chromiumcodereview.appspot.com/10539169/diff/42026/chrome/browser/res...
chrome/browser/resources/set_as_default_browser.html:27: <div>
nit: Remove extra div.

[motek., 2012-06-22 20:40:18]

All fixed, I believe. Thank you.

https://chromiumcodereview.appspot.com/10539169/diff/42026/chrome/browser/res...
File chrome/browser/resources/set_as_default_browser.css (right):

https://chromiumcodereview.appspot.com/10539169/diff/42026/chrome/browser/res...
chrome/browser/resources/set_as_default_browser.css:88: 
On 2012/06/22 20:04:52, James Hawkins wrote:
> nit: Remove blank line.

Done.

https://chromiumcodereview.appspot.com/10539169/diff/42026/chrome/browser/res...
File chrome/browser/resources/set_as_default_browser.html (right):

https://chromiumcodereview.appspot.com/10539169/diff/42026/chrome/browser/res...
chrome/browser/resources/set_as_default_browser.html:15: 
On 2012/06/22 20:04:52, James Hawkins wrote:
> nit: Remove all blank lines.

Done.

https://chromiumcodereview.appspot.com/10539169/diff/42026/chrome/browser/res...
chrome/browser/resources/set_as_default_browser.html:27: <div>
On 2012/06/22 20:04:52, James Hawkins wrote:
> nit: Remove extra div.

Done.

https://chromiumcodereview.appspot.com/10539169/diff/42026/chrome/browser/she...
File chrome/browser/shell_integration.h (right):

https://chromiumcodereview.appspot.com/10539169/diff/42026/chrome/browser/she...
chrome/browser/shell_integration.h:174: virtual void
OnSetAsDefaultConcluded(bool succeeded) { }
On 2012/06/22 19:19:32, sky wrote:
> { } -> {}

Done.

https://chromiumcodereview.appspot.com/10539169/diff/42026/chrome/browser/she...
chrome/browser/shell_integration.h:177: virtual bool IsOwnedByWorker() { return
false; }
On 2012/06/22 19:19:32, sky wrote:
> This and 180 should not be inlined.

Done.

https://chromiumcodereview.appspot.com/10539169/diff/42026/chrome/browser/she...
chrome/browser/shell_integration.h:232: //     in context where user interaction
is required (CanSetAsDefault*
On 2012/06/22 19:19:32, sky wrote:
> No extra indentation (same comment on 247).

Done.

https://chromiumcodereview.appspot.com/10539169/diff/42026/chrome/browser/ui/...
File chrome/browser/ui/startup/default_browser_prompt_win.cc (right):

https://chromiumcodereview.appspot.com/10539169/diff/42026/chrome/browser/ui/...
chrome/browser/ui/startup/default_browser_prompt_win.cc:63:
registrar_.Remove(this, content::NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME,
On 2012/06/22 19:19:32, sky wrote:
> registrar_.RemoveAll(). Although since you're about to 'delete this', do you
> need to do anything?
I am not sure. I got this from first run bubble, this was what it did. Seemed
logical to explicitly remove. Changed to RemoveAll.

https://chromiumcodereview.appspot.com/10539169/diff/42026/chrome/browser/ui/...
chrome/browser/ui/startup/default_browser_prompt_win.cc:84: // this action as a
delayed task (including the check).
On 2012/06/22 19:19:32, sky wrote:
> The second sentence here is confusing. In particular the 'delayed task'.
Please
> update it, or just remove it.

Done.

[commit-bot: I haz the power, 2012-06-22 20:41:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/motek@chromium.org/10539169/66001

[commit-bot: I haz the power, 2012-06-22 22:23:40]

Change committed as 143729