Write last_modified date to Content Settings in the PrefProvider

Write last_modified date to Content Settings in the PrefProvider

Introduce a last_modified timestamp for content settings stored inside the PrefProvider. 
This timestamp will enable deleting content-settings by a selected time range from the clear browsing data dialog. 
The timestamp will only be recorded if the tabs-in-cbd flag is activated because otherwise there would be no way to delete these history-like entries.

BUG=681523
Review-Url: https://codereview.chromium.org/2812113004
Cr-Commit-Position: refs/heads/master@{#468950}
Committed: https://chromium.googlesource.com/chromium/src/+/39d6d5f89a382f3f6f23ae80a9c9c87c2f184371

[dullweber, 2017-04-12 15:50:58]

Description was changed from

==========
Write last modified date to preferences PROPOSAL

BUG=
==========

to

==========
Write last modified date to Content Settings PROPOSAL

BUG=
==========

[dullweber, 2017-04-13 12:09:53]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 12:10:09]

Dry run: 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, 2017-04-13 12:31:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-13 12:31:24]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[dullweber, 2017-04-13 15:07:20]

Description was changed from

==========
Write last modified date to Content Settings PROPOSAL

BUG=
==========

to

==========
Write last_modified date to Content Settings in the PrefProvider

Introduce a last_modified timestamp for content settings stored inside the
PrefProvider. This timestamp will enable deleting content-settings by a selected
time range from the clear browsing data dialog. 

BUG=
==========

[dullweber, 2017-04-13 15:08:38]

Description was changed from

==========
Write last_modified date to Content Settings in the PrefProvider

Introduce a last_modified timestamp for content settings stored inside the
PrefProvider. This timestamp will enable deleting content-settings by a selected
time range from the clear browsing data dialog. 

BUG=
==========

to

==========
Write last_modified date to Content Settings in the PrefProvider

Introduce a last_modified timestamp for content settings stored inside the
PrefProvider. This timestamp will enable deleting content-settings by a selected
time range from the clear browsing data dialog. 

BUG=681523
==========

[dullweber, 2017-04-13 15:10:55]

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

[dullweber, 2017-04-13 15:10:56]

Hi Martin, 
here is the CL to add a timestamp for content-settings. Please take a look at
it. 
Should I use the tabs-in-cbd flag to enable recording of time-stamps or is the
current approach with a #ifdef for OS_ANDROID better?

[dullweber, 2017-04-19 09:07:00]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 09:07:31]

Dry run: 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, 2017-04-19 10:13:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-19 10:13:27]

Dry run: This issue passed the CQ dry run.

[msramek, 2017-04-19 10:49:14]

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

[msramek, 2017-04-19 10:49:16]

Thanks, this seems to work correctly. I added a number of suggestions, and one
open question about the treatment of the default provider.

Adding +Raymes explicitly, since he'll have opinions as well.

https://codereview.chromium.org/2812113004/diff/40001/chrome/browser/content_...
File
chrome/browser/content_settings/content_settings_internal_extension_provider.cc
(right):

https://codereview.chromium.org/2812113004/diff/40001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_internal_extension_provider.cc:215:
CONTENT_SETTINGS_TYPE_PLUGINS, resource, base::Time(),
nit: Extension settings are processed when the extension is installed or at
browser startup; so I agree that a timestamp such as base::Time::Now() would be
meaningless here. But we should probably document it somewhere.

https://codereview.chromium.org/2812113004/diff/40001/chrome/browser/content_...
File
chrome/browser/content_settings/content_settings_origin_identifier_value_map_unittest.cc
(right):

https://codereview.chromium.org/2812113004/diff/40001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_origin_identifier_value_map_unittest.cc:241:
EXPECT_EQ(t2, rule.last_modified);
Please add a check that the other rules still have their old timestamps.

https://codereview.chromium.org/2812113004/diff/40001/chrome/browser/content_...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

https://codereview.chromium.org/2812113004/diff/40001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:609:
base::Time t1 = base::Time::Now();
optional: This might be a good place for SimpleTestClock, but I'm personally
fine with the current coverage using LT and GT.

https://codereview.chromium.org/2812113004/diff/40001/chrome/browser/content_...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/2812113004/diff/40001/chrome/browser/content_...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1793: }
Please also add a test with resource identifiers.

https://codereview.chromium.org/2812113004/diff/40001/components/content_sett...
File
components/content_settings/core/browser/content_settings_default_provider.cc
(right):

https://codereview.chromium.org/2812113004/diff/40001/components/content_sett...
components/content_settings/core/browser/content_settings_default_provider.cc:73:
ContentSettingsPattern::Wildcard(), base::Time(),
I would argue that we *do* want to use real timestamps here.

Deleting content settings for a time period should also reset the default
settings changed in that period. It would be complicated to reset them to the
previous state (because that would require us to keep a full history of their
state), but we could at least reset them to default.

WDYT?

https://codereview.chromium.org/2812113004/diff/40001/components/content_sett...
File
components/content_settings/core/browser/content_settings_origin_identifier_value_map.h
(right):

https://codereview.chromium.org/2812113004/diff/40001/components/content_sett...
components/content_settings/core/browser/content_settings_origin_identifier_value_map.h:110:
base::Time last_modified,
Given the number of callsites that pass base::Time(), we should probably have a
version of this method that doesn't take a time parameter.

https://codereview.chromium.org/2812113004/diff/40001/components/content_sett...
File components/content_settings/core/browser/content_settings_pref.cc (right):

https://codereview.chromium.org/2812113004/diff/40001/components/content_sett...
components/content_settings/core/browser/content_settings_pref.cc:245: // Get
last modified timestamp.
We're using one last modified timestamp per pattern pair, regardless of resource
identifiers. As discussed offline, I'm fine with that to avoid complexity, since
the only downside is that deletion of content settings would reset all plugins
on a page where you recently changed the setting for one of them.

...if that even works after all the Flash changes. I have a vague memory that
Raymes was about to remove those.

However, please comment on that :)

https://codereview.chromium.org/2812113004/diff/40001/components/content_sett...
File components/content_settings/core/browser/content_settings_pref_provider.cc
(right):

