Currently, only a few types of events have any output besides the event name. Even those only output a few of the properties. We probably want to output it like the console and Chrome do, as an object with properties.

could you help elaborate more about this bug?

You can see the way that reps currently display an event in the test in Bug 1264682. You can see what the console currently outputs for an event using this URL: data:text/html;charset=utf-8,<div id="test">test</div><script>document.getElementById("test").addEventListener("click", function( event ) {event.target.innerHTML = "click count: " + event.detail; console.log(event)}, false);</script> When you open the console and click the word test, it will log the event. That's how we want it to be displayed. We want reps to output the event as the console does. It might make sense for the event to just be rendered as an object.

> We want reps to output the event as the console does. > > It might make sense for the event to just be rendered as an object. Hi Lin, Looks like we want to display the event as `Object {clickclientX: 16, clientY: 18}` instead of current `clickclientX=16, clientY=18` on console, right? Thanks.

We would like to have the kind of event in front, and we want to have all the properties. If you run the example that I showed in Chrome, you'll see the desired output is something like: > MouseEvent {isTrusted: true, screenX: 20, screenY: 110, more…} As an additional step, Honza would prefer that some of these props be reordered so that it is something like this: > MouseEvent {clientX: 20, clientY: 16, isTrusted: true, more…} But getting it like the first one is step one.

Hi Lin, Got it, thanks.

We take care about the below three types of event in this bug. 1. MouseEvent { clientX:0, clientY: 0 } 2. KeyEvent { keyCode: 0 } 3. MessageEvent { data: "message" }

