It currently reimplements this.
Created attachment 271373 [details] Fix
Comment on attachment 271373 [details] Fix View in context: https://bugs.webkit.org/attachment.cgi?id=271373&action=review EWS reports a failure as well. > Source/JavaScriptCore/runtime/Lookup.h:219 > + const HashTableValue* entry = table.entry(propertyName); auto* > Source/JavaScriptCore/runtime/Lookup.h:249 > const HashTableValue* entry = table.entry(propertyName); auto* > Source/JavaScriptCore/runtime/Lookup.h:269 > + const HashTableValue* entry = table.entry(propertyName); auto*
Comment on attachment 271373 [details] Fix Attachment 271373 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/837196 New failing tests: js/dom/shadow-navigator-geolocation-in-strict-mode-does-not-throw.html
Created attachment 271378 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 271373 [details] Fix Attachment 271373 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/837221 New failing tests: js/dom/shadow-navigator-geolocation-in-strict-mode-does-not-throw.html
Created attachment 271379 [details] Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 271373 [details] Fix Attachment 271373 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/837223 New failing tests: js/dom/shadow-navigator-geolocation-in-strict-mode-does-not-throw.html
Created attachment 271380 [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
(In reply to comment #7) > Comment on attachment 271373 [details] > Fix > > Attachment 271373 [details] did not pass mac-debug-ews (mac): > Output: http://webkit-queues.webkit.org/results/837223 > > New failing tests: > js/dom/shadow-navigator-geolocation-in-strict-mode-does-not-throw.html Note that JSNavigator::getOwnPropertySlot() uses getStaticValueSlot<JSNavigator, Base>() internally and that all its attributes are on the instance. You updated getStaticPropertySlot() to search for overrides before checking the property table. Also note that Navigator.geolocation is (wrongly but for compatibility reasons) marked as replaceable so it is not really readonly. I think the reason you don't see the deprecation warning for the second access is because the setter actually succeeded and navigator.geolocation was actually set to 1. Therefore, on second access we read the override, don't call the native getter and thus don't print the deprecation warning anymore. It should be easy to confirm.
(In reply to comment #9) > (In reply to comment #7) > > Comment on attachment 271373 [details] > > Fix > > > > Attachment 271373 [details] did not pass mac-debug-ews (mac): > > Output: http://webkit-queues.webkit.org/results/837223 > > > > New failing tests: > > js/dom/shadow-navigator-geolocation-in-strict-mode-does-not-throw.html > > Note that JSNavigator::getOwnPropertySlot() uses > getStaticValueSlot<JSNavigator, Base>() internally and that all its > attributes are on the instance. You updated getStaticPropertySlot() to > search for overrides before checking the property table. > > Also note that Navigator.geolocation is (wrongly but for compatibility > reasons) marked as replaceable so it is not really readonly. I think the > reason you don't see the deprecation warning for the second access is > because the setter actually succeeded and navigator.geolocation was actually > set to 1. Therefore, on second access we read the override, don't call the > native getter and thus don't print the deprecation warning anymore. It > should be easy to confirm. Note that if I am right though, then it means that https://bugs.webkit.org/show_bug.cgi?id=133559 needs to be fixed some other way. That bug was trying to make sure we did not throw a TypeError when trying to set navigator.geolocation in strict mode by marking the attribute as [Replaceable]. The idea was that marking it as [Replaceable] was a minimal change, stopped the exception throwing and the assignment was still a no-op (due to the bug you fixed in getStaticValueSlot).
Created attachment 271477 [details] Fix
Updated patch, which makes [Replaceable] work correctly. Will work with Chris to determine desired behavior for navigator.geolocation [i.e. whether we still need a hack here] in bug#154304.
Comment on attachment 271477 [details] Fix View in context: https://bugs.webkit.org/attachment.cgi?id=271477&action=review r=me > Source/JavaScriptCore/runtime/Lookup.h:219 > + const HashTableValue* entry = table.entry(propertyName); auto* > Source/JavaScriptCore/runtime/Lookup.h:249 > const HashTableValue* entry = table.entry(propertyName); auto* > Source/JavaScriptCore/runtime/Lookup.h:269 > + const HashTableValue* entry = table.entry(propertyName); auto* > LayoutTests/js/dom/script-tests/shadow-navigator-geolocation-in-strict-mode-does-not-throw.js:-15 > -shouldBe("myNavigator.geolocation", "navigator.geolocation"); Maybe a FIXME comment explaining that returning 1 here is actually wrong?
Comment on attachment 271477 [details] Fix Attachment 271477 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/841814 New failing tests: fast/dom/Window/get-set-properties.html imported/w3c/web-platform-tests/html/dom/interfaces.html fast/dom/Window/window-property-shadowing.html accessibility/mac/test-convenience-methods.html
Created attachment 271482 [details] Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 271477 [details] Fix Attachment 271477 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/841832 New failing tests: fast/dom/Window/get-set-properties.html imported/w3c/web-platform-tests/html/dom/interfaces.html fast/dom/Window/window-property-shadowing.html accessibility/mac/test-convenience-methods.html
Created attachment 271485 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 271477 [details] Fix Attachment 271477 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/841835 New failing tests: fast/dom/Window/get-set-properties.html fast/dom/Window/window-property-shadowing.html accessibility/mac/test-convenience-methods.html
Created attachment 271488 [details] Archive of layout-test-results from ews115 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 271477 [details] Fix The interfaces.html failure seems real: --- /Volumes/Data/EWS/WebKit/WebKitBuild/Release/layout-test-results/retries/imported/w3c/web-platform-tests/html/dom/interfaces-expected.txt +++ /Volumes/Data/EWS/WebKit/WebKitBuild/Release/layout-test-results/retries/imported/w3c/web-platform-tests/html/dom/interfaces-actual.txt @@ -3824,11 +3824,11 @@ PASS Window interface: attribute statusbar PASS Window interface: attribute toolbar PASS Window interface: attribute status -PASS Window interface: operation close() +FAIL Window interface: operation close() assert_equals: property should be writable if and only if not unforgeable expected true but got false PASS Window interface: attribute closed PASS Window interface: operation stop() -PASS Window interface: operation focus() -PASS Window interface: operation blur() +FAIL Window interface: operation focus() assert_equals: property should be writable if and only if not unforgeable expected true but got false +FAIL Window interface: operation blur() assert_equals: property should be writable if and only if not unforgeable expected true but got false PASS Window interface: attribute frames PASS Window interface: attribute length PASS Window interface: attribute opener @@ -3845,7 +3845,7 @@ PASS Window interface: operation prompt(DOMString,DOMString) PASS Window interface: operation print() PASS Window interface: operation showModalDialog(DOMString,any) -PASS Window interface: operation postMessage(any,DOMString,[object Object]) +FAIL Window interface: operation postMessage(any,DOMString,[object Object]) assert_equals: property should be writable if and only if not unforgeable expected true but got false PASS Window interface: operation captureEvents() PASS Window interface: operation releaseEvents() PASS Window interface: attribute onabort
Created attachment 271505 [details] Fix
It might be nice to land https://bugs.webkit.org/show_bug.cgi?id=154304 first so that we don't temporarily allow shadowing Navigator.geolocation.
Comment on attachment 271505 [details] Fix r=me but I think it'd be cleaner to land https://bugs.webkit.org/show_bug.cgi?id=154304 first.
Created attachment 271513 [details] Patch for EWS (already reviewed, retesting post bug#154304)
Comment on attachment 271513 [details] Patch for EWS (already reviewed, retesting post bug#154304) Clearing flags on attachment: 271513 Committed r196678: <http://trac.webkit.org/changeset/196678>
All reviewed patches have been landed. Closing bug.