Bug 42587 – Handle NP_ASFILE and NP_ASFILEONLY transfer modes

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Anders Carlsson]

Handle NP_ASFILE and NP_ASFILEONLY transfer modes

[Anders Carlsson]

Created attachment 61984 [details]
Patch

[WebKit Review Bot]

Attachment 61984 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.h:71:  NPP_StreamAsFile is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.cpp:193:  NetscapePlugin::NPP_StreamAsFile is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 61984 [details]
Patch

> +    return CString(temporaryFilePath.data());

Does the CString constructor really require a vector that already has the '\0' in it? Or does this code end up adding two of them?

> +    void NPP_StreamAsFile(NPStream* stream, const char* filename);

Leave out the "stream" argument name?

> +    RefPtr<PluginView::Stream> stream = m_streams.get(streamID).get();

Here inside the class we can just call it Stream instead of PluginView::Stream.

[Anders Carlsson]

(In reply to comment #3)
> (From update of attachment 61984 [details])
> > +    return CString(temporaryFilePath.data());
> 
> Does the CString constructor really require a vector that already has the '\0' in it? Or does this code end up adding two of them?
> 

The constructor that takes a const char* requires it to be null terminated. The constructor that takes a length still adds a null terminator.

[Adam Roben (:aroben)]

Comment on attachment 61984 [details]
Patch

> +        // NPP_StreamAsFile could call NPN_DestroyStream and destroy the stream.
> +        if (!m_isStarted)
> +            return;
> +            return;
> +    }

Does this really do what you want?

[Darin Adler]

(In reply to comment #5)
> (From update of attachment 61984 [details])
> > +        // NPP_StreamAsFile could call NPN_DestroyStream and destroy the stream.
> > +        if (!m_isStarted)
> > +            return;
> > +            return;
> > +    }
> 
> Does this really do what you want?

Uh oh! I’m sure that at least one test will fail because of this!!!

Comment on attachment 61984 [details]
Patch

> --- a/WebCore/WebCore.xcodeproj/project.pbxproj
> +++ b/WebCore/WebCore.xcodeproj/project.pbxproj

Not mentioned in the change log.

> +#import <wtf/text/CString.h>
>  #import "PlatformString.h"

Not in ASCII order.

> +        // We failed to open the file, stop the stream.
> +        if (m_fileHandle == invalidPlatformFileHandle)
> +            stop(NPRES_NETWORK_ERR);

Is it safer to return here?

> +        if (!m_isStarted)
> +            return;
> +            return;

Too many of something.

[Sam Weinig]

Comment on attachment 61984 [details]
Patch

What everyone else said. r=me too!

[Anders Carlsson]

Committed r63704: <http://trac.webkit.org/changeset/63704>