|
|
Created:
6 years, 9 months ago by ericu Modified:
6 years, 8 months ago CC:
blink-reviews, jamesr, dglazkov+blink, abarth-chromium Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionAdd two classes [as yet unused] needed for upcoming IDB Blob support.
BUG=108012
R=cmumford,jsbell,abarth
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170686
Patch Set 1 #
Total comments: 20
Patch Set 2 : Rolled in code review feedback. #
Total comments: 1
Patch Set 3 : Remove BlobInfo; we can just use WebBlobInfo. #Patch Set 4 : tiny style fix #Patch Set 5 : Clean up branch #Patch Set 6 : Clean up branch again. #Patch Set 7 : Trying again #Patch Set 8 : More fixing #Patch Set 9 : Fixed the branch #Messages
Total messages: 16 (0 generated)
Here's the first piece of the blink side.
Just a quick glance, leaving the details to cmumford https://codereview.chromium.org/205413004/diff/1/Source/platform/blink_platfo... File Source/platform/blink_platform.gypi (right): https://codereview.chromium.org/205413004/diff/1/Source/platform/blink_platfo... Source/platform/blink_platform.gypi:243: 'blob/BlobInfo.h', So... with BlobDataHandle, BlobData, BlobDataItem and now BlobInfo it looks like there may be some redundancy? I haven't delved too deeply, but possibly there could be some cleanup there once everything is in and tested. https://codereview.chromium.org/205413004/diff/1/Source/platform/blob/BlobInfo.h File Source/platform/blob/BlobInfo.h (right): https://codereview.chromium.org/205413004/diff/1/Source/platform/blob/BlobInf... Source/platform/blob/BlobInfo.h:2: * Copyright (C) 2014 Google Inc. All rights reserved. Use short copyright declaration https://codereview.chromium.org/205413004/diff/1/Source/platform/blob/BlobInf... Source/platform/blob/BlobInfo.h:67: , m_size(-1) Is there any consistency checking we should do when size == -1? https://codereview.chromium.org/205413004/diff/1/Source/platform/blob/BlobInf... Source/platform/blob/BlobInfo.h:85: String m_type; // mime type Nit: Capitalize https://codereview.chromium.org/205413004/diff/1/Source/platform/blob/BlobInf... Source/platform/blob/BlobInfo.h:87: String m_filePath; // only for File Nit: ditto https://codereview.chromium.org/205413004/diff/1/Source/platform/exported/Web... File Source/platform/exported/WebBlobInfo.cpp (right): https://codereview.chromium.org/205413004/diff/1/Source/platform/exported/Web... Source/platform/exported/WebBlobInfo.cpp:2: * Copyright (C) 2014 Google Inc. All rights reserved. Use short copyright declaration https://codereview.chromium.org/205413004/diff/1/public/platform/WebBlobInfo.h File public/platform/WebBlobInfo.h (right): https://codereview.chromium.org/205413004/diff/1/public/platform/WebBlobInfo.... public/platform/WebBlobInfo.h:2: * Copyright (C) 2014 Google Inc. All rights reserved. Use short copyright declaration
https://codereview.chromium.org/205413004/diff/1/Source/platform/blob/BlobInfo.h File Source/platform/blob/BlobInfo.h (right): https://codereview.chromium.org/205413004/diff/1/Source/platform/blob/BlobInf... Source/platform/blob/BlobInfo.h:42: , m_size(-1) Should initialize m_lastModified. https://codereview.chromium.org/205413004/diff/1/Source/platform/blob/BlobInf... Source/platform/blob/BlobInfo.h:85: String m_type; // mime type On 2014/03/20 16:00:46, jsbell wrote: > Nit: Capitalize Why not call this m_MimeType instead of m_Type?
https://codereview.chromium.org/205413004/diff/1/Source/platform/blob/BlobInfo.h File Source/platform/blob/BlobInfo.h (right): https://codereview.chromium.org/205413004/diff/1/Source/platform/blob/BlobInf... Source/platform/blob/BlobInfo.h:85: String m_type; // mime type On 2014/03/20 18:22:32, cmumford wrote: > On 2014/03/20 16:00:46, jsbell wrote: > > Nit: Capitalize > > Why not call this m_MimeType instead of m_Type? Sorry, I meant: capitalize the comment (here and below) No strong opinion on m_mimeType vs. just m_type; the former clarifies what it is vs. e.g. m_isFile, but MIME has become meaningless. m_contentType would be the pedantic form... and coincidentally that's what's used in BlobData.h FWIW.
https://codereview.chromium.org/205413004/diff/1/Source/platform/blob/BlobInfo.h File Source/platform/blob/BlobInfo.h (right): https://codereview.chromium.org/205413004/diff/1/Source/platform/blob/BlobInf... Source/platform/blob/BlobInfo.h:85: String m_type; // mime type On 2014/03/20 18:30:29, jsbell wrote: > On 2014/03/20 18:22:32, cmumford wrote: > > On 2014/03/20 16:00:46, jsbell wrote: > > > Nit: Capitalize > > > > Why not call this m_MimeType instead of m_Type? > > Sorry, I meant: capitalize the comment (here and below) > > No strong opinion on m_mimeType vs. just m_type; the former clarifies what it is > vs. e.g. m_isFile, but MIME has become meaningless. m_contentType would be the > pedantic form... and coincidentally that's what's used in BlobData.h FWIW. Well being more naive than you I'd prefer the more pedantic name, but I guess that whatever is more consistent with current variable names is best.
https://codereview.chromium.org/205413004/diff/1/Source/platform/blink_platfo... File Source/platform/blink_platform.gypi (right): https://codereview.chromium.org/205413004/diff/1/Source/platform/blink_platfo... Source/platform/blink_platform.gypi:243: 'blob/BlobInfo.h', On 2014/03/20 16:00:46, jsbell wrote: > So... with BlobDataHandle, BlobData, BlobDataItem and now BlobInfo it looks like > there may be some redundancy? > > I haven't delved too deeply, but possibly there could be some cleanup there once > everything is in and tested. BlobDataHandle manages lifetime. BlobDataItem's the smallest fragment of a blob. BlobData has the closest fit, but it's serving a different purpose. It's not clear to me that expanding it to handle the BlobInfo use cases as well, but it might work; we can look at it later. https://codereview.chromium.org/205413004/diff/1/Source/platform/blob/BlobInfo.h File Source/platform/blob/BlobInfo.h (right): https://codereview.chromium.org/205413004/diff/1/Source/platform/blob/BlobInf... Source/platform/blob/BlobInfo.h:2: * Copyright (C) 2014 Google Inc. All rights reserved. On 2014/03/20 16:00:46, jsbell wrote: > Use short copyright declaration Done. https://codereview.chromium.org/205413004/diff/1/Source/platform/blob/BlobInf... Source/platform/blob/BlobInfo.h:42: , m_size(-1) On 2014/03/20 18:22:32, cmumford wrote: > Should initialize m_lastModified. Done. https://codereview.chromium.org/205413004/diff/1/Source/platform/blob/BlobInf... Source/platform/blob/BlobInfo.h:67: , m_size(-1) On 2014/03/20 16:00:46, jsbell wrote: > Is there any consistency checking we should do when size == -1? It's used to indicate that it's a File, but that we don't have a snapshot for it yet. I added an assert that it's not -1 in the Blob-type constructor. We could add more asserts in the accessors, but that seems a bit heavyweight. What do you think? https://codereview.chromium.org/205413004/diff/1/Source/platform/blob/BlobInf... Source/platform/blob/BlobInfo.h:85: String m_type; // mime type On 2014/03/20 16:00:46, jsbell wrote: > Nit: Capitalize Done. I left it as type for consistency with Blob. At some point we should probably switch everybody over to contentType, but for now I think it's clearer to match Blob, since it's used together in the code more than BlobData. https://codereview.chromium.org/205413004/diff/1/Source/platform/blob/BlobInf... Source/platform/blob/BlobInfo.h:87: String m_filePath; // only for File On 2014/03/20 16:00:46, jsbell wrote: > Nit: ditto Done. https://codereview.chromium.org/205413004/diff/1/Source/platform/exported/Web... File Source/platform/exported/WebBlobInfo.cpp (right): https://codereview.chromium.org/205413004/diff/1/Source/platform/exported/Web... Source/platform/exported/WebBlobInfo.cpp:2: * Copyright (C) 2014 Google Inc. All rights reserved. On 2014/03/20 16:00:46, jsbell wrote: > Use short copyright declaration Done. https://codereview.chromium.org/205413004/diff/1/Source/platform/exported/Web... Source/platform/exported/WebBlobInfo.cpp:2: * Copyright (C) 2014 Google Inc. All rights reserved. On 2014/03/20 16:00:46, jsbell wrote: > Use short copyright declaration Done. https://codereview.chromium.org/205413004/diff/1/public/platform/WebBlobInfo.h File public/platform/WebBlobInfo.h (right): https://codereview.chromium.org/205413004/diff/1/public/platform/WebBlobInfo.... public/platform/WebBlobInfo.h:2: * Copyright (C) 2014 Google Inc. All rights reserved. On 2014/03/20 16:00:46, jsbell wrote: > Use short copyright declaration Done.
lgtm
The CQ bit was checked by ericu@chromium.org
The CQ bit was unchecked by ericu@chromium.org
+abarth for owner approval.
LGTM https://codereview.chromium.org/205413004/diff/20001/Source/platform/blob/Blo... File Source/platform/blob/BlobInfo.h (right): https://codereview.chromium.org/205413004/diff/20001/Source/platform/blob/Blo... Source/platform/blob/BlobInfo.h:12: class BlobInfo { You might not need this class. Code that would use this class might be able to just use WebBlobInfo directly.
On 2014/04/01 21:36:27, ericu wrote: > The CQ bit was unchecked by mailto:ericu@chromium.org LGTM
Great--thanks Adam. That simplifies things a bunch.
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/205413004/160001
Message was sent while issue was closed.
Change committed as 170686 |