Bug 59437

Summary:

Moar strict OwnPtr fixes for WebKit/chromium

Product:

WebKit

Reporter:

Adam Barth <abarth>

Component:

New Bugs

Assignee:

Adam Barth <abarth>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, eric, levin

Priority:

Version:

528+ (Nightly build)

Hardware:

Other

OS:

OS X 10.5

Attachments:

Description	Flags
Patch	none
Patch	none

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.