Issue 1782053004: Change how the quota system computes the total poolsize for temporary storage

michaeln

Description was changed from ========== poolSize BUG= ========== to ========== Change how the quota system ...

4 years, 9 months ago (2016-03-22 21:22:00 UTC) #1

michaeln

Description was changed from ========== Change how the quota system computes the total poolsize for ...

4 years, 9 months ago (2016-03-22 21:23:15 UTC) #2

michaeln

Description was changed from ========== Change how the quota system computes the total poolsize for ...

4 years, 9 months ago (2016-03-22 21:25:41 UTC) #3

michaeln

Description was changed from ========== Change how the quota system computes the total poolsize for ...

4 years, 9 months ago (2016-03-22 21:29:24 UTC) #4

michaeln

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

4 years, 9 months ago (2016-03-22 21:40:14 UTC) #5

michaeln

I think this is ready for a look. https://codereview.chromium.org/1782053004/diff/40001/storage/browser/quota/quota_manager.cc File storage/browser/quota/quota_manager.cc (left): https://codereview.chromium.org/1782053004/diff/40001/storage/browser/quota/quota_manager.cc#oldcode303 storage/browser/quota/quota_manager.cc:303: if ...

4 years, 9 months ago (2016-03-22 21:40:15 UTC) #6

michaeln

https://codereview.chromium.org/1782053004/diff/40001/storage/browser/quota/quota_manager.cc File storage/browser/quota/quota_manager.cc (right): https://codereview.chromium.org/1782053004/diff/40001/storage/browser/quota/quota_manager.cc#newcode65 storage/browser/quota/quota_manager.cc:65: const uint64_t kOsAccomodation = 250 * kMBytes; these specific ...

4 years, 9 months ago (2016-03-22 22:23:55 UTC) #7

michaeln

Description was changed from ========== Change how the quota system computes the total poolsize for ...

4 years, 9 months ago (2016-03-22 22:24:26 UTC) #8

michaeln

Description was changed from ========== Change how the quota system computes the total poolsize for ...

4 years, 9 months ago (2016-03-23 17:31:00 UTC) #9

michaeln

Description was changed from ========== Change how the quota system computes the total poolsize for ...

4 years, 9 months ago (2016-03-23 17:31:17 UTC) #10

jsbell

Initial notes. Still wrapping my brain around things. We should probably add more comments throughout ...

4 years, 9 months ago (2016-03-23 18:08:29 UTC) #11

Initial notes. Still wrapping my brain around things.

We should probably add more comments throughout to explain what's going on.

https://codereview.chromium.org/1782053004/diff/40001/storage/browser/quota/q...
File storage/browser/quota/quota_manager.cc (right):

https://codereview.chromium.org/1782053004/diff/40001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:62: const double kTemporaryQuotaRatio =
1.0 / 3.0;  // 33%
While you're here, can you comment on what these are used for?

https://codereview.chromium.org/1782053004/diff/40001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:284: // nominal desired amount of data.
(TODO: what should this be)
On 2016/03/22 21:40:15, michaeln wrote:
> ~66 MBytes for small andoid devcices and ~66GB for PCs with terrabyte drives,
> maybe add clamp this to no more than 10G?
> 
> too_low = std::min(computed_too_low, 10GB)

Would the "save a bit of room for non-Chrome usage" calc go in here as well?

https://codereview.chromium.org/1782053004/diff/40001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:292: int64_t remaining_quota =
std::min(INT64_C(0), desired_quota - host_usage);
I think this might be clearer as:

return std::min(
  desired_quota,
  host_usage + (available_disk_space - too_low_threshold));

(sanity check that this is the same?)

https://codereview.chromium.org/1782053004/diff/40001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:936:
GetAvailableSpace(dispatcher->GetAvailableSpaceCallback());
Can we avoid this if is_incognito_ like we used to?

https://codereview.chromium.org/1782053004/diff/40001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:1800: total -= kOsAccomodation; // todo:
what if total is super small???
Maybe we want a threshold, e.g. os_reserve = min(kOsAccomodation, total * 0.75)
so that on a newfangled device that e.g. we treat as desktop we assume there's
*some* storage?

Also, maybe we should have a related DCHECK so we catch new configurations
during development, e.g. if you're running on a Windows VM with only 8GB disk ?
Then we'd know we need to adjust the calcs/variables.

https://codereview.chromium.org/1782053004/diff/40001/storage/browser/quota/q...
File storage/browser/quota/quota_manager.h (right):

https://codereview.chromium.org/1782053004/diff/40001/storage/browser/quota/q...
storage/browser/quota/quota_manager.h:457: const
scoped_refptr<base::TaskRunner>& reply_runner);
Passing in a reply task runner seems odd. I'd expect the caller to expect to be
called back on the same thread, and pass in a callback that proxies to the other
thread if necessary so this utility is ignorant of that complexity.

(Seems like that should be a pattern factored out somewhere.)

michaeln

> Initial notes. Still wrapping my brain around things. updated the cl in response to ...

4 years, 8 months ago (2016-04-07 00:32:34 UTC) #12

> Initial notes. Still wrapping my brain around things.

updated the cl in response to comments

https://codereview.chromium.org/1782053004/diff/40001/storage/browser/quota/q...
File storage/browser/quota/quota_manager.cc (right):

https://codereview.chromium.org/1782053004/diff/40001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:62: const double kTemporaryQuotaRatio =
1.0 / 3.0;  // 33%
On 2016/03/23 18:08:28, jsbell wrote:
> While you're here, can you comment on what these are used for?

Done, also moved these values to where they're used so it's more obvious how
they're used

https://codereview.chromium.org/1782053004/diff/40001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:284: // nominal desired amount of data.
(TODO: what should this be)
On 2016/03/23 18:08:29, jsbell wrote:
> On 2016/03/22 21:40:15, michaeln wrote:
> > ~66 MBytes for small andoid devcices and ~66GB for PCs with terrabyte
drives,
> > maybe add clamp this to no more than 10G?
> > 
> > too_low = std::min(computed_too_low, 10GB)
> 
> Would the "save a bit of room for non-Chrome usage" calc go in here as well?

This is one of the two places where logic to "save a bit for others" comes into
play. The other place is around |must_remain_available_space| in the
QuotaTemporaryStorageEvictor, it uses 1/10 the volume size for that value.

If chrome is run on an approaching full disk, this check will prevent chrome
from writing data to disk.

https://codereview.chromium.org/1782053004/diff/40001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:292: int64_t remaining_quota =
std::min(INT64_C(0), desired_quota - host_usage);
On 2016/03/23 18:08:28, jsbell wrote:
> I think this might be clearer as:
> 
> return std::min(
>   desired_quota,
>   host_usage + (available_disk_space - too_low_threshold));
> 
> (sanity check that this is the same?)

I think your right. I had thought a variable named 'remaining_quota' might help
explain what its doing but smushing it into a single line helps more. Your
equation looks right, I added the inner std::max to prevent returning a negative
number.

return std::min(
    desired_quota,
    host_usage + std::max(0, available_disk_space - too_low_threshold));

https://codereview.chromium.org/1782053004/diff/40001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:936:
GetAvailableSpace(dispatcher->GetAvailableSpaceCallback());
On 2016/03/23 18:08:29, jsbell wrote:
> Can we avoid this if is_incognito_ like we used to?

that's a good question, the system is currently using GetAvailableSpace() for
the incognito case for some things, i'll have to dig some more? maybe we can use
constants for volume info in the incog case?

https://codereview.chromium.org/1782053004/diff/40001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:1800: total -= kOsAccomodation; // todo:
what if total is super small???
On 2016/03/23 18:08:28, jsbell wrote:
> Maybe we want a threshold, e.g. os_reserve = min(kOsAccomodation, total *
0.75)
> so that on a newfangled device that e.g. we treat as desktop we assume there's
> *some* storage?

Done

> Also, maybe we should have a related DCHECK so we catch new configurations
> during development, e.g. if you're running on a Windows VM with only 8GB disk
?
> Then we'd know we need to adjust the calcs/variables.

https://codereview.chromium.org/1782053004/diff/40001/storage/browser/quota/q...
File storage/browser/quota/quota_manager.h (right):

https://codereview.chromium.org/1782053004/diff/40001/storage/browser/quota/q...
storage/browser/quota/quota_manager.h:457: const
scoped_refptr<base::TaskRunner>& reply_runner);
On 2016/03/23 18:08:29, jsbell wrote:
> Passing in a reply task runner seems odd. I'd expect the caller to expect to
be
> called back on the same thread, and pass in a callback that proxies to the
other
> thread if necessary so this utility is ignorant of that complexity.
> 
> (Seems like that should be a pattern factored out somewhere.)

Done.

jsbell

I haven't looked in detail at the tests. Are we close enough that I should ...

4 years, 8 months ago (2016-04-07 19:33:41 UTC) #13

I haven't looked in detail at the tests. Are we close enough that I should take
a peek at those?

https://codereview.chromium.org/1782053004/diff/80001/storage/browser/quota/q...
File storage/browser/quota/quota_manager.cc (right):

https://codereview.chromium.org/1782053004/diff/80001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:275: // nominal desired amount of data.
(TODO: what should this be)
This seems fine to me. (i.e. remove the TODO)

https://codereview.chromium.org/1782053004/diff/80001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:279: // If its close to being too low,
cap growth to avoid it getting too low.
nit: it's

https://codereview.chromium.org/1782053004/diff/80001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:1796: #error "Port: Need to define
kOsAccomodation for unkown os."
Typo: unknown

