UI Changes to support clearing EME/CDM data

UI Changes to support clearing EME/CDM data

Update the "Clear Browsing Data" dialogs to clear the EME/CDM data
when "Content Licenses" is selected. Currently selecting "Content
Licenses" only clears Flash data, if Flash is enabled.

Added new preference kDeleteMediaLicenses which replaces
kDeauthorizeContentLicenses, which was Flash specific. Default
value for kDeleteMediaLicenses is false, so the checkbox will
not be selected initially.

EME/CDM data is stored in the plugin private file system. If there
is no data saved for EME, then deletion is a nop. Actually deleting
the data was handled in https://codereview.chromium.org/1979733002/.

Changes to chrome://settings/clearBrowserData:
1) "Content licenses" is renamed to "Media Licenses".
2) "Content licenses" check box will always be displayed. (Currently
   it is only displayed if Flash is enabled.)
3) When selected, it will display an additional message as noted in
   the UI design doc (e.g. "You may lose access to premium content
   from some sites" or "You may lose access to premium content from
   www.youtube.com and some other sites.").

Changes to chrome://md-settings/advanced -> Clear Browsing Data
1) "Content licenses" is renamed to "Media Licenses".
2) When selected, it will display an additional message as noted
   above.

BUG=418195
TEST=tested "Clear Browsing Data" dialogs manually, new tests pass
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/bffd01243bee7c038ef6c4a92814c77176d9ebdd
Cr-Commit-Position: refs/heads/master@{#403029}

[jrummell, 2016-06-17 00:01:47]

Description was changed from

==========
UI Changes to support clearing EME/CDM data

Update the "Clear Browsing Data" dialogs to clear the EME/CDM data
when "Content Licenses" is selected. Currently selecting "Content
Licenses" only clears Flash data, if Flash is enabled.

Added new preference kDeleteMediaLicenses which replaces
kDeauthorizeContentLicenses, which was Flash specific. Default
value for kDeleteMediaLicenses is false, so the checkbox will
not be selected initially.

EME/CDM data is stored in the plugin private file system. If there
is no data saved for EME, then deletion is a nop. Actually deleting
the data was handled in https://codereview.chromium.org/1979733002/.

Changes to chrome://settings/clearBrowserData:
1) "Content licenses" check box will always be displayed. (Currently
   it is only displayed if Flash is enabled.)
2) When selected, it will display an additional message as noted in
   the UI design doc (e.g. "You may lose access to premium content
   from some sites" or "You may lose access to premium content from
   www.youtube.com and some other sites.").

Changes to chrome://md-settings/advanced -> Clear Browsing Data
1) "Content licenses" is renamed to "Media Licenses"

BUG=418195
TEST=tested "Clear Browsing Data" dialogs manually, new tests pass
==========

to

==========
UI Changes to support clearing EME/CDM data

Update the "Clear Browsing Data" dialogs to clear the EME/CDM data
when "Content Licenses" is selected. Currently selecting "Content
Licenses" only clears Flash data, if Flash is enabled.

Added new preference kDeleteMediaLicenses which replaces
kDeauthorizeContentLicenses, which was Flash specific. Default
value for kDeleteMediaLicenses is false, so the checkbox will
not be selected initially.

EME/CDM data is stored in the plugin private file system. If there
is no data saved for EME, then deletion is a nop. Actually deleting
the data was handled in https://codereview.chromium.org/1979733002/.

Changes to chrome://settings/clearBrowserData:
1) "Content licenses" check box will always be displayed. (Currently
   it is only displayed if Flash is enabled.)
2) When selected, it will display an additional message as noted in
   the UI design doc (e.g. "You may lose access to premium content
   from some sites" or "You may lose access to premium content from
   www.youtube.com and some other sites.").

Changes to chrome://md-settings/advanced -> Clear Browsing Data
1) "Content licenses" is renamed to "Media Licenses"

BUG=418195
TEST=tested "Clear Browsing Data" dialogs manually, new tests pass
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[jrummell, 2016-06-17 00:04:08]

jrummell@chromium.org changed reviewers:
+ xhwang@chromium.org

[jrummell, 2016-06-17 00:04:09]

PTAL.

[xhwang, 2016-06-17 06:24:30]

Looking pretty good. I only have a few nits.

Please add a browsing_data owner, and a Flash owner since this affects flash.

https://codereview.chromium.org/2075023002/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2075023002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:7865: +      <message
name="IDS_DEL_CONTENT_LICENSES_GENERAL_COMMENT" desc="A static message shown in
the Clear Browsing Data dialog explaining to the user that clearing content
licenses will result in the user being unable to play some premium content">
s/content licenses/media licenses/?

