Hide the main window when Set-As-Default dialog is present.

Hide the main window when Set-As-Default dialog is present.

This affect startup sequences only. 

BUG=134430
TEST=The dialog on win8 should appear without the main window.  If the user chooses not to make Chrome default, it should continue to desktop Chrome. 


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

[motek., 2012-07-05 17:41:30]

Hi, I would appreciate if you could review this one. 

M.

[grt (UTC plus 2), 2012-07-05 20:08:47]

i'm not fit to review set_as_default_browser_ui.cc.  otherwise, see comments
below.

https://chromiumcodereview.appspot.com/10696093/diff/4003/chrome/browser/ui/s...
File chrome/browser/ui/startup/default_browser_prompt_win.cc (right):

https://chromiumcodereview.appspot.com/10696093/diff/4003/chrome/browser/ui/s...
chrome/browser/ui/startup/default_browser_prompt_win.cc:89:
(ShellIntegration::IsDefaultBrowser() ==
please swap the CanSetAsDefaultBrowser and IsDefaultBrowser portions of this
logic so that the cheap test is performed before the expensive test.  actually,
CanSetAsDefaultBrowser is probably even cheaper than the prefs lookup, so might
as well move it before that one, too.

https://chromiumcodereview.appspot.com/10696093/diff/4003/chrome/browser/ui/s...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://chromiumcodereview.appspot.com/10696093/diff/4003/chrome/browser/ui/s...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:723: bool as_first_run
= is_first_run_ ||
as discussed in person, i think it's cleaner to move this policy logic out into
ChromeBrowserMainParts::PreCreateThreadsImpl within the if (is_first_run_) block
so that some sort of "Start Hidden" bit is flipped on the creator when the
conditions are right for such.  then this can become more like:

  if (!browser_creator_->ShouldStartHidden()) {
    ...
  }

[cpu_(ooo_6.6-7.5), 2012-07-06 23:30:47]

http://codereview.chromium.org/10696093/diff/4003/chrome/browser/ui/webui/set...
File chrome/browser/ui/webui/set_as_default_browser_ui.cc (right):

http://codereview.chromium.org/10696093/diff/4003/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/set_as_default_browser_ui.cc:181: it !=
BrowserList::end(); ++it) {
iterating over the browsers... Please ask jam@ if this is the way to go.

[jam, 2012-07-12 18:38:40]

apologies for the delay, this slipped under my radar

http://codereview.chromium.org/10696093/diff/4003/chrome/browser/ui/webui/set...
File chrome/browser/ui/webui/set_as_default_browser_ui.cc (right):

http://codereview.chromium.org/10696093/diff/4003/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/set_as_default_browser_ui.cc:181: it !=
BrowserList::end(); ++it) {
On 2012/07/06 23:30:47, cpu wrote:
> iterating over the browsers... Please ask jam@ if this is the way to go.

that's fine. once we have different contexts for metro/desktop, from this webui
page we should be able to extra the context and then use that to get to the
proper BrowserList.

[motek., 2012-08-08 14:28:35]

Addressed comment. Since grt@ is vacationing, moved him to CC and added
robertshield@ instead.

https://chromiumcodereview.appspot.com/10696093/diff/4003/chrome/browser/ui/s...
File chrome/browser/ui/startup/default_browser_prompt_win.cc (right):

https://chromiumcodereview.appspot.com/10696093/diff/4003/chrome/browser/ui/s...
chrome/browser/ui/startup/default_browser_prompt_win.cc:89:
(ShellIntegration::IsDefaultBrowser() ==
On 2012/07/05 20:08:47, grt wrote:
> please swap the CanSetAsDefaultBrowser and IsDefaultBrowser portions of this
> logic so that the cheap test is performed before the expensive test. 
actually,
> CanSetAsDefaultBrowser is probably even cheaper than the prefs lookup, so
might
> as well move it before that one, too.
This part is gone altogether now. Changed above following the suggestion.

https://chromiumcodereview.appspot.com/10696093/diff/4003/chrome/browser/ui/s...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://chromiumcodereview.appspot.com/10696093/diff/4003/chrome/browser/ui/s...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:723: bool as_first_run
= is_first_run_ ||
On 2012/07/05 20:08:47, grt wrote:
> as discussed in person, i think it's cleaner to move this policy logic out
into
> ChromeBrowserMainParts::PreCreateThreadsImpl within the if (is_first_run_)
block
> so that some sort of "Start Hidden" bit is flipped on the creator when the
> conditions are right for such.  then this can become more like:
> 
>   if (!browser_creator_->ShouldStartHidden()) {
>     ...
>   }
Moved to the creator, but as a simple setter/getter.

https://chromiumcodereview.appspot.com/10696093/diff/4003/chrome/browser/ui/w...
File chrome/browser/ui/webui/set_as_default_browser_ui.cc (right):

https://chromiumcodereview.appspot.com/10696093/diff/4003/chrome/browser/ui/w...
chrome/browser/ui/webui/set_as_default_browser_ui.cc:181: it !=
BrowserList::end(); ++it) {
On 2012/07/06 23:30:47, cpu wrote:
> iterating over the browsers... Please ask jam@ if this is the way to go.

Done.

https://chromiumcodereview.appspot.com/10696093/diff/4003/chrome/browser/ui/w...
chrome/browser/ui/webui/set_as_default_browser_ui.cc:181: it !=
BrowserList::end(); ++it) {
On 2012/07/12 18:38:40, John Abd-El-Malek wrote:
> On 2012/07/06 23:30:47, cpu wrote:
> > iterating over the browsers... Please ask jam@ if this is the way to go.
> 
> that's fine. once we have different contexts for metro/desktop, from this
webui
> page we should be able to extra the context and then use that to get to the
> proper BrowserList.
Thank for for the comment. Keeping the iteration.

[robertshield, 2012-08-08 18:09:38]

looking good, couple of nits/questions

https://chromiumcodereview.appspot.com/10696093/diff/13001/chrome/browser/ui/...
File chrome/browser/ui/webui/set_as_default_browser_ui.cc (right):

https://chromiumcodereview.appspot.com/10696093/diff/13001/chrome/browser/ui/...
chrome/browser/ui/webui/set_as_default_browser_ui.cc:209: ResponseDelegate {
nit: indent, why private inheritance?

https://chromiumcodereview.appspot.com/10696093/diff/13001/chrome/browser/ui/...
chrome/browser/ui/webui/set_as_default_browser_ui.cc:307: // In such case we
just carry on with normal chrome session. However, for
nit: with normal -> with a normal

https://chromiumcodereview.appspot.com/10696093/diff/13001/chrome/browser/ui/...
chrome/browser/ui/webui/set_as_default_browser_ui.cc:317:
contents->GetView()->SetInitialFocus();
question: do we want to set focus to each window in sequence, or do we want to
set focus to just the last one?

[motek., 2012-08-08 19:55:36]

https://chromiumcodereview.appspot.com/10696093/diff/13001/chrome/browser/ui/...
File chrome/browser/ui/webui/set_as_default_browser_ui.cc (right):

https://chromiumcodereview.appspot.com/10696093/diff/13001/chrome/browser/ui/...
chrome/browser/ui/webui/set_as_default_browser_ui.cc:209: ResponseDelegate {
On 2012/08/08 18:09:38, robertshield wrote:
> nit: indent, why private inheritance?
Habits. Changed to public.

https://chromiumcodereview.appspot.com/10696093/diff/13001/chrome/browser/ui/...
chrome/browser/ui/webui/set_as_default_browser_ui.cc:307: // In such case we
just carry on with normal chrome session. However, for
On 2012/08/08 18:09:38, robertshield wrote:
> nit: with normal -> with a normal

Done.

https://chromiumcodereview.appspot.com/10696093/diff/13001/chrome/browser/ui/...
chrome/browser/ui/webui/set_as_default_browser_ui.cc:317:
contents->GetView()->SetInitialFocus();
On 2012/08/08 18:09:38, robertshield wrote:
> question: do we want to set focus to each window in sequence, or do we want to
> set focus to just the last one?
Good question. I assumed the initial focus is basically location of input cursor
*in the window* and not the window selection. In that case, we should do it for
all existing windows (in practice there is only one at this point).

[robertshield, 2012-08-08 20:12:19]

lgtm

[sky, 2012-08-08 21:24:13]

https://chromiumcodereview.appspot.com/10696093/diff/17002/chrome/browser/ui/...
File chrome/browser/ui/startup/startup_browser_creator.h (right):

https://chromiumcodereview.appspot.com/10696093/diff/17002/chrome/browser/ui/...
chrome/browser/ui/startup/startup_browser_creator.h:98: bool
show_main_browser_window() {
const

https://chromiumcodereview.appspot.com/10696093/diff/17002/chrome/browser/ui/...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://chromiumcodereview.appspot.com/10696093/diff/17002/chrome/browser/ui/...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:788: if
(!browser_creator_ || browser_creator_->show_main_browser_window()) {
Who is keeping chrome alive if you don't show a browser?

https://chromiumcodereview.appspot.com/10696093/diff/17002/chrome/browser/ui/...
File chrome/browser/ui/webui/set_as_default_browser_ui.cc (right):

https://chromiumcodereview.appspot.com/10696093/diff/17002/chrome/browser/ui/...
chrome/browser/ui/webui/set_as_default_browser_ui.cc:247: : profile_(profile),
browser_(browser), owns_handler_(true),
each variable on its own line.

https://chromiumcodereview.appspot.com/10696093/diff/17002/chrome/browser/ui/...
chrome/browser/ui/webui/set_as_default_browser_ui.cc:277:
handlers->push_back(handler_);
spacing is off.

https://chromiumcodereview.appspot.com/10696093/diff/17002/chrome/browser/ui/...
chrome/browser/ui/webui/set_as_default_browser_ui.cc:301:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
What happens if this is vetoed, and why does it need to be dleayed?

https://chromiumcodereview.appspot.com/10696093/diff/17002/chrome/browser/ui/...
chrome/browser/ui/webui/set_as_default_browser_ui.cc:310: for
(BrowserList::const_iterator it = BrowserList::begin();
Isn't there only going to be one browser you need to show? Can't this take a
handle to it?

[motek., 2012-08-09 14:26:56]

https://chromiumcodereview.appspot.com/10696093/diff/17002/chrome/browser/ui/...
File chrome/browser/ui/startup/startup_browser_creator.h (right):

https://chromiumcodereview.appspot.com/10696093/diff/17002/chrome/browser/ui/...
chrome/browser/ui/startup/startup_browser_creator.h:98: bool
show_main_browser_window() {
On 2012/08/08 21:24:13, sky wrote:
> const

Done.

https://chromiumcodereview.appspot.com/10696093/diff/17002/chrome/browser/ui/...
File chrome/browser/ui/startup/startup_browser_creator_impl.cc (right):

https://chromiumcodereview.appspot.com/10696093/diff/17002/chrome/browser/ui/...
chrome/browser/ui/startup/startup_browser_creator_impl.cc:788: if
(!browser_creator_ || browser_creator_->show_main_browser_window()) {
On 2012/08/08 21:24:13, sky wrote:
> Who is keeping chrome alive if you don't show a browser?
I believe the actual widget exists by this point (checked that). So, messages
should be pumped whether it is visible or not and it keeps itself alive.

https://chromiumcodereview.appspot.com/10696093/diff/17002/chrome/browser/ui/...
File chrome/browser/ui/webui/set_as_default_browser_ui.cc (right):

https://chromiumcodereview.appspot.com/10696093/diff/17002/chrome/browser/ui/...
chrome/browser/ui/webui/set_as_default_browser_ui.cc:247: : profile_(profile),
browser_(browser), owns_handler_(true),
On 2012/08/08 21:24:13, sky wrote:
> each variable on its own line.

Done.

https://chromiumcodereview.appspot.com/10696093/diff/17002/chrome/browser/ui/...
chrome/browser/ui/webui/set_as_default_browser_ui.cc:277:
handlers->push_back(handler_);
On 2012/08/08 21:24:13, sky wrote:
> spacing is off.

Done.

https://chromiumcodereview.appspot.com/10696093/diff/17002/chrome/browser/ui/...
chrome/browser/ui/webui/set_as_default_browser_ui.cc:301:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
On 2012/08/08 21:24:13, sky wrote:
> What happens if this is vetoed, and why does it need to be dleayed?
Good point, it doesn't. I copied & pasted it from code that run in FILE thread,
not needed anymore.

https://chromiumcodereview.appspot.com/10696093/diff/17002/chrome/browser/ui/...
chrome/browser/ui/webui/set_as_default_browser_ui.cc:310: for
(BrowserList::const_iterator it = BrowserList::begin();
On 2012/08/08 21:24:13, sky wrote:
> Isn't there only going to be one browser you need to show? Can't this take a
> handle to it?
Same excuse. Moved this from a place where there was no browser_ given. Fixed
now.

[sky, 2012-08-09 16:13:47]

https://chromiumcodereview.appspot.com/10696093/diff/18002/chrome/browser/ui/...
File chrome/browser/ui/webui/set_as_default_browser_ui.cc (right):

https://chromiumcodereview.appspot.com/10696093/diff/18002/chrome/browser/ui/...
chrome/browser/ui/webui/set_as_default_browser_ui.cc:69: };
Add a protected virtual destructor.

https://chromiumcodereview.appspot.com/10696093/diff/18002/chrome/browser/ui/...
chrome/browser/ui/webui/set_as_default_browser_ui.cc:311: BrowserWindow* window
= browser_->window();
Do you need to worry about browser being deleted out from under you?

https://chromiumcodereview.appspot.com/10696093/diff/18002/chrome/browser/ui/...
chrome/browser/ui/webui/set_as_default_browser_ui.cc:313: if (window)
This should never be NULL.

[motek., 2012-08-09 17:17:48]

https://chromiumcodereview.appspot.com/10696093/diff/18002/chrome/browser/ui/...
File chrome/browser/ui/webui/set_as_default_browser_ui.cc (right):

https://chromiumcodereview.appspot.com/10696093/diff/18002/chrome/browser/ui/...
chrome/browser/ui/webui/set_as_default_browser_ui.cc:69: };
On 2012/08/09 16:13:47, sky wrote:
> Add a protected virtual destructor.

Done.

https://chromiumcodereview.appspot.com/10696093/diff/18002/chrome/browser/ui/...
chrome/browser/ui/webui/set_as_default_browser_ui.cc:311: BrowserWindow* window
= browser_->window();
On 2012/08/09 16:13:47, sky wrote:
> Do you need to worry about browser being deleted out from under you?
I did, sort of. But if I am not mistaken, this code is IT (the popup that this
is a delegate of is associated with this instance of the browser). 
That said: would you think it would be better to revert to that iteration I had
here earlier?

https://chromiumcodereview.appspot.com/10696093/diff/18002/chrome/browser/ui/...
chrome/browser/ui/webui/set_as_default_browser_ui.cc:313: if (window)
On 2012/08/09 16:13:47, sky wrote:
> This should never be NULL.

Done.

[sky, 2012-08-09 19:37:38]

Leave it the way it is: LGTM

[commit-bot: I haz the power, 2012-08-09 20:17:07]

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

[commit-bot: I haz the power, 2012-08-09 21:38:08]

Try job failure for 10696093-25002 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-08-09 21:41:42]

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

[commit-bot: I haz the power, 2012-08-09 23:14:01]

Change committed as 150936