Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2620203004:
MenuRunner: Add comment to specify blocking behavior on MacViews. (Closed)
|
Created:
3 years, 11 months ago by karandeepb Modified:
3 years, 11 months ago Reviewers:
tapted CC:
chromium-reviews, tfarina, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMenuRunner: Add comment to specify blocking behavior on MacViews.
On MacViews, the COMBOBOX and CONTEXT_MENU menu run types are always shown using
a native NSMenu which runs the run loop in NSEventTrackingRunLoopMode and
blocks. Hence the ASYNC run type is ignored for these menu run types on
MacViews. Add comment to that effect.
BUG=682544
Review-Url: https://codereview.chromium.org/2620203004
Cr-Commit-Position: refs/heads/master@{#445014}
Committed: https://chromium.googlesource.com/chromium/src/+/454b8e5ae37b38fc67695e8a149dbe4bc982f3de
Patch Set 1 #
Total comments: 3
Patch Set 2 : Address review. #Messages
Total messages: 18 (8 generated)
Description was changed from ========== -- BUG= ========== to ========== MenuRunner: Add comment to specify non-blocking behavior on MacViews. On MacViews, the COMBOBOX and CONTEXT_MENU menu run types are always shown using a native NSMenu which runs a nested message loop and blocks. Hence the ASYNC run type is ignored for these menu run types on MacViews. Add comment to that effect. BUG=none ==========
karandeepb@chromium.org changed reviewers: + tapted@chromium.org
PTAL Trent.
Description was changed from ========== MenuRunner: Add comment to specify non-blocking behavior on MacViews. On MacViews, the COMBOBOX and CONTEXT_MENU menu run types are always shown using a native NSMenu which runs a nested message loop and blocks. Hence the ASYNC run type is ignored for these menu run types on MacViews. Add comment to that effect. BUG=none ========== to ========== MenuRunner: Add comment to specify blocking behavior on MacViews. On MacViews, the COMBOBOX and CONTEXT_MENU menu run types are always shown using a native NSMenu which runs a nested message loop and blocks. Hence the ASYNC run type is ignored for these menu run types on MacViews. Add comment to that effect. BUG=none ==========
hm - I'm not sure we need this. Ideally we make ASYNC work (e.g. by getting the views menu look and feel on Mac indistinguishable from native menus -- there is NSVisualEffectMaterialMenu which could make this possible). And there is actually a runloop while the menu is showing, but it has a RunLoopMode filter on it so only particular events in the queue get processed. So it's technically not "blocking", since there's a nested runloop. Trying to explain this in a comment could get really confusing. But we could also tweak things so that more events are run in that nested run loop (but then stacks get weird, and it might be more confusing). Need to think on it some more - perhaps we just need a crbug for now so that we can point devs to documentation about this.
On 2017/01/11 15:23:07, tapted wrote: > hm - I'm not sure we need this. Ideally we make ASYNC work (e.g. by getting the > views menu look and feel on Mac indistinguishable from native menus -- there is > NSVisualEffectMaterialMenu which could make this possible). > > And there is actually a runloop while the menu is showing, but it has a > RunLoopMode filter on it so only particular events in the queue get processed. > So it's technically not "blocking", since there's a nested runloop. Trying to > explain this in a comment could get really confusing. > > But we could also tweak things so that more events are run in that nested run > loop (but then stacks get weird, and it might be more confusing). > > Need to think on it some more - perhaps we just need a crbug for now so that we > can point devs to documentation about this. Yeah I think we need something, since COMBOBOX|ASYNC or CONTEXT_MENU|ASYNC blocks (in the traditional sense) on Mac, which is different to the expectation. Hence we must clarify the expectation. I am fine with filing a bug (to make ASYNC work on MacViews, although seems that is complicated) and referring to it in the comment.
On 2017/01/11 23:48:47, karandeepb wrote: > On 2017/01/11 15:23:07, tapted wrote: > > hm - I'm not sure we need this. Ideally we make ASYNC work (e.g. by getting > the > > views menu look and feel on Mac indistinguishable from native menus -- there > is > > NSVisualEffectMaterialMenu which could make this possible). > > > > And there is actually a runloop while the menu is showing, but it has a > > RunLoopMode filter on it so only particular events in the queue get processed. > > So it's technically not "blocking", since there's a nested runloop. Trying to > > explain this in a comment could get really confusing. > > > > But we could also tweak things so that more events are run in that nested run > > loop (but then stacks get weird, and it might be more confusing). > > > > Need to think on it some more - perhaps we just need a crbug for now so that > we > > can point devs to documentation about this. > > Yeah I think we need something, since COMBOBOX|ASYNC or CONTEXT_MENU|ASYNC > blocks (in the traditional sense) on Mac, which is different to the expectation. > Hence we must clarify the expectation. I am fine with filing a bug (to make > ASYNC work on MacViews, although seems that is complicated) and referring to it > in the comment. PTAL Trent.
Still, it's uncommon for implementation details to be documented in header comments -- they easily get out of date, e.g., when bugs are fixed. But citing a BUG= means we can have a more dynamic errata -- see below. https://codereview.chromium.org/2620203004/diff/1/ui/views/controls/menu/menu... File ui/views/controls/menu/menu_runner.h (right): https://codereview.chromium.org/2620203004/diff/1/ui/views/controls/menu/menu... ui/views/controls/menu/menu_runner.h:112: // Creates a new MenuRunner, which may use a native menu if available. This comment already mentions native menus. So perhaps a note here is best. Like Note that with a native menu (e.g. on Mac), the ASYNC flag in |run_types| may be ignored. See http://crbug.com/XXXXXX https://codereview.chromium.org/2620203004/diff/1/ui/views/controls/menu/menu... ui/views/controls/menu/menu_runner.h:120: MenuRunner(MenuItemView* menu, int32_t run_types); So I think using this constructor, run_types on Mac will work correctly - so documenting the enum probably is misleading.
Description was changed from ========== MenuRunner: Add comment to specify blocking behavior on MacViews. On MacViews, the COMBOBOX and CONTEXT_MENU menu run types are always shown using a native NSMenu which runs a nested message loop and blocks. Hence the ASYNC run type is ignored for these menu run types on MacViews. Add comment to that effect. BUG=none ========== to ========== MenuRunner: Add comment to specify blocking behavior on MacViews. On MacViews, the COMBOBOX and CONTEXT_MENU menu run types are always shown using a native NSMenu which runs the run loop in NSEventTrackingRunLoopMode and blocks. Hence the ASYNC run type is ignored for these menu run types on MacViews. Add comment to that effect. BUG=none ==========
Description was changed from ========== MenuRunner: Add comment to specify blocking behavior on MacViews. On MacViews, the COMBOBOX and CONTEXT_MENU menu run types are always shown using a native NSMenu which runs the run loop in NSEventTrackingRunLoopMode and blocks. Hence the ASYNC run type is ignored for these menu run types on MacViews. Add comment to that effect. BUG=none ========== to ========== MenuRunner: Add comment to specify blocking behavior on MacViews. On MacViews, the COMBOBOX and CONTEXT_MENU menu run types are always shown using a native NSMenu which runs the run loop in NSEventTrackingRunLoopMode and blocks. Hence the ASYNC run type is ignored for these menu run types on MacViews. Add comment to that effect. BUG=682544 ==========
https://codereview.chromium.org/2620203004/diff/1/ui/views/controls/menu/menu... File ui/views/controls/menu/menu_runner.h (right): https://codereview.chromium.org/2620203004/diff/1/ui/views/controls/menu/menu... ui/views/controls/menu/menu_runner.h:120: MenuRunner(MenuItemView* menu, int32_t run_types); On 2017/01/17 21:55:20, tapted wrote: > So I think using this constructor, run_types on Mac will work correctly - so > documenting the enum probably is misleading. Oh yeah, you are correct. Did not see this constructor.
PTAL.
lgtm - thanks!
The CQ bit was checked by karandeepb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484895314248710,
"parent_rev": "a3f0f3b933bfda2b1a2599e9e2172f8dd083436b", "commit_rev":
"454b8e5ae37b38fc67695e8a149dbe4bc982f3de"}
Message was sent while issue was closed.
Description was changed from ========== MenuRunner: Add comment to specify blocking behavior on MacViews. On MacViews, the COMBOBOX and CONTEXT_MENU menu run types are always shown using a native NSMenu which runs the run loop in NSEventTrackingRunLoopMode and blocks. Hence the ASYNC run type is ignored for these menu run types on MacViews. Add comment to that effect. BUG=682544 ========== to ========== MenuRunner: Add comment to specify blocking behavior on MacViews. On MacViews, the COMBOBOX and CONTEXT_MENU menu run types are always shown using a native NSMenu which runs the run loop in NSEventTrackingRunLoopMode and blocks. Hence the ASYNC run type is ignored for these menu run types on MacViews. Add comment to that effect. BUG=682544 Review-Url: https://codereview.chromium.org/2620203004 Cr-Commit-Position: refs/heads/master@{#445014} Committed: https://chromium.googlesource.com/chromium/src/+/454b8e5ae37b38fc67695e8a149d... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/454b8e5ae37b38fc67695e8a149d... |
