Refactor Contextual Search base classes

Refactor Contextual Search base classes

This is a simple renaming of some of the classes that are shared
in overlay panels.

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

[mdjones, 2016-01-20 23:48:42]

mdjones@chromium.org changed reviewers:
+ donnd@chromium.org, twellington@chromium.org

[mdjones, 2016-01-20 23:48:42]

More refactoring, PTAL.

[Donn Denman, 2016-01-20 23:59:40]

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

https://codereview.chromium.org/1614513002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java:34:
abstract class OverlayPanelBase implements ContextualSearchPromoHost {
This seems strange: that something as general as the OverlayPanelBase implements
a specific API for Contextual Search.  If you plan on more refactoring of the
PromoHost for CS, maybe just add a TODO here?

[mdjones, 2016-01-21 00:14:10]

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

https://codereview.chromium.org/1614513002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java:34:
abstract class OverlayPanelBase implements ContextualSearchPromoHost {
On 2016/01/20 23:59:39, Donn Denman wrote:
> This seems strange: that something as general as the OverlayPanelBase
implements
> a specific API for Contextual Search.  If you plan on more refactoring of the
> PromoHost for CS, maybe just add a TODO here?

Yeah, there are still quite a few things that need moving; the promos being the
most obvious. However, those moves also require logic changes. I'd prefer
renaming and logic changes be separate CLs. TODO added.

[Donn Denman, 2016-01-21 00:21:22]

LGTM!

[Theresa, 2016-01-21 00:39:02]

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

https://codereview.chromium.org/1614513002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelAnimation.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
Not sure if the copyright year gets updated when files are moved (as opposed to
newly created)... I think it should probably get updated since it's technically
a new file.

https://codereview.chromium.org/1614513002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelAnimation.java:110:
* Animates the Overlay Panel Panel to its maximized state.
Remove the extra "Panel" here and in other comments in this file.

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

https://codereview.chromium.org/1614513002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java:243:
// General methods from requiring an Activity
The previous comment doesn't make sense (since this method below isn't in
Contextual Search Manager). The new comment doesn't really make sense either,
though. Maybe we can just remove this sub-section entirely and leave
getControlContainerHeightResource() in the General API section (with the TODO
stating it should be removed)?

https://codereview.chromium.org/1614513002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java:331:
protected float calculateSearchPanelWidth() {
rename to calculateOverlayPanelWidth?

https://codereview.chromium.org/1614513002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java:475:
public void setSearchBarHeightForTesting(float searchBarHeight) {
Ideally this method & param would get renamed to
setOverlayPanelHeightForTesting. I'm okay with that happening later (since there
are a lot of "searchBar" methods), but would prefer it happen w/ this patchset.

In general, I think it's okay to still distinguish between Bar and Panel (where
the bar is the part of the panel that's visible while it's peeking), but it
would be nice to scrub "search" from this file.

https://codereview.chromium.org/1614513002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java:526:
* @return Whether the Search Panel is fully maximized.
nit: s/Search/Overlay

[mdjones, 2016-01-21 02:30:15]

Most things are renamed now, PTAL.

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

https://codereview.chromium.org/1614513002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelAnimation.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
On 2016/01/21 00:39:02, Theresa Wellington wrote:
> Not sure if the copyright year gets updated when files are moved (as opposed
to
> newly created)... I think it should probably get updated since it's
technically
> a new file.

Done.

https://codereview.chromium.org/1614513002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelAnimation.java:110:
* Animates the Overlay Panel Panel to its maximized state.
On 2016/01/21 00:39:02, Theresa Wellington wrote:
> Remove the extra "Panel" here and in other comments in this file.

Whoops. Done

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

https://codereview.chromium.org/1614513002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java:243:
// General methods from requiring an Activity
On 2016/01/21 00:39:02, Theresa Wellington wrote:
> The previous comment doesn't make sense (since this method below isn't in
> Contextual Search Manager). The new comment doesn't really make sense either,
> though. Maybe we can just remove this sub-section entirely and leave
> getControlContainerHeightResource() in the General API section (with the TODO
> stating it should be removed)?

Done.

https://codereview.chromium.org/1614513002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java:331:
protected float calculateSearchPanelWidth() {
On 2016/01/21 00:39:02, Theresa Wellington wrote:
> rename to calculateOverlayPanelWidth?

Done.

https://codereview.chromium.org/1614513002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java:475:
public void setSearchBarHeightForTesting(float searchBarHeight) {
On 2016/01/21 00:39:02, Theresa Wellington wrote:
> Ideally this method & param would get renamed to
> setOverlayPanelHeightForTesting. I'm okay with that happening later (since
there
> are a lot of "searchBar" methods), but would prefer it happen w/ this
patchset.
> 
> In general, I think it's okay to still distinguish between Bar and Panel
(where
> the bar is the part of the panel that's visible while it's peeking), but it
> would be nice to scrub "search" from this file.

Done.

https://codereview.chromium.org/1614513002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java:526:
* @return Whether the Search Panel is fully maximized.
On 2016/01/21 00:39:02, Theresa Wellington wrote:
> nit: s/Search/Overlay

Done and fixed in other places.

[Theresa, 2016-01-21 02:35:43]

lgtm -thanks!

[mdjones, 2016-01-21 16:58:13]

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

[mdjones, 2016-01-21 16:58:13]

+dtrainor for resources and compositor/*

[gone, 2016-01-21 22:19:00]

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

[gone, 2016-01-21 22:19:01]

Mechanical lgtm

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

https://codereview.chromium.org/1614513002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelAnimation.java:39:
public static final long BASE_ANIMATION_DURATION_MS = 218;
This is absurdly specific.

[David Trainor- moved to gerrit, 2016-01-22 23:43:15]

lgtm

[mdjones, 2016-01-23 00:33:08]

The CQ bit was checked by mdjones@chromium.org

[mdjones, 2016-01-23 00:33:08]

The patchset sent to the CQ was uploaded after l-g-t-m from donnd@chromium.org
Link to the patchset: https://codereview.chromium.org/1614513002/#ps60001
(title: "address comments")

[commit-bot: I haz the power, 2016-01-23 00:36:52]

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

[commit-bot: I haz the power, 2016-01-23 00:44:12]

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

[commit-bot: I haz the power, 2016-01-23 00:44:12]

Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED, no build URL)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, no build URL)

[tandrii(chromium), 2016-01-23 01:26:49]

The CQ bit was checked by tandrii@chromium.org

[commit-bot: I haz the power, 2016-01-23 01:29:56]

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

[tandrii(chromium), 2016-01-23 01:45:11]

On 2016/01/23 01:26:49, tandrii(chromium) wrote:
> The CQ bit was checked by mailto:tandrii@chromium.org

[commit-bot: I haz the power, 2016-01-23 01:48:13]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-23 01:49:43]

Description was changed from

==========
Refactor Contextual Search base classes

This is a simple renaming of some of the classes that are shared
in overlay panels.

BUG=521773
==========

to

==========
Refactor Contextual Search base classes

This is a simple renaming of some of the classes that are shared
in overlay panels.

BUG=521773

Committed: https://crrev.com/86b713a13f4e6d5438e94ee191286d892de0b922
Cr-Commit-Position: refs/heads/master@{#371121}
==========

[commit-bot: I haz the power, 2016-01-23 01:49:44]

Patchset 4 (id:??) landed as
https://crrev.com/86b713a13f4e6d5438e94ee191286d892de0b922
Cr-Commit-Position: refs/heads/master@{#371121}