Bug 113473 - Rename toScriptElement to differentiate it from other to* functions and better describe its function
Summary: Rename toScriptElement to differentiate it from other to* functions and bette...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philip Rogers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-27 20:28 PDT by Philip Rogers
Modified: 2013-03-27 22:03 PDT (History)
8 users (show)

See Also:


Attachments
Patch that just handles the rename (10.93 KB, patch)
2013-03-27 21:37 PDT, Philip Rogers
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philip Rogers 2013-03-27 20:28:03 PDT
ScriptElement::toScriptElement is different from other to* functions (e.g., Element::toElement, etc) in that it returns 0 if the cast is unsuccessful. These functions have security implications and need to be clear and consistent.
http://trac.webkit.org/browser/trunk/Source/WebCore/dom/ScriptElement.cpp#L417

We should rename toScriptElement to toScriptElementIfPossible (or similar) both to clarify the function behavior and to differentiate it from other to* functions.
Comment 1 Geoffrey Garen 2013-03-27 21:02:48 PDT
In JSC, we use jsCast<T> and jsDynamicCast<T>, to mimic static_cast<T> and dynamic_cast<T>.

Ryosuke and I were discussing using domCast<T> or coreCast<T>, along with domDynamicCast<T> or coreDynamicCast<T> for the DOM. This would allow for specialized implementations while keeping the interface consistent at all call sites.
Comment 2 Philip Rogers 2013-03-27 21:27:38 PDT
(In reply to comment #1)
> In JSC, we use jsCast<T> and jsDynamicCast<T>, to mimic static_cast<T> and dynamic_cast<T>.
> 
> Ryosuke and I were discussing using domCast<T> or coreCast<T>, along with domDynamicCast<T> or coreDynamicCast<T> for the DOM. This would allow for specialized implementations while keeping the interface consistent at all call sites.

I think Abhishek is working on something similar to domCast<T> to help reduce the security issues involved in our many static_casts. My understanding is that we will only return 0 for null casts, so the toScriptElement function is really an odd one out.

I was not aware of jsDynamicCast before, but I like it. Abhishek, is there a bug that's tracking this work?
Comment 3 Philip Rogers 2013-03-27 21:37:51 PDT
Created attachment 195478 [details]
Patch that just handles the rename
Comment 4 WebKit Review Bot 2013-03-27 22:02:57 PDT
Comment on attachment 195478 [details]
Patch that just handles the rename

Clearing flags on attachment: 195478

Committed r147057: <http://trac.webkit.org/changeset/147057>
Comment 5 WebKit Review Bot 2013-03-27 22:03:02 PDT
All reviewed patches have been landed.  Closing bug.