replaceable own properties seem to ignore replacement after property caching
Created attachment 279826 [details] patch
Attachment 279826 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 279826 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=279826&action=review A JSC person should review this, just a few minor comments from me. > Source/JavaScriptCore/runtime/Lookup.h:295 > + auto result = thisObj->putDirect(vm, propertyName, value); I think bool is more readable and as short as bool :) > Source/JavaScriptCore/runtime/Lookup.h:298 > + thisObj->JSObject::setStructure(exec->vm(), Structure::attributeChangeTransition(exec->vm(), thisObj->structure(), propertyName, 0)); should be vm not exec->vm()
Comment on attachment 279826 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=279826&action=review > Source/WebCore/ChangeLog:8 > + No new tests (OOPS!). OOPS :)
Comment on attachment 279826 [details] patch Attachment 279826 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1382404 New failing tests: js/cached-window-properties.html
Created attachment 279832 [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 279826 [details] patch Attachment 279826 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1382403 New failing tests: js/cached-window-properties.html
Created attachment 279834 [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 279826 [details] patch Attachment 279826 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1382430 New failing tests: js/cached-window-properties.html
Created attachment 279835 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4
Comment on attachment 279826 [details] patch Attachment 279826 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1382632 New failing tests: js/cached-window-properties.html
Created attachment 279839 [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
Created attachment 279865 [details] patch
Comment on attachment 279865 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=279865&action=review > Source/JavaScriptCore/runtime/Lookup.h:292 > +template <class ThisImp> > +inline bool replaceStaticPropertySlot(ExecState* exec, ThisImp* thisObj, PropertyName propertyName, JSValue value) Here are things I would have done differently if I had written this: - I would have written it like this: template<typename ThisObjectClass> inline bool ... Note that I prefer template on the same line, no space after the word template, typename rather than class, no abbreviation in the class name, and in particular unclear why Imp is needed here. - I would have considered making the thisObject argument a JSObject* rather than making this a template. I am unclear on what we gain by using a function template. Actually, I would have used JSObject& but then I would have run into the rule where you all don’t allow that for JSObject in the JavaScriptCore side of things. - The first argument would be VM& vm since the function doesn’t need an ExecState and the caller is passing VM to other functions. - Or the first argument would be ExecState& state rather than ExecState* exec. - The second argument name would be thisObject rather than thisObj. > Source/JavaScriptCore/runtime/Lookup.h:300 > + bool result = thisObj->putDirect(vm, propertyName, value); > + > + if (result && !thisObj->staticFunctionsReified()) > + thisObj->JSObject::setStructure(exec->vm(), Structure::attributeChangeTransition(vm, thisObj->structure(), propertyName, 0)); > + > + return result; I would have used early return rather than a boolean: if (!thisObject.putDirect(vm, propertyName, value)) return false; ... return true; Also, the call to setStructure is using "exec->vm()" instead of just "vm" in one place.
Committed r201428: <http://trac.webkit.org/changeset/201428>