Bug 41477 - Direct calls to cast operator in WebFrameImpl.cpp
Summary: Direct calls to cast operator in WebFrameImpl.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: David Holloway
URL:
Keywords:
Depends on: 41499
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-01 11:57 PDT by David Holloway
Modified: 2010-07-08 01:49 PDT (History)
7 users (show)

See Also:


Attachments
Proposed patch. (2.60 KB, patch)
2010-07-01 12:34 PDT, David Holloway
no flags Details | Formatted Diff | Diff
Proposed patch. (2.61 KB, patch)
2010-07-01 13:09 PDT, David Holloway
no flags Details | Formatted Diff | Diff
Proposed patch. Fixes const-ness issues. (4.01 KB, patch)
2010-07-07 15:46 PDT, David Holloway
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Holloway 2010-07-01 11:57:56 PDT
Overview: Poor C++ style with direct calls to cast operator in WebFrameImpl.cpp.

Steps to Reproduce:

1) Open WebFrameImpl.cpp

2) Inspect WebKit::WebFrameImpl::registerPasswordListener and WebKit::WebFrameImpl::notifiyPasswordListenerOfAutocomplete

Actual Results: Notice the calls to:

   RefPtr<HTMLInputElement> element = inputElement.operator PassRefPtr<HTMLInputElement>();

   This is using an explicit call to a PassRefPtr cast operator to bridge from a WebInputElement back to an HTMLInputElement.

Expected Results:  Better style is to "unwrap" the WebInputElement explicitly, eg.:

   RefPtr<HTMLInputElement> element(inputElement.unwrap<HTMLInputElement>());
Comment 1 David Holloway 2010-07-01 12:31:43 PDT
Follow up for bug 41236.
Comment 2 David Holloway 2010-07-01 12:34:15 PDT
Created attachment 60272 [details]
Proposed patch.
Comment 3 David Holloway 2010-07-01 13:09:17 PDT
Created attachment 60277 [details]
Proposed patch.
Comment 4 Darin Fisher (:fishd, Google) 2010-07-01 13:18:13 PDT
Comment on attachment 60277 [details]
Proposed patch.

Thanks!  R=me
Comment 5 WebKit Commit Bot 2010-07-01 19:50:48 PDT
Comment on attachment 60277 [details]
Proposed patch.

Clearing flags on attachment: 60277

Committed r62349: <http://trac.webkit.org/changeset/62349>
Comment 6 WebKit Commit Bot 2010-07-01 19:50:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 WebKit Review Bot 2010-07-01 20:00:37 PDT
http://trac.webkit.org/changeset/62349 might have broken Chromium Linux Release
Comment 8 David Levin 2010-07-01 20:15:11 PDT
Rolled out due to breakage on Linux.

http://trac.webkit.org/changeset/62351
Comment 9 David Levin 2010-07-01 20:17:45 PDT
And OSX bustage. Here's the compile error:


/Users/cltbld/Desktop/BuildSlaveData/WebKit-BuildSlave/chromium-mac-release/build/WebKit/chromium/src/WebFrameImpl.cpp: In member function ‘virtual void WebKit::WebFrameImpl::notifiyPasswordListenerOfAutocomplete(const WebKit::WebInputElement&)’:
/Users/cltbld/Desktop/BuildSlaveData/WebKit-BuildSlave/chromium-mac-release/build/WebKit/chromium/src/WebFrameImpl.cpp:1974: error: passing ‘const WebKit::WebInputElement’ as ‘this’ argument of ‘T* WebKit::WebNode::unwrap() [with T = WebCore::HTMLInputElement]’ discards qualifiers
Comment 10 David Holloway 2010-07-07 15:46:47 PDT
Created attachment 60792 [details]
Proposed patch.  Fixes const-ness issues.

Grrrr.  False positive my end.  New patch addresses const-ness issues and pushes the const-ness of the |inputElement| parameter down a level to WebFrameImpl::getPasswordListener().  We need a const_cast at this level due to the use of non-const RefPtr<HTMLInputElement> used as the key to the |m_passwordListeners| map.
Comment 11 WebKit Commit Bot 2010-07-08 01:49:15 PDT
Comment on attachment 60792 [details]
Proposed patch.  Fixes const-ness issues.

Clearing flags on attachment: 60792

Committed r62770: <http://trac.webkit.org/changeset/62770>
Comment 12 WebKit Commit Bot 2010-07-08 01:49:20 PDT
All reviewed patches have been landed.  Closing bug.