Update content setting for app banners to store more information.

Update content setting for app banners to store more information.

This changes the app banner content setting from being a dictionary of
app key to bool, to being a dictionary of app key to dictionary.

The sub dictionary contains one bool for whether the banner has been
blocked, and also a list of dates. The list of dates will contain at
most 14 dates, and will be used to record distinct dates on which the
banner could have been shown.

This is to allow more complex triggering. The goal is to actually show
banners if they could otherwise have been shown on a previous date in
the last 14 days.

Note the complex triggering is not currently used.

BUG=452825
Committed: https://crrev.com/1fa5a3081a04c314c2dcdf8afc6b9d951be13493
Cr-Commit-Position: refs/heads/master@{#314266}

[benwells, 2015-01-29 08:02:23]

benwells@chromium.org changed reviewers:
+ bauerb@chromium.org, dfalcantara@chromium.org

[benwells, 2015-01-29 08:02:24]

This isn't ready to land yet - it desperately needs tests - but I want to find
out if this is a doomed approach.

Can we store this complex data in content settings? If we don't do it there, I
think it will have to be in a preference somewhere else, so I don't think it is
really adding much overhead being in a content setting vs. a preference; but I'm
not so sure.

[Bernhard Bauer, 2015-01-29 16:49:36]

On 2015/01/29 08:02:24, benwells wrote:
> This isn't ready to land yet - it desperately needs tests - but I want to find
> out if this is a doomed approach.
> 
> Can we store this complex data in content settings? If we don't do it there, I
> think it will have to be in a preference somewhere else, so I don't think it
is
> really adding much overhead being in a content setting vs. a preference; but
I'm
> not so sure.

Storing complex data in content settings is absolutely fine; in fact, you are
already storing data that is more complex than an ALLOW or BLOCK value.

One thing that you might have to do though is to migrate old dictionary entries
to the new format. We usually keep the migration code in for a couple of
releases (or longer in practice, because people tend to forget about it... :) ),
then remove it.

https://codereview.chromium.org/884373002/diff/20001/chrome/browser/android/b...
File chrome/browser/android/banners/app_banner_settings_helper.cc (right):

https://codereview.chromium.org/884373002/diff/20001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_settings_helper.cc:20: //
DictionaryValue doesn't allow '.' in the keys. So, first get rid of spaces
Yes, it does :) You can use SetWithoutPathExpansion().

https://codereview.chromium.org/884373002/diff/20001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_settings_helper.cc:27: // Max number
of apps (including web native apps) that a particular site may
What is a web native app? Does that mean Fizz?

https://codereview.chromium.org/884373002/diff/20001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_settings_helper.cc:42:
time.LocalExplode(&exploded);
If we are using local time here, there could of course be weird edge cases with
time zone changes, but I guess we don't care about that, as the 14 days are more
of a heuristic anyway?

https://codereview.chromium.org/884373002/diff/20001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_settings_helper.cc:69: return
scoped_ptr<base::DictionaryValue>(
You can use make_scoped_ptr() here, which is a bit less to type.

https://codereview.chromium.org/884373002/diff/20001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_settings_helper.cc:80:
origin_dict->Set(key_name, scoped_ptr<base::DictionaryValue>(app_dict));
Same here, just use make_scoped_ptr.

https://codereview.chromium.org/884373002/diff/20001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_settings_helper.cc:129:
DCHECK((*it)->GetAsDouble(&internal_date));
This will be compiled out in a Release build! The correct way to do this is to
set a success boolean value, then DCHECK that.

[benwells, 2015-01-30 06:27:02]

Feedback address, tests added, and many bugs fixed.

This content setting isn't used anywhere but android yet, but I've made it
available everywhere to make testing easy. As we'll be using the content setting
on desktop soon (m43 ish) this seems fine, but let me know if it is a problem.

https://codereview.chromium.org/884373002/diff/20001/chrome/browser/android/b...
File chrome/browser/android/banners/app_banner_settings_helper.cc (right):

https://codereview.chromium.org/884373002/diff/20001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_settings_helper.cc:20: //
DictionaryValue doesn't allow '.' in the keys. So, first get rid of spaces
On 2015/01/29 16:49:36, Bernhard Bauer wrote:
> Yes, it does :) You can use SetWithoutPathExpansion().

Done.