https://codereview.chromium.org/1782053004/diff/80001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:1801: // the OSAccomodation into account.
nit: kOsAccomodation (or otherwise be consistent in naming)

https://codereview.chromium.org/1782053004/diff/80001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:1808: total -= kOsAccomodation; // todo:
what if total is super small???
To make the calculation clearer, can we leave `total` alone and use a different
variable for this intermediate size?

...

With this, if (kOsAccomodation / total) > 1 then we just ignore kOsAccomodation
(assumption: our numbers are "whack"). But if 1 > (kOsAccomodation / total) >
0.95 then we will get a ridiculously small pool size, but our numbers are also
probably whack.

I chatted w/ dknox offline, and we have two suggestions:

#1: if (kOsAccomodation / total) > 0.80, pool = (total * 0.2) / 3

i.e. "if we're pretty sure our kOsAccomodation number is whack, just make an
educated guess"

#2: if (kOsAccomodation / total) > 0.80, pool = (available) / 3

i.e. "if we're pretty sure our kOsAccomodation number is whack, base the
calculation on what's actually available" - this is basically falling back to
our old logic, and gets weird if we're not subtracting chrome's existing
footprint (which is what we're trying to avoid in this patch!!)

(in both cases the ratios are just made up)

The ideal option, of course, would be to ask the device how big the OS is.
Android knows this, but I don't think it's exposed.

michaeln

no new snapshot yet, i'll ping you again when i have one, just replying to ...

4 years, 8 months ago (2016-04-07 20:54:48 UTC) #14

no new snapshot yet, i'll ping you again when i have one, just replying to
comments

https://codereview.chromium.org/1782053004/diff/80001/content/browser/quota/q...
File content/browser/quota/quota_manager_unittest.cc (right):

https://codereview.chromium.org/1782053004/diff/80001/content/browser/quota/q...
content/browser/quota/quota_manager_unittest.cc:847: // not sure this test is
relevant anymore?
tests aren't ready yet, all i've done thusfar is make the existing tests pass. i
need to take a pass at adding new tests and deleting old ones that are no longer
relevant

https://codereview.chromium.org/1782053004/diff/80001/storage/browser/quota/q...
File storage/browser/quota/quota_manager.cc (right):

https://codereview.chromium.org/1782053004/diff/80001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:275: // nominal desired amount of data.
(TODO: what should this be)
On 2016/04/07 19:33:41, jsbell wrote:
> This seems fine to me. (i.e. remove the TODO)

Done.

https://codereview.chromium.org/1782053004/diff/80001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:279: // If its close to being too low,
cap growth to avoid it getting too low.
On 2016/04/07 19:33:41, jsbell wrote:
> nit: it's

Done.

https://codereview.chromium.org/1782053004/diff/80001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:1796: #error "Port: Need to define
kOsAccomodation for unkown os."
On 2016/04/07 19:33:41, jsbell wrote:
> Typo: unknown

Done.

https://codereview.chromium.org/1782053004/diff/80001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:1801: // the OSAccomodation into account.
On 2016/04/07 19:33:41, jsbell wrote:
> nit: kOsAccomodation (or otherwise be consistent in naming)

Done.

https://codereview.chromium.org/1782053004/diff/80001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:1808: total -= kOsAccomodation; // todo:
what if total is super small???
On 2016/04/07 19:33:41, jsbell wrote:
> To make the calculation clearer, can we leave `total` alone and use a
different
> variable for this intermediate size?
> 
> ...
> 
> With this, if (kOsAccomodation / total) > 1 then we just ignore
kOsAccomodation
> (assumption: our numbers are "whack"). But if 1 > (kOsAccomodation / total) >
> 0.95 then we will get a ridiculously small pool size, but our numbers are also
> probably whack.
> 
> I chatted w/ dknox offline, and we have two suggestions:
> 
> #1: if (kOsAccomodation / total) > 0.80, pool = (total * 0.2) / 3
> 
> i.e. "if we're pretty sure our kOsAccomodation number is whack, just make an
> educated guess"
> 
> #2: if (kOsAccomodation / total) > 0.80, pool = (available) / 3
> 
> i.e. "if we're pretty sure our kOsAccomodation number is whack, base the
> calculation on what's actually available" - this is basically falling back to
> our old logic, and gets weird if we're not subtracting chrome's existing
> footprint (which is what we're trying to avoid in this patch!!)
> 
> (in both cases the ratios are just made up)
> 
> The ideal option, of course, would be to ask the device how big the OS is.
> Android knows this, but I don't think it's exposed.

thnx for the suggestions, i had forgotten about adjusting this one in the last
snapshot. i'll try something along the lines of #1.

michaeln

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

4 years, 4 months ago (2016-07-26 21:59:41 UTC) #15

michaeln

@kinuko @jsbell, this cl is not ready yet but I'd like to get feedback on ...

4 years, 4 months ago (2016-07-26 22:07:27 UTC) #16

kinuko

Thanks for cleaning the temp storage quota calculation. (I only looked into the content client ...

4 years, 4 months ago (2016-07-27 14:23:25 UTC) #17

michaeln

thnx for looking https://codereview.chromium.org/1782053004/diff/80001/storage/browser/quota/quota_manager.cc File storage/browser/quota/quota_manager.cc (right): https://codereview.chromium.org/1782053004/diff/80001/storage/browser/quota/quota_manager.cc#newcode1808 storage/browser/quota/quota_manager.cc:1808: total -= kOsAccomodation; // todo: what ...

4 years, 4 months ago (2016-07-27 19:57:48 UTC) #18

thnx for looking

https://codereview.chromium.org/1782053004/diff/80001/storage/browser/quota/q...
File storage/browser/quota/quota_manager.cc (right):

https://codereview.chromium.org/1782053004/diff/80001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:1808: total -= kOsAccomodation; // todo:
what if total is super small???
On 2016/04/07 20:54:47, michaeln wrote:
> On 2016/04/07 19:33:41, jsbell wrote:
> > To make the calculation clearer, can we leave `total` alone and use a
> different
> > variable for this intermediate size?
> > 
> > ...
> > 
> > With this, if (kOsAccomodation / total) > 1 then we just ignore
> kOsAccomodation
> > (assumption: our numbers are "whack"). But if 1 > (kOsAccomodation / total)
>
> > 0.95 then we will get a ridiculously small pool size, but our numbers are
also
> > probably whack.
> > 
> > I chatted w/ dknox offline, and we have two suggestions:
> > 
> > #1: if (kOsAccomodation / total) > 0.80, pool = (total * 0.2) / 3
> > 
> > i.e. "if we're pretty sure our kOsAccomodation number is whack, just make an
> > educated guess"
> > 
> > #2: if (kOsAccomodation / total) > 0.80, pool = (available) / 3
> > 
> > i.e. "if we're pretty sure our kOsAccomodation number is whack, base the
> > calculation on what's actually available" - this is basically falling back
to
> > our old logic, and gets weird if we're not subtracting chrome's existing
> > footprint (which is what we're trying to avoid in this patch!!)
> > 
> > (in both cases the ratios are just made up)
> > 
> > The ideal option, of course, would be to ask the device how big the OS is.
> > Android knows this, but I don't think it's exposed.
> 
> thnx for the suggestions, i had forgotten about adjusting this one in the last
> snapshot. i'll try something along the lines of #1.
> 

Done in chrome_content_browser_client.cc

https://codereview.chromium.org/1782053004/diff/140001/content/public/browser...
File content/public/browser/content_browser_client.cc (right):

https://codereview.chromium.org/1782053004/diff/140001/content/public/browser...
content/public/browser/content_browser_client.cc:246:
ContentBrowserClient::GetTemporaryStorageConfigurationFunction(
On 2016/07/27 14:23:25, kinuko wrote:
> I felt it might be slightly awkward to return a callback function from content
> client... I imagined ContentClient might be able to simply expose a virtual
> method like ContentBrowserClient::GetTemporaryStorageConfiguration() and
> QuotaManager can bind it to get a closure if necessary (and run it on the UI
> thread).  At least it'd be easier for embedders to understand what it's
supposed
> to do, but probably there's a reason you didn't take the approach?

That's another option, a virtual method that returns the config via a
TemporaryStorageConfigurationCallback. I think I prefer that too.

virtual void ContentBrowserClient::GetTemporaryStorageConfig(
    BrowserContext* browser_context,
    const storage::TemporaryStorageConfigurationCallback& callback);

The reason i didn't do it that way is that the QuotaManager can't depend on the
content lib defined interface, so the GetTemporaryStorageConfigurationFunc is
needed in the storage lib in any case. By having the chrome layer provide the
getterfunc, grepping for GetTemporaryStorageConfigurationFunc might let folks
find the actual code up in chrome that fills out the config more quickly. But
I'm not sure that's such a good reason.

The reason for invoking the callback on the main thread is due to the thread
affinity of the BrowserContext and ContentBrowserClient, those are main thread
only. Whatever technique is used to expose the function/method, the logic is
related to content settings and other properties of the BrowserContext (and
possibly the StoragePartition at some point).

https://codereview.chromium.org/1782053004/diff/140001/storage/browser/quota/...
File storage/browser/quota/quota_manager.h (right):

https://codereview.chromium.org/1782053004/diff/140001/storage/browser/quota/...
storage/browser/quota/quota_manager.h:133: struct TemporaryStorageConfiguration
{
wdyt about the set of config params?

jsbell

https://codereview.chromium.org/1782053004/diff/140001/storage/browser/quota/quota_manager.h File storage/browser/quota/quota_manager.h (right): https://codereview.chromium.org/1782053004/diff/140001/storage/browser/quota/quota_manager.h#newcode136 storage/browser/quota/quota_manager.h:136: // The target size of the shared pool of ...

4 years, 4 months ago (2016-07-27 20:18:30 UTC) #19

michaeln

Thnx, I tweeked the content api and set of config values as suggested. The CL ...

4 years, 4 months ago (2016-07-28 02:06:28 UTC) #20

kinuko

Thanks, reg: public API: while what you mentioned in your comment makes sense too the ...

4 years, 4 months ago (2016-07-28 15:49:18 UTC) #21

michaeln

Not yet ready, but this i hope this is getting closer +1039 lines, -1089 lines ...

4 years, 2 months ago (2016-10-14 23:51:48 UTC) #22

jsbell

looking good! A few UMA-specific comments and otherwise it looks ready for final polish. https://codereview.chromium.org/1782053004/diff/240001/chrome/browser/chrome_content_browser_client.cc ...

4 years, 2 months ago (2016-10-17 23:56:54 UTC) #23

michaeln

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

4 years, 1 month ago (2016-10-24 20:54:25 UTC) #24

commit-bot: I haz the power

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

4 years, 1 month ago (2016-10-24 20:54:56 UTC) #25

commit-bot: I haz the power

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

4 years, 1 month ago (2016-10-24 21:29:14 UTC) #26

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linux/builds/247610)

4 years, 1 month ago (2016-10-24 21:29:15 UTC) #27

michaeln

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

4 years, 1 month ago (2016-10-24 22:44:46 UTC) #28

commit-bot: I haz the power

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

4 years, 1 month ago (2016-10-24 22:45:45 UTC) #29

commit-bot: I haz the power

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

4 years, 1 month ago (2016-10-24 22:56:15 UTC) #30

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: 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-generic_chromium_compile_only_ng/builds/222379)

4 years, 1 month ago (2016-10-24 22:56:16 UTC) #31

michaeln

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

4 years, 1 month ago (2016-10-24 23:42:58 UTC) #32

commit-bot: I haz the power

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

4 years, 1 month ago (2016-10-24 23:43:58 UTC) #33

commit-bot: I haz the power

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

4 years, 1 month ago (2016-10-25 00:09:18 UTC) #34

commit-bot: I haz the power

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

4 years, 1 month ago (2016-10-25 00:09:19 UTC) #35

cmumford

Interim comments - still reviewing. https://codereview.chromium.org/1782053004/diff/240001/storage/browser/quota/quota_manager.cc File storage/browser/quota/quota_manager.cc (right): https://codereview.chromium.org/1782053004/diff/240001/storage/browser/quota/quota_manager.cc#newcode237 storage/browser/quota/quota_manager.cc:237: is_incognito_(is_incognito), field 'is_incognito_' will ...

4 years, 1 month ago (2016-10-25 20:30:54 UTC) #36

michaeln

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

4 years, 1 month ago (2016-10-25 22:51:36 UTC) #37

commit-bot: I haz the power

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

4 years, 1 month ago (2016-10-25 22:52:03 UTC) #38

commit-bot: I haz the power

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

4 years, 1 month ago (2016-10-25 23:19:12 UTC) #39

commit-bot: I haz the power

Dry run: 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_compile_dbg/builds/152473) chromeos_daisy_chromium_compile_only_ng on ...

4 years, 1 month ago (2016-10-25 23:19:13 UTC) #40

michaeln

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

4 years, 1 month ago (2016-10-26 00:21:09 UTC) #41

commit-bot: I haz the power

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

4 years, 1 month ago (2016-10-26 00:21:44 UTC) #42

commit-bot: I haz the power

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

4 years, 1 month ago (2016-10-26 01:42:12 UTC) #43

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/260119)

4 years, 1 month ago (2016-10-26 01:42:16 UTC) #44

michaeln

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

4 years, 1 month ago (2016-10-26 20:40:34 UTC) #45

commit-bot: I haz the power

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

4 years, 1 month ago (2016-10-26 20:41:12 UTC) #46

michaeln

https://codereview.chromium.org/1782053004/diff/240001/chrome/browser/chrome_content_browser_client.cc File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/1782053004/diff/240001/chrome/browser/chrome_content_browser_client.cc#newcode2104 chrome/browser/chrome_content_browser_client.cc:2104: // the os and essential application code outside of ...

4 years, 1 month ago (2016-10-26 21:47:54 UTC) #47

michaeln

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

4 years, 1 month ago (2016-10-27 00:21:40 UTC) #48

commit-bot: I haz the power

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

4 years, 1 month ago (2016-10-27 00:22:08 UTC) #49

commit-bot: I haz the power

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

4 years, 1 month ago (2016-10-27 01:44:52 UTC) #50

commit-bot: I haz the power

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

4 years, 1 month ago (2016-10-27 01:44:53 UTC) #51

michaeln

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

4 years, 1 month ago (2016-10-31 20:59:50 UTC) #52

commit-bot: I haz the power

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

4 years, 1 month ago (2016-10-31 21:00:32 UTC) #53

michaeln

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

4 years, 1 month ago (2016-11-01 00:24:56 UTC) #54

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-01 00:25:20 UTC) #55

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-01 00:50:57 UTC) #56

