feat: Allow partitions to have app set quotas OS-14197 #3
Conversation
This patch allows partitions to have a quota size, which in turn perusades the storage manager to run a garbage collector(at specific times) to free reclaimable storage space.
patches/chromium/feat_add_storage_quota_capability_for_partitions.patch
Outdated
Show resolved
Hide resolved
patches/chromium/feat_add_storage_quota_capability_for_partitions.patch
Outdated
Show resolved
Hide resolved
| content::StoragePartitionConfig::CreateDefault(this, quota_size)); | ||
| } | ||
| return BrowserContext::GetDefaultStoragePartition(); | ||
| } |
There was a problem hiding this comment.
How about this:
content::StoragePartition* partition = BrowserContext::GetDefaultStoragePartition();
partition->GetQuotaManager()->SetQuotaSettings(storage::GetHardCodedSettings(quota));
return partition;
You can then avoid modifying StoragePartitionConfig.
There was a problem hiding this comment.
How about this:
content::StoragePartition* partition = BrowserContext::GetDefaultStoragePartition(); partition->GetQuotaManager()->SetQuotaSettings(storage::GetHardCodedSettings(quota)); return partition;You can then avoid modifying StoragePartitionConfig.
I had a try with these changes. To have SetQuotaSettings() callable, the header file storage/browser/quota/quota_manager.h had to be added.
Since the SetQuotaSettings call can be done only on the IOThread, I tried the snippet below, but then there was a DCHECK fault since the IOthread is not accessible within the electron layer.
content::StoragePartition* partition = BrowserContext::GetDefaultStoragePartition();
base::WeakPtrFactory<storage::QuotaManager> quota_weak_factory_{partition->GetQuotaManager()};
content::GetIOThreadTaskRunner({})->PostTask(
FROM_HERE, base::BindOnce(&storage::QuotaManager::SetQuotaSettings,
quota_weak_factory_.GetWeakPtr(),storage::GetHardCodedSettings(quota_size)));
There was a problem hiding this comment.
What is the exact DCHECK error? ElectronBrowserContext::~ElectronBrowserContext() has these lines:
BrowserThread::DeleteSoon(BrowserThread::IO, FROM_HERE, std::move(resource_context_));
patches/chromium/feat_add_storage_quota_capability_for_partitions.patch
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
As a general change:
You have used quota = 0 as default quota setting. To my undestanding quota=0 means, you cannot write anything. There is no empty space or you have no right to write.
That is why I asked you to use absl::optional. Then we can differentiate between 0, a value and not defined. Sorry I wasn't very clear when I asked you to use absl::optional.
spec/api-session-spec.ts
Outdated
| @@ -31,6 +31,30 @@ describe('session module', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('session.fromPartition(partition, options)', () => { | |||
There was a problem hiding this comment.
We also need test to make sure default values are used when quotasize option is not defined.
We also need negative tests. Like defining negative numbers, too large numbers, 0 etc.
There was a problem hiding this comment.
I did try to add a test which queries the disk free space of the specified path. However, this test then cannot be run on all platforms and hence would fail. The procedure I followed was to use the childProcess to run df <path>, but that has to be further extracted and again this would not work on Windows which then would then fail the test and not allow the patch to land upstream.
caneraltinbasak
changed the title
Description of Change
Allow the passing of a quota to the session.fromPartition() API.
This patch allows partitions to have a quota size, which in turn persuades
the storage manager to run a garbage collector(at specific times) to free
reclaimable storage space.
Checklist
Release Notes
notes: Allow the passing of a quota to the session.fromPartition() API.