Bug 46390 – Remove unnecessary parameter from AsyncFileWriterClient::didTruncate

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Eric U.]

The FileWriter knows the length it sent to truncate, and we're never going to get the wrong answer back.  We might as well cache what we sent rather than have an extra parameter through 10 layers of calls.  It's going to get cached at some level anyway; it might as well be right in the FileWriter.

[Eric U.]

Created attachment 68582 [details]
Patch

[David Levin]

Comment on attachment 68582 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68582&action=review

Just a few things to address.

> WebCore/fileapi/FileWriter.cpp:@
>  void FileWriter::truncate(long long posi

Please init m_truncateLength to 0 in constructor.

> WebCore/fileapi/FileWriter.cpp:174
> +    ASSERT(m_truncateLength >= 0);

Why are there both of these asserts?
Should they also be in FileWriter::truncate when m_truncateLength is set?

> WebCore/fileapi/FileWriter.cpp:178
>      m_readyState = DONE;

Set m_truncateLength be set back to 0 here.

> WebKit/chromium/src/AsyncFileWriterChromium.h:-64
> -    virtual void didTruncate(long long length);

What code calls this api? (I don't see a change to it.)

[Eric U.]

Created attachment 68605 [details]
Patch

[Eric U.]

(In reply to comment #2)
> (From update of attachment 68582 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68582&action=review
> 
> Just a few things to address.
> 
> > WebCore/fileapi/FileWriter.cpp:@
> >  void FileWriter::truncate(long long posi
> 
> Please init m_truncateLength to 0 in constructor.

I used -1, as 0 is a legal value.

> > WebCore/fileapi/FileWriter.cpp:174
> > +    ASSERT(m_truncateLength >= 0);
> 
> Why are there both of these asserts?

Typo--fixed.  However, the spec has changed, and now you can use truncate to extend the file as well, so I've rolled that in to this change.

> Should they also be in FileWriter::truncate when m_truncateLength is set?

Actually, there should be a soft error there [a thrown exception], so that we can assert here.  Fixed.

> > WebCore/fileapi/FileWriter.cpp:178
> >      m_readyState = DONE;
> 
> Set m_truncateLength be set back to 0 here.

Done [-1].

> > WebKit/chromium/src/AsyncFileWriterChromium.h:-64
> > -    virtual void didTruncate(long long length);
> 
> What code calls this api? (I don't see a change to it.)

I haven't written it yet ;'>.
My Chromium implementation is starting at http://codereview.chromium.org/3440021/show, and the WebCore implementation will follow shortly thereafter.

[WebKit Commit Bot]

Comment on attachment 68605 [details]
Patch

Clearing flags on attachment: 68605

Committed r68242: <http://trac.webkit.org/changeset/68242>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.