Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2919853002:
[MD settings] site exceptions, use embedding origin rather than embeddingDisplayName (Closed)
|
Created:
3 years, 6 months ago by dschuyler Modified:
3 years, 5 months ago Reviewers:
msramek CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org, Patti Lor, raymes Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[MD settings] site exceptions, use embedding origin rather than embeddingDisplayName
This CL removes the displayEmbeddingOrigin member and derives a
display name from the embeddingOrigin value instead.
BUG=728774
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2919853002
Cr-Commit-Position: refs/heads/master@{#483197}
Committed: https://chromium.googlesource.com/chromium/src/+/cb2ef31906237103697cb5ed600386e9b887f625
Patch Set 1 #Patch Set 2 : remove IDS reference #
Total comments: 9
Patch Set 3 : re-added embedded description #
Total comments: 4
Patch Set 4 : review changes #
Messages
Total messages: 29 (21 generated)
Description was changed from ========== [MD settings] site exceptions, use embedding origin rather than embeddingDisplayName This CL removes the displayEmbeddingOrigin member ans uses the embeddingOrigin value instead. BUG=728774 ========== to ========== [MD settings] site exceptions, use embedding origin rather than embeddingDisplayName This CL removes the displayEmbeddingOrigin member ans uses the embeddingOrigin value instead. BUG=728774 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dschuyler@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 checked by dschuyler@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...
dschuyler@chromium.org changed reviewers: + msramek@chromium.org
https://codereview.chromium.org/2919853002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc (left): https://codereview.chromium.org/2919853002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:1692: {"embeddedOnHost", IDS_EXCEPTIONS_GEOLOCATION_EMBEDDED_ON_HOST}, IDS_EXCEPTIONS_GEOLOCATION_EMBEDDED_ON_HOST is still used by the old options (the same string was being used/mapped in both options and settings). So I'm only removing the settings version here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2919853002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_list.js (right): https://codereview.chromium.org/2919853002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_list.js:384: * Returns the appropriate site description to display. This can, for example, Please update the jsdoc to reflect the change. https://codereview.chromium.org/2919853002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_list.js:390: computeSiteDescription_: function(item) { So, this will change the current strings """ google.com embedded on example.com """ into """ google.com example.com """ and """ google.com Current Incognito session (embedded on example.com) """ into """ google.com Current Incognito session (example.com) """ correct? I'm not sure I understand the rationale. It is quite clear to me that a pair of exceptions means requesting and embedding origin, but I'm familiar with this UI. I would still spell it out for the user, especially since we're already verbose with the Incognito part anyway. https://codereview.chromium.org/2919853002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc (left): https://codereview.chromium.org/2919853002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:1689: {"cookiePlural", IDS_SETTINGS_COOKIES_PLURAL_COOKIES}, Not related to this patchset, but this is wrong - it should use an ICU string. There are languages that have dual number (Arabic, Slovenian), or a different case for "few" (2,3,4 - most Slavic languages). https://codereview.chromium.org/2919853002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:1692: {"embeddedOnHost", IDS_EXCEPTIONS_GEOLOCATION_EMBEDDED_ON_HOST}, On 2017/06/01 22:07:12, dschuyler wrote: > IDS_EXCEPTIONS_GEOLOCATION_EMBEDDED_ON_HOST is still used by the old options > (the same string was being used/mapped in both options and settings). So I'm > only removing the settings version here. Acknowledged.
The CQ bit was checked by dschuyler@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.
Description was changed from ========== [MD settings] site exceptions, use embedding origin rather than embeddingDisplayName This CL removes the displayEmbeddingOrigin member ans uses the embeddingOrigin value instead. BUG=728774 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] site exceptions, use embedding origin rather than embeddingDisplayName This CL removes the displayEmbeddingOrigin member and derives a display name from the embeddingOrigin value instead. BUG=728774 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2919853002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_list.js (right): https://codereview.chromium.org/2919853002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_list.js:384: * Returns the appropriate site description to display. This can, for example, On 2017/06/01 23:09:59, msramek (slow) wrote: > Please update the jsdoc to reflect the change. I've changed the code to match the prior output more closely. https://codereview.chromium.org/2919853002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_list.js:390: computeSiteDescription_: function(item) { On 2017/06/01 23:09:59, msramek (slow) wrote: > So, this will change the current strings > > """ > http://google.com > embedded on http://example.com > """ > > into > > """ > http://google.com > http://example.com > """ > > and > > """ > http://google.com > Current Incognito session (embedded on http://example.com) > """ > > into > > """ > http://google.com > Current Incognito session (http://example.com) > """ > > correct? I'm not sure I understand the rationale. It is quite clear to me that a > pair of exceptions means requesting and embedding origin, but I'm familiar with > this UI. I would still spell it out for the user, especially since we're already > verbose with the Incognito part anyway. Okay, thanks! I've changed the code to match the prior output more closely. https://codereview.chromium.org/2919853002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc (left): https://codereview.chromium.org/2919853002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:1689: {"cookiePlural", IDS_SETTINGS_COOKIES_PLURAL_COOKIES}, On 2017/06/01 23:09:59, msramek (slow) wrote: > Not related to this patchset, but this is wrong - it should use an ICU string. > > There are languages that have dual number (Arabic, Slovenian), or a different > case for "few" (2,3,4 - most Slavic languages). Added Issue 732610 to track this.
On 2017/06/13 20:49:25, dschuyler wrote: > https://codereview.chromium.org/2919853002/diff/20001/chrome/browser/resource... > File chrome/browser/resources/settings/site_settings/site_list.js (right): > > https://codereview.chromium.org/2919853002/diff/20001/chrome/browser/resource... > chrome/browser/resources/settings/site_settings/site_list.js:384: * Returns the > appropriate site description to display. This can, for example, > On 2017/06/01 23:09:59, msramek (slow) wrote: > > Please update the jsdoc to reflect the change. > > I've changed the code to match the prior output more closely. > > https://codereview.chromium.org/2919853002/diff/20001/chrome/browser/resource... > chrome/browser/resources/settings/site_settings/site_list.js:390: > computeSiteDescription_: function(item) { > On 2017/06/01 23:09:59, msramek (slow) wrote: > > So, this will change the current strings > > > > """ > > http://google.com > > embedded on http://example.com > > """ > > > > into > > > > """ > > http://google.com > > http://example.com > > """ > > > > and > > > > """ > > http://google.com > > Current Incognito session (embedded on http://example.com) > > """ > > > > into > > > > """ > > http://google.com > > Current Incognito session (http://example.com) > > """ > > > > correct? I'm not sure I understand the rationale. It is quite clear to me that > a > > pair of exceptions means requesting and embedding origin, but I'm familiar > with > > this UI. I would still spell it out for the user, especially since we're > already > > verbose with the Incognito part anyway. > > Okay, thanks! I've changed the code to match the prior output more closely. > > https://codereview.chromium.org/2919853002/diff/20001/chrome/browser/ui/webui... > File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc > (left): > > https://codereview.chromium.org/2919853002/diff/20001/chrome/browser/ui/webui... > chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:1689: > {"cookiePlural", IDS_SETTINGS_COOKIES_PLURAL_COOKIES}, > On 2017/06/01 23:09:59, msramek (slow) wrote: > > Not related to this patchset, but this is wrong - it should use an ICU string. > > > > There are languages that have dual number (Arabic, Slovenian), or a different > > case for "few" (2,3,4 - most Slavic languages). > > Added Issue 732610 to track this. A friendly ping
Apologies for the unreasonable delay, and please do feel free to ping me more aggressively next time. I got a bit stuck thinking if I sent you in the right direction in this CL, and never had time to finish that thought. Specifically, I'm convinced that saying "embedded on" is better than no string, but I'm not sure if it's precise in all cases. Technically, nothing mandates that the primary pattern and secondary pattern are the requesting and embedding origin. And in fact, it wasn't always that way (see https://docs.google.com/spreadsheets/d/1Y0gFR5evM410O-CjuG3h-3aW3PQ9XiUE4-zYF... ). However, given that we do not support manually adding double-keyed exceptions, the only thing to add a double-keyed exception is through a permission prompt, and in those cases primary and secondary pattern should always have their standard meaning. +cc raymes@ for sanity check Otherwise, LGTM. https://codereview.chromium.org/2919853002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc (left): https://codereview.chromium.org/2919853002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:1689: {"cookiePlural", IDS_SETTINGS_COOKIES_PLURAL_COOKIES}, On 2017/06/13 20:49:25, dschuyler wrote: > On 2017/06/01 23:09:59, msramek (slow) wrote: > > Not related to this patchset, but this is wrong - it should use an ICU string. > > > > There are languages that have dual number (Arabic, Slovenian), or a different > > case for "few" (2,3,4 - most Slavic languages). > > Added Issue 732610 to track this. Thank you! https://codereview.chromium.org/2919853002/diff/40001/chrome/app/settings_str... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2919853002/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:1934: <message name="IDS_SETTINGS_EXCEPTIONS_GEOLOCATION_EMBEDDED_ON_HOST" desc="Template text for a child row in the Geolocation Exceptions page view. Controls the permission setting for the parent page when embedded on the specified site."> nit: This is no longer related just to geolocation. https://codereview.chromium.org/2919853002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_list.js (right): https://codereview.chromium.org/2919853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_list.js:396: displayName = loadTimeData.getStringF('embeddedOnHost', '*'); Optional: Should we have a custom string saying "embedded on any host"?
The CQ bit was checked by dschuyler@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...
https://codereview.chromium.org/2919853002/diff/40001/chrome/app/settings_str... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2919853002/diff/40001/chrome/app/settings_str... chrome/app/settings_strings.grdp:1934: <message name="IDS_SETTINGS_EXCEPTIONS_GEOLOCATION_EMBEDDED_ON_HOST" desc="Template text for a child row in the Geolocation Exceptions page view. Controls the permission setting for the parent page when embedded on the specified site."> On 2017/06/28 19:41:33, msramek (slow) wrote: > nit: This is no longer related just to geolocation. Done. https://codereview.chromium.org/2919853002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/site_settings/site_list.js (right): https://codereview.chromium.org/2919853002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/site_settings/site_list.js:396: displayName = loadTimeData.getStringF('embeddedOnHost', '*'); On 2017/06/28 19:41:33, msramek (slow) wrote: > Optional: Should we have a custom string saying "embedded on any host"? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dschuyler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org Link to the patchset: https://codereview.chromium.org/2919853002/#ps60001 (title: "review changes")
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": 1498692508420950,
"parent_rev": "dacc85d6208232ec0b2af40786ceec71559fa39c", "commit_rev":
"cb2ef31906237103697cb5ed600386e9b887f625"}
Message was sent while issue was closed.
Description was changed from ========== [MD settings] site exceptions, use embedding origin rather than embeddingDisplayName This CL removes the displayEmbeddingOrigin member and derives a display name from the embeddingOrigin value instead. BUG=728774 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [MD settings] site exceptions, use embedding origin rather than embeddingDisplayName This CL removes the displayEmbeddingOrigin member and derives a display name from the embeddingOrigin value instead. BUG=728774 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2919853002 Cr-Commit-Position: refs/heads/master@{#483197} Committed: https://chromium.googlesource.com/chromium/src/+/cb2ef31906237103697cb5ed6003... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/cb2ef31906237103697cb5ed6003... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
