fix partially broken patterns, delete totally brocken patterns

Fix partially broken pattern keys and delete invalid pattern keys of the obsolete content settings pattern pref.

BUG=90490
TEST=content_settings_pref_provider_unittest.cc

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95991

[markusheintz_, 2011-08-02 11:03:12]

Please review my CL.

[Bernhard Bauer, 2011-08-02 11:32:31]

http://codereview.chromium.org/7529025/diff/4001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7529025/diff/4001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:979:
exceptions_dictionary = update.Get();
Declare |exceptions_dictionary| in this line?

http://codereview.chromium.org/7529025/diff/4001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:996: //
TODO(markusheintz): Delete the "!pattern_pair.second.IsValid()".
?

http://codereview.chromium.org/7529025/diff/4001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:999:
!(pattern_pair.second == ContentSettingsPattern::Wildcard()))) {
|pattern_pair.second != ContentSettingsPattern::Wildcard()|?

http://codereview.chromium.org/7529025/diff/4001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:1003: if
(all_settings_dictionary->HasKey(
Can you fit this on one line?

http://codereview.chromium.org/7529025/diff/4001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:1011: // the
valule of previous keys.
Nit: "value"

http://codereview.chromium.org/7529025/diff/4001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:1033: for
(std::list<StringPair>::iterator i(keys_to_swap.begin());
Why can't you do this directly in the loop above?

http://codereview.chromium.org/7529025/diff/4001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:1039:
pattern_str_pair.first, &dict);
Nit: indent

[markusheintz_, 2011-08-02 12:24:53]

http://codereview.chromium.org/7529025/diff/4001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7529025/diff/4001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:979:
exceptions_dictionary = update.Get();
On 2011/08/02 11:32:32, Bernhard Bauer wrote:
> Declare |exceptions_dictionary| in this line?

Absolutely.

http://codereview.chromium.org/7529025/diff/4001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:996: //
TODO(markusheintz): Delete the "!pattern_pair.second.IsValid()".
On 2011/08/02 11:32:32, Bernhard Bauer wrote:
> ?

Removed

http://codereview.chromium.org/7529025/diff/4001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:999:
!(pattern_pair.second == ContentSettingsPattern::Wildcard()))) {
On 2011/08/02 11:32:32, Bernhard Bauer wrote:
> |pattern_pair.second != ContentSettingsPattern::Wildcard()|?

ContentSettingsPattern does not have a != operator. If I recall that correctly
you didn't like the != operator. Sorry if my memory is blurry here.

http://codereview.chromium.org/7529025/diff/4001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:1003: if
(all_settings_dictionary->HasKey(
On 2011/08/02 11:32:32, Bernhard Bauer wrote:
> Can you fit this on one line?
 yep. Done.

http://codereview.chromium.org/7529025/diff/4001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:1011: // the
valule of previous keys.
On 2011/08/02 11:32:32, Bernhard Bauer wrote:
> Nit: "value"

Done.

http://codereview.chromium.org/7529025/diff/4001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:1033: for
(std::list<StringPair>::iterator i(keys_to_swap.begin());
On 2011/08/02 11:32:32, Bernhard Bauer wrote:
> Why can't you do this directly in the loop above?

Last time I tried that the iterator didn't like it if I remember correctly.

http://codereview.chromium.org/7529025/diff/4001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:1039:
pattern_str_pair.first, &dict);
On 2011/08/02 11:32:32, Bernhard Bauer wrote:
> Nit: indent

Done.

[Bernhard Bauer, 2011-08-02 12:36:40]

http://codereview.chromium.org/7529025/diff/4001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7529025/diff/4001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:1033: for
(std::list<StringPair>::iterator i(keys_to_swap.begin());
On 2011/08/02 12:24:53, markusheintz_ wrote:
> On 2011/08/02 11:32:32, Bernhard Bauer wrote:
> > Why can't you do this directly in the loop above?
> 
> Last time I tried that the iterator didn't like it if I remember correctly.

Ah, right.

I think you could still copy the new value to |exceptions_dictionary| (if a
value doesn't already exist for the key), and just keep a set of keys to remove.

http://codereview.chromium.org/7529025/diff/4004/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:997: if
(pattern_pair.first.IsValid() &&
Also, an invalid pattern shouldn't compare equal to a wildcard pattern, so it
seems you could remove the |!pattern_pair.second.IsValid()|.

[markusheintz_, 2011-08-02 13:02:56]

http://codereview.chromium.org/7529025/diff/4004/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7529025/diff/4004/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:997:
!(pattern_pair.second == ContentSettingsPattern::Wildcard()))) {
On 2011/08/02 12:36:40, Bernhard Bauer wrote:
> Also, an invalid pattern shouldn't compare equal to a wildcard pattern, so it
> seems you could remove the |!pattern_pair.second.IsValid()|.

Yep!

[markusheintz_, 2011-08-03 08:27:05]

Addressed all comments. Pls take another look.

[Bernhard Bauer, 2011-08-03 08:33:32]

http://codereview.chromium.org/7529025/diff/4001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7529025/diff/4001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:1033: for
(std::list<StringPair>::iterator i(keys_to_swap.begin());
On 2011/08/02 12:36:40, Bernhard Bauer wrote:
> On 2011/08/02 12:24:53, markusheintz_ wrote:
> > On 2011/08/02 11:32:32, Bernhard Bauer wrote:
> > > Why can't you do this directly in the loop above?
> > 
> > Last time I tried that the iterator didn't like it if I remember correctly.
> 
> Ah, right.
> 
> I think you could still copy the new value to |exceptions_dictionary| (if a
> value doesn't already exist for the key), and just keep a set of keys to
remove.

What about this one?

[markusheintz_, 2011-08-03 11:23:40]

I fixed the bug you pointed out offline and added a test for it. Pls take
another look at it.

http://codereview.chromium.org/7529025/diff/4001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7529025/diff/4001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:1033: for
(std::list<StringPair>::iterator i(keys_to_swap.begin());
On 2011/08/03 08:33:33, Bernhard Bauer wrote:
> On 2011/08/02 12:36:40, Bernhard Bauer wrote:
> > On 2011/08/02 12:24:53, markusheintz_ wrote:
> > > On 2011/08/02 11:32:32, Bernhard Bauer wrote:
> > > > Why can't you do this directly in the loop above?
> > > 
> > > Last time I tried that the iterator didn't like it if I remember
correctly.
> > 
> > Ah, right.
> > 
> > I think you could still copy the new value to |exceptions_dictionary| (if a
> > value doesn't already exist for the key), and just keep a set of keys to
> remove.
> 
> What about this one?

I shouldn't add new keys while I'm iterating over it.

[Bernhard Bauer, 2011-08-03 14:53:41]

http://codereview.chromium.org/7529025/diff/14001/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7529025/diff/14001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pref_provider.cc:991:
primary_pattern = ContentSettingsPattern::FromString(key);
You could actually simplify this to always use |key.substr(0, sep_pos)|.

[markusheintz_, 2011-08-03 21:42:28]

I removed the keys_to_remove list like discussed offline.

http://codereview.chromium.org/7529025/diff/14001/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7529025/diff/14001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pref_provider.cc:991:
primary_pattern = ContentSettingsPattern::FromString(key);
On 2011/08/03 14:53:41, Bernhard Bauer wrote:
> You could actually simplify this to always use |key.substr(0, sep_pos)|.

Done.

[Bernhard Bauer, 2011-08-04 08:44:03]

LGTM!

http://codereview.chromium.org/7529025/diff/18001/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7529025/diff/18001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pref_provider.cc:1034: Value*
dict;
Initialize please :-)

http://codereview.chromium.org/7529025/diff/18001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pref_provider.cc:1039: delete
dict;
It sucks that we have to delete |dict| manually. RemoveWithoutPathExpansion
should totally take a *scoped_ptr<> as its second argument ;-)

[markusheintz_, 2011-08-04 12:13:38]

Thanks for reviewing.

http://codereview.chromium.org/7529025/diff/18001/chrome/browser/content_sett...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7529025/diff/18001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pref_provider.cc:1034: Value*
dict;
On 2011/08/04 08:44:03, Bernhard Bauer wrote:
> Initialize please :-)

Done.

http://codereview.chromium.org/7529025/diff/18001/chrome/browser/content_sett...
chrome/browser/content_settings/content_settings_pref_provider.cc:1039: delete
dict;
On 2011/08/04 08:44:03, Bernhard Bauer wrote:
> It sucks that we have to delete |dict| manually. RemoveWithoutPathExpansion
> should totally take a *scoped_ptr<> as its second argument ;-)

I totally support you on that.

I guess I better put the pointer into a scoped_ptr after calling
RemoveWithout...