* TEST <script> document.addEventListener("click", function(e) { console.log(e); x = e; }, false); </script> * STEPS TO REPRODUCE 1. Inspect test 2. Click the document 3. In console expand MouseEvent, and the __proto__ => all values are undefined, expected numeric values for obvious things like clientX and clientY * REGRESSION Yes. And this seems pretty annoying.
<rdar://problem/18517505>
Seems like under the hood the inspector depends on the Object.getOwnPropertyNames. Given a simple test: <button id="x">Click Me</button> <script> document.getElementById('x').addEventListener('click', function(event) { console.log( Object.getOwnPropertyNames(event) ); }, false); </script> Browsers seem to disagree: Safari: ["clipboardData"] Firefox: [ "isTrusted" ] Chrome: ["dataTransfer", "which", "toElement", "fromElement", "y", "x", "offsetY", "offsetX", "webkitMovementY", "webkitMovementX", "movementY", "movementX", "relatedTarget", "button", "metaKey", "altKey", "shiftKey", "ctrlKey", "clientY", "clientX", "screenY", "screenX", "keyCode", "charCode", "pageY", "pageX", "layerY", "layerX", "detail", "view", "clipboardData", "path", "cancelBubble", "returnValue", "srcElement", "defaultPrevented", "timeStamp", "cancelable", "bubbles", "eventPhase", "currentTarget", "target", "type"] So, it looks like what is happening is: 1. Inspector builds RemoteObject for “event” => gets a shallow list of event’s own properties and __proto__ => the own properties list is pretty shallow, most event properties are on the __proto__ => inspector UI builds an object tree (ObjectPropertiesSection) 2. When expanding __proto__ Inspector builds a RemoteObject for “event.__proto__” => gets a shallow list of event.__proto__’s pwn properties, using event.__proto__ as the base obj => the list of properties is correct => the values of properties is incorrect (event.__proto__[x] !== event[x]) So, this looks totally broken. My interpretation of this is that prototype properties would have never worked in an ObjectPropertiesSection, as they would always have been relative to the wrong object. Doing the same steps manually in the console I would have gotten a deprecation warning anyways: js> Object.getOwnPropertyDescriptor(x.__proto__.screenX) [Error] Deprecated attempt to access property 'screenX' on a non-MouseEvent object. < undefined The move of properties to prototype is expected behavior defined by WebIDL implemented in WebKit in: http://trac.webkit.org/changeset/r163562
I think we need to make a couple changes here. Given: js> console.log(o) (1) First, get all the properties on "o" (all prototypes, all configurations) => expanding "o" you will see ALL the properties and values, no matter where they are in the prototype chain => we can display own properties / non-own properties differently if we feel that is useful. - requires a change in InjectedScriptSource.js (2) Expanding prototypes, get all the own properties of that prototype and show their value on "o" => now you will see specifically the properties defined by that prototype => they will have real values, instead of always undefined and a silenced exception.
*** Bug 139655 has been marked as a duplicate of this bug. ***
Currently ObjectPropertiesSection populates either ALL properties (ScopeChainSidebar only) or OWN properties (normal case). ObjectPropertyTreeElement populates just OWN properties. Instead, what I want to do is make the normal case fetch not just OWN properties, but OWN+GETTER properties. The only time we should fetch OWN properties is describing a __proto__. What OWN+GETTER properties means, is OWN properties, and all the getter properties from parent prototypes. -- That should meant that logging an Event object, you will immediately get all of the useful properties and values on that object. When you expand the __proto__ chain, it will be to see the APIs of those prototypes (what getters/setters they define, any own property constants on those prototypes, any methods they define). -- There will be next steps after this: (1) WebKit's Native Property descriptors are invalid. Object.getOwnPropertyDescriptor(MouseEvent.prototype, "altKey") gives us a descriptor with "get" undefined. It should be a function. (2) We probably don't want to always evaluate a getter. For the native MouseEvent.prototype.altKey getter that should be fine, but for a non-native getter Foo.prototype.foo, we may not want to. We should detect this case, and provide a way to manually evaluate the getter or not. (3) In the case of overridden getters, users might want to get the value of the getter in a parent prototype. If both SuperClassA and SuperClassB define a getter for "foo", you might want to know the SuperClassA and SuperClassB values of "foo". With the above proposal, __proto__'s will just show APIs, to then interact with the object will require another step.
Created attachment 244833 [details] [PATCH] Proposed Fix
Created attachment 244834 [details] [IMAGE] MouseEvent - with patch
Created attachment 244835 [details] [IMAGE] MouseEvent - with patch - iOS 8
Created attachment 244836 [details] [IMAGE] MouseEvent - without patch
I wouldn't expect to see the getters still under the prototype. We should remove them from there when they are promoted to the object. Agree?
(In reply to comment #10) > I wouldn't expect to see the getters still under the prototype. We should > remove them from there when they are promoted to the object. Agree? Ideally no-one will need to expand the __proto__ chain anymore. If they do, they are looking at the prototype chain API surface (constants, functions, getter/setter functions). In this case I think it is fine to show the getters/setters under the prototype. We should improve their appearance in some way. Currently they are undefined because of bug 140575, but they should be functions. However we make them appear, I think showing them, alongside the other function properties, is useful.
Another way of thinking about this is to make the own properties visually distinct at the first level. If you have: function Foo() { this._foo = 5; } Foo.prototype = { __proto__: Object.prototype, get foo() { return this._foo; } }; Then showing the object: console.log(new Foo); We will see: _foo // OWN property on the object itself foo // getter from prototype chain (Foo.prototype.foo), not an OWN property, expect to see a "get foo" somewhere if you expand the prototype chain This way if you have lots of properties that are actually getters in the prototype chain appearing at the top level they could be distinct from the real own properties that won't have anything higher up in the prototype chain.
Still reviewable.
Comment on attachment 244833 [details] [PATCH] Proposed Fix Clearing flags on attachment: 244833 Committed r178768: <http://trac.webkit.org/changeset/178768>
All reviewed patches have been landed. Closing bug.
Joe landed an update to inspector/model/remote-object-get-properties.html in <http://trac.webkit.org/changeset/178795>. However, the test still crashes and times out on various bots, see <https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fmodel%2Fremote-object-get-properties.html>. Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.JavaScriptCore 0x000000010c5fed7a WTFCrash + 42 (Assertions.cpp:321) 1 com.apple.JavaScriptCore 0x000000010c223ee9 Inspector::Protocol::PrimitiveBindingTraits<(Inspector::InspectorValue::Type)1>::assertValueHasExpectedType(Inspector::InspectorValue*) + 89 (InspectorProtocolTypes.h:125) 2 com.apple.JavaScriptCore 0x000000010c222af1 Inspector::Protocol::BindingTraits<Inspector::Protocol::Runtime::PropertyDescriptor>::assertValueHasExpectedType(Inspector::InspectorValue*) + 1297 (InspectorProtocolObjects.cpp:615) 3 com.apple.JavaScriptCore 0x000000010c19d31c Inspector::Protocol::BindingTraits<Inspector::Protocol::Array<Inspector::Protocol::Runtime::PropertyDescriptor> >::assertValueHasExpectedType(Inspector::InspectorValue*) + 236 (InspectorProtocolTypes.h:153) 4 com.apple.JavaScriptCore 0x000000010c19c26c Inspector::Protocol::BindingTraits<Inspector::Protocol::Array<Inspector::Protocol::Runtime::PropertyDescriptor> >::runtimeCast(WTF::RefPtr<Inspector::InspectorValue>&&) + 204 (InspectorProtocolTypes.h:139) ... I think that I'm going to roll out for now.
(In reply to comment #16) > Joe landed an update to inspector/model/remote-object-get-properties.html in > <http://trac.webkit.org/changeset/178795>. > > However, the test still crashes and times out on various bots, see > <https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard. > html#showAllRuns=true&tests=inspector%2Fmodel%2Fremote-object-get-properties. > html>. > > Thread 0 Crashed:: Dispatch queue: com.apple.main-thread > 0 com.apple.JavaScriptCore 0x000000010c5fed7a WTFCrash + 42 > (Assertions.cpp:321) > 1 com.apple.JavaScriptCore 0x000000010c223ee9 > Inspector::Protocol::PrimitiveBindingTraits<(Inspector::InspectorValue:: > Type)1>::assertValueHasExpectedType(Inspector::InspectorValue*) + 89 > (InspectorProtocolTypes.h:125) > 2 com.apple.JavaScriptCore 0x000000010c222af1 > Inspector::Protocol::BindingTraits<Inspector::Protocol::Runtime:: > PropertyDescriptor>::assertValueHasExpectedType(Inspector::InspectorValue*) > + 1297 (InspectorProtocolObjects.cpp:615) > 3 com.apple.JavaScriptCore 0x000000010c19d31c > Inspector::Protocol::BindingTraits<Inspector::Protocol::Array<Inspector:: > Protocol::Runtime::PropertyDescriptor> > >::assertValueHasExpectedType(Inspector::InspectorValue*) + 236 > (InspectorProtocolTypes.h:153) > 4 com.apple.JavaScriptCore 0x000000010c19c26c > Inspector::Protocol::BindingTraits<Inspector::Protocol::Array<Inspector:: > Protocol::Runtime::PropertyDescriptor> > >::runtimeCast(WTF::RefPtr<Inspector::InspectorValue>&&) + 204 > (InspectorProtocolTypes.h:139) > ... > > I think that I'm going to roll out for now. Wow, I didn't notice that. Please roll out, I'll address this tomorrow.
There are two other changes blocking rollout (http://trac.webkit.org/changeset/178791 and http://trac.webkit.org/projects/webkit/changeset/178796), so it seems more practical to skip the test until tomorrow. I'll do that.
I have a fix for the ASSERT.