Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2820433002:
arc: Add icon resource for the notification in migration-skipped session. (Closed)
Descriptionarc: Add icon resource for the notification in migration-skipped session.
BUG=710285
TEST=manual (the intended image was shown.)
Review-Url: https://codereview.chromium.org/2820433002
Cr-Commit-Position: refs/heads/master@{#466246}
Committed: https://chromium.googlesource.com/chromium/src/+/2e1744c8c2203042c4f459b26eabf19eb0c99641
Patch Set 1 #Patch Set 2 : Optimize png #Patch Set 3 : vector icon #
Total comments: 10
Patch Set 4 : Partially addressed review comments, and invited the designer #Patch Set 5 : reflect after confirmation with the designer #
Total comments: 4
Patch Set 6 : Reflects review comments of #39 #
Messages
Total messages: 46 (27 generated)
Description was changed from ========== arc: Add icon resource for the notification in migration-skipped session. BUG=710285 TEST=compiles (just resource addition, no behavior change is intended.) ========== to ========== arc: Add icon resource for the notification in migration-skipped session. BUG=710285 TEST=manual (the intended image was shown.) ==========
The CQ bit was checked by kinaba@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
kinaba@chromium.org changed reviewers: + oshima@chromium.org
oshima@, could you take a look (as the OWNER of chrome/app/theme/)?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kinaba@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
can this be vector icons? (not sure if it supports gray scale tho)
On 2017/04/13 14:19:51, oshima wrote: > can this be vector icons? (not sure if it supports gray scale tho) Actually I'm not familiar with how these resources are handled. Could you tell me the way to add vector icons? I have .svg version of those two .png files, too, but I could not figure out where to add/how to use them
On 2017/04/13 14:26:00, kinaba wrote: > On 2017/04/13 14:19:51, oshima wrote: > > can this be vector icons? (not sure if it supports gray scale tho) > > Actually I'm not familiar with how these resources are handled. > Could you tell me the way to add vector icons? I have .svg version of those two > .png files, too, but I could not figure out where to add/how to use them Hm, I somehow found https://www.chromium.org/developers/how-tos/vectorized-icons-in-native-chrome-ui So, am I correct that what should I do is to convert .svg to .icon, put it in src/chrome/app/vector_icons, and use gfx::CreateVectorIcon(kIconName) instead of loading from resource bundle? It says the format can hard-code colors if I manually tweak the .icon file by hand.
On 2017/04/13 14:50:51, kinaba wrote: > On 2017/04/13 14:26:00, kinaba wrote: > > On 2017/04/13 14:19:51, oshima wrote: > > > can this be vector icons? (not sure if it supports gray scale tho) > > > > Actually I'm not familiar with how these resources are handled. > > Could you tell me the way to add vector icons? I have .svg version of those > two > > .png files, too, but I could not figure out where to add/how to use them > > Hm, I somehow found > https://www.chromium.org/developers/how-tos/vectorized-icons-in-native-chrome-ui > > So, am I correct that what should I do is to convert .svg to .icon, > put it in src/chrome/app/vector_icons, and use gfx::CreateVectorIcon(kIconName) > instead of loading from resource bundle? > It says the format can hard-code colors if I manually tweak the .icon file by > hand. Yes, so it's not suitable for complicated color icon (yet), but this one may be doable.
The CQ bit was checked by kinaba@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
kinaba@chromium.org changed reviewers: + estade@chromium.org
Updated to use a vector icon. Please take another look. +estade@ from chrome/app/vector_icons/OWNERS
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm thanks!
https://codereview.chromium.org/2820433002/diff/40001/chrome/app/vector_icons... File chrome/app/vector_icons/arc_migrate_encryption_notification.icon (right): https://codereview.chromium.org/2820433002/diff/40001/chrome/app/vector_icons... chrome/app/vector_icons/arc_migrate_encryption_notification.icon:1: CANVAS_DIMENSIONS, 112, nit: copyright header https://codereview.chromium.org/2820433002/diff/40001/chrome/app/vector_icons... chrome/app/vector_icons/arc_migrate_encryption_notification.icon:2: MOVE_TO, 0, 0, you don't need/want this box which is an unfortunate artifact of the design tool that SVGOMG and Skiafy aren't smart enough to remove. https://codereview.chromium.org/2820433002/diff/40001/chrome/app/vector_icons... chrome/app/vector_icons/arc_migrate_encryption_notification.icon:11: PATH_COLOR_ARGB, 0xFF, 0x75, 0x75, 0x75, can you double check with the designer that we don't want to reuse the same shade of grey that we apply to icons throughout most of chrome and chrome os, i.e. #5a5a5a While you're at it, can we verify we don't want to reuse an existing settings icon (cog) like the one in the system menu? https://codereview.chromium.org/2820433002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_migration_guide_notification.cc (right): https://codereview.chromium.org/2820433002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_migration_guide_notification.cc:68: gfx::Image(gfx::CreateVectorIcon( I believe you need to specify a size or this icon will be created at 112x112 dip. (It may look normal anyway because we're scaling it down in the UI but that's not optimal.) It looks like this icon only actually uses 80x80 dip (extra 16dip of padding on all sides built in) which aligns with the 80 of kNotificationIconSize. Anyway, I'm pretty confused about this --- could you post a screenshot? https://codereview.chromium.org/2820433002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_migration_guide_notification.cc:69: kArcMigrateEncryptionNotificationIcon, SK_ColorTRANSPARENT)), if you don't need to pass a color (because all the colors are defined in the icon) you should use gfx::kPlaceholderColor. However I suspect you put transparent to make that extra box in the .icon file go away.
The CQ bit was checked by kinaba@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Elizabeth, could you answer the questions from Evan? (Inserted "elizabethchiu@" below for the points I want to ask your insight.) https://codereview.chromium.org/2820433002/diff/40001/chrome/app/vector_icons... File chrome/app/vector_icons/arc_migrate_encryption_notification.icon (right): https://codereview.chromium.org/2820433002/diff/40001/chrome/app/vector_icons... chrome/app/vector_icons/arc_migrate_encryption_notification.icon:1: CANVAS_DIMENSIONS, 112, On 2017/04/14 14:46:41, Evan Stade wrote: > nit: copyright header Done. https://codereview.chromium.org/2820433002/diff/40001/chrome/app/vector_icons... chrome/app/vector_icons/arc_migrate_encryption_notification.icon:2: MOVE_TO, 0, 0, On 2017/04/14 14:46:41, Evan Stade wrote: > you don't need/want this box which is an unfortunate artifact of the design tool > that SVGOMG and Skiafy aren't smart enough to remove. Done. https://codereview.chromium.org/2820433002/diff/40001/chrome/app/vector_icons... chrome/app/vector_icons/arc_migrate_encryption_notification.icon:11: PATH_COLOR_ARGB, 0xFF, 0x75, 0x75, 0x75, On 2017/04/14 14:46:41, Evan Stade wrote: > can you double check with the designer that we don't want to reuse the same > shade of grey that we apply to icons throughout most of chrome and chrome os, > i.e. #5a5a5a > > While you're at it, can we verify we don't want to reuse an existing settings > icon (cog) like the one in the system menu? elizabethchiu@, could you answer the question on the color? fyi, by reusing the system menu cog icon what I could replicate was something like this: https://drive.google.com/file/d/0B5GWhwdfG_jTZElBcjhWM2ZGRnM/view?usp=sharing (without the background round rect.) https://codereview.chromium.org/2820433002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_migration_guide_notification.cc (right): https://codereview.chromium.org/2820433002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_migration_guide_notification.cc:68: gfx::Image(gfx::CreateVectorIcon( On 2017/04/14 14:46:41, Evan Stade wrote: > I believe you need to specify a size or this icon will be created at 112x112 > dip. (It may look normal anyway because we're scaling it down in the UI but > that's not optimal.) > > It looks like this icon only actually uses 80x80 dip (extra 16dip of padding on > all sides built in) which aligns with the 80 of kNotificationIconSize. > > Anyway, I'm pretty confused about this --- could you post a screenshot? elizabethchiu@ Here's the screenshot: https://drive.google.com/file/d/0B5GWhwdfG_jTdVFJWjFZOFlZajA/view?usp=sharing From the UI mock I guessed that there's the intentional margin otherwise the image fills up to the border of the notification popup. https://codereview.chromium.org/2820433002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_migration_guide_notification.cc:69: kArcMigrateEncryptionNotificationIcon, SK_ColorTRANSPARENT)), On 2017/04/14 14:46:41, Evan Stade wrote: > if you don't need to pass a color (because all the colors are defined in the > icon) you should use gfx::kPlaceholderColor. However I suspect you put > transparent to make that extra box in the .icon file go away. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
estade@ For the size and spec please refer to this doc: https://drive.google.com/open?id=0B6x6iYCtKinEb1YxeXAtSHJzZmM This icon is being use in the other system notifications as well, so we should keep the same colors. Here is the screenshot of the notification. https://folio.googleplex.com/ext4-crypto-migration-ui-flow/2-user%20skip%20th... On 2017/04/17 08:08:55, kinaba wrote: > Elizabeth, could you answer the questions from Evan? > > (Inserted "elizabethchiu@" below for the points I want to ask your insight.) > > https://codereview.chromium.org/2820433002/diff/40001/chrome/app/vector_icons... > File chrome/app/vector_icons/arc_migrate_encryption_notification.icon (right): > > https://codereview.chromium.org/2820433002/diff/40001/chrome/app/vector_icons... > chrome/app/vector_icons/arc_migrate_encryption_notification.icon:1: > CANVAS_DIMENSIONS, 112, > On 2017/04/14 14:46:41, Evan Stade wrote: > > nit: copyright header > > Done. > > https://codereview.chromium.org/2820433002/diff/40001/chrome/app/vector_icons... > chrome/app/vector_icons/arc_migrate_encryption_notification.icon:2: MOVE_TO, 0, > 0, > On 2017/04/14 14:46:41, Evan Stade wrote: > > you don't need/want this box which is an unfortunate artifact of the design > tool > > that SVGOMG and Skiafy aren't smart enough to remove. > > Done. > > https://codereview.chromium.org/2820433002/diff/40001/chrome/app/vector_icons... > chrome/app/vector_icons/arc_migrate_encryption_notification.icon:11: > PATH_COLOR_ARGB, 0xFF, 0x75, 0x75, 0x75, > On 2017/04/14 14:46:41, Evan Stade wrote: > > can you double check with the designer that we don't want to reuse the same > > shade of grey that we apply to icons throughout most of chrome and chrome os, > > i.e. #5a5a5a > > > > While you're at it, can we verify we don't want to reuse an existing settings > > icon (cog) like the one in the system menu? > > elizabethchiu@, could you answer the question on the color? > > > fyi, by reusing the system menu cog icon what I could replicate was something > like this: > https://drive.google.com/file/d/0B5GWhwdfG_jTZElBcjhWM2ZGRnM/view?usp=sharing > (without the background round rect.) > > https://codereview.chromium.org/2820433002/diff/40001/chrome/browser/chromeos... > File chrome/browser/chromeos/arc/arc_migration_guide_notification.cc (right): > > https://codereview.chromium.org/2820433002/diff/40001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_migration_guide_notification.cc:68: > gfx::Image(gfx::CreateVectorIcon( > On 2017/04/14 14:46:41, Evan Stade wrote: > > I believe you need to specify a size or this icon will be created at 112x112 > > dip. (It may look normal anyway because we're scaling it down in the UI but > > that's not optimal.) > > > > It looks like this icon only actually uses 80x80 dip (extra 16dip of padding > on > > all sides built in) which aligns with the 80 of kNotificationIconSize. > > > > Anyway, I'm pretty confused about this --- could you post a screenshot? > > elizabethchiu@ > > Here's the screenshot: > https://drive.google.com/file/d/0B5GWhwdfG_jTdVFJWjFZOFlZajA/view?usp=sharing > > From the UI mock I guessed that there's the intentional margin otherwise the > image fills up to the border of the notification popup. > > https://codereview.chromium.org/2820433002/diff/40001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_migration_guide_notification.cc:69: > kArcMigrateEncryptionNotificationIcon, SK_ColorTRANSPARENT)), > On 2017/04/14 14:46:41, Evan Stade wrote: > > if you don't need to pass a color (because all the colors are defined in the > > icon) you should use gfx::kPlaceholderColor. However I suspect you put > > transparent to make that extra box in the .icon file go away. > > Done.
I will note that the screenshot of the implementation doesn't match the mock. For my edification, why are we using the "normal" icon layout instead of the custom "full" image? -- Evan Stade On Mon, Apr 17, 2017 at 9:52 AM, <elizabethchiu@chromium.org> wrote: > estade@ > > For the size and spec please refer to this doc: > https://drive.google.com/open?id=0B6x6iYCtKinEb1YxeXAtSHJzZmM > > This icon is being use in the other system notifications as well, so we > should > keep the same colors. Here is the screenshot of the notification. > https://folio.googleplex.com/ext4-crypto-migration-ui-flow/ > 2-user%20skip%20then%20migrate#%2F2D%20-%20update%20notification.png > > > > On 2017/04/17 08:08:55, kinaba wrote: > > Elizabeth, could you answer the questions from Evan? > > > > (Inserted "elizabethchiu@" below for the points I want to ask your > insight.) > > > > > https://codereview.chromium.org/2820433002/diff/40001/ > chrome/app/vector_icons/arc_migrate_encryption_notification.icon > > File chrome/app/vector_icons/arc_migrate_encryption_notification.icon > (right): > > > > > https://codereview.chromium.org/2820433002/diff/40001/ > chrome/app/vector_icons/arc_migrate_encryption_notification.icon#newcode1 > > chrome/app/vector_icons/arc_migrate_encryption_notification.icon:1: > > CANVAS_DIMENSIONS, 112, > > On 2017/04/14 14:46:41, Evan Stade wrote: > > > nit: copyright header > > > > Done. > > > > > https://codereview.chromium.org/2820433002/diff/40001/ > chrome/app/vector_icons/arc_migrate_encryption_notification.icon#newcode2 > > chrome/app/vector_icons/arc_migrate_encryption_notification.icon:2: > MOVE_TO, > 0, > > 0, > > On 2017/04/14 14:46:41, Evan Stade wrote: > > > you don't need/want this box which is an unfortunate artifact of the > design > > tool > > > that SVGOMG and Skiafy aren't smart enough to remove. > > > > Done. > > > > > https://codereview.chromium.org/2820433002/diff/40001/ > chrome/app/vector_icons/arc_migrate_encryption_notification.icon#newcode11 > > chrome/app/vector_icons/arc_migrate_encryption_notification.icon:11: > > PATH_COLOR_ARGB, 0xFF, 0x75, 0x75, 0x75, > > On 2017/04/14 14:46:41, Evan Stade wrote: > > > can you double check with the designer that we don't want to reuse the > same > > > shade of grey that we apply to icons throughout most of chrome and > chrome > os, > > > i.e. #5a5a5a > > > > > > While you're at it, can we verify we don't want to reuse an existing > settings > > > icon (cog) like the one in the system menu? > > > > elizabethchiu@, could you answer the question on the color? > > > > > > fyi, by reusing the system menu cog icon what I could replicate was > something > > like this: > > https://drive.google.com/file/d/0B5GWhwdfG_jTZElBcjhWM2ZGRnM/view?usp= > sharing > > (without the background round rect.) > > > > > https://codereview.chromium.org/2820433002/diff/40001/ > chrome/browser/chromeos/arc/arc_migration_guide_notification.cc > > File chrome/browser/chromeos/arc/arc_migration_guide_notification.cc > (right): > > > > > https://codereview.chromium.org/2820433002/diff/40001/ > chrome/browser/chromeos/arc/arc_migration_guide_notification.cc#newcode68 > > chrome/browser/chromeos/arc/arc_migration_guide_notification.cc:68: > > gfx::Image(gfx::CreateVectorIcon( > > On 2017/04/14 14:46:41, Evan Stade wrote: > > > I believe you need to specify a size or this icon will be created at > 112x112 > > > dip. (It may look normal anyway because we're scaling it down in the > UI but > > > that's not optimal.) > > > > > > It looks like this icon only actually uses 80x80 dip (extra 16dip of > padding > > on > > > all sides built in) which aligns with the 80 of kNotificationIconSize. > > > > > > Anyway, I'm pretty confused about this --- could you post a screenshot? > > > > elizabethchiu@ > > > > Here's the screenshot: > > https://drive.google.com/file/d/0B5GWhwdfG_jTdVFJWjFZOFlZajA/view?usp= > sharing > > > > From the UI mock I guessed that there's the intentional margin otherwise > the > > image fills up to the border of the notification popup. > > > > > https://codereview.chromium.org/2820433002/diff/40001/ > chrome/browser/chromeos/arc/arc_migration_guide_notification.cc#newcode69 > > chrome/browser/chromeos/arc/arc_migration_guide_notification.cc:69: > > kArcMigrateEncryptionNotificationIcon, SK_ColorTRANSPARENT)), > > On 2017/04/14 14:46:41, Evan Stade wrote: > > > if you don't need to pass a color (because all the colors are defined > in the > > > icon) you should use gfx::kPlaceholderColor. However I suspect you put > > > transparent to make that extra box in the .icon file go away. > > > > Done. > > > > https://codereview.chromium.org/2820433002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/04/17 15:52:02, elizabethchiu wrote: > estade@ > > For the size and spec please refer to this doc: > https://drive.google.com/open?id=0B6x6iYCtKinEb1YxeXAtSHJzZmM If 40x40 floating in the 80x80 area is the request, we need to adjust the icon file as so. The current svg source image is 80x80 in 112x112, so the round rect becomes bigger than 40x40. Or, if "full" size is intended, I need to remove the margin. Which is needed? (and why? as Evan asking.) > This icon is being use in the other system notifications as well, so we should > keep the same colors. Here is the screenshot of the notification. > https://folio.googleplex.com/ext4-crypto-migration-ui-flow/2-user%20skip%20th... Is the icon is already use in Chrome? I could not find it but if so I can reuse it rather than newly adding it.
kinaba@ and estade@ I updated the svg icons to remove the 112x112 box, so now you can apply the icons without the margin. It's 160 in 2x and 80 in 1x. Let me know if that works for you. Also you can ping me directly on hangout or leave comments on the bug if you have questions, as I do not normally use code review. On 2017/04/17 16:08:39, kinaba wrote: > On 2017/04/17 15:52:02, elizabethchiu wrote: > > > > > > For the size and spec please refer to this doc: > > https://drive.google.com/open?id=0B6x6iYCtKinEb1YxeXAtSHJzZmM > > If 40x40 floating in the 80x80 area is the request, we need to adjust the icon > file as so. > The current svg source image is 80x80 in 112x112, so the round rect becomes > bigger than 40x40. > Or, if "full" size is intended, I need to remove the margin. Which is needed? > (and why? as Evan asking.) > > > This icon is being use in the other system notifications as well, so we should > > keep the same colors. Here is the screenshot of the notification. > > > https://folio.googleplex.com/ext4-crypto-migration-ui-flow/2-user%20skip%20th... > > Is the icon is already use in Chrome? I could not find it but if so I can reuse > it rather than newly adding it.
I removed all the padding and now the icons are 80 for 2x and 40 for 1x. Let me know if you have any questions. On 2017/04/19 16:08:37, elizabethchiu wrote: > kinaba@ and estade@ > > I updated the svg icons to remove the 112x112 box, so now you can apply the > icons without the margin. > It's 160 in 2x and 80 in 1x. Let me know if that works for you. Also you can > ping me directly on hangout or leave comments on the bug if you have questions, > as I do not normally use code review. > > On 2017/04/17 16:08:39, kinaba wrote: > > On 2017/04/17 15:52:02, elizabethchiu wrote: > > > > > > > > > For the size and spec please refer to this doc: > > > https://drive.google.com/open?id=0B6x6iYCtKinEb1YxeXAtSHJzZmM > > > > If 40x40 floating in the 80x80 area is the request, we need to adjust the icon > > file as so. > > The current svg source image is 80x80 in 112x112, so the round rect becomes > > bigger than 40x40. > > Or, if "full" size is intended, I need to remove the margin. Which is needed? > > (and why? as Evan asking.) > > > > > This icon is being use in the other system notifications as well, so we > should > > > keep the same colors. Here is the screenshot of the notification. > > > > > > https://folio.googleplex.com/ext4-crypto-migration-ui-flow/2-user%20skip%20th... > > > > Is the icon is already use in Chrome? I could not find it but if so I can > reuse > > it rather than newly adding it.
The CQ bit was checked by kinaba@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Chatted offline with Elizabeth and confirmed that what we want from UI design perspective is: * The gray color #757575 as provided. * "normal" layout https://drive.google.com/open?id=0B6x6iYCtKinEb1YxeXAtSHJzZmM So, Patch Set 5 adjusted to the plan ,the vector icon now comes with 80x80 size, with center 40x40 filled by the image. Screenshot on Pixel 2 Chromebook: https://drive.google.com/file/d/0B5GWhwdfG_jTM3hBQlR2UldTWTg/view?usp=sharing Evan, could you take yet another look?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2820433002/diff/80001/chrome/app/vector_icons... File chrome/app/vector_icons/arc_migrate_encryption_notification.icon (right): https://codereview.chromium.org/2820433002/diff/80001/chrome/app/vector_icons... chrome/app/vector_icons/arc_migrate_encryption_notification.icon:3: // found in the LICENSE file. nit: Insert blank line https://codereview.chromium.org/2820433002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_migration_guide_notification.cc (right): https://codereview.chromium.org/2820433002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_migration_guide_notification.cc:72: message_center::kNotificationIconSize, gfx::kPlaceholderColor)), nit: you don't need to specify the size
thanks https://codereview.chromium.org/2820433002/diff/80001/chrome/app/vector_icons... File chrome/app/vector_icons/arc_migrate_encryption_notification.icon (right): https://codereview.chromium.org/2820433002/diff/80001/chrome/app/vector_icons... chrome/app/vector_icons/arc_migrate_encryption_notification.icon:3: // found in the LICENSE file. On 2017/04/20 14:26:38, Evan Stade wrote: > nit: Insert blank line Done. https://codereview.chromium.org/2820433002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_migration_guide_notification.cc (right): https://codereview.chromium.org/2820433002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_migration_guide_notification.cc:72: message_center::kNotificationIconSize, gfx::kPlaceholderColor)), On 2017/04/20 14:26:38, Evan Stade wrote: > nit: you don't need to specify the size Done.
The CQ bit was checked by kinaba@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/2820433002/#ps100001 (title: "Reflects review comments of #39")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492740358406180,
"parent_rev": "bba53b15c45ce0fa66d3343befae28aa695311c1", "commit_rev":
"2e1744c8c2203042c4f459b26eabf19eb0c99641"}
Message was sent while issue was closed.
Description was changed from ========== arc: Add icon resource for the notification in migration-skipped session. BUG=710285 TEST=manual (the intended image was shown.) ========== to ========== arc: Add icon resource for the notification in migration-skipped session. BUG=710285 TEST=manual (the intended image was shown.) Review-Url: https://codereview.chromium.org/2820433002 Cr-Commit-Position: refs/heads/master@{#466246} Committed: https://chromium.googlesource.com/chromium/src/+/2e1744c8c2203042c4f459b26eab... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/2e1744c8c2203042c4f459b26eab... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
