Add initial Screen Orientation API support: IDL changes

Add initial Screen Orientation API support: IDL changes

Add initial Screen Orientation API support: IDL changes:
https://dvcs.w3.org/hg/screen-orientation/raw-file/tip/Overview.html

This new API is implemented behind a "ScreenOrientation" runtime-enabled flag.
The intent to implement was sent to blink-dev:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/lgUq8K9IGlU

This CL changes Screen to be an EventTarget. While the Screen Orientation API
has been implemented behind a RuntimeEnabled flag, there is no way of having
inheritance depend on the flag's value. The Web exposed effect of this is that
people will now be able to dispatch custom events on the window.screen
interface, which in itself is harmless.

R=abarth
BUG=162827
TEST=screen_orientation/screenorientation-api.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167259

[Inactive, 2014-02-16 02:01:54]

https://codereview.chromium.org/168763002/diff/50001/Source/core/frame/Screen...
File Source/core/frame/Screen.cpp (right):

https://codereview.chromium.org/168763002/diff/50001/Source/core/frame/Screen...
Source/core/frame/Screen.cpp:131: return 0;
You asked about the risk of returning 0 here. I actually used the same
implementation I found in other places (e.g. Performance or ApplicationCache).

I looked at the callers of executionContext(), e.g. in EventTarget.cpp, and they
seem to have null checks in place.

https://codereview.chromium.org/168763002/diff/50001/Source/modules/screen_or...
File Source/modules/screen_orientation/ScreenOrientation.h (right):

