Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2889293002:
Filter run loop modes in message_pump_mac.mm (Closed)
|
Created:
3 years, 7 months ago by tapted Modified:
3 years, 7 months ago Reviewers:
Mark Mentovai CC:
chromium-reviews, danakj+watch_chromium.org, mac-reviews_chromium.org, sadrul, vmpstr+watch_chromium.org, chrome-apps-syd-reviews_chromium.org, Robert Sesek Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFilter run loop modes in message_pump_mac.mm
There's a non-trivial overhead for the machinery to run posted tasks in
a given run loop mode. Only observe modes known to be run for a particular
thread.
BUG=640466, 724076
Review-Url: https://codereview.chromium.org/2889293002
Cr-Commit-Position: refs/heads/master@{#473391}
Committed: https://chromium.googlesource.com/chromium/src/+/80575da0e067c7c941edfd106287dd58ed578f8f
Patch Set 1 #Patch Set 2 : selfnits #
Total comments: 6
Patch Set 3 : respond to comments, sort methods #
Messages
Total messages: 22 (16 generated)
The CQ bit was checked by tapted@chromium.org 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 tapted@chromium.org 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...
tapted@chromium.org changed reviewers: + mark@chromium.org
Hi Mark, please take a look +robert CC if you have any comments too. Thanks!
https://codereview.chromium.org/2889293002/diff/20001/base/message_loop/messa... File base/message_loop/message_pump_mac.h (right): https://codereview.chromium.org/2889293002/diff/20001/base/message_loop/messa... base/message_loop/message_pump_mac.h:87: explicit MessagePumpCFRunLoopBase(int mode_mask); Since you aren’t providing the enum that has masky bits out in this file, it’d be nice if you documented the kinds of values that it could receive. https://codereview.chromium.org/2889293002/diff/20001/base/message_loop/messa... base/message_loop/message_pump_mac.h:119: void InvokeForAllModes(void method(CFRunLoopRef, Argument, CFStringRef), Since it’s restricted to mode_mask_, it shouldn’t advertise “all” modes. Enabled modes? https://codereview.chromium.org/2889293002/diff/20001/base/message_loop/messa... base/message_loop/message_pump_mac.h:119: void InvokeForAllModes(void method(CFRunLoopRef, Argument, CFStringRef), More a function than a method?
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 tapted@chromium.org 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...
Sorry for the ugly diff.. the messed up definition order finally got to me https://codereview.chromium.org/2889293002/diff/20001/base/message_loop/messa... File base/message_loop/message_pump_mac.h (right): https://codereview.chromium.org/2889293002/diff/20001/base/message_loop/messa... base/message_loop/message_pump_mac.h:87: explicit MessagePumpCFRunLoopBase(int mode_mask); On 2017/05/19 03:29:13, Mark Mentovai wrote: > Since you aren’t providing the enum that has masky bits out in this file, it’d > be nice if you documented the kinds of values that it could receive. Done. Made this constructor protected also and fixed up the method ordering. https://codereview.chromium.org/2889293002/diff/20001/base/message_loop/messa... base/message_loop/message_pump_mac.h:119: void InvokeForAllModes(void method(CFRunLoopRef, Argument, CFStringRef), On 2017/05/19 03:29:13, Mark Mentovai wrote: > More a function than a method? Done. https://codereview.chromium.org/2889293002/diff/20001/base/message_loop/messa... base/message_loop/message_pump_mac.h:119: void InvokeForAllModes(void method(CFRunLoopRef, Argument, CFStringRef), On 2017/05/19 03:29:13, Mark Mentovai wrote: > Since it’s restricted to mode_mask_, it shouldn’t advertise “all” modes. Enabled > modes? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
The CQ bit was checked by tapted@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": 40001, "attempt_start_ts": 1495238363729930,
"parent_rev": "bd5768e5f739762df9dbb9201f9aac7c89e0625d", "commit_rev":
"80575da0e067c7c941edfd106287dd58ed578f8f"}
Message was sent while issue was closed.
Description was changed from ========== Filter run loop modes in message_pump_mac.mm There's a non-trivial overhead for the machinery to run posted tasks in a given run loop mode. Only observe modes known to be run for a particular thread. BUG=640466, 724076 ========== to ========== Filter run loop modes in message_pump_mac.mm There's a non-trivial overhead for the machinery to run posted tasks in a given run loop mode. Only observe modes known to be run for a particular thread. BUG=640466, 724076 Review-Url: https://codereview.chromium.org/2889293002 Cr-Commit-Position: refs/heads/master@{#473391} Committed: https://chromium.googlesource.com/chromium/src/+/80575da0e067c7c941edfd106287... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/80575da0e067c7c941edfd106287... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
