Bug 71205

Summary:

[Chromium] The Chromium port calls OwnPtr::get and RefPtr::get way more often than needed

Product:

WebKit

Reporter:

Adam Barth <abarth>

Component:

New Bugs

Assignee:

Adam Barth <abarth>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dglazkov, eric, levin, rniwa, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch for landing	none

Adam Barth

Reported 2011-10-30 23:26:04 PDT

[Chromium] The Chromium port calls OwnPtr::get and RefPtr::get way more often than needed

Attachments
Patch (44.34 KB, patch) 2011-10-30 23:28 PDT, Adam Barth	no flags	Details Formatted Diff Diff
Patch (42.90 KB, patch) 2011-10-31 00:06 PDT, Adam Barth	no flags	Details Formatted Diff Diff
Patch for landing (42.91 KB, patch) 2011-10-31 00:16 PDT, Adam Barth	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Adam Barth

Comment 1 2011-10-30 23:28:41 PDT

Created attachment 113015 [details] Patch

WebKit Review Bot

Comment 2 2011-10-30 23:31:35 PDT

Comment on attachment 113015 [details] Patch Attachment 113015 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10242684

Ryosuke Niwa

Comment 3 2011-10-30 23:33:24 PDT

Comment on attachment 113015 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113015&action=review > Source/WebKit/chromium/ChangeLog:9 > + calling get() on these pointer types. This patch cleans up most (all?) Nit: one space after period. > Source/WebKit/chromium/src/WebViewImpl.cpp:2180 > + HTMLInputElement* inputElem = static_cast<HTMLInputElement*>(focusedNode.get()); Can we call Node::toInputElement here?

Adam Barth

Comment 4 2011-10-30 23:33:47 PDT

Comment on attachment 113015 [details] Patch Sigh. WebPrivatePtr makes this trickier.

Adam Barth

Comment 5 2011-10-30 23:34:22 PDT

> > Source/WebKit/chromium/src/WebViewImpl.cpp:2180 > > + HTMLInputElement* inputElem = static_cast<HTMLInputElement*>(focusedNode.get()); > > Can we call Node::toInputElement here? Yeah, that would be better. This code looks sketchy.

Ryosuke Niwa

Comment 6 2011-10-30 23:35:05 PDT

Comment on attachment 113015 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113015&action=review > Source/WebKit/chromium/src/WebIDBKey.cpp:128 > - if (!m_private.get()) > + if (!m_private) Apparently this one is WebPrivatePtr :(

Ryosuke Niwa

Comment 7 2011-10-30 23:36:42 PDT

(In reply to comment #5) > > > Source/WebKit/chromium/src/WebViewImpl.cpp:2180 > > > + HTMLInputElement* inputElem = static_cast<HTMLInputElement*>(focusedNode.get()); > > > > Can we call Node::toInputElement here? > > Yeah, that would be better. This code looks sketchy. I'm seeing a horror a few lines beneath that: 2185 m_autofillPopupClient = adoptPtr(new AutofillPopupMenuClient);

Adam Barth

Comment 8 2011-10-31 00:06:31 PDT

Created attachment 113017 [details] Patch

Ryosuke Niwa

Comment 9 2011-10-31 00:09:11 PDT

Comment on attachment 113017 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113017&action=review > Source/WebKit/chromium/src/WebViewImpl.cpp:2180 > + HTMLInputElement* inputElem = static_cast<HTMLInputElement*>(focusedNode.get()); Please do focusedNode->toInputElement();

Adam Barth

Comment 10 2011-10-31 00:16:27 PDT

Created attachment 113018 [details] Patch for landing

WebKit Review Bot

Comment 11 2011-10-31 00:48:38 PDT

Comment on attachment 113018 [details] Patch for landing Clearing flags on attachment: 113018 Committed r98837: <http://trac.webkit.org/changeset/98837>

WebKit Review Bot

Comment 12 2011-10-31 00:48:45 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.