Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1661673004:
Enables hot-tracking for overflow extension buttons in the app menu (Closed)
DescriptionEnables hot-tracking for overflow extension buttons in the app menu
BUG=583559
Committed: https://crrev.com/ef6637ffae36b30abf91c6cd78dcd2fa7a3f59b5
Cr-Commit-Position: refs/heads/master@{#377999}
Patch Set 1 #Patch Set 2 : Restores hot-tracking of extension buttons in app menu with MD #
Total comments: 2
Patch Set 3 : Restores hot-tracking of extension buttons in app menu with MD #
Total comments: 19
Patch Set 4 : Restores hot-tracking of extension buttons in app menu with MD (comments) #
Total comments: 14
Patch Set 5 : Restores hot-tracking of extension buttons in app menu with MD (comments) #Patch Set 6 : Restores hot-tracking of extension buttons in app menu with MD (fixed a test) #Patch Set 7 : Restores hot-tracking of extension buttons in app menu with MD (fixed Win build) #Patch Set 8 : Restores hot-tracking of extension buttons in app menu with MD (removed a DCHECK) #Patch Set 9 : Restores hot-tracking of extension buttons in app menu with MD (default ctor for ExtraParams) #
Total comments: 2
Patch Set 10 : Restores hot-tracking of extension buttons in app menu with MD (win build) #
Total comments: 12
Patch Set 11 : Restores hot-tracking of extension buttons in app menu with MD (added tests) #Patch Set 12 : Restores hot-tracking of extension buttons in app menu with MD (tests) #
Total comments: 8
Patch Set 13 : Restores hot-tracking of extension buttons in app menu with MD (comments) #
Total comments: 10
Patch Set 14 : Restores hot-tracking of extension buttons in app menu with MD (rebase and comments) #Patch Set 15 : Restores hot-tracking of extension buttons in app menu with MD (with ButtonStateObserver) #Patch Set 16 : Restores hot-tracking of extension buttons in app menu with MD (deferred changes to MenuController) #Patch Set 17 : Restores hot-tracking of extension buttons in app menu with MD (rebase) #
Total comments: 2
Patch Set 18 : Restores hot-tracking of extension buttons in app menu with MD (nit) #
Total comments: 2
Messages
Total messages: 121 (49 generated)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1661673004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1661673004/1
varkha@chromium.org changed reviewers: + bruthig@chromium.org
bruthig@, can you please take a first look? Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
varkha@chromium.org changed reviewers: + pkasting@chromium.org, sadrul@chromium.org
sadrul@chromium.org: Can you please review changes in ui/views/controls/menu/. pkasting@chromium.org: Can you please review changes in ui/native_theme/ and chrome/browser/ui/. Thanks!
https://codereview.chromium.org/1661673004/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/app_menu.cc (right): https://codereview.chromium.org/1661673004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/app_menu.cc:173: bounds.Inset(gfx::Insets(0, 1, 0, 0)); This small bug was causing a left border vertical line drawn with FillRect above to be obscured by the hot-tracking background. https://codereview.chromium.org/1661673004/diff/60001/ui/native_theme/native_... File ui/native_theme/native_theme_base.h (right): https://codereview.chromium.org/1661673004/diff/60001/ui/native_theme/native_... ui/native_theme/native_theme_base.h:111: const MenuItemExtraParams& menu_item) const; This seems to be a typo as seen by a call site here: https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1661673004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1661673004/60001
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1661673004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1661673004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
https://codereview.chromium.org/1661673004/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/app_menu.cc (right): https://codereview.chromium.org/1661673004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/app_menu.cc:145: corner_radius_(ui::MaterialDesignController::IsModeMaterial() ? 2 : 0) { Why is this a member at all instead of simply being coded in directly at the one use below? https://codereview.chromium.org/1661673004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/app_menu.cc:1153: break; It really worries me that |extension_toolbar_| is left pointing to a dangling object here. I think this member should only be set in the case where we're going to add the child view, and it should be reset anywhere the code deletes that child view. Another way would be to make the member a scoped_ptr and mark this as client-owned, but that would keep the object alive longer even when not necessary, which may not be desirable. https://codereview.chromium.org/1661673004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/app_menu.cc:1161: InMenuButtonBackground::RIGHT_BUTTON); Nit: I would inline this into the next statement mostly to avoid creating any objects of raw pointer type. (I have a dream that someday Chrome will never store the result of an allocation directly into a raw pointer type.) https://codereview.chromium.org/1661673004/diff/80001/ui/views/controls/menu/... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1661673004/diff/80001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.cc:2622: NOTREACHED(); Nit: Simpler: DCHECK(is_hot_tracked || (hot_button_ == hot_button)); hot_button_ = is_hot_tracked ? hot_button : nullptr; https://codereview.chromium.org/1661673004/diff/80001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.cc:2631: View* to_make_hot = Nit: |hot_view| is shorter and, to me, less awkward but just as clear; it's also parallel with my next suggestion https://codereview.chromium.org/1661673004/diff/80001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.cc:2633: CustomButton* button_hot = CustomButton::AsCustomButton(to_make_hot); Nit: Use |hot_button| for consistency with previous function https://codereview.chromium.org/1661673004/diff/80001/ui/views/controls/menu/... File ui/views/controls/menu/menu_scroll_view_container.cc (right): https://codereview.chromium.org/1661673004/diff/80001/ui/views/controls/menu/... ui/views/controls/menu/menu_scroll_view_container.cc:78: NativeTheme::ExtraParams extra = NativeTheme::ExtraParams(); Why is explicitly calling the null constructor needed?
Thanks, can you PTAL? https://codereview.chromium.org/1661673004/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/app_menu.cc (right): https://codereview.chromium.org/1661673004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/app_menu.cc:145: corner_radius_(ui::MaterialDesignController::IsModeMaterial() ? 2 : 0) { On 2016/02/06 02:51:12, Peter Kasting wrote: > Why is this a member at all instead of simply being coded in directly at the one > use below? Done. https://codereview.chromium.org/1661673004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/app_menu.cc:1153: break; On 2016/02/06 02:51:12, Peter Kasting wrote: > It really worries me that |extension_toolbar_| is left pointing to a dangling > object here. > > I think this member should only be set in the case where we're going to add the > child view, and it should be reset anywhere the code deletes that child view. > > Another way would be to make the member a scoped_ptr and mark this as > client-owned, but that would keep the object alive longer even when not > necessary, which may not be desirable. Done. https://codereview.chromium.org/1661673004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/app_menu.cc:1161: InMenuButtonBackground::RIGHT_BUTTON); On 2016/02/06 02:51:12, Peter Kasting wrote: > Nit: I would inline this into the next statement mostly to avoid creating any > objects of raw pointer type. (I have a dream that someday Chrome will never > store the result of an allocation directly into a raw pointer type.) Done. https://codereview.chromium.org/1661673004/diff/80001/ui/views/controls/menu/... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1661673004/diff/80001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.cc:2622: NOTREACHED(); On 2016/02/06 02:51:12, Peter Kasting wrote: > Nit: Simpler: > > DCHECK(is_hot_tracked || (hot_button_ == hot_button)); > hot_button_ = is_hot_tracked ? hot_button : nullptr; Done. https://codereview.chromium.org/1661673004/diff/80001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.cc:2631: View* to_make_hot = On 2016/02/06 02:51:13, Peter Kasting wrote: > Nit: |hot_view| is shorter and, to me, less awkward but just as clear; it's also > parallel with my next suggestion Done. https://codereview.chromium.org/1661673004/diff/80001/ui/views/controls/menu/... ui/views/controls/menu/menu_controller.cc:2633: CustomButton* button_hot = CustomButton::AsCustomButton(to_make_hot); On 2016/02/06 02:51:12, Peter Kasting wrote: > Nit: Use |hot_button| for consistency with previous function Done. https://codereview.chromium.org/1661673004/diff/80001/ui/views/controls/menu/... File ui/views/controls/menu/menu_scroll_view_container.cc (right): https://codereview.chromium.org/1661673004/diff/80001/ui/views/controls/menu/... ui/views/controls/menu/menu_scroll_view_container.cc:78: NativeTheme::ExtraParams extra = NativeTheme::ExtraParams(); On 2016/02/06 02:51:13, Peter Kasting wrote: > Why is explicitly calling the null constructor needed? To make sure all members are zero-initialized (NativeTheme::ExtraParams doesn't have a declared constructor so the initial values are not defined).
https://codereview.chromium.org/1661673004/diff/80001/ui/views/controls/menu/... File ui/views/controls/menu/menu_scroll_view_container.cc (right): https://codereview.chromium.org/1661673004/diff/80001/ui/views/controls/menu/... ui/views/controls/menu/menu_scroll_view_container.cc:78: NativeTheme::ExtraParams extra = NativeTheme::ExtraParams(); On 2016/02/16 19:59:44, varkha wrote: > On 2016/02/06 02:51:13, Peter Kasting wrote: > > Why is explicitly calling the null constructor needed? > > To make sure all members are zero-initialized (NativeTheme::ExtraParams doesn't > have a declared constructor so the initial values are not defined). I thought the standard said that for a union without a user-defined default constructor, value initialization is zero initialization, and zero initialization zero-initializes the first member of the union and then zeroes the rest of the memory (padding) in the union. I've never seen anyone need to explicitly call a default constructor like this. Its necessity would really surprise me. Can you find some language (or demonstrate via a test program output) that falsifies my understanding above? https://codereview.chromium.org/1661673004/diff/100001/chrome/browser/ui/libg... File chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc (right): https://codereview.chromium.org/1661673004/diff/100001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:223: SkIntToScalar(menu_item.corner_radius), paint); Nit: Shorter: const SkScalar radius = SkIntToScalar(menu_item.corner_radius); canvas->drawRoundRect(gfx::RectToSkRect(rect), radius, radius, paint); https://codereview.chromium.org/1661673004/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu.cc (right): https://codereview.chromium.org/1661673004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu.cc:134: const int kBackgroundCornerRadius = 2; This does not need to be class-scope; it can be function-scope (and scoped as narrowly as possible within) in the function where you use it. https://codereview.chromium.org/1661673004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu.cc:1145: extension_toolbar_ = nullptr; Nit: I would probably put this inside the ShouldShow() conditional body below to make clearer that that's the reason why we need this. https://codereview.chromium.org/1661673004/diff/100001/ui/native_theme/common... File ui/native_theme/common_theme.cc (right): https://codereview.chromium.org/1661673004/diff/100001/ui/native_theme/common... ui/native_theme/common_theme.cc:486: SkIntToScalar(menu_item.corner_radius), paint); Nit: Same comment https://codereview.chromium.org/1661673004/diff/100001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1661673004/diff/100001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:73: static int menu_selection_hold_time_ms = kMinimumMsPressedToActivate; Nit: While here: Can you remove the unnecessary "static" qualifiers on the variables and functions in this anonymous namespace? https://codereview.chromium.org/1661673004/diff/100001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:117: CustomButton* OtherHotTrackedButton(View* view, CustomButton* hot_button) { Nit: Add comment explaining what this function does, as its function is not immediately clear from its name. https://codereview.chromium.org/1661673004/diff/100001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:2632: return; Nit: Unnecessary statement
Patchset #5 (id:120001) has been deleted
Thanks, PTAL. https://codereview.chromium.org/1661673004/diff/80001/ui/views/controls/menu/... File ui/views/controls/menu/menu_scroll_view_container.cc (right): https://codereview.chromium.org/1661673004/diff/80001/ui/views/controls/menu/... ui/views/controls/menu/menu_scroll_view_container.cc:78: NativeTheme::ExtraParams extra = NativeTheme::ExtraParams(); On 2016/02/16 21:36:14, Peter Kasting wrote: > On 2016/02/16 19:59:44, varkha wrote: > > On 2016/02/06 02:51:13, Peter Kasting wrote: > > > Why is explicitly calling the null constructor needed? > > > > To make sure all members are zero-initialized (NativeTheme::ExtraParams > doesn't > > have a declared constructor so the initial values are not defined). > > I thought the standard said that for a union without a user-defined default > constructor, value initialization is zero initialization, and zero > initialization zero-initializes the first member of the union and then zeroes > the rest of the memory (padding) in the union. > > I've never seen anyone need to explicitly call a default constructor like this. > Its necessity would really surprise me. Can you find some language (or > demonstrate via a test program output) that falsifies my understanding above? Yes, I have tried this without this line and the corner radius was random. This is because there is no initializer / constructor at all. C++ allows the first member of a union to be initialized [1]. In our case the union is as initialized as its first structure which does not have a constructor so this [2] applies and thus there is no default value for any of the union members. So I think that when there is some, even a partial initializer, the rest would get zero-initialized but since we have none at all the initial values are not defined. Note that I am using a zero-init syntax (similar to "int var = int();"), not a default constructor (which we do not have in code and maybe we should have in this case). That is what is ensuring zero values [3]. As for the tracing output: If I omit "= ui::NativeTheme::ExtraParams()" in line 220 of app_menu.cc I get this output from the following added line: 221: LOG(ERROR) << __FUNCTION__ << " radius:" << params.menu_background.corner_radius; [24068:24068:0217/121146:ERROR:app_menu.cc(221)] DrawBackground radius:24 [24068:24068:0217/121146:ERROR:app_menu.cc(221)] DrawBackground radius:-761010977 [24068:24068:0217/121146:ERROR:app_menu.cc(221)] DrawBackground radius:410410428 [24068:24068:0217/121146:ERROR:app_menu.cc(221)] DrawBackground radius:24 [24068:24068:0217/121146:ERROR:app_menu.cc(221)] DrawBackground radius:24 [24068:24068:0217/121146:ERROR:app_menu.cc(221)] DrawBackground radius:-761010977 [24068:24068:0217/121146:ERROR:app_menu.cc(221)] DrawBackground radius:24 If I have it, I get this: [24721:24721:0217/121300:ERROR:app_menu.cc(221)] DrawBackground radius:0 [24721:24721:0217/121300:ERROR:app_menu.cc(221)] DrawBackground radius:0 [24721:24721:0217/121300:ERROR:app_menu.cc(221)] DrawBackground radius:0 [24721:24721:0217/121301:ERROR:app_menu.cc(221)] DrawBackground radius:0 [24721:24721:0217/121302:ERROR:app_menu.cc(221)] DrawBackground radius:0 [24721:24721:0217/121302:ERROR:app_menu.cc(221)] DrawBackground radius:0 [1] - "The default initializer for a union with static storage is the default for the first component. A union with automatic storage has no default initialization." https://www-01.ibm.com/support/knowledgecenter/SSLTBW_2.1.0/com.ibm.zos.v2r1.... [2] - "If a structure variable does not have an initializer, the initial values of the structure members depend on the storage class associated with the structure variable: If a structure variable has static storage, its members are implicitly initialized to zero of the appropriate type. If a structure variable has automatic storage, its members have no default initialization. If a structure variable is partially initialized, all the uninitialized structure members are implicitly initialized to zero no matter what the storage class of the structure variable is." [3] - http://en.cppreference.com/w/cpp/language/zero_initialization https://codereview.chromium.org/1661673004/diff/100001/chrome/browser/ui/libg... File chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc (right): https://codereview.chromium.org/1661673004/diff/100001/chrome/browser/ui/libg... chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc:223: SkIntToScalar(menu_item.corner_radius), paint); On 2016/02/16 21:36:15, Peter Kasting wrote: > Nit: Shorter: > > const SkScalar radius = SkIntToScalar(menu_item.corner_radius); > canvas->drawRoundRect(gfx::RectToSkRect(rect), radius, radius, paint); Done. https://codereview.chromium.org/1661673004/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu.cc (right): https://codereview.chromium.org/1661673004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu.cc:134: const int kBackgroundCornerRadius = 2; On 2016/02/16 21:36:15, Peter Kasting wrote: > This does not need to be class-scope; it can be function-scope (and scoped as > narrowly as possible within) in the function where you use it. Done. https://codereview.chromium.org/1661673004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu.cc:1145: extension_toolbar_ = nullptr; On 2016/02/16 21:36:15, Peter Kasting wrote: > Nit: I would probably put this inside the ShouldShow() conditional body below to > make clearer that that's the reason why we need this. Done. https://codereview.chromium.org/1661673004/diff/100001/ui/native_theme/common... File ui/native_theme/common_theme.cc (right): https://codereview.chromium.org/1661673004/diff/100001/ui/native_theme/common... ui/native_theme/common_theme.cc:486: SkIntToScalar(menu_item.corner_radius), paint); On 2016/02/16 21:36:15, Peter Kasting wrote: > Nit: Same comment Done. https://codereview.chromium.org/1661673004/diff/100001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1661673004/diff/100001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:73: static int menu_selection_hold_time_ms = kMinimumMsPressedToActivate; On 2016/02/16 21:36:15, Peter Kasting wrote: > Nit: While here: Can you remove the unnecessary "static" qualifiers on the > variables and functions in this anonymous namespace? Done. I have also moved other constants into the anonymous namespace. https://codereview.chromium.org/1661673004/diff/100001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:117: CustomButton* OtherHotTrackedButton(View* view, CustomButton* hot_button) { On 2016/02/16 21:36:15, Peter Kasting wrote: > Nit: Add comment explaining what this function does, as its function is not > immediately clear from its name. Done, PTAL if this is OK. https://codereview.chromium.org/1661673004/diff/100001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:2632: return; On 2016/02/16 21:36:15, Peter Kasting wrote: > Nit: Unnecessary statement Done.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1661673004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1661673004/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1661673004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1661673004/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1661673004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1661673004/200001
LGTM other than the below https://codereview.chromium.org/1661673004/diff/80001/ui/views/controls/menu/... File ui/views/controls/menu/menu_scroll_view_container.cc (right): https://codereview.chromium.org/1661673004/diff/80001/ui/views/controls/menu/... ui/views/controls/menu/menu_scroll_view_container.cc:78: NativeTheme::ExtraParams extra = NativeTheme::ExtraParams(); That language in your footnotes is exactly what I was missing. Thanks! I think the safest thing to do here is to add some sort of default constructor to the union so that no one else can run into this issue in the future. How's that sound?
+sadrul@, can you please take a look in ui/views/? https://codereview.chromium.org/1661673004/diff/80001/ui/views/controls/menu/... File ui/views/controls/menu/menu_scroll_view_container.cc (right): https://codereview.chromium.org/1661673004/diff/80001/ui/views/controls/menu/... ui/views/controls/menu/menu_scroll_view_container.cc:78: NativeTheme::ExtraParams extra = NativeTheme::ExtraParams(); On 2016/02/17 22:12:04, Peter Kasting wrote: > That language in your footnotes is exactly what I was missing. Thanks! > > I think the safest thing to do here is to add some sort of default constructor > to the union so that no one else can run into this issue in the future. How's > that sound? I'll do that.
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 varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1661673004/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1661673004/220001
Patchset #9 (id:220001) has been deleted
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1661673004/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1661673004/240001
https://codereview.chromium.org/1661673004/diff/80001/ui/views/controls/menu/... File ui/views/controls/menu/menu_scroll_view_container.cc (right): https://codereview.chromium.org/1661673004/diff/80001/ui/views/controls/menu/... ui/views/controls/menu/menu_scroll_view_container.cc:78: NativeTheme::ExtraParams extra = NativeTheme::ExtraParams(); On 2016/02/17 22:12:04, Peter Kasting wrote: > That language in your footnotes is exactly what I was missing. Thanks! > > I think the safest thing to do here is to add some sort of default constructor > to the union so that no one else can run into this issue in the future. How's > that sound? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
LGTM with question https://codereview.chromium.org/1661673004/diff/240001/ui/native_theme/native... File ui/native_theme/native_theme.h (right): https://codereview.chromium.org/1661673004/diff/240001/ui/native_theme/native... ui/native_theme/native_theme.h:201: ExtraParams(); Does just using = default work? Or does the compiler refuse to generate this?
https://codereview.chromium.org/1661673004/diff/240001/ui/native_theme/native... File ui/native_theme/native_theme.h (right): https://codereview.chromium.org/1661673004/diff/240001/ui/native_theme/native... ui/native_theme/native_theme.h:201: ExtraParams(); On 2016/02/18 04:03:43, Peter Kasting wrote: > Does just using = default work? Or does the compiler refuse to generate this? No, unfortunately I am still getting uninitialized values if I do (in cc file): NativeTheme::ExtraParams::ExtraParams() = default; Unrelated, I am getting style violation if I do "= default" in the header: ../../ui/native_theme/native_theme.h:201:5: error: [chromium-style] Complex constructor has an inlined body. ExtraParams() = default; I believe "= default" merely creates an empty constructor with an empty initializer list and an empty body and still does not initialize values. Definitions here - http://en.cppreference.com/w/cpp/language/default_initialization
pkasting, I am looking at the link failure here (search for "fatal") - https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_com... It only fails on win component build with not being able to see ui::NativeTheme::ExtraParams::ExtraParams() in the native_theme.dll when linking views.dll. I could not see immediately what is wrong here. Do I need to do anything special on Windows to mark exports (other than NATIVE_THEME_EXPORT)? Do you think it could be the union constructor support in MSVC? I might be missing something trivial but so far could not spot it. native_theme is included in dependencies in views.gyp - https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/views.gyp... On a chance that you can see what's wrong - Thanks!
https://codereview.chromium.org/1661673004/diff/260001/ui/native_theme/native... File ui/native_theme/native_theme.h (right): https://codereview.chromium.org/1661673004/diff/260001/ui/native_theme/native... ui/native_theme/native_theme.h:200: union NATIVE_THEME_EXPORT ExtraParams { pkasting, does this seem right to you (it seems to fix the Windows component build)? Shouldn't the parent class export modifier have taken care of this as on other platforms?
https://codereview.chromium.org/1661673004/diff/260001/ui/native_theme/native... File ui/native_theme/native_theme.h (right): https://codereview.chromium.org/1661673004/diff/260001/ui/native_theme/native... ui/native_theme/native_theme.h:200: union NATIVE_THEME_EXPORT ExtraParams { On 2016/02/18 07:13:37, varkha wrote: > pkasting, does this seem right to you (it seems to fix the Windows component > build)? Shouldn't the parent class export modifier have taken care of this as on > other platforms? I don't know, as I don't really understand the symbol export stuff. You may want to check with thakis or brucedawson, who know linker vagaries better.
varkha@chromium.org changed reviewers: + sky@chromium.org - sadrul@chromium.org
+sky@ for OWNERS in ui/views/controls/menu. -sadrul@
How about some test coverage? https://codereview.chromium.org/1661673004/diff/260001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1661673004/diff/260001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:121: CustomButton* OtherHotTrackedButton(View* view, CustomButton* hot_button) { GetFirstHotTrackedViewDifferingFrom (which better explains what this does). https://codereview.chromium.org/1661673004/diff/260001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:961: CustomButton* new_hot_button = OtherHotTrackedButton(menu_item, hot_button_); It isn't obvious to me why are you doing this. Please add a comment. https://codereview.chromium.org/1661673004/diff/260001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:2617: DCHECK(hot_button); What happens if hot_button is deleted while the menu is up? https://codereview.chromium.org/1661673004/diff/260001/ui/views/controls/menu... File ui/views/controls/menu/menu_scroll_view_container.cc (left): https://codereview.chromium.org/1661673004/diff/260001/ui/views/controls/menu... ui/views/controls/menu/menu_scroll_view_container.cc:79: extra.menu_item.is_selected = false; Why are you changing this?
> How about some test coverage? I've added a test that emulates hot tracking with both keyboard and mouse as well as a test for a child deletion while a menu is open. PTAL. https://codereview.chromium.org/1661673004/diff/260001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1661673004/diff/260001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:121: CustomButton* OtherHotTrackedButton(View* view, CustomButton* hot_button) { On 2016/02/19 01:07:14, sky wrote: > GetFirstHotTrackedViewDifferingFrom (which better explains what this does). Done. https://codereview.chromium.org/1661673004/diff/260001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:961: CustomButton* new_hot_button = OtherHotTrackedButton(menu_item, hot_button_); On 2016/02/19 01:07:14, sky wrote: > It isn't obvious to me why are you doing this. Please add a comment. Done. https://codereview.chromium.org/1661673004/diff/260001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:2617: DCHECK(hot_button); On 2016/02/19 01:07:14, sky wrote: > What happens if hot_button is deleted while the menu is up? You mean |hot_tracked_| (which can be passed in line 964 above). I have updated ViewHierarchyChanged handler to watch for this and have also added a test for this. I have tried deleting a button when some key is pressed while it is hot and sure enough I can reproduce a crash unless we watch for it. Thanks! https://codereview.chromium.org/1661673004/diff/260001/ui/views/controls/menu... File ui/views/controls/menu/menu_scroll_view_container.cc (left): https://codereview.chromium.org/1661673004/diff/260001/ui/views/controls/menu... ui/views/controls/menu/menu_scroll_view_container.cc:79: extra.menu_item.is_selected = false; On 2016/02/19 01:07:14, sky wrote: > Why are you changing this? Because ExtraParams now has a zero-initializing ctor so I thought default is correct in this case. I guess it is cleaner to pass an explicit value in this case so I reverted this change in the new patch.
Patchset #11 (id:280001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1661673004/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1661673004/300001
https://codereview.chromium.org/1661673004/diff/260001/ui/views/controls/menu... File ui/views/controls/menu/menu_scroll_view_container.cc (left): https://codereview.chromium.org/1661673004/diff/260001/ui/views/controls/menu... ui/views/controls/menu/menu_scroll_view_container.cc:79: extra.menu_item.is_selected = false; On 2016/02/23 00:41:05, varkha wrote: > On 2016/02/19 01:07:14, sky wrote: > > Why are you changing this? > > Because ExtraParams now has a zero-initializing ctor so I thought default is > correct in this case. I guess it is cleaner to pass an explicit value in this > case so I reverted this change in the new patch. Please put it back. It is indeed better to not specify initializers that are equivalent to the default value.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1661673004/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1661673004/320001
https://codereview.chromium.org/1661673004/diff/260001/ui/views/controls/menu... File ui/views/controls/menu/menu_scroll_view_container.cc (left): https://codereview.chromium.org/1661673004/diff/260001/ui/views/controls/menu... ui/views/controls/menu/menu_scroll_view_container.cc:79: extra.menu_item.is_selected = false; On 2016/02/23 02:22:56, Peter Kasting wrote: > On 2016/02/23 00:41:05, varkha wrote: > > On 2016/02/19 01:07:14, sky wrote: > > > Why are you changing this? > > > > Because ExtraParams now has a zero-initializing ctor so I thought default is > > correct in this case. I guess it is cleaner to pass an explicit value in this > > case so I reverted this change in the new patch. > > Please put it back. It is indeed better to not specify initializers that are > equivalent to the default value. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1661673004/diff/320001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu.cc (right): https://codereview.chromium.org/1661673004/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu.cc:220: view->GetClassName() == views::MenuButton::kViewClassName) { I'm not a fan of using the class name as a replacement for RTTI. If someone subclassed the view and returned a different name you'll incorrectly not paint things. At the time you create the InMenuButtonBackground don't you know if it's a type that requires this extra logic? https://codereview.chromium.org/1661673004/diff/320001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1661673004/diff/320001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:1062: // A button could become hot-tracked when a mouse is moved without changing I still don't get what case you are hitting that is requiring the change. What specific case are you fixing? https://codereview.chromium.org/1661673004/diff/320001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:2642: DCHECK(hot_button); The constraints on calling this function are not obvious and confusing. Can't this function take care of updating existing state so the call site doesn't need: if (hot_button_) SetHotTrackedButton(hot_button_, false); SetHotTrackedButton(new_hot_button, true); and instead can do: SetHotTrackedButton(new_hot_button, true) and it takes the necessary steps?
Will look at the items, thanks sky@! Just posting this to answer the second question. https://codereview.chromium.org/1661673004/diff/320001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu.cc (right): https://codereview.chromium.org/1661673004/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu.cc:220: view->GetClassName() == views::MenuButton::kViewClassName) { On 2016/02/23 20:11:13, sky wrote: > I'm not a fan of using the class name as a replacement for RTTI. If someone > subclassed the view and returned a different name you'll incorrectly not paint > things. At the time you create the InMenuButtonBackground don't you know if it's > a type that requires this extra logic? I will look if this is possible. https://codereview.chromium.org/1661673004/diff/320001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1661673004/diff/320001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:1062: // A button could become hot-tracked when a mouse is moved without changing On 2016/02/23 20:11:13, sky wrote: > I still don't get what case you are hitting that is requiring the change. What > specific case are you fixing? This scenario: 1. Open app menu with extension buttons overflow 2. Place mouse pointer over one of them (triggers hot-tracking) Call that "button1" 3. Move visible focus with a keyboard (down) to the next extension button (call that button2) 4. Move a mouse just enough to re-trigger a hot-tracked state on "button1". 5. Press [arrow down] key again. This should move to "button2" but moves instead to "button3" (since step 4 did not update the hot-tracked state in menu controller and the last updated state in MenuController refers to step 3). This is not affecting just the extension overflow but any menu items with sub-views such as zoom panel or copy-cut-paste. This may result in multiple hot-tracked visually active views as well. https://codereview.chromium.org/1661673004/diff/320001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:2642: DCHECK(hot_button); On 2016/02/23 20:11:13, sky wrote: > The constraints on calling this function are not obvious and confusing. Can't > this function take care of updating existing state so the call site doesn't > need: > if (hot_button_) > SetHotTrackedButton(hot_button_, false); > SetHotTrackedButton(new_hot_button, true); > and instead can do: > SetHotTrackedButton(new_hot_button, true) and it takes the necessary steps? Yes, will try that.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1661673004/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1661673004/340001
PTAL. Thanks for the comments! https://codereview.chromium.org/1661673004/diff/320001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu.cc (right): https://codereview.chromium.org/1661673004/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu.cc:220: view->GetClassName() == views::MenuButton::kViewClassName) { On 2016/02/23 21:01:39, varkha wrote: > On 2016/02/23 20:11:13, sky wrote: > > I'm not a fan of using the class name as a replacement for RTTI. If someone > > subclassed the view and returned a different name you'll incorrectly not paint > > things. At the time you create the InMenuButtonBackground don't you know if > it's > > a type that requires this extra logic? > > I will look if this is possible. Done. https://codereview.chromium.org/1661673004/diff/320001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1661673004/diff/320001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:2642: DCHECK(hot_button); On 2016/02/23 21:01:39, varkha wrote: > On 2016/02/23 20:11:13, sky wrote: > > The constraints on calling this function are not obvious and confusing. Can't > > this function take care of updating existing state so the call site doesn't > > need: > > if (hot_button_) > > SetHotTrackedButton(hot_button_, false); > > SetHotTrackedButton(new_hot_button, true); > > and instead can do: > > SetHotTrackedButton(new_hot_button, true) and it takes the necessary steps? > > Yes, will try that. Done.
Some comment suggestsion https://codereview.chromium.org/1661673004/diff/340001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu.cc (right): https://codereview.chromium.org/1661673004/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu.cc:139: ROUNDED_BUTTON, Nit: These probably deserve comments, as it's not immediately clear how e.g. SINGLE_BUTTON and ROUNDED_BUTTON differ, or why four of these seem to talk about positioning but the one new one seems to talk instead about shape. https://codereview.chromium.org/1661673004/diff/340001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1661673004/diff/340001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:1065: // This allows correct hot-tracked button to be used when alternating between Nit: correct -> the correct https://codereview.chromium.org/1661673004/diff/340001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:2645: CustomButton* hot_button = CustomButton::AsCustomButton(hot_view); Nit: Inline into next line https://codereview.chromium.org/1661673004/diff/340001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.h (right): https://codereview.chromium.org/1661673004/diff/340001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.h:501: // The menu is also canceled dependent on the target of the event. The event Nit: is also canceled dependent -> may also be canceled depending ? https://codereview.chromium.org/1661673004/diff/340001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.h:502: // is then reprocessed to cause its result if the menu had not been present. Nit: The event is then reprocessed to cause its result if the menu had not been present. -> |event| is then processed without the menu present. ?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1661673004/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1661673004/360001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #14 (id:360001) has been deleted
Patchset #14 (id:380001) has been deleted
Patchset #14 (id:400001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
https://codereview.chromium.org/1661673004/diff/340001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu.cc (right): https://codereview.chromium.org/1661673004/diff/340001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu.cc:139: ROUNDED_BUTTON, On 2016/02/24 02:04:48, Peter Kasting wrote: > Nit: These probably deserve comments, as it's not immediately clear how e.g. > SINGLE_BUTTON and ROUNDED_BUTTON differ, or why four of these seem to talk about > positioning but the one new one seems to talk instead about shape. Done. https://codereview.chromium.org/1661673004/diff/340001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1661673004/diff/340001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:1065: // This allows correct hot-tracked button to be used when alternating between On 2016/02/24 02:04:48, Peter Kasting wrote: > Nit: correct -> the correct Done. https://codereview.chromium.org/1661673004/diff/340001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:2645: CustomButton* hot_button = CustomButton::AsCustomButton(hot_view); On 2016/02/24 02:04:48, Peter Kasting wrote: > Nit: Inline into next line Done. https://codereview.chromium.org/1661673004/diff/340001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.h (right): https://codereview.chromium.org/1661673004/diff/340001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.h:501: // The menu is also canceled dependent on the target of the event. The event On 2016/02/24 02:04:49, Peter Kasting wrote: > Nit: is also canceled dependent -> may also be canceled depending ? Done. https://codereview.chromium.org/1661673004/diff/340001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.h:502: // is then reprocessed to cause its result if the menu had not been present. On 2016/02/24 02:04:49, Peter Kasting wrote: > Nit: The event is then reprocessed to cause its result if the menu had not been > present. -> |event| is then processed without the menu present. ? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1661673004/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1661673004/420001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Patchset #14 (id:420001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1661673004/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1661673004/440001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On Tue, Feb 23, 2016 at 1:01 PM, <varkha@chromium.org> wrote: > Will look at the items, thanks sky@! Just posting this to answer the second > question. > > > https://codereview.chromium.org/1661673004/diff/320001/chrome/browser/ui/view... > File chrome/browser/ui/views/toolbar/app_menu.cc (right): > > https://codereview.chromium.org/1661673004/diff/320001/chrome/browser/ui/view... > chrome/browser/ui/views/toolbar/app_menu.cc:220: view->GetClassName() == > views::MenuButton::kViewClassName) { > On 2016/02/23 20:11:13, sky wrote: >> I'm not a fan of using the class name as a replacement for RTTI. If > someone >> subclassed the view and returned a different name you'll incorrectly > not paint >> things. At the time you create the InMenuButtonBackground don't you > know if it's >> a type that requires this extra logic? > > I will look if this is possible. > > https://codereview.chromium.org/1661673004/diff/320001/ui/views/controls/menu... > File ui/views/controls/menu/menu_controller.cc (right): > > https://codereview.chromium.org/1661673004/diff/320001/ui/views/controls/menu... > ui/views/controls/menu/menu_controller.cc:1062: // A button could become > hot-tracked when a mouse is moved without changing > On 2016/02/23 20:11:13, sky wrote: >> I still don't get what case you are hitting that is requiring the > change. What >> specific case are you fixing? > > This scenario: > 1. Open app menu with extension buttons overflow > 2. Place mouse pointer over one of them (triggers hot-tracking) > Call that "button1" > 3. Move visible focus with a keyboard (down) to the next extension > button (call that button2) > 4. Move a mouse just enough to re-trigger a hot-tracked state on > "button1". > 5. Press [arrow down] key again. > > This should move to "button2" but moves instead to "button3" (since step > 4 did not update the hot-tracked state in menu controller and the last > updated state in MenuController refers to step 3). Thanks. Now I get what you are trying to fix. I think the bug is in step 4 when the mouse is moved the selection doesn't change accordingly. I think your fix is after the fact. You'll likely need to update the mouse move code. -Scott > > This is not affecting just the extension overflow but any menu items > with sub-views such as zoom panel or copy-cut-paste. This may result in > multiple hot-tracked visually active views as well. > > https://codereview.chromium.org/1661673004/diff/320001/ui/views/controls/menu... > ui/views/controls/menu/menu_controller.cc:2642: DCHECK(hot_button); > On 2016/02/23 20:11:13, sky wrote: >> The constraints on calling this function are not obvious and > confusing. Can't >> this function take care of updating existing state so the call site > doesn't >> need: >> if (hot_button_) >> SetHotTrackedButton(hot_button_, false); >> SetHotTrackedButton(new_hot_button, true); >> and instead can do: >> SetHotTrackedButton(new_hot_button, true) and it takes the necessary > steps? > > Yes, will try that. > > https://codereview.chromium.org/1661673004/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> Thanks. Now I get what you are trying to fix. > I think the bug is in step 4 when the mouse is moved the selection > doesn't change accordingly. I think your fix is after the fact. You'll > likely need to update the mouse move code. > > -Scott Right. What do you think would be more promising: 1. Add controller as an observer to the CustomButton children for state changes. I think that would be most reliable and straightforward since it will stay in sync with the button state changes. I could add / remove an observer in hierarchy changes notifications. 2. Update MenuController::HandleMouseLocation() (or MenuController::OnMouseMoved() that calls it) to keep track of hit testing and a child view under the cursor 3. Use some other existing mechanism that maybe you can think of?
On Wed, Feb 24, 2016 at 11:04 AM, <varkha@chromium.org> wrote: >> Thanks. Now I get what you are trying to fix. >> I think the bug is in step 4 when the mouse is moved the selection >> doesn't change accordingly. I think your fix is after the fact. You'll >> likely need to update the mouse move code. >> >> -Scott > > Right. What do you think would be more promising: > 1. Add controller as an observer to the CustomButton children for state > changes. > I think that would be most reliable and straightforward since it will stay > in > sync with the button state changes. I could add / remove an observer in > hierarchy changes notifications. Buttons aren't observable at the current time, so this would require some plumbing. > 2. Update MenuController::HandleMouseLocation() (or > MenuController::OnMouseMoved() that calls it) to keep track of hit testing > and a > child view under the cursor This is what I was thinking. -Scott > 3. Use some other existing mechanism that maybe you can think of? > > https://codereview.chromium.org/1661673004/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #15 (id:460001) has been deleted
sky@, I have added plumbing for observing button state, PTAL. I could not convince myself that I could ensure state sync with the hit-testing since it is up to CustomButton descendant to implement HOVERED state in any way so it is incorrect to assume that a view that gets mouse pointer in it will be hot-tracked. There are also other ways how a button could gain HOVERED state - visibility changes, gestures, etc. So this would be ultimately much more complicated and in some corner case incorrect.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1661673004/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1661673004/480001
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 varkha@chromium.org to run a CQ dry run
sky@ and I have chatted offline and I have now separated the bulk of the changes to MenuController and post it separately. PTAL. Thanks!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1661673004/500001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1661673004/500001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1661673004/520001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1661673004/520001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sky@, ping, the changes in ui/views/ are now just the setting of hot-tracked view correctly on the first item from the top in addition to doing it on subsequent items.
LGTM https://codereview.chromium.org/1661673004/diff/520001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1661673004/diff/520001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:2613: CustomButton* button_hot = CustomButton::AsCustomButton(hot_view); nit: hot_button to match hot_view.
https://codereview.chromium.org/1661673004/diff/520001/ui/views/controls/menu... File ui/views/controls/menu/menu_controller.cc (right): https://codereview.chromium.org/1661673004/diff/520001/ui/views/controls/menu... ui/views/controls/menu/menu_controller.cc:2613: CustomButton* button_hot = CustomButton::AsCustomButton(hot_view); On 2016/02/26 20:51:04, sky wrote: > nit: hot_button to match hot_view. Done.
The CQ bit was checked by varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1661673004/#ps540001 (title: "Restores hot-tracking of extension buttons in app menu with MD (nit)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1661673004/540001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1661673004/540001
Message was sent while issue was closed.
Committed patchset #18 (id:540001)
Message was sent while issue was closed.
Description was changed from ========== Enables hot-tracking for overflow extension buttons in the app menu BUG=583559 ========== to ========== Enables hot-tracking for overflow extension buttons in the app menu BUG=583559 Committed: https://crrev.com/ef6637ffae36b30abf91c6cd78dcd2fa7a3f59b5 Cr-Commit-Position: refs/heads/master@{#377999} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/ef6637ffae36b30abf91c6cd78dcd2fa7a3f59b5 Cr-Commit-Position: refs/heads/master@{#377999}
Message was sent while issue was closed.
estade@chromium.org changed reviewers: + estade@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1661673004/diff/540001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1661673004/diff/540001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:130: return !delegate_->ShownInsideMenu() && This seems contrary to what you wrote on the bug: "mouse hot-tracking should produce immediate feedback".
Message was sent while issue was closed.
On 2016/03/01 00:00:08, Evan Stade wrote: > https://codereview.chromium.org/1661673004/diff/540001/chrome/browser/ui/view... > File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): > > https://codereview.chromium.org/1661673004/diff/540001/chrome/browser/ui/view... > chrome/browser/ui/views/toolbar/toolbar_action_view.cc:130: return > !delegate_->ShownInsideMenu() && > This seems contrary to what you wrote on the bug: "mouse hot-tracking should > produce immediate feedback". nevermind, I guess I see how this works --- it was actually broken because of https://bugs.chromium.org/p/chromium/issues/detail?id=590452
Message was sent while issue was closed.
https://codereview.chromium.org/1661673004/diff/540001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right): https://codereview.chromium.org/1661673004/diff/540001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/toolbar_action_view.cc:130: return !delegate_->ShownInsideMenu() && On 2016/03/01 00:00:08, Evan Stade wrote: > This seems contrary to what you wrote on the bug: "mouse hot-tracking should > produce immediate feedback". Yes, the immediate feedback is produced by a background created in app_menu. This check here just disables the "other" hover that works on mouse enter / exit with an animation. |
