Make Blob::m_size an Optional
authoryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 12 Aug 2019 19:13:17 +0000 (19:13 +0000)
committeryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 12 Aug 2019 19:13:17 +0000 (19:13 +0000)
https://bugs.webkit.org/show_bug.cgi?id=200617

Reviewed by Alex Christensen.

Use an Optional instead of -1 to know that m_size is initialized or not.
No change of behavior.

Refactoring to make all Blob members private.
Remove one static Blob create method.

Covered by existing tests.

* Modules/fetch/FetchBody.cpp:
(WebCore::FetchBody::fromFormData):
* fileapi/Blob.cpp:
(WebCore::Blob::Blob):
(WebCore::Blob::size const):
* fileapi/Blob.h:
(WebCore::Blob::setInternalURL):
* fileapi/File.cpp:
(WebCore::File::create):
(WebCore::File::File):
(WebCore::File::computeNameAndContentType):
* fileapi/File.h:
* html/FileListCreator.cpp:
(WebCore::FileListCreator::createFileList):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@248538 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/Modules/fetch/FetchBody.cpp
Source/WebCore/fileapi/Blob.cpp
Source/WebCore/fileapi/Blob.h
Source/WebCore/fileapi/File.cpp
Source/WebCore/fileapi/File.h
Source/WebCore/html/FileListCreator.cpp

index e749160..88fcade 100644 (file)
@@ -1,3 +1,33 @@
+2019-08-12  Youenn Fablet  <youenn@apple.com>
+
+        Make Blob::m_size an Optional
+        https://bugs.webkit.org/show_bug.cgi?id=200617
+
+        Reviewed by Alex Christensen.
+
+        Use an Optional instead of -1 to know that m_size is initialized or not.
+        No change of behavior.
+
+        Refactoring to make all Blob members private.
+        Remove one static Blob create method.
+
+        Covered by existing tests.
+
+        * Modules/fetch/FetchBody.cpp:
+        (WebCore::FetchBody::fromFormData):
+        * fileapi/Blob.cpp:
+        (WebCore::Blob::Blob):
+        (WebCore::Blob::size const):
+        * fileapi/Blob.h:
+        (WebCore::Blob::setInternalURL):
+        * fileapi/File.cpp:
+        (WebCore::File::create):
+        (WebCore::File::File):
+        (WebCore::File::computeNameAndContentType):
+        * fileapi/File.h:
+        * html/FileListCreator.cpp:
+        (WebCore::FileListCreator::createFileList):
+
 2019-08-12  Chris Dumez  <cdumez@apple.com>
 
         GPUBuffer seems to be ref'd / deref'd from multiple thread concurrently but is not ThreadSafeRefCounted
index 8eca13b..ad26b34 100644 (file)
@@ -88,7 +88,7 @@ Optional<FetchBody> FetchBody::fromFormData(PAL::SessionID sessionID, FormData&
     auto url = formData.asBlobURL();
     if (!url.isNull()) {
         // FIXME: Properly set mime type and size of the blob.
-        Ref<const Blob> blob = Blob::deserialize(sessionID, url, { }, 0, { });
+        Ref<const Blob> blob = Blob::deserialize(sessionID, url, { }, { }, { });
         return FetchBody { WTFMove(blob) };
     }
 
index 464c137..2a06d03 100644 (file)
@@ -72,8 +72,10 @@ URLRegistry& BlobURLRegistry::registry()
     return instance;
 }
 
-Blob::Blob(UninitializedContructor, PAL::SessionID sessionID)
+Blob::Blob(UninitializedContructor, PAL::SessionID sessionID, URL&& url, String&& type)
     : m_sessionID(sessionID)
+    , m_internalURL(WTFMove(url))
+    , m_type(WTFMove(type))
 {
 }
 
