Parse "purpose" member from Web Manifest

Parse "purpose" member from Web Manifest

This CL adds functionality to parse "purpose" member of Web Manifest. The
"purpose" member identifies the intended use of the icon. Chrome will use the
"purpose" member to identify icons to use as system tray icons for
notifications. On Android, system tray icons must be "white on a transparent
background"
https://developer.android.com/guide/practices/ui_guidelines/icon_design_status_bar.html

Link to Intent to Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/1ebMBNhqRew

BUG=649771
Committed: https://crrev.com/8fb77ce37de979cade424ef74f4301ba7241068b
Cr-Commit-Position: refs/heads/master@{#439967}

[F, 2016-10-17 20:35:03]

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

[commit-bot: I haz the power, 2016-10-17 20:35:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[F, 2016-10-17 20:35:35]

The CQ bit was unchecked by zpeng@chromium.org

[F, 2016-10-17 20:42:04]

Patchset #1 (id:1) has been deleted

[F, 2016-10-17 20:42:24]

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

[commit-bot: I haz the power, 2016-10-17 20:42:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[F, 2016-10-17 20:45:18]

zpeng@chromium.org changed reviewers:
+ mlamouri@chromium.org, pkotwicz@chromium.org

[F, 2016-10-17 20:45:18]

Hi Mounir & Peter, PTAL. Thanks!

[commit-bot: I haz the power, 2016-10-17 21:17:03]

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

[commit-bot: I haz the power, 2016-10-17 21:17:04]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[pkotwicz, 2016-10-18 14:13:56]

LGTM

mlamouri@ your turn

I have also CCed Peter Beverloo who championed the new "purpose" member in the
Web Manifest

https://codereview.chromium.org/2425833002/diff/20001/content/public/common/m...
File content/public/common/manifest.h (right):

https://codereview.chromium.org/2425833002/diff/20001/content/public/common/m...
content/public/common/manifest.h:55: // Empty if the field was not present or
not of type "string". Contains
Nit: "Contains" -> "Defaults to"

https://codereview.chromium.org/2425833002/diff/20001/content/public/common/m...
content/public/common/manifest.h:57: std::vector<IconPurpose> purpose;
I think that "purposes" is a better name because "purposes" is a vector and can
contain many elements. I know that this conflicts with the spec.

Aside: The spec for "purposes" is only 2 months old. We could change "purpose"
to "purposes" in the spec if we wanted to.

https://codereview.chromium.org/2425833002/diff/20001/content/renderer/manife...
File content/renderer/manifest/manifest_parser.cc (right):

https://codereview.chromium.org/2425833002/diff/20001/content/renderer/manife...
content/renderer/manifest/manifest_parser.cc:282:
std::vector<Manifest::Icon::IconPurpose> purpose;
Nit: |purpose| -> |purposes| because this is a std::vector which can have
multiple purposes

https://codereview.chromium.org/2425833002/diff/20001/content/renderer/manife...
content/renderer/manifest/manifest_parser.cc:290: for (base::string16 keyword :
keywords) {
I think that your for() loop copies elements into |keyword|. I think that this
is better

for (const base::string16& keyword : keywords) {
}

https://codereview.chromium.org/2425833002/diff/20001/content/renderer/manife...
File content/renderer/manifest/manifest_parser.h (right):

https://codereview.chromium.org/2425833002/diff/20001/content/renderer/manife...
content/renderer/manifest/manifest_parser.h:137: // parsed icon purposes, an
empty vector if the field was not present or of
Nit: "or of type" -> "or not of type"

https://codereview.chromium.org/2425833002/diff/20001/content/renderer/manife...
content/renderer/manifest/manifest_parser.h:138: // type "string", or a vector
containing Manifest::Icon::IconPurpose::Any for
Nit: "or a vector" -> "and a vector"

https://codereview.chromium.org/2425833002/diff/20001/content/renderer/manife...
File content/renderer/manifest/manifest_parser_unittest.cc (right):

https://codereview.chromium.org/2425833002/diff/20001/content/renderer/manife...
content/renderer/manifest/manifest_parser_unittest.cc:910: EXPECT_EQ(1u,
GetErrorCount());
When the remainder of the test will crash (as opposed to just failing) if the
condition if false, we use ASSERT_EQ() instead of EXPECT_EQ()

ASSERT_EQ() makes the test stop & fail if the condition fails
EXPECT_EQ() makes the test fail but subsequent conditions are still evaluated.
I know that the remainder of this file does not use ASSERT_EQ() but we might as
well make new tests better.

In the case of this test.
EXPECT_EQ(1u, GetErrorCount()) should be ASSERT_EQ(1u, GetErrorCount())
because the array access on line 911 will fail if the condition on line 910
fails

https://codereview.chromium.org/2425833002/diff/20001/content/renderer/manife...
content/renderer/manifest/manifest_parser_unittest.cc:928: "\"purpose\": \"Any
Badge\" } ] }");
You should add:
ASSERT_EQ(manifest.icons[0].purpose.size(), 2u);

https://codereview.chromium.org/2425833002/diff/20001/content/renderer/manife...
content/renderer/manifest/manifest_parser_unittest.cc:937: "\"purpose\": \"badge
badge\" } ] }");
You should add:
ASSERT_EQ(manifest.icons[0].purpose.size(), 2u);

https://codereview.chromium.org/2425833002/diff/20001/content/renderer/manife...
content/renderer/manifest/manifest_parser_unittest.cc:946: "\"purpose\": \"badge
notification\" } ] }");
You should add:
ASSERT_EQ(manifest.icons[0].purpose.size(), 1u);

https://codereview.chromium.org/2425833002/diff/20001/content/renderer/manife...
content/renderer/manifest/manifest_parser_unittest.cc:953: // 'any' is correctly
added when the purpose is empty.
How about: 'any' is added when developer-supplied-purpose is invalid.

https://codereview.chromium.org/2425833002/diff/20001/content/renderer/manife...
content/renderer/manifest/manifest_parser_unittest.cc:961: }
Maybe add a test case for: \"purpose\": \"any  badge\" to ensure that
whitespaces between elements are trimmed properly

