Bug 42304 – DeviceOrientation event listeners should never be called synchronously from addEventListener()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Steve Block]

See Bug 39588 for background.

[Steve Block]

Created attachment 61785 [details]
Patch

[Ojan Vafai]

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.

[Steve Block]

> 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.

[Steve Block]

Created attachment 62390 [details]
Patch

[Jeremy Orlow]

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

[Steve Block]

> 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.

[Steve Block]

Committed r64048: <http://trac.webkit.org/changeset/64048>