Closed Bug 1017765 Opened 7 years ago Closed 7 years ago

FileReader: Deprecate Necko channel from DOMFileReader; use StreamTransportService instead

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: seward.zheng, Assigned: seward.zheng)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 11 obsolete files)

Currently, we use necko (nsChannel) to implement FileReader API. We propose a change to use StreamTransportService. The benefit is that the support of FileReader on web worker could be easier.
Summary: Deprecate Necko channel from DOMFileReader; use StreamTransportService instead → FileReader: Deprecate Necko channel from DOMFileReader; use StreamTransportService instead
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → szheng
Status: NEW → ASSIGNED
Attachment #8431027 - Flags: review?(bent.mozilla)
Comment on attachment 8431027 [details] [diff] [review]
Patch

Review of attachment 8431027 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/FileIOObject.cpp
@@ +148,3 @@
>  NS_IMETHODIMP
> +FileIOObject::OnInputStreamReady(nsIAsyncInputStream* aStream) {
> +  if (mReadyState != 1 || !IsCurrentStream(aStream)) {

This shouldn't be needed, so let's file a followup bug to make canceling a stream never call back after.

@@ +154,2 @@
>    nsresult rv;
> +  uint64_t aCount = 0;

No need to set aCount here, and just name it |count|.

@@ +154,3 @@
>    nsresult rv;
> +  uint64_t aCount = 0;
> +  rv = aStream->Available(&aCount);

How about:

  if (rv == NS_BASE_STREAM_CLOSED) {
    MOZ_ASSERT(!count);
    rv = NS_OK;
  }

@@ +154,5 @@
>    nsresult rv;
> +  uint64_t aCount = 0;
> +  rv = aStream->Available(&aCount);
> +  
> +  if (aCount) {

if (NS_SUCCEEDED(rv) && count)

@@ +155,5 @@
> +  uint64_t aCount = 0;
> +  rv = aStream->Available(&aCount);
> +  
> +  if (aCount) {
> +    nsresult rvr = DoReadData(aStream, aCount);

Now there's no need for |rvr|, just do:

  rv = DoReadData(aStream, aCount);

@@ +160,5 @@
> +    if (NS_FAILED(rvr)) rv = rvr;
> +  }
> +  
> +  // If stream is closed (either success or failure), conclude this transfer
> +  if (NS_FAILED(rv)) {

This should probably be |!count || NS_FAILED(rv)|

@@ +164,5 @@
> +  if (NS_FAILED(rv)) {
> +    return OnLoadEnd(rv);
> +  }
> +  
> +  DoAsyncWait(aStream);

You should check the return value from this, and call OnLoadEnd if it fails

@@ +181,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +FileIOObject::OnLoadEnd(nsresult aStatus)

This should s/NS_IMETHODIMP/nsresult/

@@ +195,1 @@
>    NS_ENSURE_SUCCESS(rv, rv);

Let's file a followup for making sure we always dispatch an error event if DoOnLoadEnd fails.

@@ +195,4 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    // Set the status field as appropriate
> +  if (NS_FAILED(aStatus) && aStatus != NS_BASE_STREAM_CLOSED) {

The extra check shouldn't be necessary now.

@@ +206,5 @@
>  
>    return NS_OK;
>  }
>  
> +bool FileIOObject::DoAsyncWait(nsIAsyncInputStream* aStream)

Nit: return types are usually on their own line

@@ +210,5 @@
> +bool FileIOObject::DoAsyncWait(nsIAsyncInputStream* aStream)
> +{
> +  nsresult rv;
> +  nsCOMPtr<nsIThread> currentThread = do_GetCurrentThread();
> +  rv = aStream->AsyncWait(this, 0, 0, currentThread);

You can remove the extra nsCOMPtr and just call NS_GetCurrentThread():

  rv = aStream->AsyncWait(this, 0, 0, NS_GetCurrentThread());

::: content/base/src/FileIOObject.h
@@ +15,4 @@
>  
>  #include "mozilla/dom/DOMError.h"
>  
> +

Nit: Extra line, please remove.

@@ +70,5 @@
>    virtual void DoAbort(nsAString& aEvent) = 0;
> +  
> +  NS_IMETHOD DoReadData(nsIAsyncInputStream *aStream, uint64_t aCount) = 0;
> +  
> +  NS_IMETHOD OnLoadEnd(nsresult aStatus);

It doesn't look like this needs to be virtual, and private.

@@ +74,5 @@
> +  NS_IMETHOD OnLoadEnd(nsresult aStatus);
> +  
> +  NS_IMETHOD DoOnLoadEnd(nsresult aStatus, nsAString& aSuccessEvent,
> +                         nsAString& aTerminationEvent) = 0;
> +                         

Nit: Extra spaces here and elsewhere, please remove

@@ +77,5 @@
> +                         nsAString& aTerminationEvent) = 0;
> +                         
> +  virtual bool IsCurrentStream(nsIAsyncInputStream* aStream) = 0;
> +
> +  bool DoAsyncWait(nsIAsyncInputStream* aStream);

Let's make this return nsresult.

::: content/base/src/nsDOMFileReader.cpp
@@ +46,5 @@
>  #include "jsfriendapi.h"
>  
> +#include "mozilla/dom/WorkerPrivate.h"
> +#include "mozilla/dom/WorkerScope.h"
> +#include "File.h"

Nit: These belong to a different patch.

@@ +55,3 @@
>  using namespace mozilla;
>  using namespace mozilla::dom;
> +using namespace mozilla::dom::workers;

Nit: This belongs in a different patch.

@@ +107,5 @@
>  
>  //nsDOMFileReader constructors/initializers
>  
>  nsDOMFileReader::nsDOMFileReader()
> +  : mAsyncStream(nullptr), mFileData(nullptr),

Nit: nsCOMPtrs don't need to be initialized

@@ +331,2 @@
>  {
> +  

Nit: Extra space here.

@@ +339,5 @@
>    aSuccessEvent = NS_LITERAL_STRING(LOAD_STR);
>    aTerminationEvent = NS_LITERAL_STRING(LOADEND_STR);
>  
>    // Clear out the data if necessary
> +  if (NS_FAILED(aStatus) && aStatus != NS_BASE_STREAM_CLOSED) {

This extra check shouldn't be needed now.

@@ +444,5 @@
>    mDataFormat = aDataFormat;
>    CopyUTF16toUTF8(aCharset, mCharset);
>  
> +  nsresult rv;
> +  nsCOMPtr<nsIStreamTransportService> sts = 

Nit: trailing space

@@ +447,5 @@
> +  nsresult rv;
> +  nsCOMPtr<nsIStreamTransportService> sts = 
> +    do_GetService(kStreamTransportServiceCID, &rv);
> +  if (NS_FAILED(rv)) {
> +    aRv.Throw(rv); return;

Nit: Only one statement per line, please do for all below.

@@ +457,5 @@
> +    aRv.Throw(rv); return;
> +  }
> +
> +  nsCOMPtr<nsITransport> transport;
> +  rv = sts->CreateInputTransport(stream, 0, -1, true, getter_AddRefs(transport));

Let's do:

  rv =  sts->CreateInputTransport(stream,
                                  /* aStartOffset */ 0,
                                  /* aReadLimit */ -1,
                                  /* aCloseWhenDone */ true,
                                  getter_AddRefs(transport));

@@ +463,5 @@
> +    aRv.Throw(rv); return;
> +  }
> +
> +  nsCOMPtr<nsIInputStream> wrapper;
> +  rv = transport->OpenInputStream(0, 0, 0, getter_AddRefs(wrapper));

Same here!

@@ +467,5 @@
> +  rv = transport->OpenInputStream(0, 0, 0, getter_AddRefs(wrapper));
> +  if (NS_FAILED(rv)) {
> +    aRv.Throw(rv); return;
> +  }
> +  

Nit: spaces

@@ +468,5 @@
> +  if (NS_FAILED(rv)) {
> +    aRv.Throw(rv); return;
> +  }
> +  
> +  mAsyncStream = do_QueryInterface(wrapper, &rv);

This one doesn't need to be error checked or use rv. You can MOZ_ASSERT that |mAsyncStream| is non-null.

Also, before setting, you can assert that |mAsyncStream| is null.

@@ +477,5 @@
>    mTotal = mozilla::dom::kUnknownSize;
>    mFile->GetSize(&mTotal);
>  
> +  if (!DoAsyncWait(mAsyncStream)) {
> +    aRv.Throw(rv); return;

Here |rv| will be a success code.

::: content/base/src/nsDOMFileReader.h
@@ +16,5 @@
>  #include "nsTArray.h"              
>  #include "nsIJSNativeInitializer.h"
>  #include "prtime.h"                
>  #include "nsITimer.h"              
> +#include "nsIAsyncInputStream.h"

You can just forward declare this.

@@ +27,4 @@
>  
>  #include "FileIOObject.h"
>  
> +

Nit: extra line here.

@@ +50,5 @@
>  
>    // FileIOObject overrides
>    virtual void DoAbort(nsAString& aEvent) MOZ_OVERRIDE;
> +  
> +  NS_IMETHOD DoReadData(nsIAsyncInputStream *aStream, uint64_t aCount) MOZ_OVERRIDE;

s/NS_IMETHOD/nsresult/

@@ +53,5 @@
> +  
> +  NS_IMETHOD DoReadData(nsIAsyncInputStream *aStream, uint64_t aCount) MOZ_OVERRIDE;
> +  
> +  NS_IMETHOD DoOnLoadEnd(nsresult aStatus, nsAString& aSuccessEvent,
> +                         nsAString& aTerminationEvent) MOZ_OVERRIDE;

Same here.

@@ +173,5 @@
>      mFileData = nullptr;
>      mDataLen = 0;
>    }
>  
> +  nsCOMPtr<nsIAsyncInputStream> mAsyncStream;

Let's move this to the the FileIOObject class. Then you can get rid of the IsCurrentStream() virtual function also.

@@ +183,5 @@
>  
>    eDataFormat mDataFormat;
>  
>    nsString mResult;
> +  

Nit: This looks like extra whitespace.

@@ +188,1 @@
>    nsCOMPtr<nsIPrincipal> mPrincipal;

As you noticed it looks like you can get rid of this now!

::: dom/webidl/FileReader.webidl
@@ +13,5 @@
>  [Constructor]
>  interface FileReader : EventTarget {
>    // async read methods
>    [Throws]
> +  void readAsArrayBuffer(any blob);

We'll need to revert these changes and move them into the patch for worker bindings.
Attachment #8431027 - Flags: review?(bent.mozilla)
Blocks: 1018021
No longer blocks: 1018021
Blocks: 1018021
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #8431027 - Attachment is obsolete: true
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #8431336 - Attachment is obsolete: true
Attachment #8431337 - Flags: review?(bent.mozilla)
Attached patch Patch v3 (obsolete) — Splinter Review
Attachment #8431337 - Attachment is obsolete: true
Attachment #8431337 - Flags: review?(bent.mozilla)
Attachment #8431348 - Flags: review?(bent.mozilla)
Attachment #8431348 - Flags: review?(bent.mozilla)
Attached patch Patch v4 (obsolete) — Splinter Review
Link to try run: https://tbpl.mozilla.org/?tree=Try&rev=0cb2230749dc
Attachment #8431348 - Attachment is obsolete: true
Attachment #8432791 - Flags: review?(bent.mozilla)
Attached patch Patch v4 (obsolete) — Splinter Review
Attachment #8432791 - Attachment is obsolete: true
Attachment #8432791 - Flags: review?(bent.mozilla)
Attachment #8432796 - Flags: review?(bent.mozilla)
Comment on attachment 8432796 [details] [diff] [review]
Patch v4

Review of attachment 8432796 [details] [diff] [review]:
-----------------------------------------------------------------

This looks really good! Mostly cosmetic stuff to fix below, but a few problems also:

Oh, please revert as many whitespace-only changes as you can from this patch. We can do a followup patch that just does whitespace later.

::: content/base/src/FileIOObject.cpp
@@ +148,2 @@
>  NS_IMETHODIMP
> +FileIOObject::OnInputStreamReady(nsIAsyncInputStream* aStream) {

Nit: Please put this { on its own line.

@@ +153,4 @@
>  
> +  nsresult rv;
> +  uint64_t aCount;
> +  rv = aStream->Available(&aCount);

Nit: Move the definition in line with the assignment (e.g. |nsresult rv = aStream->Available(...);|

@@ +163,5 @@
> +    rv = DoAsyncWait(aStream);
> +  }
> +
> +  if (!aCount || NS_FAILED(rv)) {
> +    if (rv == NS_BASE_STREAM_CLOSED) rv = NS_OK;

Nit: if statements should always be braced and each statement should be on its own line.

@@ +213,5 @@
> +FileIOObject::DoAsyncWait(nsIAsyncInputStream* aStream)
> +{
> +  nsresult rv;
> +  rv = aStream->AsyncWait(this, 0, 0, NS_GetCurrentThread());
> +  return rv;

Nit: The stack variable is not used here.

::: content/base/src/FileIOObject.h
@@ +74,5 @@
> +
> +  NS_IMETHOD DoOnLoadEnd(nsresult aStatus, nsAString& aSuccessEvent,
> +                         nsAString& aTerminationEvent) = 0;
> +
> +  nsresult DoAsyncWait(nsIAsyncInputStream* aStream);

Nit: It would be nice to put all the pure virtual methods together and then have all the non-virtuals together below. That way it is easier for other developers to read and see what methods they need to implement in subclasses.

::: content/base/src/nsDOMFileReader.cpp
@@ +239,5 @@
> +
> +  if (mAsyncStream) {
> +    mAsyncStream->Close();
> +    nsCOMPtr<nsIAsyncInputStream> stream;
> +    mAsyncStream.swap(stream);

The swap-to-stack-nsCOMPtr pattern is only really useful if you're doing other things and always need to remember to free something. Here it's much simpler to simply set mAsyncStream to null.

@@ +323,5 @@
>    return rv;
>  }
>  
> +NS_IMETHODIMP
> +nsDOMFileReader::DoReadData(nsIAsyncInputStream* aStream, uint64_t aCount) {

Nit: { on its own line.

@@ +326,5 @@
> +NS_IMETHODIMP
> +nsDOMFileReader::DoReadData(nsIAsyncInputStream* aStream, uint64_t aCount) {
> +  MOZ_ASSERT(aStream);
> +
> +  if (mDataFormat == FILE_AS_BINARY) {

Here you should be able to assert that |mResult.Length() == mDataLen| like the old code used to do with |aOffset|.

@@ +331,5 @@
> +    //Continuously update our binary string as data comes in
> +    uint32_t oldLen = mResult.Length();
> +    if (uint64_t(oldLen) + aCount > UINT32_MAX)
> +      return NS_ERROR_OUT_OF_MEMORY;
> +    char16_t *buf = nullptr;

Nit: It looks like you removed a bunch of whitespace that broke this code up into easier-to-read chunks. Please add them back.

@@ +336,5 @@
> +    mResult.GetMutableData(&buf, oldLen + aCount, fallible_t());
> +    NS_ENSURE_TRUE(buf, NS_ERROR_OUT_OF_MEMORY);
> +    uint32_t bytesRead = 0;
> +    aStream->ReadSegments(ReadFuncBinaryString, buf + oldLen, aCount,
> +                         &bytesRead);

Nit: Your indentation is off by one here.

@@ +357,5 @@
> +
> +    uint32_t bytesRead = 0;
> +    aStream->Read(mFileData + mDataLen, aCount, &bytesRead);
> +    NS_ASSERTION(bytesRead == aCount, "failed to read data");
> +

Nit: Please remove the extra line here.

@@ +368,4 @@
>  // Helper methods
>  
>  void
>  nsDOMFileReader::ReadFileContent(JSContext* aCx,

I don't see anywhere where mDataLen is being reset here... What happens when these objects are reused?

@@ +393,4 @@
>  
> +  nsCOMPtr<nsIStreamTransportService> sts =
> +    do_GetService(kStreamTransportServiceCID, &rv);
> +  if (NS_FAILED(rv)) {

Let's use |if (NS_WARN_IF(NS_FAILED(rv))) {| here and in the other places you've modified below.

@@ +420,5 @@
> +  rv = transport->OpenInputStream(/* aFlags */ 0,
> +                                  /* aSegmentSize */ 0,
> +                                  /* aSegmentCount */ 0,
> +                                  getter_AddRefs(wrapper));
> +/*  if (NS_FAILED(rv)) {

Why is this commented out?

@@ +434,5 @@
>    mFile->GetSize(&mTotal);
>  
> +  rv = DoAsyncWait(mAsyncStream);
> +  if (NS_FAILED(rv)) {
> +    return;

This needs to call |aRv.Throw(rv)| also.

::: content/base/src/nsDOMFileReader.h
@@ +26,4 @@
>  
>  #include "FileIOObject.h"
>  
> +

Nit: Please remove this extra line.

@@ +49,5 @@
>  
>    // FileIOObject overrides
>    virtual void DoAbort(nsAString& aEvent) MOZ_OVERRIDE;
> +
> +  NS_IMETHOD DoReadData(nsIAsyncInputStream *aStream, uint64_t aCount) MOZ_OVERRIDE;

s/NS_IMETHOD/virtual nsresult/

Here and below.
Attachment #8432796 - Flags: review?(bent.mozilla) → review-
Attached patch Patch v5 (obsolete) — Splinter Review
Q:Here you should be able to assert that |mResult.Length() == mDataLen| like the old code used to do with |aOffset|.

No, we do not have aOffset now. What we have is aCount, which means the bytes available. aOffset means the bytes read. They are inherently different.

Q:I don't see anywhere where mDataLen is being reset here... What happens when these objects are reused?

It is reset in the function FreeFileData. In the history, it lives in the header file unfortunately.

Other questions are solved.
Attachment #8432796 - Attachment is obsolete: true
Attachment #8433053 - Flags: review?(bent.mozilla)
Attached patch Patch v5 (obsolete) — Splinter Review
Ok, I misunderstood your point in the assertion. The assertion is added.
Attachment #8433053 - Attachment is obsolete: true
Attachment #8433053 - Flags: review?(bent.mozilla)
Attachment #8433055 - Flags: review?(bent.mozilla)
Attached patch Patch v6 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=b3b6d10da605

The try run looks okay. BTW, OS X 10.6 seems to be down for a while.
Attachment #8433055 - Attachment is obsolete: true
Attachment #8433055 - Flags: review?(bent.mozilla)
Attachment #8433637 - Flags: review?(bent.mozilla)
(In reply to Shihua Zheng from comment #10)
> Q:Here you should be able to assert that |mResult.Length() == mDataLen| like
> the old code used to do with |aOffset|.
> 
> No, we do not have aOffset now. What we have is aCount, which means the
> bytes available. aOffset means the bytes read. They are inherently different.

Hm, I think maybe I didn't explain clearly.

The old code used to assert |mResult.Length() == aOffset|. You've replaced |aOffset| everywhere with |mDataLen| since that is the same as what |aOffset| used to mean.

So you should now be able to assert that |mResult.Length() == mDataLen|, right?
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #13)
> (In reply to Shihua Zheng from comment #10)
> > Q:Here you should be able to assert that |mResult.Length() == mDataLen| like
> > the old code used to do with |aOffset|.
> > 
> > No, we do not have aOffset now. What we have is aCount, which means the
> > bytes available. aOffset means the bytes read. They are inherently different.
> 
> Hm, I think maybe I didn't explain clearly.
> 
> The old code used to assert |mResult.Length() == aOffset|. You've replaced
> |aOffset| everywhere with |mDataLen| since that is the same as what
> |aOffset| used to mean.
> 
> So you should now be able to assert that |mResult.Length() == mDataLen|,
> right?

Yes, I've added it (Line 332).
Attached patch Patch v7 (obsolete) — Splinter Review
Attachment #8433637 - Attachment is obsolete: true
Attachment #8433637 - Flags: review?(bent.mozilla)
Attached patch Patch v7 (obsolete) — Splinter Review
Attachment #8437313 - Attachment is obsolete: true
Attachment #8437314 - Flags: review?(bent.mozilla)
Comment on attachment 8437314 [details] [diff] [review]
Patch v7

Review of attachment 8437314 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great! r+ with these things fixed:

::: content/base/src/FileIOObject.cpp
@@ +213,5 @@
>  
> +nsresult
> +FileIOObject::DoAsyncWait(nsIAsyncInputStream* aStream)
> +{
> +  return aStream->AsyncWait(this, 0, 0, NS_GetCurrentThread());

Nit: Let's add /* */ comments around those 0 args so that the reader knows what they mean.

::: content/base/src/FileIOObject.h
@@ +67,5 @@
>  protected:
>    // Implemented by the derived class to do whatever it needs to do for abort
>    virtual void DoAbort(nsAString& aEvent) = 0;
> +
> +  virtual nsresult DoReadData(nsIAsyncInputStream *aStream, uint64_t aCount) = 0;

Nit: Let's put the * on the left there.

@@ +70,5 @@
> +
> +  virtual nsresult DoReadData(nsIAsyncInputStream *aStream, uint64_t aCount) = 0;
> +
> +  virtual nsresult DoOnLoadEnd(nsresult aStatus, nsAString& aSuccessEvent,
> +                         nsAString& aTerminationEvent) = 0;

Nit: Your indentation is off here.

::: content/base/src/nsDOMFileReader.h
@@ +46,5 @@
>    // nsIInterfaceRequestor 
>    NS_DECL_NSIINTERFACEREQUESTOR
>  
>    // FileIOObject overrides
> +  void DoAbort(nsAString& aEvent) MOZ_OVERRIDE;

Nit: Please keep this marked as 'virtual', the other two below.

@@ +48,5 @@
>  
>    // FileIOObject overrides
> +  void DoAbort(nsAString& aEvent) MOZ_OVERRIDE;
> +
> +  nsresult DoReadData(nsIAsyncInputStream *aStream, uint64_t aCount) MOZ_OVERRIDE;

Nit: Let's put the * on the left there.

@@ +51,5 @@
> +
> +  nsresult DoReadData(nsIAsyncInputStream *aStream, uint64_t aCount) MOZ_OVERRIDE;
> +
> +  nsresult DoOnLoadEnd(nsresult aStatus, nsAString& aSuccessEvent,
> +                         nsAString& aTerminationEvent) MOZ_OVERRIDE;

Nit: Your indentation is off here.
Attachment #8437314 - Flags: review?(bent.mozilla) → review+
Attached patch Patch v7Splinter Review
Style cleared, need checkin?
Attachment #8437314 - Attachment is obsolete: true
Flags: needinfo?(bent.mozilla)
https://hg.mozilla.org/mozilla-central/rev/69e8adfd9257
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.