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 tree / graph
 
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
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

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.