Bug 44389 - Switch the Blob implementation to using the blob data registration model
Summary: Switch the Blob implementation to using the blob data registration model
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jian Li
URL:
Keywords:
Depends on:
Blocks: 51366
  Show dependency treegraph
 
Reported: 2010-08-21 18:55 PDT by Jian Li
Modified: 2010-12-20 17:20 PST (History)
7 users (show)

See Also:


Attachments
Proposed Patch (117.15 KB, patch)
2010-08-21 19:29 PDT, Jian Li
fishd: review+
jianli: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jian Li 2010-08-21 18:55:38 PDT
We need to update the Blob implementation in order to switch to the blob data registration model.
Comment 1 Jian Li 2010-08-21 19:29:34 PDT
Created attachment 65043 [details]
Proposed Patch
Comment 2 Darin Fisher (:fishd, Google) 2010-08-21 21:17:22 PDT
Comment on attachment 65043 [details]
Proposed Patch

WebCore/html/Blob.cpp:93
 +          static_cast<const File*>(this)->captureSnapshot(size, modificationTime);
nit: add a FIXME about eliminating this synchronous file operation perhaps?

WebCore/html/Blob.h:66
 +      virtual unsigned long long size() const { return static_cast<long long>(m_size); }
nit: looks like this static_cast shouldn't be necessary since m_size is type "long long"

WebCore/html/BlobBuilder.cpp:88
 +          file->captureSnapshot(snapshotSize, snapshotModificationTime);
same comment about snapshotting asynchronously one day.

WebCore/html/File.cpp:74
 +      // come up with an exception to throw if file size is not represetable.
represetable -> representable

WebCore/html/File.cpp:86
 +      if (!getFileSize(m_path, snapshotSize) || !getFileModificationTime(m_path, modificationTime)) {
it would be nice to combine these into a single file system call.  that way in
chromium, it will be only a single synchronous IPC.  this can be deferred to
another patch of course.

WebCore/html/File.h:69
 +      void captureSnapshot(long long& snapshotSize, double& snapshotModificationTime) const;
please put a warning comment about the fact that this does a synchronous file operation.
we want people to think twice before calling this function.

WebCore/html/FormDataList.h:56
 +      class Item {
nit: nicer to put inner class definitions at the top of the public section?

WebCore/platform/network/FormData.cpp:143
 +               break;
indentation

WebCore/platform/network/FormData.cpp:222
 +                      name = name.replace("-", ""); // For safty, remove '-' from the filename snce some servers may not like it.
snce -> since

those are just nits, so R=me
Comment 3 David Levin 2010-08-21 23:39:35 PDT
(In reply to comment #2)
> (From update of attachment 65043 [details])
> WebCore/platform/network/FormData.cpp:222
>  +                      name = name.replace("-", ""); // For safty, remove '-' from the filename snce some servers may not like it.
> snce -> since
safty -> safety
as well.
Comment 4 WebKit Review Bot 2010-08-30 21:53:50 PDT
http://trac.webkit.org/changeset/66452 might have broken Qt Linux Release minimal, Qt Linux ARMv5 Release, and Qt Linux ARMv7 Release
Comment 5 Jian Li 2010-08-30 23:30:52 PDT
Committed as http://trac.webkit.org/changeset/66452.