Closed Bug 593045 Opened 10 years ago Closed 10 years ago

Add SQLite file size quota management to mozStorage

Categories

(Toolkit :: Storage, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

References

Details

Attachments

(2 files)

Attached patch Patch, v1Splinter Review
I need this ASAP for IndexedDB quota management. Patch attached, thanks to the SQLite team for the VFS shim.

Eventually we should merge the new interface with mozIStorageService. It's separate in this patch to avoid changing interfaces on a locked down tree.
Attachment #471506 - Flags: review?(sdwilsh)
Attachment #471506 - Flags: review?(bugmail)
Comment on attachment 471506 [details] [diff] [review]
Patch, v1

(it's probably better if sdwilsh reviews this given his familiarity with IndexedDB and the architecture decisions involved)
Attachment #471506 - Flags: review?(bugmail)
(In reply to comment #1)
> (it's probably better if sdwilsh reviews this given his familiarity with
> IndexedDB and the architecture decisions involved)
Want to give it a first pass?  I won't be able to look at this until Wednesday next week.
Comment on attachment 471506 [details] [diff] [review]
Patch, v1

Just for build changes.
Attachment #471506 - Flags: review?(ted.mielczarek)
Comment on attachment 471506 [details] [diff] [review]
Patch, v1

Kind of gross, but hey, it's not my module.
Attachment #471506 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 471506 [details] [diff] [review]
Patch, v1

Tres polished for a v1 patch!

Update the doxygen block for aVFSName on Connection::initialize and you're golden.

Although there are no explicit storage tests in this patch, bent points out that all IndexedDB tests exercise the functionality which I agree obviates the need for explicit storage tests.  (And it's on IndexedDB to verify the specific quota logic anyhow.)
Attachment #471506 - Flags: review?(sdwilsh) → review+
http://hg.mozilla.org/mozilla-central/rev/06f2a47a936f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
(In reply to comment #5)
> Although there are no explicit storage tests in this patch, bent points out
> that all IndexedDB tests exercise the functionality which I agree obviates the
> need for explicit storage tests.  (And it's on IndexedDB to verify the specific
> quota logic anyhow.)
My concern with this is that until now we didn't need to run tests on the whole product to see if a change in storage breaks things (with a few exceptions due to poor test coverage).  Additionally, while it's unlikely that this would happen, but if IndexedDb were ever removed from the product, we'd lose our test coverage.
(In reply to comment #6)
> Created attachment 471869 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=471869
> Comment fix
This also would require sr since it's an API change.  Run this by shaver please still?
(In reply to comment #10)
> This also would require sr since it's an API change.  Run this by shaver please
> still?

Er... Doesn't that just mean "public" APIs? Every code change is an API change at some level. This is just an internal interface that only has a few callers...
(In reply to comment #11)
> Er... Doesn't that just mean "public" APIs? Every code change is an API change
> at some level. This is just an internal interface that only has a few
> callers...
mozIStorageServiceQuotaManagement is an interface defined with idl.  That is pretty much public, and I thought we planned on bringing it into mozIStorageService after we branched?
Oh, I thought you were referring to the mozStorageConnection.h change. We added a separate interface specifically to avoid public API changes, so I'm surprised you'd want sr for that. Merging the two would for sure be a change to mozIStorageService, but that's coming later.
Comment on attachment 471506 [details] [diff] [review]
Patch, v1

Vlad, we added a new interface to open databases with a different SQLite VFS. We didn't want to change mozIStorageService this far into the beta cycle. We can merge it later on once we've branched. Can you sr?
Attachment #471506 - Flags: superreview?(vladimir)
Attachment #471506 - Flags: superreview?(vladimir) → superreview+
(In reply to comment #13)
> Oh, I thought you were referring to the mozStorageConnection.h change. We added
> a separate interface specifically to avoid public API changes, so I'm surprised
> you'd want sr for that. Merging the two would for sure be a change to
> mozIStorageService, but that's coming later.
Still useful to get sr feedback on the new API since I don't think we'd want to change it when we moved it over to mozIStorageService.

Can you please file the follow-up to merge these two (and bonus points if you put that patch up).
You need to log in before you can comment on or make changes to this bug.