https://codereview.chromium.org/2812113004/diff/40001/components/content_sett...
components/content_settings/core/browser/content_settings_pref_provider.cc:84: :
PrefProvider(prefs, incognito, false) {}
Do we need this? HCSM needs to specify where to use the timestamps, and in
tests, I would rather be explicit which one we use. If you still prefer a
shortcut, you can just define in the test file.

https://codereview.chromium.org/2812113004/diff/40001/components/content_sett...
File components/content_settings/core/browser/host_content_settings_map.h
(right):

https://codereview.chromium.org/2812113004/diff/40001/components/content_sett...
components/content_settings/core/browser/host_content_settings_map.h:53: #if
defined(OS_ANDROID)
To answer your question - I'd prefer if this was guarded by our experiment flag.
I trust that you finish this by the next release, but still :) Let's be
conservative.

[dullweber, 2017-04-19 14:54:03]

Description was changed from

==========
Write last_modified date to Content Settings in the PrefProvider

Introduce a last_modified timestamp for content settings stored inside the
PrefProvider. This timestamp will enable deleting content-settings by a selected
time range from the clear browsing data dialog. 

BUG=681523
==========

to

==========
Write last_modified date to Content Settings in the PrefProvider

Introduce a last_modified timestamp for content settings stored inside the
PrefProvider. 
This timestamp will enable deleting content-settings by a selected time range
from the clear browsing data dialog. 
The timestamp will only be recorded if the tabs-in-cbd flag is activated because
otherwise there would be no way to delete these history-like entries.

BUG=681523
==========

[dullweber, 2017-04-19 14:54:21]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 14:54:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dullweber, 2017-04-19 15:02:47]

Thanks, I responded to your comments or fixed the issues. I think we should
decide on how to treat resource_identifier before we add all this special code
for them. If they are removed soon it might be best to not support timestamps
for them until they are completely removed.

https://codereview.chromium.org/2812113004/diff/40001/chrome/browser/content_...
File
chrome/browser/content_settings/content_settings_internal_extension_provider.cc
(right):

https://codereview.chromium.org/2812113004/diff/40001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_internal_extension_provider.cc:215:
CONTENT_SETTINGS_TYPE_PLUGINS, resource, base::Time(),
On 2017/04/19 10:49:15, msramek wrote:
> nit: Extension settings are processed when the extension is installed or at
> browser startup; so I agree that a timestamp such as base::Time::Now() would
be
> meaningless here. But we should probably document it somewhere.

I added a comment to show that this is done intentionally

https://codereview.chromium.org/2812113004/diff/40001/chrome/browser/content_...
File
chrome/browser/content_settings/content_settings_origin_identifier_value_map_unittest.cc
(right):

https://codereview.chromium.org/2812113004/diff/40001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_origin_identifier_value_map_unittest.cc:241:
EXPECT_EQ(t2, rule.last_modified);
On 2017/04/19 10:49:15, msramek wrote:
> Please add a check that the other rules still have their old timestamps.

Done.

https://codereview.chromium.org/2812113004/diff/40001/chrome/browser/content_...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

https://codereview.chromium.org/2812113004/diff/40001/chrome/browser/content_...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:609:
base::Time t1 = base::Time::Now();
On 2017/04/19 10:49:15, msramek wrote:
> optional: This might be a good place for SimpleTestClock, but I'm personally
> fine with the current coverage using LT and GT.

Thanks for pointing at this, it will probably be useful for other tests. I think
here it is easier to use timestamps than passing a clock from PrefProvider to
each ContentSettingsPref.

(At least if the clocks on all platforms are precise enough to ensure that
base::Time::Now() != base::Time::Now())

https://codereview.chromium.org/2812113004/diff/40001/chrome/browser/content_...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/2812113004/diff/40001/chrome/browser/content_...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1793: }
On 2017/04/19 10:49:16, msramek wrote:
> Please also add a test with resource identifiers.

I added a test and discovered that ClearSettingsForOneTypeWithPredicate
currently doesn't work for resource settings because internally it calls
GetSettingsForOneType(type, std::string()). I changed it to also have a
resource_identifier parameter but that might be unnecessary if we remove all
resource_identifier

https://codereview.chromium.org/2812113004/diff/40001/components/content_sett...
File
components/content_settings/core/browser/content_settings_default_provider.cc
(right):

https://codereview.chromium.org/2812113004/diff/40001/components/content_sett...
components/content_settings/core/browser/content_settings_default_provider.cc:73:
ContentSettingsPattern::Wildcard(), base::Time(),
On 2017/04/19 10:49:16, msramek wrote:
> I would argue that we *do* want to use real timestamps here.
> 
> Deleting content settings for a time period should also reset the default
> settings changed in that period. It would be complicated to reset them to the
> previous state (because that would require us to keep a full history of their
> state), but we could at least reset them to default.
> 
> WDYT?

We currently don't delete the default setting and I think I would prefer to keep
it this way.
(https://cs.chromium.org/chromium/src/chrome/browser/browsing_data/chrome_brow...)

My interpretation is that the default permission is something that belongs to
the normal profile settings which are normally not deleted by CBD. Site specific
settings should be removable because they are usually set while browsing.

https://codereview.chromium.org/2812113004/diff/40001/components/content_sett...
File
components/content_settings/core/browser/content_settings_origin_identifier_value_map.h
(right):

https://codereview.chromium.org/2812113004/diff/40001/components/content_sett...
components/content_settings/core/browser/content_settings_origin_identifier_value_map.h:110:
base::Time last_modified,
On 2017/04/19 10:49:16, msramek wrote:
> Given the number of callsites that pass base::Time(), we should probably have
a
> version of this method that doesn't take a time parameter.

Most calls with base::Time() are in tests. I added comments to the
content_settings_store and the internal_extension_provider to explain that we
don't pass a timestamp intentionally. Is it  ok to keep it this way?

https://codereview.chromium.org/2812113004/diff/40001/components/content_sett...
File components/content_settings/core/browser/content_settings_pref.cc (right):

https://codereview.chromium.org/2812113004/diff/40001/components/content_sett...
components/content_settings/core/browser/content_settings_pref.cc:245: // Get
last modified timestamp.
On 2017/04/19 10:49:16, msramek wrote:
> We're using one last modified timestamp per pattern pair, regardless of
resource
> identifiers. As discussed offline, I'm fine with that to avoid complexity,
since
> the only downside is that deletion of content settings would reset all plugins
> on a page where you recently changed the setting for one of them.
> 
> ...if that even works after all the Flash changes. I have a vague memory that
> Raymes was about to remove those.
> 
> However, please comment on that :)

