Bug 72855

Created attachment 116040 [details] Patch

This is just a preliminary result. What should land would be generated from the ToT at that timing.

Created attachment 116064 [details] Patch

Comment on attachment 116064 [details] Patch Uploaded to the wrong bug...

Created attachment 116700 [details] It's ready to review

Attachment 116700 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/OpaqueJSString.h..." exit_code: 1 Last 3072 characters of output: [readability/parameter_name] [5] Source/JavaScriptCore/runtime/JSFunction.h:57: The parameter name "nativeFunction" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/wtf/FastMalloc.h:32: Code inside a namespace should not be indented. [whitespace/indent] [4] Source/JavaScriptCore/runtime/PropertyDescriptor.h:50: The parameter name "value" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/wtf/MainThread.h:49: The parameter name "paused" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/JSWrapperObject.h:31: Code inside a namespace should not be indented. [whitespace/indent] [4] Source/JavaScriptCore/runtime/JSLock.h:96: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/wtf/text/StringImpl.h:279: The parameter name "buffer" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/JSGlobalData.h:301: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/wtf/ByteArray.h:86: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/InitializeThreading.h:36: Code inside a namespace should not be indented. [whitespace/indent] [4] Source/JavaScriptCore/wtf/ThreadingPrimitives.h:136: The parameter name "mutex" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/bytecode/SamplingTool.h:223: Missing spaces around = [whitespace/operators] [4] Source/JavaScriptCore/wtf/RandomNumber.h:33: Code inside a namespace should not be indented. [whitespace/indent] [4] Source/JavaScriptCore/runtime/JSCell.h:79: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/JSCell.h:80: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/JSCell.h:81: Use 0 or null instead of NULL (even in *comments*). [readability/null] [4] Source/JavaScriptCore/wtf/text/AtomicString.h:57: The parameter name "s" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/JSObject.h:78: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/JSObject.h:78: The parameter name "entry" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/JSObject.h:78: The parameter name "slot" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/StringObject.h:29: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 25 in 90 files If any of these errors are false positives, please file a bug against check-webkit-style.

Blocking bugs being removed, now this is ready for the review for applying the rewrite! You can validate the build by applying the the preliminary patch on Bug 72854, which disables export list. Note that - The rewrite is based on svn r101220. - Reported style error is false positive. touched line already contained such errors.

Comment on attachment 116700 [details] It's ready to review Attachment 116700 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10639428

Comment on attachment 116700 [details] It's ready to review Attachment 116700 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10643406

Oops. I need to change WebCore/config.h... Filed at Bug 73191.

The fact that we have to switch from inline to JS_INLINE in such a large number of places makes this super ugly. I can also see people being confused as to when JS_INLINE should be used rather than just the regular inline.