https://codereview.chromium.org/2075023002/diff/1/chrome/app/settings_strings...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2075023002/diff/1/chrome/app/settings_strings...
chrome/app/settings_strings.grdp:582: <message
name="IDS_SETTINGS_CLEAR_MEDIA_LICENSES" desc="Checkbox for deleting media
licenses (DRM videos, etc.)">
How about s/DRM videos/premium contents to be consistent?

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.cc:911: if (remove_mask &
REMOVE_CONTENT_LICENSES) {
Should this be renamed to REMOVE_MEDIA_LICENSES? (can be in a separate CL)

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.cc:957:
UserMetricsAction("ClearBrowsingData_ContentLicenses"));
This should probably be renamed as well. Can you add a todo?

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.cc:964:
deauthorize_content_licenses_request_id_ =
deauthorize_flash_content_licenses_request_id_

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.cc:1234: void
BrowsingDataRemover::OnDeauthorizeContentLicensesCompleted(
OnDeauthorizeFlashContentLicensesCompleted

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/media_license_counter.cc (right):

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/media_license_counter.cc:16: using
OriginsDeterminedCB = base::Callback<void(std::set<GURL>)>;
pass std::set by const-ref

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/media_license_counter.cc:20: void
CountOriginsOnFileTaskRunner(
put this in an anonymous namespace?

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/media_license_counter.cc:22: OriginsDeterminedCB
callback) {
pass callback by const-ref

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/media_license_counter.h (right):

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/media_license_counter.h:5: #ifndef
CHROME_BROWSER_BROWSING_DATA_MEDIA_LICENSE_COUNTER_H_
This file should be media_license*s*_counter.h to be consistent with the UI and
pref.

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/media_license_counter.h:27: std::set<GURL>
origins);
pass origins by const-ref to avoid copy?

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/media_license_counter.h:49: void
OnContentLicensesObtained(std::set<GURL> origins);
pass by const ref

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/media_license_counter_browsertest.cc (right):

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/media_license_counter_browsertest.cc:31: class
AwaitCompletionHelper {
Add a TODO to avoid duplicating this class?

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/media_license_counter_browsertest.cc:87: void
WaitForIOThread() {
This is copied from cache_counter_browsertest.cc but I feel the name is
confusing. IO thread has a special meaning that it's the thread dispatching IPC
messages. 

This is more like just Run(), or RunAndWaitForResult().

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/clear_browser_data_overlay.html (right):

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/clear_browser_data_overlay.html:79: <div
id="deauthorize-content-licenses-container"
should this whole block be renamed to "media-licenses..."?

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/clear_browser_data_overlay.html:80:
class="checkbox pepper-flash-settings">
Is pepper-flash-settings removed from options_page.css? This should just be a
normal checkbox now.

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/clear_browser_data_overlay.html:84:
aria-controls="delete-content-licenses-counter">
ditto

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/ui/webui/set...
File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc
(right):

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:138:
prefs::kDeleteMediaLicenses,
Is this done by "git cl format"?

[jrummell, 2016-06-21 00:13:42]

jrummell@chromium.org changed reviewers:
+ ihf@google.com, michaeln@chromium.org, pkasting@google.com

[jrummell, 2016-06-21 00:13:44]

+michaeln@ for chrome/browser/browsing_data
+pkasting@ for chrome/browser/ui
+ihf@ for comments on Flash changes

https://codereview.chromium.org/2075023002/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2075023002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:7865: +      <message
name="IDS_DEL_CONTENT_LICENSES_GENERAL_COMMENT" desc="A static message shown in
the Clear Browsing Data dialog explaining to the user that clearing content
licenses will result in the user being unable to play some premium content">
On 2016/06/17 06:24:29, xhwang wrote:
> s/content licenses/media licenses/?

Done.

https://codereview.chromium.org/2075023002/diff/1/chrome/app/settings_strings...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2075023002/diff/1/chrome/app/settings_strings...
chrome/app/settings_strings.grdp:582: <message
name="IDS_SETTINGS_CLEAR_MEDIA_LICENSES" desc="Checkbox for deleting media
licenses (DRM videos, etc.)">
On 2016/06/17 06:24:29, xhwang wrote:
> How about s/DRM videos/premium contents to be consistent?

Done.

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.cc:911: if (remove_mask &
REMOVE_CONTENT_LICENSES) {
On 2016/06/17 06:24:29, xhwang wrote:
> Should this be renamed to REMOVE_MEDIA_LICENSES? (can be in a separate CL)

Done.

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.cc:957:
UserMetricsAction("ClearBrowsingData_ContentLicenses"));
On 2016/06/17 06:24:29, xhwang wrote:
> This should probably be renamed as well. Can you add a todo?

Done.

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.cc:964:
deauthorize_content_licenses_request_id_ =
On 2016/06/17 06:24:29, xhwang wrote:
> deauthorize_flash_content_licenses_request_id_

Done.

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover.cc:1234: void
BrowsingDataRemover::OnDeauthorizeContentLicensesCompleted(
On 2016/06/17 06:24:29, xhwang wrote:
> OnDeauthorizeFlashContentLicensesCompleted

Done.

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/media_license_counter.cc (right):

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/media_license_counter.cc:16: using
OriginsDeterminedCB = base::Callback<void(std::set<GURL>)>;
On 2016/06/17 06:24:29, xhwang wrote:
> pass std::set by const-ref

Done.

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/media_license_counter.cc:20: void
CountOriginsOnFileTaskRunner(
On 2016/06/17 06:24:29, xhwang wrote:
> put this in an anonymous namespace?

Done.

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/media_license_counter.cc:22: OriginsDeterminedCB
callback) {
On 2016/06/17 06:24:30, xhwang wrote:
> pass callback by const-ref

Done.

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/media_license_counter.h (right):

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/media_license_counter.h:5: #ifndef
CHROME_BROWSER_BROWSING_DATA_MEDIA_LICENSE_COUNTER_H_
On 2016/06/17 06:24:30, xhwang wrote:
> This file should be media_license*s*_counter.h to be consistent with the UI
and
> pref.

Hmmm. There is autofill_counter, cache_counter, but also hosted_apps_counter and
passwords_counter. Changed.

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/media_license_counter.h:27: std::set<GURL>
origins);
On 2016/06/17 06:24:30, xhwang wrote:
> pass origins by const-ref to avoid copy?

Done.

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/media_license_counter.h:49: void
OnContentLicensesObtained(std::set<GURL> origins);
On 2016/06/17 06:24:30, xhwang wrote:
> pass by const ref

Done.

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/media_license_counter_browsertest.cc (right):

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/media_license_counter_browsertest.cc:31: class
AwaitCompletionHelper {
On 2016/06/17 06:24:30, xhwang wrote:
> Add a TODO to avoid duplicating this class?

Removed it, as the places it is used is all asynchronous and can call
MessageLoop directly.

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/media_license_counter_browsertest.cc:87: void
WaitForIOThread() {
On 2016/06/17 06:24:30, xhwang wrote:
> This is copied from cache_counter_browsertest.cc but I feel the name is
> confusing. IO thread has a special meaning that it's the thread dispatching
IPC
> messages. 
> 
> This is more like just Run(), or RunAndWaitForResult().

Done (and simplified to use base::MessageLoop like the rest).

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/resources/op...
File chrome/browser/resources/options/clear_browser_data_overlay.html (right):

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/clear_browser_data_overlay.html:79: <div
id="deauthorize-content-licenses-container"
On 2016/06/17 06:24:30, xhwang wrote:
> should this whole block be renamed to "media-licenses..."?

Done.

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/clear_browser_data_overlay.html:80:
class="checkbox pepper-flash-settings">
On 2016/06/17 06:24:30, xhwang wrote:
> Is pepper-flash-settings removed from options_page.css? This should just be a
> normal checkbox now.

Done.

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/resources/op...
chrome/browser/resources/options/clear_browser_data_overlay.html:84:
aria-controls="delete-content-licenses-counter">
On 2016/06/17 06:24:30, xhwang wrote:
> ditto

Done.

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/ui/webui/set...
File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc
(right):

https://codereview.chromium.org/2075023002/diff/1/chrome/browser/ui/webui/set...
chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:138:
prefs::kDeleteMediaLicenses,
On 2016/06/17 06:24:30, xhwang wrote:
> Is this done by "git cl format"?

Yup.

[xhwang, 2016-06-21 06:02:41]

looking pretty good. I only have two more comments.

Next time it'd be nice to upload a renaming-only CL first, then upload changes.
That way it'll be much easier to track what's changed in the new files.

https://chromiumcodereview.appspot.com/2075023002/diff/20001/chrome/browser/b...
File chrome/browser/browsing_data/media_licenses_counter_browsertest.cc (right):

https://chromiumcodereview.appspot.com/2075023002/diff/20001/chrome/browser/b...
chrome/browser/browsing_data/media_licenses_counter_browsertest.cc:54:
base::MessageLoop::current()->Run();
MessageLoop::Run() is being deprecated. The new way to do this is
RunLoop().Run().

See
https://cs.chromium.org/chromium/src/base/message_loop/message_loop.h?rcl=0&l...

See also https://codereview.chromium.org/2053123002/

https://chromiumcodereview.appspot.com/2075023002/diff/20001/chrome/browser/u...
File chrome/browser/ui/webui/options/clear_browser_data_handler.cc (right):

https://chromiumcodereview.appspot.com/2075023002/diff/20001/chrome/browser/u...
chrome/browser/ui/webui/options/clear_browser_data_handler.cc:177:
{"deleteMediaLicensesCheckbox", IDS_DEAUTHORIZE_CONTENT_LICENSES_CHKBOX},
IDS_DEAUTHORIZE_CONTENT_LICENSES_CHKBOX should be renamed and its description
should be updated:

https://cs.chromium.org/chromium/src/chrome/app/generated_resources.grd?rcl=0...

[michaeln, 2016-06-21 20:23:42]

I might not the best reviewer for the browsing_data changes since i'm not
familiar with the BrowsingDataCounter class.

Also please be sure to run the browser_tests with dcheck enabled prior to
committing. The filesystem mostly runs on the IO thread and your browsertests
are doing some setup on the main thread. It looks like it should be ok, but
please run with dchecks on to see of it runs afoul of any thread checking.

https://codereview.chromium.org/2075023002/diff/20001/chrome/browser/browsing...
File chrome/browser/browsing_data/media_licenses_counter.cc (right):

https://codereview.chromium.org/2075023002/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/media_licenses_counter.cc:75:
base::Passed(std::move(filesystem_context)),
This looks a little dicey, is it possible for the closure be constructed prior
to filesystem_context deref on line 72?

I think you might be able to compose this more cleanly/safely using
PostTaskAndReplyWithResult, have CountOriginsOnFileTaskRunner return a
std::set<GURL>.

    PostTaskAndReplyWithResult(
        filesystem_context->default_file_task_runner(),
        FROM_HERE,
        base::Bind(&CountOriginsOnFileTaskRunner,
base::RetainedRef(filesystem_context)),
        Bind(&OnContentLicensesObtained, weak_factory_.GetWeakPtr()));


wdyt?

https://codereview.chromium.org/2075023002/diff/20001/chrome/browser/browsing...
File chrome/browser/browsing_data/media_licenses_counter_browsertest.cc (right):

https://codereview.chromium.org/2075023002/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/media_licenses_counter_browsertest.cc:54:
base::MessageLoop::current()->Run();
On 2016/06/21 06:02:41, xhwang wrote:
> MessageLoop::Run() is being deprecated. The new way to do this is
> RunLoop().Run().
> 
> See
>
https://cs.chromium.org/chromium/src/base/message_loop/message_loop.h?rcl=0&l...
> 
> See also https://codereview.chromium.org/2053123002/

Maybe take a look at passwords_counter_browsertest.cc for RunLoop usage in the
context of a similar test.

[Peter Kasting, 2016-06-22 00:05:13]

pkasting@chromium.org changed reviewers:
+ pkasting@chromium.org

[Peter Kasting, 2016-06-22 00:05:14]

LGTM

https://codereview.chromium.org/2075023002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/clear_browser_data_handler.cc (right):

https://codereview.chromium.org/2075023002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/clear_browser_data_handler.cc:298:
prefs::kDeleteMediaLicenses,
Nit: Honestly, I think the one-per-line wrapping of the old code was more
readable in this case.

https://codereview.chromium.org/2075023002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc
(right):

https://codereview.chromium.org/2075023002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:138:
prefs::kDeleteMediaLicenses,
Nit: Same comment.

[jrummell, 2016-06-23 02:48:56]

Updated.

https://codereview.chromium.org/2075023002/diff/20001/chrome/browser/browsing...
File chrome/browser/browsing_data/media_licenses_counter.cc (right):

https://codereview.chromium.org/2075023002/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/media_licenses_counter.cc:75:
base::Passed(std::move(filesystem_context)),
On 2016/06/21 20:23:41, michaeln wrote:
> This looks a little dicey, is it possible for the closure be constructed prior
> to filesystem_context deref on line 72?
> 
> I think you might be able to compose this more cleanly/safely using
> PostTaskAndReplyWithResult, have CountOriginsOnFileTaskRunner return a
> std::set<GURL>.
> 
>     PostTaskAndReplyWithResult(
>         filesystem_context->default_file_task_runner(),
>         FROM_HERE,
>         base::Bind(&CountOriginsOnFileTaskRunner,
> base::RetainedRef(filesystem_context)),
>         Bind(&OnContentLicensesObtained, weak_factory_.GetWeakPtr()));
> 
> 
> wdyt?

Done.

https://codereview.chromium.org/2075023002/diff/20001/chrome/browser/browsing...
File chrome/browser/browsing_data/media_licenses_counter_browsertest.cc (right):

https://codereview.chromium.org/2075023002/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/media_licenses_counter_browsertest.cc:54:
base::MessageLoop::current()->Run();
On 2016/06/21 06:02:41, xhwang wrote:
> MessageLoop::Run() is being deprecated. The new way to do this is
> RunLoop().Run().
> 
> See
>
https://cs.chromium.org/chromium/src/base/message_loop/message_loop.h?rcl=0&l...
> 
> See also https://codereview.chromium.org/2053123002/

Done.

https://codereview.chromium.org/2075023002/diff/20001/chrome/browser/browsing...
chrome/browser/browsing_data/media_licenses_counter_browsertest.cc:54:
base::MessageLoop::current()->Run();
On 2016/06/21 20:23:41, michaeln wrote:
> On 2016/06/21 06:02:41, xhwang wrote:
> > MessageLoop::Run() is being deprecated. The new way to do this is
> > RunLoop().Run().
> > 
> > See
> >
>
https://cs.chromium.org/chromium/src/base/message_loop/message_loop.h?rcl=0&l...
> > 
> > See also https://codereview.chromium.org/2053123002/
> 
> Maybe take a look at passwords_counter_browsertest.cc for RunLoop usage in the
> context of a similar test.

Thanks for the reference.

https://codereview.chromium.org/2075023002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/clear_browser_data_handler.cc (right):

https://codereview.chromium.org/2075023002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/clear_browser_data_handler.cc:177:
{"deleteMediaLicensesCheckbox", IDS_DEAUTHORIZE_CONTENT_LICENSES_CHKBOX},
On 2016/06/21 06:02:41, xhwang wrote:
> IDS_DEAUTHORIZE_CONTENT_LICENSES_CHKBOX should be renamed and its description
> should be updated:
> 
>
https://cs.chromium.org/chromium/src/chrome/app/generated_resources.grd?rcl=0...

I don't want to change the name, as it is part of the old UI and still shows
"Content licenses". Description updated, though.

https://codereview.chromium.org/2075023002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/clear_browser_data_handler.cc:298:
prefs::kDeleteMediaLicenses,
On 2016/06/22 00:05:14, Peter Kasting (OOO) wrote:
> Nit: Honestly, I think the one-per-line wrapping of the old code was more
> readable in this case.

Done (and ignored format trying to change it back).

https://codereview.chromium.org/2075023002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc
(right):

https://codereview.chromium.org/2075023002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc:138:
prefs::kDeleteMediaLicenses,
On 2016/06/22 00:05:14, Peter Kasting (OOO) wrote:
> Nit: Same comment.

Done.

[xhwang, 2016-06-23 04:30:54]

https://codereview.chromium.org/2075023002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/clear_browser_data_handler.cc (right):

https://codereview.chromium.org/2075023002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/clear_browser_data_handler.cc:177:
{"deleteMediaLicensesCheckbox", IDS_DEAUTHORIZE_CONTENT_LICENSES_CHKBOX},
On 2016/06/23 02:48:55, jrummell wrote:
> On 2016/06/21 06:02:41, xhwang wrote:
> > IDS_DEAUTHORIZE_CONTENT_LICENSES_CHKBOX should be renamed and its
description
> > should be updated:
> > 
> >
>
https://cs.chromium.org/chromium/src/chrome/app/generated_resources.grd?rcl=0...
> 
> I don't want to change the name, as it is part of the old UI and still shows
> "Content licenses". Description updated, though.

hmm, I am a bit confused. I thought we'll be updating both old and new UI to
"Media Licenses" (see the UI review doc). Also it might be better to be
consistent.

[jrummell, 2016-06-23 18:28:02]

Patchset #4 (id:60001) has been deleted

[jrummell, 2016-06-23 19:17:35]

Updated to use Media License in all tags.

https://codereview.chromium.org/2075023002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/clear_browser_data_handler.cc (right):

https://codereview.chromium.org/2075023002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/clear_browser_data_handler.cc:177:
{"deleteMediaLicensesCheckbox", IDS_DEAUTHORIZE_CONTENT_LICENSES_CHKBOX},
On 2016/06/23 04:30:54, xhwang wrote:
> On 2016/06/23 02:48:55, jrummell wrote:
> > On 2016/06/21 06:02:41, xhwang wrote:
> > > IDS_DEAUTHORIZE_CONTENT_LICENSES_CHKBOX should be renamed and its
> description
> > > should be updated:
> > > 
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/app/generated_resources.grd?rcl=0...
> > 
> > I don't want to change the name, as it is part of the old UI and still shows
> > "Content licenses". Description updated, though.
> 
> hmm, I am a bit confused. I thought we'll be updating both old and new UI to
> "Media Licenses" (see the UI review doc). Also it might be better to be
> consistent.

You're right. The original doc didn't change the text for this dialog, but the
most recent includes both. Done.

[michaeln, 2016-06-23 20:38:01]

browser/browsing_data lgtm

[jrummell, 2016-06-23 21:03:15]

jrummell@chromium.org changed reviewers:
+ rkaplow@chromium.org, stevenjb@chromium.org, thakis@chromium.org

[jrummell, 2016-06-23 21:03:16]

Adding more OWNERS
+stevenjb@ for chrome/browser/ui, chrome/browser/resources,
chrome/app/settings_strings.grdp
+thakis@ for the other chrome/browser files
+rkaplow@ for tools/metrics/actions/actions.xml

[Nico, 2016-06-23 21:23:11]

chrome/browser lgtm with html fixed.

Feels like a mix of a renaming cl and a new stuff cl, but oh well. The new stuff
looks well written!

https://codereview.chromium.org/2075023002/diff/80001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_counter_utils.cc (right):

https://codereview.chromium.org/2075023002/diff/80001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_counter_utils.cc:167:
(media_license_result->Value() > 0)
if your ternary op spans 6 lines, you probably want to use an if instead

https://codereview.chromium.org/2075023002/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html
(right):

https://codereview.chromium.org/2075023002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html:123:
label="$i18n{clearMediaLicenses}"
You probably want to keep the > here, no?

[stevenjb, 2016-06-23 21:40:12]

stevenjb@chromium.org changed reviewers:
+ dpapad@chromium.org

[stevenjb, 2016-06-23 21:40:13]

-> dpapad@ for options/settings review since he is more familiar with the
privacy settings.

[xhwang, 2016-06-23 23:24:26]

One last comment. Otherwise LGTM

https://chromiumcodereview.appspot.com/2075023002/diff/80001/chrome/app/gener...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/2075023002/diff/80001/chrome/app/gener...
chrome/app/generated_resources.grd:7844: +      <message
name="IDS_DEL_MEDIA_LICENSES_COUNTER" desc="A static message shown in the Clear
Browsing Data dialog explaining to the user that the 'Media licenses' option
will deauthorize their Flash Player licenses for protected content.">
This is recently added. We should talk to the owner to make sure we are on the
same page.

[dpapad, 2016-06-24 16:51:37]

https://codereview.chromium.org/2075023002/diff/80001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2075023002/diff/80001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7865: +      <message
name="IDS_DEL_MEDIA_LICENSES_GENERAL_COMMENT" desc="A static message shown in
the Clear Browsing Data dialog explaining to the user that clearing media
licenses will result in the user being unable to play some premium content">
Where are those 2 new strings being used? I could not find any references in
this CL.

[jrummell, 2016-06-24 18:26:04]

jrummell@chromium.org changed reviewers:
+ msramek@chromium.org

[jrummell, 2016-06-24 18:26:05]

Updated.

+msramek@

https://codereview.chromium.org/2075023002/diff/80001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2075023002/diff/80001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7844: +      <message
name="IDS_DEL_MEDIA_LICENSES_COUNTER" desc="A static message shown in the Clear
Browsing Data dialog explaining to the user that the 'Media licenses' option
will deauthorize their Flash Player licenses for protected content.">
On 2016/06/23 23:24:26, xhwang wrote:
> This is recently added. We should talk to the owner to make sure we are on the
> same page.
> 

Sent an email, and adding them to this CL. Removed since it doesn't appear to be
used.

https://codereview.chromium.org/2075023002/diff/80001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7865: +      <message
name="IDS_DEL_MEDIA_LICENSES_GENERAL_COMMENT" desc="A static message shown in
the Clear Browsing Data dialog explaining to the user that clearing media
licenses will result in the user being unable to play some premium content">
On 2016/06/24 16:51:36, dpapad wrote:
> Where are those 2 new strings being used? I could not find any references in
> this CL.

They are used in chrome/browser/browsing_data/browsing_data_counter_utils.cc.

https://codereview.chromium.org/2075023002/diff/80001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_counter_utils.cc (right):

https://codereview.chromium.org/2075023002/diff/80001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_counter_utils.cc:167:
(media_license_result->Value() > 0)
On 2016/06/23 21:23:11, Nico wrote:
> if your ternary op spans 6 lines, you probably want to use an if instead

Done.

https://codereview.chromium.org/2075023002/diff/80001/chrome/browser/resource...
File
chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html
(right):

https://codereview.chromium.org/2075023002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/clear_browsing_data_dialog/clear_browsing_data_dialog.html:123:
label="$i18n{clearMediaLicenses}"
On 2016/06/23 21:23:11, Nico wrote:
> You probably want to keep the > here, no?

Agreed. Strange that the page appears to load properly if it's broken.

[dpapad, 2016-06-24 20:21:59]

On 2016/06/23 at 21:40:13, stevenjb wrote:
> -> dpapad@ for options/settings review since he is more familiar with the
privacy settings.

LGTM for options/settings bits.

[msramek, 2016-06-27 14:27:33]

One of the last two missing counters in the CBD dialog, thanks for implementing
that :)

This LG, but I left a number of comments; the most important one is to check
back with the UI review.

https://codereview.chromium.org/2075023002/diff/100001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2075023002/diff/100001/chrome/app/generated_r...
chrome/app/generated_resources.grd:7862: +      <message
name="IDS_DEL_MEDIA_LICENSES_GENERAL_COMMENT" desc="A static message shown in
the Clear Browsing Data dialog explaining to the user that clearing media
licenses will result in the user being unable to play some premium content">
We have only recently got the new strings for the CBD dialog approved by the UI
review. Please discuss the changes with them (and loop me in).

It's often non-trivial to craft these strings so they're understandable, don't
overpromise, yet don't sound awkward or are actually technically feasible to
compute...

https://codereview.chromium.org/2075023002/diff/100001/chrome/app/generated_r...
chrome/app/generated_resources.grd:7863: +        You may lose access to premium
content from some sites.
This is displayed when the counter value is 0. Is it because there might
possibly be Flash content licenses?

https://codereview.chromium.org/2075023002/diff/100001/chrome/app/generated_r...
chrome/app/generated_resources.grd:7866: +        You may lose access to premium
content from <ph name="SITE">$1<ex>youtube.com</ex></ph> and some other sites.
Wouldn't it make sense to also show the number of sites?

https://codereview.chromium.org/2075023002/diff/100001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_counter_utils.cc (right):

https://codereview.chromium.org/2075023002/diff/100001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_counter_utils.cc:163: } else if
(pref_name == prefs::kDeleteMediaLicenses) {
Please add an empty line before "} else if {", as we do in the rest of this
method. This is a long chunk of text, so it's good to have at least a small
visual aid.

https://codereview.chromium.org/2075023002/diff/100001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/2075023002/diff/100001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover.cc:956: // TODO(jrummell):
This UMA should be renamed to indicate it is for Flash.
This metric isn't here because of Flash; it's here to measure the occurrence of
REMOVE_CONTENT_LICENSES in |remove_mask|.

So perhaps it should be renamed to "ClearBrowsingData_MediaLicenses", or just
left as is, if you don't mind the little inconsistency.

It might also make sense to move it upwards, so it's recorded even if plugins
aren't enabled, although I'm not sure if that happens in practice on desktop
builds.

https://codereview.chromium.org/2075023002/diff/100001/chrome/browser/browsin...
File chrome/browser/browsing_data/media_licenses_counter.cc (right):

https://codereview.chromium.org/2075023002/diff/100001/chrome/browser/browsin...
chrome/browser/browsing_data/media_licenses_counter.cc:1: // Copyright (c) 2016
The Chromium Authors. All rights reserved.
Nit: We don't use (c) anymore (also in the .h file)

https://codereview.chromium.org/2075023002/diff/100001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_ui_prefs.cc (right):

https://codereview.chromium.org/2075023002/diff/100001/chrome/browser/ui/brow...
chrome/browser/ui/browser_ui_prefs.cc:69: prefs::kDeleteMediaLicenses, false,
nit: for consistency (and thus better readability), false should be on the next
line

https://codereview.chromium.org/2075023002/diff/100001/chrome/common/pref_nam...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/2075023002/diff/100001/chrome/common/pref_nam...
chrome/common/pref_names.cc:893: const char kDeleteMediaLicenses[] =
"browser.clear_data.media_licenses";
Note that you're replacing the preference without doing any migration.

This means that users will have the "media licenses" checkbox unchecked after
this change, regardless of whether they used to have the "content licenses"
checkbox checked or not.

This is probably OK, since the new checkbox is more powerful, so we might want
to force users to redecide whether they want to have it checked.

(just wanted to point this out)

[rkaplow, 2016-06-27 19:34:02]

lgtm

actions lg

[jrummell, 2016-06-28 01:16:48]

Updated.

https://codereview.chromium.org/2075023002/diff/100001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2075023002/diff/100001/chrome/app/generated_r...
chrome/app/generated_resources.grd:7862: +      <message
name="IDS_DEL_MEDIA_LICENSES_GENERAL_COMMENT" desc="A static message shown in
the Clear Browsing Data dialog explaining to the user that clearing media
licenses will result in the user being unable to play some premium content">
On 2016/06/27 14:27:33, msramek wrote:
> We have only recently got the new strings for the CBD dialog approved by the
UI
> review. Please discuss the changes with them (and loop me in).
> 
> It's often non-trivial to craft these strings so they're understandable, don't
> overpromise, yet don't sound awkward or are actually technically feasible to
> compute...

Added you to the email thread with the UI people.

https://codereview.chromium.org/2075023002/diff/100001/chrome/app/generated_r...
chrome/app/generated_resources.grd:7863: +        You may lose access to premium
content from some sites.
On 2016/06/27 14:27:33, msramek wrote:
> This is displayed when the counter value is 0. Is it because there might
> possibly be Flash content licenses?

Correct. It appears that it would be possible to get the list of domains from
Flash as well, but that is not implemented.

https://codereview.chromium.org/2075023002/diff/100001/chrome/app/generated_r...
chrome/app/generated_resources.grd:7866: +        You may lose access to premium
content from <ph name="SITE">$1<ex>youtube.com</ex></ph> and some other sites.
On 2016/06/27 14:27:32, msramek wrote:
> Wouldn't it make sense to also show the number of sites?

I think that this would require changes to get the list from Flash as well.

https://codereview.chromium.org/2075023002/diff/100001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_counter_utils.cc (right):

https://codereview.chromium.org/2075023002/diff/100001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_counter_utils.cc:163: } else if
(pref_name == prefs::kDeleteMediaLicenses) {
On 2016/06/27 14:27:33, msramek wrote:
> Please add an empty line before "} else if {", as we do in the rest of this
> method. This is a long chunk of text, so it's good to have at least a small
> visual aid.

Done.

https://codereview.chromium.org/2075023002/diff/100001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/2075023002/diff/100001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover.cc:956: // TODO(jrummell):
This UMA should be renamed to indicate it is for Flash.
On 2016/06/27 14:27:33, msramek wrote:
> This metric isn't here because of Flash; it's here to measure the occurrence
of
> REMOVE_CONTENT_LICENSES in |remove_mask|.
> 
> So perhaps it should be renamed to "ClearBrowsingData_MediaLicenses", or just
> left as is, if you don't mind the little inconsistency.
> 
> It might also make sense to move it upwards, so it's recorded even if plugins
> aren't enabled, although I'm not sure if that happens in practice on desktop
> builds.

I'm wasn't sure what Actions do, and the description in actions.xml didn't help.
I have no problem renaming it, if that won't break anything (looks like obsolete
actions need to be kept). 

WDYT?

For now I've moved it out of the #if, since the dialog will show the option
always, and we should count it for everybody.

https://codereview.chromium.org/2075023002/diff/100001/chrome/browser/browsin...
File chrome/browser/browsing_data/media_licenses_counter.cc (right):

https://codereview.chromium.org/2075023002/diff/100001/chrome/browser/browsin...
chrome/browser/browsing_data/media_licenses_counter.cc:1: // Copyright (c) 2016
The Chromium Authors. All rights reserved.
On 2016/06/27 14:27:33, msramek wrote:
> Nit: We don't use (c) anymore (also in the .h file)

Done. Just cut&paste from a different file, change the date.

https://codereview.chromium.org/2075023002/diff/100001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_ui_prefs.cc (right):

https://codereview.chromium.org/2075023002/diff/100001/chrome/browser/ui/brow...
chrome/browser/ui/browser_ui_prefs.cc:69: prefs::kDeleteMediaLicenses, false,
On 2016/06/27 14:27:33, msramek wrote:
> nit: for consistency (and thus better readability), false should be on the
next
> line

Done. It's cl format that wants it this way.

https://codereview.chromium.org/2075023002/diff/100001/chrome/common/pref_nam...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/2075023002/diff/100001/chrome/common/pref_nam...
chrome/common/pref_names.cc:893: const char kDeleteMediaLicenses[] =
"browser.clear_data.media_licenses";
On 2016/06/27 14:27:33, msramek wrote:
> Note that you're replacing the preference without doing any migration.
> 
> This means that users will have the "media licenses" checkbox unchecked after
> this change, regardless of whether they used to have the "content licenses"
> checkbox checked or not.
> 
> This is probably OK, since the new checkbox is more powerful, so we might want
> to force users to redecide whether they want to have it checked.
> 
> (just wanted to point this out)

Acknowledged.

[msramek, 2016-06-28 09:07:51]

browsing_data/ LGTM with a few more comments

Note also that there's another CL in review which starts componentizing
counters. But I think the only conflict it would cause if it lands first is that
in media_licenses_counter.h, you'll have to include browsing_data_counter.h from
//components instead of //chrome.

https://codereview.chromium.org/2075023002/diff/100001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2075023002/diff/100001/chrome/app/generated_r...
chrome/app/generated_resources.grd:7866: +        You may lose access to premium
content from <ph name="SITE">$1<ex>youtube.com</ex></ph> and some other sites.
On 2016/06/28 01:16:47, jrummell wrote:
> On 2016/06/27 14:27:32, msramek wrote:
> > Wouldn't it make sense to also show the number of sites?
> 
> I think that this would require changes to get the list from Flash as well.

I see. As a future improvement, I liked the idea from the UI thread about
showing numbers if Flash licenses are not present.

https://codereview.chromium.org/2075023002/diff/100001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_remover.cc (right):

https://codereview.chromium.org/2075023002/diff/100001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover.cc:956: // TODO(jrummell):
This UMA should be renamed to indicate it is for Flash.
On 2016/06/28 01:16:48, jrummell wrote:
> On 2016/06/27 14:27:33, msramek wrote:
> > This metric isn't here because of Flash; it's here to measure the occurrence
> of
> > REMOVE_CONTENT_LICENSES in |remove_mask|.
> > 
> > So perhaps it should be renamed to "ClearBrowsingData_MediaLicenses", or
just
> > left as is, if you don't mind the little inconsistency.
> > 
> > It might also make sense to move it upwards, so it's recorded even if
plugins
> > aren't enabled, although I'm not sure if that happens in practice on desktop
> > builds.
> 
> I'm wasn't sure what Actions do, and the description in actions.xml didn't
help.
> I have no problem renaming it, if that won't break anything (looks like
obsolete
> actions need to be kept). 
> 
> WDYT?
> 
> For now I've moved it out of the #if, since the dialog will show the option
> always, and we should count it for everybody.

Yep, this works for me. Not renaming it means we won't lose the continuity of
data.

If somebody is bothered that the metric is still called "ContentLicenses", it
can always be renamed later.

https://codereview.chromium.org/2075023002/diff/120001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2075023002/diff/120001/chrome/app/generated_r...
chrome/app/generated_resources.grd:7862: +      <message
name="IDS_DEL_MEDIA_LICENSES_GENERAL_COMMENT" desc="A static message shown in
the Clear Browsing Data dialog explaining to the user that clearing media
licenses will result in the user being unable to play some premium content">
nit: Consider naming these strings IDS_DEL_.+_COUNTER.* for consistency with the
others.

https://codereview.chromium.org/2075023002/diff/120001/chrome/browser/browsin...
File chrome/browser/browsing_data/media_licenses_counter.h (right):

https://codereview.chromium.org/2075023002/diff/120001/chrome/browser/browsin...
chrome/browser/browsing_data/media_licenses_counter.h:18: class
MediaLicenseCounter : public BrowsingDataCounter {
Sorry, I missed this before: This should be MediaLicense*s*Counter, no? That's
how this file, the browsertest, and other counters (HostedApp*s*Counter,
Download*s*Counter) are named.

https://codereview.chromium.org/2075023002/diff/120001/chrome/browser/browsin...
File chrome/browser/browsing_data/media_licenses_counter_browsertest.cc (right):

https://codereview.chromium.org/2075023002/diff/120001/chrome/browser/browsin...
chrome/browser/browsing_data/media_licenses_counter_browsertest.cc:167: // Tests
that for a non-empty cache, the result is nonzero.
Copy&paste? :)

[jrummell, 2016-06-28 18:12:47]

Updated.

stevenjb@, looks like I still need your review for
chrome/browser/resources/options/ and chrome/browser/ui/webui/options/

https://codereview.chromium.org/2075023002/diff/120001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2075023002/diff/120001/chrome/app/generated_r...
chrome/app/generated_resources.grd:7862: +      <message
name="IDS_DEL_MEDIA_LICENSES_GENERAL_COMMENT" desc="A static message shown in
the Clear Browsing Data dialog explaining to the user that clearing media
licenses will result in the user being unable to play some premium content">
On 2016/06/28 09:07:51, msramek wrote:
> nit: Consider naming these strings IDS_DEL_.+_COUNTER.* for consistency with
the
> others.

Done.

https://codereview.chromium.org/2075023002/diff/120001/chrome/browser/browsin...
File chrome/browser/browsing_data/media_licenses_counter.h (right):

https://codereview.chromium.org/2075023002/diff/120001/chrome/browser/browsin...
chrome/browser/browsing_data/media_licenses_counter.h:18: class
MediaLicenseCounter : public BrowsingDataCounter {
On 2016/06/28 09:07:51, msramek wrote:
> Sorry, I missed this before: This should be MediaLicense*s*Counter, no? That's
> how this file, the browsertest, and other counters (HostedApp*s*Counter,
> Download*s*Counter) are named.

Done.

https://codereview.chromium.org/2075023002/diff/120001/chrome/browser/browsin...
File chrome/browser/browsing_data/media_licenses_counter_browsertest.cc (right):

https://codereview.chromium.org/2075023002/diff/120001/chrome/browser/browsin...
chrome/browser/browsing_data/media_licenses_counter_browsertest.cc:167: // Tests
that for a non-empty cache, the result is nonzero.
On 2016/06/28 09:07:51, msramek wrote:
> Copy&paste? :)

Yup. Done.

[jrummell, 2016-06-28 20:54:52]

Description was changed from

==========
UI Changes to support clearing EME/CDM data

Update the "Clear Browsing Data" dialogs to clear the EME/CDM data
when "Content Licenses" is selected. Currently selecting "Content
Licenses" only clears Flash data, if Flash is enabled.

Added new preference kDeleteMediaLicenses which replaces
kDeauthorizeContentLicenses, which was Flash specific. Default
value for kDeleteMediaLicenses is false, so the checkbox will
not be selected initially.

EME/CDM data is stored in the plugin private file system. If there
is no data saved for EME, then deletion is a nop. Actually deleting
the data was handled in https://codereview.chromium.org/1979733002/.

Changes to chrome://settings/clearBrowserData:
1) "Content licenses" check box will always be displayed. (Currently
   it is only displayed if Flash is enabled.)
2) When selected, it will display an additional message as noted in
   the UI design doc (e.g. "You may lose access to premium content
   from some sites" or "You may lose access to premium content from
   www.youtube.com and some other sites.").

Changes to chrome://md-settings/advanced -> Clear Browsing Data
1) "Content licenses" is renamed to "Media Licenses"

BUG=418195
TEST=tested "Clear Browsing Data" dialogs manually, new tests pass
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
UI Changes to support clearing EME/CDM data

Update the "Clear Browsing Data" dialogs to clear the EME/CDM data
when "Content Licenses" is selected. Currently selecting "Content
Licenses" only clears Flash data, if Flash is enabled.

Added new preference kDeleteMediaLicenses which replaces
kDeauthorizeContentLicenses, which was Flash specific. Default
value for kDeleteMediaLicenses is false, so the checkbox will
not be selected initially.

EME/CDM data is stored in the plugin private file system. If there
is no data saved for EME, then deletion is a nop. Actually deleting
the data was handled in https://codereview.chromium.org/1979733002/.

Changes to chrome://settings/clearBrowserData:
1) "Content licenses" is renamed to "Media Licenses".
2) "Content licenses" check box will always be displayed. (Currently
   it is only displayed if Flash is enabled.)
3) When selected, it will display an additional message as noted in
   the UI design doc (e.g. "You may lose access to premium content
   from some sites" or "You may lose access to premium content from
   www.youtube.com and some other sites.").

Changes to chrome://md-settings/advanced -> Clear Browsing Data
1) "Content licenses" is renamed to "Media Licenses".
2) When selected, it will display an additional message as noted
   above.

