[Doodle] Don't refresh more than once per 15 mins

[Doodle] Don't refresh more than once per 15 mins

TBRing an enum value addition in histograms.xml
TBR=asvitkine

BUG=690467
Review-Url: https://codereview.chromium.org/2772313002
Cr-Commit-Position: refs/heads/master@{#460040}
Committed: https://chromium.googlesource.com/chromium/src/+/dc5711b2c96b24793ce55afa5807f441021b2503

[Marc Treib, 2017-03-27 13:21:32]

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

[commit-bot: I haz the power, 2017-03-27 13:21:43]

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

[Marc Treib, 2017-03-27 13:33:46]

Description was changed from

==========
[Doodle] Don't refresh more than once per 15 mins

BUG=690467
==========

to

==========
[Doodle] Don't refresh more than once per 15 mins

TBRing an enum value addition in histograms.xml
TBR=asvitkine

BUG=690467
==========

[Marc Treib, 2017-03-27 13:34:06]

treib@chromium.org changed reviewers:
+ fhorschig@chromium.org

[Marc Treib, 2017-03-27 13:34:06]

PTAL!

[fhorschig, 2017-03-27 15:45:05]

I really like this idea!
Just for clarification: Why 15 minutes? 
I am fine with that but still :)

Also:
Somehow, refresh interval is confusing me because
"interval" is usually something repeating
(SetInterval, RenderingInterval, ...).

Can we find a name that makes more clear that we don't want
to repeat but restrict during that time?

I am not sure myself, but maybe no_refresh_period, pausation, withholding,
suspension, ...

https://codereview.chromium.org/2772313002/diff/40001/components/doodle/doodl...
File components/doodle/doodle_service.h (right):

https://codereview.chromium.org/2772313002/diff/40001/components/doodle/doodl...
components/doodle/doodle_service.h:35: // All pointer parameters must be
non-null. If |min_refresh_interval| doesn't
nit: "All pointer parameters must be non-null." seems sufficient to me.
(Having the parameter optional pretty much states that it's optional).

https://codereview.chromium.org/2772313002/diff/40001/components/doodle/doodl...
File components/doodle/doodle_service_unittest.cc (right):

https://codereview.chromium.org/2772313002/diff/40001/components/doodle/doodl...
components/doodle/doodle_service_unittest.cc:467:
RecreateService(/*min_refresh_interval=*/base::nullopt);
nit: Can you please set some arbitrary value here?
(Just so we have one constructor call with value and one without)

[Marc Treib, 2017-03-27 16:02:22]

https://codereview.chromium.org/2772313002/diff/40001/components/doodle/doodl...
File components/doodle/doodle_service.h (right):

https://codereview.chromium.org/2772313002/diff/40001/components/doodle/doodl...
components/doodle/doodle_service.h:35: // All pointer parameters must be
non-null. If |min_refresh_interval| doesn't
On 2017/03/27 15:45:05, fhorschig wrote:
> nit: "All pointer parameters must be non-null." seems sufficient to me.
> (Having the parameter optional pretty much states that it's optional).

The relevant part of the comment is not that it's optional, but rather that
absence means "use the default value" rather than "no min refresh interval".

https://codereview.chromium.org/2772313002/diff/40001/components/doodle/doodl...
File components/doodle/doodle_service_unittest.cc (right):

https://codereview.chromium.org/2772313002/diff/40001/components/doodle/doodl...
components/doodle/doodle_service_unittest.cc:467:
RecreateService(/*min_refresh_interval=*/base::nullopt);
On 2017/03/27 15:45:05, fhorschig wrote:
> nit: Can you please set some arbitrary value here?
> (Just so we have one constructor call with value and one without)

Done.

[Marc Treib, 2017-03-27 16:03:35]

On 2017/03/27 15:45:05, fhorschig wrote:
> I really like this idea!
> Just for clarification: Why 15 minutes? 
> I am fine with that but still :)
> 
> Also:
> Somehow, refresh interval is confusing me because
> "interval" is usually something repeating
> (SetInterval, RenderingInterval, ...).
> 
> Can we find a name that makes more clear that we don't want
> to repeat but restrict during that time?
> 
> I am not sure myself, but maybe no_refresh_period, pausation, withholding,
> suspension, ...

Hm, not sure I like any of those more...

It's a *minimum* interval, i.e. the minimum time between successive refreshes,
and arguably it *is* repeating ;)
I guess I could live with "no_refresh_period"?

