[DOMStorage] Rate limiting writes to disk.

[DOMStorage] Rate limiting writes to disk.

Given access patterns that are just wrong, local & session storage can write many gigabytes per day or keep the disk perpetually spun up. To avoid excessive IO, we apply limits to the amount of data being written and the frequency of writes to address both kinds of 'bad' behaviors.

TBR=rvargas
BUG=176727
Committed: https://crrev.com/770daec923bb61392afbf9ca16ee0dad147e154f
Cr-Commit-Position: refs/heads/master@{#317424}

[michaeln, 2015-02-03 00:23:18]

michaeln@chromium.org changed reviewers:
+ cmumford@chromium.org

[michaeln, 2015-02-03 00:24:16]

need to add some tests still

[cmumford, 2015-02-03 00:45:25]

You should run "git cl format" and then "git cl lint".

https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
File content/browser/dom_storage/dom_storage_area.cc (right):

https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
content/browser/dom_storage/dom_storage_area.cc:41: static const int
kMaxDataPerHour = 500 * 1024;;
double semicolon

https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
File content/browser/dom_storage/dom_storage_area.h (right):

https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
content/browser/dom_storage/dom_storage_area.h:98: class RateLimiter {
Why not put the implementation of RateLimiter in the *.cc file? I'm not sure
what the rule here is.

https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
content/browser/dom_storage/dom_storage_area.h:104: DCHECK(desired_rate > 0);
Consider using DCHECK_GT instead of DCHECK(a > b)

https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
content/browser/dom_storage/dom_storage_area.h:105: }
Indenting looks wrong - Reitveld issue?

https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
content/browser/dom_storage/dom_storage_area.h:111: base::TimeDelta
ComputeTimeNeeded() {
Should this be const?

https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
content/browser/dom_storage/dom_storage_area.h:119: base::TimeDelta
ComputeDelayNeeded(const base::TimeDelta elapsed_time) {
Should be a static method?

https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
content/browser/dom_storage/dom_storage_area.h:138: size_t GetDataSize();
const method.

https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
content/browser/dom_storage/dom_storage_area.h:155: base::TimeDelta
ComputeCommitDelay();
const

[cmumford, 2015-02-03 16:35:40]

On 2015/02/03 00:45:25, cmumford wrote:
> You should run "git cl format" and then "git cl lint".
> 
>
https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
> File content/browser/dom_storage/dom_storage_area.cc (right):
> 
>
https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
> content/browser/dom_storage/dom_storage_area.cc:41: static const int
> kMaxDataPerHour = 500 * 1024;;
> double semicolon
> 
>
https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
> File content/browser/dom_storage/dom_storage_area.h (right):
> 
>
https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
> content/browser/dom_storage/dom_storage_area.h:98: class RateLimiter {
> Why not put the implementation of RateLimiter in the *.cc file? I'm not sure
> what the rule here is.
> 
>
https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
> content/browser/dom_storage/dom_storage_area.h:104: DCHECK(desired_rate > 0);
> Consider using DCHECK_GT instead of DCHECK(a > b)
> 
>
https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
> content/browser/dom_storage/dom_storage_area.h:105: }
> Indenting looks wrong - Reitveld issue?
> 
>
https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
> content/browser/dom_storage/dom_storage_area.h:111: base::TimeDelta
> ComputeTimeNeeded() {
> Should this be const?
> 
>
https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
> content/browser/dom_storage/dom_storage_area.h:119: base::TimeDelta
> ComputeDelayNeeded(const base::TimeDelta elapsed_time) {
> Should be a static method?
> 
>
https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
> content/browser/dom_storage/dom_storage_area.h:138: size_t GetDataSize();
> const method.
> 
>
https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
> content/browser/dom_storage/dom_storage_area.h:155: base::TimeDelta
> ComputeCommitDelay();
> const

Also, consider using bug 176727 for this CL

[michaeln, 2015-02-03 19:59:15]

still haven't written any tests but fixed up the style and switched to using
base::TimeTicks instead of Time so changing the clock doesn't break things

https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
File content/browser/dom_storage/dom_storage_area.cc (right):

https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
content/browser/dom_storage/dom_storage_area.cc:41: static const int
kMaxDataPerHour = 500 * 1024;;
On 2015/02/03 00:45:24, cmumford wrote:
> double semicolon

Done. Also made this a function of the constant perareaquota value.

https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
File content/browser/dom_storage/dom_storage_area.h (right):

https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
content/browser/dom_storage/dom_storage_area.h:98: class RateLimiter {
On 2015/02/03 00:45:24, cmumford wrote:
> Why not put the implementation of RateLimiter in the *.cc file?

because i wasn't done yet :)

https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
content/browser/dom_storage/dom_storage_area.h:104: DCHECK(desired_rate > 0);
On 2015/02/03 00:45:24, cmumford wrote:
> Consider using DCHECK_GT instead of DCHECK(a > b)

Done.

https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
content/browser/dom_storage/dom_storage_area.h:111: base::TimeDelta
ComputeTimeNeeded() {
On 2015/02/03 00:45:24, cmumford wrote:
> Should this be const?

Done.

https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
content/browser/dom_storage/dom_storage_area.h:119: base::TimeDelta
ComputeDelayNeeded(const base::TimeDelta elapsed_time) {
On 2015/02/03 00:45:24, cmumford wrote:
> Should be a static method?

can't be, this uses the instance method ComputeTimeNeeded()

https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
content/browser/dom_storage/dom_storage_area.h:138: size_t GetDataSize();
On 2015/02/03 00:45:24, cmumford wrote:
> const method.

Done.

https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
content/browser/dom_storage/dom_storage_area.h:155: base::TimeDelta
ComputeCommitDelay();
On 2015/02/03 00:45:24, cmumford wrote:
> const

Done.

[cmumford, 2015-02-04 18:43:54]

Looking good - just waiting for the tests...

https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
File content/browser/dom_storage/dom_storage_area.h (right):

https://codereview.chromium.org/896643002/diff/1/content/browser/dom_storage/...
content/browser/dom_storage/dom_storage_area.h:119: base::TimeDelta
ComputeDelayNeeded(const base::TimeDelta elapsed_time) {
On 2015/02/03 19:59:15, michaeln wrote:
> On 2015/02/03 00:45:24, cmumford wrote:
> > Should be a static method?
> 
> can't be, this uses the instance method ComputeTimeNeeded()

Then it can be const right?

[michaeln, 2015-02-04 21:43:11]

Snapshot 6 alters logic around purging to treat areas with uncommitted changes
differently. Before any such changes would have been quickly committed, but now
they may stay uncommitted for a very long period of time (depending on the sites
usage).

> Then it can be const right?

yup and it is

[michaeln, 2015-02-11 03:15:07]

ptal

[cmumford, 2015-02-11 22:52:27]

https://codereview.chromium.org/896643002/diff/180001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_area.cc (right):

https://codereview.chromium.org/896643002/diff/180001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:33: static const int
kCommitDefaultDelay = 5;
What is best practice regarding units? Like kCommitDefaultDelaySecs?

https://codereview.chromium.org/896643002/diff/180001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:41: static const int
kMaxDataPerDay = kPerStorageAreaQuota * 2;
kMaxBytesPerDay?

https://codereview.chromium.org/896643002/diff/180001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:394:
base::TimeDelta::FromSeconds(kCommitDuringStartupDelay);
Did you want to assign these values to delay?

[michaeln, 2015-02-11 23:30:28]

i'm going to massage it a little to rely on bool IsStartupComplete() like we
talked about

https://codereview.chromium.org/896643002/diff/180001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_area.cc (right):

https://codereview.chromium.org/896643002/diff/180001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:33: static const int
kCommitDefaultDelay = 5;
On 2015/02/11 22:52:27, cmumford wrote:
> What is best practice regarding units? Like kCommitDefaultDelaySecs?

yup, done

https://codereview.chromium.org/896643002/diff/180001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:41: static const int
kMaxDataPerDay = kPerStorageAreaQuota * 2;
On 2015/02/11 22:52:27, cmumford wrote:
> kMaxBytesPerDay?

Done.

https://codereview.chromium.org/896643002/diff/180001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:394:
base::TimeDelta::FromSeconds(kCommitDuringStartupDelay);
On 2015/02/11 22:52:27, cmumford wrote:
> Did you want to assign these values to delay?

that would be a huge improvement!

[michaeln, 2015-02-12 00:26:20]

ptal

> i'm going to massage it a little to rely on bool IsStartupComplete() like we
> talked about

done

[cmumford, 2015-02-12 21:58:04]

lgtm

[michaeln, 2015-02-12 22:17:43]

New patchsets have been uploaded after l-g-t-m from cmumford@chromium.org

[cmumford, 2015-02-12 22:21:53]

https://codereview.chromium.org/896643002/diff/240001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_area.cc (right):

https://codereview.chromium.org/896643002/diff/240001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:419:
UMA_HISTOGRAM_BOOLEAN("LocalStorage.CommitDeferredDuringStartup", true);
Do you want to do this, or something like:

UMA_HISTOGRAM_BOOLEAN("...", IsBrowserStartingUp());

[michaeln, 2015-02-12 22:26:23]

michaeln@chromium.org changed reviewers:
+ asvitkine@chromium.org

[michaeln, 2015-02-12 22:30:26]

@alexei, ptal at the histograms

[Alexei Svitkine (slow), 2015-02-13 16:02:35]

https://codereview.chromium.org/896643002/diff/240001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_area.cc (right):

https://codereview.chromium.org/896643002/diff/240001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:419:
UMA_HISTOGRAM_BOOLEAN("LocalStorage.CommitDeferredDuringStartup", true);
On 2015/02/12 22:21:53, cmumford wrote:
> Do you want to do this, or something like:
> 
> UMA_HISTOGRAM_BOOLEAN("...", IsBrowserStartingUp());

I agree that it's better to log both true and false, so you have something to
compare to.

Otherwise, what will some random value tell you? What's 394838? It depends on
number of users of Chrome, etc. It's better to have a baseline.

https://codereview.chromium.org/896643002/diff/240001/content/common/dom_stor...
File content/common/dom_storage/dom_storage_map.cc (right):

https://codereview.chromium.org/896643002/diff/240001/content/common/dom_stor...
content/common/dom_storage/dom_storage_map.cc:112: if (values.size() == 0)
Nit: .empty()

https://codereview.chromium.org/896643002/diff/240001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/896643002/diff/240001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5554: - 
<owner>michaeln@chromium.org</owner>
Please mark these with an <obsolete> tag instead of removing, so old data can
continue to be deciphered.

[michaeln, 2015-02-14 00:24:45]

https://codereview.chromium.org/896643002/diff/240001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_area.cc (right):

https://codereview.chromium.org/896643002/diff/240001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:419:
UMA_HISTOGRAM_BOOLEAN("LocalStorage.CommitDeferredDuringStartup", true);
On 2015/02/13 16:02:35, Alexei Svitkine wrote:
> On 2015/02/12 22:21:53, cmumford wrote:
> > Do you want to do this, or something like:
> > 
> > UMA_HISTOGRAM_BOOLEAN("...", IsBrowserStartingUp());
> 
> I agree that it's better to log both true and false, so you have something to
> compare to.
> 
> Otherwise, what will some random value tell you? What's 394838? It depends on
> number of users of Chrome, etc. It's better to have a baseline.

Done.

https://codereview.chromium.org/896643002/diff/240001/content/common/dom_stor...
File content/common/dom_storage/dom_storage_map.cc (right):

https://codereview.chromium.org/896643002/diff/240001/content/common/dom_stor...
content/common/dom_storage/dom_storage_map.cc:112: if (values.size() == 0)
On 2015/02/13 16:02:35, Alexei Svitkine wrote:
> Nit: .empty()

Done.

https://codereview.chromium.org/896643002/diff/240001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/896643002/diff/240001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:5554: - 
<owner>michaeln@chromium.org</owner>
On 2015/02/13 16:02:35, Alexei Svitkine wrote:
> Please mark these with an <obsolete> tag instead of removing, so old data can
> continue to be deciphered.

Done.

[Alexei Svitkine (slow), 2015-02-17 01:05:20]

https://codereview.chromium.org/896643002/diff/280001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_area.cc (right):

https://codereview.chromium.org/896643002/diff/280001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:33: static const int
kCommitDefaultDelaySecs = 5;
Nit: Move these constants to the anonymous namespace below and remove "static"
keyword.

https://codereview.chromium.org/896643002/diff/280001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:47: bool IsBrowserStartingUp() {
Nit: It seems like this function should instead be named "IsSystemStartingUp()"
since it checks system uptime rather than browser uptime.

https://codereview.chromium.org/896643002/diff/280001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:51: }
Nit: add "  // namespace"

https://codereview.chromium.org/896643002/diff/280001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:59: void
DOMStorageArea::RateLimiter::add_samples(size_t samples) {
Nit: Either inline this into the header or name it using CamelCase

https://codereview.chromium.org/896643002/diff/280001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:66: return
base::TimeDelta::FromInternalValue((samples_ / rate_) *
The comment from time.h says this about ToInternalValue():

"Please don't use this and do arithmetic on it, as it is more error prone than
using the provided operators."

Actually, I think TimeDelta overrides * operator - can you just use that
directly?

https://codereview.chromium.org/896643002/diff/280001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:72: // seen at the desired rate.
Nit: Comments should be in the header file. Same above.

https://codereview.chromium.org/896643002/diff/280001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_area.h (right):

https://codereview.chromium.org/896643002/diff/280001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.h:160: };
Nit: Is there a reason this class doesn't have DISALLOW_COPY_AND_ASSIGN()?

Also, please add it to RateLimiter above if possible.

[michaeln, 2015-02-17 19:36:51]

https://codereview.chromium.org/896643002/diff/280001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_area.cc (right):

https://codereview.chromium.org/896643002/diff/280001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:47: bool IsBrowserStartingUp() {
On 2015/02/17 01:05:20, Alexei Svitkine wrote:
> Nit: It seems like this function should instead be named
"IsSystemStartingUp()"
> since it checks system uptime rather than browser uptime.

Oh! Thank you for this comment. I mistakenly thought Uptime() returned the
process uptime. I'll upload a new CL after revisiting this part of it.

https://codereview.chromium.org/896643002/diff/280001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:66: return
base::TimeDelta::FromInternalValue((samples_ / rate_) *
On 2015/02/17 01:05:20, Alexei Svitkine wrote:
> The comment from time.h says this about ToInternalValue():
> 
> "Please don't use this and do arithmetic on it, as it is more error prone than
> using the provided operators."
> 
> Actually, I think TimeDelta overrides * operator - can you just use that
> directly?

Nice idea. There isn't an operator that takes floats so I'll add one.

I saw that comment too, I don't think the usage here counts as error prone since
its not making any assumptions about the resolution of the internal value.

[michaeln, 2015-02-19 01:19:20]

ptal

i'm looking into adding floating pt operators to TimeDelta seperately

https://codereview.chromium.org/896643002/diff/280001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_area.cc (right):

https://codereview.chromium.org/896643002/diff/280001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:33: static const int
kCommitDefaultDelaySecs = 5;
On 2015/02/17 01:05:20, Alexei Svitkine wrote:
> Nit: Move these constants to the anonymous namespace below and remove "static"
> keyword.

Done.

https://codereview.chromium.org/896643002/diff/280001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:51: }
On 2015/02/17 01:05:20, Alexei Svitkine wrote:
> Nit: add "  // namespace"

Done.

https://codereview.chromium.org/896643002/diff/280001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:59: void
DOMStorageArea::RateLimiter::add_samples(size_t samples) {
On 2015/02/17 01:05:20, Alexei Svitkine wrote:
> Nit: Either inline this into the header or name it using CamelCase

Done.

https://codereview.chromium.org/896643002/diff/280001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:72: // seen at the desired rate.
On 2015/02/17 01:05:20, Alexei Svitkine wrote:
> Nit: Comments should be in the header file. Same above.

Done.

[michaeln, 2015-02-19 01:20:03]

https://codereview.chromium.org/896643002/diff/280001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_area.cc (right):

https://codereview.chromium.org/896643002/diff/280001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.cc:47: bool IsBrowserStartingUp() {
On 2015/02/17 19:36:51, michaeln wrote:
> On 2015/02/17 01:05:20, Alexei Svitkine wrote:
> > Nit: It seems like this function should instead be named
> "IsSystemStartingUp()"
> > since it checks system uptime rather than browser uptime.
> 
> Oh! Thank you for this comment. I mistakenly thought Uptime() returned the
> process uptime. I'll upload a new CL after revisiting this part of it.

Done.

[michaeln, 2015-02-19 02:04:39]

https://codereview.chromium.org/896643002/diff/340001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/896643002/diff/340001/base/time/time.h#newcod...
base/time/time.h:182: TimeDelta multiply_by(double a) const {
Done

adding an operator*(dbl) makes usages with literal numeric values ambiguous

[michaeln, 2015-02-19 04:01:52]

I have a couple unittests to take a look at.

DOMStorageAreaTest.BackingDatabaseOpened
DOMStorageAreaTest.DeleteOrigin

[michaeln, 2015-02-20 01:23:47]

ptal

I'm backing out the explicit delay during startup part of this CL and will take
that up separately. The rate limiting part should help some during that time
period, just not as much as it could.

[Alexei Svitkine (slow), 2015-02-20 15:01:37]

LGTM % comments

https://codereview.chromium.org/896643002/diff/380001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_area.h (right):

https://codereview.chromium.org/896643002/diff/380001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.h:125: };
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/896643002/diff/380001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_area.h:170: };
nit: DISALLOW_COPY_AND_ASSIGN

[michaeln, 2015-02-20 19:41:07]

michaeln@chromium.org changed reviewers:
+ rvargas@chromium.org

[michaeln, 2015-02-20 19:41:34]

@rvargas, ptal a base/time.h

[michaeln, 2015-02-20 19:45:34]

New patchsets have been uploaded after l-g-t-m from asvitkine@chromium.org

[cmumford, 2015-02-20 19:50:42]

lgtm

[michaeln, 2015-02-20 21:17:16]

@rvargas, tbr'ing you about the small additions to base/time.h

[michaeln, 2015-02-20 21:17:23]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2015-02-20 21:18:22]

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

[rvargas (doing something else), 2015-02-20 21:49:43]

https://codereview.chromium.org/896643002/diff/400001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/896643002/diff/400001/base/time/time.h#newcod...
base/time/time.h:161: // Computations with ints, note that we only allow
multiplicative operations
This comment looks stale now.

https://codereview.chromium.org/896643002/diff/400001/base/time/time.h#newcod...
base/time/time.h:182: TimeDelta multiply_by(double a) const {
I know that in general we prefer named methods over overloading, but all
operations are currently performed with operator overloading so it seems better
to be consistent with what this class is doing.

[michaeln, 2015-02-20 22:18:04]

https://codereview.chromium.org/896643002/diff/400001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/896643002/diff/400001/base/time/time.h#newcod...
base/time/time.h:161: // Computations with ints, note that we only allow
multiplicative operations
On 2015/02/20 21:49:42, rvargas wrote:
> This comment looks stale now.

It's still valid, there is no operator+=(int).

https://codereview.chromium.org/896643002/diff/400001/base/time/time.h#newcod...
base/time/time.h:182: TimeDelta multiply_by(double a) const {
On 2015/02/20 21:49:42, rvargas wrote:
> I know that in general we prefer named methods over overloading, but all
> operations are currently performed with operator overloading so it seems
better
> to be consistent with what this class is doing.

i initially tried it with operators but adding an operator*(dbl) makes usages
with literal numeric values ambiguous

[commit-bot: I haz the power, 2015-02-20 22:22:46]

Committed patchset #21 (id:400001)

[commit-bot: I haz the power, 2015-02-20 22:23:37]

Patchset 21 (id:??) landed as
https://crrev.com/770daec923bb61392afbf9ca16ee0dad147e154f
Cr-Commit-Position: refs/heads/master@{#317424}

[michaeln, 2015-02-20 22:31:38]

> i initially tried it with operators but adding an operator*(dbl) makes usages
> with literal numeric values ambiguous

Do you think it would be better to modify the callers to remove the ambiquity
instead?

[rvargas (doing something else), 2015-02-20 22:44:56]

https://codereview.chromium.org/896643002/diff/400001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/896643002/diff/400001/base/time/time.h#newcod...
base/time/time.h:161: // Computations with ints, note that we only allow
multiplicative operations
On 2015/02/20 22:18:04, michaeln wrote:
> On 2015/02/20 21:49:42, rvargas wrote:
> > This comment looks stale now.
> 
> It's still valid, there is no operator+=(int).

"we only allow multiplicative operations with ints" is not correct anymore

https://codereview.chromium.org/896643002/diff/400001/base/time/time.h#newcod...
base/time/time.h:182: TimeDelta multiply_by(double a) const {
On 2015/02/20 22:18:04, michaeln wrote:
> On 2015/02/20 21:49:42, rvargas wrote:
> > I know that in general we prefer named methods over overloading, but all
> > operations are currently performed with operator overloading so it seems
> better
> > to be consistent with what this class is doing.
> 
> i initially tried it with operators but adding an operator*(dbl) makes usages
> with literal numeric values ambiguous

hmmm, how about 
  template<typename T>
  TimeDelta operator*(T a) const {
    return TimeDelta(delta_ * a);
  }

It extends the allowed types, but in practice we are saying that we are ok with
things that the compiler can multiply with delta_, no?

[rvargas (doing something else), 2015-02-20 22:51:36]

On 2015/02/20 22:31:38, michaeln wrote:
> > i initially tried it with operators but adding an operator*(dbl) makes
usages
> > with literal numeric values ambiguous
> 
> Do you think it would be better to modify the callers to remove the ambiquity
> instead?

I don't think we can do that, can we?

[michaeln, 2015-02-20 23:06:06]

On 2015/02/20 22:44:56, rvargas wrote:
> https://codereview.chromium.org/896643002/diff/400001/base/time/time.h
> File base/time/time.h (right):
> 
>
https://codereview.chromium.org/896643002/diff/400001/base/time/time.h#newcod...
> base/time/time.h:161: // Computations with ints, note that we only allow
> multiplicative operations
> On 2015/02/20 22:18:04, michaeln wrote:
> > On 2015/02/20 21:49:42, rvargas wrote:
> > > This comment looks stale now.
> > 
> > It's still valid, there is no operator+=(int).
> 
> "we only allow multiplicative operations with ints" is not correct anymore

That's not how i was reading it, its qualifying which "Computations with ints".
 
>
https://codereview.chromium.org/896643002/diff/400001/base/time/time.h#newcod...
> base/time/time.h:182: TimeDelta multiply_by(double a) const {
> On 2015/02/20 22:18:04, michaeln wrote:
> > On 2015/02/20 21:49:42, rvargas wrote:
> > > I know that in general we prefer named methods over overloading, but all
> > > operations are currently performed with operator overloading so it seems
> > better
> > > to be consistent with what this class is doing.
> > 
> > i initially tried it with operators but adding an operator*(dbl) makes
usages
> > with literal numeric values ambiguous
> 
> hmmm, how about 
>   template<typename T>
>   TimeDelta operator*(T a) const {
>     return TimeDelta(delta_ * a);
>   }
> 
> It extends the allowed types, but in practice we are saying that we are ok
with
> things that the compiler can multiply with delta_, no?

I'll give that a try!

[michaeln, 2015-02-20 23:56:55]

> > hmmm, how about 
> >   template<typename T>
> >   TimeDelta operator*(T a) const {
> >     return TimeDelta(delta_ * a);
> >   }
> > 
> > It extends the allowed types, but in practice we are saying that we are ok
> with
> > things that the compiler can multiply with delta_, no?
> 
> I'll give that a try!

see https://codereview.chromium.org/945143002/