I just changed it to store a timestamp per resource but if the whole
per-resource settings can be removed anyway that would be much better.

https://codereview.chromium.org/2812113004/diff/40001/components/content_sett...
File components/content_settings/core/browser/content_settings_pref_provider.cc
(right):

https://codereview.chromium.org/2812113004/diff/40001/components/content_sett...
components/content_settings/core/browser/content_settings_pref_provider.cc:84: :
PrefProvider(prefs, incognito, false) {}
On 2017/04/19 10:49:16, msramek wrote:
> Do we need this? HCSM needs to specify where to use the timestamps, and in
> tests, I would rather be explicit which one we use. If you still prefer a
> shortcut, you can just define in the test file.

Done.

https://codereview.chromium.org/2812113004/diff/40001/components/content_sett...
File components/content_settings/core/browser/host_content_settings_map.h
(right):

https://codereview.chromium.org/2812113004/diff/40001/components/content_sett...
components/content_settings/core/browser/host_content_settings_map.h:53: #if
defined(OS_ANDROID)
On 2017/04/19 10:49:16, msramek wrote:
> To answer your question - I'd prefer if this was guarded by our experiment
flag.
> I trust that you finish this by the next release, but still :) Let's be
> conservative.

Done.

[commit-bot: I haz the power, 2017-04-19 15:03:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-19 15:03:52]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[raymes, 2017-04-20 00:07:58]

We end up polluting a lot of things with the base::Time here which I think adds
complexity to the APIs and makes it confusing when the last modified time can be
trusted and when it can't.

It looks like we are only storing the base::Time for user prefs, is that
correct? Could we get away with only exposing an API on
ClearSettingsForOneTypeWithPredicate? 

HostContentSettingsMap::ClearSettingsForOneTypeWithPredicate could take a
base::Time which is the time after which to clear settings for. That function
eventually calls into PrefProvider::SetWebsiteSetting -- in fact, even though it
loops through the providers, it only ends up modifying that one provider. Rather
than looping through the providers, we could call into the PrefProvider directly
and pass in the base::Time. The setting would only be updated if the base::Time
passed in was prior to the last modified time in the pref provider.

What do you think of this approach? Any pitfalls I'm missing? It would avoid
needing to modify content_settings::Rule and friends which I see as a big
benefit. It would introduce some slight special-casing but in this case I think
it would be worth the tradeoff in complexity.

Thanks!

https://codereview.chromium.org/2812113004/diff/60001/chrome/browser/content_...
File chrome/browser/content_settings/host_content_settings_map_factory.cc
(right):

https://codereview.chromium.org/2812113004/diff/60001/chrome/browser/content_...
chrome/browser/content_settings/host_content_settings_map_factory.cc:85:
store_last_modified));
Is it necessary to pass this in? Can we check it inside the object instead?

[raymes, 2017-04-20 00:10:13]

https://codereview.chromium.org/2812113004/diff/40001/components/content_sett...
File components/content_settings/core/browser/content_settings_pref.cc (right):

https://codereview.chromium.org/2812113004/diff/40001/components/content_sett...
components/content_settings/core/browser/content_settings_pref.cc:245: // Get
last modified timestamp.
On 2017/04/19 15:02:45, dullweber wrote:
> On 2017/04/19 10:49:16, msramek wrote:
> > We're using one last modified timestamp per pattern pair, regardless of
> resource
> > identifiers. As discussed offline, I'm fine with that to avoid complexity,
> since
> > the only downside is that deletion of content settings would reset all
plugins
> > on a page where you recently changed the setting for one of them.
> > 
> > ...if that even works after all the Flash changes. I have a vague memory
that
> > Raymes was about to remove those.
> > 
> > However, please comment on that :)
> 
> I just changed it to store a timestamp per resource but if the whole
> per-resource settings can be removed anyway that would be much better.

I think we'd like to remove the per-resource identifiers. I think they're not
really used by anything anymore. We just need to get around to doing it.

[dullweber, 2017-04-20 13:14:45]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 13:15:02]

Dry run: 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, 2017-04-20 13:23:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-20 13:23:50]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[dullweber, 2017-04-20 13:27:43]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 13:28:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dullweber, 2017-04-20 13:42:37]

On 2017/04/20 00:07:58, raymes wrote:
> We end up polluting a lot of things with the base::Time here which I think
adds
> complexity to the APIs and makes it confusing when the last modified time can
be
> trusted and when it can't.
> 
> It looks like we are only storing the base::Time for user prefs, is that
> correct? Could we get away with only exposing an API on
> ClearSettingsForOneTypeWithPredicate? 
> 
> HostContentSettingsMap::ClearSettingsForOneTypeWithPredicate could take a
> base::Time which is the time after which to clear settings for. That function
> eventually calls into PrefProvider::SetWebsiteSetting -- in fact, even though
it
> loops through the providers, it only ends up modifying that one provider.
Rather
> than looping through the providers, we could call into the PrefProvider
directly
> and pass in the base::Time. The setting would only be updated if the
base::Time
> passed in was prior to the last modified time in the pref provider.
> 
> What do you think of this approach? Any pitfalls I'm missing? It would avoid
> needing to modify content_settings::Rule and friends which I see as a big
> benefit. It would introduce some slight special-casing but in this case I
think
> it would be worth the tradeoff in complexity.
> 
> Thanks!
> 
>
https://codereview.chromium.org/2812113004/diff/60001/chrome/browser/content_...
> File chrome/browser/content_settings/host_content_settings_map_factory.cc
> (right):
> 
>
https://codereview.chromium.org/2812113004/diff/60001/chrome/browser/content_...
> chrome/browser/content_settings/host_content_settings_map_factory.cc:85:
> store_last_modified));
> Is it necessary to pass this in? Can we check it inside the object instead?

