Bug 32982

Summary: plugins/get-url-with-iframe-target.html fails on SnowLeopard (64-bit)
Product: WebKit Reporter: Maciej Stachowiak <mjs>
Component: Plug-insAssignee: Maciej Stachowiak <mjs>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, eric
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch ap: review+, ap: commit-queue-

Description Maciej Stachowiak 2009-12-27 23:40:19 PST
plugins/get-url-with-iframe-target.html fails on SnowLeopard (64-bit)
Comment 1 Maciej Stachowiak 2009-12-27 23:51:14 PST
Created attachment 45548 [details]
Patch
Comment 2 Eric Seidel (no email) 2009-12-28 00:03:58 PST
Comment on attachment 45548 [details]
Patch

argument name seems superflous:
 73     static NPReason reasonForError(NSError *error);

Can't tell if this spacing is right or wrong, seems like spaces beofre * are wrong here:
 77     static PassRefPtr<PluginRequest> create(uint32_t requestID, NSURLRequest *request, NSString *frameName, bool allowPopups)

And here:
 88     PluginRequest(uint32_t requestID, NSURLRequest *request, NSString *frameName, bool allowPopups)

Or shoudl they be " *" because this is a .mm file?

This looks sane.  Looks like a good change.  I still feel like you probably want someone who's been in this code before to review it though.
Comment 3 Eric Seidel (no email) 2009-12-28 00:04:25 PST
Comment on attachment 45548 [details]
Patch

This seems like a style violation too:
 453     if (error != nil)
Comment 4 Maciej Stachowiak 2009-12-28 00:16:05 PST
I think Anders is the only one truly familiar with this code and he is going to be on vacation for several weeks still. I made the suggested style fixes in my copy.
Comment 5 Alexey Proskuryakov 2009-12-28 09:53:11 PST
Comment on attachment 45548 [details]
Patch

> +    void webFrameDidFinishLoadWithReason(WebFrame*, NPReason);

Style nit: should be "WebFrame *". I'm not sure if the changes in your local copy are in right or wrong direction - ObjC classes should have detached stars.

> -    NetscapePluginInstanceProxy(NetscapePluginHostProxy*, WebHostedNetscapePluginView *, bool fullFramePlugin);
> +    NetscapePluginInstanceProxy(NetscapePluginHostProxy*, WebHostedNetscapePluginView*, bool fullFramePlugin);

Same issue (old code was correct).

> -    NPError loadRequest(NSURLRequest *, const char* cTarget, bool currentEventIsUserGesture, uint32_t& streamID);
> +    NPError loadRequest(NSURLRequest*, const char* cTarget, bool currentEventIsUserGesture, uint32_t& streamID);

Same issue.

> +    FrameLoadMap  m_pendingFrameLoads;

Two spaces instead of one.

> +	// Check if another plug-in view or even this view is waiting for the frame to load.
> +        // If it is, tell it that the load was cancelled because it will be anyway.

Tab here.

> +void NetscapePluginInstanceProxy::webFrameDidFinishLoadWithReason(WebFrame* webFrame, NPReason reason)

Misplaced star again (I probably missed some instances anyway).

> +    RefPtr<PluginRequest> pluginRequest = m_pendingFrameLoads.get(webFrame);

Why does this need to be a RefPtr?

> -        PluginRequest* pluginRequest = new PluginRequest(requestID, request, target, allowPopups);
> +        RefPtr<PluginRequest> pluginRequest = PluginRequest::create(requestID, request, target, allowPopups);
>          m_pluginRequests.append(pluginRequest);

This is probably not performance critical code, but I think that a release() would prevent refcount thrash.

r=me with style fixes.
Comment 6 Eric Seidel (no email) 2009-12-28 22:41:20 PST
Attachment 45548 [details] was posted by a committer and has review+, assigning to Maciej Stachowiak for commit.
Comment 7 Maciej Stachowiak 2009-12-29 05:46:40 PST
(In reply to comment #5)
> (From update of attachment 45548 [details])
> > +    void webFrameDidFinishLoadWithReason(WebFrame*, NPReason);
> 
> Style nit: should be "WebFrame *". I'm not sure if the changes in your local
> copy are in right or wrong direction - ObjC classes should have detached stars.

I reviewed our coding style guidelines, and our rule does not appear to be based on the kind of class a pointer type points to, but rather to the language of the code using it:

2. Pointer types in non-C++ code — Pointer types should be written with a space between the type and the * (so the * is adjacent to the following identifier if any).
3. Pointer and reference types in C++ code — Both pointer types and reference types should be written with no space between the type name and the * or &.

I'm not sure whether Objective-C++ counts as "C++ code" or "non-C++ code" but since the contents of these files are primarily C++ classes, I will take it as C++ code, and thus leave the *s next to the type name.

I am not sure whether it would be more consistent or less confusing to follow the alternate rule of separating the * only for ObjC classes regardless of the type of code. It might be an improvement but does not seem to be the current rule. Thus, landed without any * changes, but:

> 
> > +    FrameLoadMap  m_pendingFrameLoads;
> 
> Two spaces instead of one.

I fixed this.

> 
> > +	// Check if another plug-in view or even this view is waiting for the frame to load.
> > +        // If it is, tell it that the load was cancelled because it will be anyway.
> 
> Tab here.

And this.

> 
> > +    RefPtr<PluginRequest> pluginRequest = m_pendingFrameLoads.get(webFrame);
> 
> Why does this need to be a RefPtr?

That's what .get() on a a RefPtr-valued HashMap returns. I suppose I could make it a raw pointer. Another alternative would be to use find() and both access and then remove by iterator. That's what I ended up doing (the find() version).

> 
> > -        PluginRequest* pluginRequest = new PluginRequest(requestID, request, target, allowPopups);
> > +        RefPtr<PluginRequest> pluginRequest = PluginRequest::create(requestID, request, target, allowPopups);
> >          m_pluginRequests.append(pluginRequest);
> 
> This is probably not performance critical code, but I think that a release()
> would prevent refcount thrash.

Done.
Comment 8 Maciej Stachowiak 2009-12-29 05:53:20 PST
Committed r52619: <http://trac.webkit.org/changeset/52619>
Comment 9 Alexey Proskuryakov 2009-12-29 11:19:34 PST
> I reviewed our coding style guidelines, and our rule does not appear to be
> based on the kind of class a pointer type points to, but rather to the language
> of the code using it:
<...>
> I'm not sure whether Objective-C++ counts as "C++ code" or "non-C++ code" but
> since the contents of these files are primarily C++ classes, I will take it as
> C++ code, and thus leave the *s next to the type name.

Most existing code uses spacing appropriate to the particular variable. For example, until your commit, there were no instances of "NSURLRequest*" in WebKit, and only a few in WebCore.

Since my r+ was based on the expectation of fixing the style issues, I think it would have been appropriate to discuss them a bit more before landing, or at least not to make the changes to code you didn't touch otherwise.