Bug 59437 - Moar strict OwnPtr fixes for WebKit/chromium
Summary: Moar strict OwnPtr fixes for WebKit/chromium
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: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-26 01:00 PDT by Adam Barth
Modified: 2011-04-26 11:23 PDT (History)
3 users (show)

See Also:

Attachments
Patch (14.52 KB, patch)
2011-04-26 01:01 PDT, Adam Barth 		no flags Details | Formatted Diff | Diff
Patch (15.41 KB, patch)
2011-04-26 08:21 PDT, Adam Barth 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2011-04-26 01:00:55 PDT 
Moar strict OwnPtr fixes for WebKit/chromium
Comment 1 Adam Barth 2011-04-26 01:01:42 PDT 
Created attachment 91079 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-04-26 08:12:28 PDT 
Comment on attachment 91079 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=91079&action=review

> Source/WebKit/chromium/src/WebDataSourceImpl.cpp:43
> +OwnPtr<WebPluginLoadObserver> WebDataSourceImpl::m_nextPluginLoadObserver;

Won't this make an exit-time destructor?
Comment 3 Adam Barth 2011-04-26 08:17:42 PDT 
(In reply to comment #2)
> (From update of attachment 91079 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91079&action=review
> 
> > Source/WebKit/chromium/src/WebDataSourceImpl.cpp:43
> > +OwnPtr<WebPluginLoadObserver> WebDataSourceImpl::m_nextPluginLoadObserver;
> 
> Won't this make an exit-time destructor?

exit-time destructors are allowed in Chromium.  I can change this not be one, if you like.
Comment 4 Adam Barth 2011-04-26 08:21:28 PDT 
Created attachment 91109 [details]
Patch
Comment 5 Adam Barth 2011-04-26 08:27:11 PDT 
Patch updated to avoid exit-time destructor.
Comment 6 David Levin 2011-04-26 08:38:38 PDT 
Comment on attachment 91109 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=91109&action=review

Glad you got rid of the exit time destructor (as it bothered me too when I glanced at this before going off to bed).

Only one comment to consider.

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:319
> +void WebDevToolsAgent::interruptAndDispatch(MessageDescriptor* rawDescriptor)

Yeah for decent variable names.

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:342
> +    PageScriptDebugServer::interruptAndRun(new DebuggerTask(descriptor.release()));

Seems odd that there is no adopt of the "new DebuggerTask" here.
Comment 7 Adam Barth 2011-04-26 10:02:28 PDT 
> > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:342
> > +    PageScriptDebugServer::interruptAndRun(new DebuggerTask(descriptor.release()));
> 
> Seems odd that there is no adopt of the "new DebuggerTask" here.

Good question:

    static void interruptAndRun(PassOwnPtr<Task>);

Ah!  I've figured it out.  I haven't been removing the LOOSE_PASS_OWN_PTR define in PassOwnPtr.h!
Comment 8 WebKit Commit Bot 2011-04-26 11:23:33 PDT 
Comment on attachment 91109 [details]
Patch

Clearing flags on attachment: 91109

Committed r84933: <http://trac.webkit.org/changeset/84933>
Comment 9 WebKit Commit Bot 2011-04-26 11:23:38 PDT 
All reviewed patches have been landed.  Closing bug.