RESOLVED FIXED 71205
[Chromium] The Chromium port calls OwnPtr::get and RefPtr::get way more often than needed 
https://bugs.webkit.org/show_bug.cgi?id=71205
Summary [Chromium] The Chromium port calls OwnPtr::get and RefPtr::get way more often...
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
DetailsFormatted DiffDiff
Patch (42.90 KB, patch)
2011-10-31 00:06 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Patch for landing (42.91 KB, patch)
2011-10-31 00:16 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
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.