[F, 2016-10-18 15:33:48]

Patchset #2 (id:40001) has been deleted

[F, 2016-10-18 15:33:56]

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

[commit-bot: I haz the power, 2016-10-18 15:34:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[F, 2016-10-18 15:34:47]

Thanks Peter, PTAL!

https://codereview.chromium.org/2425833002/diff/20001/content/public/common/m...
File content/public/common/manifest.h (right):

https://codereview.chromium.org/2425833002/diff/20001/content/public/common/m...
content/public/common/manifest.h:55: // Empty if the field was not present or
not of type "string". Contains
On 2016/10/18 14:13:56, pkotwicz wrote:
> Nit: "Contains" -> "Defaults to"

Done.

https://codereview.chromium.org/2425833002/diff/20001/content/public/common/m...
content/public/common/manifest.h:57: std::vector<IconPurpose> purpose;
On 2016/10/18 14:13:56, pkotwicz wrote:
> I think that "purposes" is a better name because "purposes" is a vector and
can
> contain many elements. I know that this conflicts with the spec.
> 
> Aside: The spec for "purposes" is only 2 months old. We could change "purpose"
> to "purposes" in the spec if we wanted to.

I agree that "purposes" is a much better name. But I think for now we should
stay with the spec. We can update the naming after the spec is updated. WDYT?

https://codereview.chromium.org/2425833002/diff/20001/content/renderer/manife...
File content/renderer/manifest/manifest_parser.cc (right):

https://codereview.chromium.org/2425833002/diff/20001/content/renderer/manife...
content/renderer/manifest/manifest_parser.cc:282:
std::vector<Manifest::Icon::IconPurpose> purpose;
On 2016/10/18 14:13:56, pkotwicz wrote:
> Nit: |purpose| -> |purposes| because this is a std::vector which can have
> multiple purposes

Done.

https://codereview.chromium.org/2425833002/diff/20001/content/renderer/manife...
content/renderer/manifest/manifest_parser.cc:290: for (base::string16 keyword :
keywords) {
On 2016/10/18 14:13:56, pkotwicz wrote:
> I think that your for() loop copies elements into |keyword|. I think that this
> is better
> 
> for (const base::string16& keyword : keywords) {
> }

Done. Thanks!

https://codereview.chromium.org/2425833002/diff/20001/content/renderer/manife...
File content/renderer/manifest/manifest_parser.h (right):

https://codereview.chromium.org/2425833002/diff/20001/content/renderer/manife...
content/renderer/manifest/manifest_parser.h:137: // parsed icon purposes, an
empty vector if the field was not present or of
On 2016/10/18 14:13:56, pkotwicz wrote:
> Nit: "or of type" -> "or not of type"

Done.

https://codereview.chromium.org/2425833002/diff/20001/content/renderer/manife...
content/renderer/manifest/manifest_parser.h:138: // type "string", or a vector
containing Manifest::Icon::IconPurpose::Any for
On 2016/10/18 14:13:56, pkotwicz wrote:
> Nit: "or a vector" -> "and a vector"

Done.

https://codereview.chromium.org/2425833002/diff/20001/content/renderer/manife...
File content/renderer/manifest/manifest_parser_unittest.cc (right):

https://codereview.chromium.org/2425833002/diff/20001/content/renderer/manife...
content/renderer/manifest/manifest_parser_unittest.cc:910: EXPECT_EQ(1u,
GetErrorCount());
On 2016/10/18 14:13:56, pkotwicz wrote:
> When the remainder of the test will crash (as opposed to just failing) if the
> condition if false, we use ASSERT_EQ() instead of EXPECT_EQ()
> 
> ASSERT_EQ() makes the test stop & fail if the condition fails
> EXPECT_EQ() makes the test fail but subsequent conditions are still evaluated.
> I know that the remainder of this file does not use ASSERT_EQ() but we might
as
> well make new tests better.
> 
> In the case of this test.
> EXPECT_EQ(1u, GetErrorCount()) should be ASSERT_EQ(1u, GetErrorCount())
> because the array access on line 911 will fail if the condition on line 910
> fails

Done.

https://codereview.chromium.org/2425833002/diff/20001/content/renderer/manife...
content/renderer/manifest/manifest_parser_unittest.cc:928: "\"purpose\": \"Any
Badge\" } ] }");
On 2016/10/18 14:13:56, pkotwicz wrote:
> You should add:
> ASSERT_EQ(manifest.icons[0].purpose.size(), 2u);

Done.

https://codereview.chromium.org/2425833002/diff/20001/content/renderer/manife...
content/renderer/manifest/manifest_parser_unittest.cc:937: "\"purpose\": \"badge
badge\" } ] }");
On 2016/10/18 14:13:56, pkotwicz wrote:
> You should add:
> ASSERT_EQ(manifest.icons[0].purpose.size(), 2u);