(In reply to Lin Clark [:linclark] from comment #4) > We would like to have the kind of event in front, and we want to have all > the properties. If you run the example that I showed in Chrome, you'll see > the desired output is something like: > > > MouseEvent {isTrusted: true, screenX: 20, screenY: 110, more…} > > As an additional step, Honza would prefer that some of these props be > reordered so that it is something like this: > > > MouseEvent {clientX: 20, clientY: 16, isTrusted: true, more…} > > But getting it like the first one is step one. Agree with Lin, let's start with step one and mimic what Chrome does. As soon as it's done we'll discuss the prop prioritization. (In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #6) > We take care about the below three types of event in this bug. > 1. MouseEvent { clientX:0, clientY: 0 } > 2. KeyEvent { keyCode: 0 } > 3. MessageEvent { data: "message" } Yes, this bug is related to these three event types. Honza

> Agree with Lin, let's start with step one and mimic what Chrome does. > As soon as it's done we'll discuss the prop prioritization. Hi Honza, Are you saying we can do the step one(Comment 4) in this bug and do step two in another new bug? I also prefer this way. We can fix problem one(step one) first and discuss problem two in a new bug.

(In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #8) > > Agree with Lin, let's start with step one and mimic what Chrome does. > > As soon as it's done we'll discuss the prop prioritization. > > Hi Honza, > > Are you saying we can do the step one(Comment 4) in this bug and do step two > in another new bug? > > I also prefer this way. We can fix problem one(step one) first and discuss > problem two in a new bug. Yes, sounds good. Honza

Hi Honza and Lin, What do you think of that we render the event object in `grip.js`[1] and remove the `event.js`[2]? I did that in the patch. [1]: https://github.com/mozilla/gecko-dev/blob/master/devtools/client/shared/components/reps/grip.js [2]: https://github.com/mozilla/gecko-dev/blob/master/devtools/client/shared/components/reps/event.js

Comment on attachment 8773658 [details] [diff] [review] bug-1284838.patch Review of attachment 8773658 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/shared/components/reps/event.js @@ +67,5 @@ > if (!isGrip(grip)) { > return false; > } > > + return false; It means let's remove the `event.js` file.

(In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #11) > Comment on attachment 8773658 [details] [diff] [review] > bug-1284838.patch > > Review of attachment 8773658 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: devtools/client/shared/components/reps/event.js > @@ +67,5 @@ > > if (!isGrip(grip)) { > > return false; > > } > > > > + return false; > > It means let's remove the `event.js` file. Even if the Event rep should render events the same way as Grip rep, I believe there it would still be a bit better to handle DOMEvent things inside Event rep instead of Grip rep. Specifically: 1) Handling DOM event properties: object.preview.properties 2) Supporting DOMEvent type (grip.preview.kind == "DOMEvent") Doing it this way we can later support the step #2 (highlighting the important prope) Honza

Comment on attachment 8773658 [details] [diff] [review] bug-1284838.patch Review of attachment 8773658 [details] [diff] [review]: ----------------------------------------------------------------- I agree with what Honza said. You should be able to depend on the grip rep from inside the event rep. This would mean just returning a grip from event's render method.

According to Comment 12 and Comment 13, this bug needs the patch of Bug 1282791 to continue to work.

It's the WIP patch. Will continue to work after Bug 1282791 is landed in mozilla-central.

Hi Honza and Lin, Agreed with your opinions. I'm doing the below ideas in the WIP patch[1]. > 1) Handling DOM event properties: object.preview.properties > 2) Supporting DOMEvent type (grip.preview.kind == "DOMEvent") > I agree with what Honza said. You should be able to depend on the grip rep > from inside the event rep. This would mean just returning a grip from > event's render method. [1]: https://bugzilla.mozilla.org/attachment.cgi?id=8774268

Hi Lin, Could you help to review the patch? Thanks.

(patch updated) Hi Lin, Could you help to review the patch? Thanks.

The try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fe46d59bced&selectedJob=24516508

Comment on attachment 8774596 [details] [diff] [review] bug-1284838.patch Review of attachment 8774596 [details] [diff] [review]: ----------------------------------------------------------------- I don't think that the objectProperties logic should be necessary here. You should just be able to create a clone of the object (using Object.assign or something similar) and then reorder the properties on the cloned object. Then you can pass it in to the Grip rep as the object prop.

Hi Lin, I updated the patch for your comments. Please help to review the patch. Thanks.

Comment on attachment 8775475 [details] [diff] [review] bug-1284838.patch Review of attachment 8775475 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/shared/components/reps/event.js @@ +27,4 @@ > render: function () { > + let rep = React.createFactory(Grip.rep); > + let object = Object.assign({}, this.props.object); > + object.preview.ownProperties = object.preview.properties; Object.assign only clones the first level. The preview object here is the same one that came in via props, attached to the original object. We don't want to alter that in the render function. Instead, you should be assigning a new object to object.preview and then acting on that new object. Also, what should be happening to this object is that the props should be reordered. For example, if it's a MoustEvent, then clientX should be added first, then clientY, then the rest of the props. ::: devtools/client/shared/components/reps/grip.js @@ +111,5 @@ > > indexes.forEach((i) => { > let name = Object.keys(ownProperties)[i]; > + let prop = ownProperties[name]; > + let value = prop.value !== undefined ? prop.value : prop; Why is this necessary? @@ +144,5 @@ > return indexes; > } > > let prop = ownProperties[name]; > + let value = prop.value !== undefined ? prop.value : prop; Why is this necessary?

I updated patch for the comments. > Object.assign only clones the first level. The preview object here is the > same one that came in via props, attached to the original object. We don't > want to alter that in the render function. Instead, you should be assigning > a new object to object.preview and then acting on that new object. If we don't want to alter the `this.props.object.preview.properties` object, let's just clone it. > Also, what should be happening to this object is that the props should be > reordered. For example, if it's a MoustEvent, then clientX should be added > first, then clientY, then the rest of the props. I discussed the reorder things with Honza(Comment 9). We can discuss and do it in a new bug(Bug 1289062)

If we aren't doing the reordering here, we don't need to clone it because nothing should change in the object.

Comment on attachment 8776607 [details] [diff] [review] bug-1284838.patch Review of attachment 8776607 [details] [diff] [review]: ----------------------------------------------------------------- Hi Lin, I updated the patch for the comments, and explained why I did `let value = prop.value !== undefined ? prop.value : prop;`. Please help to review the patch again. Thanks. ::: devtools/client/shared/components/reps/event.js @@ +27,3 @@ > render: function () { > + let rep = React.createFactory(Grip.rep); > + let properties = this.props.object.preview.properties > If we aren't doing the reordering here, we don't need to clone it because nothing should change in the object. Sure, so we don't need to clone the `this.props.object.preview.properties` object. Just rename the object as `ownProperties` here. ::: devtools/client/shared/components/reps/grip.js @@ +110,5 @@ > > indexes.forEach((i) => { > let name = Object.keys(ownProperties)[i]; > + let prop = ownProperties[name]; > + let value = prop.value !== undefined ? prop.value : prop; I did this because the RDP grip package of event rep don't have the `value` property. For example, the RDP grip package of MouseEvent looks like: ``` { type: "object" class: "MouseEvent" extensible: true frozen: false sealed: false ownPropertyLength: 1 preview: { kind: "DOMEvent", type: "click", target: Object, properties: { buttons: 0, clientX: 26, clientY: 13, layerX: 0, layerY: 0 } } } ``` The values of `clientX`, `clientY`, and others of the `properties` property are just numbers. So I did this to get the values of them. @@ +144,5 @@ > return indexes; > } > > let prop = ownProperties[name]; > + let value = prop.value !== undefined ? prop.value : prop; Same as above. ^

Comment on attachment 8776607 [details] [diff] [review] bug-1284838.patch Review of attachment 8776607 [details] [diff] [review]: ----------------------------------------------------------------- I'm pretty sure the tests won't pass with this. I haven't tested it, but since the properties aren't being sorted and the tests check for that, I don't think that they would. Once you've updated the test (specially test_reps_event.js) I can see whether or not the output is what we expect.

The try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b260ca5d348b

Hi Lin, You're right. I updated the tests. Now it's all passed. And the try is here[1]. Please help to review the patch again, thanks a lot. [1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=953b040cf728

Comment on attachment 8778195 [details] [diff] [review] bug-1284838.patch Review of attachment 8778195 [details] [diff] [review]: ----------------------------------------------------------------- I pointed out an issue, but since Honza is coming back soon, I think it would be worth getting his review on this too before moving further. ::: devtools/client/shared/components/reps/event.js @@ +27,5 @@ > render: function () { > + let rep = React.createFactory(Grip.rep); > + let properties = this.props.object.preview.properties; > + this.props.object.preview.ownProperties = properties; > + delete this.props.object.preview.properties; I think you removed the Object.assign code because of what I said here: > If we aren't doing the reordering here, we don't need to clone it because nothing should change in the object. That only applies if you aren't changing things on the object. However, since this is adding and deleting properties, you do need to clone it. `this.props.object` could potentially be passed to another component and if you change it here, then it will change mysteriously in that other component as well.

Honza, could you give this a look when you get back.

(In reply to Lin Clark [:linclark] from comment #32) > Comment on attachment 8778195 [details] [diff] [review] > bug-1284838.patch > > Review of attachment 8778195 [details] [diff] [review]: > ----------------------------------------------------------------- > > I pointed out an issue, but since Honza is coming back soon, I think it > would be worth getting his review on this too before moving further. > > ::: devtools/client/shared/components/reps/event.js > @@ +27,5 @@ > > render: function () { > > + let rep = React.createFactory(Grip.rep); > > + let properties = this.props.object.preview.properties; > > + this.props.object.preview.ownProperties = properties; > > + delete this.props.object.preview.properties; > > I think you removed the Object.assign code because of what I said here: > > > If we aren't doing the reordering here, we don't need to clone it because nothing should change in the object. > > That only applies if you aren't changing things on the object. However, > since this is adding and deleting properties, you do need to clone it. > `this.props.object` could potentially be passed to another component and if > you change it here, then it will change mysteriously in that other component > as well. Got it. Had some misunderstanding before. Updated patch for that.

Hi Honza, Updated the patch for Lin's comments. Could you also help to review the patch? Thanks.

Comment on attachment 8778745 [details] [diff] [review] bug-1284838.patch Hi Honza, Could you help to review the patch? This patch uses grip rep to render event rep. And let discuss and reorder the event's props in Bug 1289062. Thanks.

Comment on attachment 8778745 [details] [diff] [review] bug-1284838.patch Review of attachment 8778745 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch! Please, read my inline comments. Honza ::: devtools/client/shared/components/reps/event.js @@ +27,2 @@ > render: function () { > + let rep = React.createFactory(Grip.rep); You should call createFactory() just once. Put something as follows at the top of the file const { rep: GripRepFactory } = createFactories(GripRep); See also: attributes.js (StringRep is used the same way) @@ +27,3 @@ > render: function () { > + let rep = React.createFactory(Grip.rep); > + let object = Object.assign(this.props.object); The first argument of Object.assign is just returned so, this code doesn't clone the `props.object`. You need to do: var copy = Object.assign({}, obj); See also: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign @@ +29,5 @@ > + let object = Object.assign(this.props.object); > + object.preview = Object.assign(this.props.object.preview); > + object.preview.ownProperties = object.preview.properties; > + delete object.preview.properties; > + object.ownPropertyLength = Object.keys(object.preview.ownProperties).length; I am not big fan of cloning the grip object this way. Perhaps we could use Immutable API (or just plain JSON.parse(JSON.stringify(this.props.object))? )

@linclark: the attached patch is cloning a grip using Object.assign (see the previous comment). It works, but the code is a bit clumpy. Is there something better we could use for cloning Grips? Could we use Immutable API? Honza

Btw. I was just talking to ochameau and JSON.stringify/JSON.parse could be ok. Honza

I'm not opposed to using Immutable.js, but if we do then we'd want to have it throughout the whole reps system. That seems like it would take considerable effort. AFAIK, JSON.stringify/JSON.parse is worse for performance. Do we have shouldComponentUpdate setup on reps? If we don't, I worry about a JSON.stringify/JSON.parse on every single render

(In reply to Lin Clark [:linclark] from comment #40) > I'm not opposed to using Immutable.js, but if we do then we'd want to have > it throughout the whole reps system. That seems like it would take > considerable effort. > > AFAIK, JSON.stringify/JSON.parse is worse for performance. Do we have > shouldComponentUpdate setup on reps? No > If we don't, I worry about a > JSON.stringify/JSON.parse on every single render Yes, I share the same feeling. OK, let's use Object.assign(); @Evan, please make a short comment in the code explaining why Object.assign is used (saying that JSON.stringify/JSON.parse is slow and Immutable is planned for the future). Thanks Honza

Hi Honza and Lin, I updated patch for the comments. And I think using Immutable.js would be a simple and efficient solution for the situation(deep clone) in the patch. How about we just do `Immutable.Map(this.props).toJS()` to deep clone the `this.props` object? Check it in the patch. The main reason I don't suggest using `JSON.stringify/JSON.parse` here is that it cannot clone Function object. We want to add a new func property on GripRep component for filtering interesting props[1] in Bug 1289062. [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1289062#c2

The try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=265de95b56a5

(In reply to Jan Honza Odvarko [:Honza] from comment #41) > (In reply to Lin Clark [:linclark] from comment #40) > > I'm not opposed to using Immutable.js, but if we do then we'd want to have > > it throughout the whole reps system. That seems like it would take > > considerable effort. > > > > AFAIK, JSON.stringify/JSON.parse is worse for performance. Do we have > > shouldComponentUpdate setup on reps? > No > > > > If we don't, I worry about a > > JSON.stringify/JSON.parse on every single render > Yes, I share the same feeling. > > OK, let's use Object.assign(); Didn't see the comment before I upload the update patch[1]. How about we just do `Immutable.Map(this.props).toJS()` to deep clone the `this.props` object? Or you prefer Object.assign()? [1]: https://bugzilla.mozilla.org/attachment.cgi?id=8780003

(In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #44) Thanks for quick response! > How about we just do `Immutable.Map(this.props).toJS()` to deep clone the > `this.props` object? Or you prefer Object.assign()? Yes, let's use Object.assign() for now. I agree with Lin, using Immutable is bigger task and we should do it in separate bug/discussion. Honza

Comment on attachment 8780003 [details] [diff] [review] bug-1284838.patch Review of attachment 8780003 [details] [diff] [review]: ----------------------------------------------------------------- So, please remove Immutable() for now. Thanks, Honza

Hi Honza, I removed Immutable. Pleas review it again. Thanks.

The try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3553585f920

Comment on attachment 8780038 [details] [diff] [review] bug-1284838.patch Review of attachment 8780038 [details] [diff] [review]: ----------------------------------------------------------------- Looks great now, thanks! Honza

Thanks for the review, Honza. And patch is r+, and the try is passed. Do checkin-needed.

Hi Honza, Since Bug 1284855 is landed, our rep output shows up as `Object { foo: "bar" }` instead of `Object {foo: "bar"}`. And it make the mochi tests of previous patch failed. I updated the patch(already got r+ before) for adding spaces to make mochi tests passed. And the try is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c42a2302e8b3 Could you help to review it? Thanks.

Comment on attachment 8781013 [details] [diff] [review] bug-1284838.patch v2 Review of attachment 8781013 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Thanks, Honza

Comment on attachment 8780038 [details] [diff] [review] bug-1284838.patch This one is now obsolete, I guess. Honza

(In reply to Jan Honza Odvarko [:Honza] from comment #53) > Comment on attachment 8780038 [details] [diff] [review] > bug-1284838.patch > > This one is now obsolete, I guess. Yes, that's right. Thanks for the review, Honza.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/fx-team/rev/1837517f42e4 Return Grip in Events render method. r=Honza