[fhorschig, 2017-03-27 16:23:53]

On 2017/03/27 16:03:35, Marc Treib wrote:
> On 2017/03/27 15:45:05, fhorschig wrote:
> > I really like this idea!
> > Just for clarification: Why 15 minutes? 
> > I am fine with that but still :)

Got it, thanks.

> > 
> > Also:
> > Somehow, refresh interval is confusing me because
> > "interval" is usually something repeating
> > (SetInterval, RenderingInterval, ...).
> > 
> > Can we find a name that makes more clear that we don't want
> > to repeat but restrict during that time?
> > 
> > I am not sure myself, but maybe no_refresh_period, pausation, withholding,
> > suspension, ...
> 
> Hm, not sure I like any of those more...
> 
> It's a *minimum* interval, i.e. the minimum time between successive refreshes,
> and arguably it *is* repeating ;)

I get the intention but still find it easy to read wrong.
But we can agree that it's kind of nitty ;D

> I guess I could live with "no_refresh_period"?

Me too, obviously.

[Marc Treib, 2017-03-27 16:46:34]

On 2017/03/27 16:23:53, fhorschig wrote:
> On 2017/03/27 16:03:35, Marc Treib wrote:
> > On 2017/03/27 15:45:05, fhorschig wrote:
> > > I really like this idea!
> > > Just for clarification: Why 15 minutes? 
> > > I am fine with that but still :)
> 
> Got it, thanks.
> 
> > > 
> > > Also:
> > > Somehow, refresh interval is confusing me because
> > > "interval" is usually something repeating
> > > (SetInterval, RenderingInterval, ...).
> > > 
> > > Can we find a name that makes more clear that we don't want
> > > to repeat but restrict during that time?
> > > 
> > > I am not sure myself, but maybe no_refresh_period, pausation, withholding,
> > > suspension, ...
> > 
> > Hm, not sure I like any of those more...
> > 
> > It's a *minimum* interval, i.e. the minimum time between successive
refreshes,
> > and arguably it *is* repeating ;)
> 
> I get the intention but still find it easy to read wrong.
> But we can agree that it's kind of nitty ;D
> 
> > I guess I could live with "no_refresh_period"?
> 
> Me too, obviously.

Hm. On further reflection, "no_refresh_period" is IMO also confusing, since it
doesn't make clear at all that this applies after a successful fetch. And OTOH,
googling for "minimum interval" or "maximum interval" does give a bunch of
results with matching semantics. So unless you feel strongly, I'll leave it as
is.

[Marc Treib, 2017-03-27 16:46:53]

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

[commit-bot: I haz the power, 2017-03-27 16:47:16]

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-03-27 16:54:53]

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

[commit-bot: I haz the power, 2017-03-27 16:54:54]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Marc Treib, 2017-03-27 16:55:42]

Patchset #5 (id:80001) has been deleted

[Marc Treib, 2017-03-27 16:57:09]

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

[commit-bot: I haz the power, 2017-03-27 16:57:43]

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-03-27 19:35:03]

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

[commit-bot: I haz the power, 2017-03-27 19:35:03]

Dry run: This issue passed the CQ dry run.

[fhorschig, 2017-03-28 07:52:14]

You may keep your interval ;)
lgtm and thanks for the nice feature!

[Marc Treib, 2017-03-28 08:03:21]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2017-03-28 08:03:34]

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-03-28 08:07:41]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490688201800250,
"parent_rev": "c9bff741d84b56d8f5963a398958fe85cc110fae", "commit_rev":
"dc5711b2c96b24793ce55afa5807f441021b2503"}

[commit-bot: I haz the power, 2017-03-28 08:08:28]

Description was changed from

==========
[Doodle] Don't refresh more than once per 15 mins

TBRing an enum value addition in histograms.xml
TBR=asvitkine

BUG=690467
==========

to

==========
[Doodle] Don't refresh more than once per 15 mins

TBRing an enum value addition in histograms.xml
TBR=asvitkine

BUG=690467

Review-Url: https://codereview.chromium.org/2772313002
Cr-Commit-Position: refs/heads/master@{#460040}
Committed:
https://chromium.googlesource.com/chromium/src/+/dc5711b2c96b24793ce55afa5807...
==========

[commit-bot: I haz the power, 2017-03-28 08:08:29]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/dc5711b2c96b24793ce55afa5807...