Bug 39588 – Provide implementation of DeviceOrientationController and hook into DOMWindow

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Steve Block]

The DeviceOrientation receives registrations from the DOMWindow for device orientation events and starts and stops the DeviceOrientationClient. When it receives orientation updates, it fires these on the window object.

[Steve Block]

Created attachment 57101 [details]
Patch

[Steve Block]

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.

[Jeremy Orlow]

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.

[Steve Block]

Created attachment 57646 [details]
Patch

[Steve Block]

> 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

[Jeremy Orlow]

(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?

[Steve Block]

> 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.

[Steve Block]

ping?

[Jeremy Orlow]

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

[Hans Wennborg]

> 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.

[Steve Block]

> 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?

[Jeremy Orlow]

Comment on attachment 57646 [details]
Patch

Hm..ok, seems fine.

[Steve Block]

Comment on attachment 57646 [details]
Patch

Will update the patch once Bug 41607 is fixed.

[Steve Block]

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

[Steve Block]

Created attachment 61365 [details]
Patch

[Hans Wennborg]

(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.

[Hans Wennborg]

(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.

[Steve Block]

> 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.

[Steve Block]

Created attachment 61370 [details]
Patch

[Hans Wennborg]

(In reply to comment #19)
> Created an attachment (id=61370) [details]
> Patch

Looks good as far as I can tell.

[Jeremy Orlow]

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.

[Steve Block]

> 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?

[Jeremy Orlow]

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

[Steve Block]

> 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.

[WebKit Commit Bot]

Comment on attachment 61370 [details]
Patch

Clearing flags on attachment: 61370

Committed r63312: <http://trac.webkit.org/changeset/63312>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.