[aw] Reuse WebContents PowerSaveBlocker in fullscreen.

[aw] Transfer anchor views between container views.

The WebView swaps container views when transitioning
from/to fullscreen. This change ensures that
anchor views are transferred across container views too.

We also remove the PowerSaveBlocker attached to the
ContentVideoView because as a result it is not longer
needed. We now reuse the PowerSaveBlocker attached to
the WebContents which is implemented as an anchor
view. This has a number of advantages:
1. The PowerSaveBlocker now works correctly between
   transitions from/to fullscreen.
2. A PowerSaveBlocker is now also created for video
   elements contained inside a div (for these a
   ContentVideoView is not created when going
   fullscreen).

This change depends on:
https://codereview.chromium.org/639923003/

This change also adds comprehensive tests to verify the
new behaviour. For this we need a longer video, so we
replace the old one-frame test video. We also refactor
the tests to remove flakiness and unnecessary complexity.

BUG=398485
Committed: https://crrev.com/c698e079008a03048933818525310fb34908a10a
Cr-Commit-Position: refs/heads/master@{#302094}

[Ignacio Solla, 2014-10-10 14:39:25]

igsolla@chromium.org changed reviewers:
+ jam@chromium.org, michaelbai@chromium.org, miguelg@chromium.org

[Ignacio Solla, 2014-10-10 14:39:26]

jam@chromium.org: Please review changes in content/

miguelg@chromium.org: Please review changes in ui/android

michaelbai@chromium.org: Please review changes in android_webview/

[Ignacio Solla, 2014-10-10 15:21:56]

On 2014/10/10 14:39:26, Ignacio Solla wrote:
> mailto:jam@chromium.org: Please review changes in content/
> 
> mailto:miguelg@chromium.org: Please review changes in ui/android
> 
> mailto:michaelbai@chromium.org: Please review changes in android_webview/

If you need some more context about this change you might find these documents
useful:
https://docs.google.com/a/google.com/document/d/1y4HAXvx6WqBPu8gKVSDgwG6zW4pl...
https://docs.google.com/a/google.com/document/d/1Yxz8vPks-rZff5SNk2xPM-mxGnGu...

[Ignacio Solla, 2014-10-10 18:14:04]

https://codereview.chromium.org/639413002/diff/20001/android_webview/javatest...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java
(left):

https://codereview.chromium.org/639413002/diff/20001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java:78:
if (webServer != null) webServer.getTestWebServer().shutdown();
Note to  myself: add shutdown back

[Ignacio Solla, 2014-10-11 09:57:47]

igsolla@chromium.org changed reviewers:
+ sievers@chromium.org
- jam@chromium.org

[Ignacio Solla, 2014-10-11 09:58:39]

On 2014/10/10 14:39:26, Ignacio Solla wrote:
> mailto:jam@chromium.org: Please review changes in content/
> 
> mailto:miguelg@chromium.org: Please review changes in ui/android
> 
> mailto:michaelbai@chromium.org: Please review changes in android_webview/

added sievers instead of jam for content/

[no sievers, 2014-10-14 21:51:34]

looks fine as long as michaelbai is happy

https://codereview.chromium.org/639413002/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/639413002/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:514:
public void anchorAnchorView(View anchorView) {
nit: maybe addAnchorView() would be a better name

https://codereview.chromium.org/639413002/diff/20001/content/public/test/andr...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java
(right):

https://codereview.chromium.org/639413002/diff/20001/content/public/test/andr...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java:31:
sb.append("  if (!video) return null;");
nit: if (video) video.pause();

https://codereview.chromium.org/639413002/diff/20001/content/public/test/andr...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java:81:
if (jsonText.trim().equalsIgnoreCase("undefined")) {
When does this need to be silently handled? Should it blow up instead?

[Ignacio Solla, 2014-10-15 11:43:27]

https://codereview.chromium.org/639413002/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/639413002/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:514:
public void anchorAnchorView(View anchorView) {
On 2014/10/14 21:51:34, sievers wrote:
> nit: maybe addAnchorView() would be a better name

Done.

https://codereview.chromium.org/639413002/diff/20001/content/public/test/andr...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java
(right):

https://codereview.chromium.org/639413002/diff/20001/content/public/test/andr...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java:31:
sb.append("  if (!video) return null;");
On 2014/10/14 21:51:34, sievers wrote:
> nit: if (video) video.pause();

Done.

https://codereview.chromium.org/639413002/diff/20001/content/public/test/andr...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/DOMUtils.java:81:
if (jsonText.trim().equalsIgnoreCase("undefined")) {
On 2014/10/14 21:51:34, sievers wrote:
> When does this need to be silently handled? Should it blow up instead?

Good catch, not needed. If undefined blowing up is better.

[michaelbai, 2014-10-15 18:03:27]

Could you explain why don't you create new PowerSaveBlocker when it needed,
instead of reusing the existing one? It looks to me this make code unnecessary
complicated.

[Ignacio Solla, 2014-10-15 18:26:13]

On 2014/10/15 18:03:27, michaelbai wrote:
> Could you explain why don't you create new PowerSaveBlocker when it needed,
> instead of reusing the existing one? It looks to me this make code unnecessary
> complicated.

Creating a PowerSaveBlocker when needed would be much more complicated because
we will need to check the playing state after entering/exiting fullscreen to
decide whether a PowerSaveBlocker needs to be created/destroyed if present.

Take a look at these tests:

doTestPowerSaveBlockerIsEnabledDuringFullscreenPlayback
testPowerSaveBlockerIsTransferredToFullscreen
testPowerSaveBlockerIsTransferredToEmbedded 

Transferring a PowerSaveBlocker if present is easier than having to implement
these rules:
-> After entering fullscreen:
   - If the video is playing create a fullscreen PowerSaveBlocker
   - If the video is not playing do nothing
-> After exiting fullscreen:
   - If the video is playing create an embedded PowerSaveBlocker if one does not
already exist
   - If the video is not playing destroy the embedded PowerSaveBlocker if one
exists

And note that checking whether the video is playing is by itself something that
we should really not be doing at this level.

[michaelbai, 2014-10-15 18:38:33]

On 2014/10/15 18:26:13, Ignacio Solla wrote:
> On 2014/10/15 18:03:27, michaelbai wrote:
> > Could you explain why don't you create new PowerSaveBlocker when it needed,
> > instead of reusing the existing one? It looks to me this make code
unnecessary
> > complicated.
> 
> Creating a PowerSaveBlocker when needed would be much more complicated because
> we will need to check the playing state after entering/exiting fullscreen to
> decide whether a PowerSaveBlocker needs to be created/destroyed if present.
> 
> Take a look at these tests:
> 
> doTestPowerSaveBlockerIsEnabledDuringFullscreenPlayback
> testPowerSaveBlockerIsTransferredToFullscreen
> testPowerSaveBlockerIsTransferredToEmbedded 
> 
> Transferring a PowerSaveBlocker if present is easier than having to implement
> these rules:
> -> After entering fullscreen:
>    - If the video is playing create a fullscreen PowerSaveBlocker
>    - If the video is not playing do nothing
> -> After exiting fullscreen:
>    - If the video is playing create an embedded PowerSaveBlocker if one does
not
> already exist
>    - If the video is not playing destroy the embedded PowerSaveBlocker if one
> exists
> 
> And note that checking whether the video is playing is by itself something
that
> we should really not be doing at this level.



First of all, PowerSaveBlocker is not designed to just work with video playback,
and I want
to keep it consistent with other platform.

For your use case, could you just replace 'transfer' with 'destory old
one/create new one'?

[Ignacio Solla, 2014-10-15 18:51:16]

On 2014/10/15 18:38:33, michaelbai wrote:
> On 2014/10/15 18:26:13, Ignacio Solla wrote:
> > On 2014/10/15 18:03:27, michaelbai wrote:
> > > Could you explain why don't you create new PowerSaveBlocker when it
needed,
> > > instead of reusing the existing one? It looks to me this make code
> unnecessary
> > > complicated.
> > 
> > Creating a PowerSaveBlocker when needed would be much more complicated
because
> > we will need to check the playing state after entering/exiting fullscreen to
> > decide whether a PowerSaveBlocker needs to be created/destroyed if present.
> > 
> > Take a look at these tests:
> > 
> > doTestPowerSaveBlockerIsEnabledDuringFullscreenPlayback
> > testPowerSaveBlockerIsTransferredToFullscreen
> > testPowerSaveBlockerIsTransferredToEmbedded 
> > 
> > Transferring a PowerSaveBlocker if present is easier than having to
implement
> > these rules:
> > -> After entering fullscreen:
> >    - If the video is playing create a fullscreen PowerSaveBlocker
> >    - If the video is not playing do nothing
> > -> After exiting fullscreen:
> >    - If the video is playing create an embedded PowerSaveBlocker if one does
> not
> > already exist
> >    - If the video is not playing destroy the embedded PowerSaveBlocker if
one
> > exists
> > 
> > And note that checking whether the video is playing is by itself something
> that
> > we should really not be doing at this level.
> 
> 
> 
> First of all, PowerSaveBlocker is not designed to just work with video
playback,
> and I want
> to keep it consistent with other platform.


Not sure I follow. If you check web_contents_impl.cc it appears to me that the
power save blocker is created when playback starts, and destroyed when the
OnMediaPause notification is received. If you play a video in embedded mode you
will see that the PowerSaveBlocker is only active while the video is playing. As
soon as the playback stops the PowerSaveBlocker goes away. So what do you
exactly mean by "PowerSaveBlocker is not designed to just work with video
playback"?

> 
> For your use case, could you just replace 'transfer' with 'destroy old
> one/create new one'?

Given the above this is simpler.

[Ignacio Solla, 2014-10-15 18:54:54]

On 2014/10/15 18:51:16, Ignacio Solla wrote:
> On 2014/10/15 18:38:33, michaelbai wrote:
> > On 2014/10/15 18:26:13, Ignacio Solla wrote:
> > > On 2014/10/15 18:03:27, michaelbai wrote:
> > > > Could you explain why don't you create new PowerSaveBlocker when it
> needed,
> > > > instead of reusing the existing one? It looks to me this make code
> > unnecessary
> > > > complicated.
> > > 
> > > Creating a PowerSaveBlocker when needed would be much more complicated
> because
> > > we will need to check the playing state after entering/exiting fullscreen
to
> > > decide whether a PowerSaveBlocker needs to be created/destroyed if
present.
> > > 
> > > Take a look at these tests:
> > > 
> > > doTestPowerSaveBlockerIsEnabledDuringFullscreenPlayback
> > > testPowerSaveBlockerIsTransferredToFullscreen
> > > testPowerSaveBlockerIsTransferredToEmbedded 
> > > 
> > > Transferring a PowerSaveBlocker if present is easier than having to
> implement
> > > these rules:
> > > -> After entering fullscreen:
> > >    - If the video is playing create a fullscreen PowerSaveBlocker
> > >    - If the video is not playing do nothing
> > > -> After exiting fullscreen:
> > >    - If the video is playing create an embedded PowerSaveBlocker if one
does
> > not
> > > already exist
> > >    - If the video is not playing destroy the embedded PowerSaveBlocker if
> one
> > > exists
> > > 
> > > And note that checking whether the video is playing is by itself something
> > that
> > > we should really not be doing at this level.
> > 
> > 
> > 
> > First of all, PowerSaveBlocker is not designed to just work with video
> playback,
> > and I want
> > to keep it consistent with other platform.
> 
> 
> Not sure I follow. If you check web_contents_impl.cc it appears to me that the
> power save blocker is created when playback starts, and destroyed when the
> OnMediaPause notification is received. If you play a video in embedded mode
you
> will see that the PowerSaveBlocker is only active while the video is playing.
As
> soon as the playback stops the PowerSaveBlocker goes away. So what do you
> exactly mean by "PowerSaveBlocker is not designed to just work with video
> playback"?
> 
> > 
> > For your use case, could you just replace 'transfer' with 'destroy old
> > one/create new one'?
> 
> Given the above this is simpler.

BTW, this is also consistent with Clank. Clank does not need to transfer the
PowerSaveBlocker because they don't change container views when entering/exiting
fullscreen, they just keep using the same container view.

[michaelbai, 2014-10-15 18:55:08]

On 2014/10/15 18:51:16, Ignacio Solla wrote:
> On 2014/10/15 18:38:33, michaelbai wrote:
> > On 2014/10/15 18:26:13, Ignacio Solla wrote:
> > > On 2014/10/15 18:03:27, michaelbai wrote:
> > > > Could you explain why don't you create new PowerSaveBlocker when it
> needed,
> > > > instead of reusing the existing one? It looks to me this make code
> > unnecessary
> > > > complicated.
> > > 
> > > Creating a PowerSaveBlocker when needed would be much more complicated
> because
> > > we will need to check the playing state after entering/exiting fullscreen
to
> > > decide whether a PowerSaveBlocker needs to be created/destroyed if
present.
> > > 
> > > Take a look at these tests:
> > > 
> > > doTestPowerSaveBlockerIsEnabledDuringFullscreenPlayback
> > > testPowerSaveBlockerIsTransferredToFullscreen
> > > testPowerSaveBlockerIsTransferredToEmbedded 
> > > 
> > > Transferring a PowerSaveBlocker if present is easier than having to
> implement
> > > these rules:
> > > -> After entering fullscreen:
> > >    - If the video is playing create a fullscreen PowerSaveBlocker
> > >    - If the video is not playing do nothing
> > > -> After exiting fullscreen:
> > >    - If the video is playing create an embedded PowerSaveBlocker if one
does
> > not
> > > already exist
> > >    - If the video is not playing destroy the embedded PowerSaveBlocker if
> one
> > > exists
> > > 
> > > And note that checking whether the video is playing is by itself something
> > that
> > > we should really not be doing at this level.
> > 
> > 
> > 
> > First of all, PowerSaveBlocker is not designed to just work with video
> playback,
> > and I want
> > to keep it consistent with other platform.
> 
> 
> Not sure I follow. If you check web_contents_impl.cc it appears to me that the
> power save blocker is created when playback starts, and destroyed when the
> OnMediaPause notification is received. If you play a video in embedded mode
you
> will see that the PowerSaveBlocker is only active while the video is playing.
As
> soon as the playback stops the PowerSaveBlocker goes away. So what do you
> exactly mean by "PowerSaveBlocker is not designed to just work with video
> playback"?
>

PowerSaveBlock could be used for other case that needs to keep screen on.

 
> > 
> > For your use case, could you just replace 'transfer' with 'destroy old
> > one/create new one'?
> 
> Given the above this is simpler.

[Ignacio Solla, 2014-10-15 18:57:23]

On 2014/10/15 18:55:08, michaelbai wrote:
> On 2014/10/15 18:51:16, Ignacio Solla wrote:
> > On 2014/10/15 18:38:33, michaelbai wrote:
> > > On 2014/10/15 18:26:13, Ignacio Solla wrote:
> > > > On 2014/10/15 18:03:27, michaelbai wrote:
> > > > > Could you explain why don't you create new PowerSaveBlocker when it
> > needed,
> > > > > instead of reusing the existing one? It looks to me this make code
> > > unnecessary
> > > > > complicated.
> > > > 
> > > > Creating a PowerSaveBlocker when needed would be much more complicated
> > because
> > > > we will need to check the playing state after entering/exiting
fullscreen
> to
> > > > decide whether a PowerSaveBlocker needs to be created/destroyed if
> present.
> > > > 
> > > > Take a look at these tests:
> > > > 
> > > > doTestPowerSaveBlockerIsEnabledDuringFullscreenPlayback
> > > > testPowerSaveBlockerIsTransferredToFullscreen
> > > > testPowerSaveBlockerIsTransferredToEmbedded 
> > > > 
> > > > Transferring a PowerSaveBlocker if present is easier than having to
> > implement
> > > > these rules:
> > > > -> After entering fullscreen:
> > > >    - If the video is playing create a fullscreen PowerSaveBlocker
> > > >    - If the video is not playing do nothing
> > > > -> After exiting fullscreen:
> > > >    - If the video is playing create an embedded PowerSaveBlocker if one
> does
> > > not
> > > > already exist
> > > >    - If the video is not playing destroy the embedded PowerSaveBlocker
if
> > one
> > > > exists
> > > > 
> > > > And note that checking whether the video is playing is by itself
something
> > > that
> > > > we should really not be doing at this level.
> > > 
> > > 
> > > 
> > > First of all, PowerSaveBlocker is not designed to just work with video
> > playback,
> > > and I want
> > > to keep it consistent with other platform.
> > 
> > 
> > Not sure I follow. If you check web_contents_impl.cc it appears to me that
the
> > power save blocker is created when playback starts, and destroyed when the
> > OnMediaPause notification is received. If you play a video in embedded mode
> you
> > will see that the PowerSaveBlocker is only active while the video is
playing.
> As
> > soon as the playback stops the PowerSaveBlocker goes away. So what do you
> > exactly mean by "PowerSaveBlocker is not designed to just work with video
> > playback"?
> >
> 
> PowerSaveBlock could be used for other case that needs to keep screen on.

Well, in those cases we should probably also keep the screen on when going
fullscreen. Not sure how you tell that the correct thing to do is to remove the
PowerSaveBlocker when we enter fullscreen. This is what Clank does.

[michaelbai, 2014-10-15 19:08:57]

On 2014/10/15 18:57:23, Ignacio Solla wrote:
> On 2014/10/15 18:55:08, michaelbai wrote:
> > On 2014/10/15 18:51:16, Ignacio Solla wrote:
> > > On 2014/10/15 18:38:33, michaelbai wrote:
> > > > On 2014/10/15 18:26:13, Ignacio Solla wrote:
> > > > > On 2014/10/15 18:03:27, michaelbai wrote:
> > > > > > Could you explain why don't you create new PowerSaveBlocker when it
> > > needed,
> > > > > > instead of reusing the existing one? It looks to me this make code
> > > > unnecessary
> > > > > > complicated.
> > > > > 
> > > > > Creating a PowerSaveBlocker when needed would be much more complicated
> > > because
> > > > > we will need to check the playing state after entering/exiting
> fullscreen
> > to
> > > > > decide whether a PowerSaveBlocker needs to be created/destroyed if
> > present.
> > > > > 
> > > > > Take a look at these tests:
> > > > > 
> > > > > doTestPowerSaveBlockerIsEnabledDuringFullscreenPlayback
> > > > > testPowerSaveBlockerIsTransferredToFullscreen
> > > > > testPowerSaveBlockerIsTransferredToEmbedded 
> > > > > 
> > > > > Transferring a PowerSaveBlocker if present is easier than having to
> > > implement
> > > > > these rules:
> > > > > -> After entering fullscreen:
> > > > >    - If the video is playing create a fullscreen PowerSaveBlocker
> > > > >    - If the video is not playing do nothing
> > > > > -> After exiting fullscreen:
> > > > >    - If the video is playing create an embedded PowerSaveBlocker if
one
> > does
> > > > not
> > > > > already exist
> > > > >    - If the video is not playing destroy the embedded PowerSaveBlocker
> if
> > > one
> > > > > exists
> > > > > 
> > > > > And note that checking whether the video is playing is by itself
> something
> > > > that
> > > > > we should really not be doing at this level.
> > > > 
> > > > 
> > > > 
> > > > First of all, PowerSaveBlocker is not designed to just work with video
> > > playback,
> > > > and I want
> > > > to keep it consistent with other platform.
> > > 
> > > 
> > > Not sure I follow. If you check web_contents_impl.cc it appears to me that
> the
> > > power save blocker is created when playback starts, and destroyed when the
> > > OnMediaPause notification is received. If you play a video in embedded
mode
> > you
> > > will see that the PowerSaveBlocker is only active while the video is
> playing.
> > As
> > > soon as the playback stops the PowerSaveBlocker goes away. So what do you
> > > exactly mean by "PowerSaveBlocker is not designed to just work with video
> > > playback"?
> > >
> > 
> > PowerSaveBlock could be used for other case that needs to keep screen on.
> 
> Well, in those cases we should probably also keep the screen on when going
> fullscreen. Not sure how you tell that the correct thing to do is to remove
the
> PowerSaveBlocker when we enter fullscreen. This is what Clank does.


I don't think we should always keep screen on in full screen mode(no video
playback).

Just talk to Min, we should always get Pause/Resume message no matter it is in
fullscreen mode or not, could you take this advantage?

[Ignacio Solla, 2014-10-15 19:19:26]

On 2014/10/15 19:08:57, michaelbai wrote:
> On 2014/10/15 18:57:23, Ignacio Solla wrote:
> > On 2014/10/15 18:55:08, michaelbai wrote:
> > > On 2014/10/15 18:51:16, Ignacio Solla wrote:
> > > > On 2014/10/15 18:38:33, michaelbai wrote:
> > > > > On 2014/10/15 18:26:13, Ignacio Solla wrote:
> > > > > > On 2014/10/15 18:03:27, michaelbai wrote:
> > > > > > > Could you explain why don't you create new PowerSaveBlocker when
it
> > > > needed,
> > > > > > > instead of reusing the existing one? It looks to me this make code
> > > > > unnecessary
> > > > > > > complicated.
> > > > > > 
> > > > > > Creating a PowerSaveBlocker when needed would be much more
complicated
> > > > because
> > > > > > we will need to check the playing state after entering/exiting
> > fullscreen
> > > to
> > > > > > decide whether a PowerSaveBlocker needs to be created/destroyed if
> > > present.
> > > > > > 
> > > > > > Take a look at these tests:
> > > > > > 
> > > > > > doTestPowerSaveBlockerIsEnabledDuringFullscreenPlayback
> > > > > > testPowerSaveBlockerIsTransferredToFullscreen
> > > > > > testPowerSaveBlockerIsTransferredToEmbedded 
> > > > > > 
> > > > > > Transferring a PowerSaveBlocker if present is easier than having to
> > > > implement
> > > > > > these rules:
> > > > > > -> After entering fullscreen:
> > > > > >    - If the video is playing create a fullscreen PowerSaveBlocker
> > > > > >    - If the video is not playing do nothing
> > > > > > -> After exiting fullscreen:
> > > > > >    - If the video is playing create an embedded PowerSaveBlocker if
> one
> > > does
> > > > > not
> > > > > > already exist
> > > > > >    - If the video is not playing destroy the embedded
PowerSaveBlocker
> > if
> > > > one
> > > > > > exists
> > > > > > 
> > > > > > And note that checking whether the video is playing is by itself
> > something
> > > > > that
> > > > > > we should really not be doing at this level.
> > > > > 
> > > > > 
> > > > > 
> > > > > First of all, PowerSaveBlocker is not designed to just work with video
> > > > playback,
> > > > > and I want
> > > > > to keep it consistent with other platform.
> > > > 
> > > > 
> > > > Not sure I follow. If you check web_contents_impl.cc it appears to me
that
> > the
> > > > power save blocker is created when playback starts, and destroyed when
the
> > > > OnMediaPause notification is received. If you play a video in embedded
> mode
> > > you
> > > > will see that the PowerSaveBlocker is only active while the video is
> > playing.
> > > As
> > > > soon as the playback stops the PowerSaveBlocker goes away. So what do
you
> > > > exactly mean by "PowerSaveBlocker is not designed to just work with
video
> > > > playback"?
> > > >
> > > 
> > > PowerSaveBlock could be used for other case that needs to keep screen on.
> > 
> > Well, in those cases we should probably also keep the screen on when going
> > fullscreen. Not sure how you tell that the correct thing to do is to remove
> the
> > PowerSaveBlocker when we enter fullscreen. This is what Clank does.
> 
> 
> I don't think we should always keep screen on in full screen mode(no video
> playback).
> 
> Just talk to Min, we should always get Pause/Resume message no matter it is in
> fullscreen mode or not, could you take this advantage?

We don't always keep the screen on in full screen. Once the video stops we
remove the PowerSaveBlocker.

As you said, we might use a PowerSaveBlocker for something else other than
video. Think of it this other way round. The state of the WebContents is what
determines whether a PowerSaveBlocker should be active or not. Whether we are
rendering the WebContents in containerView A or in containerView B is
irrelevant. By transferring the PowerSaveBlocker around if present we're just
letting the WebContents decide. It does not need to be more or less complicated
than this.

[michaelbai, 2014-10-15 21:07:11]

On 2014/10/15 19:19:26, Ignacio Solla wrote:
> On 2014/10/15 19:08:57, michaelbai wrote:
> > On 2014/10/15 18:57:23, Ignacio Solla wrote:
> > > On 2014/10/15 18:55:08, michaelbai wrote:
> > > > On 2014/10/15 18:51:16, Ignacio Solla wrote:
> > > > > On 2014/10/15 18:38:33, michaelbai wrote:
> > > > > > On 2014/10/15 18:26:13, Ignacio Solla wrote:
> > > > > > > On 2014/10/15 18:03:27, michaelbai wrote:
> > > > > > > > Could you explain why don't you create new PowerSaveBlocker when
> it
> > > > > needed,
> > > > > > > > instead of reusing the existing one? It looks to me this make
code
> > > > > > unnecessary
> > > > > > > > complicated.
> > > > > > > 
> > > > > > > Creating a PowerSaveBlocker when needed would be much more
> complicated
> > > > > because
> > > > > > > we will need to check the playing state after entering/exiting
> > > fullscreen
> > > > to
> > > > > > > decide whether a PowerSaveBlocker needs to be created/destroyed if
> > > > present.
> > > > > > > 
> > > > > > > Take a look at these tests:
> > > > > > > 
> > > > > > > doTestPowerSaveBlockerIsEnabledDuringFullscreenPlayback
> > > > > > > testPowerSaveBlockerIsTransferredToFullscreen
> > > > > > > testPowerSaveBlockerIsTransferredToEmbedded 
> > > > > > > 
> > > > > > > Transferring a PowerSaveBlocker if present is easier than having
to
> > > > > implement
> > > > > > > these rules:
> > > > > > > -> After entering fullscreen:
> > > > > > >    - If the video is playing create a fullscreen PowerSaveBlocker
> > > > > > >    - If the video is not playing do nothing
> > > > > > > -> After exiting fullscreen:
> > > > > > >    - If the video is playing create an embedded PowerSaveBlocker
if
> > one
> > > > does
> > > > > > not
> > > > > > > already exist
> > > > > > >    - If the video is not playing destroy the embedded
> PowerSaveBlocker
> > > if
> > > > > one
> > > > > > > exists
> > > > > > > 
> > > > > > > And note that checking whether the video is playing is by itself
> > > something
> > > > > > that
> > > > > > > we should really not be doing at this level.
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > First of all, PowerSaveBlocker is not designed to just work with
video
> > > > > playback,
> > > > > > and I want
> > > > > > to keep it consistent with other platform.
> > > > > 
> > > > > 
> > > > > Not sure I follow. If you check web_contents_impl.cc it appears to me
> that
> > > the
> > > > > power save blocker is created when playback starts, and destroyed when
> the
> > > > > OnMediaPause notification is received. If you play a video in embedded
> > mode
> > > > you
> > > > > will see that the PowerSaveBlocker is only active while the video is
> > > playing.
> > > > As
> > > > > soon as the playback stops the PowerSaveBlocker goes away. So what do
> you
> > > > > exactly mean by "PowerSaveBlocker is not designed to just work with
> video
> > > > > playback"?
> > > > >
> > > > 
> > > > PowerSaveBlock could be used for other case that needs to keep screen
on.
> > > 
> > > Well, in those cases we should probably also keep the screen on when going
> > > fullscreen. Not sure how you tell that the correct thing to do is to
remove
> > the
> > > PowerSaveBlocker when we enter fullscreen. This is what Clank does.
> > 
> > 
> > I don't think we should always keep screen on in full screen mode(no video
> > playback).
> > 
> > Just talk to Min, we should always get Pause/Resume message no matter it is
in
> > fullscreen mode or not, could you take this advantage?
> 
> We don't always keep the screen on in full screen. Once the video stops we
> remove the PowerSaveBlocker.
> 
> As you said, we might use a PowerSaveBlocker for something else other than
> video. Think of it this other way round. The state of the WebContents is what
> determines whether a PowerSaveBlocker should be active or not. Whether we are
> rendering the WebContents in containerView A or in containerView B is
> irrelevant. By transferring the PowerSaveBlocker around if present we're just
> letting the WebContents decide. It does not need to be more or less
complicated
> than this.


I understand your purpose, but it makes the code complicated from ViewAndroid
aspect.
 
You patch will change the ViewAndroidDelegate, but we have
ViewAndroid.getViewAndroidDelegate(), the user of it could have reference on
returned obj, will you have observer interface to notify the delegate has been
changed? Should user expect this kind of change?

Also the ViewAndroidDelegate implementation in ContentViewCore has comment

// mContainerView can change, but this ViewAndroidDelegate can only be used to
// add and remove views from the mContainerViewAtCreation.

Is it indicated the ViewAndroidDelegate shouldn't be changed for its user?

[Ignacio Solla, 2014-10-16 09:44:36]

On 2014/10/15 21:07:11, michaelbai wrote:
> On 2014/10/15 19:19:26, Ignacio Solla wrote:
> > On 2014/10/15 19:08:57, michaelbai wrote:
> > > On 2014/10/15 18:57:23, Ignacio Solla wrote:
> > > > On 2014/10/15 18:55:08, michaelbai wrote:
> > > > > On 2014/10/15 18:51:16, Ignacio Solla wrote:
> > > > > > On 2014/10/15 18:38:33, michaelbai wrote:
> > > > > > > On 2014/10/15 18:26:13, Ignacio Solla wrote:
> > > > > > > > On 2014/10/15 18:03:27, michaelbai wrote:
> > > > > > > > > Could you explain why don't you create new PowerSaveBlocker
when
> > it
> > > > > > needed,
> > > > > > > > > instead of reusing the existing one? It looks to me this make
> code
> > > > > > > unnecessary
> > > > > > > > > complicated.
> > > > > > > > 
> > > > > > > > Creating a PowerSaveBlocker when needed would be much more
> > complicated
> > > > > > because
> > > > > > > > we will need to check the playing state after entering/exiting
> > > > fullscreen
> > > > > to
> > > > > > > > decide whether a PowerSaveBlocker needs to be created/destroyed
if
> > > > > present.
> > > > > > > > 
> > > > > > > > Take a look at these tests:
> > > > > > > > 
> > > > > > > > doTestPowerSaveBlockerIsEnabledDuringFullscreenPlayback
> > > > > > > > testPowerSaveBlockerIsTransferredToFullscreen
> > > > > > > > testPowerSaveBlockerIsTransferredToEmbedded 
> > > > > > > > 
> > > > > > > > Transferring a PowerSaveBlocker if present is easier than having
> to
> > > > > > implement
> > > > > > > > these rules:
> > > > > > > > -> After entering fullscreen:
> > > > > > > >    - If the video is playing create a fullscreen
PowerSaveBlocker
> > > > > > > >    - If the video is not playing do nothing
> > > > > > > > -> After exiting fullscreen:
> > > > > > > >    - If the video is playing create an embedded PowerSaveBlocker
> if
> > > one
> > > > > does
> > > > > > > not
> > > > > > > > already exist
> > > > > > > >    - If the video is not playing destroy the embedded
> > PowerSaveBlocker
> > > > if
> > > > > > one
> > > > > > > > exists
> > > > > > > > 
> > > > > > > > And note that checking whether the video is playing is by itself
> > > > something
> > > > > > > that
> > > > > > > > we should really not be doing at this level.
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > First of all, PowerSaveBlocker is not designed to just work with
> video
> > > > > > playback,
> > > > > > > and I want
> > > > > > > to keep it consistent with other platform.
> > > > > > 
> > > > > > 
> > > > > > Not sure I follow. If you check web_contents_impl.cc it appears to
me
> > that
> > > > the
> > > > > > power save blocker is created when playback starts, and destroyed
when
> > the
> > > > > > OnMediaPause notification is received. If you play a video in
embedded
> > > mode
> > > > > you
> > > > > > will see that the PowerSaveBlocker is only active while the video is
> > > > playing.
> > > > > As
> > > > > > soon as the playback stops the PowerSaveBlocker goes away. So what
do
> > you
> > > > > > exactly mean by "PowerSaveBlocker is not designed to just work with
> > video
> > > > > > playback"?
> > > > > >
> > > > > 
> > > > > PowerSaveBlock could be used for other case that needs to keep screen
> on.
> > > > 
> > > > Well, in those cases we should probably also keep the screen on when
going
> > > > fullscreen. Not sure how you tell that the correct thing to do is to
> remove
> > > the
> > > > PowerSaveBlocker when we enter fullscreen. This is what Clank does.
> > > 
> > > 
> > > I don't think we should always keep screen on in full screen mode(no video
> > > playback).
> > > 
> > > Just talk to Min, we should always get Pause/Resume message no matter it
is
> in
> > > fullscreen mode or not, could you take this advantage?
> > 
> > We don't always keep the screen on in full screen. Once the video stops we
> > remove the PowerSaveBlocker.
> > 
> > As you said, we might use a PowerSaveBlocker for something else other than
> > video. Think of it this other way round. The state of the WebContents is
what
> > determines whether a PowerSaveBlocker should be active or not. Whether we
are
> > rendering the WebContents in containerView A or in containerView B is
> > irrelevant. By transferring the PowerSaveBlocker around if present we're
just
> > letting the WebContents decide. It does not need to be more or less
> complicated
> > than this.
> 
> 
> I understand your purpose, but it makes the code complicated from ViewAndroid
> aspect.
>  
> You patch will change the ViewAndroidDelegate, but we have
> ViewAndroid.getViewAndroidDelegate(), the user of it could have reference on
> returned obj, will you have observer interface to notify the delegate has been
> changed? Should user expect this kind of change?

I disagree with "makes the code complicated from ViewAndroid aspect".
First, this patch does not change the validity of the reference that they hold.
If
the container view has changed then they are already holding an obsolete
reference today.

Now step back and consider the whole picture. The container view only ever
changes when
transitioning from/to fullscreen in the WebView, never in Clank. (see big
WARNING above
ContentViewCore.setContainerView). So the problem of dealing with mutable
ViewAndroidDelegates
boils down to ensuring that anchor views behave correctly during those
transitions.

Only two different types of anchor views are created:
1. ViewAndroid creates anchor views to be used as PowerSaveBlockers
2. DropdownPopupWindow creates anchor views to show the DropDown

Let's go one step at the time. This change ensures that 1 works correctly.
Regarding 2
DropDowns ARE NOT supported in the WebView in fullscreen today. DropDowns are
out of scope,
but this change probably also makes creating DropDowns while in fullscreen
possible (I
believe that a fresh ViewAndroidDelegate reference is retrieved every time a new
DropDown is
created). What is definitely not supported is to transfer DropDowns around
during transitions,
but we probably don't need that (dropdowns will go away when clicking on the
fullscreen button). If transferring other anchor views in addition to
PowerSaveBlockers is ever
needed then your suggestion of implementing an observer interface is a good one,
but let’s
not make things unnecessarily complicated for not good reason.


