Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
210203002:
events: Introduce EventRewriter. (Closed)
|
Created:
6 years, 9 months ago by kpschoedel Modified:
6 years, 9 months ago Reviewers:
sadrul CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptionevents: Introduce EventRewriter.
EventSource gets a list of EventRewriters, which each get a chance to
rewrite an event before it gets sent to the EventProcessor.
BUG=354033
R=sadrul@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260132
Patch Set 1 #
Total comments: 15
Patch Set 2 : Address review comments. #
Total comments: 7
Patch Set 3 : Address review comments II #
Total comments: 1
Patch Set 4 : Sort targets in events.gyp #Patch Set 5 : Fix missing 'virtual' #
Messages
Total messages: 27 (0 generated)
https://codereview.chromium.org/210203002/diff/1/ui/events/event_rewriter.h File ui/events/event_rewriter.h (right): https://codereview.chromium.org/210203002/diff/1/ui/events/event_rewriter.h#n... ui/events/event_rewriter.h:18: EVENT_REWRITE_DISPATCH_ANOTHER, Comments for each to explain what they mean. https://codereview.chromium.org/210203002/diff/1/ui/events/event_rewriter.h#n... ui/events/event_rewriter.h:21: class EVENTS_EXPORT EventRewriter { A brief class-level comment to give an overview of the responsibilities of EventRewriter. https://codereview.chromium.org/210203002/diff/1/ui/events/event_rewriter_uni... File ui/events/event_rewriter_unittest.cc (right): https://codereview.chromium.org/210203002/diff/1/ui/events/event_rewriter_uni... ui/events/event_rewriter_unittest.cc:27: ~TestEventRewriteProcessor() { CheckAllReceived(); } virtual https://codereview.chromium.org/210203002/diff/1/ui/events/event_rewriter_uni... ui/events/event_rewriter_unittest.cc:30: virtual EventDispatchDetails OnEventFromSource(Event* event) OVERRIDE { Overrides typically go at the end of the block. So this should be after CheckAllReceived() https://codereview.chromium.org/210203002/diff/1/ui/events/event_rewriter_uni... ui/events/event_rewriter_unittest.cc:42: std::list<EventType> expected_events_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/210203002/diff/1/ui/events/event_rewriter_uni... ui/events/event_rewriter_unittest.cc:51: Event* New(EventType type) { This should return a scoped_ptr<> https://codereview.chromium.org/210203002/diff/1/ui/events/event_rewriter_uni... ui/events/event_rewriter_unittest.cc:87: (void)SendEventToProcessor(event.get()); You don't need to Delete the event in the event-factory? https://codereview.chromium.org/210203002/diff/1/ui/events/event_rewriter_uni... ui/events/event_rewriter_unittest.cc:102: EXPECT_NE(EVENT_REWRITE_DISPATCH_ANOTHER, status); CHECK instead https://codereview.chromium.org/210203002/diff/1/ui/events/event_rewriter_uni... ui/events/event_rewriter_unittest.cc:115: EXPECT_TRUE(false); Replace this with a NOTREACHED instead https://codereview.chromium.org/210203002/diff/1/ui/events/event_rewriter_uni... ui/events/event_rewriter_unittest.cc:156: event_factory_.Check(last_rewritten_event_); We should add a unique-identifer for each TestEvent, and compare that id here to verify that the event rewritten by this rewriter is the one that comes back as |last_event|. This would allow us to get rid of EventFactory, and instead have just TestEvent that the tests create directly where necessary. https://codereview.chromium.org/210203002/diff/1/ui/events/event_source.cc File ui/events/event_source.cc (right): https://codereview.chromium.org/210203002/diff/1/ui/events/event_source.cc#ne... ui/events/event_source.cc:27: Event* rewritten_event = 0; We should use a scoped_ptr<> to avoid accidentally leaking a rewritten event (e.g. I think we leak it if we return from line 38-39 in this patch). https://codereview.chromium.org/210203002/diff/1/ui/events/event_source.cc#ne... ui/events/event_source.cc:35: while (status == EVENT_REWRITE_DISPATCH_ANOTHER) { Can we do this more like: for (....) { status = (*it)->RewriteEvent(*event, &rewritten_event); if (status == DISCARD) { CHECK(!rewritten_event); return EventDispatchDetails(); } if (status == CONTINUE) { CHECK(!rewritten_event); continue; } break; } CHECK((it == end() && !rewritten_event) || rewritten_event); details = DeliverEventToProcessor(rewritten_event ? rewritten_event : event); if (details.dispatcher_destroyed) return details; while (status == ANOTHER) { status = *ite->NextDispatchEvent(....); if (status == DISCARD) return ...; CHECK_EQ(ANOTHER, status); .... deliver ... } https://codereview.chromium.org/210203002/diff/1/ui/events/event_source.h File ui/events/event_source.h (right): https://codereview.chromium.org/210203002/diff/1/ui/events/event_source.h#new... ui/events/event_source.h:38: EventRewriterList rewriter_list_; DISALLOW_COPY_AND_ASSIGN
https://codereview.chromium.org/210203002/diff/1/ui/events/event_rewriter_uni... File ui/events/event_rewriter_unittest.cc (right): https://codereview.chromium.org/210203002/diff/1/ui/events/event_rewriter_uni... ui/events/event_rewriter_unittest.cc:156: event_factory_.Check(last_rewritten_event_); On 2014/03/24 21:15:38, sadrul wrote: > We should add a unique-identifer for each TestEvent, and compare that id here to > verify that the event rewritten by this rewriter is the one that comes back as > |last_event|. Is it really necessary to pass back last_event? If the caller passes in the same scoped_ptr for new_event as it did previously (which is reasonable), then they alias and last_event will be destroyed upon new_event.reset(), which could be surprising.
https://codereview.chromium.org/210203002/diff/1/ui/events/event_rewriter_uni... File ui/events/event_rewriter_unittest.cc (right): https://codereview.chromium.org/210203002/diff/1/ui/events/event_rewriter_uni... ui/events/event_rewriter_unittest.cc:156: event_factory_.Check(last_rewritten_event_); On 2014/03/25 18:12:01, kpschoedel wrote: > On 2014/03/24 21:15:38, sadrul wrote: > > We should add a unique-identifer for each TestEvent, and compare that id here > to > > verify that the event rewritten by this rewriter is the one that comes back as > > |last_event|. > > Is it really necessary to pass back last_event? If the caller passes in the same > scoped_ptr for new_event as it did previously (which is reasonable), then they > alias and last_event will be destroyed upon new_event.reset(), which could be > surprising. I am thinking the rewriter will need to maintain less internal state if we include |last_event| in this call to NextDispatchEvent(). We should have the source make sure that setting |new_event| doesn't invalidate |last_event|. If we end up not needing |last_event| in the rewriter-implementations, then we can come back and remove this. But I feel like we should start with it in place.
Addressed review comment & lint errors.
Nice! A few nits, other than that, looks good! https://codereview.chromium.org/210203002/diff/40001/ui/events/event_rewriter.h File ui/events/event_rewriter.h (right): https://codereview.chromium.org/210203002/diff/40001/ui/events/event_rewriter... ui/events/event_rewriter.h:52: const Event& event, scoped_ptr<Event>& rewritten_event) = 0; chromium doesn't allow non-const refs. So use a pointer for |rewritten_event| instead. You can remove 'arguments must not alias'. scoped_ptr<> is sufficient to convey that the EventRewriter takes ownership of the event, and the caller should not try to free it. https://codereview.chromium.org/210203002/diff/40001/ui/events/event_rewriter... ui/events/event_rewriter.h:55: // alias. It is only valid to call this after the immediately previous ditto https://codereview.chromium.org/210203002/diff/40001/ui/events/event_source.cc File ui/events/event_source.cc (right): https://codereview.chromium.org/210203002/diff/40001/ui/events/event_source.c... ui/events/event_source.cc:19: DCHECK(rewriter); Also DCHECK that |rewriter| isn't already in the list? https://codereview.chromium.org/210203002/diff/40001/ui/events/event_source.c... ui/events/event_source.cc:26: if (find != rewriter_list_.end()) rewriter_list_.erase(find); line-break after the conditional https://codereview.chromium.org/210203002/diff/40001/ui/events/event_source.c... ui/events/event_source.cc:49: if (details.dispatcher_destroyed) return details; line-break https://codereview.chromium.org/210203002/diff/40001/ui/events/event_source.c... ui/events/event_source.cc:54: if (status == EVENT_REWRITE_DISCARD) { You don't need braces for single-line blocks
https://codereview.chromium.org/210203002/diff/40001/ui/events/event_source.cc File ui/events/event_source.cc (right): https://codereview.chromium.org/210203002/diff/40001/ui/events/event_source.c... ui/events/event_source.cc:26: if (find != rewriter_list_.end()) rewriter_list_.erase(find); On 2014/03/26 17:26:27, sadrul wrote: > line-break after the conditional Ah, I was mistakenly picking up the wrong clang-format.
LGTM! https://codereview.chromium.org/210203002/diff/120001/ui/events/events.gyp File ui/events/events.gyp (right): https://codereview.chromium.org/210203002/diff/120001/ui/events/events.gyp#ne... ui/events/events.gyp:102: 'event_rewriter.h', sort (sorry, missed in earlier reviews)
The CQ bit was checked by kpschoedel@chromium.org
The CQ bit was unchecked by kpschoedel@chromium.org
The CQ bit was checked by kpschoedel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kpschoedel@chromium.org/210203002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on android_clang_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
The CQ bit was checked by kpschoedel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kpschoedel@chromium.org/210203002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by sadrul@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kpschoedel@chromium.org/210203002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
The CQ bit was checked by kpschoedel@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kpschoedel@chromium.org/210203002/160001
Message was sent while issue was closed.
Change committed as 260132 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
