This is the third installment of the ongoing effort to phase out the use of constructors for GC-managed objects in favor of static create methods. We want to avoid calling operator new any more, so the best way to do that is to remove it. Actually, the best way is to assert that we never use it. If we fully remove the calls and somebody uses them or if a call was missed during the refactoring process we can get strange behavior in the browser that is difficult to track down to the source. By leaving an assert in, it is easy to determine exactly where in the code the offending call to new took place.
Created attachment 102156 [details] Patch
Comment on attachment 102156 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102156&action=review > Source/JavaScriptCore/debugger/DebuggerActivation.h:37 > + static DebuggerActivation* create(JSGlobalData& globalData, JSObject* obj) We use words like “object”, not abbreviations like “obj”. > Source/JavaScriptCore/runtime/JSGlobalObject.h:130 > + static JS_EXPORTDATA const ClassInfo s_info; Why are we making this public?
> > Source/JavaScriptCore/runtime/JSGlobalObject.h:130 > > + static JS_EXPORTDATA const ClassInfo s_info; > > Why are we making this public? It was public before. The way the diff worked out made it look like I changed it.
Comment on attachment 102156 [details] Patch Attachment 102156 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9260004
Should I put a comment on the operator new declarations in JSCell saying that there are no concrete implementations to prevent successful linking? This would prevent somebody going and looking for them in the future and wasting a lot of time.
(In reply to comment #5) > Should I put a comment on the operator new declarations in JSCell saying that there are no concrete implementations to prevent successful linking? This would prevent somebody going and looking for them in the future and wasting a lot of time. Yeah, that probably makes sense. Also look into why the Qt build is failing (_sigh_)
> Yeah, that probably makes sense. Will do. > Also look into why the Qt build is failing (_sigh_) I didn't actually test any of the builds on the other platforms yet. I didn't expect the patch to clear the build bots and the commit queue in its current state. Removing operator new from JSCell affects pretty much every platform, so I'm using the bots to catch the build errors more or less simultaneously.
Created attachment 102172 [details] Patch
Comment on attachment 102172 [details] Patch Attachment 102172 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9247893
EWS says Qt fails to build.
Created attachment 102175 [details] Patch
Comment on attachment 102175 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102175&action=review > Source/JavaScriptCore/runtime/JSCell.h:127 > + // Note that the first two declarations of operator new have no corresponding implementation and > + // will cause link errors if you use them. > void* operator new(size_t, ExecState*); > void* operator new(size_t, JSGlobalData*); Can these be made private so we are more likely to get compilation errors rather than linking errors?
Created attachment 102193 [details] Patch
Comment on attachment 102193 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102193&action=review noticed a couple of additional abbreviations, correct those and this will be finished :D > Source/WebCore/bridge/c/CRuntimeObject.h:40 > + static CRuntimeObject* create(ExecState* exec, JSGlobalObject* globalObject, PassRefPtr<CInstance> cinst) normal name for 'cinst' would be instance > Source/WebCore/bridge/objc/objc_runtime.h:95 > + static ObjcFallbackObjectImp* create(ExecState* exec, JSGlobalObject* globalObject, ObjcInstance* inst, const Identifier& propertyName) s/inst/instance > Source/WebKit/mac/Plugins/Hosted/ProxyRuntimeObject.h:39 > + static ProxyRuntimeObject* create(JSC::ExecState* exec, JSC::JSGlobalObject* globalObject, PassRefPtr<ProxyInstance> inst) s/inst/instance
Created attachment 102196 [details] Patch
Comment on attachment 102196 [details] Patch Clearing flags on attachment: 102196 Committed r91903: <http://trac.webkit.org/changeset/91903>
All reviewed patches have been landed. Closing bug.