Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2229943003:
Reusing Ok/Cancel buttons for intent picker (Closed)
DescriptionReusing Ok/Cancel buttons for intent picker
Modifying IntentPickerBubbleView to reuse the title Ok/Cancel buttons in
the BubbleDialogDelegateView class.
Creating a custom LabelButton that adds hovering effeects. Moved some of
the customizations to the definition of this new custom class, also
renamed a couple of accessory methods that directly handle these items.
Modified the unit tests accordingly.
Also in this CL, changed the default value of |selected_app_tag_|
so by default the app at index 0 on the list is highlighted, for this
same reason we don't need to disable the DialogButtons anymore.
BUG=620129, 638063
TEST=Manual test, also IntentPickerBubbleViewTest unit tests passing.
Committed: https://crrev.com/a1fd64d48d76266f9c6db6378346cdec2f416c27
Cr-Commit-Position: refs/heads/master@{#426094}
Patch Set 1 #Patch Set 2 : Removing unused variables #Patch Set 3 : Overwritting the Close() method #Patch Set 4 : Adding hovering effects for the app candidates, also removing non-used paddings. #Patch Set 5 : Disabling buttons until the user make a selection #Patch Set 6 : Changing LabelButton -> views::LabelButton #Patch Set 7 : Casting with static_cast, its ok to convert to labelButton as we only need to check for the contain… #Patch Set 8 : Fixing hovering color, modifying the test so it only access the minimum necessary info #
Total comments: 35
Patch Set 9 : Deleting custom inner-class and moving to MdTextButton::CreateSecondaryUiButton #Patch Set 10 : Reverting is_null and Reset usage for the callback. #Patch Set 11 : Fixing the intent picker class to use is_null() and Reset() properly. #
Total comments: 4
Patch Set 12 : Inner button class for hovering/selected/unselected effects #
Total comments: 10
Patch Set 13 : Minor fixes and unit test #Patch Set 14 : Fixing test cases #Patch Set 15 : For testing we don't create a widget to contain the class, so DialogButtons are not in place, this … #
Total comments: 5
Patch Set 16 : Adding PressButtonTwice case to unit test #Patch Set 17 : Adding TODO for further ScrollView/Layer investigation. #Patch Set 18 : Changing strings to lowercase. #
Total comments: 16
Patch Set 19 : Changing inner class name, plus changing default button for the dialog delegate #Patch Set 20 : Removing extra line #
Total comments: 8
Patch Set 21 : Sending package_name info to the UI, this will be used to uniquely identify the user's selection. #Patch Set 22 : Rebasing #Patch Set 23 : Adding a default app selection at index 0 #
Total comments: 32
Patch Set 24 : Minor fixes: introducing RunCallback and moved IntentPickerResponse #Patch Set 25 : Activating a layer for the ScrollView's Viewport #Patch Set 26 : Removing callback usage on WebContentsDestroyed #
Total comments: 2
Patch Set 27 : Temporary fix to force Layout enabling within contents_viewport_ #
Total comments: 5
Patch Set 28 : Reusing EnableViewPortLayer(), also pointing insets customization to an open crbug #Patch Set 29 : Getting rid of GetDefaultDialogButton() #
Total comments: 1
Messages
Total messages: 170 (127 generated)
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Reusing Ok/Cancel buttons for intent picker Modifying IntentPickerBubbleView to reuse the title Ok/Cancel buttons in the BubbleDialogDelegateView class. BUG=620129 TEST=Manual test, also IntentPickerBubbleViewTest unit tests passing. ========== to ========== Reusing Ok/Cancel buttons for intent picker Modifying IntentPickerBubbleView to reuse the title Ok/Cancel buttons in the BubbleDialogDelegateView class. BUG=620129 TEST=Manual test, also IntentPickerBubbleViewTest unit tests passing. ==========
djacobo@google.com changed reviewers: + yusukes@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Reusing Ok/Cancel buttons for intent picker Modifying IntentPickerBubbleView to reuse the title Ok/Cancel buttons in the BubbleDialogDelegateView class. BUG=620129 TEST=Manual test, also IntentPickerBubbleViewTest unit tests passing. ========== to ========== Reusing Ok/Cancel buttons for intent picker Modifying IntentPickerBubbleView to reuse the title Ok/Cancel buttons in the BubbleDialogDelegateView class. BUG=620129,638063 TEST=Manual test, also IntentPickerBubbleViewTest unit tests passing. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.h (right): https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. Can you also mention that this CL adds the hovering effect?
Description was changed from
==========
Reusing Ok/Cancel buttons for intent picker
Modifying IntentPickerBubbleView to reuse the title Ok/Cancel buttons in
the BubbleDialogDelegateView class.
BUG=620129,638063
TEST=Manual test, also IntentPickerBubbleViewTest unit tests passing.
==========
to
==========
Reusing Ok/Cancel buttons for intent picker
Modifying IntentPickerBubbleView to reuse the title Ok/Cancel buttons in
the BubbleDialogDelegateView class.
This is also for adding hovering effects for the same UI.
Creating a custom LabelButton that adds hovering effeects. Moved some of
the customizations to the definition of this new custom class, also
renamed a couple of accessory methods that directly handle these items.
Modified the unit tests accordingly.
BUG=620129,638063
TEST=Manual test, also IntentPickerBubbleViewTest unit tests passing.
==========
Description was changed from
==========
Reusing Ok/Cancel buttons for intent picker
Modifying IntentPickerBubbleView to reuse the title Ok/Cancel buttons in
the BubbleDialogDelegateView class.
This is also for adding hovering effects for the same UI.
Creating a custom LabelButton that adds hovering effeects. Moved some of
the customizations to the definition of this new custom class, also
renamed a couple of accessory methods that directly handle these items.
Modified the unit tests accordingly.
BUG=620129,638063
TEST=Manual test, also IntentPickerBubbleViewTest unit tests passing.
==========
to
==========
Reusing Ok/Cancel buttons for intent picker
Modifying IntentPickerBubbleView to reuse the title Ok/Cancel buttons in
the BubbleDialogDelegateView class.
This is also for adding hovering effects for the same UI.
Creating a custom LabelButton that adds hovering effeects. Moved some of
the customizations to the definition of this new custom class, also
renamed a couple of accessory methods that directly handle these items.
Modified the unit tests accordingly.
BUG=620129,638063
TEST=Manual test, also IntentPickerBubbleViewTest unit tests passing.
==========
Description was changed from
==========
Reusing Ok/Cancel buttons for intent picker
Modifying IntentPickerBubbleView to reuse the title Ok/Cancel buttons in
the BubbleDialogDelegateView class.
This is also for adding hovering effects for the same UI.
Creating a custom LabelButton that adds hovering effeects. Moved some of
the customizations to the definition of this new custom class, also
renamed a couple of accessory methods that directly handle these items.
Modified the unit tests accordingly.
BUG=620129,638063
TEST=Manual test, also IntentPickerBubbleViewTest unit tests passing.
==========
to
==========
Reusing Ok/Cancel buttons for intent picker
Modifying IntentPickerBubbleView to reuse the title Ok/Cancel buttons in
the BubbleDialogDelegateView class.
Creating a custom LabelButton that adds hovering effeects. Moved some of
the customizations to the definition of this new custom class, also
renamed a couple of accessory methods that directly handle these items.
Modified the unit tests accordingly.
BUG=620129,638063
TEST=Manual test, also IntentPickerBubbleViewTest unit tests passing.
==========
On 2016/08/17 23:18:22, Yusuke Sato (ooo Aug 8) wrote: > lgtm > > https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... > File chrome/browser/ui/views/intent_picker_bubble_view.h (right): > > https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... > chrome/browser/ui/views/intent_picker_bubble_view.h:1: // Copyright 2016 The > Chromium Authors. All rights reserved. > Can you also mention that this CL adds the hovering effect? Totally, thanks!
djacobo@google.com changed reviewers: + estade@chromium.org, sky@chromium.org
https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:50: class IntentPickerMenuButton : public views::LabelButton { I don't believe this class is necessary. You could just create a LabelButton and set the properties you want. For the ink drop color, set_ink_drop_base_color should work. https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:58: if (ui::MaterialDesignController::IsModeMaterial()) { this isn't a useful check because it always returns true. You could replace it with IsSecondaryUiMaterial() but I take it you want the MD look before it's launched to other dialogs. https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:78: throttle_cb.Run(kAppTagNoneSelected, seems like these two blocks could/should be combined. https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:108: widget->client_view()->AsDialogClientView()->ok_button()->SetState( use IsDialogButtonEnabled() https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:128: // Internally this button uses the |JUST ONCE| logic. I don't understand this comment and I don't think it's necessary. Ditto below. https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:130: was_callback_run_ = true; instead of adding was_callback_run_, you can use Reset() and is_null() https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:175: IntentPickerMenuButton* tmp_label = new IntentPickerMenuButton( I don't think tmp_ is a good prefix for local variable names https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:250: return ui::DIALOG_BUTTON_OK | ui::DIALOG_BUTTON_CANCEL; this is the default https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:262: SetIntentPickerMenuButtonBackgroundColor(selected_app_tag_, seems like you should be setting the ink drop state to ACTIVATED instead of setting a bg color (like when you click the app menu button and the menu is showing) https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:264: GetDialogClientView()->ok_button()->SetState(views::Button::STATE_NORMAL); GetDialogClientView()->UpdateDialogButtons(); https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:303: IntentPickerMenuButton* temp_lb = GetIntentPickerMenuButtonAt(index); s/temp_lb/button
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:50: class IntentPickerMenuButton : public views::LabelButton { On 2016/08/18 05:45:41, Evan Stade wrote: > I don't believe this class is necessary. You could just create a LabelButton and > set the properties you want. For the ink drop color, set_ink_drop_base_color > should work. Hmm, that is true for all but SetInkDropMode(), it is a protected method and I have seen it called only on classes that directly or indirectly inherit from views::CustomButton. is there a public accessory method for this? I am guessing something like LabelButton* button = ...; button->set_ink_drop_mode(); Or does this means we will not have hovering effects in the meantime? https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:58: if (ui::MaterialDesignController::IsModeMaterial()) { On 2016/08/18 05:45:41, Evan Stade (ooo wed-thurs) wrote: > this isn't a useful check because it always returns true. You could replace it > with IsSecondaryUiMaterial() but I take it you want the MD look before it's > launched to other dialogs. I am removing this whole inner-class, done. https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:78: throttle_cb.Run(kAppTagNoneSelected, On 2016/08/18 05:45:41, Evan Stade wrote: > seems like these two blocks could/should be combined. Done. https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:108: widget->client_view()->AsDialogClientView()->ok_button()->SetState( On 2016/08/18 05:45:41, Evan Stade (ooo wed-thurs) wrote: > use IsDialogButtonEnabled() Awesome, done. https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:128: // Internally this button uses the |JUST ONCE| logic. On 2016/08/18 05:45:41, Evan Stade wrote: > I don't understand this comment and I don't think it's necessary. Ditto below. Done. https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:130: was_callback_run_ = true; On 2016/08/18 05:45:41, Evan Stade wrote: > instead of adding was_callback_run_, you can use Reset() and is_null() I did use something like: if (!throttle_cb_.is_null()) { throttle_cb_.Run(params); throttle_cb_.Reset(); } and got a crash, poked with the code for a while and this way Accept() and Cancel() were trying to execute throttle_cb_.Run() (yes, did the same change to both methods). So ATM I would like to stick with this implementation if you don't mind, I could investigate further next if you want. https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:175: IntentPickerMenuButton* tmp_label = new IntentPickerMenuButton( On 2016/08/18 05:45:41, Evan Stade wrote: > I don't think tmp_ is a good prefix for local variable names Done. https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:250: return ui::DIALOG_BUTTON_OK | ui::DIALOG_BUTTON_CANCEL; On 2016/08/18 05:45:41, Evan Stade wrote: > this is the default Done. https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:262: SetIntentPickerMenuButtonBackgroundColor(selected_app_tag_, On 2016/08/18 05:45:41, Evan Stade (ooo wed-thurs) wrote: > seems like you should be setting the ink drop state to ACTIVATED instead of > setting a bg color (like when you click the app menu button and the menu is > showing) If we wanted to use InkDropState, shouldn't we have our own button class and call AnimateInkDrop() ? I will make this change if you think is necessary, I just want to know if that's the correct way of implementing it. https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:264: GetDialogClientView()->ok_button()->SetState(views::Button::STATE_NORMAL); On 2016/08/18 05:45:41, Evan Stade (ooo wed-thurs) wrote: > GetDialogClientView()->UpdateDialogButtons(); Done. https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:303: IntentPickerMenuButton* temp_lb = GetIntentPickerMenuButtonAt(index); On 2016/08/18 05:45:41, Evan Stade (ooo wed-thurs) wrote: > s/temp_lb/button Done.
The CQ bit was checked by djacobo@google.com to run a CQ dry run
https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:130: was_callback_run_ = true; On 2016/08/24 22:18:35, djacobo wrote: > On 2016/08/18 05:45:41, Evan Stade wrote: > > instead of adding was_callback_run_, you can use Reset() and is_null() > > I did use something like: > if (!throttle_cb_.is_null()) { > throttle_cb_.Run(params); > throttle_cb_.Reset(); > } > > and got a crash, poked with the code for a while and this way Accept() and > Cancel() were trying to execute throttle_cb_.Run() (yes, did the same change to > both methods). So ATM I would like to stick with this implementation if you > don't mind, I could investigate further next if you want. Arg, I didn't undo the changes, my bad reverting this part.
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
estade@chromium.org changed reviewers: + bruthig@chromium.org
https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:50: class IntentPickerMenuButton : public views::LabelButton { On 2016/08/24 22:18:35, djacobo wrote: > On 2016/08/18 05:45:41, Evan Stade wrote: > > I don't believe this class is necessary. You could just create a LabelButton > and > > set the properties you want. For the ink drop color, set_ink_drop_base_color > > should work. > > Hmm, that is true for all but SetInkDropMode(), it is a protected method and I > have seen it called only on classes that directly or indirectly inherit from > views::CustomButton. is there a public accessory method for this? I am guessing > something like > > LabelButton* button = ...; > button->set_ink_drop_mode(); > > Or does this means we will not have hovering effects in the meantime? I don't know if there's a good reason for SetInkDropMode to be protected. +bruthig https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:130: was_callback_run_ = true; On 2016/08/24 22:18:35, djacobo wrote: > On 2016/08/18 05:45:41, Evan Stade wrote: > > instead of adding was_callback_run_, you can use Reset() and is_null() > > I did use something like: > if (!throttle_cb_.is_null()) { > throttle_cb_.Run(params); > throttle_cb_.Reset(); > } > > and got a crash, poked with the code for a while and this way Accept() and > Cancel() were trying to execute throttle_cb_.Run() (yes, did the same change to > both methods). So ATM I would like to stick with this implementation if you > don't mind, I could investigate further next if you want. Yes, please figure out why it's crashing. If it's because running the callback somehow tries to close this dialog, that's probably something work fixing.
The CQ bit was checked by djacobo@google.com to run a CQ dry run
https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:130: was_callback_run_ = true; On 2016/08/26 22:44:23, Evan Stade wrote: > On 2016/08/24 22:18:35, djacobo wrote: > > On 2016/08/18 05:45:41, Evan Stade wrote: > > > instead of adding was_callback_run_, you can use Reset() and is_null() > > > > I did use something like: > > if (!throttle_cb_.is_null()) { > > throttle_cb_.Run(params); > > throttle_cb_.Reset(); > > } > > > > and got a crash, poked with the code for a while and this way Accept() and > > Cancel() were trying to execute throttle_cb_.Run() (yes, did the same change > to > > both methods). So ATM I would like to stick with this implementation if you > > don't mind, I could investigate further next if you want. > > Yes, please figure out why it's crashing. If it's because running the callback > somehow tries to close this dialog, that's probably something work fixing. Somehow Reset()-ing the callback after actually Run()-ing it let the ArcNavigationThrottle in a bad state, the issue was observed on another UI which can be displayed before this one under certain scenarios (it's an incognito warning). Using the same approach here and on the other class. (adding you as a reviewer so you can see what's going on).
Dry run: 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
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:50: class IntentPickerMenuButton : public views::LabelButton { On 2016/08/26 22:44:23, Evan Stade wrote: > On 2016/08/24 22:18:35, djacobo wrote: > > On 2016/08/18 05:45:41, Evan Stade wrote: > > > I don't believe this class is necessary. You could just create a LabelButton > > and > > > set the properties you want. For the ink drop color, set_ink_drop_base_color > > > should work. > > > > Hmm, that is true for all but SetInkDropMode(), it is a protected method and I > > have seen it called only on classes that directly or indirectly inherit from > > views::CustomButton. is there a public accessory method for this? I am > guessing > > something like > > > > LabelButton* button = ...; > > button->set_ink_drop_mode(); > > > > Or does this means we will not have hovering effects in the meantime? > > I don't know if there's a good reason for SetInkDropMode to be protected. > +bruthig It should be ok to promote InkDropHostView::SetInkDropMode from protected to public. If you do this can you remove the accessor in ink_drop_host_view_test_api as well? https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:262: SetIntentPickerMenuButtonBackgroundColor(selected_app_tag_, On 2016/08/24 22:18:35, djacobo wrote: > On 2016/08/18 05:45:41, Evan Stade (ooo wed-thurs) wrote: > > seems like you should be setting the ink drop state to ACTIVATED instead of > > setting a bg color (like when you click the app menu button and the menu is > > showing) > > If we wanted to use InkDropState, shouldn't we have our own button class and > call AnimateInkDrop() ? I will make this change if you think is necessary, I > just want to know if that's the correct way of implementing it. I can't speak definitively but I agree with Evan. If you are going to use an InkDropHighlight then I think it would make sense to use the ripple to show pressed/activated state. I'm not sure if you actually need the ACTIVATED state but be aware that the LabelButton doesn't make use of the ACTIVATED state. We can probably speak offline about this if needed. https://codereview.chromium.org/2229943003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:20: #include "ui/views/animation/ink_drop_highlight.h" Is this needed? https://codereview.chromium.org/2229943003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:177: SetPaintToLayer(true); Why is this needed? Is it related to the InkDrop?
The CQ bit was checked by djacobo@google.com to run a CQ dry run
https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:50: class IntentPickerMenuButton : public views::LabelButton { On 2016/08/30 19:16:36, bruthig wrote: > On 2016/08/26 22:44:23, Evan Stade wrote: > > On 2016/08/24 22:18:35, djacobo wrote: > > > On 2016/08/18 05:45:41, Evan Stade wrote: > > > > I don't believe this class is necessary. You could just create a > LabelButton > > > and > > > > set the properties you want. For the ink drop color, > set_ink_drop_base_color > > > > should work. > > > > > > Hmm, that is true for all but SetInkDropMode(), it is a protected method and > I > > > have seen it called only on classes that directly or indirectly inherit from > > > views::CustomButton. is there a public accessory method for this? I am > > guessing > > > something like > > > > > > LabelButton* button = ...; > > > button->set_ink_drop_mode(); > > > > > > Or does this means we will not have hovering effects in the meantime? > > > > I don't know if there's a good reason for SetInkDropMode to be protected. > > +bruthig > > It should be ok to promote InkDropHostView::SetInkDropMode from protected to > public. > > If you do this can you remove the accessor in ink_drop_host_view_test_api as > well? Done, change here https://codereview.chromium.org/2289403002/. https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:262: SetIntentPickerMenuButtonBackgroundColor(selected_app_tag_, On 2016/08/30 19:16:36, bruthig wrote: > On 2016/08/24 22:18:35, djacobo wrote: > > On 2016/08/18 05:45:41, Evan Stade (ooo wed-thurs) wrote: > > > seems like you should be setting the ink drop state to ACTIVATED instead of > > > setting a bg color (like when you click the app menu button and the menu is > > > showing) > > > > If we wanted to use InkDropState, shouldn't we have our own button class and > > call AnimateInkDrop() ? I will make this change if you think is necessary, I > > just want to know if that's the correct way of implementing it. > > I can't speak definitively but I agree with Evan. If you are going to use an > InkDropHighlight then I think it would make sense to use the ripple to show > pressed/activated state. I'm not sure if you actually need the ACTIVATED state > but be aware that the LabelButton doesn't make use of the ACTIVATED state. We > can probably speak offline about this if needed. As chatted offline, using a custom class that overrides NotifyClick(), using InkDropState::HIDDEN and InkDropState::ACTIVATED instead of manually changing the background color. https://codereview.chromium.org/2229943003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:20: #include "ui/views/animation/ink_drop_highlight.h" On 2016/08/30 19:16:37, bruthig wrote: > Is this needed? I guess I tried to use .../ink_drop_host_view.h replacing. Done. https://codereview.chromium.org/2229943003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:177: SetPaintToLayer(true); On 2016/08/30 19:16:37, bruthig wrote: > Why is this needed? Is it related to the InkDrop? I don't think it's related, I had some issues while painting the ScrollView so estade@ provided this fix :)
Dry run: 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
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:50: class IntentPickerMenuButton : public views::LabelButton { On 2016/09/06 23:21:23, djacobo wrote: > On 2016/08/30 19:16:36, bruthig wrote: > > On 2016/08/26 22:44:23, Evan Stade wrote: > > > On 2016/08/24 22:18:35, djacobo wrote: > > > > On 2016/08/18 05:45:41, Evan Stade wrote: > > > > > I don't believe this class is necessary. You could just create a > > LabelButton > > > > and > > > > > set the properties you want. For the ink drop color, > > set_ink_drop_base_color > > > > > should work. > > > > > > > > Hmm, that is true for all but SetInkDropMode(), it is a protected method > and > > I > > > > have seen it called only on classes that directly or indirectly inherit > from > > > > views::CustomButton. is there a public accessory method for this? I am > > > guessing > > > > something like > > > > > > > > LabelButton* button = ...; > > > > button->set_ink_drop_mode(); > > > > > > > > Or does this means we will not have hovering effects in the meantime? > > > > > > I don't know if there's a good reason for SetInkDropMode to be protected. > > > +bruthig > > > > It should be ok to promote InkDropHostView::SetInkDropMode from protected to > > public. > > > > If you do this can you remove the accessor in ink_drop_host_view_test_api as > > well? > > Done, change here https://codereview.chromium.org/2289403002/. Is this needed anymore? https://codereview.chromium.org/2229943003/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:47: nit: no newline required. https://codereview.chromium.org/2229943003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:56: set_has_ink_drop_action_on_click(true); You should be able to leave this as the default false since it won't be used. i.e. IntentPickerMenuButton::NotifyClick() calls directly to Button::NotifyClick() thus skipping CustomButton::NotifyClick() which is the one that actually uses |has_ink_drop_action_on_click_|. https://codereview.chromium.org/2229943003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:62: AnimateInkDrop(views::InkDropState::HIDDEN, I would expect the InkDrop to transition to the DEACTIVATED state when hiding the menu intentionally instead of directly to the HIDDEN state. (FYI animating to DEACTIVATED will auto-transition to HIDDEN) https://codereview.chromium.org/2229943003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:68: AnimateInkDrop(views::InkDropState::ACTIVATED, I'm not very familiar with this code so I can't say for sure if the InkDrop will be making the correct transitions. You should be able to add some tests to verify the ink drop state. See the ShelfViewInkDropTest for an example to work from here: https://cs.chromium.org/chromium/src/ash/shelf/shelf_view_unittest.cc?l=2129&... Correct me if I'm wrong but IntentPickerMenuButton::NotifyClick() will be invoked when showing the menu as well as hiding the menu right? Ideally the InkDrop wouldn't be animated to ACTIVATED when hiding the menu before being animated to HIDDEN by IntentPickerBubbleView::ButtonPressed()->IntentPickerMenuButton::MarkAsUnselected(). I think one solution would be to move this AnimateInkDrop(ACTIVATED) to a new MarkAsSelected() method and have IntentPickerBubbleView::ButtonPressed() invoke the correct one. https://codereview.chromium.org/2229943003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:205: if (ui::MaterialDesignController::IsModeMaterial()) { This block shouldn't be needed for adding the ripple.
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:50: class IntentPickerMenuButton : public views::LabelButton { On 2016/09/07 14:58:43, bruthig wrote: > On 2016/09/06 23:21:23, djacobo wrote: > > On 2016/08/30 19:16:36, bruthig wrote: > > > On 2016/08/26 22:44:23, Evan Stade wrote: > > > > On 2016/08/24 22:18:35, djacobo wrote: > > > > > On 2016/08/18 05:45:41, Evan Stade wrote: > > > > > > I don't believe this class is necessary. You could just create a > > > LabelButton > > > > > and > > > > > > set the properties you want. For the ink drop color, > > > set_ink_drop_base_color > > > > > > should work. > > > > > > > > > > Hmm, that is true for all but SetInkDropMode(), it is a protected method > > and > > > I > > > > > have seen it called only on classes that directly or indirectly inherit > > from > > > > > views::CustomButton. is there a public accessory method for this? I am > > > > guessing > > > > > something like > > > > > > > > > > LabelButton* button = ...; > > > > > button->set_ink_drop_mode(); > > > > > > > > > > Or does this means we will not have hovering effects in the meantime? > > > > > > > > I don't know if there's a good reason for SetInkDropMode to be protected. > > > > +bruthig > > > > > > It should be ok to promote InkDropHostView::SetInkDropMode from protected to > > > public. > > > > > > If you do this can you remove the accessor in ink_drop_host_view_test_api as > > > well? > > > > Done, change here https://codereview.chromium.org/2289403002/. > > Is this needed anymore? Not really since I can already access SetInkDropMode with this inner class, up to you guys whether you want that interface public or not. https://codereview.chromium.org/2229943003/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:47: On 2016/09/07 14:58:43, bruthig wrote: > nit: no newline required. Done. https://codereview.chromium.org/2229943003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:56: set_has_ink_drop_action_on_click(true); On 2016/09/07 14:58:43, bruthig wrote: > You should be able to leave this as the default false since it won't be used. > i.e. IntentPickerMenuButton::NotifyClick() calls directly to > Button::NotifyClick() thus skipping CustomButton::NotifyClick() which is the one > that actually uses |has_ink_drop_action_on_click_|. Done. https://codereview.chromium.org/2229943003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:62: AnimateInkDrop(views::InkDropState::HIDDEN, On 2016/09/07 14:58:43, bruthig wrote: > I would expect the InkDrop to transition to the DEACTIVATED state when hiding > the menu intentionally instead of directly to the HIDDEN state. (FYI animating > to DEACTIVATED will auto-transition to HIDDEN) Done. https://codereview.chromium.org/2229943003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:68: AnimateInkDrop(views::InkDropState::ACTIVATED, On 2016/09/07 14:58:43, bruthig wrote: > I'm not very familiar with this code so I can't say for sure if the InkDrop will > be making the correct transitions. You should be able to add some tests to > verify the ink drop state. See the ShelfViewInkDropTest for an example to work > from here: > https://cs.chromium.org/chromium/src/ash/shelf/shelf_view_unittest.cc?l=2129&... > > Correct me if I'm wrong but IntentPickerMenuButton::NotifyClick() will be > invoked when showing the menu as well as hiding the menu right? Ideally the > InkDrop wouldn't be animated to ACTIVATED when hiding the menu before being > animated to HIDDEN by > IntentPickerBubbleView::ButtonPressed()->IntentPickerMenuButton::MarkAsUnselected(). > > I think one solution would be to move this AnimateInkDrop(ACTIVATED) to a new > MarkAsSelected() method and have IntentPickerBubbleView::ButtonPressed() invoke > the correct one. Agree on implementing MarkAsSelected(), done. https://codereview.chromium.org/2229943003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:205: if (ui::MaterialDesignController::IsModeMaterial()) { On 2016/09/07 14:58:43, bruthig wrote: > This block shouldn't be needed for adding the ripple. Done.
Just a couple of nits, otherwise lgtm https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:50: class IntentPickerMenuButton : public views::LabelButton { On 2016/09/08 00:10:30, djacobo wrote: > On 2016/09/07 14:58:43, bruthig wrote: > > On 2016/09/06 23:21:23, djacobo wrote: > > > On 2016/08/30 19:16:36, bruthig wrote: > > > > On 2016/08/26 22:44:23, Evan Stade wrote: > > > > > On 2016/08/24 22:18:35, djacobo wrote: > > > > > > On 2016/08/18 05:45:41, Evan Stade wrote: > > > > > > > I don't believe this class is necessary. You could just create a > > > > LabelButton > > > > > > and > > > > > > > set the properties you want. For the ink drop color, > > > > set_ink_drop_base_color > > > > > > > should work. > > > > > > > > > > > > Hmm, that is true for all but SetInkDropMode(), it is a protected > method > > > and > > > > I > > > > > > have seen it called only on classes that directly or indirectly > inherit > > > from > > > > > > views::CustomButton. is there a public accessory method for this? I am > > > > > guessing > > > > > > something like > > > > > > > > > > > > LabelButton* button = ...; > > > > > > button->set_ink_drop_mode(); > > > > > > > > > > > > Or does this means we will not have hovering effects in the meantime? > > > > > > > > > > I don't know if there's a good reason for SetInkDropMode to be > protected. > > > > > +bruthig > > > > > > > > It should be ok to promote InkDropHostView::SetInkDropMode from protected > to > > > > public. > > > > > > > > If you do this can you remove the accessor in ink_drop_host_view_test_api > as > > > > well? > > > > > > Done, change here https://codereview.chromium.org/2289403002/. > > > > Is this needed anymore? > > Not really since I can already access SetInkDropMode with this inner class, up > to you guys whether you want that interface public or not. Ok, let's abandon that other change then. https://codereview.chromium.org/2229943003/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:205: SetPaintToLayer(true); I meant the contents of that if block as well. i.e. I don't think lines 205-207 are needed. https://codereview.chromium.org/2229943003/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc (right): https://codereview.chromium.org/2229943003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:132: } nit: It might be worth a test that clicks the same button twice.
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:50: class IntentPickerMenuButton : public views::LabelButton { On 2016/09/08 15:13:12, bruthig wrote: > On 2016/09/08 00:10:30, djacobo wrote: > > On 2016/09/07 14:58:43, bruthig wrote: > > > On 2016/09/06 23:21:23, djacobo wrote: > > > > On 2016/08/30 19:16:36, bruthig wrote: > > > > > On 2016/08/26 22:44:23, Evan Stade wrote: > > > > > > On 2016/08/24 22:18:35, djacobo wrote: > > > > > > > On 2016/08/18 05:45:41, Evan Stade wrote: > > > > > > > > I don't believe this class is necessary. You could just create a > > > > > LabelButton > > > > > > > and > > > > > > > > set the properties you want. For the ink drop color, > > > > > set_ink_drop_base_color > > > > > > > > should work. > > > > > > > > > > > > > > Hmm, that is true for all but SetInkDropMode(), it is a protected > > method > > > > and > > > > > I > > > > > > > have seen it called only on classes that directly or indirectly > > inherit > > > > from > > > > > > > views::CustomButton. is there a public accessory method for this? I > am > > > > > > guessing > > > > > > > something like > > > > > > > > > > > > > > LabelButton* button = ...; > > > > > > > button->set_ink_drop_mode(); > > > > > > > > > > > > > > Or does this means we will not have hovering effects in the > meantime? > > > > > > > > > > > > I don't know if there's a good reason for SetInkDropMode to be > > protected. > > > > > > +bruthig > > > > > > > > > > It should be ok to promote InkDropHostView::SetInkDropMode from > protected > > to > > > > > public. > > > > > > > > > > If you do this can you remove the accessor in > ink_drop_host_view_test_api > > as > > > > > well? > > > > > > > > Done, change here https://codereview.chromium.org/2289403002/. > > > > > > Is this needed anymore? > > > > Not really since I can already access SetInkDropMode with this inner class, up > > to you guys whether you want that interface public or not. > > Ok, let's abandon that other change then. Done. https://codereview.chromium.org/2229943003/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:205: SetPaintToLayer(true); On 2016/09/08 15:13:12, bruthig wrote: > I meant the contents of that if block as well. i.e. I don't think lines 205-207 > are needed. They are, otherwise there is a bad behavior while selecting a row and scrolling up/down on the ScrollView. https://codereview.chromium.org/2229943003/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc (right): https://codereview.chromium.org/2229943003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view_unittest.cc:132: } On 2016/09/08 15:13:12, bruthig wrote: > nit: It might be worth a test that clicks the same button twice. Done.
https://codereview.chromium.org/2229943003/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:205: SetPaintToLayer(true); On 2016/09/08 21:27:06, djacobo wrote: > On 2016/09/08 15:13:12, bruthig wrote: > > I meant the contents of that if block as well. i.e. I don't think lines > 205-207 > > are needed. > > They are, otherwise there is a bad behavior while selecting a row and scrolling > up/down on the ScrollView. The purpose of this code is to clip child views that are painting to Layers. And the item icons will be painted to Layers when an ink drop is active. I suspect you will still get the list item icons floating over the |header| and |footer| views if you perform this clipping via the IntentPickerBubbleView. I think the better approach would be to fix the ScrollView::ScrollsWithLayers() check to inspect all child views of the |contents_viewport_| to see if any of them have a layer.
On 2016/09/12 17:33:11, bruthig wrote: > https://codereview.chromium.org/2229943003/diff/280001/chrome/browser/ui/view... > File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): > > https://codereview.chromium.org/2229943003/diff/280001/chrome/browser/ui/view... > chrome/browser/ui/views/intent_picker_bubble_view.cc:205: SetPaintToLayer(true); > On 2016/09/08 21:27:06, djacobo wrote: > > On 2016/09/08 15:13:12, bruthig wrote: > > > I meant the contents of that if block as well. i.e. I don't think lines > > 205-207 > > > are needed. > > > > They are, otherwise there is a bad behavior while selecting a row and > scrolling > > up/down on the ScrollView. > > The purpose of this code is to clip child views that are painting to Layers. > And the item icons will be painted to Layers when an ink drop is active. I > suspect you will still get the list item icons floating over the |header| and > |footer| views if you perform this clipping via the IntentPickerBubbleView. I > think the better approach would be to fix the ScrollView::ScrollsWithLayers() > check to inspect all child views of the |contents_viewport_| to see if any of > them have a layer. I cannot see painting problems with this approach (tested with many rows), on the other hand I try to craft a fix for ScrollView::ScrollsWithLayers() like: bool ScrollView::ScrollsWithLayers() const { // Check for all the children within |contents_viewport_|. size_t size = contents_viewport_->child_count(); bool layer_on = false; for (size_t i = 0; i < size; ++i) layer_on |= (contents_viewport_->child_at(i)->layer() != nullptr); return layer_on; } I am not sure if you meant |contents_viewport| or |contents_|, could you please clarify?
On 2016/09/12 23:03:24, djacobo wrote: > On 2016/09/12 17:33:11, bruthig wrote: > > > https://codereview.chromium.org/2229943003/diff/280001/chrome/browser/ui/view... > > File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): > > > > > https://codereview.chromium.org/2229943003/diff/280001/chrome/browser/ui/view... > > chrome/browser/ui/views/intent_picker_bubble_view.cc:205: > SetPaintToLayer(true); > > On 2016/09/08 21:27:06, djacobo wrote: > > > On 2016/09/08 15:13:12, bruthig wrote: > > > > I meant the contents of that if block as well. i.e. I don't think lines > > > 205-207 > > > > are needed. > > > > > > They are, otherwise there is a bad behavior while selecting a row and > > scrolling > > > up/down on the ScrollView. > > > > The purpose of this code is to clip child views that are painting to Layers. > > And the item icons will be painted to Layers when an ink drop is active. I > > suspect you will still get the list item icons floating over the |header| and > > |footer| views if you perform this clipping via the IntentPickerBubbleView. I > > think the better approach would be to fix the ScrollView::ScrollsWithLayers() > > check to inspect all child views of the |contents_viewport_| to see if any of > > them have a layer. > > I cannot see painting problems with this approach (tested with many rows), > on the other hand I try to craft a fix for ScrollView::ScrollsWithLayers() like: > > bool ScrollView::ScrollsWithLayers() const { > // Check for all the children within |contents_viewport_|. > size_t size = contents_viewport_->child_count(); > bool layer_on = false; > for (size_t i = 0; i < size; ++i) > layer_on |= (contents_viewport_->child_at(i)->layer() != nullptr); > return layer_on; > } > > I am not sure if you meant |contents_viewport| or |contents_|, could you please > clarify? I took a closer look at ScrollView and it appears like it should be clipping child view layers already. See here https://cs.chromium.org/chromium/src/ui/views/controls/scroll_view.cc?rcl=147... So there might be a deeper problem. Let's not hold this review up on that. Can you please add the following TODO above this block: TODO(djacobo|bruthig): Investigate why the ScrollView does not clip child view layers when the Ink Drop is active.
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/13 16:41:31, bruthig wrote: > On 2016/09/12 23:03:24, djacobo wrote: > > On 2016/09/12 17:33:11, bruthig wrote: > > > > > > https://codereview.chromium.org/2229943003/diff/280001/chrome/browser/ui/view... > > > File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): > > > > > > > > > https://codereview.chromium.org/2229943003/diff/280001/chrome/browser/ui/view... > > > chrome/browser/ui/views/intent_picker_bubble_view.cc:205: > > SetPaintToLayer(true); > > > On 2016/09/08 21:27:06, djacobo wrote: > > > > On 2016/09/08 15:13:12, bruthig wrote: > > > > > I meant the contents of that if block as well. i.e. I don't think lines > > > > 205-207 > > > > > are needed. > > > > > > > > They are, otherwise there is a bad behavior while selecting a row and > > > scrolling > > > > up/down on the ScrollView. > > > > > > The purpose of this code is to clip child views that are painting to Layers. > > > > And the item icons will be painted to Layers when an ink drop is active. I > > > suspect you will still get the list item icons floating over the |header| > and > > > |footer| views if you perform this clipping via the IntentPickerBubbleView. > I > > > think the better approach would be to fix the > ScrollView::ScrollsWithLayers() > > > check to inspect all child views of the |contents_viewport_| to see if any > of > > > them have a layer. > > > > I cannot see painting problems with this approach (tested with many rows), > > on the other hand I try to craft a fix for ScrollView::ScrollsWithLayers() > like: > > > > bool ScrollView::ScrollsWithLayers() const { > > // Check for all the children within |contents_viewport_|. > > size_t size = contents_viewport_->child_count(); > > bool layer_on = false; > > for (size_t i = 0; i < size; ++i) > > layer_on |= (contents_viewport_->child_at(i)->layer() != nullptr); > > return layer_on; > > } > > > > I am not sure if you meant |contents_viewport| or |contents_|, could you > please > > clarify? > > I took a closer look at ScrollView and it appears like it should be clipping > child view layers already. See here > https://cs.chromium.org/chromium/src/ui/views/controls/scroll_view.cc?rcl=147... > > So there might be a deeper problem. Let's not hold this review up on that. Can > you please add the following TODO above this block: > > TODO(djacobo|bruthig): Investigate why the ScrollView does not clip child view > layers when the Ink Drop is active. @bruthig and myself agreed on investigating regarding ScrollView clipping. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2229943003/diff/340001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:43: // IntentPickerMenuButton this is not a MenuButton (i.e. doesn't inherit from views::MenuButton). Could you change the name? https://codereview.chromium.org/2229943003/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:60: AnimateInkDrop(views::InkDropState::DEACTIVATED, I thought you wanted the highlight effect, not an ink drop? https://codereview.chromium.org/2229943003/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:157: return ui::DIALOG_BUTTON_NONE; this means that the dialog won't respond to "enter". It seems like it should though? Why did you choose to have no default? https://codereview.chromium.org/2229943003/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:253: DCHECK(static_cast<size_t>(sender->tag()) < app_info_.size()); DCHECK_LT? https://codereview.chromium.org/2229943003/diff/340001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.h (right): https://codereview.chromium.org/2229943003/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:62: using ThrottleCallback = I thought we were going to change the name of this? https://codereview.chromium.org/2229943003/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:63: base::Callback<void(size_t, arc::ArcNavigationThrottle::CloseReason)>; I'm told we should use int for indexes and not to use size_t as an indication that the value can't be negative.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2229943003/diff/340001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:43: // IntentPickerMenuButton On 2016/09/14 17:48:00, Evan Stade wrote: > this is not a MenuButton (i.e. doesn't inherit from views::MenuButton). Could > you change the name? You are right this is kind of deceiving, done. https://codereview.chromium.org/2229943003/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:60: AnimateInkDrop(views::InkDropState::DEACTIVATED, On 2016/09/14 17:47:59, Evan Stade wrote: > I thought you wanted the highlight effect, not an ink drop? We decided to go with ink drops, as it is at the moment this allow us to see highlights while hovering and pressing a row. https://codereview.chromium.org/2229943003/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:157: return ui::DIALOG_BUTTON_NONE; On 2016/09/14 17:48:00, Evan Stade wrote: > this means that the dialog won't respond to "enter". It seems like it should > though? Why did you choose to have no default? You may be right, we should select "Just once" by default. I just didn't want to bias the selection here. Done. https://codereview.chromium.org/2229943003/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:253: DCHECK(static_cast<size_t>(sender->tag()) < app_info_.size()); On 2016/09/14 17:47:59, Evan Stade wrote: > DCHECK_LT? Done. https://codereview.chromium.org/2229943003/diff/340001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.h (right): https://codereview.chromium.org/2229943003/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:62: using ThrottleCallback = On 2016/09/14 17:48:00, Evan Stade wrote: > I thought we were going to change the name of this? We agreed on changing the callback name for the incognito dialog, but I agree we should do it in here too. let me know what you think, Done. https://codereview.chromium.org/2229943003/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:63: base::Callback<void(size_t, arc::ArcNavigationThrottle::CloseReason)>; On 2016/09/14 17:48:00, Evan Stade wrote: > I'm told we should use int for indexes and not to use size_t as an indication > that the value can't be negative. I am not using size_t just to ensure we have something non-negative, the main reason is I am using that value directly as an index for a vector on the method we access via the callback, given the style guide it makes sense to me to keep this type, but I can agree to change it if required https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... wdyt?
https://codereview.chromium.org/2229943003/diff/340001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:60: AnimateInkDrop(views::InkDropState::DEACTIVATED, On 2016/09/14 21:29:34, djacobo wrote: > On 2016/09/14 17:47:59, Evan Stade wrote: > > I thought you wanted the highlight effect, not an ink drop? > > We decided to go with ink drops, as it is at the moment this allow us to see > highlights while hovering and pressing a row. did you decide to go with ink drops because that's what currently works or because that's what you want long term? If the former, I have a CL that should be landing real soon now to fix it. If the latter, it would greatly simplify my CL not to support invisible/non-existent ink drops. https://codereview.chromium.org/2229943003/diff/340001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.h (right): https://codereview.chromium.org/2229943003/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:63: base::Callback<void(size_t, arc::ArcNavigationThrottle::CloseReason)>; On 2016/09/14 21:29:35, djacobo wrote: > On 2016/09/14 17:48:00, Evan Stade wrote: > > I'm told we should use int for indexes and not to use size_t as an indication > > that the value can't be negative. > > I am not using size_t just to ensure we have something non-negative, the main > reason is I am using that value directly as an index for a vector on the method > we access via the callback, given the style guide it makes sense to me to keep > this type, but I can agree to change it if required > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > wdyt? okey dokey, size_t it is https://codereview.chromium.org/2229943003/diff/380001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:46: // manipulate the internal hovering behavior. Suggested comment: A button that represents a candidate intent handler. https://codereview.chromium.org/2229943003/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:179: app_button->set_tag(i); seems like it would be more graceful to pass the app info struct into the button, then expose some sort of identifier() on IntentPickerLabelButton. Also it seems a little less robust to pass indexes back in the callback because who knows if the model may have changed while the dialog is showing? e.g. if I alt-tabbed and installed a new app that can handle this intent. Is there an app identifier we can return instead? https://codereview.chromium.org/2229943003/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:184: app_button->SetBorder(views::Border::CreateEmptyBorder(10, 16, 10, 0)); all of these calls into app_button I think would be better encapsulated inside the IntentPickerLabelButton class https://codereview.chromium.org/2229943003/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:218: return l10n_util::GetStringUTF16(IDS_INTENT_PICKER_BUBBLE_VIEW_JUST_ONCE); nit: ternary return l10n_util::GetString(button == ... ? ... : ...);
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2229943003/diff/340001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:60: AnimateInkDrop(views::InkDropState::DEACTIVATED, On 2016/09/14 21:53:08, Evan Stade wrote: > On 2016/09/14 21:29:34, djacobo wrote: > > On 2016/09/14 17:47:59, Evan Stade wrote: > > > I thought you wanted the highlight effect, not an ink drop? > > > > We decided to go with ink drops, as it is at the moment this allow us to see > > highlights while hovering and pressing a row. > > did you decide to go with ink drops because that's what currently works or > because that's what you want long term? If the former, I have a CL that should > be landing real soon now to fix it. If the latter, it would greatly simplify my > CL not to support invisible/non-existent ink drops. Pretty much because it is what it works ATM, but also I have no plans to change it unless you guys require it. https://codereview.chromium.org/2229943003/diff/340001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.h (right): https://codereview.chromium.org/2229943003/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:63: base::Callback<void(size_t, arc::ArcNavigationThrottle::CloseReason)>; On 2016/09/14 21:53:08, Evan Stade wrote: > On 2016/09/14 21:29:35, djacobo wrote: > > On 2016/09/14 17:48:00, Evan Stade wrote: > > > I'm told we should use int for indexes and not to use size_t as an > indication > > > that the value can't be negative. > > > > I am not using size_t just to ensure we have something non-negative, the main > > reason is I am using that value directly as an index for a vector on the > method > > we access via the callback, given the style guide it makes sense to me to keep > > this type, but I can agree to change it if required > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > wdyt? > > okey dokey, size_t it is Given our conversation I am using package_name to communicate externally (i.e. with the Navigation Throttle that initially calls the UI), but internally it still makes sense to me to keep a simple integer that matches the position in the ScrollView, wdyt? https://codereview.chromium.org/2229943003/diff/380001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:46: // manipulate the internal hovering behavior. On 2016/09/14 21:53:08, Evan Stade wrote: > Suggested comment: A button that represents a candidate intent handler. Done. https://codereview.chromium.org/2229943003/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:179: app_button->set_tag(i); On 2016/09/14 21:53:08, Evan Stade wrote: > seems like it would be more graceful to pass the app info struct into the > button, then expose some sort of identifier() on IntentPickerLabelButton. > > Also it seems a little less robust to pass indexes back in the callback because > who knows if the model may have changed while the dialog is showing? e.g. if I > alt-tabbed and installed a new app that can handle this intent. Is there an app > identifier we can return instead? taking this offline. https://codereview.chromium.org/2229943003/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:184: app_button->SetBorder(views::Border::CreateEmptyBorder(10, 16, 10, 0)); On 2016/09/14 21:53:08, Evan Stade wrote: > all of these calls into app_button I think would be better encapsulated inside > the IntentPickerLabelButton class Most of the stuff was moved to IntentPickerLabelButton, except for the positioning on the ScrollView and set_tag() which I would like to use for internally identifying the app candidates. https://codereview.chromium.org/2229943003/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:218: return l10n_util::GetStringUTF16(IDS_INTENT_PICKER_BUBBLE_VIEW_JUST_ONCE); On 2016/09/14 21:53:08, Evan Stade wrote: > nit: ternary > > return l10n_util::GetString(button == ... ? ... : ...); Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
Description was changed from
==========
Reusing Ok/Cancel buttons for intent picker
Modifying IntentPickerBubbleView to reuse the title Ok/Cancel buttons in
the BubbleDialogDelegateView class.
Creating a custom LabelButton that adds hovering effeects. Moved some of
the customizations to the definition of this new custom class, also
renamed a couple of accessory methods that directly handle these items.
Modified the unit tests accordingly.
BUG=620129,638063
TEST=Manual test, also IntentPickerBubbleViewTest unit tests passing.
==========
to
==========
Reusing Ok/Cancel buttons for intent picker
Modifying IntentPickerBubbleView to reuse the title Ok/Cancel buttons in
the BubbleDialogDelegateView class.
Creating a custom LabelButton that adds hovering effeects. Moved some of
the customizations to the definition of this new custom class, also
renamed a couple of accessory methods that directly handle these items.
Modified the unit tests accordingly.
Also in this CL, changed the default value of |selected_app_tag_|
so by default the app at index 0 on the list is highlighted, for this
same reason we don't need to disable the DialogButtons anymore.
BUG=620129,638063
TEST=Manual test, also IntentPickerBubbleViewTest unit tests passing.
==========
https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:46: class MousePressedEvent : public ui::Event { This seems like a total hack, can you describe why you need this? It seems like you need it to force the first item to show an icon drop. Is that really necessary? Assuming it is, can you pass in null? From the description: // *** NOTE ***: |event| has been plumbed through on a best effort basis for // the purposes of centering ink drop ripples on located Events. Thus nullptr // has been used by clients who do not have an Event instance available to // them. https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:68: if (!icon->IsEmpty()) { no {} https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:103: intent_picker_cb.Run("" /* Invalid package name */, "" -> std::string() unless somewhere defines a constant as empty string for invalid. https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:127: widget->Show(); Why do you show first, then change a bunch of stuff and trigger layout? https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:149: auto callback = intent_picker_cb_; It's worth commenting as to why you do this rather than using intent_picker_cb_ directly. https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:158: if (!intent_picker_cb_.is_null()) { This close is nearly the same as the accept/cancel/close. Please refactor to a common function so that you aren't duplicating the logic in three places. https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:173: callback.Run("" /* Invalid package name */, std::string(). But as you have this in a couple of places please use a constant for invalid package. https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:217: // TODO(djacobo|bruthig): Investigate why the ScrollView does not clip child Can you clarify what behavior you are seeing that is bad? I could easily see you needing to turn on a layer for the viewport of the scrollview and make it clip children. https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:290: if (!intent_picker_cb_.is_null()) { Refactor this to the common place too. That said, is this really necessary? Doesn't the Close() function you're now overriding cover it? https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.h (right): https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:62: using IntentPickerResponse = How about moving this using to the browser_dialogs so that it's clear they are the same? https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:85: // views::WidgetDelegate overrides: optional: you subclass BubbleDialogDelegateView. I would group the DialogDelegate, WidgetDelegate and BubbleDialogDelegateView overrides there as that is where you get them from. https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:125: size_t selected_app_tag_; optional: = 0 means you don't need to say '0 by default'.
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:46: class MousePressedEvent : public ui::Event { On 2016/10/13 13:35:07, sky wrote: > This seems like a total hack, can you describe why you need this? It seems like > you need it to force the first item to show an icon drop. Is that really > necessary? Assuming it is, can you pass in null? From the description: > > // *** NOTE ***: |event| has been plumbed through on a best effort basis for > // the purposes of centering ink drop ripples on located Events. Thus nullptr > // has been used by clients who do not have an Event instance available to > // them. I modified MarkAsSelected() so we could pass nullptr. Done https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:68: if (!icon->IsEmpty()) { On 2016/10/13 13:35:07, sky wrote: > no {} Done. https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:103: intent_picker_cb.Run("" /* Invalid package name */, On 2016/10/13 13:35:07, sky wrote: > "" -> std::string() unless somewhere defines a constant as empty string for > invalid. Done. https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:127: widget->Show(); On 2016/10/13 13:35:07, sky wrote: > Why do you show first, then change a bunch of stuff and trigger layout? That was a mistake, for a minute I thought Init() was called right before Show(), but that function is tied to CreateBubbel(). Done https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:149: auto callback = intent_picker_cb_; On 2016/10/13 13:35:07, sky wrote: > It's worth commenting as to why you do this rather than using intent_picker_cb_ > directly. Adding a comment to RunCallback(), method that now concentrates repeated code for Running and Reseting the callback. Done. https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:158: if (!intent_picker_cb_.is_null()) { On 2016/10/13 13:35:07, sky wrote: > This close is nearly the same as the accept/cancel/close. Please refactor to a > common function so that you aren't duplicating the logic in three places. Done. https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:173: callback.Run("" /* Invalid package name */, On 2016/10/13 13:35:07, sky wrote: > std::string(). But as you have this in a couple of places please use a constant > for invalid package. Done. https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:217: // TODO(djacobo|bruthig): Investigate why the ScrollView does not clip child On 2016/10/13 13:35:07, sky wrote: > Can you clarify what behavior you are seeing that is bad? I could easily see you > needing to turn on a layer for the viewport of the scrollview and make it clip > children. The problem I was observing is, suppose you select the top-most row on the scroll view and scroll so the previously highlighted row goes outside of the visible region (provided there are enough rows). Of course the row shouldn't be visible, but I was able to still see the highlight and icon floating outside of the scrollview region. I was told these instructions would fix that (and they do) but I am not very knowledgeable about why or if this is actually a proper solution. wdyt? https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:290: if (!intent_picker_cb_.is_null()) { On 2016/10/13 13:35:07, sky wrote: > Refactor this to the common place too. That said, is this really necessary? > Doesn't the Close() function you're now overriding cover it? Refactored, but WebContentsDestroyed() is still needed, I did test destroying the WebContents form outside of this class and the picker didn't close and respond to the caller as we do with this method. Done https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.h (right): https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:62: using IntentPickerResponse = On 2016/10/13 13:35:07, sky wrote: > How about moving this using to the browser_dialogs so that it's clear they are > the same? Done. https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:85: // views::WidgetDelegate overrides: On 2016/10/13 13:35:07, sky wrote: > optional: you subclass BubbleDialogDelegateView. I would group the > DialogDelegate, WidgetDelegate and BubbleDialogDelegateView overrides there as > that is where you get them from. Done. https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.h:125: size_t selected_app_tag_; On 2016/10/13 13:35:07, sky wrote: > optional: = 0 means you don't need to say '0 by default'. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:217: // TODO(djacobo|bruthig): Investigate why the ScrollView does not clip child On 2016/10/14 02:35:31, djacobo wrote: > On 2016/10/13 13:35:07, sky wrote: > > Can you clarify what behavior you are seeing that is bad? I could easily see > you > > needing to turn on a layer for the viewport of the scrollview and make it clip > > children. > > The problem I was observing is, suppose you select the top-most row on the > scroll view and scroll so the previously highlighted row goes outside of the > visible region (provided there are enough rows). Of course the row shouldn't be > visible, but I was able to still see the highlight and icon floating outside of > the scrollview region. I was told these instructions would fix that (and they > do) but I am not very knowledgeable about why or if this is actually a proper > solution. wdyt? It's been a while since I looked at this, but from what I can remember I think we weren't able to turn layer based scrolling on easily because of the feature list check here: https://cs.chromium.org/chromium/src/ui/views/controls/scroll_view.cc?rcl=0&l...
https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:217: // TODO(djacobo|bruthig): Investigate why the ScrollView does not clip child On 2016/10/14 02:35:31, djacobo wrote: > On 2016/10/13 13:35:07, sky wrote: > > Can you clarify what behavior you are seeing that is bad? I could easily see > you > > needing to turn on a layer for the viewport of the scrollview and make it clip > > children. > > The problem I was observing is, suppose you select the top-most row on the > scroll view and scroll so the previously highlighted row goes outside of the > visible region (provided there are enough rows). Of course the row shouldn't be > visible, but I was able to still see the highlight and icon floating outside of > the scrollview region. I was told these instructions would fix that (and they > do) but I am not very knowledgeable about why or if this is actually a proper > solution. wdyt? I'm surprised your fix works. I would expect the children to show ontop of the scrollview border. Is that not the case? The problem would be more apparent if 'this' had another child. As I said, the real fix is to turn on the layer of the viewport. The code Ben points at below is what you want. A fix I'm fine with for the short term is to copy/paste it here. The real fix is for scrollview to detect a child has a layer and automatically turn on a layer for the viewport. https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:290: if (!intent_picker_cb_.is_null()) { On 2016/10/14 02:35:31, djacobo wrote: > On 2016/10/13 13:35:07, sky wrote: > > Refactor this to the common place too. That said, is this really necessary? > > Doesn't the Close() function you're now overriding cover it? > > Refactored, but WebContentsDestroyed() is still needed, I did test destroying > the WebContents form outside of this class and the picker didn't close and > respond to the caller as we do with this method. Done Sorry if I wasn't clear. You definitely want the WebContentsDestroyed() override, but shouldn't the GetWidget()->Close() call in this function trigger IntentPickerBubbleView::Close(), so that you don't need to call to the callback here?
https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:217: // TODO(djacobo|bruthig): Investigate why the ScrollView does not clip child On 2016/10/14 16:08:24, sky wrote: > On 2016/10/14 02:35:31, djacobo wrote: > > On 2016/10/13 13:35:07, sky wrote: > > > Can you clarify what behavior you are seeing that is bad? I could easily see > > you > > > needing to turn on a layer for the viewport of the scrollview and make it > clip > > > children. > > > > The problem I was observing is, suppose you select the top-most row on the > > scroll view and scroll so the previously highlighted row goes outside of the > > visible region (provided there are enough rows). Of course the row shouldn't > be > > visible, but I was able to still see the highlight and icon floating outside > of > > the scrollview region. I was told these instructions would fix that (and they > > do) but I am not very knowledgeable about why or if this is actually a proper > > solution. wdyt? > > I'm surprised your fix works. I would expect the children to show ontop of the > scrollview border. Is that not the case? The problem would be more apparent if > 'this' had another child. As I said, the real fix is to turn on the layer of the > viewport. The code Ben points at below is what you want. A fix I'm fine with for > the short term is to copy/paste it here. The real fix is for scrollview to > detect a child has a layer and automatically turn on a layer for the viewport. To be clear here, and hopefully helpful :), this would need to check recursively if any immediate, or non immediate child view was painting to a layer.
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:217: // TODO(djacobo|bruthig): Investigate why the ScrollView does not clip child On 2016/10/14 16:49:51, bruthig wrote: > On 2016/10/14 16:08:24, sky wrote: > > On 2016/10/14 02:35:31, djacobo wrote: > > > On 2016/10/13 13:35:07, sky wrote: > > > > Can you clarify what behavior you are seeing that is bad? I could easily > see > > > you > > > > needing to turn on a layer for the viewport of the scrollview and make it > > clip > > > > children. > > > > > > The problem I was observing is, suppose you select the top-most row on the > > > scroll view and scroll so the previously highlighted row goes outside of the > > > visible region (provided there are enough rows). Of course the row shouldn't > > be > > > visible, but I was able to still see the highlight and icon floating outside > > of > > > the scrollview region. I was told these instructions would fix that (and > they > > > do) but I am not very knowledgeable about why or if this is actually a > proper > > > solution. wdyt? > > > > I'm surprised your fix works. I would expect the children to show ontop of the > > scrollview border. Is that not the case? The problem would be more apparent if > > 'this' had another child. As I said, the real fix is to turn on the layer of > the > > viewport. The code Ben points at below is what you want. A fix I'm fine with > for > > the short term is to copy/paste it here. The real fix is for scrollview to > > detect a child has a layer and automatically turn on a layer for the viewport. > > To be clear here, and hopefully helpful :), this would need to check recursively > if any immediate, or non immediate child view was painting to a layer. > So the quick fix would be to reuse the part of the code that is never reached in ScrollView, this is: background_color_ = SK_ColorWHITE; contents_viewport_->set_background( Background::CreateSolidBackground(background_color_)); contents_viewport_->SetPaintToLayer(true); contents_viewport_->layer()->SetMasksToBounds(true); Since I would prefer not to expose |contents_viewport_| I included an interface for this as ActivateLayer(bool), it works as expected. I am not completely sure about how to implement what you suggest Ben, what I understand is to check for layer()s within the |contents_viewport_| subtree, this of course should be called after all the contents are added to the ScrollView. In theory then, I should be able to call ActivateLayer(false) after all the labels are added to the ScrollView? Do I need to update anything else within the ScrollView's children? https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:290: if (!intent_picker_cb_.is_null()) { On 2016/10/14 16:08:24, sky wrote: > On 2016/10/14 02:35:31, djacobo wrote: > > On 2016/10/13 13:35:07, sky wrote: > > > Refactor this to the common place too. That said, is this really necessary? > > > Doesn't the Close() function you're now overriding cover it? > > > > Refactored, but WebContentsDestroyed() is still needed, I did test destroying > > the WebContents form outside of this class and the picker didn't close and > > respond to the caller as we do with this method. Done > > Sorry if I wasn't clear. You definitely want the WebContentsDestroyed() > override, but shouldn't the GetWidget()->Close() call in this function trigger > IntentPickerBubbleView::Close(), so that you don't need to call to the callback > here? Done.
https://codereview.chromium.org/2229943003/diff/500001/ui/views/controls/scro... File ui/views/controls/scroll_view.cc (right): https://codereview.chromium.org/2229943003/diff/500001/ui/views/controls/scro... ui/views/controls/scroll_view.cc:686: void ScrollView::ActivateLayer(bool force_layer_creation) { This name is mildly confusing and why does it take a boolean and have the checks below? You only call it in one place and with true. How about calling it EnableViewportLayer()? Also, please file a bug that it shouldn't be necessary with a TODO referencing said bug.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2229943003/diff/500001/ui/views/controls/scro... File ui/views/controls/scroll_view.cc (right): https://codereview.chromium.org/2229943003/diff/500001/ui/views/controls/scro... ui/views/controls/scroll_view.cc:686: void ScrollView::ActivateLayer(bool force_layer_creation) { On 2016/10/14 22:59:27, sky wrote: > This name is mildly confusing and why does it take a boolean and have the checks > below? You only call it in one place and with true. How about calling it > EnableViewportLayer()? Also, please file a bug that it shouldn't be necessary > with a TODO referencing said bug. I am still trying to figure out how to do the proper fix, so I was basically mixing both ideas, sorry for the confusion. I will get rid of ChildrenLayerCheck() for now and set this temporary fix. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:217: // TODO(djacobo|bruthig): Investigate why the ScrollView does not clip child > check for layer()s within the |contents_viewport_| subtree, Sorry for my poor wording, but that is what I meant. You should check the entire |contents_viewport_| subtree and not just |contents_viewport_|'s immediate children.
LGTM with the following https://codereview.chromium.org/2229943003/diff/520001/ui/views/controls/scro... File ui/views/controls/scroll_view.cc (right): https://codereview.chromium.org/2229943003/diff/520001/ui/views/controls/scro... ui/views/controls/scroll_view.cc:186: background_color_ = SK_ColorWHITE; Call EnableViewPortLayer() rather than duplicating?
(no need to wait for my lg since I'm still ooo) https://codereview.chromium.org/2229943003/diff/520001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/520001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:102: // Add a 1-pixel extra boundary left and right when using secondary UI. I kind of expect this is papering over a bug where we're drawing the border inside rather than outside the dialog --- can you add a todo to remove this with a link to bug 656662 https://codereview.chromium.org/2229943003/diff/520001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:163: return ui::DIALOG_BUTTON_OK; do you need this? isn't it the default (more or less)?
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/17 15:56:43, Evan Stade (ooo till 10-20) wrote: > (no need to wait for my lg since I'm still ooo) > > https://codereview.chromium.org/2229943003/diff/520001/chrome/browser/ui/view... > File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): > > https://codereview.chromium.org/2229943003/diff/520001/chrome/browser/ui/view... > chrome/browser/ui/views/intent_picker_bubble_view.cc:102: // Add a 1-pixel extra > boundary left and right when using secondary UI. > I kind of expect this is papering over a bug where we're drawing the border > inside rather than outside the dialog --- can you add a todo to remove this with > a link to bug 656662 > > https://codereview.chromium.org/2229943003/diff/520001/chrome/browser/ui/view... > chrome/browser/ui/views/intent_picker_bubble_view.cc:163: return > ui::DIALOG_BUTTON_OK; > do you need this? isn't it the default (more or less)? You are right. Done.
https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:217: // TODO(djacobo|bruthig): Investigate why the ScrollView does not clip child On 2016/10/17 13:55:42, bruthig wrote: > > check for layer()s within the |contents_viewport_| subtree, > > Sorry for my poor wording, but that is what I meant. You should check the > entire |contents_viewport_| subtree and not just |contents_viewport_|'s > immediate children. > > let me give it another shot in the separated bug. Ack. https://codereview.chromium.org/2229943003/diff/520001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/520001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:102: // Add a 1-pixel extra boundary left and right when using secondary UI. On 2016/10/17 15:56:43, Evan Stade (ooo till 10-20) wrote: > I kind of expect this is papering over a bug where we're drawing the border > inside rather than outside the dialog --- can you add a todo to remove this with > a link to bug 656662 Done. https://codereview.chromium.org/2229943003/diff/520001/ui/views/controls/scro... File ui/views/controls/scroll_view.cc (right): https://codereview.chromium.org/2229943003/diff/520001/ui/views/controls/scro... ui/views/controls/scroll_view.cc:186: background_color_ = SK_ColorWHITE; On 2016/10/17 15:32:49, sky wrote: > Call EnableViewPortLayer() rather than duplicating? Done.
The CQ bit was checked by djacobo@google.com to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by djacobo@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from yusukes@chromium.org, bruthig@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2229943003/#ps560001 (title: "Getting rid of GetDefaultDialogButton()")
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
==========
Reusing Ok/Cancel buttons for intent picker
Modifying IntentPickerBubbleView to reuse the title Ok/Cancel buttons in
the BubbleDialogDelegateView class.
Creating a custom LabelButton that adds hovering effeects. Moved some of
the customizations to the definition of this new custom class, also
renamed a couple of accessory methods that directly handle these items.
Modified the unit tests accordingly.
Also in this CL, changed the default value of |selected_app_tag_|
so by default the app at index 0 on the list is highlighted, for this
same reason we don't need to disable the DialogButtons anymore.
BUG=620129,638063
TEST=Manual test, also IntentPickerBubbleViewTest unit tests passing.
==========
to
==========
Reusing Ok/Cancel buttons for intent picker
Modifying IntentPickerBubbleView to reuse the title Ok/Cancel buttons in
the BubbleDialogDelegateView class.
Creating a custom LabelButton that adds hovering effeects. Moved some of
the customizations to the definition of this new custom class, also
renamed a couple of accessory methods that directly handle these items.
Modified the unit tests accordingly.
Also in this CL, changed the default value of |selected_app_tag_|
so by default the app at index 0 on the list is highlighted, for this
same reason we don't need to disable the DialogButtons anymore.
BUG=620129,638063
TEST=Manual test, also IntentPickerBubbleViewTest unit tests passing.
==========
Message was sent while issue was closed.
Committed patchset #29 (id:560001)
Message was sent while issue was closed.
I'm seeing consistent ASan failure in IntentPickerBubbleViewTest.InkDropStateTransition IntentPickerBubbleViewTest.PressButtonTwice Link: https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2... There might be some memory issues in this CL, so I'm going to revert. Sorry for the inconvenience.
Message was sent while issue was closed.
Filed http://crbug.com/657240 Have to manually revert.
Message was sent while issue was closed.
CL for the revert: https://chromiumcodereview.appspot.com/2433733002
Message was sent while issue was closed.
On 2016/10/19 03:53:17, huangs wrote: > CL for the revert: https://chromiumcodereview.appspot.com/2433733002 Revert landed and ASan failure disappeared.
Message was sent while issue was closed.
https://codereview.chromium.org/2229943003/diff/560001/chrome/browser/ui/view... File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): https://codereview.chromium.org/2229943003/diff/560001/chrome/browser/ui/view... chrome/browser/ui/views/intent_picker_bubble_view.cc:103: // http://crbug.com/656662 gets fixed. I see this patch had to be reverted temporarily, which is unfortunate, but on the bright side this bug should be fixed by https://codereview.chromium.org/2439793002/ so depending on how quickly that lands, you may not need to add this workaround.
Message was sent while issue was closed.
On 2016/10/20 16:13:47, Evan Stade wrote: > https://codereview.chromium.org/2229943003/diff/560001/chrome/browser/ui/view... > File chrome/browser/ui/views/intent_picker_bubble_view.cc (right): > > https://codereview.chromium.org/2229943003/diff/560001/chrome/browser/ui/view... > chrome/browser/ui/views/intent_picker_bubble_view.cc:103: // > http://crbug.com/656662 gets fixed. > I see this patch had to be reverted temporarily, which is unfortunate, but on > the bright side this bug should be fixed by > https://codereview.chromium.org/2439793002/ so depending on how quickly that > lands, you may not need to add this workaround. I tried to speed it up, already landed, I will keep track on your CL to get rid of the TODO accordingly, thanks Evan.
Message was sent while issue was closed.
Description was changed from
==========
Reusing Ok/Cancel buttons for intent picker
Modifying IntentPickerBubbleView to reuse the title Ok/Cancel buttons in
the BubbleDialogDelegateView class.
Creating a custom LabelButton that adds hovering effeects. Moved some of
the customizations to the definition of this new custom class, also
renamed a couple of accessory methods that directly handle these items.
Modified the unit tests accordingly.
Also in this CL, changed the default value of |selected_app_tag_|
so by default the app at index 0 on the list is highlighted, for this
same reason we don't need to disable the DialogButtons anymore.
BUG=620129,638063
TEST=Manual test, also IntentPickerBubbleViewTest unit tests passing.
==========
to
==========
Reusing Ok/Cancel buttons for intent picker
Modifying IntentPickerBubbleView to reuse the title Ok/Cancel buttons in
the BubbleDialogDelegateView class.
Creating a custom LabelButton that adds hovering effeects. Moved some of
the customizations to the definition of this new custom class, also
renamed a couple of accessory methods that directly handle these items.
Modified the unit tests accordingly.
Also in this CL, changed the default value of |selected_app_tag_|
so by default the app at index 0 on the list is highlighted, for this
same reason we don't need to disable the DialogButtons anymore.
BUG=620129,638063
TEST=Manual test, also IntentPickerBubbleViewTest unit tests passing.
Committed: https://crrev.com/a1fd64d48d76266f9c6db6378346cdec2f416c27
Cr-Commit-Position: refs/heads/master@{#426094}
==========
Message was sent while issue was closed.
Patchset 29 (id:??) landed as https://crrev.com/a1fd64d48d76266f9c6db6378346cdec2f416c27 Cr-Commit-Position: refs/heads/master@{#426094} |
