[Contextual Search] Add a handle to the bar when Chrome Home is enabled

[Contextual Search] Add a handle to the bar when Chrome Home is enabled

Also increases the height of the bar to match the toolbar.

BUG=706256
Review-Url: https://codereview.chromium.org/2801153003
Cr-Commit-Position: refs/heads/master@{#463673}
Committed: https://chromium.googlesource.com/chromium/src/+/3a1c3c6a3eca1dd32503f059458e3d9709c37e7a

[Theresa, 2017-04-06 20:46:54]

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

[Theresa, 2017-04-06 20:46:55]

ptal

[Theresa, 2017-04-06 20:46:57]

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

[commit-bot: I haz the power, 2017-04-06 20:47:49]

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-04-06 23:38:19]

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

[commit-bot: I haz the power, 2017-04-06 23:38:20]

Dry run: This issue passed the CQ dry run.

[mdjones, 2017-04-07 15:47:13]

https://codereview.chromium.org/2801153003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
(right):

https://codereview.chromium.org/2801153003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:296:
if (!FeatureUtilities.isChromeHomeEnabled()) return;
Does contextual search work on CCTs or webapps? If contextual search still has a
handle to the activity, you could check if getBottomSheet() returns null.

https://codereview.chromium.org/2801153003/diff/1/chrome/browser/android/comp...
File chrome/browser/android/compositor/layer/overlay_panel_layer.cc (right):

https://codereview.chromium.org/2801153003/diff/1/chrome/browser/android/comp...
chrome/browser/android/compositor/layer/overlay_panel_layer.cc:130: if
(bar_handle_resource_id_ != 0) {
Do you think it makes sense to lazily create the layer here instead of in the
constructor? Or does that go against the style?

[Theresa, 2017-04-07 16:26:08]

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

[Theresa, 2017-04-07 16:26:43]

https://codereview.chromium.org/2801153003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
(right):

https://codereview.chromium.org/2801153003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:296:
if (!FeatureUtilities.isChromeHomeEnabled()) return;
On 2017/04/07 15:47:13, mdjones wrote:
> Does contextual search work on CCTs or webapps? If contextual search still has
a
> handle to the activity, you could check if getBottomSheet() returns null.

It works in CCTs. Changed the check.

https://codereview.chromium.org/2801153003/diff/1/chrome/browser/android/comp...
File chrome/browser/android/compositor/layer/overlay_panel_layer.cc (right):

https://codereview.chromium.org/2801153003/diff/1/chrome/browser/android/comp...
chrome/browser/android/compositor/layer/overlay_panel_layer.cc:130: if
(bar_handle_resource_id_ != 0) {
On 2017/04/07 15:47:13, mdjones wrote:
> Do you think it makes sense to lazily create the layer here instead of in the
> constructor? Or does that go against the style?

Done.

[commit-bot: I haz the power, 2017-04-07 16:26:45]

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

[mdjones, 2017-04-07 16:50:44]

https://codereview.chromium.org/2801153003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
(right):

https://codereview.chromium.org/2801153003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:298:
(BottomToolbarPhone) mActivity.getToolbarManager().getToolbar());
Do you think it's worth making generateHandleBitmap static and having a separate
bitmap in contextual search to avoid this plumbing?

[commit-bot: I haz the power, 2017-04-07 17:13:08]

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

[commit-bot: I haz the power, 2017-04-07 17:13:09]

Dry run: This issue passed the CQ dry run.

[Theresa, 2017-04-07 17:43:57]

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

[Theresa, 2017-04-07 17:44:18]

https://codereview.chromium.org/2801153003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
(right):

https://codereview.chromium.org/2801153003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:298:
(BottomToolbarPhone) mActivity.getToolbarManager().getToolbar());
On 2017/04/07 16:50:43, mdjones wrote:
> Do you think it's worth making generateHandleBitmap static and having a
separate
> bitmap in contextual search to avoid this plumbing?

Done.

[commit-bot: I haz the power, 2017-04-07 17:44:42]

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

[mdjones, 2017-04-07 17:58:30]

lgtm, thanks!

[Theresa, 2017-04-07 18:00:06]

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

[Theresa, 2017-04-07 18:00:07]

+dtrainor@ for OWNERS review

[commit-bot: I haz the power, 2017-04-07 19:02:09]

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

[commit-bot: I haz the power, 2017-04-07 19:02:10]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[David Trainor- moved to gerrit, 2017-04-10 18:57:27]

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

https://codereview.chromium.org/2801153003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java:1162:
BitmapDynamicResource handleResource = new
BitmapDynamicResource(mBarHandleResourceId);
It would be nice to do this as a static resource if we never invalidate it. 
Otherwise every time we query we're going to Java to just make sure it's not
dirty (which we should probably fix anyway).

Have you tried defining this is xml and just pointing to the drawable id as a
static resource?

[Theresa, 2017-04-10 20:57:58]

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

