|Summary:||DeviceOrientation event listeners should never be called synchronously from addEventListener()|
|Product:||WebKit||Reporter:||Steve Block <steveblock>|
|Component:||WebCore Misc.||Assignee:||Nobody <webkit-unassigned>|
|Severity:||Normal||CC:||hans, jorlow, ojan, steveblock|
|Version:||528+ (Nightly build)|
|Bug Depends on:|
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 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.