commit-bot: I haz the power

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_swarming_rel/builds/59206)

4 years, 1 month ago (2016-11-01 00:50:59 UTC) #57

michaeln

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

4 years, 1 month ago (2016-11-02 22:44:56 UTC) #58

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-02 22:45:57 UTC) #59

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-02 22:50:38 UTC) #60

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_clobber_rel_ng/builds/264693) mac_chromium_compile_dbg_ng on ...

4 years, 1 month ago (2016-11-02 22:50:39 UTC) #61

michaeln

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

4 years, 1 month ago (2016-11-03 20:47:03 UTC) #62

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-03 20:47:54 UTC) #63

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-03 20:52:35 UTC) #64

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/255960) mac_chromium_compile_dbg_ng on ...

4 years, 1 month ago (2016-11-03 20:52:37 UTC) #65

michaeln

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

4 years, 1 month ago (2016-11-03 22:44:58 UTC) #66

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-03 22:45:51 UTC) #67

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-03 23:18:58 UTC) #68

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-03 23:19:00 UTC) #69

michaeln

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

4 years, 1 month ago (2016-11-04 01:16:54 UTC) #70

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-04 01:18:11 UTC) #71

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-04 01:29:17 UTC) #72

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_chromium_compile_only_ng/builds/228957) linux_chromium_chromeos_rel_ng on ...

4 years, 1 month ago (2016-11-04 01:29:18 UTC) #73

michaeln

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

4 years, 1 month ago (2016-11-04 22:48:34 UTC) #74

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-04 22:49:40 UTC) #75

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-04 23:52:10 UTC) #76

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/311798)

4 years, 1 month ago (2016-11-04 23:52:12 UTC) #77

michaeln

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

4 years, 1 month ago (2016-11-08 00:06:50 UTC) #78

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-08 00:07:35 UTC) #79

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-08 02:09:50 UTC) #80

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/258039) linux_chromium_chromeos_ozone_rel_ng on ...

4 years, 1 month ago (2016-11-08 02:09:51 UTC) #81

michaeln

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

4 years, 1 month ago (2016-11-08 20:53:19 UTC) #82

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-08 20:53:51 UTC) #83

michaeln

Description was changed from ========== Change how the quota system computes the total poolsize for ...

4 years, 1 month ago (2016-11-08 21:07:59 UTC) #84

michaeln

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

4 years, 1 month ago (2016-11-08 21:10:13 UTC) #85

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-08 21:10:59 UTC) #86

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-08 23:07:29 UTC) #88

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/311242)

4 years, 1 month ago (2016-11-08 23:07:31 UTC) #89

jsbell

I believe this lgtm with a few nits. https://codereview.chromium.org/1782053004/diff/720001/content/browser/storage_partition_impl.cc File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/1782053004/diff/720001/content/browser/storage_partition_impl.cc#newcode886 content/browser/storage_partition_impl.cc:886: const ...

4 years, 1 month ago (2016-11-09 00:12:04 UTC) #90

michaeln

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

4 years, 1 month ago (2016-11-09 01:18:24 UTC) #91

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-09 01:19:08 UTC) #92

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-09 01:24:08 UTC) #93

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-09 01:24:09 UTC) #94

michaeln

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

4 years, 1 month ago (2016-11-09 23:58:05 UTC) #95

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-09 23:58:42 UTC) #96

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-10 03:22:31 UTC) #97

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 1 month ago (2016-11-10 03:22:33 UTC) #98

kinuko

looking good, haven't really finished reviewing (sorry for being slow) but sending some minor comments. ...

4 years, 1 month ago (2016-11-10 17:49:22 UTC) #99

michaeln

thnx for looking! https://codereview.chromium.org/1782053004/diff/720001/content/browser/storage_partition_impl.cc File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/1782053004/diff/720001/content/browser/storage_partition_impl.cc#newcode886 content/browser/storage_partition_impl.cc:886: const base::FilePath& parition_path, On 2016/11/09 00:12:04, ...

4 years, 1 month ago (2016-11-11 02:31:53 UTC) #100

michaeln

https://codereview.chromium.org/1782053004/diff/760001/storage/browser/quota/quota_manager.cc File storage/browser/quota/quota_manager.cc (right): https://codereview.chromium.org/1782053004/diff/760001/storage/browser/quota/quota_manager.cc#newcode1461 storage/browser/quota/quota_manager.cc:1461: // TODO(michaeln): remove support for this for now? i ...

