Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2716353003:
[Extensions UI] Initially disabled OK button for extension install prompts. (Closed)
Description[Extensions UI] Initially disabled OK button for extension install prompts and enable them after a 500 ms time period.
BUG=394518
Review-Url: https://codereview.chromium.org/2716353003
Cr-Commit-Position: refs/heads/master@{#461933}
Committed: https://chromium.googlesource.com/chromium/src/+/303d78445257d1eec726c4ebadb3517cb16c8c09
Patch Set 1 #Patch Set 2 : Add functions to update and reenable the install button. #Patch Set 3 : Adding browser test for delaying the enable state of the add extension button. #Patch Set 4 : Add unit tests for cocoa and making function names consistent. #Patch Set 5 : Modify the install delay to be configurable for testing purposes. #Patch Set 6 : Add configurable install delay for cocoa unit testing. #Patch Set 7 : Fixing grammar in comments. #
Total comments: 18
Patch Set 8 : Removing cocoa changes, which will be done using a separate change. #
Total comments: 12
Patch Set 9 : Addressing Devlin's comments. #
Messages
Total messages: 28 (14 generated)
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Description was changed from ========== In progress: Disabled OK button for inline install prompts and attempting to re-enabled after a time period. BUG=394518 ========== to ========== [Extensions UI] Initially disabled OK button for extension install prompts and enable them after a 500 ms time period. BUG=394518 ==========
ackermanb@chromium.org changed reviewers: + avi@chromium.org, meacer@chromium.org, rdevlin.cronin@chromium.org
Nice! Overall, this looks pretty good. One other comment - when adding multiple reviewers to a CL, it's good to describe what you want them to look at, e.g. devlin/mustafa: everything avi: cocoa https://codereview.chromium.org/2716353003/diff/180001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/extensions/extension_install_view_controller.h (right): https://codereview.chromium.org/2716353003/diff/180001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.h:87: + (void)setInstallDelayForTesting:(NSTimeInterval)time_delay_in_s; nit: Cocoa uses camelCaseSyntax https://codereview.chromium.org/2716353003/diff/180001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.h:87: + (void)setInstallDelayForTesting:(NSTimeInterval)time_delay_in_s; I've also seen us use a separate interface for testing, e.g. @interface ExtensionInstallViewController(TestingAPI) Avi will know which is best practice (if either is preferable). https://codereview.chromium.org/2716353003/diff/180001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm (right): https://codereview.chromium.org/2716353003/diff/180001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:394: [self startTimerToEnableInstall]; I'm a little worried that we start this at construction, rather than at appearance. During a particularly laggy flow, that might mean that the button is enabled before the dialog is shown. We should probably start the timer in response to the view being displayed. https://codereview.chromium.org/2716353003/diff/180001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:788: [NSTimer scheduledTimerWithTimeInterval:g_install_delay_in_s nit: maybe just inline this method https://codereview.chromium.org/2716353003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): https://codereview.chromium.org/2716353003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:179: StartTimerToEnableInstall(); Here, too, just because the dialog is constructed doesn't mean it's visible. I think the canonical way to observe view visibility changes in Views is with views::WidgetObserver::OnWidgetVisibilityChanged. So we probably want to observe this dialog's widget (accessible with GetWidget()), and wait for that. https://codereview.chromium.org/2716353003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:584: timer_.Start(FROM_HERE, Here too, let's just inline this at the call site. https://codereview.chromium.org/2716353003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2716353003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:175: base::OneShotTimer timer; needed? https://codereview.chromium.org/2716353003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:217: ASSERT_FALSE(delegate_view->IsDialogButtonEnabled(ui::DIALOG_BUTTON_OK)); nit: we can make these EXPECTs instead of ASSERTs
https://codereview.chromium.org/2716353003/diff/180001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/extensions/extension_install_view_controller.h (right): https://codereview.chromium.org/2716353003/diff/180001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.h:87: + (void)setInstallDelayForTesting:(NSTimeInterval)time_delay_in_s; On 2017/03/23 01:25:29, Devlin wrote: > I've also seen us use a separate interface for testing, e.g. > @interface ExtensionInstallViewController(TestingAPI) > > Avi will know which is best practice (if either is preferable). I've seen the (TestingAPI) too, but naming it with ForTesting means that the presubmit will catch misuse. Or do both! BTW don't do "time_delay_in_s". It's not "in s", it's an NSTimeInterval. https://codereview.chromium.org/2716353003/diff/180001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm (right): https://codereview.chromium.org/2716353003/diff/180001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:101: NSTimeInterval g_install_delay_in_s = 0.5; camelcase. No "in s".
Patchset #8 (id:200001) has been deleted
Patchset #8 (id:220001) has been deleted
ackermanb@chromium.org changed reviewers: - avi@chromium.org
I removed the cocoa changes, since it's going to take a little work to get it to work properly. I will send out a separate change for cocoa. Devlin/Mustafa, if you want to take another look. https://codereview.chromium.org/2716353003/diff/180001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/extensions/extension_install_view_controller.h (right): https://codereview.chromium.org/2716353003/diff/180001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.h:87: + (void)setInstallDelayForTesting:(NSTimeInterval)time_delay_in_s; On 2017/03/23 02:32:32, Avi wrote: > On 2017/03/23 01:25:29, Devlin wrote: > > I've also seen us use a separate interface for testing, e.g. > > @interface ExtensionInstallViewController(TestingAPI) > > > > Avi will know which is best practice (if either is preferable). > > I've seen the (TestingAPI) too, but naming it with ForTesting means that the > presubmit will catch misuse. Or do both! > > BTW don't do "time_delay_in_s". It's not "in s", it's an NSTimeInterval. Noted. I will used both. https://codereview.chromium.org/2716353003/diff/180001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm (right): https://codereview.chromium.org/2716353003/diff/180001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:101: NSTimeInterval g_install_delay_in_s = 0.5; On 2017/03/23 02:32:33, Avi wrote: > camelcase. No "in s". Noted. https://codereview.chromium.org/2716353003/diff/180001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:394: [self startTimerToEnableInstall]; On 2017/03/23 01:25:29, Devlin wrote: > I'm a little worried that we start this at construction, rather than at > appearance. During a particularly laggy flow, that might mean that the button > is enabled before the dialog is shown. We should probably start the timer in > response to the view being displayed. Noted. This will require a little work, so I'm going to separate out the views and cocoa changes from each other. https://codereview.chromium.org/2716353003/diff/180001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:788: [NSTimer scheduledTimerWithTimeInterval:g_install_delay_in_s On 2017/03/23 01:25:29, Devlin wrote: > nit: maybe just inline this method Noted. https://codereview.chromium.org/2716353003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): https://codereview.chromium.org/2716353003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:179: StartTimerToEnableInstall(); On 2017/03/23 01:25:30, Devlin wrote: > Here, too, just because the dialog is constructed doesn't mean it's visible. I > think the canonical way to observe view visibility changes in Views is with > views::WidgetObserver::OnWidgetVisibilityChanged. So we probably want to > observe this dialog's widget (accessible with GetWidget()), and wait for that. Done. https://codereview.chromium.org/2716353003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:584: timer_.Start(FROM_HERE, On 2017/03/23 01:25:30, Devlin wrote: > Here too, let's just inline this at the call site. Done. https://codereview.chromium.org/2716353003/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2716353003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:175: base::OneShotTimer timer; On 2017/03/23 01:25:30, Devlin wrote: > needed? Nope, removed. https://codereview.chromium.org/2716353003/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:217: ASSERT_FALSE(delegate_view->IsDialogButtonEnabled(ui::DIALOG_BUTTON_OK)); On 2017/03/23 01:25:30, Devlin wrote: > nit: we can make these EXPECTs instead of ASSERTs Done.
Nice! https://codereview.chromium.org/2716353003/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): https://codereview.chromium.org/2716353003/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:615: if (is_visible) { We should make this: if (is_visible && !install_enabled_) so that subsequent changes to visibility don't trigger the timer unnecessarily. https://codereview.chromium.org/2716353003/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:619: base::Unretained(this))); Each time I see this unretained, it makes me nervous, and then I realize why it's safe. Can we add a quick comment above Start() that says: // This base::Unretained is safe because the task is owned by the timer, which is in turn owned by this object. https://codereview.chromium.org/2716353003/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_install_dialog_view.h (right): https://codereview.chromium.org/2716353003/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_install_dialog_view.h:78: // Enables installs and updates the dialog buttons. nitty nit: Maybe "Enables the ['install'|'accept'] button."? "Enables installs" sounds kinda weird to me for some reason... but maybe just me. https://codereview.chromium.org/2716353003/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_install_dialog_view.h:79: void EnableInstall(); similarly, maybe Enable[Install|Accept]Button. https://codereview.chromium.org/2716353003/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_install_dialog_view.h:126: bool install_enabled_ = false; We should be consistent in our initialization of POD in classes. So either move this initialization to the ctor definition, or add initializers for the rest of the POD in this class. (Either is fine; the former is less work.) https://codereview.chromium.org/2716353003/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2716353003/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:212: views::DialogDelegateView* delegate_view = CreateAndShowPrompt(&helper); Can we check that the dialog was, in fact, visible at this time?
PTAL. https://codereview.chromium.org/2716353003/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_install_dialog_view.cc (right): https://codereview.chromium.org/2716353003/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:615: if (is_visible) { On 2017/03/31 22:21:41, Devlin wrote: > We should make this: > if (is_visible && !install_enabled_) > > so that subsequent changes to visibility don't trigger the timer unnecessarily. Done. https://codereview.chromium.org/2716353003/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_install_dialog_view.cc:619: base::Unretained(this))); On 2017/03/31 22:21:41, Devlin wrote: > Each time I see this unretained, it makes me nervous, and then I realize why > it's safe. Can we add a quick comment above Start() that says: > // This base::Unretained is safe because the task is owned by the timer, which > is in turn owned by this object. Done. https://codereview.chromium.org/2716353003/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_install_dialog_view.h (right): https://codereview.chromium.org/2716353003/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_install_dialog_view.h:78: // Enables installs and updates the dialog buttons. On 2017/03/31 22:21:41, Devlin wrote: > nitty nit: Maybe "Enables the ['install'|'accept'] button."? "Enables installs" > sounds kinda weird to me for some reason... but maybe just me. Yeah, it's a little awkward. I will change it to enable install button. I also renamed a couple of other things to make it more clear as well. https://codereview.chromium.org/2716353003/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_install_dialog_view.h:79: void EnableInstall(); On 2017/03/31 22:21:41, Devlin wrote: > similarly, maybe Enable[Install|Accept]Button. Done. https://codereview.chromium.org/2716353003/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_install_dialog_view.h:126: bool install_enabled_ = false; On 2017/03/31 22:21:41, Devlin wrote: > We should be consistent in our initialization of POD in classes. So either move > this initialization to the ctor definition, or add initializers for the rest of > the POD in this class. (Either is fine; the former is less work.) Done. https://codereview.chromium.org/2716353003/diff/240001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc (right): https://codereview.chromium.org/2716353003/diff/240001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc:212: views::DialogDelegateView* delegate_view = CreateAndShowPrompt(&helper); On 2017/03/31 22:21:41, Devlin wrote: > Can we check that the dialog was, in fact, visible at this time? Done.
lgtm; thanks for doing this!
Sorry for the delay, non-owner lgtm.
The CQ bit was checked by ackermanb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
ackermanb@chromium.org changed reviewers: + benwells@chromium.org
Ben, as an owner do you mind reviewing?
On 2017/04/04 17:33:30, Ackerman wrote: > Ben, as an owner do you mind reviewing? stamp lgtm (as Devlin has looked already)
The CQ bit was checked by ackermanb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1491353095967160,
"parent_rev": "0c187ee3c2b8fc9d0dd81758f892cf6fd0ba4c3f", "commit_rev":
"303d78445257d1eec726c4ebadb3517cb16c8c09"}
Message was sent while issue was closed.
Description was changed from ========== [Extensions UI] Initially disabled OK button for extension install prompts and enable them after a 500 ms time period. BUG=394518 ========== to ========== [Extensions UI] Initially disabled OK button for extension install prompts and enable them after a 500 ms time period. BUG=394518 Review-Url: https://codereview.chromium.org/2716353003 Cr-Commit-Position: refs/heads/master@{#461933} Committed: https://chromium.googlesource.com/chromium/src/+/303d78445257d1eec726c4ebadb3... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:260001) as https://chromium.googlesource.com/chromium/src/+/303d78445257d1eec726c4ebadb3... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
