Don't use webmediaplayer to FullscreenController about fullscreen change

Don't use webmediaplayer to FullscreenController about fullscreen change

This is cloned from https://codereview.chromium.org/275163002/
The previous change will break webview because AwWebContentsDelegate::ToggleFullscreenModeForTab is not implemented.
This change passes a runtime switch to blink to disable fullscreen for non-media elements for android webview.
when AwWebContentsDelegate::ToggleFullscreenModeForTab is called, android webview will send a FullscreenChanged IPC to finish the will/DidEnterFullscreen event for the media element.

blink side CL: https://codereview.chromium.org/290643005/

This CL only impacts android webview

BUG=367346
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281781

[qinmin, 2014-05-16 20:41:35]

PTAL

[boliu, 2014-05-16 20:43:18]

No need for switches and whatnot. Just modify aw_settings.cc

[boliu, 2014-05-16 21:16:20]

Oh wait, I'm mixing up RuntimeFeatures with WebSettings. But I think you can
still just do it somewhere under android_webview without changing content.

[trchen, 2014-05-16 21:19:33]

On 2014/05/16 21:16:20, boliu wrote:
> Oh wait, I'm mixing up RuntimeFeatures with WebSettings. But I think you can
> still just do it somewhere under android_webview without changing content.

My opinion is that we always prefer command line switch over hardcoded, for
better testability. Unless, say the code won't even compile on Linux.

[boliu, 2014-05-16 21:21:44]

On 2014/05/16 21:19:33, trchen wrote:
> On 2014/05/16 21:16:20, boliu wrote:
> > Oh wait, I'm mixing up RuntimeFeatures with WebSettings. But I think you can
> > still just do it somewhere under android_webview without changing content.
> 
> My opinion is that we always prefer command line switch over hardcoded, for
> better testability. Unless, say the code won't even compile on Linux.

It's already a *Runtime*Feature. You can certainly write tests for this.

If you meant manual testing, then ok. I still think it's not worth it.

[trchen, 2014-05-16 21:25:03]

On 2014/05/16 21:21:44, boliu wrote:
> On 2014/05/16 21:19:33, trchen wrote:
> > On 2014/05/16 21:16:20, boliu wrote:
> > > Oh wait, I'm mixing up RuntimeFeatures with WebSettings. But I think you
can
> > > still just do it somewhere under android_webview without changing content.
> > 
> > My opinion is that we always prefer command line switch over hardcoded, for
> > better testability. Unless, say the code won't even compile on Linux.
> 
> It's already a *Runtime*Feature. You can certainly write tests for this.
> 
> If you meant manual testing, then ok. I still think it's not worth it.

Yes for unit tests, not for layout tests. For layout tests the REFs must be
initialized at content shell entry point. Because they are immutable during
runtime, we can't export a JS function to toggle them.

[qinmin, 2014-05-17 01:28:28]

OK, switched to websettings from content switch. Since webview has its own test
suites, so we can use webview's test to test the setting.

[boliu, 2014-05-27 21:45:50]

https://codereview.chromium.org/292573004/diff/20001/android_webview/native/a...
File android_webview/native/aw_web_contents_delegate.cc (right):