4 years, 1 month ago (2016-11-11 02:36:28 UTC) #101

michaeln

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

4 years, 1 month ago (2016-11-11 02:37:18 UTC) #102

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-11 02:37:53 UTC) #103

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-11 03:11:05 UTC) #104

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg/builds/30884)

4 years, 1 month ago (2016-11-11 03:11:07 UTC) #105

jsbell

Looks like it's missing an #include ../../ash/shell/content/client/shell_content_browser_client.cc:37:47: error: member access into incomplete type 'content::BrowserContext' partition->GetPath(), ...

4 years, 1 month ago (2016-11-11 17:07:47 UTC) #106

kinuko

Thanks for fixing all those tests. Looks almost good https://codereview.chromium.org/1782053004/diff/780001/content/browser/fileapi/file_system_browsertest.cc File content/browser/fileapi/file_system_browsertest.cc (right): https://codereview.chromium.org/1782053004/diff/780001/content/browser/fileapi/file_system_browsertest.cc#newcode77 content/browser/fileapi/file_system_browsertest.cc:77: ...

4 years, 1 month ago (2016-11-11 17:14:57 UTC) #107

michaeln

hopefully i got the compilation errors fixed now too https://codereview.chromium.org/1782053004/diff/780001/content/browser/fileapi/file_system_browsertest.cc File content/browser/fileapi/file_system_browsertest.cc (right): https://codereview.chromium.org/1782053004/diff/780001/content/browser/fileapi/file_system_browsertest.cc#newcode77 content/browser/fileapi/file_system_browsertest.cc:77: ...

4 years, 1 month ago (2016-11-11 22:41:33 UTC) #108

michaeln

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

4 years, 1 month ago (2016-11-12 02:03:29 UTC) #109

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-12 02:04:01 UTC) #110

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-12 02:08:07 UTC) #111

commit-bot: I haz the power

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_compile_dbg_ng/builds/304747)

4 years, 1 month ago (2016-11-12 02:08:09 UTC) #112

kinuko

On 2016/11/11 22:41:33, michaeln wrote: > hopefully i got the compilation errors fixed now too ...

4 years, 1 month ago (2016-11-14 14:49:37 UTC) #113

kinuko

https://codereview.chromium.org/1782053004/diff/840001/content/browser/quota/quota_manager_unittest.cc File content/browser/quota/quota_manager_unittest.cc (right): https://codereview.chromium.org/1782053004/diff/840001/content/browser/quota/quota_manager_unittest.cc#newcode829 content/browser/quota/quota_manager_unittest.cc:829: // Provided diskspace is not tight,. global usage does ...

4 years, 1 month ago (2016-11-14 14:50:20 UTC) #114

michaeln

https://codereview.chromium.org/1782053004/diff/780001/content/browser/quota/quota_manager_unittest.cc File content/browser/quota/quota_manager_unittest.cc (right): https://codereview.chromium.org/1782053004/diff/780001/content/browser/quota/quota_manager_unittest.cc#newcode714 content/browser/quota/quota_manager_unittest.cc:714: EXPECT_EQ(0, quota()); // Over-budget case. todo TOODOOOOO??? On 2016/11/11 ...

4 years, 1 month ago (2016-11-15 01:31:32 UTC) #115

https://codereview.chromium.org/1782053004/diff/780001/content/browser/quota/...
File content/browser/quota/quota_manager_unittest.cc (right):

https://codereview.chromium.org/1782053004/diff/780001/content/browser/quota/...
content/browser/quota/quota_manager_unittest.cc:714: EXPECT_EQ(0, quota());  //
Over-budget case. todo TOODOOOOO???
On 2016/11/11 22:41:33, michaeln wrote:
> On 2016/11/11 17:14:56, kinuko wrote:
> > Do you plan to change the capping code or should we remove the todo comment?
> 
> ooops, thnx for pointing this out, i meant to clean this up and just haven't
> yet.
> 
> Here's the thing, the quotamgr no longer looks at policy->CanQueryDiskSize()
for
> anything. These existing tests for 'installed' apps are testing subtle
> difference in behavior depending on CanQueryDiskSize() return value (its true
> for 'installed' apps). I'm not sure why these difference are needed or if the
> new system needs to do something special for this case?
> 
> It first got introduced here, i'll take a look at why.
> https://chromiumcodereview.appspot.com/10826270/
> https://bugs.chromium.org/p/chromium/issues/detail?id=141583

I deleted CanQueryDiskSize() from the policy. Looks like the intent was to avoid
returning kNoLimit instead of a number that reflected the actual space
available. That isn't an issue anymore since t actual space available is always
taken into account.

    // Constrain the desired |host_quota| to something that fits.
    // If available space is too low, cap usage at current levels.
    // If it's close to being too low, cap growth to avoid it getting too low.
    int64_t host_quota =
        std::min(desired_host_quota_,
                 host_usage_ +
                     std::max(INT64_C(0), available_space_ -
                                              settings_.must_remain_available));
    callback_.Run(kQuotaStatusOk, host_usage_, host_quota);

https://codereview.chromium.org/1782053004/diff/780001/storage/browser/quota/...
File storage/browser/quota/quota_manager.cc (right):

https://codereview.chromium.org/1782053004/diff/780001/storage/browser/quota/...
storage/browser/quota/quota_manager.cc:1461: // TODO(michaeln): remove support
for this for now?
On 2016/11/11 17:14:56, kinuko wrote:
> I didn't remember what this code was for, and found that it's relatively new
> (crbug.com/464234).  I don't have enough context but if it's not really used
+1
> for removing this

it was used for an experimental attempt at karmic influenced eviction which has
been abandoned, i've removed support for this now, i think we can revisit
if/when we need to

https://codereview.chromium.org/1782053004/diff/840001/content/browser/quota/...
File content/browser/quota/quota_manager_unittest.cc (right):

https://codereview.chromium.org/1782053004/diff/840001/content/browser/quota/...
content/browser/quota/quota_manager_unittest.cc:829: // Provided diskspace is
not tight,. global usage does not affect the
On 2016/11/14 14:50:20, kinuko wrote:
> nit: ,. -> ,

Done.

michaeln

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

4 years, 1 month ago (2016-11-15 01:31:50 UTC) #116

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-15 01:33:23 UTC) #117

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-15 03:49:43 UTC) #118

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_compile_dbg_ng/builds/192009) linux_chromium_rel_ng on ...

4 years, 1 month ago (2016-11-15 03:49:46 UTC) #119

kinuko

Thanks, I think this lgtm https://codereview.chromium.org/1782053004/diff/880001/content/public/browser/content_browser_client.cc File content/public/browser/content_browser_client.cc (right): https://codereview.chromium.org/1782053004/diff/880001/content/public/browser/content_browser_client.cc#newcode234 content/public/browser/content_browser_client.cc:234: << "to provide storage ...

4 years, 1 month ago (2016-11-15 07:39:10 UTC) #120

michaeln

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

4 years, 1 month ago (2016-11-15 21:44:31 UTC) #121

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-15 21:45:04 UTC) #122

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-15 23:48:05 UTC) #123

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/262767) linux_chromium_rel_ng on ...

4 years, 1 month ago (2016-11-15 23:48:07 UTC) #124

michaeln

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

4 years, 1 month ago (2016-11-21 20:48:17 UTC) #125

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-21 20:49:08 UTC) #126

michaeln

michaeln@chromium.org changed reviewers: + boliu@chromium.org, dcheng@chromium.org, lazyboy@chromium.org, nyquist@chromium.org, sky@chromium.org

4 years, 1 month ago (2016-11-21 21:09:51 UTC) #127

michaeln

michaeln@chromium.org changed reviewers: + benwells@chromium.org, isherman@chromium.org, lcwu@chromium.org, skyostil@chromium.org, thestig@chromium.org - cmumford@chromium.org, dcheng@chromium.org, jsbell@chromium.org

4 years, 1 month ago (2016-11-21 21:44:28 UTC) #128

michaeln

Ptal. Long list of reviewers since the CL is adding a new content public API ...

4 years, 1 month ago (2016-11-21 21:52:48 UTC) #129

commit-bot: I haz the power

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

4 years, 1 month ago (2016-11-21 22:04:43 UTC) #130

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linux/builds/265082)

4 years, 1 month ago (2016-11-21 22:04:45 UTC) #131

boliu

I'd suggest avoid changing behavior in the same CL that does a huge refactor. Can ...

4 years, 1 month ago (2016-11-21 22:56:58 UTC) #132

Ilya Sherman

Metrics LGTM % nits: https://codereview.chromium.org/1782053004/diff/920001/storage/browser/quota/quota_settings.cc File storage/browser/quota/quota_settings.cc (right): https://codereview.chromium.org/1782053004/diff/920001/storage/browser/quota/quota_settings.cc#newcode13 storage/browser/quota/quota_settings.cc:13: UMA_HISTOGRAM_CUSTOM_COUNTS((name), static_cast<int>((sample) / kMBytes), 1, ...

4 years, 1 month ago (2016-11-21 23:25:37 UTC) #133

benwells

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

4 years, 1 month ago (2016-11-22 05:29:05 UTC) #134

michaeln

On 2016/11/21 22:56:58, boliu wrote: > I'd suggest avoid changing behavior in the same CL ...

4 years ago (2016-11-22 23:33:10 UTC) #138

michaeln

Description was changed from ========== Adds a QuotaSettings datastructure and changes how the total poolsize ...