Thanks for your suggestions. I removed base::Time from nearly all content
settings APIs and added a special method to the PrefProvider to delete a setting
if it is newer than a specified timestamp. Only the OriginIdentifierValueMap
still knows about timestamps.

[commit-bot: I haz the power, 2017-04-20 14:20:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-20 14:20:33]

Dry run: This issue passed the CQ dry run.

[dullweber, 2017-04-20 15:23:35]

https://codereview.chromium.org/2812113004/diff/60001/chrome/browser/content_...
File chrome/browser/content_settings/host_content_settings_map_factory.cc
(right):

https://codereview.chromium.org/2812113004/diff/60001/chrome/browser/content_...
chrome/browser/content_settings/host_content_settings_map_factory.cc:85:
store_last_modified));
On 2017/04/20 00:07:57, raymes wrote:
> Is it necessary to pass this in? Can we check it inside the object instead?

Whether the map stores these timestamps should depend on the new CBD dialog
because there would be no way to delete these history-like entries without the
new dialog. I need "chrome/common/chrome_features.h" to check the flag, which
isn't available inside the map.

[raymes, 2017-04-24 03:20:23]

Thanks! A bunch of comment from a closer look.

https://codereview.chromium.org/2812113004/diff/100001/chrome/browser/content...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

https://codereview.chromium.org/2812113004/diff/100001/chrome/browser/content...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:117:
PrefProvider pref_content_settings_provider(profile.GetPrefs(), false, true);
nit (here and below): please document these boolean parameters, e.g.  
, true /* param_name */);

If you could document the existing param it would be great as well!

https://codereview.chromium.org/2812113004/diff/100001/chrome/browser/content...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:610:
PrefProvider provider(&prefs, false, /*store_last_modified=*/true);
The usual way to do this is:
true /* store_last_modified */);

https://codereview.chromium.org/2812113004/diff/100001/chrome/browser/content...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/2812113004/diff/100001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1765:
base::Time t2 = base::Time::Now();
I'm not sure if this will be brittle since it relies on timing. A less brittle
way would pass in a base::Clock and that would be used to get the current time.
You can look at the pattern for example in
https://cs.chromium.org/chromium/src/chrome/browser/budget_service/budget_dat...

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
File
components/content_settings/core/browser/content_settings_origin_identifier_value_map.cc
(right):

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_origin_identifier_value_map.cc:178:
entries_[key][patterns].last_modified = last_modified;
This is going to cost 2 lookups which could potentially be significant. You may
want to grab a reference to entries_[key][patterns] and then modify it.

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
File
components/content_settings/core/browser/content_settings_origin_identifier_value_map.h
(right):

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_origin_identifier_value_map.h:107:
const ResourceIdentifier& resource_identifier) const;
Cool, that makes more sense to me.

I was thinking that we might be able to get away with not modifying
OriginIdentifierValueMap since some callsites don't use the last modified
functionality and instead write a wrapper that stores the time:

class OriginIdentifierValueMapWithLastModified { // naming is hard!
  std::unique_ptr<RuleIterator> GetRuleIterator(...);
  void SetValue(...);
  base::Time GetLastModified(...);
  size_t size();
  void Clear();  // Would clear both underlying maps

  std::map<PatternPair, base::Time> times_;
  OriginIdentifierValueMap map_;
};

It would be cleaner, but if you feel that the number of callsites is low (I
think it's ~5-6) and it's not worth it, I will leave it up to you and Martin to
decide.

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_origin_identifier_value_map.h:116:
base::Time last_modified,
const base::Time&

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
File components/content_settings/core/browser/content_settings_pref.cc (right):

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_pref.cc:59: const
std::string& path) {
Do we need to pass this in if it's always kLastModifiedPath?

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_pref.cc:385:
resource_dictionary = new base::DictionaryValue();
nit: () isn't needed

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_pref.cc:406: NULL);
nullptr

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
File components/content_settings/core/browser/content_settings_pref.h (right):

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_pref.h:64: base::Time
begin_time);
const base::Time&

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_pref.h:90: const
base::Time last_modified,
const base::Time&

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
File components/content_settings/core/browser/content_settings_pref_provider.cc
(right):

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:160:
// |begin_time|.
nit: this is in the header so it's not needed here

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
File components/content_settings/core/browser/content_settings_pref_provider.h
(right):

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.h:55:
base::Time begin_time);
const base::Time&

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.h:55:
base::Time begin_time);
Should we add a unittest for this?

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
File components/content_settings/core/browser/host_content_settings_map.cc
(right):

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.cc:665:
DCHECK(!SupportsResourceIdentifier(content_type));
Could this potentially get called for plugins? Won't this fail in that case?

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.cc:677:
pref_provider_->DeleteWebsiteSetting(setting.primary_pattern,
Just checking this approach seems ok with msramek? Maybe we could clarify the
name of this function, e.g.
DeleteWebsiteSettingAfterDate

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
File components/content_settings/core/browser/host_content_settings_map.h
(right):

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.h:230:
base::Time begin_time,
const base::Time&

[dullweber, 2017-04-25 10:48:21]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 10:48:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dullweber, 2017-04-25 10:50:45]

Thanks, I fixed most of the issues you mentioned but I have a question regarding
passing base::Time by value below:

https://codereview.chromium.org/2812113004/diff/100001/chrome/browser/content...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

https://codereview.chromium.org/2812113004/diff/100001/chrome/browser/content...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:117:
PrefProvider pref_content_settings_provider(profile.GetPrefs(), false, true);
On 2017/04/24 03:20:22, raymes wrote:
> nit (here and below): please document these boolean parameters, e.g.  
> , true /* param_name */);
> 
> If you could document the existing param it would be great as well!

Done.

https://codereview.chromium.org/2812113004/diff/100001/chrome/browser/content...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:610:
PrefProvider provider(&prefs, false, /*store_last_modified=*/true);
On 2017/04/24 03:20:22, raymes wrote:
> The usual way to do this is:
> true /* store_last_modified */);

I changed it but do you know if this style is documented somewhere? 
The chromium style guide doesn't mention parameter comments all and the google
style guide suggests the /*comment=*/ style. 