[commit-bot: I haz the power, 2017-04-10 20:58:44]

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

[Theresa, 2017-04-10 20:59:33]

ptal

Matt, will you please do another pass? I changed the handle to an XML drawable.

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

https://codereview.chromium.org/2801153003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java:1162:
BitmapDynamicResource handleResource = new
BitmapDynamicResource(mBarHandleResourceId);
On 2017/04/10 18:57:27, David Trainor-ping if over 24h wrote:
> It would be nice to do this as a static resource if we never invalidate it. 
> Otherwise every time we query we're going to Java to just make sure it's not
> dirty (which we should probably fix anyway).
> 
> Have you tried defining this is xml and just pointing to the drawable id as a
> static resource?

Great suggestion, thanks!

[David Trainor- moved to gerrit, 2017-04-10 21:06:44]

One last suggestion/nit but lgtm.

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

https://codereview.chromium.org/2801153003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java:1162:
public int getBarHandleDynamicResourceId() {
getBarHandleResourceId (- dynamic)?  Or if it's always dark, do we even need to
pipe it through?  Can the ContextualSearchSceneLayer just do
R.drawable.toolbar_handle_dark?

[mdjones, 2017-04-10 21:06:57]

lgtm, thanks for converting to xml!

[Theresa, 2017-04-10 21:27:22]

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

[commit-bot: I haz the power, 2017-04-10 21:27:40]

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

[Theresa, 2017-04-10 21:28:07]

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

https://codereview.chromium.org/2801153003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelBase.java:1162:
public int getBarHandleDynamicResourceId() {
On 2017/04/10 21:06:44, David Trainor-ping if over 24h wrote:
> getBarHandleResourceId (- dynamic)?  Or if it's always dark, do we even need
to
> pipe it through?  Can the ContextualSearchSceneLayer just do
> R.drawable.toolbar_handle_dark?

Yes, it should just be getBarHandleResourceId().

It's always dark, but I'd like OverlayPanelBase to be responsible for the bar
handle since it needs to adjust the bar height when there is a handle. Bar
handle responsibility includes knowing the resource id and the offset/padding
dimensions.

In the future, other overlay panel UI's should include the bar handle. For now,
contextual search is the only place that needs it since reader mode is changing
its UI. There will be some future cleanup to make the bar handle included by
default.

[Donn Denman, 2017-04-10 21:28:28]

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

[Donn Denman, 2017-04-10 21:28:29]

Theresa, QQ: did you test with huge fonts?  I seem to remember some edge cases
with huge fonts when we added the Caption, so that might be worth testing (or
asking QA to).

Thanks for doing this!

[Theresa, 2017-04-10 21:37:16]

On 2017/04/10 21:28:29, Donn Denman wrote:
> Theresa, QQ: did you test with huge fonts?  I seem to remember some edge cases
> with huge fonts when we added the Caption, so that might be worth testing (or
> asking QA to).
> 
> Thanks for doing this!

Yes, I tested with large fonts. Everything still works as expected.

[commit-bot: I haz the power, 2017-04-10 22:09:11]

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

[commit-bot: I haz the power, 2017-04-10 22:09:12]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Theresa, 2017-04-10 22:48:10]

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

[commit-bot: I haz the power, 2017-04-10 22:48:40]

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

[Theresa, 2017-04-10 22:49:39]

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

[commit-bot: I haz the power, 2017-04-10 22:50:10]

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-04-10 23:33:24]

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

[commit-bot: I haz the power, 2017-04-10 23:33:25]

Dry run: This issue passed the CQ dry run.

[Theresa, 2017-04-11 17:11:59]

The CQ bit was checked by twellington@chromium.org

[Theresa, 2017-04-11 17:11:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
mdjones@chromium.org, dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2801153003/#ps160001
(title: "Revert changes to OverlayPanelBase#initializeUiState()")

[commit-bot: I haz the power, 2017-04-11 17:12:38]

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-04-11 17:39:59]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1491930719216230,
"parent_rev": "23910e967cb775dbc38dee6b0f06c3c69d93be1e", "commit_rev":
"3a1c3c6a3eca1dd32503f059458e3d9709c37e7a"}

[commit-bot: I haz the power, 2017-04-11 17:40:46]

Description was changed from

==========
[Contextual Search] Add a handle to the bar when Chrome Home is enabled

Also increases the height of the bar to match the toolbar.

BUG=706256
==========

to

==========
[Contextual Search] Add a handle to the bar when Chrome Home is enabled

Also increases the height of the bar to match the toolbar.

BUG=706256

Review-Url: https://codereview.chromium.org/2801153003
Cr-Commit-Position: refs/heads/master@{#463673}
Committed:
https://chromium.googlesource.com/chromium/src/+/3a1c3c6a3eca1dd32503f059458e...
==========

[commit-bot: I haz the power, 2017-04-11 17:40:50]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/3a1c3c6a3eca1dd32503f059458e...