Summary: | document.createTouch crashes when document has no frame. | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Takashi Sakamoto <tasak> | ||||||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED CONFIGURATION CHANGED | ||||||||||||||||
Severity: | Normal | CC: | abarth, ahmad.saleem792, ap, benjamin, commit-queue, darin, esprehn+autocc, jkjiang, kangil.han, kevers, myid.shin, rniwa, tonikitoo, webkit-bug-importer, zalan | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Takashi Sakamoto
2013-03-26 02:23:12 PDT
This bug is caused by change r122286 (https://bugs.webkit.org/show_bug.cgi?id=88807). Source/WebCore/dom/Touch.cpp: @@Touch::Touch(Frame* frame, EventTarget* target, unsigned identifier, int screenX , m_rotationAngle(rotationAngle) , m_force(force) { + float scaleFactor = frame->pageZoomFactor() * frame->frameScaleFactor(); + float x = pageX * scaleFactor; + float y = pageY * scaleFactor; + m_absoluteLocation = roundedLayoutPoint(FloatPoint(x, y)); } Need to check whether frame is available or not. Created attachment 229215 [details]
Patch
Comment on attachment 229215 [details]
Patch
LayoutTest?
(In reply to comment #3) > (From update of attachment 229215 [details]) > LayoutTest? Ok. I got it Created attachment 229226 [details]
Patch
Comment on attachment 229226 [details]
Patch
Please generalize the code from mouse events instead.
Comment on attachment 229226 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229226&action=review > Source/WebCore/ChangeLog:9 > + Because it is possible that the Document has no the Frame. "Because it is..." isn't necessary, you can delete that line. > LayoutTests/fast/events/touch/document-create-touch-new-document.html:15 > +<p id="description"></p> You may want to state the situation being tested here (i.e., "Should not crash when creating touch event in document without a frame") Created attachment 229646 [details]
Patch
Attachment 229646 [details] did not pass style-queue:
ERROR: Source/WebCore/dom/MouseRelatedEvent.cpp:47: Wrong number of spaces before statement. (expected: 41) [whitespace/indent] [4]
Total errors found: 1 in 13 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #6) > (From update of attachment 229226 [details]) > Please generalize the code from mouse events instead. Hi, I've uploaded new patch. There is the style checking error, but I'm not sure whether I should fix it or not because It's already existed. I hope to get your feedback. Thanks. (In reply to comment #7) > (From update of attachment 229226 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=229226&action=review > > > Source/WebCore/ChangeLog:9 > > + Because it is possible that the Document has no the Frame. > > "Because it is..." isn't necessary, you can delete that line. > > > LayoutTests/fast/events/touch/document-create-touch-new-document.html:15 > > +<p id="description"></p> > > You may want to state the situation being tested here (i.e., "Should not crash when creating touch event in document without a frame") ==> I've uloaded new patch to change some test code regarding to your comment Comment on attachment 229646 [details]
Patch
It is *not* good to store a Frame* in every event, which is what this patch does. This looks like overkill. These helper functions don’t need to be put into a class, and we really don’t want to get multiple inheritance and virtual functions in the event classes just to do a bit more code sharing.
Created attachment 229831 [details]
Patch
Attachment 229831 [details] did not pass style-queue:
ERROR: Source/WebCore/dom/ComputeContent.h:31: #ifndef header guard has wrong style, please use: ComputeContent_h [build/header_guard] [5]
Total errors found: 1 in 10 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 229833 [details]
Patch
Based on MDN - https://developer.mozilla.org/en-US/docs/Web/API/Document/createTouch It is deprecated standard and is not supported by browsers (e.g. referring to compatibility table). Although, I am able to see some reference to "createTouch" in Webkit Github source code mirror in following files: Source/WebCore/dom/DocumentTouch.h Source/WebCore/dom/DocumentTouch.cpp Further, I am not able to run "repo.html" test and it shows error in console but using "patch" and getting layout test out of it (Changed into below JSFiddle), it shows "PASSED". Link - https://jsfiddle.net/sj9pc2m3/ Is something needed here or considering the test now pass from this patch reflects that it was fixed along the way? Thanks! We don't crash with the attached test case anymore. |