https://codereview.chromium.org/884373002/diff/20001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_settings_helper.cc:27: // Max number
of apps (including web native apps) that a particular site may
On 2015/01/29 16:49:35, Bernhard Bauer wrote:
> What is a web native app? Does that mean Fizz?

Yes it means Fizz. I'll explain better.

https://codereview.chromium.org/884373002/diff/20001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_settings_helper.cc:42:
time.LocalExplode(&exploded);
On 2015/01/29 16:49:36, Bernhard Bauer wrote:
> If we are using local time here, there could of course be weird edge cases
with
> time zone changes, but I guess we don't care about that, as the 14 days are
more
> of a heuristic anyway?

Yep that's right, its a heuristic. I think the same edge cases exist with UTC
time and someone changing their clock.

I think local time works better for the heuristics, which are about having the
banner appear if a site is visited on different days. This will feel more
predictable if the days are local days.

https://codereview.chromium.org/884373002/diff/20001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_settings_helper.cc:69: return
scoped_ptr<base::DictionaryValue>(
On 2015/01/29 16:49:35, Bernhard Bauer wrote:
> You can use make_scoped_ptr() here, which is a bit less to type.

nice, thanks. I think I've used it everywhere I can.

https://codereview.chromium.org/884373002/diff/20001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_settings_helper.cc:80:
origin_dict->Set(key_name, scoped_ptr<base::DictionaryValue>(app_dict));
On 2015/01/29 16:49:36, Bernhard Bauer wrote:
> Same here, just use make_scoped_ptr.

Done.

https://codereview.chromium.org/884373002/diff/20001/chrome/browser/android/b...
chrome/browser/android/banners/app_banner_settings_helper.cc:129:
DCHECK((*it)->GetAsDouble(&internal_date));
On 2015/01/29 16:49:35, Bernhard Bauer wrote:
> This will be compiled out in a Release build! The correct way to do this is to
> set a success boolean value, then DCHECK that.

Ah, of course! Thanks for finding that. I don't think the DCHECK is really
necessary anyway, I just just being paranoid.

[Bernhard Bauer, 2015-01-30 13:25:26]

So, what do we do about migrating old settings?

https://codereview.chromium.org/884373002/diff/80001/chrome/browser/banners/a...
File chrome/browser/banners/app_banner_settings_helper.cc (right):

https://codereview.chromium.org/884373002/diff/80001/chrome/browser/banners/a...
chrome/browser/banners/app_banner_settings_helper.cc:120: if (other_date ==
date)
Another problematic edge case: If the value from content settings was written
while the computer was in a different time zone, the times here will not match,
even if they were on the same day. Maybe we need to store the times "raw", and
only round back to midnight when comparing?

https://codereview.chromium.org/884373002/diff/80001/chrome/browser/banners/a...
chrome/browser/banners/app_banner_settings_helper.cc:168: for
(base::ValueVector::iterator it = could_show_list->begin();
Use a C++11-style for-loop?

https://codereview.chromium.org/884373002/diff/80001/chrome/browser/banners/a...
File chrome/browser/banners/app_banner_settings_helper.h (right):

https://codereview.chromium.org/884373002/diff/80001/chrome/browser/banners/a...
chrome/browser/banners/app_banner_settings_helper.h:52:
DISALLOW_COPY_AND_ASSIGN(AppBannerSettingsHelper);
You probably also want to disallow the empty constructor. You can combine this
by using DISALLOW_IMPLICIT_CONSTRCUTORS(...).

[benwells, 2015-01-30 20:47:28]

This feature hasn't been released yet, it has never been enabled by default.
Given that I haven't been too worried about data migration: any old data will be
treated as invalid and ignored.

https://codereview.chromium.org/884373002/diff/80001/chrome/browser/banners/a...
File chrome/browser/banners/app_banner_settings_helper.cc (right):

https://codereview.chromium.org/884373002/diff/80001/chrome/browser/banners/a...
chrome/browser/banners/app_banner_settings_helper.cc:120: if (other_date ==
date)
On 2015/01/30 13:25:25, Bernhard Bauer wrote:
> Another problematic edge case: If the value from content settings was written
> while the computer was in a different time zone, the times here will not
match,
> even if they were on the same day. Maybe we need to store the times "raw", and
> only round back to midnight when comparing?

Good idea. Done.

https://codereview.chromium.org/884373002/diff/80001/chrome/browser/banners/a...
chrome/browser/banners/app_banner_settings_helper.cc:168: for
(base::ValueVector::iterator it = could_show_list->begin();
On 2015/01/30 13:25:26, Bernhard Bauer wrote:
> Use a C++11-style for-loop?

Done.

https://codereview.chromium.org/884373002/diff/80001/chrome/browser/banners/a...
File chrome/browser/banners/app_banner_settings_helper.h (right):

https://codereview.chromium.org/884373002/diff/80001/chrome/browser/banners/a...
chrome/browser/banners/app_banner_settings_helper.h:52:
DISALLOW_COPY_AND_ASSIGN(AppBannerSettingsHelper);
On 2015/01/30 13:25:26, Bernhard Bauer wrote:
> You probably also want to disallow the empty constructor. You can combine this
> by using DISALLOW_IMPLICIT_CONSTRCUTORS(...).

Done. This should probably stop being a class at some point, but I might save
that refactoring for another CL.

[Bernhard Bauer, 2015-02-02 09:35:40]

On 2015/01/30 20:47:28, benwells wrote:
> This feature hasn't been released yet, it has never been enabled by default.
> Given that I haven't been too worried about data migration: any old data will
be
> treated as invalid and ignored.

Gotcha. LGTM!

[benwells, 2015-02-02 16:36:01]

benwells@chromium.org changed reviewers:
+ thestig@chromium.org

[benwells, 2015-02-02 16:36:02]

+thestig for addition of new folder and owners file.

ping dfalcantara for review, if you want to have a look.

There will be more stuff moving into this folder over time.

[gone, 2015-02-02 17:54:02]

lgtm on my end

[Lei Zhang, 2015-02-02 19:50:13]

Is the goal to bring the banners code into desktop Chrome?

https://chromiumcodereview.appspot.com/884373002/diff/180001/chrome/browser/b...
File chrome/browser/banners/app_banner_settings_helper.h (right):

https://chromiumcodereview.appspot.com/884373002/diff/180001/chrome/browser/b...
chrome/browser/banners/app_banner_settings_helper.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
It's 2015

[benwells, 2015-02-03 02:33:45]

For M43 we'd like to have the app banner functionality in desktop chrome. The
triggering logic should be the same as android, so I expect this stuff, and most
of app_banner_manager.* to move. The app_banner_manager code will need a
delegate introduced before it can be moved, to abstract out showing the actual
UI.

The primary goal right now is to implement the triggering logic described in the
bug. We've been asked to try and get this in for M42. While doing it I want to
add as much testing as I can, so I moved this logic over to desktop now to
facilitate testing.

Let me know if you need more info.

https://codereview.chromium.org/884373002/diff/180001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_settings_helper.h (right):

https://codereview.chromium.org/884373002/diff/180001/chrome/browser/banners/...
chrome/browser/banners/app_banner_settings_helper.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2015/02/02 19:50:13, Lei Zhang wrote:
> It's 2015

This file has been moved from chrome/browser/android (and then modified a fair
bit). Are we meant to update copyright dates on file moves?

[Lei Zhang, 2015-02-03 02:39:39]

lgtm. Just wanted to make sure this was going to desktop Chrome.

https://codereview.chromium.org/884373002/diff/180001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_settings_helper.h (right):

https://codereview.chromium.org/884373002/diff/180001/chrome/browser/banners/...
chrome/browser/banners/app_banner_settings_helper.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2015/02/03 02:33:45, benwells wrote:
> On 2015/02/02 19:50:13, Lei Zhang wrote:
> > It's 2015
> 
> This file has been moved from chrome/browser/android (and then modified a fair
> bit). Are we meant to update copyright dates on file moves?

Oh, for some reason the file's status is "A" instead of "A+" so I saw it as a
new file. I wrote this before realizing this was a move and forgot to delete the
comment.

[benwells, 2015-02-03 03:27:43]

The CQ bit was checked by benwells@chromium.org

[commit-bot: I haz the power, 2015-02-03 03:28:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/884373002/180001

[commit-bot: I haz the power, 2015-02-03 04:14:02]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-02-03 04:15:23]

Patchset 10 (id:??) landed as
https://crrev.com/1fa5a3081a04c314c2dcdf8afc6b9d951be13493
Cr-Commit-Position: refs/heads/master@{#314266}