Settings reset prompt: the dialog should have focus when shown.

Settings reset prompt: the dialog should have focus when shown.

BUG=703701
Review-Url: https://codereview.chromium.org/2764033004
Cr-Commit-Position: refs/heads/master@{#458603}
Committed: https://chromium.googlesource.com/chromium/src/+/e33bb720f168fdb711b15b20832632970166a81b

[alito, 2017-03-21 18:23:52]

alito@chromium.org changed reviewers:
+ sky@chromium.org

[alito, 2017-03-21 18:23:53]

PTAL. Thx.

[sky, 2017-03-21 20:36:01]

https://codereview.chromium.org/2764033004/diff/1/chrome/browser/safe_browsin...
File
chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_controller.cc
(right):

https://codereview.chromium.org/2764033004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_controller.cc:78:
chrome::ScopedTabbedBrowserDisplayer displayer(profile);
If a new browser is created by ScopedTabbedBrowserDisplayer then I don't see you
adding anything to it, which would result in an empty tabbed-browser and all
sorts of problems. You should only use ScopedTabbedBrowserDisplayer  if you're
going to add a tab to it.

[alito2, 2017-03-21 20:44:16]

alito@google.com changed reviewers:
+ alito@google.com

[alito2, 2017-03-21 20:44:17]

Please see response to comment inline.

https://codereview.chromium.org/2764033004/diff/1/chrome/browser/safe_browsin...
File
chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_controller.cc
(right):

https://codereview.chromium.org/2764033004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_controller.cc:78:
chrome::ScopedTabbedBrowserDisplayer displayer(profile);
On 2017/03/21 20:36:01, sky wrote:
> If a new browser is created by ScopedTabbedBrowserDisplayer then I don't see
you
> adding anything to it, which would result in an empty tabbed-browser and all
> sorts of problems. You should only use ScopedTabbedBrowserDisplayer  if you're
> going to add a tab to it.

Just above this line, I do make sure that there is at least one non-incognito
tabbed browser already open. I think that should be enough to ensure that we
don't end up in a weird situation?

Another option is to explicitly use that tabbed browser found above and slightly
duplicate the code in the ScopedTabbedBrowserDisplayer's destructor.

[sky, 2017-03-21 20:52:18]

https://codereview.chromium.org/2764033004/diff/1/chrome/browser/safe_browsin...
File
chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_controller.cc
(right):

https://codereview.chromium.org/2764033004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_controller.cc:78:
chrome::ScopedTabbedBrowserDisplayer displayer(profile);
On 2017/03/21 20:44:17, alito2 wrote:
> On 2017/03/21 20:36:01, sky wrote:
> > If a new browser is created by ScopedTabbedBrowserDisplayer then I don't see
> you
> > adding anything to it, which would result in an empty tabbed-browser and all
> > sorts of problems. You should only use ScopedTabbedBrowserDisplayer  if
you're
> > going to add a tab to it.
> 
> Just above this line, I do make sure that there is at least one non-incognito
> tabbed browser already open. I think that should be enough to ensure that we
> don't end up in a weird situation?
> 
> Another option is to explicitly use that tabbed browser found above and
slightly
> duplicate the code in the ScopedTabbedBrowserDisplayer's destructor.

Indeed. In that case you should use the return value from FindTabbedBrowser and
you should have no need for ScopedTabbedBrowserDisplayer.

[alito, 2017-03-21 21:02:42]

No longer using the ScopedTabbedBrowserDisplayer. PTAL.

https://codereview.chromium.org/2764033004/diff/1/chrome/browser/safe_browsin...
File
chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_controller.cc
(right):

https://codereview.chromium.org/2764033004/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_controller.cc:78:
chrome::ScopedTabbedBrowserDisplayer displayer(profile);
On 2017/03/21 20:52:18, sky wrote:
> On 2017/03/21 20:44:17, alito2 wrote:
> > On 2017/03/21 20:36:01, sky wrote:
> > > If a new browser is created by ScopedTabbedBrowserDisplayer then I don't
see
> > you
> > > adding anything to it, which would result in an empty tabbed-browser and
all
> > > sorts of problems. You should only use ScopedTabbedBrowserDisplayer  if
> you're
> > > going to add a tab to it.
> > 
> > Just above this line, I do make sure that there is at least one
non-incognito
> > tabbed browser already open. I think that should be enough to ensure that we
> > don't end up in a weird situation?
> > 
> > Another option is to explicitly use that tabbed browser found above and
> slightly
> > duplicate the code in the ScopedTabbedBrowserDisplayer's destructor.
> 
> Indeed. In that case you should use the return value from FindTabbedBrowser
and
> you should have no need for ScopedTabbedBrowserDisplayer.

Done.

[sky, 2017-03-21 21:16:44]

LGTM

[alito, 2017-03-21 21:20:53]

The CQ bit was checked by alito@chromium.org

[commit-bot: I haz the power, 2017-03-21 21:21:43]

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

[commit-bot: I haz the power, 2017-03-21 21:46:07]

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

[commit-bot: I haz the power, 2017-03-21 21:46:08]

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

[alito, 2017-03-21 22:44:17]

The CQ bit was checked by alito@chromium.org

[commit-bot: I haz the power, 2017-03-21 22:44:53]

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

[commit-bot: I haz the power, 2017-03-21 23:07:57]

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

[commit-bot: I haz the power, 2017-03-21 23:07:58]

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

[alito, 2017-03-21 23:22:25]

The CQ bit was checked by alito@chromium.org

[commit-bot: I haz the power, 2017-03-21 23:22:43]

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

[commit-bot: I haz the power, 2017-03-21 23:40:44]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490138545491070,
"parent_rev": "a0a9840a39b61caca179486dcf20927fe99ad4a6", "commit_rev":
"e33bb720f168fdb711b15b20832632970166a81b"}

[commit-bot: I haz the power, 2017-03-21 23:42:25]

Description was changed from

==========
Settings reset prompt: the dialog should have focus when shown.

BUG=703701
==========

to

==========
Settings reset prompt: the dialog should have focus when shown.

BUG=703701

Review-Url: https://codereview.chromium.org/2764033004
Cr-Commit-Position: refs/heads/master@{#458603}
Committed:
https://chromium.googlesource.com/chromium/src/+/e33bb720f168fdb711b15b208326...
==========

[commit-bot: I haz the power, 2017-03-21 23:42:29]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/e33bb720f168fdb711b15b208326...