> 
> Also the ViewAndroidDelegate implementation in ContentViewCore has comment
> 
> // mContainerView can change, but this ViewAndroidDelegate can only be used to
> // add and remove views from the mContainerViewAtCreation.
> 
> Is it indicated the ViewAndroidDelegate shouldn't be changed for its user?

No, it does not mean that. It means that ContentViewCore.mContainerView is
mutable, but ContentViewCore.ViewAndroidDelegate.mContainerViewAtCreation is
not.
So if you need a ViewAndroidDelegate that works with the new mContainerView then
you will have to create a new instance. This is to avoid using the same
ViewAndroidDelegate
to add anchor views to mContainerView A, and later try to remove them from
mContainerView B.
As you can see, changing the mContainerView requires some other actions. 

An alternative implementation would be:
- cache the created anchorViews and positions inside
ContentViewCore.ViewAndroidDelegate
- make getViewAndroidDelegate() return a ViewAndroidDelegateWrapper that just
forwards requests
  to the real ViewAndroidDelegate
- when containerViews change move then around and update the ViewAndroidDelegate
ref inside
  ViewAndroidDelegateWrapper

But that's more complicated and as I said above I don't think that there is an
use case for it.

[michaelbai, 2014-10-16 14:44:13]

On 2014/10/16 09:44:36, Ignacio Solla wrote:
> On 2014/10/15 21:07:11, michaelbai wrote:
> > On 2014/10/15 19:19:26, Ignacio Solla wrote:
> > > On 2014/10/15 19:08:57, michaelbai wrote:
> > > > On 2014/10/15 18:57:23, Ignacio Solla wrote:
> > > > > On 2014/10/15 18:55:08, michaelbai wrote:
> > > > > > On 2014/10/15 18:51:16, Ignacio Solla wrote:
> > > > > > > On 2014/10/15 18:38:33, michaelbai wrote:
> > > > > > > > On 2014/10/15 18:26:13, Ignacio Solla wrote:
> > > > > > > > > On 2014/10/15 18:03:27, michaelbai wrote:
> > > > > > > > > > Could you explain why don't you create new PowerSaveBlocker
> when
> > > it
> > > > > > > needed,
> > > > > > > > > > instead of reusing the existing one? It looks to me this
make
> > code
> > > > > > > > unnecessary
> > > > > > > > > > complicated.
> > > > > > > > > 
> > > > > > > > > Creating a PowerSaveBlocker when needed would be much more
> > > complicated
> > > > > > > because
> > > > > > > > > we will need to check the playing state after entering/exiting
> > > > > fullscreen
> > > > > > to
> > > > > > > > > decide whether a PowerSaveBlocker needs to be
created/destroyed
> if
> > > > > > present.
> > > > > > > > > 
> > > > > > > > > Take a look at these tests:
> > > > > > > > > 
> > > > > > > > > doTestPowerSaveBlockerIsEnabledDuringFullscreenPlayback
> > > > > > > > > testPowerSaveBlockerIsTransferredToFullscreen
> > > > > > > > > testPowerSaveBlockerIsTransferredToEmbedded 
> > > > > > > > > 
> > > > > > > > > Transferring a PowerSaveBlocker if present is easier than
having
> > to
> > > > > > > implement
> > > > > > > > > these rules:
> > > > > > > > > -> After entering fullscreen:
> > > > > > > > >    - If the video is playing create a fullscreen
> PowerSaveBlocker
> > > > > > > > >    - If the video is not playing do nothing
> > > > > > > > > -> After exiting fullscreen:
> > > > > > > > >    - If the video is playing create an embedded
PowerSaveBlocker
> > if
> > > > one
> > > > > > does
> > > > > > > > not
> > > > > > > > > already exist
> > > > > > > > >    - If the video is not playing destroy the embedded
> > > PowerSaveBlocker
> > > > > if
> > > > > > > one
> > > > > > > > > exists
> > > > > > > > > 
> > > > > > > > > And note that checking whether the video is playing is by
itself
> > > > > something
> > > > > > > > that
> > > > > > > > > we should really not be doing at this level.
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > First of all, PowerSaveBlocker is not designed to just work with
> > video
> > > > > > > playback,
> > > > > > > > and I want
> > > > > > > > to keep it consistent with other platform.
> > > > > > > 
> > > > > > > 
> > > > > > > Not sure I follow. If you check web_contents_impl.cc it appears to
> me
> > > that
> > > > > the
> > > > > > > power save blocker is created when playback starts, and destroyed
> when
> > > the
> > > > > > > OnMediaPause notification is received. If you play a video in
> embedded
> > > > mode
> > > > > > you
> > > > > > > will see that the PowerSaveBlocker is only active while the video
is
> > > > > playing.
> > > > > > As
> > > > > > > soon as the playback stops the PowerSaveBlocker goes away. So what
> do
> > > you
> > > > > > > exactly mean by "PowerSaveBlocker is not designed to just work
with
> > > video
> > > > > > > playback"?
> > > > > > >
> > > > > > 
> > > > > > PowerSaveBlock could be used for other case that needs to keep
screen
> > on.
> > > > > 
> > > > > Well, in those cases we should probably also keep the screen on when
> going
> > > > > fullscreen. Not sure how you tell that the correct thing to do is to
> > remove
> > > > the
> > > > > PowerSaveBlocker when we enter fullscreen. This is what Clank does.
> > > > 
> > > > 
> > > > I don't think we should always keep screen on in full screen mode(no
video
> > > > playback).
> > > > 
> > > > Just talk to Min, we should always get Pause/Resume message no matter it
> is
> > in
> > > > fullscreen mode or not, could you take this advantage?
> > > 
> > > We don't always keep the screen on in full screen. Once the video stops we
> > > remove the PowerSaveBlocker.
> > > 
> > > As you said, we might use a PowerSaveBlocker for something else other than
> > > video. Think of it this other way round. The state of the WebContents is
> what
> > > determines whether a PowerSaveBlocker should be active or not. Whether we
> are
> > > rendering the WebContents in containerView A or in containerView B is
> > > irrelevant. By transferring the PowerSaveBlocker around if present we're
> just
> > > letting the WebContents decide. It does not need to be more or less
> > complicated
> > > than this.
> > 
> > 
> > I understand your purpose, but it makes the code complicated from
ViewAndroid
> > aspect.
> >  
> > You patch will change the ViewAndroidDelegate, but we have
> > ViewAndroid.getViewAndroidDelegate(), the user of it could have reference on
> > returned obj, will you have observer interface to notify the delegate has
been
> > changed? Should user expect this kind of change?
> 
> I disagree with "makes the code complicated from ViewAndroid aspect".
> First, this patch does not change the validity of the reference that they
hold.
> If
> the container view has changed then they are already holding an obsolete
> reference today.
> 
> Now step back and consider the whole picture. The container view only ever
> changes when
> transitioning from/to fullscreen in the WebView, never in Clank. (see big
> WARNING above
> ContentViewCore.setContainerView). So the problem of dealing with mutable
> ViewAndroidDelegates
> boils down to ensuring that anchor views behave correctly during those
> transitions.
> 
> Only two different types of anchor views are created:
> 1. ViewAndroid creates anchor views to be used as PowerSaveBlockers
> 2. DropdownPopupWindow creates anchor views to show the DropDown
> 
> Let's go one step at the time. This change ensures that 1 works correctly.
> Regarding 2
> DropDowns ARE NOT supported in the WebView in fullscreen today. DropDowns are
> out of scope,
> but this change probably also makes creating DropDowns while in fullscreen
> possible (I
> believe that a fresh ViewAndroidDelegate reference is retrieved every time a
new
> DropDown is
> created). What is definitely not supported is to transfer DropDowns around
> during transitions,
> but we probably don't need that (dropdowns will go away when clicking on the
> fullscreen button). If transferring other anchor views in addition to
> PowerSaveBlockers is ever
> needed then your suggestion of implementing an observer interface is a good
one,
> but let’s
> not make things unnecessarily complicated for not good reason.
> 
> 
> > 
> > Also the ViewAndroidDelegate implementation in ContentViewCore has comment
> > 
> > // mContainerView can change, but this ViewAndroidDelegate can only be used
to
> > // add and remove views from the mContainerViewAtCreation.
> > 
> > Is it indicated the ViewAndroidDelegate shouldn't be changed for its user?
> 
> No, it does not mean that. It means that ContentViewCore.mContainerView is
> mutable, but ContentViewCore.ViewAndroidDelegate.mContainerViewAtCreation is
> not.
> So if you need a ViewAndroidDelegate that works with the new mContainerView
then
> you will have to create a new instance. This is to avoid using the same
> ViewAndroidDelegate
> to add anchor views to mContainerView A, and later try to remove them from
> mContainerView B.
> As you can see, changing the mContainerView requires some other actions. 
> 
> An alternative implementation would be:
> - cache the created anchorViews and positions inside
> ContentViewCore.ViewAndroidDelegate
> - make getViewAndroidDelegate() return a ViewAndroidDelegateWrapper that just
> forwards requests
>   to the real ViewAndroidDelegate
> - when containerViews change move then around and update the
ViewAndroidDelegate
> ref inside
>   ViewAndroidDelegateWrapper
> 
> But that's more complicated and as I said above I don't think that there is an
> use case for it.

