Photo Picker dialog: Make toolbar behave more like mocks.

Photo Picker dialog: Make toolbar behave more like mocks.

- Background should not change in selection mode.
- Close icon instead of arrow (visible in selection and normal mode).

BUG=656015
Review-Url: https://codereview.chromium.org/2874033004
Cr-Commit-Position: refs/heads/master@{#472132}
Committed: https://chromium.googlesource.com/chromium/src/+/c73bc5c480b880aac3a5a228e2b2102602be9619

[Finnur, 2017-05-11 14:25:28]

Description was changed from

==========
Photo Picker dialog: Make toolbar behave more like mocks.

- Mocks don't use the NumberView
- Background should be dark in both cases.

BUG=656015
==========

to

==========
Photo Picker dialog: Make toolbar behave more like mocks.

- Mocks don't use the NumberView
- Background should be dark in both cases.
- Title color should be white.

BUG=656015
==========

[Finnur, 2017-05-11 14:25:28]

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

[Finnur, 2017-05-11 14:49:02]

Description was changed from

==========
Photo Picker dialog: Make toolbar behave more like mocks.

- Mocks don't use the NumberView
- Background should be dark in both cases.
- Title color should be white.

BUG=656015
==========

to

==========
Photo Picker dialog: Make toolbar behave more like mocks.

- Mocks don't use the NumberView
- Background should be dark in both cases.
- Title color should be white.
- No back arrow, just the title and the Done button visible.

BUG=656015
==========

[Finnur, 2017-05-11 14:49:22]

Description was changed from

==========
Photo Picker dialog: Make toolbar behave more like mocks.

- Mocks don't use the NumberView
- Background should be dark in both cases.
- Title color should be white.
- No back arrow, just the title and the Done button visible.

BUG=656015
==========

to

==========
Photo Picker dialog: Make toolbar behave more like mocks.

- Mocks don't use the NumberView
- Background should be dark in both cases.
- Title color should be white.
- No back arrow, only title and the Done button visible.

BUG=656015
==========

[Finnur, 2017-05-11 14:51:24]

https://codereview.chromium.org/2874033004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java
(right):

https://codereview.chromium.org/2874033004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PickerCategoryView.java:112:
R.color.photo_picker_special_tile_bg_color, android.R.color.white, false, this);
The mocks show the Toolbar in the Photo Picker as dark gray in both modes
(normal and selection), so the font color for the title needs to be white. This
is also why I'm investigating changing the font color for the *menu* to white,
but have been unsuccessful so far. Would very much appreciate some tips...

[Theresa, 2017-05-12 15:38:07]

Instead of passing in extra configuration params to SelectableListToolbar, I
think it makes more sense to override some of the methods in
PhotoPickerToolbar.java.

PhotoPickerToolbar needs a custom navigation button (always an 'x'), so it
should override setNavigationButton() and onClick(View view).

Its normal/selected views also don't change nearly as much as the regular
SelectableListToolbar. I think PhotoPickerToolbar should override
showNormalView() and showSelectionView().

[Finnur, 2017-05-15 15:17:30]

Description was changed from

==========
Photo Picker dialog: Make toolbar behave more like mocks.

- Mocks don't use the NumberView
- Background should be dark in both cases.
- Title color should be white.
- No back arrow, only title and the Done button visible.

BUG=656015
==========

to

==========
Photo Picker dialog: Make toolbar behave more like mocks.

- Background should not change in selection mode.
- Close icon instead of arrow (visible in selection and normal mode).

BUG=656015
==========

[Finnur, 2017-05-15 15:21:04]

Patchset #3 (id:40001) has been deleted

[Finnur, 2017-05-15 15:21:51]

PTAL.

[Theresa, 2017-05-15 16:17:05]

https://codereview.chromium.org/2874033004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerToolbar.java
(right):

https://codereview.chromium.org/2874033004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerToolbar.java:34:
setNavigationOnClickListener(listener);
Since setNavigationOnClickListener is a public method, I think it's a bit
simpler to call that directly from PickerCategoryView.

https://codereview.chromium.org/2874033004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerToolbar.java:41:
updateDisplayStyleIfNecessary();
Instead of setting these each time, can they be set once in onFinishInflate()?
Then this method would intentionally do nothing.

