Bug 54415 - Refactor handles and weak pointers to become nicer and more automatic
Summary: Refactor handles and weak pointers to become nicer and more automatic
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Oliver Hunt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-14 15:06 PST by Oliver Hunt
Modified: 2011-02-15 15:58 PST (History)
8 users (show)

See Also:


Attachments
Patch (189.63 KB, patch)
2011-02-14 15:26 PST, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (189.64 KB, patch)
2011-02-14 16:41 PST, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (190.13 KB, patch)
2011-02-14 17:04 PST, Oliver Hunt
no flags Details | Formatted Diff | Diff
Qt buildfix (5.38 KB, patch)
2011-02-15 01:47 PST, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (195.72 KB, patch)
2011-02-15 13:22 PST, Oliver Hunt
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2011-02-14 15:06:34 PST
Refactor handles and weak pointers to become nicer and more automatic
Comment 1 Oliver Hunt 2011-02-14 15:26:36 PST
Created attachment 82373 [details]
Patch
Comment 2 WebKit Review Bot 2011-02-14 15:29:39 PST
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.
Comment 3 Build Bot 2011-02-14 16:36:30 PST
Attachment 82373 [details] did not build on win:
Build output: http://queues.webkit.org/results/7905761
Comment 4 Oliver Hunt 2011-02-14 16:41:28 PST
Created attachment 82385 [details]
Patch
Comment 5 Oliver Hunt 2011-02-14 16:41:45 PST
Only change is windows build fix.
Comment 6 WebKit Review Bot 2011-02-14 16:43:40 PST
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.
Comment 7 Early Warning System Bot 2011-02-14 16:51:33 PST
Attachment 82373 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7913656
Comment 8 Oliver Hunt 2011-02-14 17:04:43 PST
Created attachment 82390 [details]
Patch
Comment 9 Oliver Hunt 2011-02-14 17:05:04 PST
Now fix qt
Comment 10 WebKit Review Bot 2011-02-14 17:07:07 PST
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 11 Geoffrey Garen 2011-02-14 18:04:18 PST
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?
Comment 12 Early Warning System Bot 2011-02-14 19:11:39 PST
Attachment 82390 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7909770
Comment 13 Csaba Osztrogonác 2011-02-15 00:35:45 PST
(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.
Comment 14 Csaba Osztrogonác 2011-02-15 01:47:44 PST
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.
Comment 15 Csaba Osztrogonác 2011-02-15 09:10:41 PST
(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
Comment 16 Oliver Hunt 2011-02-15 10:17:10 PST
(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
Comment 17 Oliver Hunt 2011-02-15 13:22:55 PST
Created attachment 82513 [details]
Patch
Comment 18 Oliver Hunt 2011-02-15 13:23:59 PST
Don't review this, i'm just running it by the EWS bots as i had to make a bunch of project file changes.
Comment 19 Geoffrey Garen 2011-02-15 13:37:10 PST
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.
Comment 20 Early Warning System Bot 2011-02-15 15:31:08 PST
Attachment 82513 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7918058
Comment 21 Oliver Hunt 2011-02-15 15:55:25 PST
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
Comment 22 WebKit Review Bot 2011-02-15 15:58:10 PST
http://trac.webkit.org/changeset/78634 might have broken EFL Linux Release (Build)