Summary: | It's possible to clone consumable user gestures | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | jochen | ||||||||||||
Component: | WebCore Misc. | Assignee: | jochen | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, cdn, cevans, dglazkov, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
jochen
2012-09-24 14:31:49 PDT
Created attachment 166662 [details]
Patch
Comment on attachment 166662 [details] Patch Attachment 166662 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14114676 New failing tests: platform/mac/editing/deleting/backward-delete.html fast/events/popup-blocking-timers.html svg/custom/use-instanceRoot-event-bubbling.xhtml Comment on attachment 166662 [details] Patch Attachment 166662 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14138159 New failing tests: fast/events/popup-blocking-timers.html svg/custom/use-instanceRoot-event-bubbling.xhtml Created attachment 166685 [details]
Patch
*** Bug 98060 has been marked as a duplicate of this bug. *** Comment on attachment 166685 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166685&action=review > Source/WebCore/ChangeLog:10 > + is suppoed to forward the user gesture, take a reference to this token suppoed -> supposed > Source/WebCore/dom/UserGestureIndicator.cpp:37 > + return adoptRef(static_cast<UserGestureIndicator::Token*>(new GestureToken)); Is this static_cast needed? I thought PassRefPtr was smart enough to know how to do the type conversion for you. > Source/WebCore/dom/UserGestureIndicator.cpp:42 > + void inc() { m_consumableGestures++; } inc -> increment? Please use complete words in variable names. Maybe addGesture() is a better name? > Source/WebCore/dom/UserGestureIndicator.cpp:43 > + bool consume() consume -> consumeGesture? > Source/WebCore/dom/UserGestureIndicator.cpp:50 > + bool isValid() const { return m_consumableGestures > 0; } isValid -> hasGestures() ? Comment on attachment 166685 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166685&action=review > Source/WebCore/dom/UserGestureIndicator.h:64 > + ProcessingUserGestureState m_state; > + Token* m_previousToken; > + RefPtr<Token> m_token; It seems strange to need to add all this information to the stack. Comment on attachment 166685 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166685&action=review > Source/WebCore/page/DOMTimer.h:73 > - bool m_shouldForwardUserGesture; > + RefPtr<UserGestureIndicator::Token> m_userGestureToken; This part makes sense. DOMTimer wants to forward the user gesture only if it wasn't consumed. (In reply to comment #7) > (From update of attachment 166685 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=166685&action=review > > > Source/WebCore/dom/UserGestureIndicator.h:64 > > + ProcessingUserGestureState m_state; > > + Token* m_previousToken; > > + RefPtr<Token> m_token; > > It seems strange to need to add all this information to the stack. hum, indeed m_state is not required. The other two, however, are required. This design seems more complicated than needed. What question is DOMTimer really trying to answer? It wants to know whether all the user gestures for its call stack where consumed before that stack unwound. That means we only need one counter per call stack, not stack of counters, right? Created attachment 166738 [details]
Patch
Created attachment 166739 [details]
Patch
I believe I addressed all your comments Created attachment 166747 [details]
Patch
Comment on attachment 166747 [details]
Patch
Looks great. Thanks.
Comment on attachment 166747 [details] Patch Clearing flags on attachment: 166747 Committed r130267: <http://trac.webkit.org/changeset/130267> All reviewed patches have been landed. Closing bug. |