https://google.github.io/styleguide/cppguide.html#Implementation_Comments -->
Function Argument Comments

https://codereview.chromium.org/2812113004/diff/100001/chrome/browser/content...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/2812113004/diff/100001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1765:
base::Time t2 = base::Time::Now();
On 2017/04/24 03:20:22, raymes wrote:
> I'm not sure if this will be brittle since it relies on timing. A less brittle
> way would pass in a base::Clock and that would be used to get the current
time.
> You can look at the pattern for example in
>
https://cs.chromium.org/chromium/src/chrome/browser/budget_service/budget_dat...

I added a Sleep(tiny_timeout) before t2, to make sure that the timestamp for
url1 is before t2. Using Clock would probably be more elegant but that would
mean that I would have to create a setClockForPrefProviderForTesting method in
HostContentSettingsMap and pass it to the PrefProvider because there is no
direct way to access the PrefProvider?

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
File
components/content_settings/core/browser/content_settings_origin_identifier_value_map.cc
(right):

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_origin_identifier_value_map.cc:178:
entries_[key][patterns].last_modified = last_modified;
On 2017/04/24 03:20:22, raymes wrote:
> This is going to cost 2 lookups which could potentially be significant. You
may
> want to grab a reference to entries_[key][patterns] and then modify it.

Done.

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
File
components/content_settings/core/browser/content_settings_origin_identifier_value_map.h
(right):

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_origin_identifier_value_map.h:107:
const ResourceIdentifier& resource_identifier) const;
On 2017/04/24 03:20:22, raymes wrote:
> Cool, that makes more sense to me.
> 
> I was thinking that we might be able to get away with not modifying
> OriginIdentifierValueMap since some callsites don't use the last modified
> functionality and instead write a wrapper that stores the time:
> 
> class OriginIdentifierValueMapWithLastModified { // naming is hard!
>   std::unique_ptr<RuleIterator> GetRuleIterator(...);
>   void SetValue(...);
>   base::Time GetLastModified(...);
>   size_t size();
>   void Clear();  // Would clear both underlying maps
> 
>   std::map<PatternPair, base::Time> times_;
>   OriginIdentifierValueMap map_;
> };
> 
> It would be cleaner, but if you feel that the number of callsites is low (I
> think it's ~5-6) and it's not worth it, I will leave it up to you and Martin
to
> decide.

that sounds like a good idea and I tried to implement it but I noticed that I
have to duplicate basically all the code from set and get methods because I have
to create the keys, check if the map is empty...
The number of callsites that are passing base::Time() is not very high, so I
would suggest to leave it this way or add another method that passes
base::Time() by default.

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_origin_identifier_value_map.h:116:
base::Time last_modified,
On 2017/04/24 03:20:22, raymes wrote:
> const base::Time&

I thought that base::Time and other very small classes like StringPiece can be
passed by value because that is as efficient or even more efficient than passing
them by reference? base::Time is just an int64_t. The value can be passed in a
register while a reference needs to dereferenced from memory.

The current code seems to have about 300 functions with pass by value and 257
with pass by const ref for base::Time:
https://cs.chromium.org/search/?q=%22(base::time+%22&type=cs
https://cs.chromium.org/search/?q=%22,+base::time+%22&type=cs

https://cs.chromium.org/search/?q=%22(const+base::time%26+%22&type=cs
https://cs.chromium.org/search/?q=%22,+const+base::time%26+%22&type=cs

base::TimeDelta is passed more often by value than reference (723 vs 264):
https://cs.chromium.org/search/?q=%22(base::timedelta+%22&type=cs
https://cs.chromium.org/search/?q=%22,+base::timedelta+%22&type=cs

https://cs.chromium.org/search/?q=%22(const+base::timedelta%26+%22&type=cs
https://cs.chromium.org/search/?q=%22,+const+base::timedelta%26+%22&type=cs

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
File components/content_settings/core/browser/content_settings_pref.cc (right):

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_pref.cc:59: const
std::string& path) {
On 2017/04/24 03:20:22, raymes wrote:
> Do we need to pass this in if it's always kLastModifiedPath?

Done.

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_pref.cc:385:
resource_dictionary = new base::DictionaryValue();
On 2017/04/24 03:20:22, raymes wrote:
> nit: () isn't needed

Done.

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_pref.cc:406: NULL);
On 2017/04/24 03:20:22, raymes wrote:
> nullptr

Done, I replaced all NULLs in this file with nullptr.

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
File components/content_settings/core/browser/content_settings_pref_provider.cc
(right):

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.cc:160:
// |begin_time|.
On 2017/04/24 03:20:23, raymes wrote:
> nit: this is in the header so it's not needed here

Done.

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
File components/content_settings/core/browser/content_settings_pref_provider.h
(right):

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.h:55:
base::Time begin_time);
On 2017/04/24 03:20:23, raymes wrote:
> Should we add a unittest for this?

I already added a PrefProviderTest.DeleteWebsiteSetting test that covers this
method. Is that enough? The ContentSettingsPref is currently not tested
separately from the PrefProvider.

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
File components/content_settings/core/browser/host_content_settings_map.cc
(right):

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.cc:665:
DCHECK(!SupportsResourceIdentifier(content_type));
On 2017/04/24 03:20:23, raymes wrote:
> Could this potentially get called for plugins? Won't this fail in that case?

This method is called by
HostContentSettingsMap::ClearSettingsForOneTypeWithPredicate. This method is
currently not called for plugins and the method also doesn't have the
resource_identifier parameter.

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.cc:677:
pref_provider_->DeleteWebsiteSetting(setting.primary_pattern,
On 2017/04/24 03:20:23, raymes wrote:
> Just checking this approach seems ok with msramek? Maybe we could clarify the
> name of this function, e.g.
> DeleteWebsiteSettingAfterDate

I renamed it. Martin is ooo until next week but we discussed this approach
before I implemented it, so I think he is ok with it.

[commit-bot: I haz the power, 2017-04-25 11:33:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-25 11:33:21]

Dry run: This issue passed the CQ dry run.

[raymes, 2017-04-26 01:54:17]

Thanks!

https://codereview.chromium.org/2812113004/diff/100001/chrome/browser/content...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

https://codereview.chromium.org/2812113004/diff/100001/chrome/browser/content...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:610:
PrefProvider provider(&prefs, false, /*store_last_modified=*/true);
On 2017/04/25 10:50:44, dullweber wrote:
> On 2017/04/24 03:20:22, raymes wrote:
> > The usual way to do this is:
> > true /* store_last_modified */);
> 
> I changed it but do you know if this style is documented somewhere? 
> The chromium style guide doesn't mention parameter comments all and the google
> style guide suggests the /*comment=*/ style. 
> 
> https://google.github.io/styleguide/cppguide.html#Implementation_Comments -->
> Function Argument Comments

