Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2682653005:
Settings: Add a new Enable Protected Content checkbox on all platforms (Closed)
DescriptionSettings: Add a new Enable Protected Content checkbox on all platforms
This CL adds a new checkbox for the new WebKit preference to enable
HTML5 Encrypted Media / DRM.
BUG=690077
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2682653005
Cr-Commit-Position: refs/heads/master@{#449105}
Committed: https://chromium.googlesource.com/chromium/src/+/239f8c5202eb155ff3bc22dbc1db77c0753497fa
Patch Set 1 #
Total comments: 3
Patch Set 2 : fix up #
Total comments: 3
Depends on Patchset: Messages
Total messages: 26 (13 generated)
Description was changed from ========== Settings: Add a new Enable Protected Content checkbox on all platforms This CL adds a new checkbox for the new WebKit preference to disable HTML5 Encrypted Media / DRM. BUG=690077 ========== to ========== Settings: Add a new Enable Protected Content checkbox on all platforms This CL adds a new checkbox for the new WebKit preference to disable HTML5 Encrypted Media / DRM. BUG=690077 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by tommycli@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...
xhwang@chromium.org changed reviewers: + xhwang@chromium.org
I tested this CL on Linux on top of https://chromiumcodereview.appspot.com/2676633006/, it's working pretty well. Here are cases I tested: - Open Chrome, got to about://settings/content. The new checkbox is checked. - In another tab, go to shaka-player-demo.appspot.com. Play a widevine movie and it is working. - In about://settings/content, disable protected content. - In shaka-player-demo.appspot.com, click load again and playback failed. - In about://settings/content, enable protected content. - In shaka-player-demo.appspot.com, click load again and playback is working. - In about://settings/content, disable protected content. - Close Chrome. - Open Chrome again using the same --user-data-dir. - In about://settings/content, make sure protected content is still disabled. - In about://settings/content, enable protected content. - Close Chrome. - Open Chrome again using the same --user-data-dir. - In about://settings/content, make sure protected content is still enabled. https://codereview.chromium.org/2682653005/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2682653005/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:8304: + Protected content Not for this CL. I am not sure whether we should use "Protected content" or "Protected Content". It seems we have examples for either case today: - Unsandboxed plugin access - Automatic Downloads - MIDI devices full control - USB Devices - Background sync - Zoom levels - PDF Documents
FWIW, toggling the setting won't affect currently playing protected content. But new playback attempt will fail.
tommycli@chromium.org changed reviewers: + stevenjb@chromium.org
stevenjb: PTAL. This CL obsoletes https://codereview.chromium.org/2679723003/ due to design changes. I have tested these changes manually (as well as xhwang). Will send screenshots soon.
On 2017/02/08 20:03:13, tommycli wrote: > stevenjb: PTAL. This CL obsoletes https://codereview.chromium.org/2679723003/ > due to design changes. > > I have tested these changes manually (as well as xhwang). Will send screenshots > soon. Linux and CrOS screenshots respectively: http://imgur.com/a/9IudG
Description was changed from ========== Settings: Add a new Enable Protected Content checkbox on all platforms This CL adds a new checkbox for the new WebKit preference to disable HTML5 Encrypted Media / DRM. BUG=690077 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Settings: Add a new Enable Protected Content checkbox on all platforms This CL adds a new checkbox for the new WebKit preference to enable HTML5 Encrypted Media / DRM. BUG=690077 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
lgtm w/ suggestion https://codereview.chromium.org/2682653005/diff/1/chrome/browser/resources/op... File chrome/browser/resources/options/content_settings.html (right): https://codereview.chromium.org/2682653005/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/content_settings.html:337: </if> I think it would be more clear if this </if> was above line 330 (<if expr="chromeos">) and that <if> was unnested / unindented.
done thanks for suggestion https://codereview.chromium.org/2682653005/diff/1/chrome/browser/resources/op... File chrome/browser/resources/options/content_settings.html (right): https://codereview.chromium.org/2682653005/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/content_settings.html:337: </if> On 2017/02/08 20:24:36, stevenjb wrote: > I think it would be more clear if this </if> was above line 330 (<if > expr="chromeos">) and that <if> was unnested / unindented. Done.
The CQ bit was checked by tommycli@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...
On 2017/02/08 20:54:18, tommycli wrote: > done thanks for suggestion > > https://codereview.chromium.org/2682653005/diff/1/chrome/browser/resources/op... > File chrome/browser/resources/options/content_settings.html (right): > > https://codereview.chromium.org/2682653005/diff/1/chrome/browser/resources/op... > chrome/browser/resources/options/content_settings.html:337: </if> > On 2017/02/08 20:24:36, stevenjb wrote: > > I think it would be more clear if this </if> was above line 330 (<if > > expr="chromeos">) and that <if> was unnested / unindented. > > Done. Windows screenshot btw: http://imgur.com/a/Y2ZB0 I'm sending this in. I tested the functionality on Windows too. Seems to work.
The CQ bit was unchecked by tommycli@chromium.org
The CQ bit was checked by tommycli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2682653005/#ps20001 (title: "fix up")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486591120721410,
"parent_rev": "cd84e7a30b0a3318bbe93c4653c0f57bb950b075", "commit_rev":
"239f8c5202eb155ff3bc22dbc1db77c0753497fa"}
Message was sent while issue was closed.
Description was changed from ========== Settings: Add a new Enable Protected Content checkbox on all platforms This CL adds a new checkbox for the new WebKit preference to enable HTML5 Encrypted Media / DRM. BUG=690077 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Settings: Add a new Enable Protected Content checkbox on all platforms This CL adds a new checkbox for the new WebKit preference to enable HTML5 Encrypted Media / DRM. BUG=690077 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2682653005 Cr-Commit-Position: refs/heads/master@{#449105} Committed: https://chromium.googlesource.com/chromium/src/+/239f8c5202eb155ff3bc22dbc1db... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/239f8c5202eb155ff3bc22dbc1db...
Message was sent while issue was closed.
https://codereview.chromium.org/2682653005/diff/20001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2682653005/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:8313: + <message name="IDS_PROTECTED_CONTENT_ENABLE_IDENTIFIERS_CHECKBOX" desc="The label of the checkbox for enabling machine identifiers to uniquely identiy the user for protected content."> I am not sure whether IDS tag renaming would affect the translation process. One option is that when we merge this change back to M57, we keep the old name here. WDYT?
Message was sent while issue was closed.
thanks https://codereview.chromium.org/2682653005/diff/20001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2682653005/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:8313: + <message name="IDS_PROTECTED_CONTENT_ENABLE_IDENTIFIERS_CHECKBOX" desc="The label of the checkbox for enabling machine identifiers to uniquely identiy the user for protected content."> On 2017/02/09 17:43:07, xhwang_slow wrote: > I am not sure whether IDS tag renaming would affect the translation process. One > option is that when we merge this change back to M57, we keep the old name here. > WDYT? Hi! I talked to laforge and he said that the hash of the string mattered - not the IDS tag name, and not the description! (Which surprised me)
Message was sent while issue was closed.
https://codereview.chromium.org/2682653005/diff/20001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2682653005/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:8313: + <message name="IDS_PROTECTED_CONTENT_ENABLE_IDENTIFIERS_CHECKBOX" desc="The label of the checkbox for enabling machine identifiers to uniquely identiy the user for protected content."> On 2017/02/09 17:49:13, tommycli wrote: > On 2017/02/09 17:43:07, xhwang_slow wrote: > > I am not sure whether IDS tag renaming would affect the translation process. > One > > option is that when we merge this change back to M57, we keep the old name > here. > > WDYT? > > Hi! I talked to laforge and he said that the hash of the string mattered - not > the IDS tag name, and not the description! (Which surprised me) Awesome! Thanks for confirmation! I learned something today ;) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
