[Smart Lock, UI] Add ability to have an icon to the SnanackBar.

[Smart Lock, UI] Add ability to have an icon to the SnanackBar.

BUG=539355
Committed: https://crrev.com/1ece07a85c1c20072a2f6415d0887c55b5302d83
Cr-Commit-Position: refs/heads/master@{#362962}

[melandory, 2015-11-09 22:38:13]

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

[melandory, 2015-11-09 22:38:13]

PTAL, thanks!

[gone, 2015-11-09 22:40:48]

not lgtm; the spec explicitly disallows icons in snackbars:
https://www.google.com/design/spec/components/snackbars-toasts.html#snackbars...

Has this gone through Chrome UX?

[melandory, 2015-11-09 22:49:23]

On 2015/11/09 22:40:48, dfalcantara wrote:
> not lgtm; the spec explicitly disallows icons in snackbars:
>
https://www.google.com/design/spec/components/snackbars-toasts.html#snackbars...
> 
> Has this gone through Chrome UX?

Yep,  mocks for the auto sign-in snackbar for the Smart Lock have an icon:
https://chromium.googlecode.com/issues/attachment?aid=5393550003000&name=%5BU...

I'll talk to Hwi, thanks for pointing me to specs.

[gone, 2015-11-09 22:55:20]

Alright, will review as soon as I'm done with my other review. Thanks!

[gone, 2015-11-09 23:24:26]

(For future reference: https://goto.google.com/identity-toast)

[gone, 2015-11-10 00:35:04]

lgtm % comments

lgtming to avoid another world rotation before review.

https://chromiumcodereview.appspot.com/1430273002/diff/20001/chrome/android/j...
File chrome/android/java/res/layout/snackbar.xml (right):

https://chromiumcodereview.appspot.com/1430273002/diff/20001/chrome/android/j...
chrome/android/java/res/layout/snackbar.xml:13: android:layout_width="24dp"
nit: indentation is messed up

https://chromiumcodereview.appspot.com/1430273002/diff/20001/chrome/android/j...
chrome/android/java/res/layout/snackbar.xml:16:
android:layout_marginStart="12dp"
can you explicitly state what ImageView.ScaleType to use in case a big icon is
passed in?  (Maybe FIT_CENTER?)

https://chromiumcodereview.appspot.com/1430273002/diff/20001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/snackbar/Snackbar.java
(right):

https://chromiumcodereview.appspot.com/1430273002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/Snackbar.java:73:
public Snackbar setIcon(Bitmap icon) {
The spec says that an "icon" only gets added for identity toasts.  Can you make
that requirement explicit and call this function "setProfileImage" (adjusting
all related names accordingly)?  I don't want random Snackbars landing in the
future that will conflict with the Material Design spec.

https://chromiumcodereview.appspot.com/1430273002/diff/20001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarPopupWindow.java
(right):

https://chromiumcodereview.appspot.com/1430273002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/SnackbarPopupWindow.java:106:
mIconView.setVisibility(View.GONE);
Can you change this to make mIconView visible by default, then remove mIconView
instead of setting it to GONE?  If it's removed from the view hierarchy then you
don't have to deal with it, memory or layout-wise.

[gone, 2015-11-10 00:44:41]

Please change SnanackBar -> Snackbar in the commit message, too.

https://chromiumcodereview.appspot.com/1430273002/diff/20001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/snackbar/Snackbar.java
(right):

https://chromiumcodereview.appspot.com/1430273002/diff/20001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/snackbar/Snackbar.java:73:
public Snackbar setIcon(Bitmap icon) {
On 2015/11/10 00:35:04, dfalcantara wrote:
> The spec says that an "icon" only gets added for identity toasts.  Can you
make
> that requirement explicit and call this function "setProfileImage" (adjusting
> all related names accordingly)?  I don't want random Snackbars landing in the
> future that will conflict with the Material Design spec.

Also, could you make javadoc say explicitly that this should only be used for
identity toasts?

[melandory, 2015-12-03 10:20:55]

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

[commit-bot: I haz the power, 2015-12-03 10:21:41]

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

[commit-bot: I haz the power, 2015-12-03 11:09:48]

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

[commit-bot: I haz the power, 2015-12-03 11:09:49]

Dry run: This issue passed the CQ dry run.

[melandory, 2015-12-03 12:34:08]

The CQ bit was checked by melandory@chromium.org

[melandory, 2015-12-03 12:34:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
dfalcantara@chromium.org
Link to the patchset: https://codereview.chromium.org/1430273002/#ps40001
(title: "adress comments")

[commit-bot: I haz the power, 2015-12-03 12:36:20]

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

[commit-bot: I haz the power, 2015-12-03 12:41:47]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-12-03 12:43:05]

Description was changed from

==========
[Smart Lock, UI] Add ability to have an icon to the SnanackBar.

BUG=539355
==========

to

==========
[Smart Lock, UI] Add ability to have an icon to the SnanackBar.

BUG=539355

Committed: https://crrev.com/1ece07a85c1c20072a2f6415d0887c55b5302d83
Cr-Commit-Position: refs/heads/master@{#362962}
==========

[commit-bot: I haz the power, 2015-12-03 12:43:06]

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