Summary: | plugins/get-url-with-iframe-target.html fails on SnowLeopard (64-bit) | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Maciej Stachowiak <mjs> | ||||
Component: | Plug-ins | Assignee: | 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
Maciej Stachowiak
2009-12-27 23:40:19 PST
Created attachment 45548 [details]
Patch
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 on attachment 45548 [details]
Patch
This seems like a style violation too:
453 if (error != nil)
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 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. Attachment 45548 [details] was posted by a committer and has review+, assigning to Maciej Stachowiak for commit.
(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. Committed r52619: <http://trac.webkit.org/changeset/52619> > 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. |