I don't like ViewAndroid.getViewAndroidDelegate() method, user of it could hold
the older ViewAndroidDelegate and 
ContainerView, let's go to ViewAndroidDelegateWrapper and remove the
setViewAndroidDelegate method from ViewAndroid.

[Ignacio Solla, 2014-10-17 14:01:32]

On 2014/10/16 14:44:13, michaelbai wrote:
> On 2014/10/16 09:44:36, Ignacio Solla wrote:
> > On 2014/10/15 21:07:11, michaelbai wrote:
> > > On 2014/10/15 19:19:26, Ignacio Solla wrote:
> > > > On 2014/10/15 19:08:57, michaelbai wrote:
> > > > > On 2014/10/15 18:57:23, Ignacio Solla wrote:
> > > > > > On 2014/10/15 18:55:08, michaelbai wrote:
> > > > > > > On 2014/10/15 18:51:16, Ignacio Solla wrote:
> > > > > > > > On 2014/10/15 18:38:33, michaelbai wrote:
> > > > > > > > > On 2014/10/15 18:26:13, Ignacio Solla wrote:
> > > > > > > > > > On 2014/10/15 18:03:27, michaelbai wrote:
> > > > > > > > > > > Could you explain why don't you create new
PowerSaveBlocker
> > when
> > > > it
> > > > > > > > needed,
> > > > > > > > > > > instead of reusing the existing one? It looks to me this
> make
> > > code
> > > > > > > > > unnecessary
> > > > > > > > > > > complicated.
> > > > > > > > > > 
> > > > > > > > > > Creating a PowerSaveBlocker when needed would be much more
> > > > complicated
> > > > > > > > because
> > > > > > > > > > we will need to check the playing state after
entering/exiting
> > > > > > fullscreen
> > > > > > > to
> > > > > > > > > > decide whether a PowerSaveBlocker needs to be
> created/destroyed
> > if
> > > > > > > present.
> > > > > > > > > > 
> > > > > > > > > > Take a look at these tests:
> > > > > > > > > > 
> > > > > > > > > > doTestPowerSaveBlockerIsEnabledDuringFullscreenPlayback
> > > > > > > > > > testPowerSaveBlockerIsTransferredToFullscreen
> > > > > > > > > > testPowerSaveBlockerIsTransferredToEmbedded 
> > > > > > > > > > 
> > > > > > > > > > Transferring a PowerSaveBlocker if present is easier than
> having
> > > to
> > > > > > > > implement
> > > > > > > > > > these rules:
> > > > > > > > > > -> After entering fullscreen:
> > > > > > > > > >    - If the video is playing create a fullscreen
> > PowerSaveBlocker
> > > > > > > > > >    - If the video is not playing do nothing
> > > > > > > > > > -> After exiting fullscreen:
> > > > > > > > > >    - If the video is playing create an embedded
> PowerSaveBlocker
> > > if
> > > > > one
> > > > > > > does
> > > > > > > > > not
> > > > > > > > > > already exist
> > > > > > > > > >    - If the video is not playing destroy the embedded
> > > > PowerSaveBlocker
> > > > > > if
> > > > > > > > one
> > > > > > > > > > exists
> > > > > > > > > > 
> > > > > > > > > > And note that checking whether the video is playing is by
> itself
> > > > > > something
> > > > > > > > > that
> > > > > > > > > > we should really not be doing at this level.
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > First of all, PowerSaveBlocker is not designed to just work
with
> > > video
> > > > > > > > playback,
> > > > > > > > > and I want
> > > > > > > > > to keep it consistent with other platform.
> > > > > > > > 
> > > > > > > > 
> > > > > > > > Not sure I follow. If you check web_contents_impl.cc it appears
to
> > me
> > > > that
> > > > > > the
> > > > > > > > power save blocker is created when playback starts, and
destroyed
> > when
> > > > the
> > > > > > > > OnMediaPause notification is received. If you play a video in
> > embedded
> > > > > mode
> > > > > > > you
> > > > > > > > will see that the PowerSaveBlocker is only active while the
video
> is
> > > > > > playing.
> > > > > > > As
> > > > > > > > soon as the playback stops the PowerSaveBlocker goes away. So
what
> > do
> > > > you
> > > > > > > > exactly mean by "PowerSaveBlocker is not designed to just work
> with
> > > > video
> > > > > > > > playback"?
> > > > > > > >
> > > > > > > 
> > > > > > > PowerSaveBlock could be used for other case that needs to keep
> screen
> > > on.
> > > > > > 
> > > > > > Well, in those cases we should probably also keep the screen on when
> > going
> > > > > > fullscreen. Not sure how you tell that the correct thing to do is to
> > > remove
> > > > > the
> > > > > > PowerSaveBlocker when we enter fullscreen. This is what Clank does.
> > > > > 
> > > > > 
> > > > > I don't think we should always keep screen on in full screen mode(no
> video
> > > > > playback).
> > > > > 
> > > > > Just talk to Min, we should always get Pause/Resume message no matter
it
> > is
> > > in
> > > > > fullscreen mode or not, could you take this advantage?
> > > > 
> > > > We don't always keep the screen on in full screen. Once the video stops
we
> > > > remove the PowerSaveBlocker.
> > > > 
> > > > As you said, we might use a PowerSaveBlocker for something else other
than
> > > > video. Think of it this other way round. The state of the WebContents is
> > what
> > > > determines whether a PowerSaveBlocker should be active or not. Whether
we
> > are
> > > > rendering the WebContents in containerView A or in containerView B is
> > > > irrelevant. By transferring the PowerSaveBlocker around if present we're
> > just
> > > > letting the WebContents decide. It does not need to be more or less
> > > complicated
> > > > than this.
> > > 
> > > 
> > > I understand your purpose, but it makes the code complicated from
> ViewAndroid
> > > aspect.
> > >  
> > > You patch will change the ViewAndroidDelegate, but we have
> > > ViewAndroid.getViewAndroidDelegate(), the user of it could have reference
on
> > > returned obj, will you have observer interface to notify the delegate has
> been
> > > changed? Should user expect this kind of change?
> > 
> > I disagree with "makes the code complicated from ViewAndroid aspect".
> > First, this patch does not change the validity of the reference that they
> hold.
> > If
> > the container view has changed then they are already holding an obsolete
> > reference today.
> > 
> > Now step back and consider the whole picture. The container view only ever
> > changes when
> > transitioning from/to fullscreen in the WebView, never in Clank. (see big
> > WARNING above
> > ContentViewCore.setContainerView). So the problem of dealing with mutable
> > ViewAndroidDelegates
> > boils down to ensuring that anchor views behave correctly during those
> > transitions.
> > 
> > Only two different types of anchor views are created:
> > 1. ViewAndroid creates anchor views to be used as PowerSaveBlockers
> > 2. DropdownPopupWindow creates anchor views to show the DropDown
> > 
> > Let's go one step at the time. This change ensures that 1 works correctly.
> > Regarding 2
> > DropDowns ARE NOT supported in the WebView in fullscreen today. DropDowns
are
> > out of scope,
> > but this change probably also makes creating DropDowns while in fullscreen
> > possible (I
> > believe that a fresh ViewAndroidDelegate reference is retrieved every time a
> new
> > DropDown is
> > created). What is definitely not supported is to transfer DropDowns around
> > during transitions,
> > but we probably don't need that (dropdowns will go away when clicking on the
> > fullscreen button). If transferring other anchor views in addition to
> > PowerSaveBlockers is ever
> > needed then your suggestion of implementing an observer interface is a good
> one,
> > but let’s
> > not make things unnecessarily complicated for not good reason.
> > 
> > 
> > > 
> > > Also the ViewAndroidDelegate implementation in ContentViewCore has comment
> > > 
> > > // mContainerView can change, but this ViewAndroidDelegate can only be
used
> to
> > > // add and remove views from the mContainerViewAtCreation.
> > > 
> > > Is it indicated the ViewAndroidDelegate shouldn't be changed for its user?
> > 
> > No, it does not mean that. It means that ContentViewCore.mContainerView is
> > mutable, but ContentViewCore.ViewAndroidDelegate.mContainerViewAtCreation is
> > not.
> > So if you need a ViewAndroidDelegate that works with the new mContainerView
> then
> > you will have to create a new instance. This is to avoid using the same
> > ViewAndroidDelegate
> > to add anchor views to mContainerView A, and later try to remove them from
> > mContainerView B.
> > As you can see, changing the mContainerView requires some other actions. 
> > 
> > An alternative implementation would be:
> > - cache the created anchorViews and positions inside
> > ContentViewCore.ViewAndroidDelegate
> > - make getViewAndroidDelegate() return a ViewAndroidDelegateWrapper that
just
> > forwards requests
> >   to the real ViewAndroidDelegate
> > - when containerViews change move then around and update the
> ViewAndroidDelegate
> > ref inside
> >   ViewAndroidDelegateWrapper
> > 
> > But that's more complicated and as I said above I don't think that there is
an
> > use case for it.
> 
> I don't like ViewAndroid.getViewAndroidDelegate() method, user of it could
hold
> the older ViewAndroidDelegate and 
> ContainerView, let's go to ViewAndroidDelegateWrapper and remove the
> setViewAndroidDelegate method from ViewAndroid.

