Use WebViewClient instead of BlinkPlatform for screen lock.

Use WebViewClient instead of BlinkPlatform for screen lock.

Using BlinkPlatform hides the WebView that requested the lock, making it
very hard to have a clever handling of screen lock on the browser side
given that it's unknown whom and when requested the lock.

It will also allow locking the appropriate screen in a multi-screen
scenario.

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

[mlamouri (slow - plz ping), 2014-06-03 17:35:39]

Chris, could you take a look at this? Thanks :)

[Inactive, 2014-06-03 17:41:59]

On 2014/06/03 17:35:39, Mounir Lamouri wrote:
> Chris, could you take a look at this? Thanks :)

typo in the title :P

[mlamouri (slow - plz ping), 2014-06-03 17:43:33]

On 2014/06/03 17:41:59, Chris Dumez wrote:
> On 2014/06/03 17:35:39, Mounir Lamouri wrote:
> > Chris, could you take a look at this? Thanks :)
> 
> typo in the title :P

You mean instaed -> instead? Coz I fixed that minutes before your reply ;)

[Inactive, 2014-06-03 18:04:52]

https://codereview.chromium.org/315693002/diff/1/Source/modules/screen_orient...
File Source/modules/screen_orientation/ScreenOrientation.cpp (right):

https://codereview.chromium.org/315693002/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientation.cpp:106:
ASSERT(document()->page());
This seems unsafe. We should probably return 0 if frame() is null.

https://codereview.chromium.org/315693002/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientation.cpp:107: return
document()->page();
We should be able to use frame()->page() since this is a DOMWindowProperty, to
avoid going through the Document.

https://codereview.chromium.org/315693002/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientation.cpp:155:
ScreenOrientationController::from(*document->page()).lockOrientation(stringToOrientationLock(lockString),
new LockOrientationCallback(resolver));
I think it is possible for page() to return null if we don't have a Frame. Also,
we should probably use ScreenOrientation::page() that you added.

https://codereview.chromium.org/315693002/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientation.cpp:162: if
(!screenOrientation.page())
That method currently has an assertion making sure it is not null. I think we
should get rid of that assertion and keep this null check.

https://codereview.chromium.org/315693002/diff/1/Source/modules/screen_orient...
File Source/modules/screen_orientation/ScreenOrientationController.cpp (right):

https://codereview.chromium.org/315693002/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientationController.cpp:99:
ASSERT(page());
Why is this safe? This assertion above seems to hint that page() can return null
here. I think we should have a null check:
LocalFrame* mainFrame = page() ? page()->mainFrame() : 0;

https://codereview.chromium.org/315693002/diff/1/public/platform/Platform.h
File public/platform/Platform.h (left):

https://codereview.chromium.org/315693002/diff/1/public/platform/Platform.h#o...
public/platform/Platform.h:647: virtual void
setScreenOrientationListener(blink::WebScreenOrientationListener*) { }
So this is no longer overridden on Chromium side?

https://codereview.chromium.org/315693002/diff/1/public/platform/WebScreenOri...
File public/platform/WebScreenOrientationClient.h (right):

https://codereview.chromium.org/315693002/diff/1/public/platform/WebScreenOri...
public/platform/WebScreenOrientationClient.h:15: class
WebScreenOrientationClient {
No virtual destructor?

[mlamouri (slow - plz ping), 2014-06-03 19:49:30]

Review comments applied. PTAL :)

https://codereview.chromium.org/315693002/diff/1/Source/modules/screen_orient...
File Source/modules/screen_orientation/ScreenOrientation.cpp (right):

https://codereview.chromium.org/315693002/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientation.cpp:106:
ASSERT(document()->page());
On 2014/06/03 18:04:53, Chris Dumez wrote:
> This seems unsafe. We should probably return 0 if frame() is null.

Done.

https://codereview.chromium.org/315693002/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientation.cpp:107: return
document()->page();
On 2014/06/03 18:04:53, Chris Dumez wrote:
> We should be able to use frame()->page() since this is a DOMWindowProperty, to
> avoid going through the Document.

Done.

https://codereview.chromium.org/315693002/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientation.cpp:155:
ScreenOrientationController::from(*document->page()).lockOrientation(stringToOrientationLock(lockString),
new LockOrientationCallback(resolver));
On 2014/06/03 18:04:53, Chris Dumez wrote:
> I think it is possible for page() to return null if we don't have a Frame.
Also,
> we should probably use ScreenOrientation::page() that you added.

Done.

https://codereview.chromium.org/315693002/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientation.cpp:162: if
(!screenOrientation.page())
On 2014/06/03 18:04:53, Chris Dumez wrote:
> That method currently has an assertion making sure it is not null. I think we
> should get rid of that assertion and keep this null check.

Done.

https://codereview.chromium.org/315693002/diff/1/Source/modules/screen_orient...
File Source/modules/screen_orientation/ScreenOrientationController.cpp (right):