@@ -89,7 +91,6 @@ Blob::Blob(PAL::SessionID sessionID, Vector<BlobPartVariant>&& blobPartVariants,
     : m_sessionID(sessionID)
     , m_internalURL(BlobURL::createInternalURL())
     , m_type(normalizedContentType(propertyBag.type))
-    , m_size(-1)
 {
     BlobBuilder builder(propertyBag.endings);
     for (auto& blobPartVariant : blobPartVariants) {
@@ -137,7 +138,7 @@ Blob::Blob(ReferencingExistingBlobConstructor, const Blob& blob)
     ThreadableBlobRegistry::registerBlobURL(m_internalURL, { BlobPart(blob.url()) } , m_type);
 }
 
-Blob::Blob(DeserializationContructor, PAL::SessionID sessionID, const URL& srcURL, const String& type, long long size, const String& fileBackedPath)
+Blob::Blob(DeserializationContructor, PAL::SessionID sessionID, const URL& srcURL, const String& type, Optional<unsigned long long> size, const String& fileBackedPath)
     : m_sessionID(sessionID)
     , m_type(normalizedContentType(type))
     , m_size(size)
@@ -152,7 +153,7 @@ Blob::Blob(DeserializationContructor, PAL::SessionID sessionID, const URL& srcUR
 Blob::Blob(PAL::SessionID sessionID, const URL& srcURL, long long start, long long end, const String& type)
     : m_sessionID(sessionID)
     , m_type(normalizedContentType(type))
-    , m_size(-1) // size is not necessarily equal to end - start.
+    // m_size is not necessarily equal to end - start so we do not initialize it here.
 {
     m_internalURL = BlobURL::createInternalURL();
     ThreadableBlobRegistry::registerBlobURLForSlice(m_internalURL, srcURL, start, end);
@@ -165,14 +166,14 @@ Blob::~Blob()
 
 unsigned long long Blob::size() const
 {
-    if (m_size < 0) {
+    if (!m_size) {
         // FIXME: JavaScript cannot represent sizes as large as unsigned long long, we need to
         // come up with an exception to throw if file size is not representable.
         unsigned long long actualSize = ThreadableBlobRegistry::blobSize(m_internalURL);
-        m_size = WTF::isInBounds<long long>(actualSize) ? static_cast<long long>(actualSize) : 0;
+        m_size = WTF::isInBounds<long long>(actualSize) ? actualSize : 0;
     }
 
-    return static_cast<unsigned long long>(m_size);
+    return *m_size;
 }
 
 bool Blob::isValidContentType(const String& contentType)
index e636f2d..2c7a7b0 100644 (file)
@@ -117,22 +117,25 @@ protected:
     Blob(ReferencingExistingBlobConstructor, const Blob&);
 
     enum UninitializedContructor { uninitializedContructor };
-    Blob(UninitializedContructor, PAL::SessionID);
+    Blob(UninitializedContructor, PAL::SessionID, URL&&, String&& type);
 
     enum DeserializationContructor { deserializationContructor };
-    Blob(DeserializationContructor, PAL::SessionID, const URL& srcURL, const String& type, long long size, const String& fileBackedPath);
+    Blob(DeserializationContructor, PAL::SessionID, const URL& srcURL, const String& type, Optional<unsigned long long> size, const String& fileBackedPath);
 
     // For slicing.
     Blob(PAL::SessionID, const URL& srcURL, long long start, long long end, const String& contentType);
 
+private:
     PAL::SessionID m_sessionID;
+
     // This is an internal URL referring to the blob data associated with this object. It serves
     // as an identifier for this blob. The internal URL is never used to source the blob's content
     // into an HTML or for FileRead'ing, public blob URLs must be used for those purposes.
     URL m_internalURL;
 
     String m_type;
-    mutable long long m_size;
+
+    mutable Optional<unsigned long long> m_size;
 };
 
 } // namespace WebCore
index eb5b6a1..c72723b 100644 (file)
@@ -46,28 +46,27 @@ Ref<File> File::createWithRelativePath(PAL::SessionID sessionID, const String& p
     return file;
 }
 
-File::File(PAL::SessionID sessionID, const String& path)
-    : Blob(uninitializedContructor, sessionID)
-    , m_path(path)
+Ref<File> File::create(PAL::SessionID sessionID, const String& path, const String& nameOverride)
 {
-    m_internalURL = BlobURL::createInternalURL();
-    m_size = -1;
-    computeNameAndContentType(m_path, String(), m_name, m_type);
-    ThreadableBlobRegistry::registerFileBlobURL(m_internalURL, path, m_type);
+    String name;
+    String type;
+    computeNameAndContentType(path, nameOverride, name, type);
+
+    auto internalURL = BlobURL::createInternalURL();
+    ThreadableBlobRegistry::registerFileBlobURL(internalURL, path, type);
+
+    return adoptRef(*new File(sessionID, WTFMove(internalURL), WTFMove(type), String { path }, WTFMove(name)));
 }
 
-File::File(PAL::SessionID sessionID, const String& path, const String& nameOverride)
-    : Blob(uninitializedContructor, sessionID)
-    , m_path(path)
+File::File(PAL::SessionID sessionID, URL&& url, String&& type, String&& path, String&& name)
+    : Blob(uninitializedContructor, sessionID, WTFMove(url), WTFMove(type))
+    , m_path(WTFMove(path))
+    , m_name(WTFMove(name))
 {
-    m_internalURL = BlobURL::createInternalURL();
-    m_size = -1;
-    computeNameAndContentType(m_path, nameOverride, m_name, m_type);
-    ThreadableBlobRegistry::registerFileBlobURL(m_internalURL, path, m_type);
 }
 
 File::File(DeserializationContructor, PAL::SessionID sessionID, const String& path, const URL& url, const String& type, const String& name, const Optional<int64_t>& lastModified)
-    : Blob(deserializationContructor, sessionID, url, type, -1, path)
+    : Blob(deserializationContructor, sessionID, url, type, { }, path)
     , m_path(path)
     , m_name(name)
     , m_lastModifiedDateOverride(lastModified)
@@ -132,7 +131,7 @@ void File::computeNameAndContentType(const String& path, const String& nameOverr
         return;
     }
 #endif
-    effectiveName = nameOverride.isNull() ? FileSystem::pathGetFileName(path) : nameOverride;
+    effectiveName = nameOverride.isEmpty() ? FileSystem::pathGetFileName(path) : nameOverride;
     size_t index = effectiveName.reverseFind('.');
     if (index != notFound)
         effectiveContentType = MIMETypeRegistry::getMIMETypeForExtension(effectiveName.substring(index + 1));
index cb8a507..de3a3cb 100644 (file)
@@ -41,10 +41,8 @@ public:
         Optional<int64_t> lastModified;
     };
 
-    static Ref<File> create(PAL::SessionID sessionID, const String& path)
-    {
-        return adoptRef(*new File(sessionID, path));
-    }
+    // Create a file with an optional name exposed to the author (via File.name and associated DOM properties) that differs from the one provided in the path.
+    WEBCORE_EXPORT static Ref<File> create(PAL::SessionID, const String& path, const String& nameOverride = { });
 
     // Create a File using the 'new File' constructor.
     static Ref<File> create(ScriptExecutionContext& context, Vector<BlobPartVariant>&& blobPartVariants, const String& filename, const PropertyBag& propertyBag)
@@ -57,14 +55,6 @@ public:
         return adoptRef(*new File(deserializationContructor, sessionID, path, srcURL, type, name, lastModified));
     }
 
-    // Create a file with a name exposed to the author (via File.name and associated DOM properties) that differs from the one provided in the path.
-    static Ref<File> createWithName(PAL::SessionID sessionID, const String& path, const String& nameOverride)
-    {
-        if (nameOverride.isEmpty())
-            return adoptRef(*new File(sessionID, path));
-        return adoptRef(*new File(sessionID, path, nameOverride));
-    }
-
     static Ref<File> create(const Blob& blob, const String& name)
     {
         return adoptRef(*new File(blob, name));
@@ -96,7 +86,7 @@ public:
 
 private:
     WEBCORE_EXPORT explicit File(PAL::SessionID, const String& path);
-    File(PAL::SessionID, const String& path, const String& nameOverride);
+    File(PAL::SessionID, URL&&, String&& type, String&& path, String&& name);
     File(ScriptExecutionContext&, Vector<BlobPartVariant>&& blobPartVariants, const String& filename, const PropertyBag&);
     File(const Blob&, const String& name);
     File(const File&, const String& name);
index c756056..874684f 100644 (file)
@@ -83,7 +83,7 @@ Ref<FileList> FileListCreator::createFileList(PAL::SessionID sessionID, const Ve
         if (shouldResolveDirectories == ShouldResolveDirectories::Yes && FileSystem::fileIsDirectory(info.path, FileSystem::ShouldFollowSymbolicLinks::No))
             appendDirectoryFiles(sessionID, info.path, FileSystem::pathGetFileName(info.path), fileObjects);
         else
-            fileObjects.append(File::createWithName(sessionID, info.path, info.displayName));
+            fileObjects.append(File::create(sessionID, info.path, info.displayName));
     }
     return FileList::create(WTFMove(fileObjects));
 }