Bug 29508 - Add ENABLE(ORIENTATION_EVENTS)
Summary: Add ENABLE(ORIENTATION_EVENTS)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Greg Bolsinga
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-18 14:59 PDT by Greg Bolsinga
Modified: 2009-09-21 16:10 PDT (History)
2 users (show)

See Also:


Attachments
patch to implement bug fix (21.49 KB, patch)
2009-09-18 15:10 PDT, Greg Bolsinga
sam: review-
Details | Formatted Diff | Diff
address Sam's review comments (21.97 KB, patch)
2009-09-21 12:11 PDT, Greg Bolsinga
no flags Details | Formatted Diff | Diff
Add onorientationchange to HTMLAttributeNames.in (22.44 KB, patch)
2009-09-21 13:13 PDT, Greg Bolsinga
no flags Details | Formatted Diff | Diff
And add it to EventNames.h as well. (22.91 KB, patch)
2009-09-21 13:37 PDT, Greg Bolsinga
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Greg Bolsinga 2009-09-18 14:59:45 PDT
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
Comment 1 Greg Bolsinga 2009-09-18 15:10:36 PDT
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.
Comment 2 Sam Weinig 2009-09-18 16:49:46 PDT
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.
Comment 3 Greg Bolsinga 2009-09-18 17:25:37 PDT
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.
Comment 4 Sam Weinig 2009-09-20 09:20:36 PDT
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-
Comment 5 Greg Bolsinga 2009-09-21 12:04:13 PDT
(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.
Comment 6 Greg Bolsinga 2009-09-21 12:11:47 PDT
Created attachment 39863 [details]
address Sam's review comments
Comment 7 Greg Bolsinga 2009-09-21 13:13:38 PDT
Created attachment 39867 [details]
Add onorientationchange to HTMLAttributeNames.in
Comment 8 Greg Bolsinga 2009-09-21 13:37:48 PDT
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.
Comment 9 Simon Fraser (smfr) 2009-09-21 14:07:46 PDT
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.
Comment 10 Sam Weinig 2009-09-21 15:38:31 PDT
> 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.
Comment 11 Simon Fraser (smfr) 2009-09-21 16:03:39 PDT
Comment on attachment 39870 [details]
And add it to EventNames.h as well.

r=me with the previously mentioned comments addressed.
Comment 12 Greg Bolsinga 2009-09-21 16:10:59 PDT
http://trac.webkit.org/changeset/48609