4 years ago (2016-11-22 23:37:02 UTC) #139

boliu

On 2016/11/22 23:33:10, michaeln wrote: > On 2016/11/21 22:56:58, boliu wrote: > > I'd suggest ...

4 years ago (2016-11-23 00:03:41 UTC) #140

On 2016/11/22 23:33:10, michaeln wrote:
> On 2016/11/21 22:56:58, boliu wrote:
> > I'd suggest avoid changing behavior in the same CL that does a huge
refactor.
> > Can they be split up?
> 
> The change affects a lot of files due to the new requirement that content
> lib embedders provide QuotaSettings by overriding CBC::GetQuotaSettings.
> We had thought about having the content lib provide a base impl instead,
> but josh and i thought it would be better to get various owners of content
> lib embedding targets to be aware of the new requirement/ability to override
> these settings to something that makes sense for the particular target.
> 
> ChromeCast and Blimp really should do something that makes sense specifically
> for those targets.

Sure. But you can still do the refactor and change the computation in separate
CLs. Up to you though, was only a suggestion

> 
> > Is there any documentation or experiment data why this the new calculation
is
> > safe? Maybe put that into the description as well? From reading the
> description,
> > if I have 3 different channels of chrome installed, then I can theoretically
> > fill up my drive? For android webview, each app that uses webview is
basically
> > its own chrome instance, so should the calculation be more conservative?
> 
> The settings returned by CalculateNominalDynamicSettings configure the new
> system
> such that it acts like old system.

CalculateNominalDynamicSettings does the "new computation" mentioned in the
description, right? That's not really "acts like old system" :p

> 
> Each profile/storagepartition has its own independent quotamgr, so the problem
> of dueling instances exists outside of webview too. The existing system has
this
> problem too where dueling instances can fill up the drive. In the new system,
> settings.must_remain_available is the primary defense against that.

Can you explain how must_remain_available works to avoid this case? Check
available space before every write? Maybe put the answer into the description as
well.

> 
> > Also wrap CL description at 72 chars.
> 
> Done

lazyboy

lazyboy@chromium.org changed reviewers: + rdevlin.cronin@chromium.org

4 years ago (2016-11-23 00:29:47 UTC) #141

lazyboy

+Devlin for extension_special_storage_policy and extension_storage_monitor as he has made changes there in the past. I've ...

4 years ago (2016-11-23 00:29:50 UTC) #142

Devlin

A few nits and questions, but otherwise lgtm. https://codereview.chromium.org/1782053004/diff/920001/chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc File chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc (right): https://codereview.chromium.org/1782053004/diff/920001/chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc#newcode71 chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc:71: content::BrowserContext* ...

4 years ago (2016-11-23 17:04:13 UTC) #143

michaeln

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

4 years ago (2016-12-03 00:04:15 UTC) #145

commit-bot: I haz the power

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

4 years ago (2016-12-03 00:05:25 UTC) #146

commit-bot: I haz the power

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

4 years ago (2016-12-03 00:46:06 UTC) #147

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linux/builds/271707)

4 years ago (2016-12-03 00:46:08 UTC) #148

michaeln

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

4 years ago (2016-12-03 01:37:28 UTC) #149

commit-bot: I haz the power

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

4 years ago (2016-12-03 01:37:54 UTC) #150

commit-bot: I haz the power

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

4 years ago (2016-12-03 05:18:18 UTC) #151

michaeln

Description was changed from ========== Change how the quota system computes the poolsize for temporary ...

4 years ago (2016-12-05 23:45:17 UTC) #153

michaeln

Description was changed from ========== Change how the quota system computes the poolsize for temporary ...

4 years ago (2016-12-05 23:51:12 UTC) #154

Description was changed from

==========
Change how the quota system computes the poolsize for temporary storage.

Adds a QuotaSettings datastructure and changes how the total poolsize
for temporary storage is determined. This CL adds a public content API
that the embedder must use to inform the quota system of the desired
QuotaSettings.

The old computation: [available_space + current_temp_usage] / 3
The new computation: [total_volume_size - os_overhead] / 3

The new computation is simplified to be a function of the size of the
volume, and no longer takes as input how much chrome is currently using.
This also reduces the time take to initialize the quota system.

To defend against filling the drive, as available space gets "too low",
the calculated per-host quota is clamped to the hosts current usage, this
prevents the writing of new data. Also the eviction logic deletes existing
data to keep |must_remain_available| disk space free.

BUG=520318
==========

to

==========
Change how the quota system computes the poolsize for temporary storage.

Adds a QuotaSettings datastructure and changes how the total poolsize
for temporary storage is determined. This CL adds a public content API
that the embedder must use to inform the quota system of the desired
QuotaSettings.

The old computation: [available_space + current_temp_usage] / 3
The new computation: [total_volume_size - os_overhead] / 3

The new computation is simplified to be a function of the size of the
volume, and no longer takes as input how much chrome is currently using.
This also reduces the time take to initialize the quota system.

To defend against filling the drive, as available space gets "too low",
the calculated per-host quota is clamped to the hosts current usage. This
prevents the writing of new data because quota clients query
GetUsageAndQuota() prior to performing a write. Also the eviction logic
deletes existing data to keep |must_remain_available| disk space free.

BUG=520318
==========

michaeln

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

4 years ago (2016-12-06 00:30:12 UTC) #155

michaeln

https://codereview.chromium.org/1782053004/diff/920001/chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc File chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc (right): https://codereview.chromium.org/1782053004/diff/920001/chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc#newcode71 chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc:71: content::BrowserContext* context = browser()->profile(); On 2016/11/23 17:04:13, Devlin wrote: ...

4 years ago (2016-12-06 00:35:55 UTC) #157

https://codereview.chromium.org/1782053004/diff/920001/chrome/browser/extensi...
File chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc
(right):

https://codereview.chromium.org/1782053004/diff/920001/chrome/browser/extensi...
chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc:71:
content::BrowserContext* context = browser()->profile();
On 2016/11/23 17:04:13, Devlin wrote:
> optional nit: There's a profile() accessor we could use, so I'd probably just
> inline this on line 78.

Done.

https://codereview.chromium.org/1782053004/diff/920001/chrome/browser/extensi...
chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc:80:
->SetQuotaSettings(settings);
On 2016/11/23 17:04:13, Devlin wrote:
> Having a comment explaining at a high level what this is doing and, more
> importantly, why we need to do it in the test, would be useful.

Done.

https://codereview.chromium.org/1782053004/diff/920001/chrome/browser/extensi...
File chrome/browser/extensions/extension_storage_monitor.cc (right):

https://codereview.chromium.org/1782053004/diff/920001/chrome/browser/extensi...
chrome/browser/extensions/extension_storage_monitor.cc:239: state.quota_manager,
event.usage));
On 2016/11/23 17:04:13, Devlin wrote:
> Is per_host_quota able to change between when the state is created and now? 
If
> not, we could cache it on the StorageState and just pass that in, rather than
> needing to pass the refcounted quota manager.  (If it is going to change, then
> this is fine.)

the setting values can change over time, i think the StorageState intances are
long lived, if so then we have to sample the settings value each time

https://codereview.chromium.org/1782053004/diff/920001/extensions/shell/brows...
File extensions/shell/browser/shell_content_browser_client.h (right):

https://codereview.chromium.org/1782053004/diff/920001/extensions/shell/brows...
extensions/shell/browser/shell_content_browser_client.h:46: // TODO(jamescook):
Quota management?
On 2016/11/23 17:04:13, Devlin wrote:
> Can we remove this TODO?

Done.

https://codereview.chromium.org/1782053004/diff/960001/android_webview/browse...
File android_webview/browser/aw_content_browser_client.h (right):

https://codereview.chromium.org/1782053004/diff/960001/android_webview/browse...
android_webview/browser/aw_content_browser_client.h:7: 
> On 2016/11/21 22:56:58, boliu wrote:
>
> CalculateNominalDynamicSettings does the "new computation" mentioned in the
> description, right? That's not really "acts like old system" :p

The settings were chosen such that new behavior should approximate the old
behavior in a new way. The code that
exhibited the old behavior is being deleted so it can't be matched exactly.

> Can you explain how must_remain_available works to avoid this case? Check
> available space before every write? Maybe put the answer into the description
as
> well.

I updated the CL description, also see the comments in quota_settings.h

halliwell

On 2016/12/06 00:35:55, michaeln wrote: > https://codereview.chromium.org/1782053004/diff/920001/chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc > File chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc > (right): > > https://codereview.chromium.org/1782053004/diff/920001/chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc#newcode71 ...

4 years ago (2016-12-06 00:44:07 UTC) #158

