Last Comment Bug 711727 - Followup cleanup for files in IndexedDB
: Followup cleanup for files in IndexedDB
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Jan Varga [:janv]
:
Mentors:
Depends on:
Blocks: buildwarning
  Show dependency treegraph
 
Reported: 2011-12-17 08:31 PST by Jan Varga [:janv]
Modified: 2012-03-22 11:54 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (24.60 KB, patch)
2011-12-17 11:47 PST, Jan Varga [:janv]
jonas: review+
bent.mozilla: review+
Details | Diff | Review
fix warnings on windows (1.31 KB, patch)
2011-12-19 11:48 PST, Jan Varga [:janv]
jonas: review+
Details | Diff | Review

Description Jan Varga [:janv] 2011-12-17 08:31:14 PST

    
Comment 1 Jan Varga [:janv] 2011-12-17 08:51:03 PST
> /Users/varga/Sources/Mozilla5/mozilla-central/dom/indexedDB/
> IndexedDatabaseManager.cpp
> @@ +679,5 @@
> > +  // are database files then we need to create file managers for them and also
> > +  // tell SQLite about all of them.
> > +
> > +  nsAutoTArray<nsString, 20> subdirectories;
> > +  nsAutoTArray<nsCOMPtr<nsIFile> , 20> unknownFiles;
> 
> I'd like to change these to be hashsets, but we can do that in a followup.
> 

fixed

> @@ +755,3 @@
> >      NS_ENSURE_SUCCESS(rv, rv);
> >  
> > +    rv = fileManager->InitDirectory(fileManagerDirectory, connection);
> 
> Hm, can you combine FileManager::Init and FileManager::InitDirectory and
> just call it Init?
> 
> @@ +930,5 @@
> >    
> >    if (!fileManager) {
> >      fileManager = new FileManager(aOrigin, aDatabaseName);
> >  
> > +    if (NS_FAILED(fileManager->Init())) {
> 
> This is the only call to Init() without a followup for InitDirectory(). I
> think we should change this to GetFileManager(), not GetOrCreate(), and just
> return null here. Then DOMWindowUtils can deal accordingly. Can do in a
> followup since this is only going to happen in mochitests.
> 

fixed, however GetOrCreateFileManager() is still needed in OpenDatabaseHelper::CreateDatabaseConnection()
Comment 2 Jan Varga [:janv] 2011-12-17 11:47:55 PST
Created attachment 582568 [details] [diff] [review]
patch
Comment 3 Jan Varga [:janv] 2011-12-19 11:48:59 PST
Created attachment 582903 [details] [diff] [review]
fix warnings on windows
Comment 4 Jonas Sicking (:sicking) 2011-12-19 16:18:45 PST
Comment on attachment 582568 [details] [diff] [review]
patch

r=me on everything except the IndexedDatabaseManager::EnsureOriginIsInitialized changes which bent better have a look at.
Comment 5 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-12-19 19:25:22 PST
Comment on attachment 582568 [details] [diff] [review]
patch

Review of attachment 582568 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +770,5 @@
>  
>      rv = ss->UpdateQuotaInformationForFile(file);
>      NS_ENSURE_SUCCESS(rv, rv);
> +
> +    validSubdirs.PutEntry(dbBaseFilename);

This can fail, need to check.
Comment 6 Jan Varga [:janv] 2011-12-20 03:32:32 PST
https://hg.mozilla.org/mozilla-central/rev/f3c943d2e763

Note You need to log in before you can comment on or make changes to this bug.