1369619 - Use ReadSegments in FetchDriver::OnDataAvailable even in the SRI case

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Would avoid some buffer copies, and arguably the code is simpler.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Use ReadSegments in the SRI case in FetchDriver::OnDataAvailable

This avoids some buffer copies and is arguably more readble.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8873706 [details] [diff] [review]
Use ReadSegments in the SRI case in FetchDriver::OnDataAvailable

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

Thanks!

::: dom/fetch/FetchDriver.cpp
@@ +658,5 @@
>  };
>  
> +struct SRIVerifierAndOutputHolder {
> +  SRICheckDataVerifier* mVerifier;
> +  nsIOutputStream* mOutputStream;

Please use a constructor to initialize and delete the default constructor.  I just worry about someone using these uninitialized pointers in the future.

@@ +671,5 @@
> +                          uint32_t aOffset,
> +                          uint32_t aCount,
> +                          uint32_t* aCountWritten)
> +{
> +  auto holder = static_cast<SRIVerifierAndOutputHolder*>(aClosure);

Can you add a MOZ_DIAGNOSTIC_ASSERT() that holder and its contents are not nullptr?

@@ +680,5 @@
> +
> +  // The rest is just like NS_CopySegmentToStream.
> +  *aCountWritten = 0;
> +  while (aCount) {
> +    uint32_t n;

Please initialize this variable.

@@ +682,5 @@
> +  *aCountWritten = 0;
> +  while (aCount) {
> +    uint32_t n;
> +    rv = holder->mOutputStream->Write(aBuffer, aCount, &n);
> +    if (NS_FAILED(rv)) {

Do you not use NS_ENSURE_SUCCESS() here because you are avoiding the warning?  Or because it was like that in the old code?

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/61599ed0f713
Use ReadSegments in the SRI case in FetchDriver::OnDataAvailable.  r=bkelly

Comment 4

[Gabriel [:gl] (ΦωΦ)]

Attachment: Bug 1369619. Use ReadSegments in the SRI case in FetchDriver::OnDataAvailable.

This avoids some buffer copies and is arguably more readble.

Review commit: https://reviewboard.mozilla.org/r/146080/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146080/

Comment 5

[Gabriel [:gl] (ΦωΦ)]

Comment on attachment 8874709 [details]
Bug 1369619.  Use ReadSegments in the SRI case in FetchDriver::OnDataAvailable.

Sorry, don't know what happened here when committing to mozreview

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Please use a constructor to initialize and delete the default constructor.

Done. 

> Can you add a MOZ_DIAGNOSTIC_ASSERT() that holder and its contents are not nullptr?

Done.

> Please initialize this variable.

Done.

> Do you not use NS_ENSURE_SUCCESS() here because you are avoiding the warning?  Or because it was like that in the old code?

Honestly, because I copy/pasted NS_CopySegmentToStream.  But if I had to pick, avoiding the warning here makes sense, because NS_BASE_STREAM_WOULD_BLOCK is a totally valid failure return value here.

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/61599ed0f713