|
Description
Beth Dakin
2015-03-18 13:19:33 PDT
Created attachment 248964 [details]
Patch
This patch leaves three follow-up bugs:
1. The patch adds the mouseforceup event but does not yet fire it.
2. The patch adds the mouseforceclick event but does not yet fire it.
3. The patch only sends mouseforcechanged between mousedown and mouseforcedown, but we do intend to send it when force changes until mouseup.
Created attachment 248965 [details]
Patch
This patch should apply to trunk.
Attachment 248965 [details] did not pass style-queue:
ERROR: Source/WebCore/dom/Element.h:472: The parameter name "platformMouseEvent" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WebCore/dom/WebKitMouseForceChangedEvent.cpp:29: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Source/WebCore/dom/WebKitMouseForceChangedEvent.cpp:51: Code inside a namespace should not be indented. [whitespace/indent] [4]
ERROR: Source/WebCore/dom/WebKitMouseForceChangedEvent.h:62: The parameter name "event" adds no information, so it should be removed. [readability/parameter_name] [5]
ERROR: Source/WebCore/dom/WebKitMouseForceChangedEvent.h:62: The parameter name "view" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 5 in 31 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 248965 [details] Patch Attachment 248965 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5793177172705280 New failing tests: js/dom/global-constructors-attributes.html Created attachment 248976 [details]
Archive of layout-test-results from ews100 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 248965 [details] Patch Attachment 248965 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6199248613801984 New failing tests: js/dom/global-constructors-attributes.html Created attachment 248977 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 249046 [details]
Patch
Hopefully this patch will make for greener bots. Attachment 249046 [details] did not pass style-queue:
ERROR: Source/WebCore/dom/WebKitMouseForceChangedEvent.cpp:51: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 1 in 36 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 249046 [details] Patch Attachment 249046 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5327447562125312 New failing tests: js/dom/global-constructors-attributes.html Created attachment 249055 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 249046 [details] Patch Attachment 249046 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5125318348111872 New failing tests: js/dom/global-constructors-attributes.html Created attachment 249058 [details]
Archive of layout-test-results from ews102 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 249059 [details]
Patch
Another go at the bots.
Attachment 249059 [details] did not pass style-queue:
ERROR: Source/WebCore/dom/WebKitMouseForceChangedEvent.cpp:51: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 1 in 37 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 249059 [details] Patch Attachment 249059 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6127471657222144 New failing tests: js/dom/global-constructors-attributes.html Created attachment 249061 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 249059 [details] Patch Attachment 249059 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6530050216165376 New failing tests: js/dom/global-constructors-attributes.html Created attachment 249064 [details]
Archive of layout-test-results from ews102 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 249124 [details]
Patch
Attachment 249124 [details] did not pass style-queue:
ERROR: Source/WebCore/dom/WebKitMouseForceChangedEvent.cpp:51: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 1 in 23 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 249124 [details] Patch Attachment 249124 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5962117026938880 New failing tests: js/dom/global-constructors-attributes.html Created attachment 249130 [details]
Archive of layout-test-results from ews102 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 249124 [details] Patch Attachment 249124 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4680757020721152 New failing tests: js/dom/global-constructors-attributes.html Created attachment 249131 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 249124 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249124&action=review > Source/WebCore/dom/Element.cpp:2137 > +#if ENABLE(MOUSE_FORCE_EVENTS) Is there a reason we don't just disappear these functions if this isn't enabled? > Source/WebCore/dom/WebKitMouseForceChangedEvent.cpp:32 > + Where's the #if ENABLE(MOUSE_FORCE_EVENTS)? > Source/WebCore/page/DOMWindow.idl:272 > + [NotEnumerable] attribute EventHandler onwebkitmouseforcecancelled; should these have Conditional=MOUSE_FORCE_EVENT? (In reply to comment #27) > Comment on attachment 249124 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=249124&action=review > > > Source/WebCore/dom/Element.cpp:2137 > > +#if ENABLE(MOUSE_FORCE_EVENTS) > > Is there a reason we don't just disappear these functions if this isn't > enabled? > Not necessarily a good one! I was just trying to minimize the number of spots I put the #ifs since they make code harder to read, but I am open to anything really. > > Source/WebCore/dom/WebKitMouseForceChangedEvent.cpp:32 > > + > > Where's the #if ENABLE(MOUSE_FORCE_EVENTS)? > That does seem like an oversight. > > Source/WebCore/page/DOMWindow.idl:272 > > + [NotEnumerable] attribute EventHandler onwebkitmouseforcecancelled; > > should these have Conditional=MOUSE_FORCE_EVENT? SO!!! I am glad you asked, because I am really confused about this. Earlier versions of my patch included this. However, in http://trac.webkit.org/changeset/181507 Darin removed all conditionals from DOMWindow.idl, and I am very confused about why. I asked Anders, who reviewed the patch, and he wasn't totally sure about it either. I should really ask Darin. Oh, I will cc him! > > > Source/WebCore/page/DOMWindow.idl:272
> > > + [NotEnumerable] attribute EventHandler onwebkitmouseforcecancelled;
> >
> > should these have Conditional=MOUSE_FORCE_EVENT?
>
> SO!!! I am glad you asked, because I am really confused about this. Earlier
> versions of my patch included this. However, in
> http://trac.webkit.org/changeset/181507 Darin removed all conditionals from
> DOMWindow.idl, and I am very confused about why. I asked Anders, who
> reviewed the patch, and he wasn't totally sure about it either. I should
> really ask Darin. Oh, I will cc him!
So to continue this thought…I didn't add the Conditional= part because Darin's patch seemed to imply that the new normal is to exclude it? But I confess that I am not clear on how the new normal prevents us from exposing all of these features to the web…
Comment on attachment 249124 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249124&action=review >>> Source/WebCore/dom/Element.cpp:2137 >>> +#if ENABLE(MOUSE_FORCE_EVENTS) >> >> Is there a reason we don't just disappear these functions if this isn't enabled? > > Not necessarily a good one! I was just trying to minimize the number of spots I put the #ifs since they make code harder to read, but I am open to anything really. My favorite pattern for this sort of thing is to have empty functions, inlined if necessary, to keep the #if statements out of all the call sites. I prefer separate functions because I don’t like all the UNUSED_PARAM you need if the conditionals are inside the functions, and also because it makes it easier to put them inlined and in headers. > Source/WebCore/dom/WebKitMouseForceChangedEvent.cpp:36 > + : force(0) better to use C++11 member initialization syntax: double force { 0 }; Then we don’t need to declare or define a constructor at all. > Source/WebCore/dom/WebKitMouseForceChangedEvent.cpp:41 > + : m_force(0) Same story here: double m_force { 0 }; Although in this case we should probably leave the default constructor, but just remove the explicit m_force initialization. > Source/WebCore/dom/WebKitMouseForceChangedEvent.cpp:63 > +WebKitMouseForceChangedEvent::~WebKitMouseForceChangedEvent() > +{ > +} Seems better to just not declare or define this and let the compiler take care of it. > Source/WebCore/dom/WebKitMouseForceChangedEvent.h:56 > + virtual EventInterface eventInterface() const override; Can this be private? > Source/WebCore/dom/WebKitMouseForceChangedEvent.idl:29 > + [InitializedByEventConstructor] readonly attribute float force; It seems strange that this is float in the IDL and double in the code. Can it be double here too? > Source/WebCore/html/HTMLAttributeNames.in:278 > +onwebkitmouseforcewillbegin > +onwebkitmouseforceup Better to keep these sorted in alphabetical order. > Source/WebCore/html/HTMLBodyElement.cpp:125 > + &onwebkitmouseforcewillbeginAttr, > + &onwebkitmouseforceupAttr, Better to keep these sorted in alphabetical order. >>> Source/WebCore/page/DOMWindow.idl:272 >>> + [NotEnumerable] attribute EventHandler onwebkitmouseforcecancelled; >> >> should these have Conditional=MOUSE_FORCE_EVENT? > > SO!!! I am glad you asked, because I am really confused about this. Earlier versions of my patch included this. However, in http://trac.webkit.org/changeset/181507 Darin removed all conditionals from DOMWindow.idl, and I am very confused about why. I asked Anders, who reviewed the patch, and he wasn't totally sure about it either. I should really ask Darin. Oh, I will cc him! It’s correct to include these unconditionally. We might decide some day we want to compile them out, but I suggest a pattern where the events are only there if needed, but the event handler machinery is always present. However, you should add coverage for these new event handlers to the Layout test named fast/dom/event-handler-attributes.html (In reply to comment #29) > So to continue this thought…I didn't add the Conditional= part because > Darin's patch seemed to imply that the new normal is to exclude it? But I > confess that I am not clear on how the new normal prevents us from exposing > all of these features to the web… The feature this exposes is that if you set an attribute named onfoo and then send an event foo, the code in that attribute will run. I think it’s harmless to expose that to the web and better for testing to not have it be conditional. As far as I know this doesn’t affect any technique for testing for the existence of a particular event, nor does it cause the engine to send events. A value for such an attribute would not be entirely ignored, but it would effectively be ignored. The handler would never be called because the event would never be dispatched. (In reply to comment #31) > (In reply to comment #29) > > So to continue this thought…I didn't add the Conditional= part because > > Darin's patch seemed to imply that the new normal is to exclude it? But I > > confess that I am not clear on how the new normal prevents us from exposing > > all of these features to the web… > > The feature this exposes is that if you set an attribute named onfoo and > then send an event foo, the code in that attribute will run. I think it’s > harmless to expose that to the web and better for testing to not have it be > conditional. > > As far as I know this doesn’t affect any technique for testing for the > existence of a particular event, nor does it cause the engine to send > events. A value for such an attribute would not be entirely ignored, but it > would effectively be ignored. The handler would never be called because the > event would never be dispatched. Cool, thank you! (In reply to comment #30) > Comment on attachment 249124 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=249124&action=review > > >>> Source/WebCore/dom/Element.cpp:2137 > >>> +#if ENABLE(MOUSE_FORCE_EVENTS) > >> > >> Is there a reason we don't just disappear these functions if this isn't enabled? > > > > Not necessarily a good one! I was just trying to minimize the number of spots I put the #ifs since they make code harder to read, but I am open to anything really. > > My favorite pattern for this sort of thing is to have empty functions, > inlined if necessary, to keep the #if statements out of all the call sites. > I prefer separate functions because I don’t like all the UNUSED_PARAM you > need if the conditionals are inside the functions, and also because it makes > it easier to put them inlined and in headers. > I added separate functions, but for the time being, I added them to the implementation file rather than the header file. I did that because these functions need to be marked WEBCORE_EXPORT, and I think that WEBCORE_EXPORT functions cannot be inlined without breaking some of the bots. If this is an incorrect understanding, I will fix. (I'll chat with Alex to confirm.) > > Source/WebCore/dom/WebKitMouseForceChangedEvent.cpp:36 > > + : force(0) > > better to use C++11 member initialization syntax: > > double force { 0 }; > > Then we don’t need to declare or define a constructor at all. > > > Source/WebCore/dom/WebKitMouseForceChangedEvent.cpp:41 > > + : m_force(0) > > Same story here: > > double m_force { 0 }; > > Although in this case we should probably leave the default constructor, but > just remove the explicit m_force initialization. > Fixed all of this. > > Source/WebCore/dom/WebKitMouseForceChangedEvent.cpp:63 > > +WebKitMouseForceChangedEvent::~WebKitMouseForceChangedEvent() > > +{ > > +} > > Seems better to just not declare or define this and let the compiler take > care of it. > Removed this. > > Source/WebCore/dom/WebKitMouseForceChangedEvent.h:56 > > + virtual EventInterface eventInterface() const override; > > Can this be private? > Looks like it can! Done. > > Source/WebCore/dom/WebKitMouseForceChangedEvent.idl:29 > > + [InitializedByEventConstructor] readonly attribute float force; > > It seems strange that this is float in the IDL and double in the code. Can > it be double here too? > Yes it can. Changed to a double in the idl. > > Source/WebCore/html/HTMLAttributeNames.in:278 > > +onwebkitmouseforcewillbegin > > +onwebkitmouseforceup > > Better to keep these sorted in alphabetical order. > Fixed. > > Source/WebCore/html/HTMLBodyElement.cpp:125 > > + &onwebkitmouseforcewillbeginAttr, > > + &onwebkitmouseforceupAttr, > > Better to keep these sorted in alphabetical order. > Fixed. > >>> Source/WebCore/page/DOMWindow.idl:272 > >>> + [NotEnumerable] attribute EventHandler onwebkitmouseforcecancelled; > >> > >> should these have Conditional=MOUSE_FORCE_EVENT? > > > > SO!!! I am glad you asked, because I am really confused about this. Earlier versions of my patch included this. However, in http://trac.webkit.org/changeset/181507 Darin removed all conditionals from DOMWindow.idl, and I am very confused about why. I asked Anders, who reviewed the patch, and he wasn't totally sure about it either. I should really ask Darin. Oh, I will cc him! > > It’s correct to include these unconditionally. We might decide some day we > want to compile them out, but I suggest a pattern where the events are only > there if needed, but the event handler machinery is always present. > > However, you should add coverage for these new event handlers to the Layout > test named fast/dom/event-handler-attributes.html Will do! New patch coming. Created attachment 249252 [details]
Patch
Attachment 249252 [details] did not pass style-queue:
ERROR: Source/WebCore/dom/WebKitMouseForceChangedEvent.cpp:45: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 1 in 39 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 249252 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249252&action=review > Source/WebCore/ChangeLog:23 > + random extra line here. > Source/WebKit2/Shared/mac/ActionMenuHitTestResult.h:68 > + bool hitTestResultPreventsDefault; This name is not fantastic. Maybe contentPreventedDefault? or scriptPreventedDefault? or... I don't know. Those don't seem great either. Maybe just "preventDefaultActions"? (In reply to comment #36) > Comment on attachment 249252 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=249252&action=review > > > Source/WebCore/ChangeLog:23 > > + > > random extra line here. > Fixed! > > Source/WebKit2/Shared/mac/ActionMenuHitTestResult.h:68 > > + bool hitTestResultPreventsDefault; > > This name is not fantastic. Maybe contentPreventedDefault? or > scriptPreventedDefault? or... I don't know. Those don't seem great either. > Maybe just "preventDefaultActions"? I do think that contentPreventedDefault, so I will switch to that. Comment on attachment 249252 [details] Patch Attachment 249252 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6475180398346240 New failing tests: js/dom/global-constructors-attributes.html Created attachment 249270 [details]
Archive of layout-test-results from ews102 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 249252 [details] Patch Attachment 249252 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5172597884977152 New failing tests: js/dom/global-constructors-attributes.html Created attachment 249271 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 249252 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=249252&action=review Are we going to update testRunner to be able to generate these mouse events? > Source/WebCore/ChangeLog:15 > + webkitmouseforcewillbegin -> Event is sent just before mousedown to indicate that > + force can be perceived if the user presses any harder. The author should prevent > + default on this event to both prevent the user agentâs default force click > + features and to receive the other 5 events. It's interesting that you have to prevent default here in order to get the other events. I'm not sure we have anything else like that. > Source/WebCore/ChangeLog:45 > + Add new files for WebKitMouseForceChangedEvent to build systems. > + * DerivedSources.cpp: > + * DerivedSources.make: > + * WebCore.vcxproj/WebCore.vcxproj: > + * WebCore.vcxproj/WebCore.vcxproj.filters: > + * WebCore.xcodeproj/project.pbxproj: > + * WebCore.xcodeproj/project.pbxproj: No changes to the cmake files? > Source/WebCore/dom/Element.h:475 > + WEBCORE_EXPORT void dispatchMouseForceDown(); > + WEBCORE_EXPORT void dispatchMouseForceUp(); > + WEBCORE_EXPORT void dispatchMouseForceClick(); Why don't you need a PlatformMouseEvent here (at least for WillBegin, Down and Cancelled)? Don't we need a target? > Source/WebCore/dom/EventNames.in:32 > WebKitAnimationEvent > +WebKitMouseForceChangedEvent > WebKitTransitionEvent Why is there only one addition here? I expected a WebKitMouseForceDownEvent, WillBegin and Cancelled. (In reply to comment #42) > Comment on attachment 249252 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=249252&action=review > > Are we going to update testRunner to be able to generate these mouse events? > Yes! I am planning to hook this up very soon. Will file a follow-up bug. > > Source/WebCore/ChangeLog:15 > > + webkitmouseforcewillbegin -> Event is sent just before mousedown to indicate that > > + force can be perceived if the user presses any harder. The author should prevent > > + default on this event to both prevent the user agentâs default force click > > + features and to receive the other 5 events. > > It's interesting that you have to prevent default here in order to get the > other events. I'm not sure we have anything else like that. > It is a bit weird, but I fear spamming way too many events on web content that simply doesn't care otherwise. I think it's the right trade-off. I'm sure this will get further discussion. > > Source/WebCore/ChangeLog:45 > > + Add new files for WebKitMouseForceChangedEvent to build systems. > > + * DerivedSources.cpp: > > + * DerivedSources.make: > > + * WebCore.vcxproj/WebCore.vcxproj: > > + * WebCore.vcxproj/WebCore.vcxproj.filters: > > + * WebCore.xcodeproj/project.pbxproj: > > + * WebCore.xcodeproj/project.pbxproj: > > No changes to the cmake files? > I did edit CMakeLists.txt, but it looks like somehow that missed the Changelog. My build is still failing on Win though, so I'm missing something… > > Source/WebCore/dom/Element.h:475 > > + WEBCORE_EXPORT void dispatchMouseForceDown(); > > + WEBCORE_EXPORT void dispatchMouseForceUp(); > > + WEBCORE_EXPORT void dispatchMouseForceClick(); > > Why don't you need a PlatformMouseEvent here (at least for WillBegin, Down > and Cancelled)? Don't we need a target? > > > Source/WebCore/dom/EventNames.in:32 > > WebKitAnimationEvent > > +WebKitMouseForceChangedEvent > > WebKitTransitionEvent > > Why is there only one addition here? I expected a WebKitMouseForceDownEvent, > WillBegin and Cancelled. Hmmm, still thinking this over… Created attachment 249345 [details]
Patch for bots
This patch should address all of Dean's comments. Uploading a new copy for the bots to chew on before I commit.
Attachment 249345 [details] did not pass style-queue:
ERROR: Source/WebCore/dom/WebKitMouseForceEvent.cpp:45: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 1 in 40 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Committed change: http://trac.webkit.org/changeset/181907 |