Bug 32982 – plugins/get-url-with-iframe-target.html fails on SnowLeopard (64-bit)

• Component: Plug-ins
• Status: RESOLVED FIXED

[Maciej Stachowiak]

plugins/get-url-with-iframe-target.html fails on SnowLeopard (64-bit)

[Maciej Stachowiak]

Created attachment 45548 [details]
Patch

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Comment on attachment 45548 [details]
Patch

This seems like a style violation too:
 453     if (error != nil)

[Maciej Stachowiak]

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.

[Alexey Proskuryakov]

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.

[Eric Seidel (no email)]

Attachment 45548 [details] was posted by a committer and has review+, assigning to Maciej Stachowiak for commit.

[Maciej Stachowiak]

(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.

[Maciej Stachowiak]

Committed r52619: <http://trac.webkit.org/changeset/52619>

[Alexey Proskuryakov]

> 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.