Ah I actually just found out today that the style you had is now considered
preferable. The reason is because clang will check it. I'm sorry about that, my
experience on Chrome has always been to use the style I suggested.

Don't feel you need to change this now, it's already a style that's used in many
places.

https://codereview.chromium.org/2812113004/diff/100001/chrome/browser/content...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/2812113004/diff/100001/chrome/browser/content...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1765:
base::Time t2 = base::Time::Now();
On 2017/04/25 10:50:44, dullweber wrote:
> On 2017/04/24 03:20:22, raymes wrote:
> > I'm not sure if this will be brittle since it relies on timing. A less
brittle
> > way would pass in a base::Clock and that would be used to get the current
> time.
> > You can look at the pattern for example in
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/budget_service/budget_dat...
> 
> I added a Sleep(tiny_timeout) before t2, to make sure that the timestamp for
> url1 is before t2. Using Clock would probably be more elegant but that would
> mean that I would have to create a setClockForPrefProviderForTesting method in
> HostContentSettingsMap and pass it to the PrefProvider because there is no
> direct way to access the PrefProvider?

Yep. I think that's what we had before (there used to be a clock in
PrefProvider). I think the approach you have is ok though, it does seem to be
used in some other tests.

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
File
components/content_settings/core/browser/content_settings_origin_identifier_value_map.h
(right):

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_origin_identifier_value_map.h:107:
const ResourceIdentifier& resource_identifier) const;
On 2017/04/25 10:50:44, dullweber wrote:
> On 2017/04/24 03:20:22, raymes wrote:
> > Cool, that makes more sense to me.
> > 
> > I was thinking that we might be able to get away with not modifying
> > OriginIdentifierValueMap since some callsites don't use the last modified
> > functionality and instead write a wrapper that stores the time:
> > 
> > class OriginIdentifierValueMapWithLastModified { // naming is hard!
> >   std::unique_ptr<RuleIterator> GetRuleIterator(...);
> >   void SetValue(...);
> >   base::Time GetLastModified(...);
> >   size_t size();
> >   void Clear();  // Would clear both underlying maps
> > 
> >   std::map<PatternPair, base::Time> times_;
> >   OriginIdentifierValueMap map_;
> > };
> > 
> > It would be cleaner, but if you feel that the number of callsites is low (I
> > think it's ~5-6) and it's not worth it, I will leave it up to you and Martin
> to
> > decide.
> 
> that sounds like a good idea and I tried to implement it but I noticed that I
> have to duplicate basically all the code from set and get methods because I
have
> to create the keys, check if the map is empty...
> The number of callsites that are passing base::Time() is not very high, so I
> would suggest to leave it this way or add another method that passes
> base::Time() by default.

Ah that makes sense. Thanks for looking at it. Perhaps adding another function
is slightly preferable, but I don't feel particularly strongly so I'll leave it
up to you.

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_origin_identifier_value_map.h:116:
base::Time last_modified,
On 2017/04/25 10:50:44, dullweber wrote:
> On 2017/04/24 03:20:22, raymes wrote:
> > const base::Time&
> 
> I thought that base::Time and other very small classes like StringPiece can be
> passed by value because that is as efficient or even more efficient than
passing
> them by reference? base::Time is just an int64_t. The value can be passed in a
> register while a reference needs to dereferenced from memory.
> 
> The current code seems to have about 300 functions with pass by value and 257
> with pass by const ref for base::Time:
> https://cs.chromium.org/search/?q=%22(base::time+%22&type=cs
> https://cs.chromium.org/search/?q=%22,+base::time+%22&type=cs
> 
> https://cs.chromium.org/search/?q=%22(const+base::time%26+%22&type=cs
> https://cs.chromium.org/search/?q=%22,+const+base::time%26+%22&type=cs
> 
> base::TimeDelta is passed more often by value than reference (723 vs 264):
> https://cs.chromium.org/search/?q=%22(base::timedelta+%22&type=cs
> https://cs.chromium.org/search/?q=%22,+base::timedelta+%22&type=cs
> 
> https://cs.chromium.org/search/?q=%22(const+base::timedelta%26+%22&type=cs
> https://cs.chromium.org/search/?q=%22,+const+base::timedelta%26+%22&type=cs

Thanks for looking at it. I think it's fine to pass by value in that case. In my
mind I just default to suggesting const& for all user-defined data types because
they are almost always bigger than a reference. But in this case it probably
doesn't matter.

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
File components/content_settings/core/browser/content_settings_pref_provider.h
(right):

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
components/content_settings/core/browser/content_settings_pref_provider.h:55:
base::Time begin_time);
On 2017/04/25 10:50:44, dullweber wrote:
> On 2017/04/24 03:20:23, raymes wrote:
> > Should we add a unittest for this?
> 
> I already added a PrefProviderTest.DeleteWebsiteSetting test that covers this
> method. Is that enough? The ContentSettingsPref is currently not tested
> separately from the PrefProvider.

Ah sorry - I missed that :)

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
File components/content_settings/core/browser/host_content_settings_map.cc
(right):

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.cc:665:
DCHECK(!SupportsResourceIdentifier(content_type));
On 2017/04/25 10:50:44, dullweber wrote:
> On 2017/04/24 03:20:23, raymes wrote:
> > Could this potentially get called for plugins? Won't this fail in that case?
> 
> This method is called by
> HostContentSettingsMap::ClearSettingsForOneTypeWithPredicate. This method is
> currently not called for plugins and the method also doesn't have the
> resource_identifier parameter.

