Bug 39588

Created attachment 57101 [details] Patch

I'm not sure that the structure of this patch is correct. Currently, when adding and removing DeviceOrientation listeners, I make the calls to the DeviceOrientation object directly from DOMWindow. However, the listener handling for touch events, which also makes calls to a client owned by the embedded, makes these calls from Document, rather than from DOMWindow directly. (See Document::addListenerTypeIfNeeded() and Document::removeAllEventListeners()) I could move the calls to DeviceOrientation to Document to match the pattern used by touch events, but this would add extra plumbing that isn't otherwise needed. Let me know if you'd like me to update the patch.

Comment on attachment 57101 [details] Patch Drive by comments (I'm not qualified to review this): WebCore/page/DOMWindow.cpp:1396 + if (eventType == eventNames().deviceorientationEvent && frame() && frame()->page()) maybe this should be an else if joined with the previous block? Could go either way about what's best. Obviously either is technically correct. I could go either way. WebCore/page/DOMWindow.cpp:1378 + if (eventType == eventNames().deviceorientationEvent && frame() && frame()->page()) Ditto WebCore/dom/DeviceOrientation.cpp:45 + ASSERT(!m_listeners.contains(window)); If you add multiple event listeners to one window, does it call addEventListener multiple times? I would have thought so...in which case, this assumption wouldn't be true.

Created attachment 57646 [details] Patch

> Drive by comments (I'm not qualified to review this): Can you recommend somebody who could review it? > WebCore/page/DOMWindow.cpp:1396 > + if (eventType == eventNames().deviceorientationEvent && frame() && frame()->page()) > maybe this should be an else if joined with the previous block? Done > WebCore/dom/DeviceOrientation.cpp:45 > + ASSERT(!m_listeners.contains(window)); > If you add multiple event listeners to one window, does it call > addEventListener multiple times? I would have thought so...in which case, this > assumption wouldn't be true. Fixed

(In reply to comment #5) > > Drive by comments (I'm not qualified to review this): > Can you recommend somebody who could review it? Not sure. I imagine many of the usual suspects like ap, mjs, sam. Maybe svn blame some of the event code or look at who reviewed Geolocation?

> Not sure. I imagine many of the usual suspects like ap, mjs, sam. Maybe svn > blame some of the event code or look at who reviewed Geolocation? Adding Sam and Antti, who reviewed some of the TouchEvents code.

ping?

Adding some reviewers who might feel qualified but (more importantly) aren't busy with WWDC. :-)

> WebCore/dom/DeviceOrientation.cpp:42 > +void DeviceOrientation::addListener(DOMWindow* window) > +{ > + if (m_listeners.isEmpty() && m_client) > + m_client->startUpdating(); > + m_listeners.add(window); > +} Just a thought here: it might be desirable to do m_listeners.add(window) before calling m_client->startUpdating(). It could be that on DeviceOrientationClient::startUpdating(), it immediately calls back into DeviceOrientation::onDeviceOrientation() with e.g. the last known orientation, or values indicating that it can't deliver any orientation data, and then there wouldn't be any registered listeners yet.

> Just a thought here: it might be desirable to do m_listeners.add(window) before > calling m_client->startUpdating(). It could be that on > DeviceOrientationClient::startUpdating(), it immediately calls back into > DeviceOrientation::onDeviceOrientation() with e.g. the last known orientation, > or values indicating that it can't deliver any orientation data, and then there > wouldn't be any registered listeners yet. Thanks for the comment Hans. My intention was that the client must always call back asynchronously. I think that's the more usual pattern. However, you're right that I should either make this requirement explicit, or make sure that re-entrant calls to DeviceOrientation::onDeviceOrientation() from DeviceOrientationClient::startUpdating() are handled correctly. Anybody care to review?

Comment on attachment 57646 [details] Patch Hm..ok, seems fine.

Comment on attachment 57646 [details] Patch Will update the patch once Bug 41607 is fixed.

Updated bug title now that DeviceOrientation has been renamed to DeviceOrientationController in bug 41608

Created attachment 61365 [details] Patch

(In reply to comment #15) > Created an attachment (id=61365) [details] > Patch Hmm, this looks wrong: In WebCore/dom/DeviceOrientationController.cpp you add definitions: +void DeviceOrientation::addListener(DOMWindow* window) +void DeviceOrientation::removeListener(DOMWindow* window) +void DeviceOrientation::removeAllListeners(DOMWindow* window) But they are members of DeviceOrientationController, right? Is it desirable to cache the orientation, as suggested by DeviceOrientationClient::lastOrientation()? I can see how this would solve the problem of notifying a new observer of the controller that orientation cannot be provided, but worry about the risk of dispatching new events with old orientation data. Not sure whether that is a real problem or not, though.

(In reply to comment #15) > Created an attachment (id=61365) [details] > Patch Also, in DeviceOrientationController::addListener(), if m_client is NULL, perhaps the controller should take the opportunity to dispatch an event with alpha, beta and gamma set to NULL so as to indicate to the web page that no orientation data can be provided, rather than doing nothing? This would be the scenario if an embedder flips the ENABLE switch but doesn't provide a client.

> Hmm, this looks wrong: Ah, merge error, fixed. > Is it desirable to cache the orientation, as suggested by > DeviceOrientationClient::lastOrientation()? This is required to make sure that new listeners receive an orientation (if available) immediately. The client has no way to know when new listeners are added - it only calls back when new data is available. > Also, in DeviceOrientationController::addListener(), if m_client is NULL, > perhaps the controller should take the opportunity to dispatch an event with > alpha, beta and gamma set to NULL so as to indicate to the web page that no > orientation data can be provided, rather than doing nothing? Sure, sounds reasonable.

Created attachment 61370 [details] Patch

(In reply to comment #19) > Created an attachment (id=61370) [details] > Patch Looks good as far as I can tell.

Comment on attachment 61370 [details] Patch 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. WebCore/dom/DeviceOrientationController.cpp:52 + window->dispatchEvent(DeviceOrientationEvent::create(eventNames().deviceorientationEvent, 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.

> 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(). > WebCore/dom/DeviceOrientationController.cpp:52 > + window->dispatchEvent(DeviceOrientationEvent::create(eventNames().deviceorientationEvent, 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?

Comment on attachment 61370 [details] Patch (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().deviceorientationEvent, 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

> 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. OK, I'll use an assert in the client, when I add it. > 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. I'll propose an update to the spec and fix this once it's agreed upon.

Comment on attachment 61370 [details] Patch Clearing flags on attachment: 61370 Committed r63312: <http://trac.webkit.org/changeset/63312>

All reviewed patches have been landed. Closing bug.