See Bug 39588 for background.
Created attachment 61785 [details]
Comment on attachment 61785 [details]
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.
> 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.
Created attachment 62390 [details]
Comment on attachment 62390 [details]
why is this necessary?
+ m_client->lastOrientation() : DeviceOrientation::create();
I'd lean towards not wrapping these.
+ // 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.
+ 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
> + m_timer.stop();
> why is this necessary?
It's not, will remove.
> + m_client->lastOrientation() : DeviceOrientation::create();
> I'd lean towards not wrapping these.
> + // 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.
Committed r64048: <http://trac.webkit.org/changeset/64048>