Done.

https://codereview.chromium.org/2425833002/diff/20001/content/renderer/manife...
content/renderer/manifest/manifest_parser_unittest.cc:946: "\"purpose\": \"badge
notification\" } ] }");
On 2016/10/18 14:13:56, pkotwicz wrote:
> You should add:
> ASSERT_EQ(manifest.icons[0].purpose.size(), 1u);

Done.

https://codereview.chromium.org/2425833002/diff/20001/content/renderer/manife...
content/renderer/manifest/manifest_parser_unittest.cc:953: // 'any' is correctly
added when the purpose is empty.
On 2016/10/18 14:13:56, pkotwicz wrote:
> How about: 'any' is added when developer-supplied-purpose is invalid.

Done.

https://codereview.chromium.org/2425833002/diff/20001/content/renderer/manife...
content/renderer/manifest/manifest_parser_unittest.cc:961: }
On 2016/10/18 14:13:56, pkotwicz wrote:
> Maybe add a test case for: \"purpose\": \"any  badge\" to ensure that
> whitespaces between elements are trimmed properly

Done.

[commit-bot: I haz the power, 2016-10-18 16:12:49]

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

[commit-bot: I haz the power, 2016-10-18 16:12:50]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[pkotwicz, 2016-10-18 17:16:12]

