Notification custom layouts: move settings button to the footer.

Notification custom layouts: move settings button to the footer.

* If no actions are specified, the buttons section no longer needs to be shown.
* If there are actions, their buttons will now have more space and can show longer text labels of a more reasonable size.
* Screenshots of the new look and feel: http://imgur.com/a/zIGi1

BUG=528410
Committed: https://crrev.com/640183fab81bde87b5a2ac82246de7a7e1e521a6
Cr-Commit-Position: refs/heads/master@{#368844}

[Michael van Ouwerkerk, 2016-01-08 14:59:19]

Description was changed from

==========
Notification custom layouts: move settings button to the footer.

If no actions are specified, the buttons section no longer needs to be shown.

BUG=528410
==========

to

==========
Notification custom layouts: move settings button to the footer.

* If no actions are specified, the buttons section no longer needs to be shown.
* If there are actions, their buttons will now have more space and can show
longer text labels of a more reasonable size.

BUG=528410
==========

[Michael van Ouwerkerk, 2016-01-08 15:21:59]

Description was changed from

==========
Notification custom layouts: move settings button to the footer.

* If no actions are specified, the buttons section no longer needs to be shown.
* If there are actions, their buttons will now have more space and can show
longer text labels of a more reasonable size.

BUG=528410
==========

to

==========
Notification custom layouts: move settings button to the footer.

* If no actions are specified, the buttons section no longer needs to be shown.
* If there are actions, their buttons will now have more space and can show
longer text labels of a more reasonable size.
* Screenshots of the new look and feel: https://goo.gl/4BW6cG

BUG=528410
==========

[Michael van Ouwerkerk, 2016-01-08 15:23:04]

mvanouwerkerk@chromium.org changed reviewers:
+ newt@chromium.org

[Michael van Ouwerkerk, 2016-01-08 15:23:05]

Newton, could you take a look please?

[Peter Beverloo, 2016-01-08 21:29:30]

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

[Peter Beverloo, 2016-01-08 21:29:31]

Whoohoo, exciting! Thanks for the patch :)

