Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2777183010:
[Quota] Lower quota for ephemeral mode (ie. session only) (Closed)
Description[Quota] Lower quota for ephemeral mode (ie. session only)
The data for session only origins is ephemeral, it gets deleted at the end of each
browsing session. This change lowers the storage quota allotted to them.
BUG=619927
Review-Url: https://codereview.chromium.org/2777183010
Cr-Commit-Position: refs/heads/master@{#463501}
Committed: https://chromium.googlesource.com/chromium/src/+/fa4c8940a8d8d6a84c60bc1f108b1d6a86ef3437
Patch Set 1 #Patch Set 2 : compile #Patch Set 3 : ctor #Patch Set 4 : randomize #
Total comments: 12
Patch Set 5 : comments #Patch Set 6 : unit test #
Total comments: 6
Patch Set 7 : comments #
Messages
Total messages: 39 (25 generated)
The CQ bit was checked by michaeln@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Lower quota for ephemeral mode (ie. session only) The data for session only origins is ephemeral, it gets deleted at the end of each browsing session. This change lowers the per_host_quota is allotted to them. BUG=619927 ========== to ========== Lower quota for ephemeral mode (ie. session only) The data for session only origins is ephemeral, it gets deleted at the end of each browsing session. This change lowers the storage quota allotted to them. BUG=619927 ==========
Description was changed from ========== Lower quota for ephemeral mode (ie. session only) The data for session only origins is ephemeral, it gets deleted at the end of each browsing session. This change lowers the storage quota allotted to them. BUG=619927 ========== to ========== [Quota] Lower quota for ephemeral mode (ie. session only) The data for session only origins is ephemeral, it gets deleted at the end of each browsing session. This change lowers the storage quota allotted to them. BUG=619927 ==========
The CQ bit was checked by michaeln@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
michaeln@chromium.org changed reviewers: + cmumford@chromium.org, jsbell@chromium.org
The CQ bit was checked by michaeln@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal, also see the comments in the bug, they mention a concern that chris had about this change potentially doing the wrong thing
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Initial nits We should probably loop in someone more authoritative about ephemeral/session-only mode for guidance. https://codereview.chromium.org/2777183010/diff/60001/storage/browser/quota/q... File storage/browser/quota/quota_settings.cc (right): https://codereview.chromium.org/2777183010/diff/60001/storage/browser/quota/q... storage/browser/quota/quota_settings.cc:33: const int k10Percent = 10; This should probably be named with respect to what it does, e.g. kRandomizedPercentage https://codereview.chromium.org/2777183010/diff/60001/storage/browser/quota/q... storage/browser/quota/quota_settings.cc:37: settings.pool_size = std::min(RandomizeByPercent(300 * kMBytes, k10Percent), Can we make the 300*kMBytes a constant, e.g. kMaxMemoryQuotaMBytes https://codereview.chromium.org/2777183010/diff/60001/storage/browser/quota/q... storage/browser/quota/quota_settings.cc:38: base::SysInfo::AmountOfPhysicalMemory() / 10); Can we make the 10 here a constant, e.g. kMemoryQuotaDivisor https://codereview.chromium.org/2777183010/diff/60001/storage/browser/quota/q... File storage/browser/quota/quota_settings.h (right): https://codereview.chromium.org/2777183010/diff/60001/storage/browser/quota/q... storage/browser/quota/quota_settings.h:41: // according to the SpecialStoargePolicy provided by the embedder. typo: 'Stoarge'
https://codereview.chromium.org/2777183010/diff/60001/storage/browser/quota/q... File storage/browser/quota/quota_manager.cc (right): https://codereview.chromium.org/2777183010/diff/60001/storage/browser/quota/q... storage/browser/quota/quota_manager.cc:850: bool is_session_only = special_storage_policy_.get() && Nit: scoped_refptr has: explicit operator bool() const { return ptr_ != nullptr; } so you can omit the ".get()" right? https://codereview.chromium.org/2777183010/diff/60001/storage/browser/quota/q... File storage/browser/quota/quota_settings.cc (right): https://codereview.chromium.org/2777183010/diff/60001/storage/browser/quota/q... storage/browser/quota/quota_settings.cc:98: std::min(RandomizeByPercent(300 * kMBytes, k10Percent), This doesn't actually do what the title of issue 619927 requests since it ignores physical memory. If that's desired then how about creating a method, named something like CalculateEphemeralOriginQuota(), and using it here, and up above in the incognito section?
The CQ bit was checked by michaeln@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2777183010/diff/60001/storage/browser/quota/q... File storage/browser/quota/quota_manager.cc (right): https://codereview.chromium.org/2777183010/diff/60001/storage/browser/quota/q... storage/browser/quota/quota_manager.cc:850: bool is_session_only = special_storage_policy_.get() && On 2017/04/04 16:38:29, cmumford wrote: > Nit: scoped_refptr has: > > explicit operator bool() const { return ptr_ != nullptr; } > > so you can omit the ".get()" right? Done. https://codereview.chromium.org/2777183010/diff/60001/storage/browser/quota/q... File storage/browser/quota/quota_settings.cc (right): https://codereview.chromium.org/2777183010/diff/60001/storage/browser/quota/q... storage/browser/quota/quota_settings.cc:33: const int k10Percent = 10; On 2017/04/04 16:23:57, jsbell wrote: > This should probably be named with respect to what it does, e.g. > kRandomizedPercentage > Done. https://codereview.chromium.org/2777183010/diff/60001/storage/browser/quota/q... storage/browser/quota/quota_settings.cc:37: settings.pool_size = std::min(RandomizeByPercent(300 * kMBytes, k10Percent), On 2017/04/04 16:23:57, jsbell wrote: > Can we make the 300*kMBytes a constant, e.g. kMaxMemoryQuotaMBytes Done. https://codereview.chromium.org/2777183010/diff/60001/storage/browser/quota/q... storage/browser/quota/quota_settings.cc:38: base::SysInfo::AmountOfPhysicalMemory() / 10); On 2017/04/04 16:23:57, jsbell wrote: > Can we make the 10 here a constant, e.g. kMemoryQuotaDivisor Done. https://codereview.chromium.org/2777183010/diff/60001/storage/browser/quota/q... storage/browser/quota/quota_settings.cc:98: std::min(RandomizeByPercent(300 * kMBytes, k10Percent), On 2017/04/04 16:38:29, cmumford wrote: > This doesn't actually do what the title of issue 619927 requests since it > ignores physical memory. If that's desired then how about creating a method, > named something like CalculateEphemeralOriginQuota(), and using it here, and up > above in the incognito section? I've decoupled them from one another rather than linked them together. https://codereview.chromium.org/2777183010/diff/60001/storage/browser/quota/q... File storage/browser/quota/quota_settings.h (right): https://codereview.chromium.org/2777183010/diff/60001/storage/browser/quota/q... storage/browser/quota/quota_settings.h:41: // according to the SpecialStoargePolicy provided by the embedder. On 2017/04/04 16:23:57, jsbell wrote: > typo: 'Stoarge' Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jsbell@chromium.org changed reviewers: + tnagel@chromium.org
+tnagel who weighed in on the bug and may have more context about Enterprise use of ephemeral mode
The CQ bit was checked by michaeln@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I think the code for this is ready modulo maybe enterprise ephemeral mode which i'm not sure we want or need to alter behavior for. I suspect we don't since cros users may stay signed in for weeks or months at a time and its not a user setting where the user made a conscious decision to deny long lived storage. Also, I have manually observed that IsStorageSessionOnly(ori) returns the desired value depending on how the top level setting and/or exceptions for only particular sites are made in the content settings gui.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2777183010/diff/100001/storage/browser/quota/... File storage/browser/quota/quota_settings.cc (right): https://codereview.chromium.org/2777183010/diff/100001/storage/browser/quota/... storage/browser/quota/quota_settings.cc:23: double random_percent = (base::RandDouble() - 0.5) * percent; Since this is *half* of the percentage, maybe * 2 here and / 100.0 on the next line, just for readability's sake? https://codereview.chromium.org/2777183010/diff/100001/storage/browser/quota/... storage/browser/quota/quota_settings.cc:27: } // anon namespace nit: don't abbreviate anonymous (looks like you did that in a few other files but it's usually spelled out)
> I think the code for this is ready modulo maybe enterprise ephemeral mode which > i'm not sure we want or need to alter behavior for. I suspect we don't since > cros users may stay signed in for weeks or months at a time and its not a user > setting where the user made a conscious decision to deny long lived storage. Why not include enterprise ephemeral? Imho, ephemeral should behave identical independent of whether it's enabled by a user or by an admin. By far the most common use case of enterprise ephemeral are short-lived (~1 hour) sessions in a classroom setting, which imho would benefit substantially from a quota reduction. https://codereview.chromium.org/2777183010/diff/100001/content/browser/quota/... File content/browser/quota/quota_manager_unittest.cc (right): https://codereview.chromium.org/2777183010/diff/100001/content/browser/quota/... content/browser/quota/quota_manager_unittest.cc:171: (per_host_quota > 0) ? (per_host_quota - 1) : INT64_C(0); Nit: What's the purpose of INT64_C() here?
In managed environments, can the administrator prescribe content settings too? If so, then I'd say we can let them decide the policy for their environment. Do you know how to test for CrOS ephemeral mode? To test for it, i imagine we'd need code up in /chrome. Why do you say it would be a substantial benefit to reduce quota in this setting? https://codereview.chromium.org/2777183010/diff/100001/content/browser/quota/... File content/browser/quota/quota_manager_unittest.cc (right): https://codereview.chromium.org/2777183010/diff/100001/content/browser/quota/... content/browser/quota/quota_manager_unittest.cc:171: (per_host_quota > 0) ? (per_host_quota - 1) : INT64_C(0); On 2017/04/10 09:41:37, Thiemo Nagel wrote: > Nit: What's the purpose of INT64_C() here? I'll remove it if its not needed to compile without warnings.
@Thiemo, the code tells the the storage layer how to treat an origins data is up
in chrome. The function that influences the behavior change in this CL is
here...
bool ExtensionSpecialStoragePolicy::IsStorageSessionOnly(const GURL& origin) {
if (cookie_settings_.get() == NULL)
return false;
return cookie_settings_->IsCookieSessionOnly(origin);
}
I don't know if this function is aware of CrOS ephemeral mode but I imagine it
could be if needed.
https://codereview.chromium.org/2777183010/diff/100001/storage/browser/quota/... File storage/browser/quota/quota_settings.cc (right): https://codereview.chromium.org/2777183010/diff/100001/storage/browser/quota/... storage/browser/quota/quota_settings.cc:23: double random_percent = (base::RandDouble() - 0.5) * percent; On 2017/04/07 23:48:58, jsbell wrote: > Since this is *half* of the percentage, maybe * 2 here and / 100.0 on the next > line, just for readability's sake? Done, but it's an additional multiply :) https://codereview.chromium.org/2777183010/diff/100001/storage/browser/quota/... storage/browser/quota/quota_settings.cc:27: } // anon namespace On 2017/04/07 23:48:58, jsbell wrote: > nit: don't abbreviate anonymous (looks like you did that in a few other files > but it's usually spelled out) Done.
The CQ bit was checked by michaeln@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org Link to the patchset: https://codereview.chromium.org/2777183010/#ps120001 (title: "comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491872784623120,
"parent_rev": "286c2eb50c88164fa96b2957ca0cb299c1a14215", "commit_rev":
"fa4c8940a8d8d6a84c60bc1f108b1d6a86ef3437"}
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491872784623120,
"parent_rev": "286c2eb50c88164fa96b2957ca0cb299c1a14215", "commit_rev":
"fa4c8940a8d8d6a84c60bc1f108b1d6a86ef3437"}
Message was sent while issue was closed.
Description was changed from ========== [Quota] Lower quota for ephemeral mode (ie. session only) The data for session only origins is ephemeral, it gets deleted at the end of each browsing session. This change lowers the storage quota allotted to them. BUG=619927 ========== to ========== [Quota] Lower quota for ephemeral mode (ie. session only) The data for session only origins is ephemeral, it gets deleted at the end of each browsing session. This change lowers the storage quota allotted to them. BUG=619927 Review-Url: https://codereview.chromium.org/2777183010 Cr-Commit-Position: refs/heads/master@{#463501} Committed: https://chromium.googlesource.com/chromium/src/+/fa4c8940a8d8d6a84c60bc1f108b... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/fa4c8940a8d8d6a84c60bc1f108b...
Message was sent while issue was closed.
Findit identified this CL at revision 463501 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