BUG=418195
TEST=tested "Clear Browsing Data" dialogs manually, new tests pass
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[jrummell, 2016-06-28 20:56:25]

Rebased. Had to make a minor change so that md-settings now shows the additional
message (instead of the static message that was being removed).

[jrummell, 2016-06-29 18:00:08]

Updated to only display host name (without scheme) in message.

stevenjb@, I still need your review for
chrome/browser/resources/options/ and chrome/browser/ui/webui/options/

[stevenjb, 2016-06-29 23:29:32]

RS lgtm

[jrummell, 2016-06-30 00:19:55]

Thanks for all the reviews.

[jrummell, 2016-06-30 00:20:05]

The CQ bit was checked by jrummell@chromium.org

[jrummell, 2016-06-30 00:20:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, michaeln@chromium.org, thakis@chromium.org,
xhwang@chromium.org, rkaplow@chromium.org, dpapad@chromium.org,
msramek@chromium.org
Link to the patchset: https://codereview.chromium.org/2075023002/#ps240001
(title: "revert options_page.css")

[commit-bot: I haz the power, 2016-06-30 00:21:13]

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-06-30 00:30:52]

Description was changed from

==========
UI Changes to support clearing EME/CDM data

Update the "Clear Browsing Data" dialogs to clear the EME/CDM data
when "Content Licenses" is selected. Currently selecting "Content
Licenses" only clears Flash data, if Flash is enabled.