But won't we be calling this for plugins when we implement clearing Site
Settings? 

The resource identifier gives you a subset of the Plugins content setting. So
clearing it still makes sense - it should just clear all resource identifiers.

https://codereview.chromium.org/2812113004/diff/140001/chrome/browser/content...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

https://codereview.chromium.org/2812113004/diff/140001/chrome/browser/content...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:675: 
Should we test clearing settings for all-time?

[dullweber, 2017-04-26 11:01:41]

I added support for deletion with resource-identifier, thanks!

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
File components/content_settings/core/browser/host_content_settings_map.cc
(right):

https://codereview.chromium.org/2812113004/diff/100001/components/content_set...
components/content_settings/core/browser/host_content_settings_map.cc:665:
DCHECK(!SupportsResourceIdentifier(content_type));
On 2017/04/26 01:54:17, raymes wrote:
> On 2017/04/25 10:50:44, dullweber wrote:
> > On 2017/04/24 03:20:23, raymes wrote:
> > > Could this potentially get called for plugins? Won't this fail in that
case?
> > 
> > This method is called by
> > HostContentSettingsMap::ClearSettingsForOneTypeWithPredicate. This method is
> > currently not called for plugins and the method also doesn't have the
> > resource_identifier parameter.
> 
> But won't we be calling this for plugins when we implement clearing Site
> Settings? 
> 
> The resource identifier gives you a subset of the Plugins content setting. So
> clearing it still makes sense - it should just clear all resource identifiers.

That's true. I changed the PrefProvider to support deletion with resource
identifier. This method probably needs some changes too because it calls
GetSettingsForOneType with std::strings() as resource which doesn't return
settings with a resource but I think that can be fixed when I actually use it to
delete settings.

https://codereview.chromium.org/2812113004/diff/140001/chrome/browser/content...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

https://codereview.chromium.org/2812113004/diff/140001/chrome/browser/content...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:675: 
On 2017/04/26 01:54:17, raymes wrote:
> Should we test clearing settings for all-time?

I added another deletion call for all-time and an additional test for deletion
with resource identifier.

[dullweber, 2017-04-26 11:01:46]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 11:01:58]

Dry run: 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, 2017-04-26 12:39:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 12:39:06]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[dullweber, 2017-04-26 12:43:07]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 12:43:20]

Dry run: 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, 2017-04-26 13:31:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 13:31:48]

Dry run: This issue passed the CQ dry run.

[dullweber, 2017-04-26 16:01:26]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 16:02:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dullweber, 2017-04-26 16:05:18]

I started to implement the Site Settings option for CBD and noticed that the
current design doesn't allow to count the number of settings that were modified
after a specific date. We need this number to show the user how many settings
are affected by a deletion.
I added a PrefProvider::GetWebsiteSettingLastModified method to give direct
access to the date. This method makes
PrefProvider::DeleteWebsiteSettingAfterDate obsolete, so I removed it.
The only alternative I could think of would be a
WasWebsiteSettingModifiedAfterDate() method but that sounds even weirder than
DeleteWebsiteSettingAfterDate :D
The PrefProvider test now only checks that the date is correctly modified but
the HostContentSettingsMap also has a test that checks the deletion with
specific times.
What do you think of this?

[commit-bot: I haz the power, 2017-04-26 17:03:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 17:03:06]

Dry run: This issue passed the CQ dry run.

[raymes, 2017-04-27 06:42:05]

On 2017/04/26 16:05:18, dullweber wrote:
> I started to implement the Site Settings option for CBD and noticed that the
> current design doesn't allow to count the number of settings that were
modified
> after a specific date. We need this number to show the user how many settings
> are affected by a deletion.
> I added a PrefProvider::GetWebsiteSettingLastModified method to give direct
> access to the date. This method makes
> PrefProvider::DeleteWebsiteSettingAfterDate obsolete, so I removed it.
> The only alternative I could think of would be a
> WasWebsiteSettingModifiedAfterDate() method but that sounds even weirder than
> DeleteWebsiteSettingAfterDate :D
> The PrefProvider test now only checks that the date is correctly modified but
> the HostContentSettingsMap also has a test that checks the deletion with
> specific times.
> What do you think of this?

This approach seems good to me :) Thanks for the iteration.

I'm still a bit confused  about the plan for resource identifiers - it looks
like they will mostly work now? That's not actually what I was suggesting
(though it's not a major problem either). Sorry if I didn't explain myself well.

Basically plugins don't really store a resource identifier anymore. They just
always use std::string(). So we don't really need to store or clear the last
used date. But we do need to make sure that when plugins is used without a
resource identifier that it can be cleared correctly. Does that make sense?

[dullweber, 2017-04-27 08:30:21]

On 2017/04/27 06:42:05, raymes wrote:
> On 2017/04/26 16:05:18, dullweber wrote:
> > I started to implement the Site Settings option for CBD and noticed that the
> > current design doesn't allow to count the number of settings that were
> modified
> > after a specific date. We need this number to show the user how many
settings
> > are affected by a deletion.
> > I added a PrefProvider::GetWebsiteSettingLastModified method to give direct
> > access to the date. This method makes
> > PrefProvider::DeleteWebsiteSettingAfterDate obsolete, so I removed it.
> > The only alternative I could think of would be a
> > WasWebsiteSettingModifiedAfterDate() method but that sounds even weirder
than
> > DeleteWebsiteSettingAfterDate :D
> > The PrefProvider test now only checks that the date is correctly modified
but
> > the HostContentSettingsMap also has a test that checks the deletion with
> > specific times.
> > What do you think of this?
> 
> This approach seems good to me :) Thanks for the iteration.
> 
> I'm still a bit confused  about the plan for resource identifiers - it looks
> like they will mostly work now? That's not actually what I was suggesting
> (though it's not a major problem either). Sorry if I didn't explain myself
well.
> 
> Basically plugins don't really store a resource identifier anymore. They just
> always use std::string(). So we don't really need to store or clear the last
> used date. But we do need to make sure that when plugins is used without a
> resource identifier that it can be cleared correctly. Does that make sense?

Ah, I understand. Then everything should already work fine now. 
I thought that there is still a resource identifier for flash that is not
std::string(). 
I already only store a single timestamp for all settings with the same patterns
but different resources but that shouldn't matter if the Resourceidentifier is
always empty.

[raymes, 2017-04-28 02:27:00]

lgtm

Ah I see I was misunderstanding the code before. Thanks!

https://codereview.chromium.org/2812113004/diff/200001/chrome/browser/content...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

https://codereview.chromium.org/2812113004/diff/200001/chrome/browser/content...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:638:
// before |t|.
nit: I think this comment is out of date?

https://codereview.chromium.org/2812113004/diff/200001/chrome/browser/content...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:651:
}
Are these braces needed?