https://codereview.chromium.org/2425833002/diff/20001/content/public/common/m...
File content/public/common/manifest.h (right):

https://codereview.chromium.org/2425833002/diff/20001/content/public/common/m...
content/public/common/manifest.h:57: std::vector<IconPurpose> purpose;
sounds good

https://codereview.chromium.org/2425833002/diff/60001/content/public/common/m...
File content/public/common/manifest.h (right):

https://codereview.chromium.org/2425833002/diff/60001/content/public/common/m...
content/public/common/manifest.h:56: // {IconPurpose::ANY} for all other parsing
exceptions.
The curly braces around {IconPurpose::ANY} are new notation to me. Did you see
it elsewhere? I probably missed the notation getting introduced elsewhere

[pkotwicz, 2016-10-18 17:23:55]

Description was changed from

==========
Add function ParseIconPurpose to ManifestParser.

This allows ManifestParser to parse the "purpose" of "icon".

BUG=649771
==========

to

==========
Parse "purpose" member from Web Manifest

This CL adds functionality to parse "purpose" member of Web Manifest. The
"purpose" member identifies the intended use of the icon. Chrome will use the
"purpose" member to identify icons to use as system tray icons for
notifications. On Android, system tray icons must be "white on a transparent
background"

BUG=649771
==========

[pkotwicz, 2016-10-18 17:24:23]

Description was changed from

==========
Parse "purpose" member from Web Manifest

This CL adds functionality to parse "purpose" member of Web Manifest. The
"purpose" member identifies the intended use of the icon. Chrome will use the
"purpose" member to identify icons to use as system tray icons for
notifications. On Android, system tray icons must be "white on a transparent
background"

BUG=649771
==========

to

==========
Parse "purpose" member from Web Manifest

This CL adds functionality to parse "purpose" member of Web Manifest. The
"purpose" member identifies the intended use of the icon. Chrome will use the
"purpose" member to identify icons to use as system tray icons for
notifications. On Android, system tray icons must be "white on a transparent
background"
https://developer.android.com/guide/practices/ui_guidelines/icon_design_statu...

BUG=649771
==========

[pkotwicz, 2016-10-18 17:24:51]

I have also updated the CL description to be more descriptive :)

[pkotwicz, 2016-10-18 17:24:51]

I have also updated the CL description to be more descriptive :)

[F, 2016-10-18 19:24:26]

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

