Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
238043007:
Complete registration of blobs before sending back an indexed DB value that contains them. (Closed)
|
Created:
6 years, 8 months ago by ericu Modified:
6 years, 8 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. |
DescriptionComplete registration of blobs before sending back an indexed DB value that contains them.
BUG=108012
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264728
Patch Set 1 #Patch Set 2 : Small tweak #
Total comments: 19
Patch Set 3 : Rolled in code review feedback. #Patch Set 4 : Ran clang-format #
Messages
Total messages: 13 (0 generated)
https://codereview.chromium.org/238043007/diff/20001/content/browser/indexed_... File content/browser/indexed_db/indexed_db_callbacks.cc (right): https://codereview.chromium.org/238043007/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_callbacks.cc:190: ShareableFileReference::Get(blob_info.file_path()); Why do your own "Get" before calling "GetOrCreate"? https://codereview.chromium.org/238043007/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_callbacks.cc:219: (*blob_or_file_info)[i].uuid = I see below the contract is that blob_or_file_info->size() == blob_info.size() before calling this function? I'd put a DCHECK_EQ here to make it obvious that's a precondition. https://codereview.chromium.org/238043007/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_callbacks.cc:229: IndexedDBMsg_CallbacksSuccessIDBCursor_Params* params, Can we make this param reference type to simplify? https://codereview.chromium.org/238043007/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_callbacks.cc:277: DCHECK(values.size() == blob_or_file_infos->size()); I'd dup/move this DCHECK to CreateAllBlobs. Also consider DCHECK_EQ. https://codereview.chromium.org/238043007/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_callbacks.cc:298: IndexedDBMsg_BlobOrFileInfo info; Where is uuid initialized? https://codereview.chromium.org/238043007/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_callbacks.cc:464: BrowserThread::PostTask( Worthy of a DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::IO)); here too?
https://codereview.chromium.org/238043007/diff/20001/content/browser/indexed_... File content/browser/indexed_db/indexed_db_callbacks.cc (right): https://codereview.chromium.org/238043007/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_callbacks.cc:228: static void BlobLookupForIDBCursor( Can this be made a template function on <ParamsType, MsgType> ? It looks like that would handle the IDBCursor, CursorContinue, ValueWithKey and Value cases. https://codereview.chromium.org/238043007/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_callbacks.cc:351: if (!value || value->blob_info.empty()) { Can callback messages ever end up re-ordered now? e.g. if a success(value-with-blob) is invoked, followed by a success(value-without-blob), could the latter ever end up sent over IPC first due to the extra hop to the IO thread? (IndexedDB requires that the order be maintained) https://codereview.chromium.org/238043007/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_callbacks.cc:446: std::vector<IndexedDBValue>::iterator iter = values.begin(); nit: can you move this to just before the for() loop that uses it?
https://codereview.chromium.org/238043007/diff/20001/content/browser/indexed_... File content/browser/indexed_db/indexed_db_callbacks.cc (right): https://codereview.chromium.org/238043007/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_callbacks.cc:190: ShareableFileReference::Get(blob_info.file_path()); On 2014/04/17 18:07:57, cmumford wrote: > Why do your own "Get" before calling "GetOrCreate"? We only want the callback to get called once. If there's already one attached, that's enough. If we call AddFinalReleaseCallback again, it'll get added to a list of callbacks, rather than replacing the one that's there, and they'll all get called. https://codereview.chromium.org/238043007/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_callbacks.cc:219: (*blob_or_file_info)[i].uuid = On 2014/04/17 18:07:57, cmumford wrote: > I see below the contract is that blob_or_file_info->size() == blob_info.size() > before calling this function? I'd put a DCHECK_EQ here to make it obvious that's > a precondition. Done. https://codereview.chromium.org/238043007/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_callbacks.cc:228: static void BlobLookupForIDBCursor( On 2014/04/17 18:22:22, jsbell wrote: > Can this be made a template function on <ParamsType, MsgType> ? It looks like > that would handle the IDBCursor, CursorContinue, ValueWithKey and Value cases. Done. https://codereview.chromium.org/238043007/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_callbacks.cc:229: IndexedDBMsg_CallbacksSuccessIDBCursor_Params* params, On 2014/04/17 18:07:57, cmumford wrote: > Can we make this param reference type to simplify? I think that leads to an extra copy due to the bind. https://codereview.chromium.org/238043007/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_callbacks.cc:277: DCHECK(values.size() == blob_or_file_infos->size()); On 2014/04/17 18:07:57, cmumford wrote: > I'd dup/move this DCHECK to CreateAllBlobs. Also consider DCHECK_EQ. CreateAllBlobs gets a single value at a time, not all of values. Changed to DCHECK_EQ. https://codereview.chromium.org/238043007/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_callbacks.cc:298: IndexedDBMsg_BlobOrFileInfo info; On 2014/04/17 18:07:57, cmumford wrote: > Where is uuid initialized? In CreateAllBlobs, called from the closure that gets created right after the call to FillInBlobData. https://codereview.chromium.org/238043007/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_callbacks.cc:351: if (!value || value->blob_info.empty()) { On 2014/04/17 18:22:22, jsbell wrote: > Can callback messages ever end up re-ordered now? > > e.g. if a success(value-with-blob) is invoked, followed by a > success(value-without-blob), could the latter ever end up sent over IPC first > due to the extra hop to the IO thread? > > (IndexedDB requires that the order be maintained) There's no extra hop. The new code jumps over earlier, but they both end up sending from the IO thread. When you call Send, it checks to see which thread you're on; if you're not on the IO thread, it posts a task across to continue. Ordering should still be guaranteed. https://codereview.chromium.org/238043007/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_callbacks.cc:446: std::vector<IndexedDBValue>::iterator iter = values.begin(); On 2014/04/17 18:22:22, jsbell wrote: > nit: can you move this to just before the for() loop that uses it? Done. https://codereview.chromium.org/238043007/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_callbacks.cc:464: BrowserThread::PostTask( On 2014/04/17 18:07:57, cmumford wrote: > Worthy of a DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::IO)); here too? All of the OnSuccess methods are called from the IDB task runner; it seems unnecessary to me. I can add it to the beginning of each of them if you feel strongly.
lgtm https://codereview.chromium.org/238043007/diff/20001/content/browser/indexed_... File content/browser/indexed_db/indexed_db_callbacks.cc (right): https://codereview.chromium.org/238043007/diff/20001/content/browser/indexed_... content/browser/indexed_db/indexed_db_callbacks.cc:351: if (!value || value->blob_info.empty()) { On 2014/04/17 21:36:29, ericu wrote: > On 2014/04/17 18:22:22, jsbell wrote: > > Can callback messages ever end up re-ordered now? > > > > e.g. if a success(value-with-blob) is invoked, followed by a > > success(value-without-blob), could the latter ever end up sent over IPC first > > due to the extra hop to the IO thread? > > > > (IndexedDB requires that the order be maintained) > > There's no extra hop. The new code jumps over earlier, but they both end up > sending from the IO thread. When you call Send, it checks to see which thread > you're on; if you're not on the IO thread, it posts a task across to continue. > Ordering should still be guaranteed. Groovy. I'd thought it was sent on the IO thread then went digging, saw thread handling code over in ipc/, got distracted by shiny things, and thought I'd just assume the worst. :) That said, seems like a good candidate for a layout test to add eventually. The master patches are getting svelte!
lgtm
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/238043007/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_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/238043007/60001
Message was sent while issue was closed.
Change committed as 264728 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
