Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2686543002:
Add "usages" field to Image in WebApk.proto (Closed)
DescriptionAdd "usages" field to Image in WebApk.proto
"usages" field is used by Chrome to specify intended usages for the image
sent in a WebAPK proto. An image can have multiple usages, e.g. "primary
icon" and "badge icon".
At most two images can have non-empty "usages" fields: the image most
suitable for primary icon, and the image most suitable for badge icon.
The "usages" field in the proto and manifest.icon.purpose defined in the W3C Web
Manifest specification are different.
- "usages" is used by Chrome to distinguish the image most suitable for primary
icon and the image most suitable for badge icon from other images, and from
each other.
- manifest.icon.purpose is used by Web developers to specify intended
usage for the images.
BUG=649771
Review-Url: https://codereview.chromium.org/2686543002
Cr-Commit-Position: refs/heads/master@{#449255}
Committed: https://chromium.googlesource.com/chromium/src/+/fb90fb92c0ce0e549112e9dfbeaeb2309aad883d
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressing comments #Messages
Total messages: 34 (23 generated)
Description was changed from ========== Add usages field to Image in WebApk.proto BUG= ========== to ========== Add "usages" field to Image in WebApk.proto "usages" field is used to specify intended usages for the image sent in a WebAPK proto. An image can have multiple usages, e.g. "primary icon" and "badge icon". The enum is named "Usage" instead of "Purpose" because value "any" in Manifest.Icon.IconPurpose is a wild card while "primary icon" is not. BUG=649771 ==========
Description was changed from ========== Add "usages" field to Image in WebApk.proto "usages" field is used to specify intended usages for the image sent in a WebAPK proto. An image can have multiple usages, e.g. "primary icon" and "badge icon". The enum is named "Usage" instead of "Purpose" because value "any" in Manifest.Icon.IconPurpose is a wild card while "primary icon" is not. BUG=649771 ========== to ========== Add "usages" field to Image in WebApk.proto "usages" field is used to specify intended usages for the image sent in a WebAPK proto. An image can have multiple usages, e.g. "primary icon" and "badge icon". The enum is named "Usage" instead of "Purpose" because value "any" in Manifest.Icon.IconPurpose is a wild card while "primary icon" is not. BUG=649771 ==========
zpeng@chromium.org changed reviewers: + dominickn@chromium.org, pkotwicz@chromium.org
Hi Dominick & Peter, PTAL. Thanks!
The CQ bit was checked by zpeng@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.
pkotwicz@chromium.org changed reviewers: + hartmanng@chromium.org
Passing this over to Glenn who is a server expert https://codereview.chromium.org/2686543002/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapk/webapk.proto (right): https://codereview.chromium.org/2686543002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:92: repeated Usage usages = 7; If we end up putting this information in the Android Manifest, we should make this a repeated field of string. The reason is that other browsers may want to extract the values from the Android Manifest. The string values are specified by the spec so are unlikely to change. The enum values are not specified by the spec so using them from a non-Chrome browser perspective is more risky.
https://codereview.chromium.org/2686543002/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapk/webapk.proto (right): https://codereview.chromium.org/2686543002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:75: enum Usage { It could be useful to have a USAGE_UNDEFINED = 0; as well. https://codereview.chromium.org/2686543002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:76: PRIMARY_ICON = 1; I haven't been following discussions around this - can you explain quickly why we're diverging from the spec here? Under what conditions do we determine that an icon is a "primary icon"? Is it just whenever it's not a badge? https://codereview.chromium.org/2686543002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:92: repeated Usage usages = 7; We've actually already added a Purpose member to the server-side proto (my apologies for not pointing you to it earlier, completely slipped my mind). We don't use it for anything yet, and it would be quite easy for us to change, so we can still support the strategy in this CL if it's better. However, if we go with "Usages" instead of "Purposes" here, please mark 7 as "reserved" and use 8 for Usages to avoid proto incompatibility.
https://codereview.chromium.org/2686543002/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapk/webapk.proto (right): https://codereview.chromium.org/2686543002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:76: PRIMARY_ICON = 1; On 2017/02/07 14:53:22, hartmanng wrote: > I haven't been following discussions around this - can you explain quickly why > we're diverging from the spec here? Under what conditions do we determine that > an icon is a "primary icon"? Is it just whenever it's not a badge? We've discussed offline and I understand the situation now. Thanks for explaining, Felix. https://codereview.chromium.org/2686543002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:92: repeated Usage usages = 7; On 2017/02/07 14:53:22, hartmanng wrote: > We've actually already added a Purpose member to the server-side proto (my > apologies for not pointing you to it earlier, completely slipped my mind). > > We don't use it for anything yet, and it would be quite easy for us to change, > so we can still support the strategy in this CL if it's better. However, if we > go with "Usages" instead of "Purposes" here, please mark 7 as "reserved" and use > 8 for Usages to avoid proto incompatibility. To sum up our offline conversation, I think what we should do (for now, at least), is that you should go forward with this CL and land a Usages member, and leave Purposes out for the time being. Please do mark 7 as reserved and move Usages to 8 in this CL, though. https://codereview.chromium.org/2686543002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:92: repeated Usage usages = 7; On 2017/02/07 14:38:06, pkotwicz wrote: > The string values are specified by the spec so are unlikely to > change. That's not entirely true. The Usage type isn't exactly the same as the spec's official Purpose type. It's true that the spec defines "badge" as a kind of Purpose, but as far as I understand, the spec has no concept of a primary icon - that's a Chrome abstraction.
Will rs lg when hartmanng is happy.
Description was changed from ========== Add "usages" field to Image in WebApk.proto "usages" field is used to specify intended usages for the image sent in a WebAPK proto. An image can have multiple usages, e.g. "primary icon" and "badge icon". The enum is named "Usage" instead of "Purpose" because value "any" in Manifest.Icon.IconPurpose is a wild card while "primary icon" is not. BUG=649771 ========== to ========== Add "usages" field to Image in WebApk.proto "usages" field is used to specify intended usages for the image sent in a WebAPK proto. An image can have multiple usages, e.g. "primary icon" and "badge icon". At most two images can have non-empty "usages" fields: the image most suitable for primary icon, and the image most suitable for badge icon. "usages" field and manifest.icon.purpose as defined in the Web Manifest specification are two different fields. "usages" is used by Chrome to distinguish the image most suitable for primary icon and the image most suitable for badge icon from other images, and from each other. manifest.icon.purpose is used by Web developers to specify intended usage for the images. BUG=649771 ==========
Description was changed from ========== Add "usages" field to Image in WebApk.proto "usages" field is used to specify intended usages for the image sent in a WebAPK proto. An image can have multiple usages, e.g. "primary icon" and "badge icon". At most two images can have non-empty "usages" fields: the image most suitable for primary icon, and the image most suitable for badge icon. "usages" field and manifest.icon.purpose as defined in the Web Manifest specification are two different fields. "usages" is used by Chrome to distinguish the image most suitable for primary icon and the image most suitable for badge icon from other images, and from each other. manifest.icon.purpose is used by Web developers to specify intended usage for the images. BUG=649771 ========== to ========== Add "usages" field to Image in WebApk.proto "usages" field is used to specify intended usages for the image sent in a WebAPK proto. An image can have multiple usages, e.g. "primary icon" and "badge icon". At most two images can have non-empty "usages" fields: the image most suitable for primary icon, and the image most suitable for badge icon. "usages" field and manifest.icon.purpose as defined in the Web Manifest specification are two different fields. "usages" is used by Chrome to distinguish the image most suitable for primary icon and the image most suitable for badge icon from other images, and from each other. manifest.icon.purpose is used by Web developers to specify intended usage for the images. BUG=649771 ==========
Description was changed from ========== Add "usages" field to Image in WebApk.proto "usages" field is used to specify intended usages for the image sent in a WebAPK proto. An image can have multiple usages, e.g. "primary icon" and "badge icon". At most two images can have non-empty "usages" fields: the image most suitable for primary icon, and the image most suitable for badge icon. "usages" field and manifest.icon.purpose as defined in the Web Manifest specification are two different fields. "usages" is used by Chrome to distinguish the image most suitable for primary icon and the image most suitable for badge icon from other images, and from each other. manifest.icon.purpose is used by Web developers to specify intended usage for the images. BUG=649771 ========== to ========== Add "usages" field to Image in WebApk.proto "usages" field is used by Chrome to specify intended usages for the image sent in a WebAPK proto. An image can have multiple usages, e.g. "primary icon" and "badge icon". At most two images can have non-empty "usages" fields: the image most suitable for primary icon, and the image most suitable for badge icon. "usages" field and manifest.icon.purpose as defined in the Web Manifest specification are two different fields. "usages" is used by Chrome to distinguish the image most suitable for primary icon and the image most suitable for badge icon from other images, and from each other. manifest.icon.purpose is used by Web developers to specify intended usage for the images. BUG=649771 ==========
Description was changed from ========== Add "usages" field to Image in WebApk.proto "usages" field is used by Chrome to specify intended usages for the image sent in a WebAPK proto. An image can have multiple usages, e.g. "primary icon" and "badge icon". At most two images can have non-empty "usages" fields: the image most suitable for primary icon, and the image most suitable for badge icon. "usages" field and manifest.icon.purpose as defined in the Web Manifest specification are two different fields. "usages" is used by Chrome to distinguish the image most suitable for primary icon and the image most suitable for badge icon from other images, and from each other. manifest.icon.purpose is used by Web developers to specify intended usage for the images. BUG=649771 ========== to ========== Add "usages" field to Image in WebApk.proto "usages" field is used by Chrome to specify intended usages for the image sent in a WebAPK proto. An image can have multiple usages, e.g. "primary icon" and "badge icon". At most two images can have non-empty "usages" fields: the image most suitable for primary icon, and the image most suitable for badge icon. "usages" field and manifest.icon.purpose as defined in the Web Manifest specification are two different fields. "usages" is used by Chrome to distinguish the image most suitable for primary icon and the image most suitable for badge icon from other images, and from each other. manifest.icon.purpose is used by Web developers to specify intended usage for the images. BUG=649771 ==========
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by zpeng@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...
Thanks Dominick, Glenn, and Peter! PTAL I've updated the the CL description and code comments to reflect the differences between "usages" and manifest.icon.purpose. https://codereview.chromium.org/2686543002/diff/1/chrome/browser/android/weba... File chrome/browser/android/webapk/webapk.proto (right): https://codereview.chromium.org/2686543002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:75: enum Usage { On 2017/02/07 14:53:22, hartmanng wrote: > It could be useful to have a > > USAGE_UNDEFINED = 0; > > as well. Resolved offline :) https://codereview.chromium.org/2686543002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:76: PRIMARY_ICON = 1; On 2017/02/07 18:18:29, hartmanng wrote: > On 2017/02/07 14:53:22, hartmanng wrote: > > I haven't been following discussions around this - can you explain quickly why > > we're diverging from the spec here? Under what conditions do we determine that > > an icon is a "primary icon"? Is it just whenever it's not a badge? > > We've discussed offline and I understand the situation now. Thanks for > explaining, Felix. :) https://codereview.chromium.org/2686543002/diff/1/chrome/browser/android/weba... chrome/browser/android/webapk/webapk.proto:92: repeated Usage usages = 7; On 2017/02/07 18:18:29, hartmanng wrote: > On 2017/02/07 14:53:22, hartmanng wrote: > > We've actually already added a Purpose member to the server-side proto (my > > apologies for not pointing you to it earlier, completely slipped my mind). > > > > We don't use it for anything yet, and it would be quite easy for us to change, > > so we can still support the strategy in this CL if it's better. However, if we > > go with "Usages" instead of "Purposes" here, please mark 7 as "reserved" and > use > > 8 for Usages to avoid proto incompatibility. > > To sum up our offline conversation, I think what we should do (for now, at > least), is that you should go forward with this CL and land a Usages member, and > leave Purposes out for the time being. > > Please do mark 7 as reserved and move Usages to 8 in this CL, though. Done. Thanks for pointing out 7 is already used :)
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add "usages" field to Image in WebApk.proto "usages" field is used by Chrome to specify intended usages for the image sent in a WebAPK proto. An image can have multiple usages, e.g. "primary icon" and "badge icon". At most two images can have non-empty "usages" fields: the image most suitable for primary icon, and the image most suitable for badge icon. "usages" field and manifest.icon.purpose as defined in the Web Manifest specification are two different fields. "usages" is used by Chrome to distinguish the image most suitable for primary icon and the image most suitable for badge icon from other images, and from each other. manifest.icon.purpose is used by Web developers to specify intended usage for the images. BUG=649771 ========== to ========== Add "usages" field to Image in WebApk.proto "usages" field is used by Chrome to specify intended usages for the image sent in a WebAPK proto. An image can have multiple usages, e.g. "primary icon" and "badge icon". At most two images can have non-empty "usages" fields: the image most suitable for primary icon, and the image most suitable for badge icon. "usages" field and manifest.icon.purpose defined in the Web Manifest specification are two different fields. - "usages" is used by Chrome to distinguish the image most suitable for primary icon and the image most suitable for badge icon from other images, and from each other. - manifest.icon.purpose is used by Web developers to specify intended usage for the images. BUG=649771 ==========
LGTM. I have added bullet points to your CL description to make it clearer :)
Description was changed from ========== Add "usages" field to Image in WebApk.proto "usages" field is used by Chrome to specify intended usages for the image sent in a WebAPK proto. An image can have multiple usages, e.g. "primary icon" and "badge icon". At most two images can have non-empty "usages" fields: the image most suitable for primary icon, and the image most suitable for badge icon. "usages" field and manifest.icon.purpose defined in the Web Manifest specification are two different fields. - "usages" is used by Chrome to distinguish the image most suitable for primary icon and the image most suitable for badge icon from other images, and from each other. - manifest.icon.purpose is used by Web developers to specify intended usage for the images. BUG=649771 ========== to ========== Add "usages" field to Image in WebApk.proto "usages" field is used by Chrome to specify intended usages for the image sent in a WebAPK proto. An image can have multiple usages, e.g. "primary icon" and "badge icon". At most two images can have non-empty "usages" fields: the image most suitable for primary icon, and the image most suitable for badge icon. The "usages" field in the proto and manifest.icon.purpose defined in the W3C Web Manifest specification are different. - "usages" is used by Chrome to distinguish the image most suitable for primary icon and the image most suitable for badge icon from other images, and from each other. - manifest.icon.purpose is used by Web developers to specify intended usage for the images. BUG=649771 ==========
rs lgtm
The CQ bit was checked by zpeng@chromium.org
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": 60001, "attempt_start_ts": 1486636490251400,
"parent_rev": "da4ec400317c86340cedfd5257cb722ff38d7c54", "commit_rev":
"fb90fb92c0ce0e549112e9dfbeaeb2309aad883d"}
Message was sent while issue was closed.
Description was changed from ========== Add "usages" field to Image in WebApk.proto "usages" field is used by Chrome to specify intended usages for the image sent in a WebAPK proto. An image can have multiple usages, e.g. "primary icon" and "badge icon". At most two images can have non-empty "usages" fields: the image most suitable for primary icon, and the image most suitable for badge icon. The "usages" field in the proto and manifest.icon.purpose defined in the W3C Web Manifest specification are different. - "usages" is used by Chrome to distinguish the image most suitable for primary icon and the image most suitable for badge icon from other images, and from each other. - manifest.icon.purpose is used by Web developers to specify intended usage for the images. BUG=649771 ========== to ========== Add "usages" field to Image in WebApk.proto "usages" field is used by Chrome to specify intended usages for the image sent in a WebAPK proto. An image can have multiple usages, e.g. "primary icon" and "badge icon". At most two images can have non-empty "usages" fields: the image most suitable for primary icon, and the image most suitable for badge icon. The "usages" field in the proto and manifest.icon.purpose defined in the W3C Web Manifest specification are different. - "usages" is used by Chrome to distinguish the image most suitable for primary icon and the image most suitable for badge icon from other images, and from each other. - manifest.icon.purpose is used by Web developers to specify intended usage for the images. BUG=649771 Review-Url: https://codereview.chromium.org/2686543002 Cr-Commit-Position: refs/heads/master@{#449255} Committed: https://chromium.googlesource.com/chromium/src/+/fb90fb92c0ce0e549112e9dfbeae... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001) as https://chromium.googlesource.com/chromium/src/+/fb90fb92c0ce0e549112e9dfbeae... |
