[Contextual Search] Fixes ContentView regressions.

[Contextual Search] Fixes ContentView regressions.

This CL fixes important regressions in the way ContentViews
are rendered in Contextual Search.

All loaded searches were being made visible, even when the
user didn't interact with the Panel. Now there's no way to
control the visibility of the Content from outside its class.
Instead, consumers should call acknowledgeIntentToShowContent()
to signal that there's an intent to show the Content, but only
the Content will actually control its visibility, which makes
the code more robust.

Also, the OverlayPanelContent class wasn't properly clearing
its internal states, which was causing other bugs. These bugs
happened, in part, because we are exposing methods to create
and destroy the ContentView. Also, tests were not creating
ContentViews, which the made the code confusing.

So, instead of making such method public, so they can be
overriden by tests (in order to keep track of whether
ContentViews have been created), now those methods are
private, and a new method didCreateContentView() has
been exposed so that the tests can assert the ContentView's
existence.

BUG=542785
BUG=542786
BUG=542790
Committed: https://crrev.com/9893e6aff9f1811ca5e0e3b3ab4f9fac19afec92
Cr-Commit-Position: refs/heads/master@{#354161}

[pedro (no code reviews), 2015-10-13 18:42:05]

pedrosimonetti@chromium.org changed reviewers:
+ donnd@chromium.org, dtrainor@google.com

[pedro (no code reviews), 2015-10-13 18:42:05]

Hey guys, please take a look at this change.

[pedro (no code reviews), 2015-10-13 18:42:47]

pedrosimonetti@chromium.org changed reviewers:
+ dtrainor@chromium.org
- dtrainor@google.com

[pedro (no code reviews), 2015-10-13 18:43:09]

Oops. Now adding the right dtrainor@!

[Donn Denman, 2015-10-13 21:06:21]

Pedro, I think this is close, but have a question about tests ,and nits and
typos.

https://codereview.chromium.org/1403813003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java
(right):

https://codereview.chromium.org/1403813003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:68:
* The observer used by this object to inform inplementers of different events.
Not in this CL, but fix typo (inplementers).

https://codereview.chromium.org/1403813003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:113:
mOverlayObserver = contentDelegate;
Maybe rename the private member too?

https://codereview.chromium.org/1403813003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:254:
* visible, therefore making the Content to be rendered.
This phrase is a bit awkward, I would change it to read "visible, rendering the
Content."

https://codereview.chromium.org/1403813003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:318:
// NOTE(pedrosimonetti): Calling onShow() on the ContentViewCore will make the
page
Here too -- maybe change to "will render the page."

https://codereview.chromium.org/1403813003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java
(right):

https://codereview.chromium.org/1403813003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:123:
private ContextualSearchPanelMetrics mPanelMetrics;
Not in this CL, but make final?

https://codereview.chromium.org/1403813003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:545:
public void destroyOverlayPanelContent() {
Add javadoc, or make private.

https://codereview.chromium.org/1403813003/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java
(right):

https://codereview.chromium.org/1403813003/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java:51:
mContentDelegate = contentDelegate;
Looks like all three of these can be made final.

https://codereview.chromium.org/1403813003/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java:64:
// when under test -- see comments in createNewSearchContentView.
I can't find createNewSearchContentView anymore, update or remove this section
of comment.

https://codereview.chromium.org/1403813003/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java:70:
// Override to prevent call to native code.
Seems like it would be good to instrument this too.  Maybe add a TODO for that? 
Not sure, since we want to get rid of all this anyway.

https://codereview.chromium.org/1403813003/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java
(right):

https://codereview.chromium.org/1403813003/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:296:
private void assertContentViewCoreCreated() {
I would think we could add tests, or update existing tests, to verify that the
recent regressions are not happening anymore.  Maybe the difficulty is in
checking whether a history entry was created or not?  Or if you want to merge
this in ASAP you could put in a TODO and address in a separate CL.

[mdjones, 2015-10-14 00:47:30]

mdjones@chromium.org changed reviewers:
+ mdjones@chromium.org

[mdjones, 2015-10-14 00:47:30]

https://codereview.chromium.org/1403813003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java
(right):

https://codereview.chromium.org/1403813003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:149:
destroyPromoView();
I think promo view creation should be controlled by this class if destruction is
going to be (currently in ContextualSearchLayout).

[pedro (no code reviews), 2015-10-14 18:07:06]

David, please take a look.

https://codereview.chromium.org/1403813003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java
(right):

https://codereview.chromium.org/1403813003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:68:
* The observer used by this object to inform inplementers of different events.
On 2015/10/13 21:06:21, Donn Denman wrote:
> Not in this CL, but fix typo (inplementers).

Done.

https://codereview.chromium.org/1403813003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:113:
mOverlayObserver = contentDelegate;
On 2015/10/13 21:06:21, Donn Denman wrote:
> Maybe rename the private member too?

Done.

https://codereview.chromium.org/1403813003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:254:
* visible, therefore making the Content to be rendered.
On 2015/10/13 21:06:21, Donn Denman wrote:
> This phrase is a bit awkward, I would change it to read "visible, rendering
the
> Content."

Rephrased it.

https://codereview.chromium.org/1403813003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:318:
// NOTE(pedrosimonetti): Calling onShow() on the ContentViewCore will make the
page
On 2015/10/13 21:06:21, Donn Denman wrote:
> Here too -- maybe change to "will render the page."

Rephrased it.

https://codereview.chromium.org/1403813003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java
(right):

https://codereview.chromium.org/1403813003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:123:
private ContextualSearchPanelMetrics mPanelMetrics;
On 2015/10/13 21:06:21, Donn Denman wrote:
> Not in this CL, but make final?

Done.

https://codereview.chromium.org/1403813003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:149:
destroyPromoView();
On 2015/10/14 00:47:30, mdjones wrote:
> I think promo view creation should be controlled by this class if destruction
is
> going to be (currently in ContextualSearchLayout).

I'm not sure if I understood your comment. This class is already controlling the
creation of the promo (see createPromoView()).

https://codereview.chromium.org/1403813003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:545:
public void destroyOverlayPanelContent() {
On 2015/10/13 21:06:21, Donn Denman wrote:
> Add javadoc, or make private.

Done.

https://codereview.chromium.org/1403813003/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java
(right):

https://codereview.chromium.org/1403813003/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java:51:
mContentDelegate = contentDelegate;
On 2015/10/13 21:06:21, Donn Denman wrote:
> Looks like all three of these can be made final.

Done.

https://codereview.chromium.org/1403813003/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java:64:
// when under test -- see comments in createNewSearchContentView.
On 2015/10/13 21:06:21, Donn Denman wrote:
> I can't find createNewSearchContentView anymore, update or remove this section
> of comment.

Removed obsolete comment.

https://codereview.chromium.org/1403813003/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchFakeServer.java:70:
// Override to prevent call to native code.
On 2015/10/13 21:06:21, Donn Denman wrote:
> Seems like it would be good to instrument this too.  Maybe add a TODO for
that? 
> Not sure, since we want to get rid of all this anyway.

I don't think this is necessary.

https://codereview.chromium.org/1403813003/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java
(right):

https://codereview.chromium.org/1403813003/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:296:
private void assertContentViewCoreCreated() {
On 2015/10/13 21:06:21, Donn Denman wrote:
> I would think we could add tests, or update existing tests, to verify that the
> recent regressions are not happening anymore.  Maybe the difficulty is in
> checking whether a history entry was created or not?  Or if you want to merge
> this in ASAP you could put in a TODO and address in a separate CL.

Yes, I agree we should do that, but I would prefer to do that in a separate CL
because this is blocking the beta release, so I want to submit this CL ASAP so
it can be merged into M47. Added a TODO.

[David Trainor- moved to gerrit, 2015-10-14 18:15:38]

lgtm

[pedro (no code reviews), 2015-10-14 20:11:25]

pedrosimonetti@chromium.org changed reviewers:
+ aurimas@chromium.org

[pedro (no code reviews), 2015-10-14 20:12:01]

Hey Aurimas, as we discussed offline, Donn is OOO for the rest of the week, so
could you take a look at this change?

[aurimas (slooooooooow), 2015-10-14 21:56:11]

Overall looks like a great clean up. LGTM

https://codereview.chromium.org/1403813003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java
(right):

https://codereview.chromium.org/1403813003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:268:
public void acknowledgeIntentToShowContent() {
I am not a big fan of this method name. We dont use acknowledge is Java method
names. Can it be called onContentShown() or onShowContent()?

https://codereview.chromium.org/1403813003/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java
(right):

https://codereview.chromium.org/1403813003/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:63:
// TODO(pedrosimonetti): add tests for recent regressions.
Is there a bug tracking that? Please mention it here.

[pedro (no code reviews), 2015-10-14 22:43:37]

Thanks!

https://codereview.chromium.org/1403813003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java
(right):

https://codereview.chromium.org/1403813003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:268:
public void acknowledgeIntentToShowContent() {
On 2015/10/14 21:56:11, aurimas wrote:
> I am not a big fan of this method name. We dont use acknowledge is Java method
> names. Can it be called onContentShown() or onShowContent()?

As we discussed offline, I'm now calling this notifyPanelTouched.

https://codereview.chromium.org/1403813003/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java
(right):

https://codereview.chromium.org/1403813003/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:63:
// TODO(pedrosimonetti): add tests for recent regressions.
On 2015/10/14 21:56:11, aurimas wrote:
> Is there a bug tracking that? Please mention it here.

Done.

[mdjones, 2015-10-14 23:20:01]

lgtm

[pedro (no code reviews), 2015-10-14 23:20:10]

The CQ bit was checked by pedrosimonetti@chromium.org

[pedro (no code reviews), 2015-10-14 23:20:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, aurimas@chromium.org
Link to the patchset: https://codereview.chromium.org/1403813003/#ps100001
(title: "fix")

[commit-bot: I haz the power, 2015-10-14 23:20:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1403813003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1403813003/100001

[commit-bot: I haz the power, 2015-10-15 00:11:08]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-10-15 00:11:43]

Patchset 6 (id:??) landed as
https://crrev.com/9893e6aff9f1811ca5e0e3b3ab4f9fac19afec92
Cr-Commit-Position: refs/heads/master@{#354161}