Added new preference kDeleteMediaLicenses which replaces
kDeauthorizeContentLicenses, which was Flash specific. Default
value for kDeleteMediaLicenses is false, so the checkbox will
not be selected initially.

EME/CDM data is stored in the plugin private file system. If there
is no data saved for EME, then deletion is a nop. Actually deleting
the data was handled in https://codereview.chromium.org/1979733002/.

Changes to chrome://settings/clearBrowserData:
1) "Content licenses" is renamed to "Media Licenses".
2) "Content licenses" check box will always be displayed. (Currently
   it is only displayed if Flash is enabled.)
3) When selected, it will display an additional message as noted in
   the UI design doc (e.g. "You may lose access to premium content
   from some sites" or "You may lose access to premium content from
   www.youtube.com and some other sites.").

Changes to chrome://md-settings/advanced -> Clear Browsing Data
1) "Content licenses" is renamed to "Media Licenses".
2) When selected, it will display an additional message as noted
   above.

BUG=418195
TEST=tested "Clear Browsing Data" dialogs manually, new tests pass
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
UI Changes to support clearing EME/CDM data

Update the "Clear Browsing Data" dialogs to clear the EME/CDM data
when "Content Licenses" is selected. Currently selecting "Content
Licenses" only clears Flash data, if Flash is enabled.

