Bug 127969

Summary: Push DOM attributes into the prototype chain
Product: WebKit Reporter: Oliver Hunt <oliver>
Component: New BugsAssignee: Oliver Hunt <oliver>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, buildbot, cdumez, cgarcia, commit-queue, eric.carlson, ggaren, glenn, jer.noble, jsbell, philipj, p.jacquemart, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
none
Patch
none
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
none
Patch
none
Patch
none
Patch
none
Patch ggaren: review+, ggaren: commit-queue+

Description Oliver Hunt 2014-01-30 17:07:57 PST
Push DOM attributes into the prototype chain
Comment 1 Oliver Hunt 2014-01-30 17:08:45 PST
Created attachment 222763 [details]
Patch

just the constructor attribute at the moment
Comment 2 Oliver Hunt 2014-01-30 17:44:09 PST
holy crap it's building on other platforms :-O
Comment 3 Geoffrey Garen 2014-01-30 18:36:20 PST
Comment on attachment 222763 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=222763&action=review

r=me

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:595
> +    my $hasNumericIndexedGetter = $indexedGetterFunction ? $codeGenerator->IsNumericType($indexedGetterFunction->signature->type) : 0;

This variable seems to be unused. Is that a mistake?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:621
> +    return 1;

This needs a comment to explain that it's temporary.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1266
> +