Is there a public version of the screenshots we can refer to in the CL
description (that's preferred since this is an open source project)?

https://codereview.chromium.org/1569783005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java
(right):

https://codereview.chromium.org/1569783005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:243:
throw new IllegalStateException("There must be an action for the settings
button.");
Could we instead have a new method for setting the Site Settings intent, and
having the *NotificationBuilder decide on the display of that? That seems much
nicer than expecting it to be the last button.

[newt (away), 2016-01-08 23:44:16]

https://codereview.chromium.org/1569783005/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/web_notification_big.xml (right):

https://codereview.chromium.org/1569783005/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/web_notification_big.xml:122:
android:layout_height="40dp"
it's safer to use minHeight and wrap_content for TextViews; that'll ensure the
text doesn't get clipped if the user's system font size is set to Huge.

https://codereview.chromium.org/1569783005/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/web_notification_big.xml:125:
android:layout_alignParentTop="true"
remove alignParentTop since this is centered using gravity

https://codereview.chromium.org/1569783005/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/web_notification_big.xml:128:
android:paddingStart="32dp"
Why paddingStart instead of using layout_toEndOf to make this come after the
settings icon?

[Michael van Ouwerkerk, 2016-01-11 16:15:37]

Description was changed from

==========
Notification custom layouts: move settings button to the footer.

* If no actions are specified, the buttons section no longer needs to be shown.
* If there are actions, their buttons will now have more space and can show
longer text labels of a more reasonable size.
* Screenshots of the new look and feel: https://goo.gl/4BW6cG

BUG=528410
==========

to

==========
Notification custom layouts: move settings button to the footer.

* If no actions are specified, the buttons section no longer needs to be shown.
* If there are actions, their buttons will now have more space and can show
longer text labels of a more reasonable size.
* Screenshots of the new look and feel: http://imgur.com/a/zIGi1

BUG=528410
==========

[Michael van Ouwerkerk, 2016-01-11 16:33:17]

Thanks, all done.

https://codereview.chromium.org/1569783005/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/web_notification_big.xml (right):

https://codereview.chromium.org/1569783005/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/web_notification_big.xml:122:
android:layout_height="40dp"
On 2016/01/08 23:44:16, newt wrote:
> it's safer to use minHeight and wrap_content for TextViews; that'll ensure the
> text doesn't get clipped if the user's system font size is set to Huge.

Done.

https://codereview.chromium.org/1569783005/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/web_notification_big.xml:125:
android:layout_alignParentTop="true"
On 2016/01/08 23:44:16, newt wrote:
> remove alignParentTop since this is centered using gravity

I'm not sure I understood this one. This button is in a RelativeLayout, does
that use gravity? I thought it didn't. My understanding is that the gravity
attribute on this button determines where the text inside the button goes, in
case the button is bigger than the text which is usually the case here. The
layout alignment attributes determine where the button goes inside the
RelativeLayout, so these attributes control different things.

https://codereview.chromium.org/1569783005/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/web_notification_big.xml:128:
android:paddingStart="32dp"
On 2016/01/08 23:44:16, newt wrote:
> Why paddingStart instead of using layout_toEndOf to make this come after the
> settings icon?

This way, the button has some blank space that overlaps the icon. This makes it
look as if the icon is touchable and part of the button. This is for the same
reason (to apply a color filter) as we discussed for
web_notification_button.xml.

https://codereview.chromium.org/1569783005/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java
(right):

https://codereview.chromium.org/1569783005/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java:243:
throw new IllegalStateException("There must be an action for the settings
button.");
On 2016/01/08 21:29:31, Peter Beverloo wrote:
> Could we instead have a new method for setting the Site Settings intent, and
> having the *NotificationBuilder decide on the display of that? That seems much
> nicer than expecting it to be the last button.

Yes, I was avoiding that because it's not needed for the
StandardNotificationBuilder, hence not great interface design. Still, it worked
out pretty cleanly. Done.

[newt (away), 2016-01-11 21:08:28]

lgtm from my side

https://codereview.chromium.org/1569783005/diff/1/chrome/android/java/res/lay...
File chrome/android/java/res/layout/web_notification_big.xml (right):

https://codereview.chromium.org/1569783005/diff/1/chrome/android/java/res/lay...
chrome/android/java/res/layout/web_notification_big.xml:125:
android:layout_alignParentTop="true"
On 2016/01/11 16:33:17, Michael van Ouwerkerk wrote:
> On 2016/01/08 23:44:16, newt wrote:
> > remove alignParentTop since this is centered using gravity
> 
> I'm not sure I understood this one. This button is in a RelativeLayout, does
> that use gravity? I thought it didn't. My understanding is that the gravity
> attribute on this button determines where the text inside the button goes, in
> case the button is bigger than the text which is usually the case here. The
> layout alignment attributes determine where the button goes inside the
> RelativeLayout, so these attributes control different things.

You're correct. I must have misread the xml when I left that comment.

[Michael van Ouwerkerk, 2016-01-12 10:03:24]

Peter: I'm going to land this now to get it all in and QA'd before branch cut,
assuming your concerns have been addressed. Let me know if that's not the case,
then I can do a followup.

[Michael van Ouwerkerk, 2016-01-12 10:03:37]

The CQ bit was checked by mvanouwerkerk@chromium.org

[commit-bot: I haz the power, 2016-01-12 10:03:48]

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

[commit-bot: I haz the power, 2016-01-12 10:08:37]

Description was changed from

==========
Notification custom layouts: move settings button to the footer.

* If no actions are specified, the buttons section no longer needs to be shown.
* If there are actions, their buttons will now have more space and can show
longer text labels of a more reasonable size.
* Screenshots of the new look and feel: http://imgur.com/a/zIGi1

BUG=528410
==========

to

==========
Notification custom layouts: move settings button to the footer.

* If no actions are specified, the buttons section no longer needs to be shown.
* If there are actions, their buttons will now have more space and can show
longer text labels of a more reasonable size.
* Screenshots of the new look and feel: http://imgur.com/a/zIGi1

BUG=528410
==========

[commit-bot: I haz the power, 2016-01-12 10:08:37]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-12 10:09:38]

Description was changed from

==========
Notification custom layouts: move settings button to the footer.

* If no actions are specified, the buttons section no longer needs to be shown.
* If there are actions, their buttons will now have more space and can show
longer text labels of a more reasonable size.
* Screenshots of the new look and feel: http://imgur.com/a/zIGi1

BUG=528410
==========

to

==========
Notification custom layouts: move settings button to the footer.

* If no actions are specified, the buttons section no longer needs to be shown.
* If there are actions, their buttons will now have more space and can show
longer text labels of a more reasonable size.
* Screenshots of the new look and feel: http://imgur.com/a/zIGi1

BUG=528410

Committed: https://crrev.com/640183fab81bde87b5a2ac82246de7a7e1e521a6
Cr-Commit-Position: refs/heads/master@{#368844}
==========

[commit-bot: I haz the power, 2016-01-12 10:09:38]

Patchset 3 (id:??) landed as
https://crrev.com/640183fab81bde87b5a2ac82246de7a7e1e521a6
Cr-Commit-Position: refs/heads/master@{#368844}