https://codereview.chromium.org/168763002/diff/50001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.h:20: class
ScreenOrientation FINAL : public Supplement<Screen>, ActiveDOMObject {
Based on your comments on my other CL, I kept only ActiveDOMObject. Please let
me know if you think this is not the right thing to do.

[abarth-chromium, 2014-02-16 04:03:48]

Thanks for breaking this down into smaller pieces.  This CL is a good,
digestible size.  A few comments below.  Hopefully we'll be able to land the
next iteration.

https://codereview.chromium.org/168763002/diff/50001/Source/core/frame/Screen...
File Source/core/frame/Screen.cpp (right):

https://codereview.chromium.org/168763002/diff/50001/Source/core/frame/Screen...
Source/core/frame/Screen.cpp:131: return 0;
On 2014/02/16 02:01:57, Chris Dumez wrote:
> You asked about the risk of returning 0 here. I actually used the same
> implementation I found in other places (e.g. Performance or ApplicationCache).
> 
> I looked at the callers of executionContext(), e.g. in EventTarget.cpp, and
they
> seem to have null checks in place.

Thanks!

https://codereview.chromium.org/168763002/diff/50001/Source/core/frame/Screen...
File Source/core/frame/Screen.idl (right):

https://codereview.chromium.org/168763002/diff/50001/Source/core/frame/Screen...
Source/core/frame/Screen.idl:30: interface Screen : EventTarget {
Unfortunately, this part of the change isn't guarded by a runtime flag...  Maybe
that's ok...

https://codereview.chromium.org/168763002/diff/50001/Source/modules/screen_or...
File Source/modules/screen_orientation/OWNERS (right):

https://codereview.chromium.org/168763002/diff/50001/Source/modules/screen_or...
Source/modules/screen_orientation/OWNERS:3: peter@chromium.org
I'd like to review a few more of these CLs before adding OWNERS to this
directory.  Some of these folks approved the previous CL, which contained a
number of errors...

https://codereview.chromium.org/168763002/diff/50001/Source/modules/screen_or...
File Source/modules/screen_orientation/ScreenOrientation.cpp (right):

https://codereview.chromium.org/168763002/diff/50001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:32:
supplement->suspendIfNeeded();
These should be in a static create function.

However, once you move to DOMWindowProperty, you won't have this problem.

https://codereview.chromium.org/168763002/diff/50001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:40: 
You've got an extra blank line here.

https://codereview.chromium.org/168763002/diff/50001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:49: return
window->screen();
Once you're a DOMWindowProperty, this will change a bit too.

https://codereview.chromium.org/168763002/diff/50001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:80: }
This function don't appear to have any callers.  Can we hold off on adding it
until we add the first caller?  That will help remind me to check whether we
really want to dispatch the event synchronously.

https://codereview.chromium.org/168763002/diff/50001/Source/modules/screen_or...
File Source/modules/screen_orientation/ScreenOrientation.h (right):

https://codereview.chromium.org/168763002/diff/50001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.h:20: class
ScreenOrientation FINAL : public Supplement<Screen>, ActiveDOMObject {
On 2014/02/16 02:01:57, Chris Dumez wrote:
> Based on your comments on my other CL, I kept only ActiveDOMObject. Please let
> me know if you think this is not the right thing to do.

Given that you're supplementing Screen, we might want to start with
DOMWindowProperty because Screen is a DOMWindowProperty.

Really, we should merge a bunch of these observer interfaces.  There isn't any
reason for DOMWindowProperty and ActiveDOMObject to be separate concepts now
that we've fixed DOMWindow's lifetime.

https://codereview.chromium.org/168763002/diff/50001/Source/platform/RuntimeE...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/168763002/diff/50001/Source/platform/RuntimeE...
Source/platform/RuntimeEnabledFeatures.in:92: ScreenOrientation
status=experimental
Let's start off with status=test.  We can move up to experimental once we've got
the basics of the API working.

[Inactive, 2014-02-16 04:35:55]

Thanks for all the feedback.

https://codereview.chromium.org/168763002/diff/50001/Source/core/frame/Screen...
File Source/core/frame/Screen.idl (right):

https://codereview.chromium.org/168763002/diff/50001/Source/core/frame/Screen...
Source/core/frame/Screen.idl:30: interface Screen : EventTarget {
On 2014/02/16 04:03:48, abarth wrote:
> Unfortunately, this part of the change isn't guarded by a runtime flag... 
Maybe
> that's ok...

Yes, unfortunately, I don't think we can guard such thing with a runtime flag.

https://codereview.chromium.org/168763002/diff/50001/Source/modules/screen_or...
File Source/modules/screen_orientation/OWNERS (right):

https://codereview.chromium.org/168763002/diff/50001/Source/modules/screen_or...
Source/modules/screen_orientation/OWNERS:3: peter@chromium.org
On 2014/02/16 04:03:48, abarth wrote:
> I'd like to review a few more of these CLs before adding OWNERS to this
> directory.  Some of these folks approved the previous CL, which contained a
> number of errors...

Done.

https://codereview.chromium.org/168763002/diff/50001/Source/modules/screen_or...
File Source/modules/screen_orientation/ScreenOrientation.cpp (right):

https://codereview.chromium.org/168763002/diff/50001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:32:
supplement->suspendIfNeeded();
On 2014/02/16 04:03:48, abarth wrote:
> These should be in a static create function.
> 
> However, once you move to DOMWindowProperty, you won't have this problem.

Done.

https://codereview.chromium.org/168763002/diff/50001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:40: 
On 2014/02/16 04:03:48, abarth wrote:
> You've got an extra blank line here.

Done.

https://codereview.chromium.org/168763002/diff/50001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:49: return
window->screen();
On 2014/02/16 04:03:48, abarth wrote:
> Once you're a DOMWindowProperty, this will change a bit too.

Done.

https://codereview.chromium.org/168763002/diff/50001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:80: }
On 2014/02/16 04:03:48, abarth wrote:
> This function don't appear to have any callers.  Can we hold off on adding it
> until we add the first caller?  That will help remind me to check whether we
> really want to dispatch the event synchronously.

Done.

https://codereview.chromium.org/168763002/diff/50001/Source/modules/screen_or...
File Source/modules/screen_orientation/ScreenOrientation.h (right):

https://codereview.chromium.org/168763002/diff/50001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.h:20: class
ScreenOrientation FINAL : public Supplement<Screen>, ActiveDOMObject {
On 2014/02/16 04:03:48, abarth wrote:
> On 2014/02/16 02:01:57, Chris Dumez wrote:
> > Based on your comments on my other CL, I kept only ActiveDOMObject. Please
let
> > me know if you think this is not the right thing to do.
> 
> Given that you're supplementing Screen, we might want to start with
> DOMWindowProperty because Screen is a DOMWindowProperty.
> 
> Really, we should merge a bunch of these observer interfaces.  There isn't any
> reason for DOMWindowProperty and ActiveDOMObject to be separate concepts now
> that we've fixed DOMWindow's lifetime.

Done.

https://codereview.chromium.org/168763002/diff/50001/Source/platform/RuntimeE...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/168763002/diff/50001/Source/platform/RuntimeE...
Source/platform/RuntimeEnabledFeatures.in:92: ScreenOrientation
status=experimental
On 2014/02/16 04:03:48, abarth wrote:
> Let's start off with status=test.  We can move up to experimental once we've
got
> the basics of the API working.

Done.

[abarth-chromium, 2014-02-16 09:14:11]

LGTM

Thanks for iterating on this CL!

https://codereview.chromium.org/168763002/diff/160001/Source/core/frame/Screen.h
File Source/core/frame/Screen.h (left):

https://codereview.chromium.org/168763002/diff/160001/Source/core/frame/Scree...
Source/core/frame/Screen.h:44: static PassRefPtr<Screen> create(Frame *frame) {
return adoptRef(new Screen(frame)); }
"Frame *frame"  ->  "Frame* frame"

https://codereview.chromium.org/168763002/diff/160001/Source/modules/screen_o...
File Source/modules/screen_orientation/ScreenOrientation.cpp (right):

https://codereview.chromium.org/168763002/diff/160001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.cpp:6: #include
"ScreenOrientation.h"
Please use paths relative to Source when including files.

https://codereview.chromium.org/168763002/diff/160001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.cpp:42: ASSERT(frame);
It's possible for frame to be zero here, but it's fine to make that an error. 
We just need to be careful when calling this function.

https://codereview.chromium.org/168763002/diff/160001/Source/modules/screen_o...
File Source/modules/screen_orientation/ScreenOrientation.h (right):

https://codereview.chromium.org/168763002/diff/160001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.h:5: 
You've got an extra blank line here.

[Inactive, 2014-02-16 14:09:13]

https://codereview.chromium.org/168763002/diff/160001/Source/core/frame/Screen.h
File Source/core/frame/Screen.h (left):

https://codereview.chromium.org/168763002/diff/160001/Source/core/frame/Scree...
Source/core/frame/Screen.h:44: static PassRefPtr<Screen> create(Frame *frame) {
return adoptRef(new Screen(frame)); }
On 2014/02/16 09:14:11, abarth wrote:
> "Frame *frame"  ->  "Frame* frame"

Done.

https://codereview.chromium.org/168763002/diff/160001/Source/modules/screen_o...
File Source/modules/screen_orientation/ScreenOrientation.cpp (right):

https://codereview.chromium.org/168763002/diff/160001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.cpp:6: #include
"ScreenOrientation.h"
On 2014/02/16 09:14:11, abarth wrote:
> Please use paths relative to Source when including files.

Done.

https://codereview.chromium.org/168763002/diff/160001/Source/modules/screen_o...
File Source/modules/screen_orientation/ScreenOrientation.h (right):

https://codereview.chromium.org/168763002/diff/160001/Source/modules/screen_o...
Source/modules/screen_orientation/ScreenOrientation.h:5: 
On 2014/02/16 09:14:11, abarth wrote:
> You've got an extra blank line here.

Done.

[Inactive, 2014-02-16 14:09:24]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-02-16 14:09:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/168763002/260001

[commit-bot: I haz the power, 2014-02-16 15:26:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-16 15:26:19]

Retried try job too often on mac_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

[Inactive, 2014-02-16 23:39:46]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-02-16 23:40:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/168763002/260001

[commit-bot: I haz the power, 2014-02-17 02:56:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-17 02:56:16]

Retried try job too often on linux_blink for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[Inactive, 2014-02-17 03:13:39]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-02-17 03:13:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/168763002/260001

[commit-bot: I haz the power, 2014-02-17 04:12:58]

Change committed as 167259