Separate contextual search state handler and log

Separate contextual search state handler and log

This change removes the ContextualSearchStateHandler and moves the
functionality of this class into more appropriate locations.

The logging functionality has been moved to a separate class that
is owned by the ContextualSearchPanel since logging should be done
closer to the top level rather than the most base class. State
handling has been moved into the ContextualSearchPanelBase since it
will eventually be a common base feature of overlay panels.

BUG=521773
Committed: https://crrev.com/dd248187f7ff9128c2cf7fdb5b1d0f3a0fc18a99
Cr-Commit-Position: refs/heads/master@{#351376}

[mdjones, 2015-09-22 16:24:21]

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

[mdjones, 2015-09-22 16:24:22]

I decided against the "observer" pattern for the state handler as I believe it
adds needless complexity to the project. Instead I have moved any state handling
functionality into the panel base and logging to the top-level class
(ContextualSearchPanel). Once there is a panel that both contextual search and
reader mode will be based on, the logging will be joined with the contextual
search specific code.

[pedro (no code reviews), 2015-09-25 19:06:32]

https://codereview.chromium.org/1342383004/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/1342383004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:41:
UNKNOWN,
Nit: Put a TODO here to separate generic state changes (reset, close, back
press, etc.) from Contextual Search specific state changes (tap, long press,
etc.).

I would prefer not having any Contextual Search logic in this class, but I can't
find of a better solution that doesn't overcomplicate things.

https://codereview.chromium.org/1342383004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:61:
// The animation duration of a URL being promoted to a tab when triggered by an
Nit: I didn't notice this before. Would you mind putting this comment in the
JavaDoc format?

https://codereview.chromium.org/1342383004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:194:
// Logging of panel state information.
As we discussed offline, this code is self-contained enough (and deals with the
complicated and important logging logic) that I think it deserves its own class.
We could call it ContextualSearchPanelMetrics or something similar.

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

https://codereview.chromium.org/1342383004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:217:
// Valid previous states for the Panel.
Nit: JavaDoc

[mdjones, 2015-09-25 21:34:34]

https://codereview.chromium.org/1342383004/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/1342383004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:41:
UNKNOWN,
On 2015/09/25 19:06:31, pedrosimonetti wrote:
> Nit: Put a TODO here to separate generic state changes (reset, close, back
> press, etc.) from Contextual Search specific state changes (tap, long press,
> etc.).
> 
> I would prefer not having any Contextual Search logic in this class, but I
can't
> find of a better solution that doesn't overcomplicate things.

Done.

https://codereview.chromium.org/1342383004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:61:
// The animation duration of a URL being promoted to a tab when triggered by an
On 2015/09/25 19:06:31, pedrosimonetti wrote:
> Nit: I didn't notice this before. Would you mind putting this comment in the
> JavaDoc format?

Done.

https://codereview.chromium.org/1342383004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:194:
// Logging of panel state information.
On 2015/09/25 19:06:31, pedrosimonetti wrote:
> As we discussed offline, this code is self-contained enough (and deals with
the
> complicated and important logging logic) that I think it deserves its own
class.
> We could call it ContextualSearchPanelMetrics or something similar.

Done.

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

https://codereview.chromium.org/1342383004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:217:
// Valid previous states for the Panel.
On 2015/09/25 19:06:31, pedrosimonetti wrote:
> Nit: JavaDoc

Done.

[pedro (no code reviews), 2015-09-28 23:28:26]

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

https://codereview.chromium.org/1342383004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:191:
mPanelMetrics.onPanelStateChanged(getPanelState(), toState, reason);
Nit: We should call this after calling super.setPanelState() (which changes the
state), otherwise the name onPanelStateChanged would be misleading (it would be
called before changing the state).

https://codereview.chromium.org/1342383004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:199:
public void setDidSearchInvolvePromo() {
Nit: We have a section below where we implement/expose all Panel delegate
methods. Could you please move these methods to that section, so that we have
all Panel delegate methods in the same place?

https://codereview.chromium.org/1342383004/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java
(right):

https://codereview.chromium.org/1342383004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java:43:
boolean isChained = isStartingSearch && isOngoingContextualSearch(toState);
Shouldn't we pass the fromState instead?

https://codereview.chromium.org/1342383004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java:173:
* Gets whether the promo is active.
Nit: Move this a few lines above, so that it appears right after the
setIsPromoActive() implementation.

https://codereview.chromium.org/1342383004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java:211:
return isOngoingContextualSearch(toState)
Shouldn't we pass the fromState instead?

[mdjones, 2015-09-29 01:24:53]

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

https://codereview.chromium.org/1342383004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:191:
mPanelMetrics.onPanelStateChanged(getPanelState(), toState, reason);
On 2015/09/28 23:28:26, pedrosimonetti wrote:
> Nit: We should call this after calling super.setPanelState() (which changes
the
> state), otherwise the name onPanelStateChanged would be misleading (it would
be
> called before changing the state).

I'm now storing the state and running onPanelStateChanged after super.

https://codereview.chromium.org/1342383004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:199:
public void setDidSearchInvolvePromo() {
On 2015/09/28 23:28:26, pedrosimonetti wrote:
> Nit: We have a section below where we implement/expose all Panel delegate
> methods. Could you please move these methods to that section, so that we have
> all Panel delegate methods in the same place?

Done.

https://codereview.chromium.org/1342383004/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java
(right):

https://codereview.chromium.org/1342383004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java:43:
boolean isChained = isStartingSearch && isOngoingContextualSearch(toState);
On 2015/09/28 23:28:26, pedrosimonetti wrote:
> Shouldn't we pass the fromState instead?

Done.

https://codereview.chromium.org/1342383004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java:173:
* Gets whether the promo is active.
On 2015/09/28 23:28:26, pedrosimonetti wrote:
> Nit: Move this a few lines above, so that it appears right after the
> setIsPromoActive() implementation.

Done.

https://codereview.chromium.org/1342383004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java:211:
return isOngoingContextualSearch(toState)
On 2015/09/28 23:28:26, pedrosimonetti wrote:
> Shouldn't we pass the fromState instead?

Done.

[pedro (no code reviews), 2015-09-29 01:31:40]

lgtm

Just a couple more nits, otherwise it looks good to me.

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

https://codereview.chromium.org/1342383004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:208:
public void setWasSearchContentViewSeen() {
Nit: setWasSearchContentViewSeen(), setIsPromoActive() and
onSearchResultsLoaded() are also method of the Panel delegate, and therefore
would be nice moving them as well to the delegate section below.

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

https://codereview.chromium.org/1342383004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java:221:
private boolean isOngoingContextualSearch(PanelState state) {
Nit: Let's rename the parameter to fromState to make it clearer that it should
get the this should be the state we're changing from.

[mdjones, 2015-09-29 15:26:05]

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

https://codereview.chromium.org/1342383004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanel.java:208:
public void setWasSearchContentViewSeen() {
On 2015/09/29 01:31:39, pedrosimonetti wrote:
> Nit: setWasSearchContentViewSeen(), setIsPromoActive() and
> onSearchResultsLoaded() are also method of the Panel delegate, and therefore
> would be nice moving them as well to the delegate section below.

Done.

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

https://codereview.chromium.org/1342383004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelMetrics.java:221:
private boolean isOngoingContextualSearch(PanelState state) {
On 2015/09/29 01:31:39, pedrosimonetti wrote:
> Nit: Let's rename the parameter to fromState to make it clearer that it should
> get the this should be the state we're changing from.

Done.

[mdjones, 2015-09-29 15:45:15]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2015-09-29 15:45:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
pedrosimonetti@chromium.org
Link to the patchset: https://codereview.chromium.org/1342383004/#ps100001
(title: "correct var name")

[commit-bot: I haz the power, 2015-09-29 15:46:29]

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

[commit-bot: I haz the power, 2015-09-29 18:32:07]

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

[commit-bot: I haz the power, 2015-09-29 18:32:10]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[mdjones, 2015-09-29 18:58:09]

The CQ bit was checked by mdjones@chromium.org

[commit-bot: I haz the power, 2015-09-29 18:59:54]

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

[commit-bot: I haz the power, 2015-09-29 19:50:25]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-09-29 19:51:03]

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