On 2016/12/06 00:35:55, michaeln wrote:
>
https://codereview.chromium.org/1782053004/diff/920001/chrome/browser/extensi...
> File
chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc
> (right):
> 
>
https://codereview.chromium.org/1782053004/diff/920001/chrome/browser/extensi...
> chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc:71:
> content::BrowserContext* context = browser()->profile();
> On 2016/11/23 17:04:13, Devlin wrote:
> > optional nit: There's a profile() accessor we could use, so I'd probably
just
> > inline this on line 78.
> 
> Done.
> 
>
https://codereview.chromium.org/1782053004/diff/920001/chrome/browser/extensi...
> chrome/browser/extensions/api/sync_file_system/sync_file_system_apitest.cc:80:
> ->SetQuotaSettings(settings);
> On 2016/11/23 17:04:13, Devlin wrote:
> > Having a comment explaining at a high level what this is doing and, more
> > importantly, why we need to do it in the test, would be useful.
> 
> Done.
> 
>
https://codereview.chromium.org/1782053004/diff/920001/chrome/browser/extensi...
> File chrome/browser/extensions/extension_storage_monitor.cc (right):
> 
>
https://codereview.chromium.org/1782053004/diff/920001/chrome/browser/extensi...
> chrome/browser/extensions/extension_storage_monitor.cc:239:
state.quota_manager,
> event.usage));
> On 2016/11/23 17:04:13, Devlin wrote:
> > Is per_host_quota able to change between when the state is created and now? 
> If
> > not, we could cache it on the StorageState and just pass that in, rather
than
> > needing to pass the refcounted quota manager.  (If it is going to change,
then
> > this is fine.)
> 
> the setting values can change over time, i think the StorageState intances are
> long lived, if so then we have to sample the settings value each time
> 
>
https://codereview.chromium.org/1782053004/diff/920001/extensions/shell/brows...
> File extensions/shell/browser/shell_content_browser_client.h (right):
> 
>
https://codereview.chromium.org/1782053004/diff/920001/extensions/shell/brows...
> extensions/shell/browser/shell_content_browser_client.h:46: //
TODO(jamescook):
> Quota management?
> On 2016/11/23 17:04:13, Devlin wrote:
> > Can we remove this TODO?
> 
> Done.
> 
>
https://codereview.chromium.org/1782053004/diff/960001/android_webview/browse...
> File android_webview/browser/aw_content_browser_client.h (right):
> 
>
https://codereview.chromium.org/1782053004/diff/960001/android_webview/browse...
> android_webview/browser/aw_content_browser_client.h:7: 
> > On 2016/11/21 22:56:58, boliu wrote:
> >
> > CalculateNominalDynamicSettings does the "new computation" mentioned in the
> > description, right? That's not really "acts like old system" :p
> 
> The settings were chosen such that new behavior should approximate the old
> behavior in a new way. The code that
> exhibited the old behavior is being deleted so it can't be matched exactly.
> 
> > Can you explain how must_remain_available works to avoid this case? Check
> > available space before every write? Maybe put the answer into the
description
> as
> > well.
> 
> I updated the CL description, also see the comments in quota_settings.h

chromecast/ lgtm

michaeln

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

4 years ago (2016-12-06 00:46:00 UTC) #159

commit-bot: I haz the power

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

4 years ago (2016-12-06 00:46:33 UTC) #160

commit-bot: I haz the power

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

4 years ago (2016-12-06 02:38:15 UTC) #163

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/350271)

4 years ago (2016-12-06 02:38:17 UTC) #164

michaeln

The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org, kinuko@chromium.org, rdevlin.cronin@chromium.org, calamity@chromium.org, ...

4 years ago (2016-12-06 21:39:41 UTC) #166

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1782053004/1000001

4 years ago (2016-12-06 21:40:14 UTC) #167

commit-bot: I haz the power

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

4 years ago (2016-12-06 21:57:57 UTC) #168

commit-bot: I haz the power

Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/319808)

4 years ago (2016-12-06 21:58:00 UTC) #169

michaeln

I need to find some more owners. ** Presubmit ERRORS ** Missing LGTM from an ...

4 years ago (2016-12-06 22:37:47 UTC) #170

sky

https://codereview.chromium.org/1782053004/diff/1000001/ash/shell/content/client/DEPS File ash/shell/content/client/DEPS (right): https://codereview.chromium.org/1782053004/diff/1000001/ash/shell/content/client/DEPS#newcode4 ash/shell/content/client/DEPS:4: "+storage/browser/quota", For ash and ui/views I see you adding ...

4 years ago (2016-12-06 23:26:24 UTC) #171

michaeln

@sky, thnx for looking https://codereview.chromium.org/1782053004/diff/1000001/content/public/browser/BUILD.gn File content/public/browser/BUILD.gn (right): https://codereview.chromium.org/1782053004/diff/1000001/content/public/browser/BUILD.gn#newcode303 content/public/browser/BUILD.gn:303: "//storage/browser", Good question about //storage/browser ...

4 years ago (2016-12-07 01:45:12 UTC) #172

sky

None-the-less since ui/views is directly including files from storage it should have storage in its ...

4 years ago (2016-12-07 16:23:53 UTC) #175

chromium-reviews

Are you sure? I'm working in ui/views_content_client (not ui/views) and gn check is happy with ...

4 years ago (2016-12-07 20:05:43 UTC) #176

michaeln

The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, calamity@chromium.org, skyostil@chromium.org, boliu@chromium.org, ...

4 years ago (2016-12-08 23:28:12 UTC) #180

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1782053004/1040001

4 years ago (2016-12-08 23:28:48 UTC) #181

commit-bot: I haz the power

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

4 years ago (2016-12-08 23:59:37 UTC) #182

commit-bot: I haz the power

Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/346774)

4 years ago (2016-12-08 23:59:40 UTC) #183

michaeln

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, rdevlin.cronin@chromium.org, calamity@chromium.org, skyostil@chromium.org, ...

4 years ago (2016-12-09 20:43:01 UTC) #185

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1782053004/1060001

4 years ago (2016-12-09 20:44:20 UTC) #186

commit-bot: I haz the power

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

4 years ago (2016-12-09 22:52:45 UTC) #187

commit-bot: I haz the power

Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/286277)

4 years ago (2016-12-09 22:52:49 UTC) #188

michaeln

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

4 years ago (2016-12-12 22:26:57 UTC) #189

commit-bot: I haz the power

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

4 years ago (2016-12-12 22:32:56 UTC) #190

michaeln

https://codereview.chromium.org/1782053004/diff/1080001/chrome/test/base/in_process_browser_test.cc File chrome/test/base/in_process_browser_test.cc (right): https://codereview.chromium.org/1782053004/diff/1080001/chrome/test/base/in_process_browser_test.cc#newcode252 chrome/test/base/in_process_browser_test.cc:252: // Use hardcoded quota settings to have a consistent ...

4 years ago (2016-12-12 23:50:38 UTC) #191

sky

https://codereview.chromium.org/1782053004/diff/1100001/chrome/test/base/in_process_browser_test.cc File chrome/test/base/in_process_browser_test.cc (right): https://codereview.chromium.org/1782053004/diff/1100001/chrome/test/base/in_process_browser_test.cc#newcode254 chrome/test/base/in_process_browser_test.cc:254: static storage::QuotaSettings g_quota_settings(kQuota * 5, kQuota, 0); Only use ...

4 years ago (2016-12-13 01:52:54 UTC) #192

michaeln

The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, calamity@chromium.org, skyostil@chromium.org, boliu@chromium.org, ...

4 years ago (2016-12-13 19:48:14 UTC) #196

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1782053004/1140001

4 years ago (2016-12-13 19:49:23 UTC) #197

commit-bot: I haz the power

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

4 years ago (2016-12-13 21:52:21 UTC) #198

commit-bot: I haz the power

Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/278848) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, ...

4 years ago (2016-12-13 21:52:25 UTC) #199

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1782053004/1140001

4 years ago (2016-12-14 00:31:45 UTC) #201

commit-bot: I haz the power

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

4 years ago (2016-12-14 04:28:28 UTC) #202

commit-bot: I haz the power

Failed to apply patch for chrome/test/DEPS: While running git apply --index -p1; error: patch failed: ...

4 years ago (2016-12-14 04:28:33 UTC) #203

michaeln

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

4 years ago (2016-12-14 19:51:02 UTC) #204

commit-bot: I haz the power

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

4 years ago (2016-12-14 19:51:56 UTC) #205

commit-bot: I haz the power

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

4 years ago (2016-12-14 20:33:27 UTC) #206

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linux/builds/278167)

4 years ago (2016-12-14 20:33:30 UTC) #207

michaeln

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, rdevlin.cronin@chromium.org, calamity@chromium.org, skyostil@chromium.org, ...

4 years ago (2016-12-14 20:54:58 UTC) #209

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1782053004/1180001

4 years ago (2016-12-14 20:55:45 UTC) #210

commit-bot: I haz the power

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

4 years ago (2016-12-15 01:04:29 UTC) #211

commit-bot: I haz the power

Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/356506)

4 years ago (2016-12-15 01:04:33 UTC) #212

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1782053004/1180001

4 years ago (2016-12-15 02:32:10 UTC) #214

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 1180001, "attempt_start_ts": 1481769113649780, "parent_rev": "4728f7fdaeb65806d306fb1ae1398a9fce4e2108", "commit_rev": "2a98bca732a767d7d47260dc2d595ce7853222a0"}

4 years ago (2016-12-15 04:35:27 UTC) #215

commit-bot: I haz the power

Description was changed from ========== Change how the quota system computes the poolsize for temporary ...

4 years ago (2016-12-15 04:35:54 UTC) #216

Message was sent while issue was closed.

Description was changed from

==========
Change how the quota system computes the poolsize for temporary storage.

Adds a QuotaSettings datastructure and changes how the total poolsize
for temporary storage is determined. This CL adds a public content API
that the embedder must use to inform the quota system of the desired
QuotaSettings.

The old computation: [available_space + current_temp_usage] / 3
The new computation: [total_volume_size - os_overhead] / 3

The new computation is simplified to be a function of the size of the
volume, and no longer takes as input how much chrome is currently using.
This also reduces the time take to initialize the quota system.

To defend against filling the drive, as available space gets "too low",
the calculated per-host quota is clamped to the hosts current usage. This
prevents the writing of new data because quota clients query
GetUsageAndQuota() prior to performing a write. Also the eviction logic
deletes existing data to keep |must_remain_available| disk space free.