[commit-bot: I haz the power, 2016-10-18 19:24:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[F, 2016-10-18 19:28:28]

Patchset #3 (id:80001) has been deleted

[F, 2016-10-18 19:29:56]

Thanks for updating the description, Peter! PTAL

https://codereview.chromium.org/2425833002/diff/60001/content/public/common/m...
File content/public/common/manifest.h (right):

https://codereview.chromium.org/2425833002/diff/60001/content/public/common/m...
content/public/common/manifest.h:56: // {IconPurpose::ANY} for all other parsing
exceptions.
On 2016/10/18 17:16:12, pkotwicz wrote:
> The curly braces around {IconPurpose::ANY} are new notation to me. Did you see
> it elsewhere? I probably missed the notation getting introduced elsewhere

Fixed. Sorry I meant a vector with IconPurpose::ANY as value.

[F, 2016-10-18 19:30:48]

The CQ bit was checked by zpeng@chromium.org

[F, 2016-10-18 19:30:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkotwicz@chromium.org
Link to the patchset: https://codereview.chromium.org/2425833002/#ps100001
(title: "Addressing comments")

[F, 2016-10-18 19:30:50]

The CQ bit was unchecked by zpeng@chromium.org

[F, 2016-10-18 19:30:53]

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

[commit-bot: I haz the power, 2016-10-18 19:31:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pkotwicz, 2016-10-18 19:53:24]

Still LGTM

[commit-bot: I haz the power, 2016-10-18 22:51:37]

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

[commit-bot: I haz the power, 2016-10-18 22:51:40]

Dry run: This issue passed the CQ dry run.

[F, 2016-10-24 14:55:09]

Hi Mounir, PTAL. Thanks!

[mlamouri (slow - plz ping), 2016-10-25 11:12:22]

Sorry for the delay, I wanted to review the spec. I filed a few issues:
https://github.com/w3c/manifest/issues/511
https://github.com/w3c/manifest/issues/510
https://github.com/w3c/manifest/issues/509
https://github.com/w3c/manifest/issues/508

Let's wait a bit and see how it goes before landing this.

Also, it might be good if you had a CL ready using this. We don't want to add
things if they are not used.

https://codereview.chromium.org/2425833002/diff/100001/content/renderer/manif...
File content/renderer/manifest/manifest_parser.cc (right):

https://codereview.chromium.org/2425833002/diff/100001/content/renderer/manif...
content/renderer/manifest/manifest_parser.cc:296: AddErrorInfo("found icon with
invalid purpose.");
Can you put the keyword that isn't valid in the error message?

https://codereview.chromium.org/2425833002/diff/100001/content/renderer/manif...
content/renderer/manifest/manifest_parser.cc:302: }
That looks odd. Not sure why we need to differentiate no purpose and `purpose:
""`. I know it's per spec but I think we should have both exceptions (not
present/not a string and no valid keywords) return the same value.

[pkotwicz, 2016-10-25 15:59:13]

mlamouri@ sorry that you weren't involved in previous discussions.

The reason for the new "badge" purpose is to enable Web Developers to customize
the system tray icon for notifications originating from a WebAPK. On Android M+,
the system tray icon can be set via the Notification.badge property (launched in
Chrome 53 https://www.chromestatus.com/feature/5630897160192000). The "badge
purpose" in the Web Manifest enables Web Developers to customize the system tray
notification icon for pre-M Android devices.

[pkotwicz, 2016-10-25 15:59:28]

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

[pkotwicz, 2016-10-25 15:59:46]

FYI: Peter Beverloo

[mlamouri (slow - plz ping), 2016-10-31 11:01:54]

This is still in my radar. I filed an issue on the error value inconsistency
which might be the last potential problem we need to resolve.

[mlamouri (slow - plz ping), 2016-10-31 11:02:19]

Also, zpeng@, I think you need to do an intent to ship on this.

[pkotwicz, 2016-10-31 14:31:06]

mlamouri@ what is an "intent to ship"?

It looks like the conversation on https://github.com/w3c/manifest/issues/509 has
died down. Should I ping the bug?

Random comment: "purposes" might be a better name than "purpose" (to match
"icons" and "sizes")

[pkotwicz, 2016-11-01 15:22:55]

mlamouri@ Ping!

[mlamouri (slow - plz ping), 2016-11-02 14:44:59]

It seems that we resolved the failing cases in the spec discussions, see below.
However, you still need an intent to ship:
https://www.chromium.org/blink#TOC-Web-Platform-Changes:-Process

Please let me know if you need any help with regards to the intent to ship
process.

https://codereview.chromium.org/2425833002/diff/100001/content/renderer/manif...
File content/renderer/manifest/manifest_parser.cc (right):

https://codereview.chromium.org/2425833002/diff/100001/content/renderer/manif...
content/renderer/manifest/manifest_parser.cc:285: return purposes;
can you return the array with Manifest::Icon::IconPurpose::ANY in that case.

[pkotwicz, 2016-11-13 22:10:37]

My understanding is that this change fits under this category:
"If your change cannot be implemented in Blink but still exposes functionality
to the open web"

I have filed https://www.chromestatus.com/feature/6690807302062080
mlamouri@ does this description look good?

[mlamouri (slow - plz ping), 2016-11-15 01:17:13]

On 2016/11/13 at 22:10:37, pkotwicz wrote:
> My understanding is that this change fits under this category:
> "If your change cannot be implemented in Blink but still exposes functionality
to the open web"
> 
> I have filed https://www.chromestatus.com/feature/6690807302062080
> mlamouri@ does this description look good?

I think you might want to make the bug a real launch bug (not super important).
It's unclear to me why this is different from the Notifications API badge
property (ie. M+ vs pre-M) but you can clarify that in the Intent to Ship email.
Finally, why isn't zpeng@ in the contacts? :)

[mlamouri (slow - plz ping), 2016-12-09 10:53:51]

Can you add a link to the intent to ship in the CL description?

lgtm but please wait for the approval of the intent to ship before landing :)

Thanks!

[F, 2016-12-09 16:32:14]

Description was changed from

==========
Parse "purpose" member from Web Manifest

This CL adds functionality to parse "purpose" member of Web Manifest. The
"purpose" member identifies the intended use of the icon. Chrome will use the
"purpose" member to identify icons to use as system tray icons for
notifications. On Android, system tray icons must be "white on a transparent
background"
https://developer.android.com/guide/practices/ui_guidelines/icon_design_statu...

BUG=649771
==========

to

==========
Parse "purpose" member from Web Manifest

This CL adds functionality to parse "purpose" member of Web Manifest. The
"purpose" member identifies the intended use of the icon. Chrome will use the
"purpose" member to identify icons to use as system tray icons for
notifications. On Android, system tray icons must be "white on a transparent
background"
https://developer.android.com/guide/practices/ui_guidelines/icon_design_statu...

Link to Intent to Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/1ebMBNhqRew

BUG=649771
==========

[F, 2016-12-09 19:07:23]

Thanks Mounir & Peter, PTAL!

I've added the link to Intent to Ship in the CL description. I'll wait for it to
get approved before I get the ownership LGTM for manifest.h

https://codereview.chromium.org/2425833002/diff/100001/content/renderer/manif...
File content/renderer/manifest/manifest_parser.cc (right):

https://codereview.chromium.org/2425833002/diff/100001/content/renderer/manif...
content/renderer/manifest/manifest_parser.cc:285: return purposes;
On 2016/11/02 14:44:59, mlamouri wrote:
> can you return the array with Manifest::Icon::IconPurpose::ANY in that case.

Done.

https://codereview.chromium.org/2425833002/diff/100001/content/renderer/manif...
content/renderer/manifest/manifest_parser.cc:296: AddErrorInfo("found icon with
invalid purpose.");
On 2016/10/25 11:12:22, mlamouri wrote:
> Can you put the keyword that isn't valid in the error message?

Unfortunately the keyword is of type string16, while AddErrorInfo only accepts
string. I don't think that transforming keyword to string is a good idea here.
WDYT?

https://codereview.chromium.org/2425833002/diff/100001/content/renderer/manif...
content/renderer/manifest/manifest_parser.cc:302: }
On 2016/10/25 11:12:22, mlamouri wrote:
> That looks odd. Not sure why we need to differentiate no purpose and `purpose:
> ""`. I know it's per spec but I think we should have both exceptions (not
> present/not a string and no valid keywords) return the same value.

