Use Promises for screen.lockOrientation().

Use Promises for screen.lockOrientation().

This change is changing the Blink implementation and adds the Platform
interface but changes in the content layer a required to make it work
fully.

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

[mlamouri (slow - plz ping), 2014-05-15 17:26:30]

Christophe, could you review the changes in:
Source/modules/screen_orientation/

abarth@, could you review the changes in:
Source/web/
public/platform/

Thanks :)

[Inactive, 2014-05-16 13:25:25]

Adam is much more familiar than me with Promises so I'd like him to review this.

https://codereview.chromium.org/283423005/diff/1/Source/modules/screen_orient...
File Source/modules/screen_orientation/LockOrientationCallback.h (right):

https://codereview.chromium.org/283423005/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/LockOrientationCallback.h:22: class
LockOrientationCallback FINAL : public
blink::WebCallbacks<blink::WebScreenOrientationType, blink::WebDOMException> {
Looks like we could reuse CallbackPromiseAdapter ?

[mlamouri (slow - plz ping), 2014-05-21 13:45:02]

https://codereview.chromium.org/283423005/diff/1/Source/modules/screen_orient...
File Source/modules/screen_orientation/LockOrientationCallback.h (right):

https://codereview.chromium.org/283423005/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/LockOrientationCallback.h:22: class
LockOrientationCallback FINAL : public
blink::WebCallbacks<blink::WebScreenOrientationType, blink::WebDOMException> {
On 2014/05/16 13:25:26, Chris Dumez wrote:
> Looks like we could reuse CallbackPromiseAdapter ?

That would require me to have a wrapper for WebScreenOrientationType and
WebDOMException. That is too much overhead IMO.

[Inactive, 2014-05-21 13:48:07]

https://codereview.chromium.org/283423005/diff/1/Source/modules/screen_orient...
File Source/modules/screen_orientation/LockOrientationCallback.h (right):

https://codereview.chromium.org/283423005/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/LockOrientationCallback.h:22: class
LockOrientationCallback FINAL : public
blink::WebCallbacks<blink::WebScreenOrientationType, blink::WebDOMException> {
On 2014/05/21 13:45:02, Mounir Lamouri wrote:
> On 2014/05/16 13:25:26, Chris Dumez wrote:
> > Looks like we could reuse CallbackPromiseAdapter ?
> 
> That would require me to have a wrapper for WebScreenOrientationType and
> WebDOMException. That is too much overhead IMO.

I don't understand, why cannot we use CallbackPromiseAdapter<blink::WebScree
    nOrientationType, blink::WebDOMException> type instead of this class? They
seem identical.

[mlamouri (slow - plz ping), 2014-05-21 15:51:33]

Chris, could you have a look at the screen_orientation/ changes?

yhirano@, could you have a look at the usage of Promises?

jochen@, could you have a look at the change in public/platform/?

[Inactive, 2014-05-21 16:01:45]

lgtm with nit.

https://codereview.chromium.org/283423005/diff/10001/Source/modules/screen_or...
File Source/modules/screen_orientation/LockOrientationCallback.h (right):

https://codereview.chromium.org/283423005/diff/10001/Source/modules/screen_or...
Source/modules/screen_orientation/LockOrientationCallback.h:23:
LockOrientationCallback(PassRefPtr<ScriptPromiseResolverWithContext>);
explicit?

[Inactive, 2014-05-21 16:02:50]

https://codereview.chromium.org/283423005/diff/10001/Source/modules/screen_or...
File Source/modules/screen_orientation/LockOrientationCallback.h (right):

https://codereview.chromium.org/283423005/diff/10001/Source/modules/screen_or...
Source/modules/screen_orientation/LockOrientationCallback.h:31:
WTF_MAKE_NONCOPYABLE(LockOrientationCallback);
nit: I think we usually have this one at the top of the class, right before
public: in Blink.

[mlamouri (slow - plz ping), 2014-05-21 16:14:37]

Thanks for the quick review Chris, I've updated the patch accordingly.

[yhirano, 2014-05-22 03:48:43]

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

https://codereview.chromium.org/283423005/diff/30001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.h:22: class ScriptPromise;
I think this forward declaration is meaningless because you are using
ScriptPromise as a return type.

https://codereview.chromium.org/283423005/diff/30001/Source/modules/screen_or...
File Source/modules/screen_orientation/ScreenOrientation.idl (right):

https://codereview.chromium.org/283423005/diff/30001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.idl:27:
[CallWith=ExecutionContext] Promise lockOrientation(OrientationLockType
orientation);
You can use CallWith=ScriptState

[jochen (gone - plz use gerrit), 2014-05-22 08:34:34]

https://codereview.chromium.org/283423005/diff/30001/LayoutTests/screen_orien...
File LayoutTests/screen_orientation/lockOrientation-basic.html (right):

https://codereview.chromium.org/283423005/diff/30001/LayoutTests/screen_orien...
LayoutTests/screen_orientation/lockOrientation-basic.html:11: var caught =
false;
4 space indent plz

https://codereview.chromium.org/283423005/diff/30001/LayoutTests/screen_orien...
File LayoutTests/screen_orientation/resources/sandboxed-iframe-locking.html
(right):

https://codereview.chromium.org/283423005/diff/30001/LayoutTests/screen_orien...
LayoutTests/screen_orientation/resources/sandboxed-iframe-locking.html:1:
<script>
please add <!DOCTYPE html>

https://codereview.chromium.org/283423005/diff/30001/Source/modules/screen_or...
File Source/modules/screen_orientation/LockOrientationCallback.cpp (right):

https://codereview.chromium.org/283423005/diff/30001/Source/modules/screen_or...
Source/modules/screen_orientation/LockOrientationCallback.cpp:50: default:
we usually omit the default branch so the compiler complains if you add a new
enum

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

https://codereview.chromium.org/283423005/diff/30001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:130: ExecutionContext*
context, Screen& screen, const AtomicString& lockString)
should go on previous line

https://codereview.chromium.org/283423005/diff/30001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:140:
DOMException::create(InvalidStateError, "The object is no longer associated to a
document.");
should go on previous line

https://codereview.chromium.org/283423005/diff/30001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:147:
DOMException::create(SecurityError, "The document is sandboxed and lacks the
'allow-orientation-lock' flag.");
and here

https://codereview.chromium.org/283423005/diff/30001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:154: new
LockOrientationCallback(resolver));
and this, all in one line

https://codereview.chromium.org/283423005/diff/30001/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/283423005/diff/30001/public/platform/Platform...
public/platform/Platform.h:638: virtual void
lockOrientation(WebScreenOrientationLockType) { }
do you still need this one?

https://codereview.chromium.org/283423005/diff/30001/public/platform/WebLockO...
File public/platform/WebLockOrientationCallback.h (right):

https://codereview.chromium.org/283423005/diff/30001/public/platform/WebLockO...
public/platform/WebLockOrientationCallback.h:20: NotAvailable, // If locking
isn't available on the platform.
enums should start all with the enum's name, i.e. ErrorTypeNotAvailable

[mlamouri (slow - plz ping), 2014-05-22 08:58:56]

Review comments applied. PTAL

https://codereview.chromium.org/283423005/diff/30001/LayoutTests/screen_orien...
File LayoutTests/screen_orientation/lockOrientation-basic.html (right):

https://codereview.chromium.org/283423005/diff/30001/LayoutTests/screen_orien...
LayoutTests/screen_orientation/lockOrientation-basic.html:11: var caught =
false;
On 2014/05/22 08:34:35, jochen wrote:
> 4 space indent plz

Done.

https://codereview.chromium.org/283423005/diff/30001/LayoutTests/screen_orien...
File LayoutTests/screen_orientation/resources/sandboxed-iframe-locking.html
(right):

https://codereview.chromium.org/283423005/diff/30001/LayoutTests/screen_orien...
LayoutTests/screen_orientation/resources/sandboxed-iframe-locking.html:1:
<script>
On 2014/05/22 08:34:35, jochen wrote:
> please add <!DOCTYPE html>

Done.

https://codereview.chromium.org/283423005/diff/30001/Source/modules/screen_or...
File Source/modules/screen_orientation/LockOrientationCallback.cpp (right):

https://codereview.chromium.org/283423005/diff/30001/Source/modules/screen_or...
Source/modules/screen_orientation/LockOrientationCallback.cpp:50: default:
On 2014/05/22 08:34:35, jochen wrote:
> we usually omit the default branch so the compiler complains if you add a new
> enum

Done.

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

https://codereview.chromium.org/283423005/diff/30001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:130: ExecutionContext*
context, Screen& screen, const AtomicString& lockString)
On 2014/05/22 08:34:35, jochen wrote:
> should go on previous line

Done. Though, that makes the line ~120 characters long.

https://codereview.chromium.org/283423005/diff/30001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:140:
DOMException::create(InvalidStateError, "The object is no longer associated to a
document.");
On 2014/05/22 08:34:35, jochen wrote:
> should go on previous line

Done. 140 characters long :(

https://codereview.chromium.org/283423005/diff/30001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:147:
DOMException::create(SecurityError, "The document is sandboxed and lacks the
'allow-orientation-lock' flag.");
On 2014/05/22 08:34:35, jochen wrote:
> and here

160 :'(

https://codereview.chromium.org/283423005/diff/30001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:154: new
LockOrientationCallback(resolver));
On 2014/05/22 08:34:35, jochen wrote:
> and this, all in one line

Obviously, having vertical screen is not compatible with working on Blink ;)

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

https://codereview.chromium.org/283423005/diff/30001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.h:22: class ScriptPromise;
On 2014/05/22 03:48:43, yhirano wrote:
> I think this forward declaration is meaningless because you are using
> ScriptPromise as a return type.

The forward declaration appears to be needed. It does not compile otherwise.

https://codereview.chromium.org/283423005/diff/30001/Source/modules/screen_or...
File Source/modules/screen_orientation/ScreenOrientation.idl (right):

https://codereview.chromium.org/283423005/diff/30001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.idl:27:
[CallWith=ExecutionContext] Promise lockOrientation(OrientationLockType
orientation);
On 2014/05/22 03:48:43, yhirano wrote:
> You can use CallWith=ScriptState

Done.

https://codereview.chromium.org/283423005/diff/30001/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/283423005/diff/30001/public/platform/Platform...
public/platform/Platform.h:638: virtual void
lockOrientation(WebScreenOrientationLockType) { }
On 2014/05/22 08:34:35, jochen wrote:
> do you still need this one?

Chrome OVERRIDE this call so I need to keep this here until I remove the
OVERRIDE, right?

https://codereview.chromium.org/283423005/diff/30001/public/platform/WebLockO...
File public/platform/WebLockOrientationCallback.h (right):

https://codereview.chromium.org/283423005/diff/30001/public/platform/WebLockO...
public/platform/WebLockOrientationCallback.h:20: NotAvailable, // If locking
isn't available on the platform.
On 2014/05/22 08:34:35, jochen wrote:
> enums should start all with the enum's name, i.e. ErrorTypeNotAvailable

Done.

[yhirano, 2014-05-23 04:44:58]

lgtm for the Promises usage.

[esprehn, 2014-05-23 04:47:09]

btw, It's nice to link to the section of the spec that says it returns a Promise
in your change.

[jochen (gone - plz use gerrit), 2014-05-23 07:43:26]

https://codereview.chromium.org/283423005/diff/30001/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/283423005/diff/30001/public/platform/Platform...
public/platform/Platform.h:638: virtual void
lockOrientation(WebScreenOrientationLockType) { }
On 2014/05/22 08:58:56, Mounir Lamouri wrote:
> On 2014/05/22 08:34:35, jochen wrote:
> > do you still need this one?
> 
> Chrome OVERRIDE this call so I need to keep this here until I remove the
> OVERRIDE, right?

we should not use OVERRIDE on implementations of blink API.

Please first land the content side changes (add the new method there, remove
OVERRIDEs) so it will be possible to remove the obsolete callback here.

[jochen (gone - plz use gerrit), 2014-05-23 07:49:37]

other than that, lgtm

[mlamouri (slow - plz ping), 2014-05-23 14:38:29]

On 2014/05/23 07:49:37, jochen wrote:
> other than that, lgtm

This is a CL to remove the OVERRIDE (and do that for the entire file + PRESUBMIT
script):
https://codereview.chromium.org/296133015/

I will land this CL when that other one will be in.

[mlamouri (slow - plz ping), 2014-05-25 13:04:37]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-05-25 13:04:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlamouri@chromium.org/283423005/50001

[commit-bot: I haz the power, 2014-05-25 14:24:32]

Change committed as 174773