BUG=520318
==========

to

==========
Change how the quota system computes the poolsize for temporary storage.

Adds a QuotaSettings datastructure and changes how the total poolsize
for temporary storage is determined. This CL adds a public content API
that the embedder must use to inform the quota system of the desired
QuotaSettings.

The old computation: [available_space + current_temp_usage] / 3
The new computation: [total_volume_size - os_overhead] / 3

The new computation is simplified to be a function of the size of the
volume, and no longer takes as input how much chrome is currently using.
This also reduces the time take to initialize the quota system.

To defend against filling the drive, as available space gets "too low",
the calculated per-host quota is clamped to the hosts current usage. This
prevents the writing of new data because quota clients query
GetUsageAndQuota() prior to performing a write. Also the eviction logic
deletes existing data to keep |must_remain_available| disk space free.

BUG=520318

Review-Url: https://codereview.chromium.org/1782053004
==========

commit-bot: I haz the power

Description was changed from ========== Change how the quota system computes the poolsize for temporary ...

4 years ago (2016-12-15 04:38:23 UTC) #218

Message was sent while issue was closed.

Description was changed from

==========
Change how the quota system computes the poolsize for temporary storage.

Adds a QuotaSettings datastructure and changes how the total poolsize
for temporary storage is determined. This CL adds a public content API
that the embedder must use to inform the quota system of the desired
QuotaSettings.

The old computation: [available_space + current_temp_usage] / 3
The new computation: [total_volume_size - os_overhead] / 3

The new computation is simplified to be a function of the size of the
volume, and no longer takes as input how much chrome is currently using.
This also reduces the time take to initialize the quota system.

To defend against filling the drive, as available space gets "too low",
the calculated per-host quota is clamped to the hosts current usage. This
prevents the writing of new data because quota clients query
GetUsageAndQuota() prior to performing a write. Also the eviction logic
deletes existing data to keep |must_remain_available| disk space free.

BUG=520318

Review-Url: https://codereview.chromium.org/1782053004
==========

to

==========
Change how the quota system computes the poolsize for temporary storage.

Adds a QuotaSettings datastructure and changes how the total poolsize
for temporary storage is determined. This CL adds a public content API
that the embedder must use to inform the quota system of the desired
QuotaSettings.

The old computation: [available_space + current_temp_usage] / 3
The new computation: [total_volume_size - os_overhead] / 3

The new computation is simplified to be a function of the size of the
volume, and no longer takes as input how much chrome is currently using.
This also reduces the time take to initialize the quota system.

To defend against filling the drive, as available space gets "too low",
the calculated per-host quota is clamped to the hosts current usage. This
prevents the writing of new data because quota clients query
GetUsageAndQuota() prior to performing a write. Also the eviction logic
deletes existing data to keep |must_remain_available| disk space free.

BUG=520318

Committed: https://crrev.com/db78e67f2c47c3b2e7bd548d9d1a4e4f1b3124e4
Cr-Commit-Position: refs/heads/master@{#438741}
==========

commit-bot: I haz the power

Patchset 60 (id:??) landed as https://crrev.com/db78e67f2c47c3b2e7bd548d9d1a4e4f1b3124e4 Cr-Commit-Position: refs/heads/master@{#438741}

4 years ago (2016-12-15 04:38:27 UTC) #219

jsbell

FYI, reverting due to https://bugs.chromium.org/p/chromium/issues/detail?id=675875 Revert CL is https://codereview.chromium.org/2592793002 (too big to just hit the ...

4 years ago (2016-12-20 19:45:12 UTC) #221

michaeln

Description was changed from ========== Change how the quota system computes the poolsize for temporary ...

3 years, 11 months ago (2017-01-05 01:30:47 UTC) #222

Message was sent while issue was closed.

Description was changed from

==========
Change how the quota system computes the poolsize for temporary storage.

Adds a QuotaSettings datastructure and changes how the total poolsize
for temporary storage is determined. This CL adds a public content API
that the embedder must use to inform the quota system of the desired
QuotaSettings.

The old computation: [available_space + current_temp_usage] / 3
The new computation: [total_volume_size - os_overhead] / 3

The new computation is simplified to be a function of the size of the
volume, and no longer takes as input how much chrome is currently using.
This also reduces the time take to initialize the quota system.

To defend against filling the drive, as available space gets "too low",
the calculated per-host quota is clamped to the hosts current usage. This
prevents the writing of new data because quota clients query
GetUsageAndQuota() prior to performing a write. Also the eviction logic
deletes existing data to keep |must_remain_available| disk space free.

BUG=520318

Committed: https://crrev.com/db78e67f2c47c3b2e7bd548d9d1a4e4f1b3124e4
Cr-Commit-Position: refs/heads/master@{#438741}
==========

to

==========
Change how the quota system computes the poolsize for temporary storage.

Adds a QuotaSettings datastructure and changes how the total poolsize
for temporary storage is determined. This CL adds a public content API
that the embedder must use to inform the quota system of the desired
QuotaSettings.

The old computation: [available_space + current_temp_usage] / 3
The new computation: [total_volume_size - os_overhead] / 3

The new computation is simplified to be a function of the size of the
volume, and no longer takes as input how much chrome is currently using.
This also reduces the time take to initialize the quota system.

To defend against filling the drive, as available space gets "too low",
the calculated per-host quota is clamped to the hosts current usage. This
prevents the writing of new data because quota clients query
GetUsageAndQuota() prior to performing a write. Also the eviction logic
deletes existing data to keep |must_remain_available| disk space free.

BUG=520318

Committed: https://crrev.com/db78e67f2c47c3b2e7bd548d9d1a4e4f1b3124e4
Cr-Commit-Position: refs/heads/master@{#438741}
==========

michaeln

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

3 years, 11 months ago (2017-01-26 21:45:26 UTC) #223

commit-bot: I haz the power

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

3 years, 11 months ago (2017-01-26 21:46:04 UTC) #224

commit-bot: I haz the power

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

3 years, 10 months ago (2017-01-27 02:17:49 UTC) #225

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 10 months ago (2017-01-27 02:17:52 UTC) #226

michaeln

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

3 years, 10 months ago (2017-01-31 22:21:24 UTC) #227

commit-bot: I haz the power

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

3 years, 10 months ago (2017-01-31 22:22:19 UTC) #228

michaeln

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

3 years, 10 months ago (2017-02-01 00:13:38 UTC) #229

michaeln

https://codereview.chromium.org/1782053004/diff/1300001/storage/browser/quota/quota_manager.cc File storage/browser/quota/quota_manager.cc (right): https://codereview.chromium.org/1782053004/diff/1300001/storage/browser/quota/quota_manager.cc#newcode1663 storage/browser/quota/quota_manager.cc:1663: std::pair<int64_t, int64_t> total_and_available = get_volume_info_fn(path); @chris, i'll change this ...

3 years, 10 months ago (2017-02-01 00:13:42 UTC) #230

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-01 02:09:09 UTC) #231

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 10 months ago (2017-02-01 02:09:12 UTC) #232

michaeln

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

3 years, 10 months ago (2017-02-02 01:54:59 UTC) #233

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-02 01:55:38 UTC) #234

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-02 01:59:14 UTC) #235

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-02 01:59:16 UTC) #236

michaeln

Description was changed from ========== Change how the quota system computes the poolsize for temporary ...

3 years, 10 months ago (2017-02-02 20:41:15 UTC) #237

Description was changed from

==========
Change how the quota system computes the poolsize for temporary storage.

Adds a QuotaSettings datastructure and changes how the total poolsize
for temporary storage is determined. This CL adds a public content API
that the embedder must use to inform the quota system of the desired
QuotaSettings.

The old computation: [available_space + current_temp_usage] / 3
The new computation: [total_volume_size - os_overhead] / 3

The new computation is simplified to be a function of the size of the
volume, and no longer takes as input how much chrome is currently using.
This also reduces the time take to initialize the quota system.

To defend against filling the drive, as available space gets "too low",
the calculated per-host quota is clamped to the hosts current usage. This
prevents the writing of new data because quota clients query
GetUsageAndQuota() prior to performing a write. Also the eviction logic
deletes existing data to keep |must_remain_available| disk space free.

BUG=520318