Added new preference kDeleteMediaLicenses which replaces
kDeauthorizeContentLicenses, which was Flash specific. Default
value for kDeleteMediaLicenses is false, so the checkbox will
not be selected initially.

EME/CDM data is stored in the plugin private file system. If there
is no data saved for EME, then deletion is a nop. Actually deleting
the data was handled in https://codereview.chromium.org/1979733002/.

Changes to chrome://settings/clearBrowserData:
1) "Content licenses" is renamed to "Media Licenses".
2) "Content licenses" check box will always be displayed. (Currently
   it is only displayed if Flash is enabled.)
3) When selected, it will display an additional message as noted in
   the UI design doc (e.g. "You may lose access to premium content
   from some sites" or "You may lose access to premium content from
   www.youtube.com and some other sites.").

Changes to chrome://md-settings/advanced -> Clear Browsing Data
1) "Content licenses" is renamed to "Media Licenses".
2) When selected, it will display an additional message as noted
   above.

BUG=418195
TEST=tested "Clear Browsing Data" dialogs manually, new tests pass
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-06-30 00:30:54]

Committed patchset #12 (id:240001)

[commit-bot: I haz the power, 2016-06-30 00:30:57]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-30 00:33:44]

Description was changed from

==========
UI Changes to support clearing EME/CDM data