Done, PTAL.

[Ignacio Solla, 2014-10-17 14:01:51]

igsolla@chromium.org changed reviewers:
- miguelg@chromium.org

[michaelbai, 2014-10-17 17:30:56]

https://codereview.chromium.org/639413002/diff/280001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java
(right):

https://codereview.chromium.org/639413002/diff/280001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java:33:
interface ContentViewAndroidDelegate extends ViewAndroidDelegate {
Maybe SwappableViewAndroidDelegate?

https://codereview.chromium.org/639413002/diff/280001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java:34:
void addAnchorView(View anchorView);
Could you add comment about why we have the method?

https://codereview.chromium.org/639413002/diff/280001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java:58:
List<View> mAnchorViews;
We should use weak reference here

https://codereview.chromium.org/639413002/diff/280001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java:59:
Map<View, Position> mAnchorViewPositions;
Using WeakHashMap.

I think you don't need List<View> at all, instead, have below in
acquireAnchorView()
mAnchorViewPositions.put(view, new Position(view.getX(), view.getY(),
view.getWidth(), view.getHeight());
or
mAnchorViewPositions.put(view, null);

https://codereview.chromium.org/639413002/diff/280001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java:96:
void setViewAndroidDelegate(ContentViewAndroidDelegate delegate) {
Maybe SwapViewAndroidDelegate()?

https://codereview.chromium.org/639413002/diff/280001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/639413002/diff/280001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:678:

Maybe move those 2 @VisibleForTesting method to line 556?

[Ignacio Solla, 2014-10-20 11:41:03]

https://codereview.chromium.org/639413002/diff/280001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java
(right):

https://codereview.chromium.org/639413002/diff/280001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java:33:
interface ContentViewAndroidDelegate extends ViewAndroidDelegate {
On 2014/10/17 17:30:55, michaelbai wrote:
> Maybe SwappableViewAndroidDelegate?

Done. Renamed to ReplaceableViewAndroidDelegate.

https://codereview.chromium.org/639413002/diff/280001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java:34:
void addAnchorView(View anchorView);
On 2014/10/17 17:30:55, michaelbai wrote:
> Could you add comment about why we have the method?

Done.

https://codereview.chromium.org/639413002/diff/280001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java:58:
List<View> mAnchorViews;
On 2014/10/17 17:30:55, michaelbai wrote:
> We should use weak reference here 

No, we should use a strong reference here.

To transfer anchor views we need to remove them from the old container view
first before adding them to the new container view. The ref from the old
container view might be the last strong ref, so we want to ensure that the view
is not garbage collected before we have a chance to add it to the new container
view.

https://codereview.chromium.org/639413002/diff/280001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java:59:
Map<View, Position> mAnchorViewPositions;
On 2014/10/17 17:30:56, michaelbai wrote:
> Using WeakHashMap.
> 
> I think you don't need List<View> at all, instead, have below in
> acquireAnchorView()
> mAnchorViewPositions.put(view, new Position(view.getX(), view.getY(),
> view.getWidth(), view.getHeight());
> or
> mAnchorViewPositions.put(view, null);

We do need the List because we want to maintain the order.

To make things a bit simpler, I have replaced the List + Map with a
LinkedHashMap now.

https://codereview.chromium.org/639413002/diff/280001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java:96:
void setViewAndroidDelegate(ContentViewAndroidDelegate delegate) {
On 2014/10/17 17:30:55, michaelbai wrote:
> Maybe SwapViewAndroidDelegate()?

Done.

https://codereview.chromium.org/639413002/diff/280001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/639413002/diff/280001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:678:

On 2014/10/17 17:30:56, michaelbai wrote:
> Maybe move those 2 @VisibleForTesting method to line 556?

Why??

These methods are only called in this file by initialize() above, and they are
similar to setcontainerView,  setContainerViewInternals and initPopupZoomer
below.

[michaelbai, 2014-10-20 17:19:50]

https://codereview.chromium.org/639413002/diff/280001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java
(right):

https://codereview.chromium.org/639413002/diff/280001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java:58:
List<View> mAnchorViews;
On 2014/10/20 11:41:02, Ignacio Solla wrote:
> On 2014/10/17 17:30:55, michaelbai wrote:
> > We should use weak reference here 
> 
> No, we should use a strong reference here.
> 
> To transfer anchor views we need to remove them from the old container view
> first before adding them to the new container view. The ref from the old
> container view might be the last strong ref, so we want to ensure that the
view
> is not garbage collected before we have a chance to add it to the new
container
> view.

Is the user expected to use the AnchorView like below?

View view = acuqireAnchroView();
...
releaseAnchorView(view);

It means user should have the reference of view, if no, he has no way to call
releaseAnchorView(), then this is memory leak if we have strong reference of
view here.

Furthermore, if you can get the view from weak reference, you already get strong
reference of it, and it can't be gc-ed when you swap the container view.

[Ignacio Solla, 2014-10-20 17:47:56]

On 2014/10/20 17:19:50, michaelbai wrote:
>
https://codereview.chromium.org/639413002/diff/280001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java
> (right):
> 
>
https://codereview.chromium.org/639413002/diff/280001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java:58:
> List<View> mAnchorViews;
> On 2014/10/20 11:41:02, Ignacio Solla wrote:
> > On 2014/10/17 17:30:55, michaelbai wrote:
> > > We should use weak reference here 
> > 
> > No, we should use a strong reference here.
> > 
> > To transfer anchor views we need to remove them from the old container view
> > first before adding them to the new container view. The ref from the old
> > container view might be the last strong ref, so we want to ensure that the
> view
> > is not garbage collected before we have a chance to add it to the new
> container
> > view.
> 
> Is the user expected to use the AnchorView like below?
> 
> View view = acuqireAnchroView();
> ...
> releaseAnchorView(view);
> 
> It means user should have the reference of view, if no, he has no way to call
> releaseAnchorView(), then this is memory leak if we have strong reference of
> view here.

Why? When the user calls releaseAnchorView we also remove the entry from the
Map, so we don't longer hold the strong ref.

If the user does not call releaseAnchorView then we already have a "memory leak"
because we already have this other strong reference path:

ContentViewAndroidDelegateWrapper > RepleacebleViewAndroidDelegate >
mContainerViewAtCreation > anchorView


> Furthermore, if you can get the view from weak reference, you already get
strong
> reference of it, and it can't be gc-ed when you swap the container view.

Fair point, but given the above it is not very clear to me that we need weak
references at all. Also, the only collection that I could find that uses
WeakReferences is WeakHashMap. Is there a Weak list or WeakLinkedHashMap that I
could use? I couldn't find any, and I'd rather not implement one if I don't need
to.

[michaelbai, 2014-10-20 18:05:00]

On 2014/10/20 17:47:56, Ignacio Solla wrote:
> On 2014/10/20 17:19:50, michaelbai wrote:
> >
>
https://codereview.chromium.org/639413002/diff/280001/content/public/android/...
> > File
> >
>
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/639413002/diff/280001/content/public/android/...
> >
>
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java:58:
> > List<View> mAnchorViews;
> > On 2014/10/20 11:41:02, Ignacio Solla wrote:
> > > On 2014/10/17 17:30:55, michaelbai wrote:
> > > > We should use weak reference here 
> > > 
> > > No, we should use a strong reference here.
> > > 
> > > To transfer anchor views we need to remove them from the old container
view
> > > first before adding them to the new container view. The ref from the old
> > > container view might be the last strong ref, so we want to ensure that the
> > view
> > > is not garbage collected before we have a chance to add it to the new
> > container
> > > view.
> > 
> > Is the user expected to use the AnchorView like below?
> > 
> > View view = acuqireAnchroView();
> > ...
> > releaseAnchorView(view);
> > 
> > It means user should have the reference of view, if no, he has no way to
call
> > releaseAnchorView(), then this is memory leak if we have strong reference of
> > view here.
> 
> Why? When the user calls releaseAnchorView we also remove the entry from the
> Map, so we don't longer hold the strong ref.
> 
> If the user does not call releaseAnchorView then we already have a "memory
leak"
> because we already have this other strong reference path:
> 
> ContentViewAndroidDelegateWrapper > RepleacebleViewAndroidDelegate >
> mContainerViewAtCreation > anchorView
> 
> 
> > Furthermore, if you can get the view from weak reference, you already get
> strong
> > reference of it, and it can't be gc-ed when you swap the container view.
> 
> Fair point, but given the above it is not very clear to me that we need weak
> references at all. Also, the only collection that I could find that uses
> WeakReferences is WeakHashMap. Is there a Weak list or WeakLinkedHashMap that
I
> could use? I couldn't find any, and I'd rather not implement one if I don't
need
> to.

Actually, we should also has weak reference of mContainerViewAtCreation.
Yes, this is not a new issue introduced by your patch, it will be great if you
can fix it, otherwise, I am fine with it as long as content owner is happy.

[Ignacio Solla, 2014-10-20 18:22:37]

On 2014/10/20 18:05:00, michaelbai wrote:
> On 2014/10/20 17:47:56, Ignacio Solla wrote:
> > On 2014/10/20 17:19:50, michaelbai wrote:
> > >
> >
>
https://codereview.chromium.org/639413002/diff/280001/content/public/android/...
> > > File
> > >
> >
>
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/639413002/diff/280001/content/public/android/...
> > >
> >
>
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java:58:
> > > List<View> mAnchorViews;
> > > On 2014/10/20 11:41:02, Ignacio Solla wrote:
> > > > On 2014/10/17 17:30:55, michaelbai wrote:
> > > > > We should use weak reference here 
> > > > 
> > > > No, we should use a strong reference here.
> > > > 
> > > > To transfer anchor views we need to remove them from the old container
> view
> > > > first before adding them to the new container view. The ref from the old
> > > > container view might be the last strong ref, so we want to ensure that
the
> > > view
> > > > is not garbage collected before we have a chance to add it to the new
> > > container
> > > > view.
> > > 
> > > Is the user expected to use the AnchorView like below?
> > > 
> > > View view = acuqireAnchroView();
> > > ...
> > > releaseAnchorView(view);
> > > 
> > > It means user should have the reference of view, if no, he has no way to
> call
> > > releaseAnchorView(), then this is memory leak if we have strong reference
of
> > > view here.
> > 
> > Why? When the user calls releaseAnchorView we also remove the entry from the
> > Map, so we don't longer hold the strong ref.
> > 
> > If the user does not call releaseAnchorView then we already have a "memory
> leak"
> > because we already have this other strong reference path:
> > 
> > ContentViewAndroidDelegateWrapper > RepleacebleViewAndroidDelegate >
> > mContainerViewAtCreation > anchorView
> > 
> > 
> > > Furthermore, if you can get the view from weak reference, you already get
> > strong
> > > reference of it, and it can't be gc-ed when you swap the container view.
> > 
> > Fair point, but given the above it is not very clear to me that we need weak
> > references at all. Also, the only collection that I could find that uses
> > WeakReferences is WeakHashMap. Is there a Weak list or WeakLinkedHashMap
that
> I
> > could use? I couldn't find any, and I'd rather not implement one if I don't
> need
> > to.
> 
> Actually, we should also has weak reference of mContainerViewAtCreation.

why?

When mContainerViewAtCreation != mContainerView then there is not any strong ref
path to it because the ref from Wrapper to ReplaceableDelegate is removed, so
there is not any need to use weak references here.

While mContainerViewAtCreation == mContainerView then there is not any leak
because the value is current.


> Yes, this is not a new issue introduced by your patch, it will be great if you
> can fix it, otherwise, I am fine with it as long as content owner is happy.

As explained above, I cannot see any issues.

If you're happy then it'd be great if you could LGTM for the android_webview
part. Daniel has already noted that he is happy with this approach, but I will
obviously ask him to take another look after the changes and for his content/
LGTM separately. Thanks!

[michaelbai, 2014-10-20 18:50:08]

On 2014/10/20 18:22:37, Ignacio Solla wrote:
> On 2014/10/20 18:05:00, michaelbai wrote:
> > On 2014/10/20 17:47:56, Ignacio Solla wrote:
> > > On 2014/10/20 17:19:50, michaelbai wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/639413002/diff/280001/content/public/android/...
> > > > File
> > > >
> > >
> >
>
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/639413002/diff/280001/content/public/android/...
> > > >
> > >
> >
>
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java:58:
> > > > List<View> mAnchorViews;
> > > > On 2014/10/20 11:41:02, Ignacio Solla wrote:
> > > > > On 2014/10/17 17:30:55, michaelbai wrote:
> > > > > > We should use weak reference here 
> > > > > 
> > > > > No, we should use a strong reference here.
> > > > > 
> > > > > To transfer anchor views we need to remove them from the old container
> > view
> > > > > first before adding them to the new container view. The ref from the
old
> > > > > container view might be the last strong ref, so we want to ensure that
> the
> > > > view
> > > > > is not garbage collected before we have a chance to add it to the new
> > > > container
> > > > > view.
> > > > 
> > > > Is the user expected to use the AnchorView like below?
> > > > 
> > > > View view = acuqireAnchroView();
> > > > ...
> > > > releaseAnchorView(view);
> > > > 
> > > > It means user should have the reference of view, if no, he has no way to
> > call
> > > > releaseAnchorView(), then this is memory leak if we have strong
reference
> of
> > > > view here.
> > > 
> > > Why? When the user calls releaseAnchorView we also remove the entry from
the
> > > Map, so we don't longer hold the strong ref.
> > > 
> > > If the user does not call releaseAnchorView then we already have a "memory
> > leak"
> > > because we already have this other strong reference path:
> > > 
> > > ContentViewAndroidDelegateWrapper > RepleacebleViewAndroidDelegate >
> > > mContainerViewAtCreation > anchorView
> > > 
> > > 
> > > > Furthermore, if you can get the view from weak reference, you already
get
> > > strong
> > > > reference of it, and it can't be gc-ed when you swap the container view.
> > > 
> > > Fair point, but given the above it is not very clear to me that we need
weak
> > > references at all. Also, the only collection that I could find that uses
> > > WeakReferences is WeakHashMap. Is there a Weak list or WeakLinkedHashMap
> that
> > I
> > > could use? I couldn't find any, and I'd rather not implement one if I
don't
> > need
> > > to.
> > 
> > Actually, we should also has weak reference of mContainerViewAtCreation.
> 
> why?
> 
> When mContainerViewAtCreation != mContainerView then there is not any strong
ref
> path to it because the ref from Wrapper to ReplaceableDelegate is removed, so
> there is not any need to use weak references here.
> 
> While mContainerViewAtCreation == mContainerView then there is not any leak
> because the value is current.
> 
> 

ReplaceableViewAndroidDelegate has no reason to hold strong reference (own) of
container view. It probably doesn't break anything now (I didn't look into it),
future change might not be aware of it, this potential memory leak is hard to be
found. anyway, this could be addressed in other CL.


> > Yes, this is not a new issue introduced by your patch, it will be great if
you
> > can fix it, otherwise, I am fine with it as long as content owner is happy.
> 
> As explained above, I cannot see any issues.
> 
> If you're happy then it'd be great if you could LGTM for the android_webview
> part. Daniel has already noted that he is happy with this approach, but I will
> obviously ask him to take another look after the changes and for his content/
> LGTM separately. Thanks!

[michaelbai, 2014-10-20 18:50:25]

android_webview LGTM

[Ignacio Solla, 2014-10-21 10:20:29]

On 2014/10/20 18:50:08, michaelbai wrote:
> On 2014/10/20 18:22:37, Ignacio Solla wrote:
> > On 2014/10/20 18:05:00, michaelbai wrote:
> > > On 2014/10/20 17:47:56, Ignacio Solla wrote:
> > > > On 2014/10/20 17:19:50, michaelbai wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/639413002/diff/280001/content/public/android/...
> > > > > File
> > > > >
> > > >
> > >
> >
>
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java
> > > > > (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/639413002/diff/280001/content/public/android/...
> > > > >
> > > >
> > >
> >
>
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java:58:
> > > > > List<View> mAnchorViews;
> > > > > On 2014/10/20 11:41:02, Ignacio Solla wrote:
> > > > > > On 2014/10/17 17:30:55, michaelbai wrote:
> > > > > > > We should use weak reference here 
> > > > > > 
> > > > > > No, we should use a strong reference here.
> > > > > > 
> > > > > > To transfer anchor views we need to remove them from the old
container
> > > view
> > > > > > first before adding them to the new container view. The ref from the
> old
> > > > > > container view might be the last strong ref, so we want to ensure
that
> > the
> > > > > view
> > > > > > is not garbage collected before we have a chance to add it to the
new
> > > > > container
> > > > > > view.
> > > > > 
> > > > > Is the user expected to use the AnchorView like below?
> > > > > 
> > > > > View view = acuqireAnchroView();
> > > > > ...
> > > > > releaseAnchorView(view);
> > > > > 
> > > > > It means user should have the reference of view, if no, he has no way
to
> > > call
> > > > > releaseAnchorView(), then this is memory leak if we have strong
> reference
> > of
> > > > > view here.
> > > > 
> > > > Why? When the user calls releaseAnchorView we also remove the entry from
> the
> > > > Map, so we don't longer hold the strong ref.
> > > > 
> > > > If the user does not call releaseAnchorView then we already have a
"memory
> > > leak"
> > > > because we already have this other strong reference path:
> > > > 
> > > > ContentViewAndroidDelegateWrapper > RepleacebleViewAndroidDelegate >
> > > > mContainerViewAtCreation > anchorView
> > > > 
> > > > 
> > > > > Furthermore, if you can get the view from weak reference, you already
> get
> > > > strong
> > > > > reference of it, and it can't be gc-ed when you swap the container
view.
> > > > 
> > > > Fair point, but given the above it is not very clear to me that we need
> weak
> > > > references at all. Also, the only collection that I could find that uses
> > > > WeakReferences is WeakHashMap. Is there a Weak list or WeakLinkedHashMap
> > that
> > > I
> > > > could use? I couldn't find any, and I'd rather not implement one if I
> don't
> > > need
> > > > to.
> > > 
> > > Actually, we should also has weak reference of mContainerViewAtCreation.
> > 
> > why?
> > 
> > When mContainerViewAtCreation != mContainerView then there is not any strong
> ref
> > path to it because the ref from Wrapper to ReplaceableDelegate is removed,
so
> > there is not any need to use weak references here.
> > 
> > While mContainerViewAtCreation == mContainerView then there is not any leak
> > because the value is current.
> > 
> > 
> 
> ReplaceableViewAndroidDelegate has no reason to hold strong reference (own) of
> container view. It probably doesn't break anything now (I didn't look into
it),
> future change might not be aware of it, this potential memory leak is hard to
be
> found. anyway, this could be addressed in other CL.

The pointer ownership model is not used in Java (it is only used in very rare
exceptions). Instead, you should just let the garbage collector do its job.
WeakReferences are very rarely needed, and you should only use them if you have
a very good reason to. WeakRefences have two big drawbacks:
1. They can introduce very hard to debug bugs if used incorrectly
2. They increase the complexity of the code

Your suggestion of using WeakReferences for the mContainerViewAtCreation does
not really fix any problem, it only addresses a symptom of an improbable problem
and not the root cause. Nobody should hold a reference to the old
ReplaceableViewAndroidDelegate once the mContainerView has been replaced. It
that ever happens, then the fix would be to remove that reference, and not use
WeakRefences for the leaf nodes (ie. the mContainerViewAtCreation).

Anyway, I do not want this review to become a discussion about memory
management. If you still want to discuss this issue we can book some time
offline to talk in person, that'd be more convenient.

[no sievers, 2014-10-22 20:10:17]

https://codereview.chromium.org/639413002/diff/340001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java
(right):

https://codereview.chromium.org/639413002/diff/340001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java:17:
* Wrapper around a {@link ReplaceableViewAndroidDelegate} that transfers anchor
views
This feels weird that this class is a wrapper around an interface that is
internal to itself.

https://codereview.chromium.org/639413002/diff/340001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java:27:
/**
I think there are too many knots in this code now. You are subclassing
ViewAndroidDelegate twice here, while the one class extending it is a wrapper
around the other. Can we somehow untangle this a bit and make the code easier to
follow between here and ContentViewCore?

[Ignacio Solla, 2014-10-27 12:10:52]

https://codereview.chromium.org/639413002/diff/340001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java
(right):

https://codereview.chromium.org/639413002/diff/340001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java:17:
* Wrapper around a {@link ReplaceableViewAndroidDelegate} that transfers anchor
views
On 2014/10/22 20:10:17, sievers wrote:
> This feels weird that this class is a wrapper around an interface that is
> internal to itself.

See below.

https://codereview.chromium.org/639413002/diff/340001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java:27:
/**
On 2014/10/22 20:10:17, sievers wrote:
> I think there are too many knots in this code now. You are subclassing
> ViewAndroidDelegate twice here, while the one class extending it is a wrapper
> around the other. Can we somehow untangle this a bit and make the code easier
to
> follow between here and ContentViewCore?

I'm willing to compromise and have removed the ReplaceableViewAndroidDelegate
interface here but I don't think that that's better.

This interface is not adding a knot, it is actually doing the exact opposite,
ie. removing a knot (aka. circular dependency) between ContentViewCore and
ContentViewAndroidDelegateWrapper. Why do we care? Because it allows to test
ContentViewAndroidDelegateWrapper in isolation (which was my original idea, but
I ended up writing integration tests between both classes). I also believe that
it makes the code more readable.

Now let me give you some context about why do we need these layers:
- ContentViewAndroidDelegateWrapper is the ViewAndroidDelegate that works with
multiple container views. Clients can hold to instances of this class, and they
will continue to work transparently when container views change.
- ReplaceableViewAndroidDelegate only works with a single container view.

The alternative is to replace both classes with a huge and confusing monolithic
class inside ContentViewCore that implements both behaviours, but you would
agree that separation of concerns makes for better design (ie. the Single
responsibility principle).

[Ignacio Solla, 2014-10-27 13:16:01]

On 2014/10/27 12:10:52, Ignacio Solla wrote:
>
https://codereview.chromium.org/639413002/diff/340001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java
> (right):
> 
>
https://codereview.chromium.org/639413002/diff/340001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java:17:
> * Wrapper around a {@link ReplaceableViewAndroidDelegate} that transfers
anchor
> views
> On 2014/10/22 20:10:17, sievers wrote:
> > This feels weird that this class is a wrapper around an interface that is
> > internal to itself.
> 
> See below.
> 
>
https://codereview.chromium.org/639413002/diff/340001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/ContentViewAndroidDelegateWrapper.java:27:
> /**
> On 2014/10/22 20:10:17, sievers wrote:
> > I think there are too many knots in this code now. You are subclassing
> > ViewAndroidDelegate twice here, while the one class extending it is a
wrapper
> > around the other. Can we somehow untangle this a bit and make the code
easier
> to
> > follow between here and ContentViewCore?
> 
> I'm willing to compromise and have removed the ReplaceableViewAndroidDelegate
> interface here but I don't think that that's better.
> 
> This interface is not adding a knot, it is actually doing the exact opposite,
> ie. removing a knot (aka. circular dependency) between ContentViewCore and
> ContentViewAndroidDelegateWrapper. Why do we care? Because it allows to test
> ContentViewAndroidDelegateWrapper in isolation (which was my original idea,
but
> I ended up writing integration tests between both classes). I also believe
that
> it makes the code more readable.
> 
> Now let me give you some context about why do we need these layers:
> - ContentViewAndroidDelegateWrapper is the ViewAndroidDelegate that works with
> multiple container views. Clients can hold to instances of this class, and
they
> will continue to work transparently when container views change.
> - ReplaceableViewAndroidDelegate only works with a single container view.
> 
> The alternative is to replace both classes with a huge and confusing
monolithic
> class inside ContentViewCore that implements both behaviours, but you would
> agree that separation of concerns makes for better design (ie. the Single
> responsibility principle).

(or there may be something better that I have missed, not the first time :) )

[no sievers, 2014-10-27 21:37:03]

In this case I'd find the code much easier to follow if it was encapsulated in
the one impl class in ContentViewCore.java since we only ever instantiate the
leaf class anyways and it's not adding that much functionality (just remembering
anchor views and positions).

And for the other interface (ReplaceableAndroidViewDelegate) do you think we
could just change all call sites (there seem to be few) and remove
acquireAnchorView() in favor of addAnchorView() so we don't need that extra
interface?

[Ignacio Solla, 2014-10-28 10:52:10]

On 2014/10/27 21:37:03, sievers wrote:
> In this case I'd find the code much easier to follow if it was encapsulated in
> the one impl class in ContentViewCore.java since we only ever instantiate the
> leaf class anyways and it's not adding that much functionality (just
remembering
> anchor views and positions).
> 
> And for the other interface (ReplaceableAndroidViewDelegate) do you think we
> could just change all call sites (there seem to be few) and remove
> acquireAnchorView() in favor of addAnchorView() so we don't need that extra
> interface?

Fine, I have encapsulated all the code into one class. We don't longer need the
addAnchorView method.

[no sievers, 2014-10-28 18:34:14]

lgtm

https://codereview.chromium.org/639413002/diff/400001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/639413002/diff/400001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:158:
private class Position {
nit: You can use android.graphics.Rect

https://codereview.chromium.org/639413002/diff/400001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:726:
void setContentViewAndroidDelegate() {
nit: createContentViewAndroidDelegate()

https://codereview.chromium.org/639413002/diff/400001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:731:
void setViewAndroid(WindowAndroid windowAndroid) {
nit: createViewAndroid()

https://codereview.chromium.org/639413002/diff/400001/content/public/android/...
File
content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreViewAndroidDelegateTest.java
(right):

https://codereview.chromium.org/639413002/diff/400001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreViewAndroidDelegateTest.java:198:
return originalLayoutParams;
nit: drop the 'original' prefix since these are actually the ones from after the
udpate

[Ignacio Solla, 2014-10-30 16:01:39]

https://codereview.chromium.org/639413002/diff/400001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/639413002/diff/400001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:158:
private class Position {
On 2014/10/28 18:34:13, sievers wrote:
> nit: You can use android.graphics.Rect

Rect defines 2 coordinates. Position defines 1 coordinate and two dimensions. To
avoid converting from/to dimensions into coordinates (we need to convert because
of the preconditions left <= right and top <= bottom), I'd like keep this class.

https://codereview.chromium.org/639413002/diff/400001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:726:
void setContentViewAndroidDelegate() {
On 2014/10/28 18:34:13, sievers wrote:
> nit: createContentViewAndroidDelegate()

Done.

https://codereview.chromium.org/639413002/diff/400001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:731:
void setViewAndroid(WindowAndroid windowAndroid) {
On 2014/10/28 18:34:13, sievers wrote:
> nit: createViewAndroid()

Done.

https://codereview.chromium.org/639413002/diff/400001/content/public/android/...
File
content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreViewAndroidDelegateTest.java
(right):

https://codereview.chromium.org/639413002/diff/400001/content/public/android/...
content/public/android/javatests/src/org/chromium/content/browser/ContentViewCoreViewAndroidDelegateTest.java:198:
return originalLayoutParams;
On 2014/10/28 18:34:13, sievers wrote:
> nit: drop the 'original' prefix since these are actually the ones from after
the
> udpate

Done. (we don't actually need to define the var)

[Ignacio Solla, 2014-10-30 16:01:53]

The CQ bit was checked by igsolla@chromium.org

[commit-bot: I haz the power, 2014-10-30 16:03:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/639413002/420001

[commit-bot: I haz the power, 2014-10-30 16:52:34]

Committed patchset #13 (id:420001)

[commit-bot: I haz the power, 2014-10-30 16:53:19]

Patchset 13 (id:??) landed as
https://crrev.com/c698e079008a03048933818525310fb34908a10a
Cr-Commit-Position: refs/heads/master@{#302094}