https://codereview.chromium.org/292573004/diff/20001/android_webview/native/a...
android_webview/native/aw_web_contents_delegate.cc:50: :
WebContentsDelegateAndroid(env, obj) {
initialize is_fullscreen_

https://codereview.chromium.org/292573004/diff/20001/android_webview/native/a...
android_webview/native/aw_web_contents_delegate.cc:213:
web_contents->GetRenderViewHost()->WasResized();
So why is this needed now but not before?

Why is WasResized the right response?

Shouldn't we wait until the webview is actually resized instead of sending a
fake resize signal?

[qinmin, 2014-05-28 17:53:38]

https://codereview.chromium.org/292573004/diff/20001/android_webview/native/a...
File android_webview/native/aw_web_contents_delegate.cc (right):

https://codereview.chromium.org/292573004/diff/20001/android_webview/native/a...
android_webview/native/aw_web_contents_delegate.cc:50: :
WebContentsDelegateAndroid(env, obj) {
On 2014/05/27 21:45:50, boliu (OOO_back_June_10) wrote:
> initialize is_fullscreen_

Done.

https://codereview.chromium.org/292573004/diff/20001/android_webview/native/a...
android_webview/native/aw_web_contents_delegate.cc:213:
web_contents->GetRenderViewHost()->WasResized();
Android webview will never get resized because it does not support fullscreen
API.
With https://codereview.chromium.org/290643005/, this call will only be called
when video goes fullscreen.
We call WasResized() to inform RenderViewHost to send an IPC to the renderer
with fullscreen flag set to true (size doesn't change, but the fullscreen flag
changed when RenderViewHost calls IsFullscreenForTabOrPending())


On 2014/05/27 21:45:50, boliu (OOO_back_June_10) wrote:
> So why is this needed now but not before?
> 
> Why is WasResized the right response?
> 
> Shouldn't we wait until the webview is actually resized instead of sending a
> fake resize signal?

[boliu, 2014-06-11 16:16:28]

https://codereview.chromium.org/292573004/diff/20001/android_webview/native/a...
File android_webview/native/aw_web_contents_delegate.cc (right):

https://codereview.chromium.org/292573004/diff/20001/android_webview/native/a...
android_webview/native/aw_web_contents_delegate.cc:213:
web_contents->GetRenderViewHost()->WasResized();
On 2014/05/28 17:53:39, qinmin wrote:
> Android webview will never get resized because it does not support fullscreen
> API.
> With https://codereview.chromium.org/290643005/, this call will only be called
> when video goes fullscreen.
> We call WasResized() to inform RenderViewHost to send an IPC to the renderer
> with fullscreen flag set to true (size doesn't change, but the fullscreen flag
> changed when RenderViewHost calls IsFullscreenForTabOrPending())
> 
> 
> On 2014/05/27 21:45:50, boliu (OOO_back_June_10) wrote:
> > So why is this needed now but not before?
> > 
> > Why is WasResized the right response?
> > 
> > Shouldn't we wait until the webview is actually resized instead of sending a
> > fake resize signal?

This API is so wrong though. The method should be called
GetRenderViewHost::FullscreenChanged, which internally calls WasResized. Or
maybe RVH should keep the state and have SetFullscreen(bool).

With a name like WasResized, someone could accidentally remove the fullscreen
bit and not notice until later.

[qinmin, 2014-06-13 01:47:31]

https://codereview.chromium.org/292573004/diff/20001/android_webview/native/a...
File android_webview/native/aw_web_contents_delegate.cc (right):

https://codereview.chromium.org/292573004/diff/20001/android_webview/native/a...
android_webview/native/aw_web_contents_delegate.cc:213:
web_contents->GetRenderViewHost()->WasResized();
Ok, changed to use a separate IPC to inform the renderer that fullscreen state
has changed.

On 2014/06/11 16:16:28, boliu wrote:
> On 2014/05/28 17:53:39, qinmin wrote:
> > Android webview will never get resized because it does not support
fullscreen
> > API.
> > With https://codereview.chromium.org/290643005/, this call will only be
called
> > when video goes fullscreen.
> > We call WasResized() to inform RenderViewHost to send an IPC to the renderer
> > with fullscreen flag set to true (size doesn't change, but the fullscreen
flag
> > changed when RenderViewHost calls IsFullscreenForTabOrPending())
> > 
> > 
> > On 2014/05/27 21:45:50, boliu (OOO_back_June_10) wrote:
> > > So why is this needed now but not before?
> > > 
> > > Why is WasResized the right response?
> > > 
> > > Shouldn't we wait until the webview is actually resized instead of sending
a
> > > fake resize signal?
> 
> This API is so wrong though. The method should be called
> GetRenderViewHost::FullscreenChanged, which internally calls WasResized. Or
> maybe RVH should keep the state and have SetFullscreen(bool).
> 
> With a name like WasResized, someone could accidentally remove the fullscreen
> bit and not notice until later.

[boliu, 2014-06-13 15:50:58]

https://codereview.chromium.org/292573004/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/292573004/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_view_host_impl.h:221: virtual void
FullscreenChanged(bool is_fullscreen) OVERRIDE;
If you pass is_fullscreen in here, then you don't need
IsFullscreenForTabOrPending on the delegate. Also you added this path but didn't
remove the same code from RenderWidgetHostImpl::WasResized, which is making it
more confusing rather than less...

[qinmin, 2014-06-13 16:40:17]

https://codereview.chromium.org/292573004/diff/80001/content/browser/renderer...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/292573004/diff/80001/content/browser/renderer...
content/browser/renderer_host/render_view_host_impl.h:221: virtual void
FullscreenChanged(bool is_fullscreen) OVERRIDE;
ok, removed the fullscreen changed IPC

On 2014/06/13 15:50:58, boliu wrote:
> If you pass is_fullscreen in here, then you don't need
> IsFullscreenForTabOrPending on the delegate. Also you added this path but
didn't
> remove the same code from RenderWidgetHostImpl::WasResized, which is making it
> more confusing rather than less...

[boliu, 2014-06-13 16:44:02]

lgtm

[qinmin, 2014-06-13 16:50:25]

+jochen for content/ OWNER
+michaeln for webkit/ OWNER

ptal

[michaeln, 2014-06-13 17:58:53]

> +michaeln for webkit/ OWNER

lgtm

[qinmin, 2014-06-16 16:20:04]

+jam@ for content/ OWNERs, jochen is OOO

[jam, 2014-06-16 20:09:06]

https://codereview.chromium.org/292573004/diff/100001/content/public/browser/...
File content/public/browser/render_view_host.h (right):

https://codereview.chromium.org/292573004/diff/100001/content/public/browser/...
content/public/browser/render_view_host.h:228: virtual void FullscreenChanged()
= 0;
why add this method to RVH when all it does is call RWH::WasResized() which is
public? i.e. just have the calling code call WasResized()

[boliu, 2014-06-16 20:12:45]

https://codereview.chromium.org/292573004/diff/100001/content/public/browser/...
File content/public/browser/render_view_host.h (right):

https://codereview.chromium.org/292573004/diff/100001/content/public/browser/...
content/public/browser/render_view_host.h:228: virtual void FullscreenChanged()
= 0;
On 2014/06/16 20:09:06, jam wrote:
> why add this method to RVH when all it does is call RWH::WasResized() which is
> public? i.e. just have the calling code call WasResized()

I asked for it in the review because it made no sense to call WasResized from
webview code where it's clearly *not* resized. WasResized does more than just
updating the size. It also handles the full screen state, and that's the bit
webview is interested in.

[jam, 2014-06-16 22:44:16]

https://codereview.chromium.org/292573004/diff/100001/content/public/browser/...
File content/public/browser/render_view_host.h (right):

https://codereview.chromium.org/292573004/diff/100001/content/public/browser/...
content/public/browser/render_view_host.h:228: virtual void FullscreenChanged()
= 0;
On 2014/06/16 20:12:45, boliu wrote:
> On 2014/06/16 20:09:06, jam wrote:
> > why add this method to RVH when all it does is call RWH::WasResized() which
is
> > public? i.e. just have the calling code call WasResized()
> 
> I asked for it in the review because it made no sense to call WasResized from
> webview code where it's clearly *not* resized. WasResized does more than just
> updating the size. It also handles the full screen state, and that's the bit
> webview is interested in.

I don't understand what you mean. This code adds a new method to RVH that just
calls WasResized(). So I don't see how that's different from calling WasResized
directly?

Unless I'm missing something above, please just use WasResized. Especially since
RVH is going away as part of site isolation, while RWH is not.

[boliu, 2014-06-16 22:49:31]

On 2014/06/16 22:44:16, jam wrote:
>
https://codereview.chromium.org/292573004/diff/100001/content/public/browser/...
> File content/public/browser/render_view_host.h (right):
> 
>
https://codereview.chromium.org/292573004/diff/100001/content/public/browser/...
> content/public/browser/render_view_host.h:228: virtual void
FullscreenChanged()
> = 0;
> On 2014/06/16 20:12:45, boliu wrote:
> > On 2014/06/16 20:09:06, jam wrote:
> > > why add this method to RVH when all it does is call RWH::WasResized()
which
> is
> > > public? i.e. just have the calling code call WasResized()
> > 
> > I asked for it in the review because it made no sense to call WasResized
from
> > webview code where it's clearly *not* resized. WasResized does more than
just
> > updating the size. It also handles the full screen state, and that's the bit
> > webview is interested in.
> 
> I don't understand what you mean. This code adds a new method to RVH that just
> calls WasResized(). So I don't see how that's different from calling
WasResized
> directly?
> 
> Unless I'm missing something above, please just use WasResized. Especially
since
> RVH is going away as part of site isolation, while RWH is not.

WasResized updates both size and fullscreen state. For webview, these two things
can change independently (not the case in chrome I believe?). So from that
perspective, it makes no sense to call "WasResized" when only the fullscreen
state changed in webview, ie this code:
https://codereview.chromium.org/292573004/diff/100001/android_webview/native/...

I guess I don't feel too strongly about this. Sorry Min for wasting some time..

[qinmin, 2014-06-16 23:16:18]

https://codereview.chromium.org/292573004/diff/100001/content/public/browser/...
File content/public/browser/render_view_host.h (right):

https://codereview.chromium.org/292573004/diff/100001/content/public/browser/...
content/public/browser/render_view_host.h:228: virtual void FullscreenChanged()
= 0;
Done. Reverting back to WasResized() now.

On 2014/06/16 22:44:16, jam wrote:
> On 2014/06/16 20:12:45, boliu wrote:
> > On 2014/06/16 20:09:06, jam wrote:
> > > why add this method to RVH when all it does is call RWH::WasResized()
which
> is
> > > public? i.e. just have the calling code call WasResized()
> > 
> > I asked for it in the review because it made no sense to call WasResized
from
> > webview code where it's clearly *not* resized. WasResized does more than
just
> > updating the size. It also handles the full screen state, and that's the bit
> > webview is interested in.
> 
> I don't understand what you mean. This code adds a new method to RVH that just
> calls WasResized(). So I don't see how that's different from calling
WasResized
> directly?
> 
> Unless I'm missing something above, please just use WasResized. Especially
since
> RVH is going away as part of site isolation, while RWH is not.

[jam, 2014-06-17 16:47:01]

lgtm

[qinmin, 2014-07-03 20:24:26]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2014-07-03 20:25:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/qinmin@chromium.org/292573004/140001

[commit-bot: I haz the power, 2014-07-03 23:35:33]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-03 23:41:23]

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

[commit-bot: I haz the power, 2014-07-03 23:41:24]

Try jobs failed on following builders:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)

[qinmin, 2014-07-04 01:43:16]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2014-07-04 01:44:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/qinmin@chromium.org/292573004/160001

[commit-bot: I haz the power, 2014-07-04 05:29:15]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)

[commit-bot: I haz the power, 2014-07-04 05:34:44]

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

[commit-bot: I haz the power, 2014-07-04 05:34:45]

Try jobs failed on following builders:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)

[qinmin, 2014-07-08 16:39:57]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2014-07-08 16:40:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/qinmin@chromium.org/292573004/160001

[commit-bot: I haz the power, 2014-07-08 19:01:19]

Change committed as 281781