Stray newline.
Comment 4 Oliver Hunt 2014-01-30 19:26:34 PST
(In reply to comment #3)
> (From update of attachment 222763 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222763&action=review
> 
> r=me
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:595
> > +    my $hasNumericIndexedGetter = $indexedGetterFunction ? $codeGenerator->IsNumericType($indexedGetterFunction->signature->type) : 0;
> 
> This variable seems to be unused. Is that a mistake?
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:621
> > +    return 1;
> 
> This needs a comment to explain that it's temporary.

I was pushing the start of my work so there was a backup and to make sure it would actually build elsewhere.

Good to have initial work reviewed though, thanks! (The bulk of the rest is done, but i'm stepping through enabling pieces one at a time to find individual failures and regressions)

> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1266
> > +
> 
> Stray newline.
Comment 5 Oliver Hunt 2014-02-01 23:22:52 PST
Created attachment 222912 [details]
Patch
Comment 6 Build Bot 2014-02-02 01:52:38 PST
Comment on attachment 222912 [details]
Patch

Attachment 222912 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6687066310049792

New failing tests:
js/dom/dfg-custom-getter-throw-inlined.html
svg/hixie/dynamic/005.xml
inspector-protocol/debugger/setPauseOnExceptions-uncaught.html
js/dom/dom-static-property-for-in-iteration.html
inspector-protocol/debugger/setPauseOnExceptions-none.html
svg/custom/class-baseValue.svg
svg/W3C-SVG-1.1-SE/types-dom-04-b.svg
svg/W3C-SVG-1.1-SE/svgdom-over-01-f.svg
inspector-protocol/debugger/setPauseOnExceptions-all.html
fast/dom/SelectorAPI/resig-SelectorsAPI-test.xhtml
fast/dom/wrapper-classes.html
js/dom/dfg-custom-getter-throw.html
Comment 7 Build Bot 2014-02-02 01:52:40 PST
Created attachment 222919 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 8 Build Bot 2014-02-02 02:15:26 PST
Comment on attachment 222912 [details]
Patch

Attachment 222912 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5797458323963904

New failing tests:
js/dom/dfg-custom-getter-throw-inlined.html
svg/hixie/dynamic/005.xml
inspector-protocol/debugger/setPauseOnExceptions-uncaught.html
js/dom/dom-static-property-for-in-iteration.html
inspector-protocol/debugger/setPauseOnExceptions-none.html
svg/custom/class-baseValue.svg
svg/W3C-SVG-1.1-SE/types-dom-04-b.svg
http/tests/security/cross-frame-access-enumeration.html
svg/W3C-SVG-1.1-SE/svgdom-over-01-f.svg
inspector-protocol/debugger/setPauseOnExceptions-all.html
fast/dom/SelectorAPI/resig-SelectorsAPI-test.xhtml
fast/dom/wrapper-classes.html
js/dom/dfg-custom-getter-throw.html
Comment 9 Build Bot 2014-02-02 02:15:31 PST
Created attachment 222921 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 10 Build Bot 2014-02-02 02:59:25 PST
Comment on attachment 222912 [details]
Patch

Attachment 222912 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6074071397695488

New failing tests:
js/dom/dfg-custom-getter-throw-inlined.html
svg/hixie/dynamic/005.xml
inspector-protocol/debugger/setPauseOnExceptions-uncaught.html
js/dom/dom-static-property-for-in-iteration.html
inspector-protocol/debugger/setPauseOnExceptions-none.html
svg/custom/class-baseValue.svg
svg/W3C-SVG-1.1-SE/types-dom-04-b.svg
http/tests/security/cross-frame-access-enumeration.html
svg/W3C-SVG-1.1-SE/svgdom-over-01-f.svg
inspector-protocol/debugger/setPauseOnExceptions-all.html
fast/dom/SelectorAPI/resig-SelectorsAPI-test.xhtml
fast/dom/wrapper-classes.html
js/dom/dfg-custom-getter-throw.html
Comment 11 Build Bot 2014-02-02 02:59:27 PST
Created attachment 222922 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 12 Build Bot 2014-02-02 03:06:01 PST
Comment on attachment 222912 [details]
Patch

Attachment 222912 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6403426669821952

New failing tests:
js/dom/dfg-custom-getter-throw-inlined.html
svg/hixie/dynamic/005.xml
inspector-protocol/debugger/setPauseOnExceptions-uncaught.html
js/dom/dom-static-property-for-in-iteration.html
inspector-protocol/debugger/setPauseOnExceptions-none.html
svg/custom/class-baseValue.svg
svg/W3C-SVG-1.1-SE/types-dom-04-b.svg
svg/W3C-SVG-1.1-SE/svgdom-over-01-f.svg
inspector-protocol/debugger/setPauseOnExceptions-all.html
fast/dom/SelectorAPI/resig-SelectorsAPI-test.xhtml
fast/dom/wrapper-classes.html
js/dom/dfg-custom-getter-throw.html
Comment 13 Build Bot 2014-02-02 03:06:05 PST
Created attachment 222923 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 14 Oliver Hunt 2014-02-02 14:05:41 PST
Created attachment 222939 [details]
Patch
Comment 15 Build Bot 2014-02-02 15:11:24 PST
Comment on attachment 222939 [details]
Patch

Attachment 222939 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6366197893300224

New failing tests:
js/dom/dfg-custom-getter-throw-inlined.html
js/dom/dfg-custom-getter-throw.html
Comment 16 Build Bot 2014-02-02 15:11:27 PST
Created attachment 222940 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 17 Build Bot 2014-02-02 15:45:42 PST
Comment on attachment 222939 [details]
Patch

Attachment 222939 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4584905304440832

New failing tests:
http/tests/security/cross-frame-access-enumeration.html
js/dom/dfg-custom-getter-throw-inlined.html
js/dom/dfg-custom-getter-throw.html
Comment 18 Build Bot 2014-02-02 15:45:45 PST
Created attachment 222942 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 19 Oliver Hunt 2014-02-02 16:20:37 PST
Created attachment 222945 [details]
Patch
Comment 20 Oliver Hunt 2014-02-02 17:26:00 PST
Created attachment 222948 [details]
Patch
Comment 21 Mark Lam 2014-02-02 18:17:14 PST
Comment on attachment 222948 [details]
Patch

r=me
Comment 22 Oliver Hunt 2014-02-02 18:20:36 PST
Committed r163280: <http://trac.webkit.org/changeset/163280>
Comment 23 Geoffrey Garen 2014-02-04 15:37:32 PST
Reopening to attach new patch.
Comment 24 Geoffrey Garen 2014-02-04 15:37:35 PST
Created attachment 223171 [details]
Patch
Comment 25 Geoffrey Garen 2014-02-04 15:38:04 PST
Comment on attachment 223171 [details]
Patch

Roll-out patch.
Comment 26 WebKit Commit Bot 2014-02-04 15:39:21 PST
Attachment 223171 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSStorageCustom.cpp:52:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 27 Geoffrey Garen 2014-02-04 15:39:27 PST
Created attachment 223172 [details]
Patch
Comment 28 WebKit Commit Bot 2014-02-04 15:42:17 PST
Attachment 223172 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSStorageCustom.cpp:52:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 29 Geoffrey Garen 2014-02-04 15:52:30 PST
Committed r163413: <http://trac.webkit.org/changeset/163413>
Comment 30 Oliver Hunt 2014-02-06 13:57:25 PST
Committed r163562: <http://trac.webkit.org/changeset/163562>
Comment 33 Oliver Hunt 2014-02-26 10:32:06 PST
(In reply to comment #32)
> This patch also appears to have regressed Chromium's dom_perf benchmark by 30-40%:
> 
> https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-mavericks%22%2C%22DOM%2FModifyAttribute%3ATime%22%5D%2C%5B%22mac-mountainlion%22%2C%22DOM%2FModifyAttribute%3ATime%22%5D%5D&days=365&zoom=%5B1389742499618.0234%2C1392139963925.9648%5D

Wow, that is vastly more concerning to me than the dromaeo regression :( Can we get a separate bug for that?