Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
240003011:
Add blob-writing functionality [as yet un-called] to IDB's backend. (Closed)
|
Created:
6 years, 8 months ago by ericu Modified:
6 years, 7 months ago CC:
chromium-reviews, jam, alecflett, ericu+idb_chromium.org, darin-cc_chromium.org, cmumford, dgrogan, jsbell+idb_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd blob-writing functionality [as yet un-called] to IDB's backend.
BUG=108012
R=cmumford@chromium.org, jsbell@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266916
Patch Set 1 #Patch Set 2 : Remove todo #
Total comments: 10
Patch Set 3 : Switched callback to a single function. #Patch Set 4 : Added a TODO #
Total comments: 2
Patch Set 5 : Factor out GetBlobFilenameForKey #
Total comments: 1
Patch Set 6 : Build fixes. #Patch Set 7 : Build fix #Patch Set 8 : Fix it harder #Patch Set 9 : Merged out #Patch Set 10 : Cleaned branch #
Messages
Total messages: 33 (0 generated)
https://codereview.chromium.org/240003011/diff/20001/content/browser/indexed_... File content/browser/indexed_db/indexed_db_backing_store.cc (right): https://codereview.chromium.org/240003011/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_backing_store.cc:1750: WriteNextFile(); By writing the first file in the constructor do you have a problem where the delegate hasn't yet been set before use? https://codereview.chromium.org/240003011/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_backing_store.cc:1800: scoped_refptr<ChainedBlobWriterImpl> self_ref_; having a self reference seems like a scary way to keep an object alive. Will it really not outlive the backing store? What about keeping the references in the backing store itself? https://codereview.chromium.org/240003011/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_backing_store.cc:1877: return false; If we can't create the dir (or copy the file below) why aren't we calling ReportWriteCompletion(..., false, ...)? https://codereview.chromium.org/240003011/diff/20001/content/browser/indexed_... File content/browser/indexed_db/indexed_db_backing_store.h (right): https://codereview.chromium.org/240003011/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_backing_store.h:410: virtual void ReportWriteCompletion(bool succeeded, Nit: This class has a completion callback with a succeeded param, but BlobWriteCallback has two functions for success/failure. Would it be clearer if they were consistent? I'd like to go one further with a failure reason. It'd be great to give API's enough information tell user cause of failure (disk full, network disconnect, etc.) https://codereview.chromium.org/240003011/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_backing_store.h:419: class ChainedBlobWriterImpl; ChainedBlobWriterImpl doesn't seem to be used in this file.
https://codereview.chromium.org/240003011/diff/20001/content/browser/indexed_... File content/browser/indexed_db/indexed_db_backing_store.cc (right): https://codereview.chromium.org/240003011/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_backing_store.cc:1750: WriteNextFile(); On 2014/04/18 17:19:06, cmumford wrote: > By writing the first file in the constructor do you have a problem where the > delegate hasn't yet been set before use? No, the delegate gets set in writeBlobToFileOnIOThread as a result of that WriteNextFile call. We create it just as we start to use it. https://codereview.chromium.org/240003011/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_backing_store.cc:1800: scoped_refptr<ChainedBlobWriterImpl> self_ref_; On 2014/04/18 17:19:06, cmumford wrote: > having a self reference seems like a scary way to keep an object alive. Will it > really not outlive the backing store? What about keeping the references in the > backing store itself? It's safe for us to outlive the backing store. The self_ref_ is only set when someone tries to abort a running write. As soon as the write completes, the ref is cleared without us taking any other action [as the abort has been reported as complete elsewhere already]. The only purpose of self_ref_ is to let us swallow that last write completion, even though the transaction that owns us has gone away. What I really want is a weak pointer, but unfortunately they're not thread safe. That's not a problem in callBlobCallbackOnIDBTaskRunner [it's on the right task_runner], but would be for the postTask in WriteBlobFile. I could put another wrapper there, but I think that would be making things even worse. I could have the backing store hold a ref in addition to the transaction, perhaps only when needed, but that makes this less self-contained. If we ever wanted to be able to run multiple write transactions at the same time, we'd have to undo that change. Granted, I think we're far from there, but I still like the encapsulation I have here. If you feel strongly that we shouldn't do a self-ref here, let's talk about it. https://codereview.chromium.org/240003011/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_backing_store.cc:1877: return false; On 2014/04/18 17:19:06, cmumford wrote: > If we can't create the dir (or copy the file below) why aren't we calling > ReportWriteCompletion(..., false, ...)? We return false, and WriteNextFile [our only caller] calls the failure callback for us. ReportWriteCompletion is for writes that got further [completed File copies or Blobs where we created a delegate_ and tried to Start it]. I was going to say it was just for cases where we had a delegate_, but then noticed the File case didn't. I've added an if to short-circuit the DeleteSoon in that case. I think it's correct either way, but cleaner and less computation this way. https://codereview.chromium.org/240003011/diff/20001/content/browser/indexed_... File content/browser/indexed_db/indexed_db_backing_store.h (right): https://codereview.chromium.org/240003011/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_backing_store.h:410: virtual void ReportWriteCompletion(bool succeeded, On 2014/04/18 17:19:06, cmumford wrote: > Nit: This class has a completion callback with a succeeded param, but > BlobWriteCallback has two functions for success/failure. Would it be clearer if > they were consistent? Done; I'm not sure why I had that difference...it may date back to the in-process webkit. > I'd like to go one further with a failure reason. It'd be great to give API's > enough information tell user cause of failure (disk full, network disconnect, > etc.) Added a TODO. https://codereview.chromium.org/240003011/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_backing_store.h:419: class ChainedBlobWriterImpl; On 2014/04/18 17:19:06, cmumford wrote: > ChainedBlobWriterImpl doesn't seem to be used in this file. It has to be declared as a member of IndexedDBBackingStore::Transaction in order to call WriteBlobFile.
lgtm https://codereview.chromium.org/240003011/diff/60001/content/browser/indexed_... File content/browser/indexed_db/indexed_db_backing_store.cc (right): https://codereview.chromium.org/240003011/diff/60001/content/browser/indexed_... content/browser/indexed_db/indexed_db_backing_store.cc:1919: // This assumes a file path of dbId/second-to-LSB-of-counter/counter. Just to keep assumptions together at the top of the file, maybe factor out a helper GetBlobFilenameForKey(blob_path_, database_id, key) ?
lgtm
https://codereview.chromium.org/240003011/diff/60001/content/browser/indexed_... File content/browser/indexed_db/indexed_db_backing_store.cc (right): https://codereview.chromium.org/240003011/diff/60001/content/browser/indexed_... content/browser/indexed_db/indexed_db_backing_store.cc:1919: // This assumes a file path of dbId/second-to-LSB-of-counter/counter. On 2014/04/24 00:16:13, jsbell wrote: > Just to keep assumptions together at the top of the file, maybe factor out a > helper GetBlobFilenameForKey(blob_path_, database_id, key) ? > Done.
The CQ bit was checked by ericu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ericu@chromium.org/240003011/80001
https://codereview.chromium.org/240003011/diff/80001/content/browser/indexed_... File content/browser/indexed_db/indexed_db_backing_store.cc (right): https://codereview.chromium.org/240003011/diff/80001/content/browser/indexed_... content/browser/indexed_db/indexed_db_backing_store.cc:1927: // This assumes a file path of dbId/second-to-LSB-of-counter/counter. Nit: This comment could be moved/removed now.
Good point; I'll take that out in a later CL. On Mon, Apr 28, 2014 at 11:04 AM, <jsbell@chromium.org> wrote: > > https://codereview.chromium.org/240003011/diff/80001/content/browser/indexed_... > File content/browser/indexed_db/indexed_db_backing_store.cc (right): > > https://codereview.chromium.org/240003011/diff/80001/content/browser/indexed_... > content/browser/indexed_db/indexed_db_backing_store.cc:1927: // This > > assumes a file path of dbId/second-to-LSB-of-counter/counter. > Nit: This comment could be moved/removed now. > > https://codereview.chromium.org/240003011/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on android_clang_dbg tryserver.chromium on linux_chromium_chromeos_clang_dbg
The CQ bit was checked by ericu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ericu@chromium.org/240003011/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by ericu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ericu@chromium.org/240003011/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by ericu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ericu@chromium.org/240003011/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_rel
The CQ bit was checked by ericu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ericu@chromium.org/240003011/100002
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by ericu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ericu@chromium.org/240003011/100002
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
Message was sent while issue was closed.
Committed patchset #10 manually as r266916 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