Update the "Clear Browsing Data" dialogs to clear the EME/CDM data
when "Content Licenses" is selected. Currently selecting "Content
Licenses" only clears Flash data, if Flash is enabled.

Added new preference kDeleteMediaLicenses which replaces
kDeauthorizeContentLicenses, which was Flash specific. Default
value for kDeleteMediaLicenses is false, so the checkbox will
not be selected initially.

EME/CDM data is stored in the plugin private file system. If there
is no data saved for EME, then deletion is a nop. Actually deleting
the data was handled in https://codereview.chromium.org/1979733002/.

Changes to chrome://settings/clearBrowserData:
1) "Content licenses" is renamed to "Media Licenses".
2) "Content licenses" check box will always be displayed. (Currently
   it is only displayed if Flash is enabled.)
3) When selected, it will display an additional message as noted in
   the UI design doc (e.g. "You may lose access to premium content
   from some sites" or "You may lose access to premium content from
   www.youtube.com and some other sites.").

Changes to chrome://md-settings/advanced -> Clear Browsing Data
1) "Content licenses" is renamed to "Media Licenses".
2) When selected, it will display an additional message as noted
   above.

BUG=418195
TEST=tested "Clear Browsing Data" dialogs manually, new tests pass
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
UI Changes to support clearing EME/CDM data

