Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2765583002:
Teach main thread event queue about closures. (Closed)
DescriptionTeach main thread event queue about closures.
This allows the rAF signal to execute all pending input right away
instead of waiting for a message pump to process any input IPCs.
This simplifies the state that we don't need to execute a post task
for every event we have to process. At delivery time we can just take
them all from the queue (except for the rAF aligned events) and execute
them.
BUG=674301
TBR=creis@chromium.org
Review-Url: https://codereview.chromium.org/2765583002
Cr-Commit-Position: refs/heads/master@{#461133}
Committed: https://chromium.googlesource.com/chromium/src/+/8ba8d7142ef4449775d3d75b6f17b80d24d07f77
Patch Set 1 #
Total comments: 27
Patch Set 2 : Rebase #Patch Set 3 : Rename WebInputEventQueue so that it is cleaner #Patch Set 4 : Fix build #
Total comments: 12
Patch Set 5 : Fix render frames using a different main thread event queue than render views #Patch Set 6 : Rebase #Patch Set 7 : Fix Mac issue #
Total comments: 6
Patch Set 8 : Address comments #
Total comments: 9
Patch Set 9 : Add comments #
Total comments: 8
Patch Set 10 : Remove two virtuals #
Total comments: 1
Patch Set 11 : Fix typo #Dependent Patchsets: Messages
Total messages: 63 (43 generated)
The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run
Description was changed from ========== Teach main thread event queue about closures. This allows the rAF signal to execute all pending input right away instead of waiting for a message pump to process any input IPCs. This simplifies the state that we don't need to execute a post task for every event we have to process. At delivery time we can just take them all from the queue (except for the rAF aligned events) and execute them. BUG=674301 ========== to ========== Teach main thread event queue about closures. This allows the rAF signal to execute all pending input right away instead of waiting for a message pump to process any input IPCs. This simplifies the state that we don't need to execute a post task for every event we have to process. At delivery time we can just take them all from the queue (except for the rAF aligned events) and execute them. BUG=674301 ==========
dtapuska@chromium.org changed reviewers: + mustaq@chromium.org
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
On 2017/03/20 20:18:09, dtapuska wrote: ping
I will take another look at the dispatch-to-main code. I will sent my current comments to unblock you and to possibly correct my understanding. https://codereview.chromium.org/2765583002/diff/1/content/common/input/web_in... File content/common/input/web_input_event_queue.h (right): https://codereview.chromium.org/2765583002/diff/1/content/common/input/web_in... content/common/input/web_input_event_queue.h:5: #ifndef CONTENT_COMMON_INPUT_WEB_INPUT_EVENT_QUEUE_H_ Should we move this source file to content/renderer/input? https://codereview.chromium.org/2765583002/diff/1/content/common/input/web_in... content/common/input/web_input_event_queue.h:42: std::unique_ptr<T> Pop() { Nit: make it |pop()| to be consistent? https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... File content/renderer/input/main_thread_event_queue.cc (right): https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... content/renderer/input/main_thread_event_queue.cc:35: bool IsEvent() const override { return false; } I think it would be unwise to consider QueuedClosure as a non-event: symantically they are events, and treating them as non-events would make all these misnomers: |main_thread_EVENT_queue|, |web_input_EVENT_queue|, |same_EVENT_class|, |EVENThandled|..., plus UMA labels! I suggest this instead: |MainThreadQueueItem| would have two subclasses: |QueuedEventClosure|, |QueuedWebInputEvent|, and use the member function |IsWebInputEvent| to distinguish between them. Does it sound reasonable? https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... content/renderer/input/main_thread_event_queue.cc:462: void MainThreadEventQueue::SendEventNotificationToMainThread() { Since there is really no notification here, may be call it |DispatchEventsToMainThread|? https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... content/renderer/input/main_thread_event_queue.cc:471: { I will take a look at this block again. https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... content/renderer/input/main_thread_event_queue.cc:492: } else if (size_before > 0) { I think we never expect size_before == 0 here right? Please add a dcheck then. https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... File content/renderer/input/main_thread_event_queue.h (right): https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... content/renderer/input/main_thread_event_queue.h:25: class QueuedItem { Since this is an exposed class, I would prefer a more meaningful/focused name than a generic |QueuedItem|. Is |MainThreadEventQueueItem| too long? Or simply |MainThreadQueueItem|? https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... content/renderer/input/main_thread_event_queue.h:135: bool IsRafAlignedEvent(const std::unique_ptr<QueuedItem>& item) const; This seems to belong to the class |QueuedItem| (|MainThreadQueueItem|)? https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... content/renderer/input/main_thread_event_queue.h:156: bool sent_post_task_; Could you please clarify what "sent" means here, through a comment perhaps? The time-span ("sent" since which point in time) is not clear to me. https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... File content/renderer/input/main_thread_event_queue_unittest.cc (right): https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... content/renderer/input/main_thread_event_queue_unittest.cc:452: // Simulate enqueing a discrete event, followed by continuous events and "Discrete" vs "continuous": do we define these terms in some raf-related header file at least? Otherwise they won't mean anything to a stranger to the raf world. https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... content/renderer/input/main_thread_event_queue_unittest.cc:485: HandleEvent(wheelEvents[2], INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING); Wondering why wheelEvent[2] not [1]? Event |modifier|s don't affect dispatches, am I right? https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... content/renderer/input/main_thread_event_queue_unittest.cc:495: handled_events_.at(2).event().modifiers()); Please add a longer raf/non-raf queued sequence, to show that a queue=[non-raf, raf, non-raf, raf] after RunUntilIdle() retains only the last (raf) event and dispatches the middle one. https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... content/renderer/input/main_thread_event_queue_unittest.cc:538: // Simulate event consumption but no rAF signal. The mouse wheel events s/mouse wheel events/touchmove event/.
The CQ bit was checked by dtapuska@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...
https://codereview.chromium.org/2765583002/diff/1/content/common/input/web_in... File content/common/input/web_input_event_queue.h (right): https://codereview.chromium.org/2765583002/diff/1/content/common/input/web_in... content/common/input/web_input_event_queue.h:5: #ifndef CONTENT_COMMON_INPUT_WEB_INPUT_EVENT_QUEUE_H_ On 2017/03/24 15:31:18, mustaq wrote: > Should we move this source file to content/renderer/input? Hmm; ya I could clean this up. https://codereview.chromium.org/2765583002/diff/1/content/common/input/web_in... content/common/input/web_input_event_queue.h:42: std::unique_ptr<T> Pop() { On 2017/03/24 15:31:18, mustaq wrote: > Nit: make it |pop()| to be consistent? Not done because I've made this no longer a templated class and it has a non-trivial implementation so it has a capital according to the style guide. https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... File content/renderer/input/main_thread_event_queue.cc (right): https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... content/renderer/input/main_thread_event_queue.cc:35: bool IsEvent() const override { return false; } On 2017/03/24 15:31:18, mustaq wrote: > I think it would be unwise to consider QueuedClosure as a non-event: > symantically they are events, and treating them as non-events would make all > these misnomers: |main_thread_EVENT_queue|, |web_input_EVENT_queue|, > |same_EVENT_class|, |EVENThandled|..., plus UMA labels! > > I suggest this instead: |MainThreadQueueItem| would have two subclasses: > |QueuedEventClosure|, |QueuedWebInputEvent|, and use the member function > |IsWebInputEvent| to distinguish between them. Does it sound reasonable? How about a QueuedClosure and a QueuedWebInputEvent... And change IsEvent to IsWebInputEvent and get rid of the IsSameEventClass... https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... content/renderer/input/main_thread_event_queue.cc:462: void MainThreadEventQueue::SendEventNotificationToMainThread() { On 2017/03/24 15:31:18, mustaq wrote: > Since there is really no notification here, may be call it > |DispatchEventsToMainThread|? That would mean that we are somehow transferring them. But I've renamed it to PostTaskToMainThread() https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... content/renderer/input/main_thread_event_queue.cc:471: { On 2017/03/24 15:31:18, mustaq wrote: > I will take a look at this block again. Acknowledged. https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... content/renderer/input/main_thread_event_queue.cc:492: } else if (size_before > 0) { On 2017/03/24 15:31:18, mustaq wrote: > I think we never expect size_before == 0 here right? Please add a dcheck then. Well the elseif condition is size_before > 0 so why would we add a dcheck? https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... File content/renderer/input/main_thread_event_queue.h (right): https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... content/renderer/input/main_thread_event_queue.h:25: class QueuedItem { On 2017/03/24 15:31:18, mustaq wrote: > Since this is an exposed class, I would prefer a more meaningful/focused name > than a generic |QueuedItem|. Is |MainThreadEventQueueItem| too long? Or simply > |MainThreadQueueItem|? Done. https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... content/renderer/input/main_thread_event_queue.h:135: bool IsRafAlignedEvent(const std::unique_ptr<QueuedItem>& item) const; On 2017/03/24 15:31:18, mustaq wrote: > This seems to belong to the class |QueuedItem| (|MainThreadQueueItem|)? I agree. But it needs to check the variables stored on this class. How do you think we should fetch the state variables? https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... content/renderer/input/main_thread_event_queue.h:156: bool sent_post_task_; On 2017/03/24 15:31:18, mustaq wrote: > Could you please clarify what "sent" means here, through a comment perhaps? The > time-span ("sent" since which point in time) is not clear to me. Done. https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... File content/renderer/input/main_thread_event_queue_unittest.cc (right): https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... content/renderer/input/main_thread_event_queue_unittest.cc:452: // Simulate enqueing a discrete event, followed by continuous events and On 2017/03/24 15:31:18, mustaq wrote: > "Discrete" vs "continuous": do we define these terms in some raf-related header > file at least? Otherwise they won't mean anything to a stranger to the raf > world. Continuous are rAF aligned events. Discrete aren't. https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... content/renderer/input/main_thread_event_queue_unittest.cc:485: HandleEvent(wheelEvents[2], INPUT_EVENT_ACK_STATE_SET_NON_BLOCKING); On 2017/03/24 15:31:18, mustaq wrote: > Wondering why wheelEvent[2] not [1]? Event |modifier|s don't affect dispatches, > am I right? Because 0 and 1 will get coalesced together. The modifiers forces them not to be coalesced. https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... content/renderer/input/main_thread_event_queue_unittest.cc:495: handled_events_.at(2).event().modifiers()); On 2017/03/24 15:31:18, mustaq wrote: > Please add a longer raf/non-raf queued sequence, to show that a queue=[non-raf, > raf, non-raf, raf] after RunUntilIdle() retains only the last (raf) event and > dispatches the middle one. Done. https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... content/renderer/input/main_thread_event_queue_unittest.cc:538: // Simulate event consumption but no rAF signal. The mouse wheel events On 2017/03/24 15:31:18, mustaq wrote: > s/mouse wheel events/touchmove event/. 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_chromeos_ozone_rel_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_...) linux_chromium_tsan_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 dtapuska@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: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Thanks for clarifying the class names, it's very clean now. Few more comments, feel free to defer until you have a clue for the Android key reversal. https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... File content/renderer/input/main_thread_event_queue.cc (right): https://codereview.chromium.org/2765583002/diff/1/content/renderer/input/main... content/renderer/input/main_thread_event_queue.cc:492: } else if (size_before > 0) { On 2017/03/24 20:16:31, dtapuska wrote: > On 2017/03/24 15:31:18, mustaq wrote: > > I think we never expect size_before == 0 here right? Please add a dcheck then. > > Well the elseif condition is size_before > 0 so why would we add a dcheck? Let me clarify: the |else| should be unconditional, and the condition should move to a dcheck inside |else {...}| instead. - shared_state_.events_.Queue() => size_after > 0. - |if|-|else| block => size_before == _after. - Therefore, size_before > 0 here. https://codereview.chromium.org/2765583002/diff/60001/content/renderer/input/... File content/renderer/input/main_thread_event_queue.cc (right): https://codereview.chromium.org/2765583002/diff/60001/content/renderer/input/... content/renderer/input/main_thread_event_queue.cc:78: const QueuedWebInputEvent& other_event = Please add a DCHECK(IsWebInputEvent()). https://codereview.chromium.org/2765583002/diff/60001/content/renderer/input/... content/renderer/input/main_thread_event_queue.cc:89: dispatch_type_ = other_event.dispatch_type_; Not clear to me why the other event's dispatch type dominates here. I think, like |originally_cancelable| bit the there is an assumed ordering of events here, right? In any case, could you please add a comment in the definition? https://codereview.chromium.org/2765583002/diff/60001/content/renderer/input/... content/renderer/input/main_thread_event_queue.cc:91: originally_cancelable_ = other_event.originally_cancelable_; I think we assume that events that can be coalesced can go from "calcelable" to "uncancelable" over time but not vice versa. Please clarify the comment in the definition. https://codereview.chromium.org/2765583002/diff/60001/content/renderer/input/... content/renderer/input/main_thread_event_queue.cc:377: in_flight_event_.reset(); Nit: call reset() if |if(in_flight_event_)| is true above. https://codereview.chromium.org/2765583002/diff/60001/content/renderer/input/... content/renderer/input/main_thread_event_queue.cc:500: bool is_coalesced_raf_aligned = Do we need to check IsRafAlignedInputDisabled() here? https://codereview.chromium.org/2765583002/diff/60001/content/renderer/input/... content/renderer/input/main_thread_event_queue.cc:502: if (was_raf_aligned != is_coalesced_raf_aligned) { I don't quite follow this "!=" part, and it turned out in our offline discussion that this may be old code now. In case part of the |else| code survives, please make sure the dependency on IsRafAlignedInputDisabled() is correctly handled.
The CQ bit was checked by dtapuska@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: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) 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 dtapuska@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: 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 dtapuska@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...
https://codereview.chromium.org/2765583002/diff/60001/content/renderer/input/... File content/renderer/input/main_thread_event_queue.cc (right): https://codereview.chromium.org/2765583002/diff/60001/content/renderer/input/... content/renderer/input/main_thread_event_queue.cc:78: const QueuedWebInputEvent& other_event = On 2017/03/27 15:54:30, mustaq wrote: > Please add a DCHECK(IsWebInputEvent()). Done. https://codereview.chromium.org/2765583002/diff/60001/content/renderer/input/... content/renderer/input/main_thread_event_queue.cc:89: dispatch_type_ = other_event.dispatch_type_; On 2017/03/27 15:54:30, mustaq wrote: > Not clear to me why the other event's dispatch type dominates here. I think, > like |originally_cancelable| bit the there is an assumed ordering of events > here, right? > > In any case, could you please add a comment in the definition? Done. https://codereview.chromium.org/2765583002/diff/60001/content/renderer/input/... content/renderer/input/main_thread_event_queue.cc:91: originally_cancelable_ = other_event.originally_cancelable_; On 2017/03/27 15:54:30, mustaq wrote: > I think we assume that events that can be coalesced can go from "calcelable" to > "uncancelable" over time but not vice versa. Please clarify the comment in the > definition. Done. https://codereview.chromium.org/2765583002/diff/60001/content/renderer/input/... content/renderer/input/main_thread_event_queue.cc:377: in_flight_event_.reset(); On 2017/03/27 15:54:30, mustaq wrote: > Nit: call reset() if |if(in_flight_event_)| is true above. Done. https://codereview.chromium.org/2765583002/diff/60001/content/renderer/input/... content/renderer/input/main_thread_event_queue.cc:500: bool is_coalesced_raf_aligned = On 2017/03/27 15:54:30, mustaq wrote: > Do we need to check IsRafAlignedInputDisabled() here? Yes it doesn't need to be there. https://codereview.chromium.org/2765583002/diff/60001/content/renderer/input/... content/renderer/input/main_thread_event_queue.cc:502: if (was_raf_aligned != is_coalesced_raf_aligned) { On 2017/03/27 15:54:30, mustaq wrote: > I don't quite follow this "!=" part, and it turned out in our offline discussion > that this may be old code now. > > In case part of the |else| code survives, please make sure the dependency on > IsRafAlignedInputDisabled() is correctly handled. Removed code.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: CQ has no permission to schedule in bucket master.tryserver.chromium.linux
LGTM with a request to add few more test sequences... https://codereview.chromium.org/2765583002/diff/120001/content/renderer/input... File content/renderer/input/input_event_filter.cc (right): https://codereview.chromium.org/2765583002/diff/120001/content/renderer/input... content/renderer/input/input_event_filter.cc:116: main_task_runner_->PostTask(FROM_HERE, closure); Does it worth adding a comment on when this line is reached? https://codereview.chromium.org/2765583002/diff/120001/content/renderer/input... File content/renderer/input/main_thread_event_queue_unittest.cc (right): https://codereview.chromium.org/2765583002/diff/120001/content/renderer/input... content/renderer/input/main_thread_event_queue_unittest.cc:947: TEST_P(MainThreadEventQueueTest, QueuingClosures) { I would like tests for the following sequences (E=event, C=closure), possibly each after both RunPendingTasksWithSimulatedRaf and RunUntilIdle (so 6 cases on total): - [C,C] - [E,C,C,E] - [C,E,E,C] https://codereview.chromium.org/2765583002/diff/120001/content/renderer/input... content/renderer/input/main_thread_event_queue_unittest.cc:967: RunPendingTasksWithSimulatedRaf(); Please add an EXPECT_EQ for event_queue.size == 4 right before the RunPendingTasksWithSimulatedRaf.
The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run
https://codereview.chromium.org/2765583002/diff/120001/content/renderer/input... File content/renderer/input/input_event_filter.cc (right): https://codereview.chromium.org/2765583002/diff/120001/content/renderer/input... content/renderer/input/input_event_filter.cc:116: main_task_runner_->PostTask(FROM_HERE, closure); On 2017/03/29 14:47:07, mustaq wrote: > Does it worth adding a comment on when this line is reached? Done. https://codereview.chromium.org/2765583002/diff/120001/content/renderer/input... File content/renderer/input/main_thread_event_queue_unittest.cc (right): https://codereview.chromium.org/2765583002/diff/120001/content/renderer/input... content/renderer/input/main_thread_event_queue_unittest.cc:947: TEST_P(MainThreadEventQueueTest, QueuingClosures) { On 2017/03/29 14:47:07, mustaq wrote: > I would like tests for the following sequences (E=event, C=closure), possibly > each after both RunPendingTasksWithSimulatedRaf and RunUntilIdle (so 6 cases on > total): > - [C,C] > - [E,C,C,E] > - [C,E,E,C] I've adjusted the tests to ensure there is sufficient coverage here. Some are redundant because for two closures there is no rAF. And the others are redundant because the test is parameterized. https://codereview.chromium.org/2765583002/diff/120001/content/renderer/input... content/renderer/input/main_thread_event_queue_unittest.cc:967: RunPendingTasksWithSimulatedRaf(); On 2017/03/29 14:47:07, mustaq wrote: > Please add an EXPECT_EQ for event_queue.size == 4 right before the > RunPendingTasksWithSimulatedRaf. Done.
dtapuska@chromium.org changed reviewers: + tdresser@chromium.org
On 2017/03/29 20:08:14, dtapuska wrote: > https://codereview.chromium.org/2765583002/diff/120001/content/renderer/input... > File content/renderer/input/input_event_filter.cc (right): > > https://codereview.chromium.org/2765583002/diff/120001/content/renderer/input... > content/renderer/input/input_event_filter.cc:116: > main_task_runner_->PostTask(FROM_HERE, closure); > On 2017/03/29 14:47:07, mustaq wrote: > > Does it worth adding a comment on when this line is reached? > > Done. > > https://codereview.chromium.org/2765583002/diff/120001/content/renderer/input... > File content/renderer/input/main_thread_event_queue_unittest.cc (right): > > https://codereview.chromium.org/2765583002/diff/120001/content/renderer/input... > content/renderer/input/main_thread_event_queue_unittest.cc:947: > TEST_P(MainThreadEventQueueTest, QueuingClosures) { > On 2017/03/29 14:47:07, mustaq wrote: > > I would like tests for the following sequences (E=event, C=closure), possibly > > each after both RunPendingTasksWithSimulatedRaf and RunUntilIdle (so 6 cases > on > > total): > > - [C,C] > > - [E,C,C,E] > > - [C,E,E,C] > > I've adjusted the tests to ensure there is sufficient coverage here. Some are > redundant because for two closures there is no rAF. And the others are redundant > because the test is parameterized. > > https://codereview.chromium.org/2765583002/diff/120001/content/renderer/input... > content/renderer/input/main_thread_event_queue_unittest.cc:967: > RunPendingTasksWithSimulatedRaf(); > On 2017/03/29 14:47:07, mustaq wrote: > > Please add an EXPECT_EQ for event_queue.size == 4 right before the > > RunPendingTasksWithSimulatedRaf. > > Done. tdresser@ care to take a look?
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.
Still LGTM, thanks.
A few preliminary questions, as I'm not completely following this. https://codereview.chromium.org/2765583002/diff/140001/content/renderer/input... File content/renderer/input/input_event_filter.cc (right): https://codereview.chromium.org/2765583002/diff/140001/content/renderer/input... content/renderer/input/input_event_filter.cc:117: // For some reason we de didn't find an event queue for the route. we de didn't -> we didn't https://codereview.chromium.org/2765583002/diff/140001/content/renderer/input... content/renderer/input/input_event_filter.cc:118: // Don't drop the task on the floor allow it to execute. It isn't clear to me what the correct behavior is here. Do you have any justification for this? https://codereview.chromium.org/2765583002/diff/140001/content/renderer/input... content/renderer/input/input_event_filter.cc:210: // ID. It isn't clear to me what's happening here. Can you add a bit more detail to the comment? Are routing_ids able to refer to either RenderViews or RenderFrames? https://codereview.chromium.org/2765583002/diff/140001/content/renderer/input... content/renderer/input/input_event_filter.cc:213: routes_.find(associated_routing_id->second) == routes_.end()) I prefer using {} with multiline conditions (though it isn't technically required). Either way is fine with me. https://codereview.chromium.org/2765583002/diff/140001/content/renderer/input... File content/renderer/input/input_event_filter.h (right): https://codereview.chromium.org/2765583002/diff/140001/content/renderer/input... content/renderer/input/input_event_filter.h:145: using AssociatedRoutes = std::unordered_map<int, int>; Add comment indicating what this maps from and to. Maybe add a similar comment on the RouteQueueMap while you're add it.
https://codereview.chromium.org/2765583002/diff/140001/content/renderer/input... File content/renderer/input/input_event_filter.cc (right): https://codereview.chromium.org/2765583002/diff/140001/content/renderer/input... content/renderer/input/input_event_filter.cc:117: // For some reason we de didn't find an event queue for the route. On 2017/03/30 14:20:50, tdresser wrote: > we de didn't -> we didn't Done. https://codereview.chromium.org/2765583002/diff/140001/content/renderer/input... content/renderer/input/input_event_filter.cc:118: // Don't drop the task on the floor allow it to execute. On 2017/03/30 14:20:50, tdresser wrote: > It isn't clear to me what the correct behavior is here. Do you have any > justification for this? So there is the off chance that the time the HandleInputEvent is passed to the compositor thread the associated route was removed so I still wanted it to run. https://codereview.chromium.org/2765583002/diff/140001/content/renderer/input... content/renderer/input/input_event_filter.cc:213: routes_.find(associated_routing_id->second) == routes_.end()) On 2017/03/30 14:20:50, tdresser wrote: > I prefer using {} with multiline conditions (though it isn't technically > required). Either way is fine with me. Ya I prefer that as well. I missed it cause I rolled two if conditions together to one. Done. https://codereview.chromium.org/2765583002/diff/140001/content/renderer/input... File content/renderer/input/input_event_filter.h (right): https://codereview.chromium.org/2765583002/diff/140001/content/renderer/input... content/renderer/input/input_event_filter.h:145: using AssociatedRoutes = std::unordered_map<int, int>; On 2017/03/30 14:20:50, tdresser wrote: > Add comment indicating what this maps from and to. Maybe add a similar comment > on the RouteQueueMap while you're add it. Done.
LGTM with nits (and a question). https://codereview.chromium.org/2765583002/diff/160001/content/renderer/input... File content/renderer/input/input_event_filter.cc (right): https://codereview.chromium.org/2765583002/diff/160001/content/renderer/input... content/renderer/input/input_event_filter.cc:215: // unncessary as this would break for mojo which doesn't guarantee unncessary -> unnecessary https://codereview.chromium.org/2765583002/diff/160001/content/renderer/input... content/renderer/input/input_event_filter.cc:262: // so it should be the same as the message routing ID. Isn't it sometimes sent with the RenderFrame routing ID? https://codereview.chromium.org/2765583002/diff/160001/content/renderer/input... File content/renderer/input/input_event_filter.h (right): https://codereview.chromium.org/2765583002/diff/160001/content/renderer/input... content/renderer/input/input_event_filter.h:67: int render_view_routing_id) override; RegisterAssociatedRenderFrameRoutingID? https://codereview.chromium.org/2765583002/diff/160001/content/renderer/input... File content/renderer/input/main_thread_event_queue_task.h (right): https://codereview.chromium.org/2765583002/diff/160001/content/renderer/input... content/renderer/input/main_thread_event_queue_task.h:27: const MainThreadEventQueueTask&) const = 0; Add a comment on this method. It took me a while to figure out that this is to enable coalescing past events from different input modalities.
https://codereview.chromium.org/2765583002/diff/160001/content/renderer/input... File content/renderer/input/input_event_filter.cc (right): https://codereview.chromium.org/2765583002/diff/160001/content/renderer/input... content/renderer/input/input_event_filter.cc:215: // unncessary as this would break for mojo which doesn't guarantee On 2017/03/30 17:46:16, tdresser wrote: > unncessary -> unnecessary Done. https://codereview.chromium.org/2765583002/diff/160001/content/renderer/input... content/renderer/input/input_event_filter.cc:262: // so it should be the same as the message routing ID. On 2017/03/30 17:46:16, tdresser wrote: > Isn't it sometimes sent with the RenderFrame routing ID? Input Events as in InputMsg_HandleInputEvent is always sent to the RenderView... I've adjusted some of the comments to make this clear the distinction between Events and messages. https://codereview.chromium.org/2765583002/diff/160001/content/renderer/input... File content/renderer/input/input_event_filter.h (right): https://codereview.chromium.org/2765583002/diff/160001/content/renderer/input... content/renderer/input/input_event_filter.h:67: int render_view_routing_id) override; On 2017/03/30 17:46:16, tdresser wrote: > RegisterAssociatedRenderFrameRoutingID? Done. https://codereview.chromium.org/2765583002/diff/160001/content/renderer/input... File content/renderer/input/main_thread_event_queue_task.h (right): https://codereview.chromium.org/2765583002/diff/160001/content/renderer/input... content/renderer/input/main_thread_event_queue_task.h:27: const MainThreadEventQueueTask&) const = 0; On 2017/03/30 17:46:16, tdresser wrote: > Add a comment on this method. It took me a while to figure out that this is to > enable coalescing past events from different input modalities. Rejigged callback to return an enum and removed two virtuals.
The CQ bit was checked by dtapuska@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...
Nice improvement with event coalescing. https://codereview.chromium.org/2765583002/diff/180001/content/renderer/input... File content/renderer/input/input_event_filter.cc (right): https://codereview.chromium.org/2765583002/diff/180001/content/renderer/input... content/renderer/input/input_event_filter.cc:213: // TODO(dtapuska): Input mesages should NOT be sent to RenderFrames and mesages -> messages
The CQ bit was checked by dtapuska@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.
dtapuska@chromium.org changed reviewers: + creis@chromium.org
creis@chromium.org: Please review changes in */BUILD.gn
Description was changed from ========== Teach main thread event queue about closures. This allows the rAF signal to execute all pending input right away instead of waiting for a message pump to process any input IPCs. This simplifies the state that we don't need to execute a post task for every event we have to process. At delivery time we can just take them all from the queue (except for the rAF aligned events) and execute them. BUG=674301 ========== to ========== Teach main thread event queue about closures. This allows the rAF signal to execute all pending input right away instead of waiting for a message pump to process any input IPCs. This simplifies the state that we don't need to execute a post task for every event we have to process. At delivery time we can just take them all from the queue (except for the rAF aligned events) and execute them. BUG=674301 TBR=creis@chromium.org ==========
The CQ bit was checked by dtapuska@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mustaq@chromium.org, tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2765583002/#ps200001 (title: "Fix typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/31 13:38:35, dtapuska wrote: > mailto:creis@chromium.org: Please review changes in > > */BUILD.gn TBR creis@ since it is just changes to BUILD.gn
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1490975116411580,
"parent_rev": "66b6e71817250892d6261bf46ca8a9429ed9b27a", "commit_rev":
"8ba8d7142ef4449775d3d75b6f17b80d24d07f77"}
Message was sent while issue was closed.
Description was changed from ========== Teach main thread event queue about closures. This allows the rAF signal to execute all pending input right away instead of waiting for a message pump to process any input IPCs. This simplifies the state that we don't need to execute a post task for every event we have to process. At delivery time we can just take them all from the queue (except for the rAF aligned events) and execute them. BUG=674301 TBR=creis@chromium.org ========== to ========== Teach main thread event queue about closures. This allows the rAF signal to execute all pending input right away instead of waiting for a message pump to process any input IPCs. This simplifies the state that we don't need to execute a post task for every event we have to process. At delivery time we can just take them all from the queue (except for the rAF aligned events) and execute them. BUG=674301 TBR=creis@chromium.org Review-Url: https://codereview.chromium.org/2765583002 Cr-Commit-Position: refs/heads/master@{#461133} Committed: https://chromium.googlesource.com/chromium/src/+/8ba8d7142ef4449775d3d75b6f17... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/8ba8d7142ef4449775d3d75b6f17...
Message was sent while issue was closed.
RS LGTM for build files. |
