Bug 71205 - [Chromium] The Chromium port calls OwnPtr::get and RefPtr::get way more often than needed
Summary: [Chromium] The Chromium port calls OwnPtr::get and RefPtr::get way more often...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-30 23:26 PDT by Adam Barth
Modified: 2011-10-31 00:48 PDT (History)
5 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2011-10-30 23:26:04 PDT
[Chromium] The Chromium port calls OwnPtr::get and RefPtr::get way more often than needed
Comment 1 Adam Barth 2011-10-30 23:28:41 PDT
Created attachment 113015 [details]
Patch
Comment 2 WebKit Review Bot 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
Comment 3 Ryosuke Niwa 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?
Comment 4 Adam Barth 2011-10-30 23:33:47 PDT
Comment on attachment 113015 [details]
Patch

Sigh.  WebPrivatePtr makes this trickier.
Comment 5 Adam Barth 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.
Comment 6 Ryosuke Niwa 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 :(
Comment 7 Ryosuke Niwa 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);
Comment 8 Adam Barth 2011-10-31 00:06:31 PDT
Created attachment 113017 [details]
Patch
Comment 9 Ryosuke Niwa 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();
Comment 10 Adam Barth 2011-10-31 00:16:27 PDT
Created attachment 113018 [details]
Patch for landing
Comment 11 WebKit Review Bot 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>
Comment 12 WebKit Review Bot 2011-10-31 00:48:45 PDT
All reviewed patches have been landed.  Closing bug.