Refactor handles and weak pointers to become nicer and more automatic
Created attachment 82373 [details] Patch
Attachment 82373 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSClassRef.cpp',..." exit_code: 1 Source/WebKit2/WebProcess/Plugins/Netscape/NPJSObject.h:49: The parameter name "objectMap" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/WebProcess/Plugins/Netscape/NPJSObject.h:49: The parameter name "jsObject" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/WebProcess/Plugins/Netscape/NPJSObject.h:65: The parameter name "jsObject" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 3 in 89 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 82373 [details] did not build on win: Build output: http://queues.webkit.org/results/7905761
Created attachment 82385 [details] Patch
Only change is windows build fix.
Attachment 82385 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSClassRef.cpp',..." exit_code: 1 Source/WebKit2/WebProcess/Plugins/Netscape/NPJSObject.h:49: The parameter name "objectMap" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/WebProcess/Plugins/Netscape/NPJSObject.h:49: The parameter name "jsObject" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/WebProcess/Plugins/Netscape/NPJSObject.h:65: The parameter name "jsObject" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 3 in 89 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 82373 [details] did not build on qt: Build output: http://queues.webkit.org/results/7913656
Created attachment 82390 [details] Patch
Now fix qt
Attachment 82390 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSClassRef.cpp',..." exit_code: 1 Source/WebKit2/WebProcess/Plugins/Netscape/NPJSObject.h:49: The parameter name "objectMap" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/WebProcess/Plugins/Netscape/NPJSObject.h:49: The parameter name "jsObject" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit2/WebProcess/Plugins/Netscape/NPJSObject.h:65: The parameter name "jsObject" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 3 in 90 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 82390 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=82390&action=review > Source/JavaScriptCore/API/JSClassRef.cpp:150 > - protoDefinition.finalize = clearReferenceToPrototype; > + protoDefinition.finalize = 0; No need to initialize to 0 -- you can just remove this now. > Source/JavaScriptCore/collector/handles/Handle.h:77 > + // It is unsafe to use a handle after invalidating it. > + m_slot = 0; I don't think this comment is true anymore. You can certainly use a handle with a NULL m_slot. You just can't dereference it. > Source/JavaScriptCore/collector/handles/SentinelLinkedList.h:1 > +/* This file should go into WTF. > Source/JavaScriptCore/collector/handles/SinglyLinkedList.h:1 > +/* This one too. > Source/JavaScriptCore/runtime/WeakGCPtr.h:114 > +template <typename T> class WeakGCPtr : public WeakGCPtrBase<T, false> { I'm still not convinced that it's worth the complexity of having two kinds of weak pointer. I only see one client of WeakGCPtr vs LazyWeakGCPtr. Why not make all WeakGCPtr lazy?
Attachment 82390 [details] did not build on qt: Build output: http://queues.webkit.org/results/7909770
(In reply to comment #12) > Attachment 82390 [details] did not build on qt: > Build output: http://queues.webkit.org/results/7909770 I am working on fixing Qt build. Please don't commit the patch before I finish. Thanks in advance.
Created attachment 82429 [details] Qt buildfix I attached the Qt buildfix for https://bugs.webkit.org/attachment.cgi?id=82390 I didn't set r?, because it isn't buildable itself, but it works with the original patch.
(In reply to comment #12) > Attachment 82390 [details] did not build on qt: > Build output: http://queues.webkit.org/results/7909770 Unfortunately an incremental build problem hid the real problem. I filed a new bug report to fix the build system: https://bugs.webkit.org/show_bug.cgi?id=54466
(In reply to comment #15) > (In reply to comment #12) > > Attachment 82390 [details] [details] did not build on qt: > > Build output: http://queues.webkit.org/results/7909770 > > Unfortunately an incremental build problem hid the real problem. > I filed a new bug report to fix the build system: https://bugs.webkit.org/show_bug.cgi?id=54466 Thanks! I had been blocked waiting for the project file fixes to get through the ews bot so hadn't seen those failures, thanks for getting the qt_runtime fixes done as i would have had a lot of difficulty with them :D
Created attachment 82513 [details] Patch
Don't review this, i'm just running it by the EWS bots as i had to make a bunch of project file changes.
I think it may be possible to remove the complexity of HandleBase+HandleConverter, and just give all handles get(), ->(), and *(). If all handles have get(), ->(), and *(), it becomes syntactically possible to get(), ->(), or *() an Unknown, which is a little unfortunate, but you can't actually do anything dangerous afterwards, since Unknown provides no API. No functions accept Unknowns as arguments, and Unknown has no data members or functions. So, this is a compiler error since you can't assign Unknown to JSObject: JSObject* object = handle.get(); This is a compiler error since Unknown doesn't have getOwnPropertySlot: JSValue v = handle->getOwnPropertySlot(); etc.
Attachment 82513 [details] did not build on qt: Build output: http://queues.webkit.org/results/7918058
Comment on attachment 82513 [details] Patch Already reviewed by gavin and geoff, was just waiting for build outpuit. 90% sure everything is good in what i've landed. Committed r78634
http://trac.webkit.org/changeset/78634 might have broken EFL Linux Release (Build)