[webkit-reviews] review granted: [Bug 39588] Provide implementation of DeviceOrientationController and hook into DOMWindow : [Attachment 61370] Patch

[bugzilla-daemon at webkit.org]

Jeremy Orlow <jorlow at chromium.org> has granted Steve Block
<steveblock at google.com>'s request for review:
Bug 39588: Provide implementation of DeviceOrientationController and hook into
DOMWindow
https://bugs.webkit.org/show_bug.cgi?id=39588

Attachment 61370: Patch
https://bugs.webkit.org/attachment.cgi?id=61370&action=review

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
(In reply to comment #22)
> > WebCore/dom/DeviceOrientationController.cpp:58
> >  +		m_client->startUpdating();
> > Shouldn't you just ask the client whether it's been started?  At very least
you
> > should probably assert that it's not currently updating.
> I don't think there's any need to ask the client. This code is the only place
where the client is started, so if it's already been started it's due to a
logic error in this code. If we're to add an assert, I think it's better to do
so in DeviceOrientationClient::start(), rather than adding a new
DeviceOrientationClient::isStarted().

Agreed it's a logic error, that's why I'm concerned about it.  I don't care
where you put asserts, but I think we need something to detect such an error.

> > WebCore/dom/DeviceOrientationController.cpp:52
> >  +	       
window->dispatchEvent(DeviceOrientationEvent::create(eventNames().deviceorienta
tionEvent, m_client->lastOrientation()));
> > Is this specced to be synchronous?	If so, I think we should consider
changing
> > it.  If so, you shouldn't do it this way.
> Are you asking whether the DeviceOrientation API specs whether
window.addEventListener() may invoke the event handler synchronously? Currently
it doesn't say either way. Why do you think it's not good to invoke the event
handler synchronously?

Because it breaks run to completion.  And in general synchronous event handlers
are evil for multi-threaded/multi-process browsers.  Unless there's a very good
reason why it _needs_ to be synchronous, newly specified event handles should
not be synchronous.  I think you should change this in the spec.  I'm happy for
you do this in a second patch though.  Your choice.


r=me