| Summary: | Static table lookup should not require getOwnPropertySlot override. | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Gavin Barraclough <barraclough> | ||||||||||||||||||||||||||||||
| Component: | JavaScriptCore | Assignee: | Gavin Barraclough <barraclough> | ||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | buildbot, commit-queue, darin, ggaren, keith_miller, mark.lam, msaboff, rniwa, saam | ||||||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||
|
Description
Gavin Barraclough
2016-05-24 23:10:06 PDT
Created attachment 279739 [details]
untested WIP
Created attachment 279740 [details]
untested WIP
Comment on attachment 279740 [details] untested WIP Attachment 279740 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1378791 Number of test failures exceeded the failure limit. Created attachment 279742 [details]
Archive of layout-test-results from ews102 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 279740 [details] untested WIP Attachment 279740 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1378785 Number of test failures exceeded the failure limit. Created attachment 279743 [details]
Archive of layout-test-results from ews117 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 279740 [details] untested WIP Attachment 279740 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1378794 Number of test failures exceeded the failure limit. Created attachment 279744 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 279740 [details] untested WIP Attachment 279740 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1378787 Number of test failures exceeded the failure limit. Created attachment 279745 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4
Created attachment 279794 [details]
Fix
Created attachment 279800 [details]
Missed One!
Comment on attachment 279800 [details] Missed One! Attachment 279800 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1382047 New failing tests: http/tests/security/cross-origin-window-property-access.html fast/dom/beforeload/frame-before-load.html js/dom/getOwnPropertyDescriptor.html fast/dom/beforeload/video-before-load.html webarchive/test-link-rel-subresource-beforeload.html imported/w3c/web-platform-tests/html/dom/dynamic-markup-insertion/opening-the-input-stream/document.open-02.html fast/encoding/parser-tests-50.html fast/dom/unforgeable-attributes.html fast/dom/Window/forbid-showModalDialog.html fast/dom/beforeload/script-before-load.html fast/dom/beforeload/image-before-load.html Created attachment 279813 [details]
Archive of layout-test-results from ews103 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 279800 [details] Missed One! Attachment 279800 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1382042 New failing tests: http/tests/security/cross-origin-window-property-access.html fast/dom/beforeload/frame-before-load.html js/dom/getOwnPropertyDescriptor.html fast/dom/beforeload/video-before-load.html webarchive/test-link-rel-subresource-beforeload.html imported/w3c/web-platform-tests/html/dom/dynamic-markup-insertion/opening-the-input-stream/document.open-02.html fast/encoding/parser-tests-50.html fast/dom/unforgeable-attributes.html fast/dom/Window/forbid-showModalDialog.html fast/dom/beforeload/script-before-load.html fast/dom/beforeload/image-before-load.html Created attachment 279815 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 279800 [details] Missed One! Attachment 279800 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1382035 New failing tests: http/tests/security/cross-origin-window-property-access.html fast/dom/beforeload/frame-before-load.html js/dom/getOwnPropertyDescriptor.html fast/dom/beforeload/video-before-load.html webarchive/test-link-rel-subresource-beforeload.html fast/replaced/table-percent-height.html imported/w3c/web-platform-tests/html/dom/dynamic-markup-insertion/opening-the-input-stream/document.open-02.html fast/encoding/parser-tests-50.html fast/dom/unforgeable-attributes.html fast/dom/Window/forbid-showModalDialog.html fast/dom/beforeload/script-before-load.html fast/dom/beforeload/image-before-load.html Created attachment 279816 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4
Comment on attachment 279800 [details] Missed One! Attachment 279800 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1382041 New failing tests: http/tests/security/cross-origin-window-property-access.html fast/dom/beforeload/frame-before-load.html js/dom/getOwnPropertyDescriptor.html fast/dom/beforeload/video-before-load.html webarchive/test-link-rel-subresource-beforeload.html imported/w3c/web-platform-tests/html/dom/dynamic-markup-insertion/opening-the-input-stream/document.open-02.html fast/encoding/parser-tests-50.html fast/dom/unforgeable-attributes.html fast/dom/Window/forbid-showModalDialog.html fast/dom/beforeload/script-before-load.html fast/dom/beforeload/image-before-load.html Created attachment 279821 [details]
Archive of layout-test-results from ews116 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 279825 [details]
Skip GlobalObject
Created attachment 279869 [details]
Fix
Comment on attachment 279869 [details] Fix View in context: https://bugs.webkit.org/attachment.cgi?id=279869&action=review Looks good to me. I don’t see any obvious downside. > Source/JavaScriptCore/runtime/ArrayConstructor.h:37 > + static const unsigned StructureFlags = HasStaticPropertyTable | InternalFunction::StructureFlags; Some stray thoughts around the edges: - Should we be using constexpr for these instead of const? - Should we get more consistent about whether we put the new flags before or after the old ones in the "|" expression? I prefer putting the new flags after. - Should we get more consistent about using Base rather than repeating the base class name? This one repeats the name, but most use Base. - For Base, should we switch from typedef to using? I like the using style better, even outside of its best use for template types. > Source/JavaScriptCore/runtime/JSObject.cpp:1679 > + for (const ClassInfo* info = classInfo(); info; info = info->parentClass) { This is the kind of code where I think auto* wins out over explicitly naming a class. > Source/JavaScriptCore/runtime/JSObject.cpp:1680 > + if (const HashTable* table = info->staticPropHashTable) { Here too. > Source/JavaScriptCore/runtime/Lookup.h:211 > +inline bool getStaticPropertySlotFromTable(VM& vm, const HashTable& table, JSObject* thisObj, PropertyName propertyName, PropertySlot& slot) Could we spell out thisObject instead of using thisObj? Fixed all review comments.
> Some stray thoughts around the edges:
>
> - Should we be using constexpr for these instead of const?
> - Should we get more consistent about whether we put the new flags before or
> after the old ones in the "|" expression? I prefer putting the new flags
> after.
> - Should we get more consistent about using Base rather than repeating the
> base class name? This one repeats the name, but most use Base.
> - For Base, should we switch from typedef to using? I like the using style
> better, even outside of its best use for template types.
Re constexpr & using – yes, we probably should.
It seems pretty unfortunate that we have to have these flag declarations at all. The HasStaticPropertyTable flag should be set if and only if the ClassInfo contains a static table, OverridesGetOwnPropertySlot should be set if the class has an implementation of GetOwnPropertySlot differing to JSObject, etc. We'd need a way to specify some custom flags, but I think most classes could stop needing to declare structureFlags at all, with them being generated by a common templates helper function. I haven't worked this idea through yet. I'm not sure that this can be determined as a constexpr, and it might be error prone if any class failed to declare dependencies correctly (i.e. if a helper template relied on Base, and a class failed to define this, you might have a subtle error - as it could pick up the parent's Base).
If computing of these flags could be performed by a common helper, then order & usage of Base would necessarily be consistent.
Committed revision 201448. (In reply to comment #24) > The HasStaticPropertyTable flag should be set if and only if the > ClassInfo contains a static table, OverridesGetOwnPropertySlot should be set > if the class has an implementation of GetOwnPropertySlot differing to > JSObject, etc. Good point. We live in a magical era where template meta-programming can accomplish wonders! |