https://codereview.chromium.org/2874033004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerToolbar.java:51:
down.setTextColor(Color.BLACK);
Can the text color be set once in onFinishInflate()? I don't believe that
SelectableListToolbar ever programatically changes the number roll text color.

[Finnur, 2017-05-16 11:40:06]

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

[Finnur, 2017-05-16 11:40:20]

All done. PTAL.

https://codereview.chromium.org/2874033004/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerToolbar.java
(right):

https://codereview.chromium.org/2874033004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerToolbar.java:34:
setNavigationOnClickListener(listener);
Ah, right. Done.

https://codereview.chromium.org/2874033004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerToolbar.java:41:
updateDisplayStyleIfNecessary();
On 2017/05/15 16:17:04, Theresa wrote:
> Instead of setting these each time, can they be set once in onFinishInflate()?
> Then this method would intentionally do nothing.

Done.

https://codereview.chromium.org/2874033004/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerToolbar.java:51:
down.setTextColor(Color.BLACK);
Sure! Done.

[commit-bot: I haz the power, 2017-05-16 11:40:32]

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-05-16 12:20:58]

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

[commit-bot: I haz the power, 2017-05-16 12:20:59]

Dry run: This issue passed the CQ dry run.

[Theresa, 2017-05-16 15:01:23]

lgtm % a couple of comments

https://codereview.chromium.org/2874033004/diff/80001/chrome/android/java/res...
File chrome/android/java/res/menu/photo_picker_menu.xml (left):

https://codereview.chromium.org/2874033004/diff/80001/chrome/android/java/res...
chrome/android/java/res/menu/photo_picker_menu.xml:1: <?xml version="1.0"
encoding="utf-8"?>
This is fully deleted right? Just checking, since the codereview tool is marking
it as "M" rather than "D".

https://codereview.chromium.org/2874033004/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListToolbar.java
(right):

https://codereview.chromium.org/2874033004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListToolbar.java:599:
protected void updateDisplayStyleIfNecessary() {
nit: I think this can be private. If the photo picker uses a width-constrained
display style in the future, we can revisit.

[Finnur, 2017-05-16 16:08:02]

https://codereview.chromium.org/2874033004/diff/80001/chrome/android/java/res...
File chrome/android/java/res/menu/photo_picker_menu.xml (left):

https://codereview.chromium.org/2874033004/diff/80001/chrome/android/java/res...
chrome/android/java/res/menu/photo_picker_menu.xml:1: <?xml version="1.0"
encoding="utf-8"?>
Yeah, it looks that way on my machine (the file is gone). Weird.

https://codereview.chromium.org/2874033004/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListToolbar.java
(right):

https://codereview.chromium.org/2874033004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/widget/selection/SelectableListToolbar.java:599:
protected void updateDisplayStyleIfNecessary() {
On 2017/05/16 15:01:22, Theresa wrote:
> nit: I think this can be private. If the photo picker uses a width-constrained
> display style in the future, we can revisit.

Done.

[Finnur, 2017-05-16 16:08:28]

The CQ bit was checked by finnur@chromium.org

[Finnur, 2017-05-16 16:08:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org
Link to the patchset: https://codereview.chromium.org/2874033004/#ps100001
(title: "Address feedback from Theresa")

[commit-bot: I haz the power, 2017-05-16 16:09:03]

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-05-16 16:59:20]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494950908208710,
"parent_rev": "7b9e08e23f6e26b385619215fc80ac93d374a604", "commit_rev":
"c73bc5c480b880aac3a5a228e2b2102602be9619"}

[commit-bot: I haz the power, 2017-05-16 16:59:27]

Description was changed from

==========
Photo Picker dialog: Make toolbar behave more like mocks.

- Background should not change in selection mode.
- Close icon instead of arrow (visible in selection and normal mode).

BUG=656015
==========

to

==========
Photo Picker dialog: Make toolbar behave more like mocks.

- Background should not change in selection mode.
- Close icon instead of arrow (visible in selection and normal mode).

BUG=656015

Review-Url: https://codereview.chromium.org/2874033004
Cr-Commit-Position: refs/heads/master@{#472132}
Committed:
https://chromium.googlesource.com/chromium/src/+/c73bc5c480b880aac3a5a228e2b2...
==========

[commit-bot: I haz the power, 2017-05-16 16:59:28]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/c73bc5c480b880aac3a5a228e2b2...