Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2090773002:
Improves first run dialog performance on Win8 (Closed)
DescriptionImproves first run dialog performance on Win8
BUG=616829
Committed: https://crrev.com/065fd8ffaebd901b6033aaa5d1d7693c3ff26d87
Cr-Commit-Position: refs/heads/master@{#407810}
Patch Set 1 #Patch Set 2 : add browser test #Patch Set 3 : fix presubmit check #Patch Set 4 #
Total comments: 20
Patch Set 5 #
Total comments: 21
Patch Set 6 #
Total comments: 5
Patch Set 7 : cr #Messages
Total messages: 27 (6 generated)
zmin@chromium.org changed reviewers: + grt@chromium.org
Hi Greg, Take a look if you have time or feel free to ignore if you don't. This CL simply replace the notification type with NOTIFICATION_BROWSER_WINDOW_READY. It does not have the race condition that previous CL have to address. Owen
Description was changed from ========== Improves first run dialog performance on Win8 BUG=616828 ========== to ========== Improves first run dialog performance on Win8 BUG=616829 ==========
zmin@chromium.org changed reviewers: + sky@chromium.org
Hi sky@ Can you please review this CL? Thanks, Owen
https://codereview.chromium.org/2090773002/diff/60001/chrome/browser/ui/start... File chrome/browser/ui/startup/default_browser_prompt_win.cc (right): https://codereview.chromium.org/2090773002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_prompt_win.cc:33: registrar_.Add(this, chrome::NOTIFICATION_BROWSER_WINDOW_READY, Use a BrowserListObserver instead of notification as notifications are deprecated. https://codereview.chromium.org/2090773002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/set_as_default_browser_ui.h (right): https://codereview.chromium.org/2090773002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/set_as_default_browser_ui.h:26: static views::Widget* GetDialogWidgetForTesting(); Instead of exposing this can you make Show() return the widget? Alternatively add a function you can call to see if visible. https://codereview.chromium.org/2090773002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/set_as_default_browser_ui_browsertest.cc (right): https://codereview.chromium.org/2090773002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/set_as_default_browser_ui_browsertest.cc:5: #include "chrome/browser/ui/webui/set_as_default_browser_ui.h" As this file is win only please name it as such. https://codereview.chromium.org/2090773002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/set_as_default_browser_ui_browsertest.cc:17: bool IsMainWindowVisible() { You shouldn't need a subclass just for this function, create a standalone function that is passed the browser(). https://codereview.chromium.org/2090773002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/set_as_default_browser_ui_browsertest.cc:41: EXPECT_NE(nullptr, dialog_widget); ASSERT_TRUE(dialog_widget), otherwise if dialog_widget is null the next line crashes. https://codereview.chromium.org/2090773002/diff/60001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/2090773002/diff/60001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:1171: 'browser/ui/webui/set_as_default_browser_ui_browsertest.cc', Why are you putting this in interactive ui tests?
Thanks for the review. https://codereview.chromium.org/2090773002/diff/60001/chrome/browser/ui/start... File chrome/browser/ui/startup/default_browser_prompt_win.cc (right): https://codereview.chromium.org/2090773002/diff/60001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_prompt_win.cc:33: registrar_.Add(this, chrome::NOTIFICATION_BROWSER_WINDOW_READY, On 2016/07/06 19:21:18, sky wrote: > Use a BrowserListObserver instead of notification as notifications are > deprecated. Ok, I thought only the notification type with "deprecated" in comments are deprecated but didn't aware of that all notifications are deprecated. https://codereview.chromium.org/2090773002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/set_as_default_browser_ui.h (right): https://codereview.chromium.org/2090773002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/set_as_default_browser_ui.h:26: static views::Widget* GetDialogWidgetForTesting(); On 2016/07/06 19:21:19, sky wrote: > Instead of exposing this can you make Show() return the widget? Alternatively > add a function you can call to see if visible. Yes, I can. However, the challenge is the browser test won't call show() directly. It's trigged by "ForceFirstRun" command line flag during the test. Also, I need the widget not only for visible but also for closing it in the browser test. https://codereview.chromium.org/2090773002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/set_as_default_browser_ui_browsertest.cc (right): https://codereview.chromium.org/2090773002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/set_as_default_browser_ui_browsertest.cc:5: #include "chrome/browser/ui/webui/set_as_default_browser_ui.h" On 2016/07/06 19:21:19, sky wrote: > As this file is win only please name it as such. You mean the name of all three files, not just the browser test file, right? https://codereview.chromium.org/2090773002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/set_as_default_browser_ui_browsertest.cc:17: bool IsMainWindowVisible() { On 2016/07/06 19:21:19, sky wrote: > You shouldn't need a subclass just for this function, create a standalone > function that is passed the browser(). Done. https://codereview.chromium.org/2090773002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/set_as_default_browser_ui_browsertest.cc:41: EXPECT_NE(nullptr, dialog_widget); On 2016/07/06 19:21:19, sky wrote: > ASSERT_TRUE(dialog_widget), otherwise if dialog_widget is null the next line > crashes. Done. https://codereview.chromium.org/2090773002/diff/60001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/2090773002/diff/60001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:1171: 'browser/ui/webui/set_as_default_browser_ui_browsertest.cc', On 2016/07/06 19:21:19, sky wrote: > Why are you putting this in interactive ui tests? Because there is a "interactive ui" operator: Close the pop-up window. I can move it to normal browser test if it's safe to do so.
https://codereview.chromium.org/2090773002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/set_as_default_browser_ui.h (right): https://codereview.chromium.org/2090773002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/set_as_default_browser_ui.h:26: static views::Widget* GetDialogWidgetForTesting(); On 2016/07/06 20:04:31, zmin wrote: > On 2016/07/06 19:21:19, sky wrote: > > Instead of exposing this can you make Show() return the widget? Alternatively > > add a function you can call to see if visible. > > Yes, I can. However, the challenge is the browser test won't call show() > directly. It's trigged by "ForceFirstRun" command line flag during the test. Would an IsVisible() function work on this class? Can you get a handle to the instance? > Also, I need the widget not only for visible but also for closing it in the > browser test. Won't it be torn down by the test? https://codereview.chromium.org/2090773002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/set_as_default_browser_ui_browsertest.cc (right): https://codereview.chromium.org/2090773002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/set_as_default_browser_ui_browsertest.cc:5: #include "chrome/browser/ui/webui/set_as_default_browser_ui.h" On 2016/07/06 20:04:31, zmin wrote: > On 2016/07/06 19:21:19, sky wrote: > > As this file is win only please name it as such. > > You mean the name of all three files, not just the browser test file, right? Good point. I had only thtought this file was win, but you're right. So, yes, all three. https://codereview.chromium.org/2090773002/diff/60001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/2090773002/diff/60001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:1171: 'browser/ui/webui/set_as_default_browser_ui_browsertest.cc', On 2016/07/06 20:04:31, zmin wrote: > On 2016/07/06 19:21:19, sky wrote: > > Why are you putting this in interactive ui tests? > > Because there is a "interactive ui" operator: Close the pop-up window. I can > move it to normal browser test if it's safe to do so. Closing the popup window doesn't necessarily mean you need interactive. You only need interactive if running this test while running another test, say the exact same one, poses problems. For example, if the popup closes on activation loss, then it should be interactive. Does the popup this is creating close on activation loss?
looks like you and sky have this pretty well covered. just a minor comment from me. https://codereview.chromium.org/2090773002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/set_as_default_browser_ui.cc (right): https://codereview.chromium.org/2090773002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/set_as_default_browser_ui.cc:55: views::Widget* g_dialog_widget; if you must use a global variable (i see you're discussing alternatives with sky@), please: - initialize it via " = nullptr;" here - make sure it isn't already set via DCHECK_EQ(nullptr, ...); when initially set - when the widget is closed, DCHECK_EQ(closing_widget, g_dialog_widget); (or DCHECK_NE(nullptr, ...) if it's difficult to get the widget ptr at close time) and set the global back to nullptr also, i'd be inclined to make this a static member of SetAsDefaultBrowserDialogImpl so that it's scoped nicely.
https://codereview.chromium.org/2090773002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/set_as_default_browser_ui.cc (right): https://codereview.chromium.org/2090773002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/set_as_default_browser_ui.cc:55: views::Widget* g_dialog_widget; On 2016/07/13 08:54:49, grt (UTC plus 2) wrote: > if you must use a global variable (i see you're discussing alternatives with > sky@), please: > - initialize it via " = nullptr;" here Done. > - make sure it isn't already set via DCHECK_EQ(nullptr, ...); when initially set Done. > - when the widget is closed, DCHECK_EQ(closing_widget, g_dialog_widget); (or > DCHECK_NE(nullptr, ...) if it's difficult to get the widget ptr at close time) > and set the global back to nullptr Done. > also, i'd be inclined to make this a static member of > SetAsDefaultBrowserDialogImpl so that it's scoped nicely. Done. https://codereview.chromium.org/2090773002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/set_as_default_browser_ui.h (right): https://codereview.chromium.org/2090773002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/set_as_default_browser_ui.h:26: static views::Widget* GetDialogWidgetForTesting(); On 2016/07/06 23:53:16, sky wrote: > On 2016/07/06 20:04:31, zmin wrote: > > On 2016/07/06 19:21:19, sky wrote: > > > Instead of exposing this can you make Show() return the widget? > Alternatively > > > add a function you can call to see if visible. > > > > Yes, I can. However, the challenge is the browser test won't call show() > > directly. It's trigged by "ForceFirstRun" command line flag during the test. > > Would an IsVisible() function work on this class? Can you get a handle to the > instance? There is no instance of SetAsDefaultBrowserUI created during the first run of Chrome. ::Show() as a static function is the only used API. The instance of real WebDialog is created by a second class(SetAsDefaultBrowserDialogImpl) who is declared in the anonymous namespace of .cc file. Also, SetAsDefaultBrowserUI does not even store the pointer of SetAsDefaultBrowserDialogImpl. The second class just deleted itself when the dialog is closed. > > Also, I need the widget not only for visible but also for closing it in the > > browser test. > > Won't it be torn down by the test? It needs to be closed before tear down so I can verify the re-show of main window. https://codereview.chromium.org/2090773002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/set_as_default_browser_ui_browsertest.cc (right): https://codereview.chromium.org/2090773002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/set_as_default_browser_ui_browsertest.cc:5: #include "chrome/browser/ui/webui/set_as_default_browser_ui.h" On 2016/07/06 23:53:16, sky wrote: > On 2016/07/06 20:04:31, zmin wrote: > > On 2016/07/06 19:21:19, sky wrote: > > > As this file is win only please name it as such. > > > > You mean the name of all three files, not just the browser test file, right? > > Good point. I had only thtought this file was win, but you're right. So, yes, > all three. Done. https://codereview.chromium.org/2090773002/diff/60001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/2090773002/diff/60001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:1171: 'browser/ui/webui/set_as_default_browser_ui_browsertest.cc', On 2016/07/06 23:53:16, sky wrote: > On 2016/07/06 20:04:31, zmin wrote: > > On 2016/07/06 19:21:19, sky wrote: > > > Why are you putting this in interactive ui tests? > > > > Because there is a "interactive ui" operator: Close the pop-up window. I can > > move it to normal browser test if it's safe to do so. > > Closing the popup window doesn't necessarily mean you need interactive. You only > need interactive if running this test while running another test, say the exact > same one, poses problems. For example, if the popup closes on activation loss, > then it should be interactive. Does the popup this is creating close on > activation loss? Moving to browser test.
https://codereview.chromium.org/2090773002/diff/80001/chrome/browser/ui/start... File chrome/browser/ui/startup/default_browser_prompt_win.cc (right): https://codereview.chromium.org/2090773002/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_prompt_win.cc:7: #include "base/macros.h" unused https://codereview.chromium.org/2090773002/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_prompt_win.cc:9: #include "chrome/browser/profiles/profile.h" i think you can remove this since this file does no more than pass a Profile* along. https://codereview.chromium.org/2090773002/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_prompt_win.cc:11: #include "chrome/browser/ui/browser.h" unused? https://codereview.chromium.org/2090773002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/set_as_default_browser_ui_browsertest_win.cc (right): https://codereview.chromium.org/2090773002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/set_as_default_browser_ui_browsertest_win.cc:29: void SetUpCommandLine(base::CommandLine* command_line) override { nit: retain the visibility of these methods. BrowserTestBase::SetUpCommandLine is public, while BrowserTestBase::TearDownInProcessBrowserTestFixture is protected. https://codereview.chromium.org/2090773002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/set_as_default_browser_ui_win.cc (right): https://codereview.chromium.org/2090773002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/set_as_default_browser_ui_win.cc:212: remove blank line and second comment so that the comment above clearly applies to both methods. https://codereview.chromium.org/2090773002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/set_as_default_browser_ui_win.cc:316: DCHECK(!dialog_widget_); shouldn't this be DCHECK(dialog_widget_);? https://codereview.chromium.org/2090773002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/set_as_default_browser_ui_win.h (right): https://codereview.chromium.org/2090773002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/set_as_default_browser_ui_win.h:9: #include "ui/views/widget/widget.h" replace this with a forward decl: namespace views { class Widget; } https://codereview.chromium.org/2090773002/diff/80001/chrome/chrome_browser_u... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/2090773002/diff/80001/chrome/chrome_browser_u... chrome/chrome_browser_ui.gypi:2592: 'browser/ui/webui/set_as_default_browser_ui_win.cc', while this is win-only, i'm inclined to put it in a more general variable and let the _win filtering take care of it. sky: what's your opinion here? https://codereview.chromium.org/2090773002/diff/80001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/2090773002/diff/80001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:2509: 'browser/ui/webui/set_as_default_browser_ui_browsertest_win.cc', move this to chrome_browser_tests_sources. it will automatically be filtered out for other platforms by virtue of the _win stuffix. https://codereview.chromium.org/2090773002/diff/80001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2090773002/diff/80001/chrome/test/BUILD.gn#ne... chrome/test/BUILD.gn:1229: sources += i think you can remove this if you move the file into chrome_browser_tests_sources.
https://codereview.chromium.org/2090773002/diff/80001/chrome/browser/ui/start... File chrome/browser/ui/startup/default_browser_prompt_win.cc (right): https://codereview.chromium.org/2090773002/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_prompt_win.cc:7: #include "base/macros.h" On 2016/07/19 08:13:01, grt (UTC plus 2) wrote: > unused Done. https://codereview.chromium.org/2090773002/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_prompt_win.cc:9: #include "chrome/browser/profiles/profile.h" On 2016/07/19 08:13:01, grt (UTC plus 2) wrote: > i think you can remove this since this file does no more than pass a Profile* > along. Done. https://codereview.chromium.org/2090773002/diff/80001/chrome/browser/ui/start... chrome/browser/ui/startup/default_browser_prompt_win.cc:11: #include "chrome/browser/ui/browser.h" On 2016/07/19 08:13:01, grt (UTC plus 2) wrote: > unused? Done. https://codereview.chromium.org/2090773002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/set_as_default_browser_ui_browsertest_win.cc (right): https://codereview.chromium.org/2090773002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/set_as_default_browser_ui_browsertest_win.cc:29: void SetUpCommandLine(base::CommandLine* command_line) override { On 2016/07/19 08:13:01, grt (UTC plus 2) wrote: > nit: retain the visibility of these methods. BrowserTestBase::SetUpCommandLine > is public, while BrowserTestBase::TearDownInProcessBrowserTestFixture is > protected. Done. https://codereview.chromium.org/2090773002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/set_as_default_browser_ui_win.cc (right): https://codereview.chromium.org/2090773002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/set_as_default_browser_ui_win.cc:212: On 2016/07/19 08:13:01, grt (UTC plus 2) wrote: > remove blank line and second comment so that the comment above clearly applies > to both methods. Done. https://codereview.chromium.org/2090773002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/set_as_default_browser_ui_win.cc:316: DCHECK(!dialog_widget_); On 2016/07/19 08:13:01, grt (UTC plus 2) wrote: > shouldn't this be DCHECK(dialog_widget_);? Done. https://codereview.chromium.org/2090773002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/set_as_default_browser_ui_win.h (right): https://codereview.chromium.org/2090773002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/set_as_default_browser_ui_win.h:9: #include "ui/views/widget/widget.h" On 2016/07/19 08:13:01, grt (UTC plus 2) wrote: > replace this with a forward decl: > > namespace views { > class Widget; > } Done. https://codereview.chromium.org/2090773002/diff/80001/chrome/chrome_browser_u... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/2090773002/diff/80001/chrome/chrome_browser_u... chrome/chrome_browser_ui.gypi:2592: 'browser/ui/webui/set_as_default_browser_ui_win.cc', On 2016/07/19 08:13:01, grt (UTC plus 2) wrote: > while this is win-only, i'm inclined to put it in a more general variable and > let the _win filtering take care of it. sky: what's your opinion here? Done. I didn't know there is a _win filter. I think we shall use that filter if there is _win suffix. https://codereview.chromium.org/2090773002/diff/80001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/2090773002/diff/80001/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:2509: 'browser/ui/webui/set_as_default_browser_ui_browsertest_win.cc', On 2016/07/19 08:13:01, grt (UTC plus 2) wrote: > move this to chrome_browser_tests_sources. it will automatically be filtered out > for other platforms by virtue of the _win stuffix. Done. https://codereview.chromium.org/2090773002/diff/80001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2090773002/diff/80001/chrome/test/BUILD.gn#ne... chrome/test/BUILD.gn:1229: sources += On 2016/07/19 08:13:01, grt (UTC plus 2) wrote: > i think you can remove this if you move the file into > chrome_browser_tests_sources. Done.
https://codereview.chromium.org/2090773002/diff/80001/chrome/chrome_browser_u... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/2090773002/diff/80001/chrome/chrome_browser_u... chrome/chrome_browser_ui.gypi:2592: 'browser/ui/webui/set_as_default_browser_ui_win.cc', On 2016/07/20 16:35:45, zmin wrote: > On 2016/07/19 08:13:01, grt (UTC plus 2) wrote: > > while this is win-only, i'm inclined to put it in a more general variable and > > let the _win filtering take care of it. sky: what's your opinion here? > > Done. > I didn't know there is a _win filter. I think we shall use that filter if there > is _win suffix. Magic! (https://cs.chromium.org/chromium/src/build/config/BUILDCONFIG.gn?dr=C&q=%5C*_...)
Hi Scott, Can you look at this CL again please? Owen
https://codereview.chromium.org/2090773002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/set_as_default_browser_ui_win.cc (right): https://codereview.chromium.org/2090773002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/set_as_default_browser_ui_win.cc:228: views::Widget* SetAsDefaultBrowserDialogImpl::dialog_widget_ = nullptr; nit: prefix with // static (see style guide) https://codereview.chromium.org/2090773002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/set_as_default_browser_ui_win.cc:341: if (browser_ || !browser || !browser->is_type_tabbed()) Should this early out if dialog_widget_?
https://codereview.chromium.org/2090773002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/set_as_default_browser_ui_win.cc (right): https://codereview.chromium.org/2090773002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/set_as_default_browser_ui_win.cc:228: views::Widget* SetAsDefaultBrowserDialogImpl::dialog_widget_ = nullptr; On 2016/07/25 20:10:03, sky wrote: > nit: prefix with // static (see style guide) Done. https://codereview.chromium.org/2090773002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/set_as_default_browser_ui_win.cc:341: if (browser_ || !browser || !browser->is_type_tabbed()) On 2016/07/25 20:10:03, sky wrote: > Should this early out if dialog_widget_? dialog_widget_ is only used for testing so I think DCHECK is good enough. And I think it's good to put the DCHECK in ShowDialog() in case someone else want to call that function in other place in the future because dialog_widget_ is assigned in that function.
https://codereview.chromium.org/2090773002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/set_as_default_browser_ui_win.cc (right): https://codereview.chromium.org/2090773002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/set_as_default_browser_ui_win.cc:341: if (browser_ || !browser || !browser->is_type_tabbed()) On 2016/07/25 20:23:11, zmin wrote: > On 2016/07/25 20:10:03, sky wrote: > > Should this early out if dialog_widget_? > > dialog_widget_ is only used for testing so I think DCHECK is good enough. What prevents another browser from being created while the dialog is showing? > And I think it's good to put the DCHECK in ShowDialog() in case someone else > want to call that function in other place in the future because dialog_widget_ > is assigned in that function.
On 2016/07/25 20:24:12, sky wrote: > https://codereview.chromium.org/2090773002/diff/100001/chrome/browser/ui/webu... > File chrome/browser/ui/webui/set_as_default_browser_ui_win.cc (right): > > https://codereview.chromium.org/2090773002/diff/100001/chrome/browser/ui/webu... > chrome/browser/ui/webui/set_as_default_browser_ui_win.cc:341: if (browser_ || > !browser || !browser->is_type_tabbed()) > On 2016/07/25 20:23:11, zmin wrote: > > On 2016/07/25 20:10:03, sky wrote: > > > Should this early out if dialog_widget_? > > > > dialog_widget_ is only used for testing so I think DCHECK is good enough. > > What prevents another browser from being created while the dialog is showing? No, there is not. The second browser window will be shown directly as normal if user launch Chrome again while the dialog is opened. This is the behavior we have right now and I personally think it's fine. However, we can discuss it in a separate issue and loop in motek@ who introduced this dialog at the very beginning. The dialog itself won't popup again because of the singleton. > > And I think it's good to put the DCHECK in ShowDialog() in case someone else > > want to call that function in other place in the future because dialog_widget_ > > is assigned in that function.
Got it, LGTM
lgtm
The CQ bit was checked by zmin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Improves first run dialog performance on Win8 BUG=616829 ========== to ========== Improves first run dialog performance on Win8 BUG=616829 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Improves first run dialog performance on Win8 BUG=616829 ========== to ========== Improves first run dialog performance on Win8 BUG=616829 Committed: https://crrev.com/065fd8ffaebd901b6033aaa5d1d7693c3ff26d87 Cr-Commit-Position: refs/heads/master@{#407810} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/065fd8ffaebd901b6033aaa5d1d7693c3ff26d87 Cr-Commit-Position: refs/heads/master@{#407810} |