https://codereview.chromium.org/315693002/diff/1/Source/modules/screen_orient...
Source/modules/screen_orientation/ScreenOrientationController.cpp:99:
ASSERT(page());
On 2014/06/03 18:04:53, Chris Dumez wrote:
> Why is this safe? This assertion above seems to hint that page() can return
null
> here. I think we should have a null check:
> LocalFrame* mainFrame = page() ? page()->mainFrame() : 0;

Done.

https://codereview.chromium.org/315693002/diff/1/public/platform/Platform.h
File public/platform/Platform.h (left):

https://codereview.chromium.org/315693002/diff/1/public/platform/Platform.h#o...
public/platform/Platform.h:647: virtual void
setScreenOrientationListener(blink::WebScreenOrientationListener*) { }
On 2014/06/03 18:04:53, Chris Dumez wrote:
> So this is no longer overridden on Chromium side?

It is. But it is no longer used. My content/ patch removes it entirely. As you
can see, the tests are fine.

https://codereview.chromium.org/315693002/diff/1/public/platform/WebScreenOri...
File public/platform/WebScreenOrientationClient.h (right):

https://codereview.chromium.org/315693002/diff/1/public/platform/WebScreenOri...
public/platform/WebScreenOrientationClient.h:15: class
WebScreenOrientationClient {
On 2014/06/03 18:04:53, Chris Dumez wrote:
> No virtual destructor?

Done.

[Inactive, 2014-06-03 20:04:40]

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

https://codereview.chromium.org/315693002/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:128: if
(!screenOrientation.document()) {
This check needs to be updated to if (!screenOrientation.page()) otherwise your
dereferencing below is unsafe.

[mlamouri (slow - plz ping), 2014-06-03 20:07:49]

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

https://codereview.chromium.org/315693002/diff/20001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:128: if
(!screenOrientation.document()) {
On 2014/06/03 20:04:40, Chris Dumez wrote:
> This check needs to be updated to if (!screenOrientation.page()) otherwise
your
> dereferencing below is unsafe.

Oups sorry. Done.

[Inactive, 2014-06-03 20:38:32]

lgtm for screen_orientation/ but you'll need OWNERS for other folders.

[eseidel, 2014-06-03 20:57:50]

daniel cheng is probably the right reviewer.

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

https://codereview.chromium.org/315693002/diff/40001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:132:
ScreenOrientationController& controller =
ScreenOrientationController::from(*screenOrientation.page());
I would just have an accessor for the controller instead and then it's a detail
as to if Page or FrameHost is used to access it.

You're right that currently Page is probably used, but it's unclear if Page is
movign to the other side of the embedder or not.  It was planned to (and
FrameHost was going to stay), but it's possible dcheng has revised that plan
since I was involved with OOPI.

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

https://codereview.chromium.org/315693002/diff/40001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.h:45: Page* page() const;
Are you sure you want Page and not FrameHost?

[dcheng, 2014-06-03 21:02:04]

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

https://codereview.chromium.org/315693002/diff/40001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientation.cpp:132:
ScreenOrientationController& controller =
ScreenOrientationController::from(*screenOrientation.page());
On 2014/06/03 20:57:49, eseidel wrote:
> I would just have an accessor for the controller instead and then it's a
detail
> as to if Page or FrameHost is used to access it.
> 
> You're right that currently Page is probably used, but it's unclear if Page is
> movign to the other side of the embedder or not.  It was planned to (and
> FrameHost was going to stay), but it's possible dcheng has revised that plan
> since I was involved with OOPI.

I don't think it's clear what the plan is yet so just doing this is probably the
simplest approach for now. I believe we'll have frame tree mirroring working in
a week or two, and rendering soon after that. At that point, people could try
out OOPI and see how it explodes their feature.

[eseidel, 2014-06-03 21:06:51]

lgtm

So this is moving things from being a detail of how the platform is implemented
to a request to/from the embedder.  Blink is asking up to the embedder above
that it would like to lock the screen at this time.  Previously it was asking
down (also to the embedder) to implment the details of screen locking.

This seems OK.

[mlamouri (slow - plz ping), 2014-06-03 21:22:37]

On 2014/06/03 21:06:51, eseidel wrote:
> lgtm
> 
> So this is moving things from being a detail of how the platform is
implemented
> to a request to/from the embedder.  Blink is asking up to the embedder above
> that it would like to lock the screen at this time.  Previously it was asking
> down (also to the embedder) to implment the details of screen locking.
> 
> This seems OK.

Thank you Daniel and Eric! :)

[mlamouri (slow - plz ping), 2014-06-03 21:23:16]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-06-03 21:23:53]

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

[commit-bot: I haz the power, 2014-06-03 23:16:25]

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

[mlamouri (slow - plz ping), 2014-06-04 10:33:28]

The CQ bit was checked by mlamouri@chromium.org

[commit-bot: I haz the power, 2014-06-04 10:34:19]

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

[commit-bot: I haz the power, 2014-06-04 10:34:50]

Change committed as 175472