Bug 42304 - DeviceOrientation event listeners should never be called synchronously from addEventListener()
Summary: DeviceOrientation event listeners should never be called synchronously from a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 30335
  Show dependency treegraph
 
Reported: 2010-07-14 16:49 PDT by Steve Block
Modified: 2010-07-26 09:03 PDT (History)
4 users (show)

See Also:


Attachments
Patch (4.84 KB, patch)
2010-07-16 02:42 PDT, Steve Block
no flags Details | Formatted Diff | Diff
Patch (4.94 KB, patch)
2010-07-23 01:04 PDT, Steve Block
jorlow: review+
jorlow: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>