Update the "Clear Browsing Data" dialogs to clear the EME/CDM data
when "Content Licenses" is selected. Currently selecting "Content
Licenses" only clears Flash data, if Flash is enabled.

Added new preference kDeleteMediaLicenses which replaces
kDeauthorizeContentLicenses, which was Flash specific. Default
value for kDeleteMediaLicenses is false, so the checkbox will
not be selected initially.

EME/CDM data is stored in the plugin private file system. If there
is no data saved for EME, then deletion is a nop. Actually deleting
the data was handled in https://codereview.chromium.org/1979733002/.

Changes to chrome://settings/clearBrowserData:
1) "Content licenses" is renamed to "Media Licenses".
2) "Content licenses" check box will always be displayed. (Currently
   it is only displayed if Flash is enabled.)
3) When selected, it will display an additional message as noted in
   the UI design doc (e.g. "You may lose access to premium content
   from some sites" or "You may lose access to premium content from
   www.youtube.com and some other sites.").

Changes to chrome://md-settings/advanced -> Clear Browsing Data
1) "Content licenses" is renamed to "Media Licenses".
2) When selected, it will display an additional message as noted
   above.

BUG=418195
TEST=tested "Clear Browsing Data" dialogs manually, new tests pass
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/bffd01243bee7c038ef6c4a92814c77176d9ebdd
Cr-Commit-Position: refs/heads/master@{#403029}
==========

[commit-bot: I haz the power, 2016-06-30 00:33:46]

Patchset 12 (id:??) landed as
https://crrev.com/bffd01243bee7c038ef6c4a92814c77176d9ebdd
Cr-Commit-Position: refs/heads/master@{#403029}