Done.

[F, 2016-12-15 20:07:36]

zpeng@chromium.org changed reviewers:
+ brettw@chromium.org

[F, 2016-12-15 20:07:37]

Hi Brett, PTAL. Thanks!

The Intent to Ship has been approved.

[F, 2016-12-16 20:21:30]

zpeng@chromium.org changed reviewers:
- brettw@chromium.org

[F, 2016-12-16 20:22:36]

zpeng@chromium.org changed reviewers:
+ pfeldman@chromium.org

[F, 2016-12-16 20:22:37]

Hi Pavel, PTAL. Thanks!

The Intent to Ship has been approved

[pfeldman, 2016-12-20 17:54:15]

lgtm

[F, 2016-12-20 18:02:58]

The CQ bit was checked by zpeng@chromium.org

[F, 2016-12-20 18:02:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkotwicz@chromium.org
Link to the patchset: https://codereview.chromium.org/2425833002/#ps120001
(title: "Addressing comments")

[commit-bot: I haz the power, 2016-12-20 18:03:23]

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, 2016-12-20 18:37:29]

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

[commit-bot: I haz the power, 2016-12-20 18:37:31]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[F, 2016-12-20 19:30:13]

The CQ bit was checked by zpeng@chromium.org

[F, 2016-12-20 19:30:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkotwicz@chromium.org, pfeldman@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2425833002/#ps140001
(title: "Fix")

[commit-bot: I haz the power, 2016-12-20 19:30:54]

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, 2016-12-20 21:15:33]

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

