[Webkit-unassigned] [Bug 39588] Provide implementation of DeviceOrientationController and hook into DOMWindow

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jul 14 03:48:07 PDT 2010


Jeremy Orlow <jorlow at chromium.org> changed:

           What    |Removed                     |Added
  Attachment #61370|review?                     |review+
               Flag|                            |

--- Comment #23 from Jeremy Orlow <jorlow at chromium.org>  2010-07-14 03:48:06 PST ---
(From update of attachment 61370)
(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.


Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

More information about the webkit-unassigned mailing list