[Contextual Search] Keep Panel upon orientation change.

[Contextual Search] Keep Panel upon orientation change.

This CL preserves the Panel state when the orientation changes,
on fullscreen-size Panels (mostly on phones). Supporting this
use case on a narrow Panel would require some changes in
the Compositor system, and will be addressed later.

BUG=568351
Committed: https://crrev.com/177e9274094c5171d5e18b5e3dc3158feb3a384b
Cr-Commit-Position: refs/heads/master@{#366164}

[pedro (no code reviews), 2015-12-17 19:31:42]

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

[pedro (no code reviews), 2015-12-17 19:31:42]

Hey guys, please take a look at this change.

[pedro (no code reviews), 2015-12-17 19:36:37]

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

[pedro (no code reviews), 2015-12-17 19:36:38]

changwan@chromium.org: Please review changes in:

chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/*

chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/*

chrome/browser/android/compositor/layer/contextual_search_layer.cc

[Donn Denman, 2015-12-17 20:54:25]

LGTM except for some nits, though I'm not an expert in this code.

https://codereview.chromium.org/1536623003/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/1536623003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:662:
int value = Math.round(255 * mBasePageBrightness);
It might be wise to Math.min(255, value), since the docs say "... if they are
out of range, the returned color is undefined".

https://codereview.chromium.org/1536623003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java
(right):

https://codereview.chromium.org/1536623003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java:94:
if (content != null) return content.getContainerView();
Nit: insert a blank line after the return?

https://codereview.chromium.org/1536623003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/TabListSceneLayer.java
(right):

https://codereview.chromium.org/1536623003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/TabListSceneLayer.java:138:
return ApiCompatibilityUtils.getColor(context.getResources(),
Could context be null here?

[pedro (no code reviews), 2015-12-17 21:23:22]

PTAL

https://codereview.chromium.org/1536623003/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/1536623003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:662:
int value = Math.round(255 * mBasePageBrightness);
On 2015/12/17 20:54:25, Donn Denman wrote:
> It might be wise to Math.min(255, value), since the docs say "... if they are
> out of range, the returned color is undefined".

Done.

https://codereview.chromium.org/1536623003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java
(right):

https://codereview.chromium.org/1536623003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java:94:
if (content != null) return content.getContainerView();
On 2015/12/17 20:54:25, Donn Denman wrote:
> Nit: insert a blank line after the return?

Done.

https://codereview.chromium.org/1536623003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/TabListSceneLayer.java
(right):

https://codereview.chromium.org/1536623003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/TabListSceneLayer.java:138:
return ApiCompatibilityUtils.getColor(context.getResources(),
On 2015/12/17 20:54:25, Donn Denman wrote:
> Could context be null here?

I don't think so. The existing code assumes Context will always be there, and
I'm assuming the existing code works.

[Donn Denman, 2015-12-17 21:34:18]

lgtm

https://codereview.chromium.org/1536623003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java
(right):

https://codereview.chromium.org/1536623003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java:92:

You can drop this new blank line (up to you).

[mdjones, 2015-12-17 22:01:12]

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

https://codereview.chromium.org/1536623003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:318:
protected boolean mayDisplayNarrowSizePanel() {
Is it ever the case that these conditions would not produce a narrow panel?

https://codereview.chromium.org/1536623003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java
(right):

https://codereview.chromium.org/1536623003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java:74:
*
nit: extra line

[pedro (no code reviews), 2015-12-17 23:24:47]

PTAL

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

https://codereview.chromium.org/1536623003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:318:
protected boolean mayDisplayNarrowSizePanel() {
On 2015/12/17 22:01:12, mdjones wrote:
> Is it ever the case that these conditions would not produce a narrow panel?

As we discussed offline, I've renamed this class to make its purpose easier to
understand.

https://codereview.chromium.org/1536623003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java
(right):

https://codereview.chromium.org/1536623003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java:74:
*
On 2015/12/17 22:01:12, mdjones wrote:
> nit: extra line

JavaDoc description was missing. Added that.

https://codereview.chromium.org/1536623003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java:92:

On 2015/12/17 21:34:18, Donn Denman wrote:
> You can drop this new blank line (up to you).

Done.

[mdjones, 2015-12-17 23:54:26]

lgtm

[pedro (no code reviews), 2015-12-18 01:05:57]

Changwan: could you please take a look at this change?

[Changwan Ryu, 2015-12-18 01:25:38]

https://codereview.chromium.org/1536623003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java
(right):

https://codereview.chromium.org/1536623003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java:65:
return Color.WHITE;
Why should this be WHITE instead of
super.getTabswitcherBackgroundColor(context)?

https://codereview.chromium.org/1536623003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/TabListSceneLayer.java
(right):

https://codereview.chromium.org/1536623003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/TabListSceneLayer.java:83:
getBackgroundColor(context),
The name is somewhat confusing. We already have t.getBackgroundColor(). How
about getTabswitcherBackgroundColor(Context)?

[Changwan Ryu, 2015-12-18 01:34:30]

https://codereview.chromium.org/1536623003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/TabListSceneLayer.java
(right):

https://codereview.chromium.org/1536623003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/TabListSceneLayer.java:83:
getBackgroundColor(context),
On 2015/12/18 01:25:38, Changwan Ryu wrote:
> The name is somewhat confusing. We already have t.getBackgroundColor(). How
> about getTabswitcherBackgroundColor(Context)?

Looking again, t.getBackgroundColor() is tab's background color while this one
is the real background color. Still somewhat confusing, though.

BTW, is 'git cl format' ok with your reformatting here? I'm afraid that it might
automatically redo this in the future, which can be extra cumbersome.

[pedro (no code reviews), 2015-12-18 01:43:09]

Changwan: please take another look.

https://codereview.chromium.org/1536623003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java
(right):

https://codereview.chromium.org/1536623003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java:65:
return Color.WHITE;
On 2015/12/18 01:25:38, Changwan Ryu wrote:
> Why should this be WHITE instead of
> super.getTabswitcherBackgroundColor(context)?

In Contextual Search, we reuse the TabListSceneLayer in order to render a single
tab (we call it base tab, the tab that is shown below the Contextual Search
Panel) when our Contextual Search Panel is opened. We are not using the
StaticTabSceneLayer because it is supposed to be optimized for the StaticLayout.

In our case, we need to fill that tab with a color when the device is being
rotated, because the tab is not immediately resized, so we fill the gap with the
color that best represents the contents of the tab. The color used in the
TabSwitcher is a very dark color (almost black), but we need to use a different
color here.

If the panel is null (which in theory should never happen in this case), we will
assume that color to be white (assuming most pages will have a white
background). This is just a first implementation. In the future we will try to
get the background color from the WebContents, similar to what is done in the
StaticTabSceneLayer.

https://codereview.chromium.org/1536623003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/TabListSceneLayer.java
(right):

https://codereview.chromium.org/1536623003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/TabListSceneLayer.java:83:
getBackgroundColor(context),
On 2015/12/18 01:25:38, Changwan Ryu wrote:
> The name is somewhat confusing. We already have t.getBackgroundColor(). How
> about getTabswitcherBackgroundColor(Context)?

Good point. This is indeed confusing since we have a tab background color
(t.getBackgroundColor()) and the tab switcher background color.

For consistency with the name of this class I'm calling it now
getTabListBackgroundColor(context).

https://codereview.chromium.org/1536623003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/TabListSceneLayer.java:83:
getBackgroundColor(context),
On 2015/12/18 01:34:29, Changwan Ryu wrote:
> On 2015/12/18 01:25:38, Changwan Ryu wrote:
> > The name is somewhat confusing. We already have t.getBackgroundColor(). How
> > about getTabswitcherBackgroundColor(Context)?
> 
> Looking again, t.getBackgroundColor() is tab's background color while this one
> is the real background color. Still somewhat confusing, though.
> 
> BTW, is 'git cl format' ok with your reformatting here? I'm afraid that it
might
> automatically redo this in the future, which can be extra cumbersome.

I don't usually use 'git cl format'. I could try and see what it does.

I did this change because I thought it would make it easier to make future
changes in this file, since adding/removing a param will be just a matter of
adding/removing a new line, instead of having to reformat the whole thing. Do
you have any preference?

[Changwan Ryu, 2015-12-18 02:03:58]

lgtm with nits

https://codereview.chromium.org/1536623003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java
(right):

https://codereview.chromium.org/1536623003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java:65:
return Color.WHITE;
On 2015/12/18 01:43:09, pedrosimonetti wrote:
> On 2015/12/18 01:25:38, Changwan Ryu wrote:
> > Why should this be WHITE instead of
> > super.getTabswitcherBackgroundColor(context)?
> 
> In Contextual Search, we reuse the TabListSceneLayer in order to render a
single
> tab (we call it base tab, the tab that is shown below the Contextual Search
> Panel) when our Contextual Search Panel is opened. We are not using the
> StaticTabSceneLayer because it is supposed to be optimized for the
StaticLayout.
> 
> In our case, we need to fill that tab with a color when the device is being
> rotated, because the tab is not immediately resized, so we fill the gap with
the
> color that best represents the contents of the tab. The color used in the
> TabSwitcher is a very dark color (almost black), but we need to use a
different
> color here.
> 
> If the panel is null (which in theory should never happen in this case), we
will
> assume that color to be white (assuming most pages will have a white
> background). This is just a first implementation. In the future we will try to
> get the background color from the WebContents, similar to what is done in the
> StaticTabSceneLayer.

Thanks for the explanation. Could you add some comment such as:

If the panel is null (which in theory should never happen in this case), we will
assume that color to be white (assuming most pages will have a white
background).

https://codereview.chromium.org/1536623003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/TabListSceneLayer.java
(right):

https://codereview.chromium.org/1536623003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/TabListSceneLayer.java:83:
getBackgroundColor(context),
On 2015/12/18 01:43:09, pedrosimonetti wrote:
> On 2015/12/18 01:34:29, Changwan Ryu wrote:
> > On 2015/12/18 01:25:38, Changwan Ryu wrote:
> > > The name is somewhat confusing. We already have t.getBackgroundColor().
How
> > > about getTabswitcherBackgroundColor(Context)?
> > 
> > Looking again, t.getBackgroundColor() is tab's background color while this
one
> > is the real background color. Still somewhat confusing, though.
> > 
> > BTW, is 'git cl format' ok with your reformatting here? I'm afraid that it
> might
> > automatically redo this in the future, which can be extra cumbersome.
> 
> I don't usually use 'git cl format'. I could try and see what it does.
> 
> I did this change because I thought it would make it easier to make future
> changes in this file, since adding/removing a param will be just a matter of
> adding/removing a new line, instead of having to reformat the whole thing. Do
> you have any preference?

I prefer sticking to automatic formatting as much as possible because that way
you don't have to go back to the editor on presubmit warnings. And if you don't
like specifics like this, I believe we need to report bugs or discuss about how
we automatically format code so that it can be gradually improved. But this is
just another nit.

[pedro (no code reviews), 2015-12-18 19:14:57]

https://codereview.chromium.org/1536623003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java
(right):

https://codereview.chromium.org/1536623003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java:65:
return Color.WHITE;
On 2015/12/18 02:03:58, Changwan Ryu wrote:
> On 2015/12/18 01:43:09, pedrosimonetti wrote:
> > On 2015/12/18 01:25:38, Changwan Ryu wrote:
> > > Why should this be WHITE instead of
> > > super.getTabswitcherBackgroundColor(context)?
> > 
> > In Contextual Search, we reuse the TabListSceneLayer in order to render a
> single
> > tab (we call it base tab, the tab that is shown below the Contextual Search
> > Panel) when our Contextual Search Panel is opened. We are not using the
> > StaticTabSceneLayer because it is supposed to be optimized for the
> StaticLayout.
> > 
> > In our case, we need to fill that tab with a color when the device is being
> > rotated, because the tab is not immediately resized, so we fill the gap with
> the
> > color that best represents the contents of the tab. The color used in the
> > TabSwitcher is a very dark color (almost black), but we need to use a
> different
> > color here.
> > 
> > If the panel is null (which in theory should never happen in this case), we
> will
> > assume that color to be white (assuming most pages will have a white
> > background). This is just a first implementation. In the future we will try
to
> > get the background color from the WebContents, similar to what is done in
the
> > StaticTabSceneLayer.
> 
> Thanks for the explanation. Could you add some comment such as:
> 
> If the panel is null (which in theory should never happen in this case), we
will
> assume that color to be white (assuming most pages will have a white
> background).

Done.

https://codereview.chromium.org/1536623003/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/TabListSceneLayer.java
(right):

https://codereview.chromium.org/1536623003/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/scene_layer/TabListSceneLayer.java:83:
getBackgroundColor(context),
On 2015/12/18 02:03:58, Changwan Ryu wrote:
> On 2015/12/18 01:43:09, pedrosimonetti wrote:
> > On 2015/12/18 01:34:29, Changwan Ryu wrote:
> > > On 2015/12/18 01:25:38, Changwan Ryu wrote:
> > > > The name is somewhat confusing. We already have t.getBackgroundColor().
> How
> > > > about getTabswitcherBackgroundColor(Context)?
> > > 
> > > Looking again, t.getBackgroundColor() is tab's background color while this
> one
> > > is the real background color. Still somewhat confusing, though.
> > > 
> > > BTW, is 'git cl format' ok with your reformatting here? I'm afraid that it
> > might
> > > automatically redo this in the future, which can be extra cumbersome.
> > 
> > I don't usually use 'git cl format'. I could try and see what it does.
> > 
> > I did this change because I thought it would make it easier to make future
> > changes in this file, since adding/removing a param will be just a matter of
> > adding/removing a new line, instead of having to reformat the whole thing.
Do
> > you have any preference?
> 
> I prefer sticking to automatic formatting as much as possible because that way
> you don't have to go back to the editor on presubmit warnings. And if you
don't
> like specifics like this, I believe we need to report bugs or discuss about
how
> we automatically format code so that it can be gradually improved. But this is
> just another nit.

Okay. I re-formatted the code with 'git cl format'.

[pedro (no code reviews), 2015-12-18 19:15:12]

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

[commit-bot: I haz the power, 2015-12-18 19:16:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1536623003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1536623003/80001

[commit-bot: I haz the power, 2015-12-18 20:04:47]

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

[commit-bot: I haz the power, 2015-12-18 20:04:47]

Dry run: This issue passed the CQ dry run.

[pedro (no code reviews), 2015-12-18 20:56:40]

The CQ bit was checked by pedrosimonetti@chromium.org

[pedro (no code reviews), 2015-12-18 20:56:40]

The patchset sent to the CQ was uploaded after l-g-t-m from donnd@chromium.org,
mdjones@chromium.org, changwan@chromium.org
Link to the patchset: https://codereview.chromium.org/1536623003/#ps80001
(title: "Addressing Changwan's comments")

[commit-bot: I haz the power, 2015-12-18 20:57:14]

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

[commit-bot: I haz the power, 2015-12-18 21:05:07]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-18 21:06:07]

Description was changed from

==========
[Contextual Search] Keep Panel upon orientation change.

This CL preserves the Panel state when the orientation changes,
on fullscreen-size Panels (mostly on phones). Supporting this
use case on a narrow Panel would require some changes in
the Compositor system, and will be addressed later.

BUG=568351
==========

to

==========
[Contextual Search] Keep Panel upon orientation change.

This CL preserves the Panel state when the orientation changes,
on fullscreen-size Panels (mostly on phones). Supporting this
use case on a narrow Panel would require some changes in
the Compositor system, and will be addressed later.

BUG=568351

Committed: https://crrev.com/177e9274094c5171d5e18b5e3dc3158feb3a384b
Cr-Commit-Position: refs/heads/master@{#366164}
==========

[commit-bot: I haz the power, 2015-12-18 21:06:08]

Patchset 5 (id:??) landed as
https://crrev.com/177e9274094c5171d5e18b5e3dc3158feb3a384b
Cr-Commit-Position: refs/heads/master@{#366164}