Summary: | JSNodeCustom should call document() instead of scriptExecutionContext() | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||
Component: | WebCore JavaScript | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | darin | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Adam Barth
2009-08-02 02:04:12 PDT
The key issue here is that we want to call an inline function, not a virtual function. The inline function could be document() or it could be an inline version of scriptExecutionContext(). See the Node::prefix() function for an example of this design pattern. Created attachment 33947 [details]
Patch v1
Comment on attachment 33947 [details]
Patch v1
On reflection, I don't think this patch is right.
Document is derived from ScriptExecutionContext, so there's no need to overload toJSDOMGlobalObject to take a Document*. The JSNodeCustom.cpp change can stand alone. No need to change JSDOMGLobalObject.h and .cpp.
I'm changing my review to review-.
> No need to change JSDOMGLobalObject.h and .cpp.
True. I guess all you're saving is the fake RTTI branch. I'll spin up a new version of the patch tomorrow. I have the Node* change in my try at the moment, so it will take a bit of time to rebuild.
(In reply to comment #4) > > No need to change JSDOMGLobalObject.h and .cpp. > > True. I guess all you're saving is the fake RTTI branch. I'll spin up a new > version of the patch tomorrow. I have the Node* change in my try at the > moment, so it will take a bit of time to rebuild. I must be really tired! I think it *is* worthwhile to save the fake RTTI branch. Sorry for going back and forth on this so much. Comment on attachment 33947 [details]
Patch v1
I changed my mind again and I think this patch is good as-is.
Comment on attachment 33947 [details] Patch v1 Clearing review flag on attachment: 33947 Committing to http://svn.webkit.org/repository/webkit/trunk ... M WebCore/ChangeLog M WebCore/bindings/js/JSDOMGlobalObject.cpp M WebCore/bindings/js/JSDOMGlobalObject.h M WebCore/bindings/js/JSNodeCustom.cpp Committed r46710 M WebCore/ChangeLog M WebCore/bindings/js/JSDOMGlobalObject.h M WebCore/bindings/js/JSDOMGlobalObject.cpp M WebCore/bindings/js/JSNodeCustom.cpp r46710 = 8e29f864509f257cdb9711b14c33ff706cb3e96b (trunk) No changes between current HEAD and refs/remotes/trunk Resetting to the latest refs/remotes/trunk http://trac.webkit.org/changeset/46710 All reviewed patches have been landed. Closing bug. |