Add support for ContentViewCore helpers on Tab

Add suppor for ContentViewCore helpers on Tab

- Add a concept of a helper ContentViewCore to Tab.
- Properly attach these ContentViewCores to the Tabs rendering layer.
- Note that these aren't rendered by ContentViewRenderView yet.  We need a Chrome-level
rendering object for that.

BUG=

Committed: https://crrev.com/307787f6e1fd7f3affa872faf6e2efefcc51eb73
Cr-Commit-Position: refs/heads/master@{#315405}

[David Trainor- moved to gerrit, 2015-02-02 22:17:03]

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

[David Trainor- moved to gerrit, 2015-02-02 22:17:04]

ptal.  Initial work on pulling feature-specific CVC/Views out of the downstream
compositor classes.  See downstream CL as well for reference.

[David Trainor- moved to gerrit, 2015-02-02 23:28:55]

dtrainor@chromium.org changed reviewers:
+ changwan@chromium.org, jaekyun@chromium.org

[David Trainor- moved to gerrit, 2015-02-04 21:19:34]

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

[gone, 2015-02-05 00:06:02]

I get the gist of the CL, but I'm honestly having trouble following the layer
code -- I'm not too familiar with it.

https://chromiumcodereview.appspot.com/900443003/diff/20001/chrome/android/ja...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://chromiumcodereview.appspot.com/900443003/diff/20001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:158: * be sized and
rendered along side this {@link Tab}s content.
alongside, indentation fix

https://chromiumcodereview.appspot.com/900443003/diff/20001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:1298: // TODO:
Properly size this new ContentViewCore for the first time.
TODO(dtrainor)?

[David Trainor- moved to gerrit, 2015-02-05 23:37:54]

https://chromiumcodereview.appspot.com/900443003/diff/20001/chrome/android/ja...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://chromiumcodereview.appspot.com/900443003/diff/20001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:158: * be sized and
rendered along side this {@link Tab}s content.
On 2015/02/05 00:06:02, dfalcantara wrote:
> alongside, indentation fix

Done.

https://chromiumcodereview.appspot.com/900443003/diff/20001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:1298: // TODO:
Properly size this new ContentViewCore for the first time.
On 2015/02/05 00:06:02, dfalcantara wrote:
> TODO(dtrainor)?

Ah I actually took care of this with the listener.  I can remove this.

[Ted C, 2015-02-06 01:52:40]

https://chromiumcodereview.appspot.com/900443003/diff/60001/chrome/android/ja...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://chromiumcodereview.appspot.com/900443003/diff/60001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:1284:
content.add(mContentViewCore);
should we be adding this if we're currently showing a native page?

https://chromiumcodereview.appspot.com/900443003/diff/60001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:1316:
mOverlayContentViewCores.remove(content);
should we be asserting the return value is true just to make sure it was in the
list?

https://chromiumcodereview.appspot.com/900443003/diff/60001/chrome/browser/an...
File chrome/browser/android/tab_android.cc (right):

https://chromiumcodereview.appspot.com/900443003/diff/60001/chrome/browser/an...
chrome/browser/android/tab_android.cc:122:
GetContentLayer()->RemoveAllChildren();
will this not happen when it gets destroyed?

https://chromiumcodereview.appspot.com/900443003/diff/60001/chrome/browser/an...
chrome/browser/android/tab_android.cc:462:
content_view_core->GetLayer()->RemoveFromParent();
is this not what the change in CVCImpl.cc is doing?  Just trying to figure out
what is different between the two.

https://chromiumcodereview.appspot.com/900443003/diff/60001/chrome/browser/an...
chrome/browser/android/tab_android.cc:782: if (!content_view_core)
should we allow this? same below

https://chromiumcodereview.appspot.com/900443003/diff/60001/chrome/browser/an...
chrome/browser/android/tab_android.cc:805: content::ContentViewCore*
content_view_core =
seems odd to have this method on tab when it doesn't actually need tab at all
and could be static.

https://chromiumcodereview.appspot.com/900443003/diff/60001/chrome/browser/an...
File chrome/browser/android/tab_android.h (right):

https://chromiumcodereview.appspot.com/900443003/diff/60001/chrome/browser/an...
chrome/browser/android/tab_android.h:199: jobject obj,
indenting is off by 2

[David Trainor- moved to gerrit, 2015-02-06 18:37:04]

https://codereview.chromium.org/900443003/diff/60001/chrome/android/java/src/...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://codereview.chromium.org/900443003/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:1284:
content.add(mContentViewCore);
On 2015/02/06 01:52:39, Ted C wrote:
> should we be adding this if we're currently showing a native page?

Good point, we probably shouldn't.  I'll take another look at the current logic.

https://codereview.chromium.org/900443003/diff/60001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:1316:
mOverlayContentViewCores.remove(content);
On 2015/02/06 01:52:39, Ted C wrote:
> should we be asserting the return value is true just to make sure it was in
the
> list?

Sure we can do that.  I should probably also null check this.

https://codereview.chromium.org/900443003/diff/60001/chrome/browser/android/t...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/900443003/diff/60001/chrome/browser/android/t...
chrome/browser/android/tab_android.cc:122:
GetContentLayer()->RemoveAllChildren();
On 2015/02/06 01:52:39, Ted C wrote:
> will this not happen when it gets destroyed?

It's a scoped_refptr, so if it's attached to another object it could stay around
(ie: if it's stored as a parent of another tab or a child of another tab?).

https://codereview.chromium.org/900443003/diff/60001/chrome/browser/android/t...
chrome/browser/android/tab_android.cc:462:
content_view_core->GetLayer()->RemoveFromParent();
On 2015/02/06 01:52:39, Ted C wrote:
> is this not what the change in CVCImpl.cc is doing?  Just trying to figure out
> what is different between the two.

This doesn't explicitly call delete content_view_core, so I wanted it to clean
it's own state up gracefully (I believe the logic is the same as pre-patch
here).

content_view_core_impl is a little different, but more safe when used with
overlays I think.  I added the method to content_view_core_impl's destructor
because for things like overlays destroy()'ing the cvc shouldn't leak the cc
layer by leaving it around as a child node of something else.

https://codereview.chromium.org/900443003/diff/60001/chrome/browser/android/t...
chrome/browser/android/tab_android.cc:782: if (!content_view_core)
On 2015/02/06 01:52:39, Ted C wrote:
> should we allow this? same below

Should we not allow null to be even passed in here?  I can DCHECK, we should
also catch this in Java :(.

https://codereview.chromium.org/900443003/diff/60001/chrome/browser/android/t...
chrome/browser/android/tab_android.cc:805: content::ContentViewCore*
content_view_core =
On 2015/02/06 01:52:39, Ted C wrote:
> seems odd to have this method on tab when it doesn't actually need tab at all
> and could be static.

Yeah I thought about that too.  I can move it to ContentViewUtils or something. 
The main issue is that I don't want the helpers to have to spin up a native
object/JNI stuff just to set the visibility of their ContentViewCore.  Actually
maybe this should be a method on ContentViewCore.java itself...

[David Trainor- moved to gerrit, 2015-02-06 22:10:24]

https://chromiumcodereview.appspot.com/900443003/diff/60001/chrome/android/ja...
File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right):

https://chromiumcodereview.appspot.com/900443003/diff/60001/chrome/android/ja...
chrome/android/java/src/org/chromium/chrome/browser/Tab.java:1316:
mOverlayContentViewCores.remove(content);
On 2015/02/06 18:37:04, David Trainor wrote:
> On 2015/02/06 01:52:39, Ted C wrote:
> > should we be asserting the return value is true just to make sure it was in
> the
> > list?
> 
> Sure we can do that.  I should probably also null check this.

Ended up just doing an assert mOverlayContentViewCores.contains() because
otherwise I have to save the return value and assert on that (which could get
marked as unused on certain builds IIUC).

https://chromiumcodereview.appspot.com/900443003/diff/60001/chrome/browser/an...
File chrome/browser/android/tab_android.h (right):

https://chromiumcodereview.appspot.com/900443003/diff/60001/chrome/browser/an...
chrome/browser/android/tab_android.h:199: jobject obj,
On 2015/02/06 01:52:39, Ted C wrote:
> indenting is off by 2

Done.

[Ted C, 2015-02-06 22:37:26]

lgtm

https://chromiumcodereview.appspot.com/900443003/diff/80001/content/public/an...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://chromiumcodereview.appspot.com/900443003/diff/80001/content/public/an...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1356:
* Sets whether or not this {@link ContentViewCore} will visibly display it's
content.
might want to say this defaults to true so that people aren't calling this
unnecessarily.

[David Trainor- moved to gerrit, 2015-02-06 23:13:33]

https://chromiumcodereview.appspot.com/900443003/diff/80001/content/public/an...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://chromiumcodereview.appspot.com/900443003/diff/80001/content/public/an...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1356:
* Sets whether or not this {@link ContentViewCore} will visibly display it's
content.
On 2015/02/06 22:37:26, Ted C wrote:
> might want to say this defaults to true so that people aren't calling this
> unnecessarily.

Good point!

[David Trainor- moved to gerrit, 2015-02-06 23:16:43]

New patchsets have been uploaded after l-g-t-m from tedchoc@chromium.org

[David Trainor- moved to gerrit, 2015-02-07 00:11:18]

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

[David Trainor- moved to gerrit, 2015-02-07 00:11:19]

enne@: Can you give me an OWNERS review for the DEPS change?  Thanks!

[enne (OOO), 2015-02-07 00:53:33]

I'm not an OWNER there, but lgtm to include and use cc::Layer in that way.

[David Trainor- moved to gerrit, 2015-02-07 01:01:56]

New patchsets have been uploaded after l-g-t-m from enne@chromium.org

[David Trainor- moved to gerrit, 2015-02-07 01:03:11]

On 2015/02/07 00:53:33, enne wrote:
> I'm not an OWNER there, but lgtm to include and use cc::Layer in that way.

Ah it seemed to say that I needed an owner of dependencies added to DEPS (so
cc/.).  I think you worked as it's no longer giving me that presubmit error (at
least not on uploading patchsets).  Thanks :).

[David Trainor- moved to gerrit, 2015-02-09 21:33:02]

The CQ bit was checked by dtrainor@chromium.org

[commit-bot: I haz the power, 2015-02-09 21:34:38]

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

[commit-bot: I haz the power, 2015-02-09 22:14:35]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-09 22:15:30]

Patchset 7 (id:??) landed as
https://crrev.com/307787f6e1fd7f3affa872faf6e2efefcc51eb73
Cr-Commit-Position: refs/heads/master@{#315405}

[David Trainor- moved to gerrit, 2015-02-10 01:37:03]

A revert of this CL (patchset #7 id:120001) has been created in
https://chromiumcodereview.appspot.com/908093003/ by dtrainor@chromium.org.

The reason for reverting is: Downstream CL didn't land cleanly.  Need to
revert..