Bug 42587 - Handle NP_ASFILE and NP_ASFILEONLY transfer modes
Summary: Handle NP_ASFILE and NP_ASFILEONLY transfer modes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Anders Carlsson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-19 12:57 PDT by Anders Carlsson
Modified: 2010-07-19 18:20 PDT (History)
2 users (show)

See Also:


Attachments
Patch (15.49 KB, patch)
2010-07-19 13:04 PDT, Anders Carlsson
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anders Carlsson 2010-07-19 12:57:58 PDT
Handle NP_ASFILE and NP_ASFILEONLY transfer modes
Comment 1 Anders Carlsson 2010-07-19 13:04:40 PDT
Created attachment 61984 [details]
Patch
Comment 2 WebKit Review Bot 2010-07-19 13:06:58 PDT
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.
Comment 3 Darin Adler 2010-07-19 13:28:40 PDT
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.
Comment 4 Anders Carlsson 2010-07-19 13:41:24 PDT
(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.
Comment 5 Adam Roben (:aroben) 2010-07-19 14:21:21 PDT
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?
Comment 6 Darin Adler 2010-07-19 14:23:06 PDT
(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 7 mitz 2010-07-19 15:30:38 PDT
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.
Comment 8 Sam Weinig 2010-07-19 15:40:59 PDT
Comment on attachment 61984 [details]
Patch

What everyone else said. r=me too!
Comment 9 Anders Carlsson 2010-07-19 18:20:47 PDT
Committed r63704: <http://trac.webkit.org/changeset/63704>