(In reply to comment #11) > The fact that we have to switch from inline to JS_INLINE in such a large number of places makes this super ugly. I can also see people being confused as to when JS_INLINE should be used rather than just the regular inline. Basically I agree with you that forcing JS_INLINE is ugly. Fortunately, JS_INLINE will be used in limited places: We need it only for inline method whose class has exported constructors and a vtable. Also, that means we can reduce the number of JS_INLINE by making create() methods non-inline. I can help such inline to non-inline conversion if you are OK. We need JS_INLINE (explicit "hidden" visibility annotation) because there is no way to make vtables visible using source annotation except marking classes as visible. I think this is a reasonable compromise compared to using export lists. But I'd love to hear your suggestion. Yet another idea is to #define inline ...

Comment on attachment 116700 [details] It's ready to review Attachment 116700 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10678739

(In reply to comment #12) > (In reply to comment #11) > > The fact that we have to switch from inline to JS_INLINE in such a large number of places makes this super ugly. I can also see people being confused as to when JS_INLINE should be used rather than just the regular inline. > > Basically I agree with you that forcing JS_INLINE is ugly. > > Fortunately, JS_INLINE will be used in limited places: > We need it only for inline method whose class has exported constructors and a vtable. > > Also, that means we can reduce the number of JS_INLINE by making create() methods non-inline. > I can help such inline to non-inline conversion if you are OK. > > We need JS_INLINE (explicit "hidden" visibility annotation) because > there is no way to make vtables visible using source annotation except marking classes as visible. > > I think this is a reasonable compromise compared to using export lists. > But I'd love to hear your suggestion. > > Yet another idea is to #define inline ... Does the build break if I get it wrong? (I would consider this a nice feature.) In order words, how can we help people to get this right? (Both when to include it and when not to include it.) PS I love the idea!

(In reply to comment #14) > (In reply to comment #12) > > (In reply to comment #11) > > > The fact that we have to switch from inline to JS_INLINE in such a large number of places makes this super ugly. I can also see people being confused as to when JS_INLINE should be used rather than just the regular inline. > > > > Basically I agree with you that forcing JS_INLINE is ugly. > > > > Fortunately, JS_INLINE will be used in limited places: > > We need it only for inline method whose class has exported constructors and a vtable. > > > > Also, that means we can reduce the number of JS_INLINE by making create() methods non-inline. > > I can help such inline to non-inline conversion if you are OK. > > > > We need JS_INLINE (explicit "hidden" visibility annotation) because > > there is no way to make vtables visible using source annotation except marking classes as visible. > > > > I think this is a reasonable compromise compared to using export lists. > > But I'd love to hear your suggestion. > > > > Yet another idea is to #define inline ... > > Does the build break if I get it wrong? (I would consider this a nice feature.) > > In order words, how can we help people to get this right? (Both when to include it and when not to include it.) > Oops. Sorry for the slow response... For Mac the build will simply fail because of the checker script. For Chromium, maybe we can improve our clang plugin to detect this. For other port, I have no idea. I guess it's possible to write a checker script which is similar to Mac's, but I'm not sure. Fortunately the EWS has chromium bot so it would help other port developers. > PS I love the idea! Thanks!

Comment on attachment 116700 [details] It's ready to review clearing r? until the blocking bugs are fixed.

Created attachment 121986 [details] Patch

Attachment 121986 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/OpaqueJSString.h..." exit_code: 1 Last 3072 characters of output: ty/parameter_name] [5] Source/JavaScriptCore/runtime/Identifier.h:123: The parameter name "r" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/JSFunction.h:54: The parameter name "nativeFunction" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/wtf/FastMalloc.h:32: Code inside a namespace should not be indented. [whitespace/indent] [4] Source/JavaScriptCore/runtime/PropertyDescriptor.h:55: The parameter name "value" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/wtf/MainThread.h:50: Extra space before ( in function call [whitespace/parens] [4] Source/JavaScriptCore/wtf/MainThread.h:52: The parameter name "paused" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/JSLock.h:96: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/wtf/text/StringImpl.h:278: The parameter name "buffer" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/JSGlobalData.h:315: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/wtf/ByteArray.h:86: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/InitializeThreading.h:36: Code inside a namespace should not be indented. [whitespace/indent] [4] Source/JavaScriptCore/wtf/ThreadingPrimitives.h:124: The parameter name "mutex" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/bytecode/SamplingTool.h:223: Missing spaces around = [whitespace/operators] [4] Source/JavaScriptCore/wtf/RandomNumber.h:33: Code inside a namespace should not be indented. [whitespace/indent] [4] Source/JavaScriptCore/runtime/JSCell.h:85: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/JSCell.h:86: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/JSCell.h:87: Use 0 or null instead of NULL (even in *comments*). [readability/null] [4] Source/JavaScriptCore/wtf/text/AtomicString.h:57: The parameter name "s" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/JSObject.h:78: The parameter name "exec" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/JSObject.h:78: The parameter name "entry" adds no information, so it should be removed. [readability/parameter_name] [5] Source/JavaScriptCore/runtime/JSObject.h:78: The parameter name "slot" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 24 in 89 files If any of these errors are false positives, please file a bug against check-webkit-style.

Finally, the patch is ready! I confirmed that this can be buit even without exp file on SL (The exp list removal patch is on Bug 72854) Thanks for the clean API design of JSC, we don't need any extra INLINE macros. After landing this change, I'll attack both exp file (on Mac) and def file (on Windows). Note that style red is false positive. That has been there before this change.

Comment on attachment 121986 [details] Patch Attachment 121986 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11207108

Darin, thanks you for the review! I'll fix the chromium bot problem (it should be done by a separate patch) then land this.

Created attachment 122369 [details] Patch for landing

Comment on attachment 122369 [details] Patch for landing Rejecting attachment 122369 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: ring.h patching file Source/JavaScriptCore/wtf/text/StringBuilder.h patching file Source/JavaScriptCore/wtf/text/StringImpl.h patching file Source/JavaScriptCore/wtf/text/WTFString.h patching file Source/JavaScriptCore/wtf/unicode/Collator.h patching file Source/JavaScriptCore/wtf/unicode/UTF8.h patching file Source/JavaScriptCore/yarr/Yarr.h patching file Source/JavaScriptCore/yarr/YarrPattern.h Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/11142891

Committed r104900: <http://trac.webkit.org/changeset/104900>