Refactor ContentViewClient (4/6)

Refactor ContentViewClient (4/6)

Moved following interface methods to ViewAndroid:
 - onBackgroundColorChanged
 - onTopControlsChanged
 - onBottomControlsChanged
 - onStartContentIntent

Remove following as they are not used:
 - getSystemWindowInsetTop
 - getSystemWindowInsetLeft
 - getSystemWindowInsetRight

BUG=620172
Review-Url: https://codereview.chromium.org/2682593002
Cr-Commit-Position: refs/heads/master@{#449186}
Committed: https://chromium.googlesource.com/chromium/src/+/d95ac3f4aad80e2102fd8cf957dc2e265c0f02f7

[Jinsuk Kim, 2017-02-07 06:36:55]

Patchset #1 (id:1) has been deleted

[Jinsuk Kim, 2017-02-07 06:37:08]

Patchset #1 (id:20001) has been deleted

[Jinsuk Kim, 2017-02-07 06:37:32]

Description was changed from

==========
Refactor ContentViewClient (4/6)

Moved following interface methods to ViewAndroid:
 - onBackgroundColorChanged
 - onTopControlsChanged
 - onBottomControlsChanged

Remove following as they are not used:
 - getSystemWindowInsetTop
 - getSystemWindowInsetLeft
 - getSystemWindowInsetRight

Moved the default implementation of the method onStartContentIntent
to Chrome.

BUG=620172
==========

to

==========
Refactor ContentViewClient (4/6)

Moved following interface methods to ViewAndroid:
 - onBackgroundColorChanged
 - onTopControlsChanged
 - onBottomControlsChanged
 - onStartContentIntent

Remove following as they are not used:
 - getSystemWindowInsetTop
 - getSystemWindowInsetLeft
 - getSystemWindowInsetRight

Moved the default implementation of the method onStartContentIntent
to Chrome.

BUG=620172
==========

[Jinsuk Kim, 2017-02-07 06:40:00]

jinsukkim@chromium.org changed reviewers:
+ boliu@chromium.org

[Jinsuk Kim, 2017-02-07 06:40:00]

Notes: these are more of mechanical migration from ContentViewClient to
ViewAndroidDelegate.

|onBackgroundColorChanged| reviewed before:
https://codereview.chromium.org/2536223003/#msg20

|onTopControlsChanged, onBottomControlsChanged| is a partial migration of
CVC::updateFrameInfo Please take a look and let me know if it looks okay to do
this migration together with the others.

|onStartContentIntent| reviewed before:
https://codereview.chromium.org/2380743003/#msg15 

I supposed onStartContentIntent's default implementation is for
Chrome/ContentShell, but the content intent on ContentShell as of now doesn't
seem to work as per my testing. So made it Chrome only. If it is a bug, I'll
reinstate the default impl.

[Jinsuk Kim, 2017-02-07 07:19:35]

Patchset #1 (id:40001) has been deleted

[boliu, 2017-02-07 21:47:07]

https://codereview.chromium.org/2682593002/diff/60001/content/browser/android...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/2682593002/diff/60001/content/browser/android...
content/browser/android/content_view_core_impl.cc:414: void
ContentViewCoreImpl::UpdateFrameInfo(
how much of this method can be cleaned up? I see top control is still needed,
but can bottom bar info be removed?

https://codereview.chromium.org/2682593002/diff/60001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right):

https://codereview.chromium.org/2682593002/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:112: * Called
whenever the background color of the page changes as notified by WebKit.
blink :p

https://codereview.chromium.org/2682593002/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:140: protected
static boolean isAcceptableContentIntentScheme(String scheme) {
you can do a step further here to reduce code duplication: put the entire uri
parsing and scheme check in this class, and don't call subclass method if scheme
is not valid at all

https://codereview.chromium.org/2682593002/diff/60001/ui/android/view_android.cc
File ui/android/view_android.cc (right):

https://codereview.chromium.org/2682593002/diff/60001/ui/android/view_android...
ui/android/view_android.cc:198: return
Java_ViewAndroidDelegate_onBackgroundColorChanged(env,
not return for void methods

[Jinsuk Kim, 2017-02-07 23:46:04]

Description was changed from

==========
Refactor ContentViewClient (4/6)

Moved following interface methods to ViewAndroid:
 - onBackgroundColorChanged
 - onTopControlsChanged
 - onBottomControlsChanged
 - onStartContentIntent

Remove following as they are not used:
 - getSystemWindowInsetTop
 - getSystemWindowInsetLeft
 - getSystemWindowInsetRight

Moved the default implementation of the method onStartContentIntent
to Chrome.

BUG=620172
==========

to

==========
Refactor ContentViewClient (4/6)

Moved following interface methods to ViewAndroid:
 - onBackgroundColorChanged
 - onTopControlsChanged
 - onBottomControlsChanged
 - onStartContentIntent

Remove following as they are not used:
 - getSystemWindowInsetTop
 - getSystemWindowInsetLeft
 - getSystemWindowInsetRight

BUG=620172
==========

[Jinsuk Kim, 2017-02-07 23:46:17]

Patchset #2 (id:80001) has been deleted

[Jinsuk Kim, 2017-02-07 23:51:03]

jinsukkim@chromium.org changed reviewers:
+ dtrainor@chromium.org

[Jinsuk Kim, 2017-02-07 23:51:04]

dtrainor@: Please review changes in chrome/

https://codereview.chromium.org/2682593002/diff/60001/content/browser/android...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/2682593002/diff/60001/content/browser/android...
content/browser/android/content_view_core_impl.cc:414: void
ContentViewCoreImpl::UpdateFrameInfo(
On 2017/02/07 21:47:06, boliu wrote:
> how much of this method can be cleaned up? I see top control is still needed,
> but can bottom bar info be removed?

Yes. Done. Also removed the relevant var/method all the way to
RenderCoordinates.

https://codereview.chromium.org/2682593002/diff/60001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java (right):

https://codereview.chromium.org/2682593002/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:112: * Called
whenever the background color of the page changes as notified by WebKit.
On 2017/02/07 21:47:06, boliu wrote:
> blink :p

Done.

https://codereview.chromium.org/2682593002/diff/60001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/ViewAndroidDelegate.java:140: protected
static boolean isAcceptableContentIntentScheme(String scheme) {
On 2017/02/07 21:47:06, boliu wrote:
> you can do a step further here to reduce code duplication: put the entire uri
> parsing and scheme check in this class, and don't call subclass method if
scheme
> is not valid at all

Good point. Done.

https://codereview.chromium.org/2682593002/diff/60001/ui/android/view_android.cc
File ui/android/view_android.cc (right):

https://codereview.chromium.org/2682593002/diff/60001/ui/android/view_android...
ui/android/view_android.cc:198: return
Java_ViewAndroidDelegate_onBackgroundColorChanged(env,
On 2017/02/07 21:47:06, boliu wrote:
> not return for void methods

Done. Interesting the compiler does't complain...

[boliu, 2017-02-07 23:54:32]

lgtm

[David Trainor- moved to gerrit, 2017-02-08 07:43:14]

chrome/ lgtm

[Jinsuk Kim, 2017-02-08 08:45:40]

The CQ bit was checked by jinsukkim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 08:45:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-08 09:35:28]

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

[commit-bot: I haz the power, 2017-02-08 09:35:29]

Dry run: This issue passed the CQ dry run.

[Jinsuk Kim, 2017-02-08 10:18:56]

boliu@: maybe you want to take a look again? Fixed a bunch of tests that
override ViewAndroidDelegate.onStartContentIntent.

Could use |CVC.setContentViewClient| when onStartContentIntent was in
ContentViewClient, but not any more now that it was moved to VAD. Chose to add a
new class  ContentViewAndroidDelegate that can override the method via injecting
a handler rather than adding |CVC.setViewAndroidDelegateForTesting|, because it
would also need a new JNI to set the VAD for testing to native ViewAndroid. I
think the former is better in the sense that it only affects ContentShell and
its test, not the other embedders. It will be removed by Proguard when other
embedders are built.

[boliu, 2017-02-08 18:51:51]

as a general rule about rebase and code reviews:

upload a patch set of just the rebase, so reviewers can ignore it, then upload a
patch of just the additional changes, then reviewer can diff the two to see
additional changes without changes from rebase polluting the diff

https://codereview.chromium.org/2682593002/diff/120001/content/public/android...
File content/public/android/BUILD.gn (right):

https://codereview.chromium.org/2682593002/diff/120001/content/public/android...
content/public/android/BUILD.gn:128:
"java/src/org/chromium/content/browser/ContentViewAndroidDelegate.java",
this looks like is only used by tests and content shell, why is it built into
the core content library?

https://codereview.chromium.org/2682593002/diff/120001/content/public/test/an...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java
(right):

https://codereview.chromium.org/2682593002/diff/120001/content/public/test/an...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java:20:

nit: remove this empty line

[Jinsuk Kim, 2017-02-08 22:49:39]

PTAL

https://codereview.chromium.org/2682593002/diff/120001/content/public/android...
File content/public/android/BUILD.gn (right):

https://codereview.chromium.org/2682593002/diff/120001/content/public/android...
content/public/android/BUILD.gn:128:
"java/src/org/chromium/content/browser/ContentViewAndroidDelegate.java",
On 2017/02/08 18:51:51, boliu wrote:
> this looks like is only used by tests and content shell, why is it built into
> the core content library?

Moved to content/shell.

Not related to this CL but there's another class (ContentViewRenderView) in the
core content but used for content/cast shell only. What is the better place for
it?

https://codereview.chromium.org/2682593002/diff/120001/content/public/test/an...
File
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java
(right):

https://codereview.chromium.org/2682593002/diff/120001/content/public/test/an...
content/public/test/android/javatests/src/org/chromium/content/browser/test/util/TestCallbackHelperContainer.java:20:

On 2017/02/08 18:51:51, boliu wrote:
> nit: remove this empty line

Done.

[boliu, 2017-02-08 22:58:18]

lgtm

https://codereview.chromium.org/2682593002/diff/120001/content/public/android...
File content/public/android/BUILD.gn (right):

https://codereview.chromium.org/2682593002/diff/120001/content/public/android...
content/public/android/BUILD.gn:128:
"java/src/org/chromium/content/browser/ContentViewAndroidDelegate.java",
On 2017/02/08 22:49:39, Jinsuk Kim wrote:
> On 2017/02/08 18:51:51, boliu wrote:
> > this looks like is only used by tests and content shell, why is it built
into
> > the core content library?
> 
> Moved to content/shell.
> 
> Not related to this CL but there's another class (ContentViewRenderView) in
the
> core content but used for content/cast shell only. What is the better place
for
> it? 

We had this confusion before. ContentViewRenderView is used by chromecast, so it
technically is part of core content..

[Jinsuk Kim, 2017-02-08 23:00:51]

The CQ bit was checked by jinsukkim@chromium.org

[Jinsuk Kim, 2017-02-08 23:00:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2682593002/#ps140001
(title: "shell vad")

[commit-bot: I haz the power, 2017-02-08 23:01:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 01:57:23]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1486594851436750,
"parent_rev": "f2ad0c868e66c4692862ed29ff6d30a97440c0f0", "commit_rev":
"d95ac3f4aad80e2102fd8cf957dc2e265c0f02f7"}

[commit-bot: I haz the power, 2017-02-09 01:57:59]

Description was changed from

==========
Refactor ContentViewClient (4/6)

Moved following interface methods to ViewAndroid:
 - onBackgroundColorChanged
 - onTopControlsChanged
 - onBottomControlsChanged
 - onStartContentIntent

Remove following as they are not used:
 - getSystemWindowInsetTop
 - getSystemWindowInsetLeft
 - getSystemWindowInsetRight

BUG=620172
==========

to

==========
Refactor ContentViewClient (4/6)

Moved following interface methods to ViewAndroid:
 - onBackgroundColorChanged
 - onTopControlsChanged
 - onBottomControlsChanged
 - onStartContentIntent

Remove following as they are not used:
 - getSystemWindowInsetTop
 - getSystemWindowInsetLeft
 - getSystemWindowInsetRight

BUG=620172

Review-Url: https://codereview.chromium.org/2682593002
Cr-Commit-Position: refs/heads/master@{#449186}
Committed:
https://chromium.googlesource.com/chromium/src/+/d95ac3f4aad80e2102fd8cf957dc...
==========

[commit-bot: I haz the power, 2017-02-09 01:58:01]

Committed patchset #4 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/d95ac3f4aad80e2102fd8cf957dc...

[amineer, 2017-02-14 18:27:44]

A revert of this CL (patchset #4 id:140001) has been created in
https://codereview.chromium.org/2694273002/ by amineer@chromium.org.

The reason for reverting is: Causing major alignment issues for custom tabs

BUG=691718.