|
|
Descriptionmedia: Add string for enabling protected content
This CL only adds the string to enable protected content. The string is copied from https://chromiumcodereview.appspot.com/2679723003/ with modifications. Landing it first so it could be localized. The CL that uses it will come shortly.
NOTRY=true
NOPRESUBMIT=true
TBR=stevenjb@chromium.org
BUG=686430
Review-Url: https://codereview.chromium.org/2688433002
Cr-Commit-Position: refs/heads/master@{#448851}
Committed: https://chromium.googlesource.com/chromium/src/+/af445fb0a75d6811c0a8991becd79986e996d647
Patch Set 1 #
Total comments: 2
Patch Set 2 : rebase #
Messages
Total messages: 19 (12 generated)
Description was changed from ========== media: Add string for enabling protected content This CL only adds the string so it could be localized. The CL that uses it will come shortly. BUG=686430 ========== to ========== media: Add string for enabling protected content This CL only adds the string to enable protected content. The string is copied from https://chromiumcodereview.appspot.com/2679723003/ with modifications. Landing it first so it could be localized. The CL that uses it will come shortly. BUG=686430 ==========
xhwang@chromium.org changed reviewers: + srahim@chromium.org, stevenjb@chromium.org, tommycli@chromium.org
The CQ bit was checked by xhwang@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...
ddorwin@chromium.org changed reviewers: + ddorwin@chromium.org
lgtm
https://codereview.chromium.org/2688433002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2688433002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:8317: + <message name="IDS_PROTECTED_CONTENT_ENABLE_CHECKBOX" desc="The label of the checkbox for enabling protected content."> Hey... I have a few reservations about this: We have both this new string (IDS_PROTECTED_CONTENT_ENABLE_CHECKBOX) and the old string (IDS_PROTECTED_CONTENT_ENABLE)... which could be a bit confusing. xhwang (over IM) said that in the long term, we should rename the old one to say iDS_PROTECTED_CONTENT_ENABLE_UNIQUE_IDENTIFIERS. How do the IDS tag names relate to the translations? If we change the IDS tag names later, do we have to re-translate it? Put another way: if we commit a subpar set of IDS tag names now, are we stuck with them? I also want to get this into 57 also, but if there's last minute design changes plus deadline pressure, I don't want to be on the path to bad code decisions. If you deem necessary, feel free to land this patch anyways, and we can revisit tomorrow.
On 2017/02/08 01:35:05, tommycli wrote: > https://codereview.chromium.org/2688433002/diff/1/chrome/app/generated_resour... > File chrome/app/generated_resources.grd (right): > > https://codereview.chromium.org/2688433002/diff/1/chrome/app/generated_resour... > chrome/app/generated_resources.grd:8317: + <message > name="IDS_PROTECTED_CONTENT_ENABLE_CHECKBOX" desc="The label of the checkbox for > enabling protected content."> > Hey... I have a few reservations about this: > > We have both this new string (IDS_PROTECTED_CONTENT_ENABLE_CHECKBOX) and the old > string (IDS_PROTECTED_CONTENT_ENABLE)... which could be a bit confusing. xhwang > (over IM) said that in the long term, we should rename the old one to say > iDS_PROTECTED_CONTENT_ENABLE_UNIQUE_IDENTIFIERS. > > How do the IDS tag names relate to the translations? If we change the IDS tag > names later, do we have to re-translate it? Put another way: if we commit a > subpar set of IDS tag names now, are we stuck with them? > > I also want to get this into 57 also, but if there's last minute design changes > plus deadline pressure, I don't want to be on the path to bad code decisions. > > If you deem necessary, feel free to land this patch anyways, and we can revisit > tomorrow. Thanks for the comments! I'll go ahead and land this. I'll find answers to your question about IDS tag names tomorrow.
https://codereview.chromium.org/2688433002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2688433002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:8317: + <message name="IDS_PROTECTED_CONTENT_ENABLE_CHECKBOX" desc="The label of the checkbox for enabling protected content."> On 2017/02/08 01:35:05, tommycli wrote: > Hey... I have a few reservations about this: > > We have both this new string (IDS_PROTECTED_CONTENT_ENABLE_CHECKBOX) and the old > string (IDS_PROTECTED_CONTENT_ENABLE)... which could be a bit confusing. xhwang > (over IM) said that in the long term, we should rename the old one to say > iDS_PROTECTED_CONTENT_ENABLE_UNIQUE_IDENTIFIERS. > > How do the IDS tag names relate to the translations? If we change the IDS tag > names later, do we have to re-translate it? Put another way: if we commit a > subpar set of IDS tag names now, are we stuck with them? > > I also want to get this into 57 also, but if there's last minute design changes > plus deadline pressure, I don't want to be on the path to bad code decisions. > > If you deem necessary, feel free to land this patch anyways, and we can revisit > tomorrow. > To be a bit more solution oriented: If we can re-name the IDS tags now to their long-term best name, I won't have any reservations about this patch, and will gladly stamp.
The CQ bit was unchecked by xhwang@chromium.org
The CQ bit was checked by xhwang@chromium.org
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 xhwang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org Link to the patchset: https://codereview.chromium.org/2688433002/#ps20001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== media: Add string for enabling protected content This CL only adds the string to enable protected content. The string is copied from https://chromiumcodereview.appspot.com/2679723003/ with modifications. Landing it first so it could be localized. The CL that uses it will come shortly. BUG=686430 ========== to ========== media: Add string for enabling protected content This CL only adds the string to enable protected content. The string is copied from https://chromiumcodereview.appspot.com/2679723003/ with modifications. Landing it first so it could be localized. The CL that uses it will come shortly. NOTRY=true NOPRESUBMIT=true TBR=stevenjb@chromium.org BUG=686430 ==========
CQ is committing da patch. Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486518341697990, "parent_rev": "40041448d2a919edd951ca49f414acba0084fe4e", "commit_rev": "af445fb0a75d6811c0a8991becd79986e996d647"}
Message was sent while issue was closed.
Description was changed from ========== media: Add string for enabling protected content This CL only adds the string to enable protected content. The string is copied from https://chromiumcodereview.appspot.com/2679723003/ with modifications. Landing it first so it could be localized. The CL that uses it will come shortly. NOTRY=true NOPRESUBMIT=true TBR=stevenjb@chromium.org BUG=686430 ========== to ========== media: Add string for enabling protected content This CL only adds the string to enable protected content. The string is copied from https://chromiumcodereview.appspot.com/2679723003/ with modifications. Landing it first so it could be localized. The CL that uses it will come shortly. NOTRY=true NOPRESUBMIT=true TBR=stevenjb@chromium.org BUG=686430 Review-Url: https://codereview.chromium.org/2688433002 Cr-Commit-Position: refs/heads/master@{#448851} Committed: https://chromium.googlesource.com/chromium/src/+/af445fb0a75d6811c0a8991becd7... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/af445fb0a75d6811c0a8991becd7... |