Bug 29508 – Add ENABLE(ORIENTATION_EVENTS)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Greg Bolsinga]

iPhone has support for onorientationevent, which fires when the device is rotated. Here's the documentation:

http://developer.apple.com/safari/library/documentation/AppleApplications/Reference/SafariWebContent/HandlingEvents/HandlingEvents.html#//apple_ref/doc/uid/TP40006511-SW16

[Greg Bolsinga]

Created attachment 39796 [details]
patch to implement bug fix

There are no tests, since this feature isn't on by default in Open Source, and the platform it works with iPhone is not yet in this repository.

[Sam Weinig]

Greg, Geoff Garen is working on a rather large patch to simplify events and this will unfortunately conflict with it.  Would it be all right to hold off landing this for a day or so and then re-merge then?  I can help with any conflicts on that side.

Sorry for the inconvenience.

[Greg Bolsinga]

I don't mind. But iPhone WebKit will be based upon this patch, so a review of this version would help out too. Especially since it's the one that can be tested! :) Thanks.

[Sam Weinig]

Comment on attachment 39796 [details]
patch to implement bug fix

> +#if ENABLE(ORIENTATION_EVENTS)
> +EventListener* HTMLBodyElement::onorientationchange() const
> +{
> +    return document()->getWindowAttributeEventListener(eventNames().orientationchangeEvent);
> +}
> +
> +void HTMLBodyElement::setOnorientationchange(PassRefPtr<EventListener> eventListener)
> +{
> +    document()->setAttributeEventListener(eventNames().orientationchangeEvent, eventListener);

This should be setWindowAttributeEventListener.

>
>  
> +#if ENABLE(ORIENTATION_EVENTS)
> +EventListener* HTMLFrameSetElement::onorientationchange() const
> +{
> +    return document()->getWindowAttributeEventListener(eventNames().orientationchangeEvent);
> +}
> +
> +void HTMLFrameSetElement::setOnorientationchange(PassRefPtr<EventListener> eventListener)
> +{
> +    document()->setAttributeEventListener(eventNames().orientationchangeEvent, eventListener);

This should also be setWindowAttributeEventListener.

> +#if ENABLE(ORIENTATION_EVENTS)
> +        void sendOrientationChangeEvent(int orientation);
> +        int orientation() const;

These don't seem to be implemented anywhere.  Why should it be implemented per platform? Shouldn't it just store the new orientation and dispatch the event?

r-

[Greg Bolsinga]

(In reply to comment #4)
> This should be setWindowAttributeEventListener.

Thanks for catching these.

> > +#if ENABLE(ORIENTATION_EVENTS)
> > +        void sendOrientationChangeEvent(int orientation);
> > +        int orientation() const;
> 
> These don't seem to be implemented anywhere.  Why should it be implemented per
> platform? Shouldn't it just store the new orientation and dispatch the event?

It is/was in FrameiPhone.mm on iPhone. Good catch.

[Greg Bolsinga]

Created attachment 39863 [details]
address Sam's review comments

[Greg Bolsinga]

Created attachment 39867 [details]
Add onorientationchange to HTMLAttributeNames.in

[Greg Bolsinga]

Created attachment 39870 [details]
And add it to EventNames.h as well.

This now links for Mac OS X when it is turned on, although nothing send this event along to WebCore there.

[Simon Fraser (smfr)]

Comment on attachment 39870 [details]
And add it to EventNames.h as well.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog

> +        No new tests. (OOPS!)

You should remove or fix this line.

> diff --git a/WebCore/page/DOMWindow.h b/WebCore/page/DOMWindow.h
> index db4edda..3fb12eb 100644
> --- a/WebCore/page/DOMWindow.h
> +++ b/WebCore/page/DOMWindow.h
> @@ -85,6 +85,10 @@ namespace WebCore {
>  
>          void clear();
>  
> +#if ENABLE(ORIENTATION_EVENTS)
> +        int orientation() const;
> +#endif

This needs a comment to say what the value means. It's arbitrary degrees, should it be a float?

> +#if defined(ENABLE_ORIENTATION_EVENTS) && ENABLE_ORIENTATION_EVENTS
> +        readonly attribute long orientation;
> +#endif

This needs a comment to say what the value is.

> +#if ENABLE(ORIENTATION_EVENTS)
> +        int m_orientation;
> +#endif

Anther place that needs a comment to say what the value means.

I'm a little wary about adding properties to the window object that are not standardized, and don't have the webkit prefix, especially since other browsers are talking about using 'orientation' for events which are related, but different. So I'm not going to r= this without approval from Maciej or Darin.

[Sam Weinig]

> I'm a little wary about adding properties to the window object that are not
> standardized, and don't have the webkit prefix, especially since other browsers
> are talking about using 'orientation' for events which are related, but
> different. So I'm not going to r= this without approval from Maciej or Darin.

I discussed this with Greg and Maciej and we decided that this should go in given that this has already shipped on the iPhone and is behind a #define.

[Simon Fraser (smfr)]

Comment on attachment 39870 [details]
And add it to EventNames.h as well.

r=me with the previously mentioned comments addressed.

[Greg Bolsinga]

http://trac.webkit.org/changeset/48609