Bug 42304

Summary: DeviceOrientation event listeners should never be called synchronously from addEventListener()
Product: WebKit Reporter: Steve Block <steveblock>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: hans, jorlow, ojan, steveblock
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 30335    
Attachments:
Description Flags
Patch
none
Patch jorlow: review+, jorlow: commit-queue-

Description Steve Block 2010-07-14 16:49:09 PDT
See Bug 39588 for background.
Comment 1 Steve Block 2010-07-16 02:42:05 PDT
Created attachment 61785 [details]
Patch
Comment 2 Ojan Vafai 2010-07-22 17:05:46 PDT
Comment on attachment 61785 [details]
Patch

Even if you can't add layout tests you should add manual tests that can be converted to layout tests once LayoutTestController supports DeviceOrientation. Also, it would be good if the ChangeLog description could point to the bugs for adding the necessary support to LayoutTestController.
Comment 3 Steve Block 2010-07-23 00:53:01 PDT
> Even if you can't add layout tests you should add manual tests that can be converted to layout tests once
> LayoutTestController supports DeviceOrientation.
I don't think this is possible. Currently, no platforms enable DeviceOrientation, so there's no real implementation to test. Addition of a mock implementation is underway at in Bug 39589. I already have Bug 39590 to track adding DeviceOrientation layout tests and assure you that I'll add a test for this case as soon as the mock is in place!

> Also, it would be good if the ChangeLog description could point to the bugs for adding the necessary
> support to LayoutTestController.
Will upload a patch to do this.
Comment 4 Steve Block 2010-07-23 01:04:41 PDT
Created attachment 62390 [details]
Patch
Comment 5 Jeremy Orlow 2010-07-26 08:18:13 PDT
Comment on attachment 62390 [details]
Patch

WebCore/dom/DeviceOrientationController.cpp:48
 +      m_timer.stop();
why is this necessary?

WebCore/dom/DeviceOrientationController.cpp:51
 +              m_client->lastOrientation() : DeviceOrientation::create();
I'd lean towards not wrapping these.

WebCore/dom/DeviceOrientationController.cpp:65
 +      // immediately trigger an asynchronous response.
This seems to imply that if it's not present, it would fire whenever it becomes so...which does't seem to be true.

Also "immediately" and "asynchronous" kind of conflict.

WebCore/dom/DeviceOrientationController.h:57
 +      typedef HashSet<DOMWindow*> ListenersSet;
I'd lean towards putting newlines between the pairs of typedef+declarations...but it's more personal preference.



r=me, but please consider making the above changes
Comment 6 Steve Block 2010-07-26 08:47:02 PDT
> WebCore/dom/DeviceOrientationController.cpp:48
>  +      m_timer.stop();
> why is this necessary?
It's not, will remove.

> WebCore/dom/DeviceOrientationController.cpp:51
>  +              m_client->lastOrientation() : DeviceOrientation::create();
> I'd lean towards not wrapping these.
Will fix.

> WebCore/dom/DeviceOrientationController.cpp:65
>  +      // immediately trigger an asynchronous response.
I was trying to describe that the event is async, but triggered right now, without waiting for the client to get new data. Will update the comment.
Comment 7 Steve Block 2010-07-26 09:03:51 PDT
Committed r64048: <http://trac.webkit.org/changeset/64048>