Closed Bug 1339871 Opened 3 years ago Closed 3 years ago

Splitting dom/file/File.{h,cpp}

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: baku, Assigned: baku)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Attached patch blob_split.patch (obsolete) — Splinter Review
This patch moves and rename code. Nothing more. It creates:

dom/file/BaseBlobImpl.{cpp,h}
dom/file/Blob.{cpp,h}
dom/file/BlobImpl.{cpp,h}
dom/file/EmptyBlobImpl.{cpp,h}
dom/file/FileBlobImpl.{cpp,h} BlobImplFile -> FileBlobImpl
dom/file/MemoryBlobImpl.{cpp,h} BlobImplMemory -> MemoryBlobImpl
dom/file/StreamBlobImpl.{cpp,h}
dom/file/StringBlobImpl.{cpp,h}
dom/file/TemporaryBlobImpl.{cpp,h} BlobImplTemporary -> TemporaryBlobImpl
Assignee: nobody → amarchesini
Attachment #8837695 - Flags: review?(bugs)
Comment on attachment 8837695 [details] [diff] [review]
blob_split.patch


>             // Ensure that file data is cached no that the content process
>             // has this data available to it when passed over:
>             blobImpl->GetSize(rv);
>+            if (rv.Failed()) {
>+              continue;
>+            }
>+
>             blobImpl->GetLastModified(rv);
>+            if (rv.Failed()) {
>+              continue;
>+            }
This has some unrelated changes. Please provide a patch which does only file splitting and now changes to behavior, otherwise reviewing is really slow.


Did you consider using hg cp so that we keep the blame still working. I think that should be done.
Attachment #8837695 - Flags: review?(bugs) → review-
Using hg cp makes the patch huge.
Attachment #8837695 - Attachment is obsolete: true
Attachment #8837958 - Flags: review?(bugs)
But way way easier to review.
Comment on attachment 8837958 [details] [diff] [review]
blob_split2.patch

-} // namespace dom
-} // namespace mozilla
+} // dom namespace
+} // mozilla namespace
Even this kind of changes shouldn't be done when moving/renaming code around.

>+namespace {
> 
> // Makes sure that aStart and aEnd is less then or equal to aSize and greater
> // than 0
>-static void
>+void
> ParseSize(int64_t aSize, int64_t& aStart, int64_t& aEnd)
weird change in middle of file copying, and even against the current mozilla coding style.


>+File::~File()
>+{}
Why this change? Why un-inline? Doesn't seem to belong to this kind of patch.


>+
>+FileBlobImpl::FileBlobImpl(nsIFile* aFile)
>+  : BaseBlobImpl(EmptyString(), EmptyString(), UINT64_MAX, INT64_MAX)
>+  , mFile(aFile)
>+  , mWholeFile(true)
Why un-inlining this constructor. Doesn't seem to belong to this kind of patch, and just make reviewing harder.

>+FileBlobImpl::FileBlobImpl(const nsAString& aName,
>+                           const nsAString& aContentType,
>+                           uint64_t aLength, nsIFile* aFile)
>+  : BaseBlobImpl(aName, aContentType, aLength, UINT64_MAX)
>+  , mFile(aFile)
>+  , mWholeFile(true)
>+{
>+  MOZ_ASSERT(mFile, "must have file");
>+}
> 
>-NS_IMPL_CYCLE_COLLECTION_CLASS(Blob)
>+FileBlobImpl::FileBlobImpl(const nsAString& aName,
>+                           const nsAString& aContentType,
>+                           uint64_t aLength, nsIFile* aFile,
>+                           int64_t aLastModificationDate)
>+  : BaseBlobImpl(aName, aContentType, aLength, aLastModificationDate)
>+  , mFile(aFile)
>+  , mWholeFile(true)
Ditto for these


>+FileBlobImpl::FileBlobImpl(nsIFile* aFile, const nsAString& aName,
>+                           const nsAString& aContentType)
>+  : BaseBlobImpl(aName, aContentType, UINT64_MAX, INT64_MAX)
>+  , mFile(aFile)
>+  , mWholeFile(true)
and this


>+FileBlobImpl::FileBlobImpl(const FileBlobImpl* aOther, uint64_t aStart,
>+                           uint64_t aLength, const nsAString& aContentType)
>+  : BaseBlobImpl(aContentType, aOther->mStart + aStart, aLength)
>+  , mFile(aOther->mFile)
>+  , mWholeFile(false)
and this



>-////////////////////////////////////////////////////////////////////////////
>-// mozilla::dom::BlobImpl implementation
>+TemporaryBlobImpl::TemporaryBlobImpl(const TemporaryBlobImpl* aOther,
>+                                     uint64_t aStart, uint64_t aLength,
>+                                     const nsAString& aContentType)
>+  : BaseBlobImpl(aContentType, aLength)
>+  , mStartPos(aStart)
>+  , mFileDescOwner(aOther->mFileDescOwner)
>+{}
Why un-inlining. Doesn't belong to this type of patch.


r+, and no need to undo those unrelated changes, but reviewing would be easier without such.
(sorry about complaining)
Attachment #8837958 - Flags: review?(bugs) → review+
> Why un-inlining. Doesn't belong to this type of patch.

The inlining changes are about the declaration forwarding of classes.
https://hg.mozilla.org/mozilla-central/rev/e2a28ac24659
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.