Committed: https://crrev.com/db78e67f2c47c3b2e7bd548d9d1a4e4f1b3124e4
Cr-Commit-Position: refs/heads/master@{#438741}
==========

to

==========
Change how the quota system computes the poolsize for temporary storage.

Adds a QuotaSettings datastructure and changes how the total poolsize
for temporary storage is determined. This CL adds a public content API
that the embedder must use to inform the quota system of the desired
QuotaSettings.

The old computation: [available_space + current_temp_usage] / 3
The new computation: [total_volume_size - os_overhead] / 3

The new computation is simplified to be a function of the size of the
volume, and no longer takes as input how much chrome is currently using.
This also reduces the time take to initialize the quota system.

To defend against filling the drive, as available space gets "too low", as it
approaches |must_remain_available|, the calculated per-host quota is clamped to
the hosts current usage. This prevents the writing of new data because quota
clients query GetUsageAndQuota() prior to performing a write. Also the eviction
logic deletes existing data to keep |should_remain_available| disk space free.

Settings.must_remain_available = 1% of total_volume_size
Settings.should_remain_available = 10%

BUG=520318

Committed: https://crrev.com/db78e67f2c47c3b2e7bd548d9d1a4e4f1b3124e4
Cr-Commit-Position: refs/heads/master@{#438741}
==========

michaeln

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

3 years, 10 months ago (2017-02-02 21:31:29 UTC) #238

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-02 21:32:20 UTC) #239

michaeln

Description was changed from ========== Change how the quota system computes the poolsize for temporary ...

3 years, 10 months ago (2017-02-02 22:32:04 UTC) #240

Description was changed from

==========
Change how the quota system computes the poolsize for temporary storage.

Adds a QuotaSettings datastructure and changes how the total poolsize
for temporary storage is determined. This CL adds a public content API
that the embedder must use to inform the quota system of the desired
QuotaSettings.

The old computation: [available_space + current_temp_usage] / 3
The new computation: [total_volume_size - os_overhead] / 3

The new computation is simplified to be a function of the size of the
volume, and no longer takes as input how much chrome is currently using.
This also reduces the time take to initialize the quota system.

To defend against filling the drive, as available space gets "too low", as it
approaches |must_remain_available|, the calculated per-host quota is clamped to
the hosts current usage. This prevents the writing of new data because quota
clients query GetUsageAndQuota() prior to performing a write. Also the eviction
logic deletes existing data to keep |should_remain_available| disk space free.

Settings.must_remain_available = 1% of total_volume_size
Settings.should_remain_available = 10%

BUG=520318

Committed: https://crrev.com/db78e67f2c47c3b2e7bd548d9d1a4e4f1b3124e4
Cr-Commit-Position: refs/heads/master@{#438741}
==========

to

==========
Change how the quota system computes the poolsize for temporary storage.

Adds a QuotaSettings datastructure and changes how the total poolsize
for temporary storage is determined. This CL adds a public content API
that the embedder must use to inform the quota system of the desired
QuotaSettings.

The old computation: [available_space + current_temp_usage] / 3
The new computation: [total_volume_size - os_overhead] / 3

The new computation is simplified to be a function of the size of the
volume, and no longer takes as input how much chrome is currently using.
This also reduces the time take to initialize the quota system.

To defend against filling the drive, as available space gets "too low", as it
approaches |must_remain_available|, the calculated per-host quota is clamped to
the hosts current usage. This prevents the writing of new data because quota
clients query GetUsageAndQuota() prior to performing a write. Also the eviction
logic deletes existing data to keep |should_remain_available| disk space free.

Settings.must_remain_available = 1% of total_volume_size
Settings.should_remain_available = 10%

BUG=520318
==========

cmumford

lgtm % nits/questions https://codereview.chromium.org/1782053004/diff/1340001/storage/browser/quota/quota_manager.cc File storage/browser/quota/quota_manager.cc (right): https://codereview.chromium.org/1782053004/diff/1340001/storage/browser/quota/quota_manager.cc#newcode420 storage/browser/quota/quota_manager.cc:420: static_cast<int>((global_usage_ * 100) / total_space_)); possible ...

3 years, 10 months ago (2017-02-03 00:21:12 UTC) #241

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-03 01:25:39 UTC) #242

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 10 months ago (2017-02-03 01:25:42 UTC) #243

michaeln

thnx https://codereview.chromium.org/1782053004/diff/1340001/storage/browser/quota/quota_manager.cc File storage/browser/quota/quota_manager.cc (right): https://codereview.chromium.org/1782053004/diff/1340001/storage/browser/quota/quota_manager.cc#newcode420 storage/browser/quota/quota_manager.cc:420: static_cast<int>((global_usage_ * 100) / total_space_)); On 2017/02/03 00:21:12, ...

3 years, 10 months ago (2017-02-03 01:56:12 UTC) #244

michaeln

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

3 years, 10 months ago (2017-02-03 01:57:42 UTC) #245

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-03 01:58:11 UTC) #246

commit-bot: I haz the power

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

3 years, 10 months ago (2017-02-03 06:02:18 UTC) #247

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 10 months ago (2017-02-03 06:02:22 UTC) #248

michaeln

Description was changed from ========== Change how the quota system computes the poolsize for temporary ...

3 years, 10 months ago (2017-02-06 21:33:52 UTC) #249

Description was changed from

==========
Change how the quota system computes the poolsize for temporary storage.

Adds a QuotaSettings datastructure and changes how the total poolsize
for temporary storage is determined. This CL adds a public content API
that the embedder must use to inform the quota system of the desired
QuotaSettings.

The old computation: [available_space + current_temp_usage] / 3
The new computation: [total_volume_size - os_overhead] / 3

The new computation is simplified to be a function of the size of the
volume, and no longer takes as input how much chrome is currently using.
This also reduces the time take to initialize the quota system.

To defend against filling the drive, as available space gets "too low", as it
approaches |must_remain_available|, the calculated per-host quota is clamped to
the hosts current usage. This prevents the writing of new data because quota
clients query GetUsageAndQuota() prior to performing a write. Also the eviction
logic deletes existing data to keep |should_remain_available| disk space free.

Settings.must_remain_available = 1% of total_volume_size
Settings.should_remain_available = 10%

BUG=520318
==========

to

==========
Change how the quota system computes the poolsize for temporary storage.

Adds a QuotaSettings datastructure and changes how the total poolsize
for temporary storage is determined. This CL adds a public content API
that the embedder must use to inform the quota system of the desired
QuotaSettings.

The old computation: [available_space + current_temp_usage] / 3
The new computation: [total_volume_size - os_overhead] / 3

The new computation is simplified to be a function of the size of the
volume, and no longer takes as input how much chrome is currently using.
This also reduces the time take to initialize the quota system.

To defend against filling the drive, as available space gets "too low",
as it approaches |must_remain_available|, the calculated per-host quota is
clamped to the hosts current usage. This prevents the writing of new data
because quota clients query GetUsageAndQuota() prior to performing a write.
Also the eviction logic deletes existing data to keep
|should_remain_available| disk space free.

Settings.must_remain_available = 1% of total_volume_size
Settings.should_remain_available = 10%

BUG=520318
==========

michaeln

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, rdevlin.cronin@chromium.org, calamity@chromium.org, skyostil@chromium.org, ...

3 years, 10 months ago (2017-02-06 21:34:43 UTC) #251

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/1782053004/1380001

3 years, 10 months ago (2017-02-06 21:35:36 UTC) #252

jsbell

What are the meaningful changes since the initial land/revert?

3 years, 10 months ago (2017-02-06 23:37:30 UTC) #253

michaeln

On 2017/02/06 23:37:30, jsbell wrote: > What are the meaningful changes since the initial land/revert? ...

3 years, 10 months ago (2017-02-07 00:11:19 UTC) #254

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 1380001, "attempt_start_ts": 1486416882119330, "parent_rev": "42407c7feb218491f72b05732fa83b4679144da4", "commit_rev": "10e5fc3598c9f2e1015458c814a1163680465b3e"}

3 years, 10 months ago (2017-02-07 02:08:04 UTC) #255

commit-bot: I haz the power

Description was changed from ========== Change how the quota system computes the poolsize for temporary ...

3 years, 10 months ago (2017-02-07 02:10:02 UTC) #256

Message was sent while issue was closed.

Description was changed from

==========
Change how the quota system computes the poolsize for temporary storage.

Adds a QuotaSettings datastructure and changes how the total poolsize
for temporary storage is determined. This CL adds a public content API
that the embedder must use to inform the quota system of the desired
QuotaSettings.

The old computation: [available_space + current_temp_usage] / 3
The new computation: [total_volume_size - os_overhead] / 3

The new computation is simplified to be a function of the size of the
volume, and no longer takes as input how much chrome is currently using.
This also reduces the time take to initialize the quota system.

To defend against filling the drive, as available space gets "too low",
as it approaches |must_remain_available|, the calculated per-host quota is
clamped to the hosts current usage. This prevents the writing of new data
because quota clients query GetUsageAndQuota() prior to performing a write.
Also the eviction logic deletes existing data to keep
|should_remain_available| disk space free.

Settings.must_remain_available = 1% of total_volume_size
Settings.should_remain_available = 10%

BUG=520318
==========

to

==========
Change how the quota system computes the poolsize for temporary storage.

Adds a QuotaSettings datastructure and changes how the total poolsize
for temporary storage is determined. This CL adds a public content API
that the embedder must use to inform the quota system of the desired
QuotaSettings.

The old computation: [available_space + current_temp_usage] / 3
The new computation: [total_volume_size - os_overhead] / 3

The new computation is simplified to be a function of the size of the
volume, and no longer takes as input how much chrome is currently using.
This also reduces the time take to initialize the quota system.

To defend against filling the drive, as available space gets "too low",
as it approaches |must_remain_available|, the calculated per-host quota is
clamped to the hosts current usage. This prevents the writing of new data
because quota clients query GetUsageAndQuota() prior to performing a write.
Also the eviction logic deletes existing data to keep
|should_remain_available| disk space free.

Settings.must_remain_available = 1% of total_volume_size
Settings.should_remain_available = 10%

BUG=520318

Review-Url: https://codereview.chromium.org/1782053004
Cr-Commit-Position: refs/heads/master@{#448510}
Committed:
https://chromium.googlesource.com/chromium/src/+/10e5fc3598c9f2e1015458c814a1...
==========

commit-bot: I haz the power

3 years, 10 months ago (2017-02-07 02:10:06 UTC) #257

Message was sent while issue was closed.

Committed patchset #70 (id:1380001) as
https://chromium.googlesource.com/chromium/src/+/10e5fc3598c9f2e1015458c814a1...

Issue 1782053004: Change how the quota system computes the total poolsize for temporary storage (Closed)