[commit-bot: I haz the power, 2016-12-20 21:15:34]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[F, 2016-12-21 00:20:26]

The CQ bit was checked by zpeng@chromium.org

[commit-bot: I haz the power, 2016-12-21 00:21:20]

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, 2016-12-21 01:31:39]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1482279626238770,
"parent_rev": "697d4f6c20a049d6c6b9fa31260904bdbdfa6191", "commit_rev":
"1ca0075f3d82d1221dad35960a192d4fafa2c7b6"}

[commit-bot: I haz the power, 2016-12-21 01:33:25]

Description was changed from

==========
Parse "purpose" member from Web Manifest

This CL adds functionality to parse "purpose" member of Web Manifest. The
"purpose" member identifies the intended use of the icon. Chrome will use the
"purpose" member to identify icons to use as system tray icons for
notifications. On Android, system tray icons must be "white on a transparent
background"
https://developer.android.com/guide/practices/ui_guidelines/icon_design_statu...

Link to Intent to Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/1ebMBNhqRew

BUG=649771
==========

to

==========
Parse "purpose" member from Web Manifest

This CL adds functionality to parse "purpose" member of Web Manifest. The
"purpose" member identifies the intended use of the icon. Chrome will use the
"purpose" member to identify icons to use as system tray icons for
notifications. On Android, system tray icons must be "white on a transparent
background"
https://developer.android.com/guide/practices/ui_guidelines/icon_design_statu...

Link to Intent to Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/1ebMBNhqRew

BUG=649771

Review-Url: https://codereview.chromium.org/2425833002
Committed:
https://chromium.googlesource.com/chromium/src/+/1ca0075f3d82d1221dad35960a19...
==========

[commit-bot: I haz the power, 2016-12-21 01:33:30]

Committed patchset #5 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/1ca0075f3d82d1221dad35960a19...

[commit-bot: I haz the power, 2016-12-21 01:34:48]

Description was changed from

==========
Parse "purpose" member from Web Manifest

This CL adds functionality to parse "purpose" member of Web Manifest. The
"purpose" member identifies the intended use of the icon. Chrome will use the
"purpose" member to identify icons to use as system tray icons for
notifications. On Android, system tray icons must be "white on a transparent
background"
https://developer.android.com/guide/practices/ui_guidelines/icon_design_statu...

Link to Intent to Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/1ebMBNhqRew

BUG=649771

Review-Url: https://codereview.chromium.org/2425833002
Committed:
https://chromium.googlesource.com/chromium/src/+/1ca0075f3d82d1221dad35960a19...
==========

to

==========
Parse "purpose" member from Web Manifest

This CL adds functionality to parse "purpose" member of Web Manifest. The
"purpose" member identifies the intended use of the icon. Chrome will use the
"purpose" member to identify icons to use as system tray icons for
notifications. On Android, system tray icons must be "white on a transparent
background"
https://developer.android.com/guide/practices/ui_guidelines/icon_design_statu...

Link to Intent to Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/1ebMBNhqRew

BUG=649771

Committed: https://crrev.com/8fb77ce37de979cade424ef74f4301ba7241068b
Cr-Commit-Position: refs/heads/master@{#439967}
==========

[commit-bot: I haz the power, 2016-12-21 01:34:50]

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