[dullweber, 2017-04-28 08:46:32]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 08:46:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dullweber, 2017-04-28 08:46:57]

The CQ bit was unchecked by dullweber@chromium.org

[dullweber, 2017-04-28 08:49:57]

Thanks!

https://codereview.chromium.org/2812113004/diff/200001/chrome/browser/content...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

https://codereview.chromium.org/2812113004/diff/200001/chrome/browser/content...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:638:
// before |t|.
On 2017/04/28 02:27:00, raymes wrote:
> nit: I think this comment is out of date?

yes, I removed it

https://codereview.chromium.org/2812113004/diff/200001/chrome/browser/content...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:651:
}
On 2017/04/28 02:27:00, raymes wrote:
> Are these braces needed?

No, not really anymore, removed.

[dullweber, 2017-04-28 09:02:25]

dullweber@chromium.org changed reviewers:
+ droger@chromium.org, jialiul@chromium.org, msarda@chromium.org,
palmer@chromium.org, reillyg@chromium.org

[dullweber, 2017-04-28 09:02:26]

Hello,
please review the following files with minor changes due to an api change in
HostContentSettingsMap:

reillyg@chromium.org
 chrome/browser/extensions/api/content_settings/content_settings_store.cc

palmer@chromium.org
 chrome/browser/ssl/chrome_ssl_host_state_delegate.cc

msarda@chromium.org
 components/signin/core/browser/signin_header_helper_unittest.cc
 components/signin/ios/browser/account_consistency_service_unittest.mm

jialiul@chromium.org
 components/safe_browsing/password_protection/password_protection_service.cc

components/safe_browsing/password_protection/password_protection_service_unittest.cc

droger@chromium.org
 ios/chrome/browser/content_settings/host_content_settings_map_factory.cc

[msarda, 2017-04-28 11:02:15]

components/signin/ changes LGTM.

[droger, 2017-04-28 12:40:27]

ios lgtm

[Reilly Grant (use Gerrit), 2017-04-28 15:57:20]

chrome/browser/extensions lgtm with a request for documentation

https://codereview.chromium.org/2812113004/diff/220001/components/content_set...
File
components/content_settings/core/browser/content_settings_origin_identifier_value_map.h
(right):

https://codereview.chromium.org/2812113004/diff/220001/components/content_set...
components/content_settings/core/browser/content_settings_origin_identifier_value_map.h:111:
// of the passed |value|.
Please add an explanation of the |last_modified| parameter here.

[Jialiu Lin, 2017-04-28 17:54:18]

components/safe_browsing/  LGTM

[palmer, 2017-04-28 19:03:33]

chrome/browser/ssl LGTM

[msramek, 2017-05-03 10:43:32]

LGTM.

I only took a quick pass on content_settings/ now, since Raymes did a detailed
review.

Regarding the website settings deletion in browsing_data/, I agree with just
using base::Time() as a no-op now. But we should investigate whether it makes
sense to start doing partial deletions for them as well. Their CBD datatypes
(history / cookies and site data) already support timestamps.

[dullweber, 2017-05-03 10:45:31]

The CQ bit was checked by dullweber@chromium.org

[dullweber, 2017-05-03 10:45:32]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org,
droger@chromium.org, reillyg@chromium.org, jialiul@chromium.org,
palmer@chromium.org, msarda@chromium.org
Link to the patchset: https://codereview.chromium.org/2812113004/#ps240001
(title: "mention last_modified in OriginIdentifierValueMap comment")

[commit-bot: I haz the power, 2017-05-03 10:45:48]

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, 2017-05-03 10:49:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 10:49:06]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dullweber, 2017-05-03 12:03:21]

The CQ bit was checked by dullweber@chromium.org

[dullweber, 2017-05-03 12:03:21]

The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org,
msramek@chromium.org, reillyg@chromium.org, raymes@chromium.org,
jialiul@chromium.org, palmer@chromium.org, msarda@chromium.org
Link to the patchset: https://codereview.chromium.org/2812113004/#ps260001
(title: "rebase")

[commit-bot: I haz the power, 2017-05-03 12:03:37]

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, 2017-05-03 12:54:28]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1493813001074250,
"parent_rev": "849f7eeb1a1e7402f69f133c50bc2167d40413a8", "commit_rev":
"39d6d5f89a382f3f6f23ae80a9c9c87c2f184371"}

[commit-bot: I haz the power, 2017-05-03 12:54:36]

Description was changed from

==========
Write last_modified date to Content Settings in the PrefProvider

Introduce a last_modified timestamp for content settings stored inside the
PrefProvider. 
This timestamp will enable deleting content-settings by a selected time range
from the clear browsing data dialog. 
The timestamp will only be recorded if the tabs-in-cbd flag is activated because
otherwise there would be no way to delete these history-like entries.

BUG=681523
==========

to

==========
Write last_modified date to Content Settings in the PrefProvider

Introduce a last_modified timestamp for content settings stored inside the
PrefProvider. 
This timestamp will enable deleting content-settings by a selected time range
from the clear browsing data dialog. 
The timestamp will only be recorded if the tabs-in-cbd flag is activated because
otherwise there would be no way to delete these history-like entries.

BUG=681523

Review-Url: https://codereview.chromium.org/2812113004
Cr-Commit-Position: refs/heads/master@{#468950}
Committed:
https://chromium.googlesource.com/chromium/src/+/39d6d5f89a382f3f6f23ae80a9c9...
==========

[commit-bot: I haz the power, 2017-05-03 12:54:38]

Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/39d6d5f89a382f3f6f23ae80a9c9...