Closed Bug 1284838 Opened 5 years ago Closed 5 years ago

Reps: render events more uniformly

Categories

(DevTools :: Shared Components, enhancement, P1)

enhancement

Tracking

(firefox50 affected, firefox51 fixed)

RESOLVED FIXED
Firefox 51
Iteration:
51.1 - Aug 15
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: linclark, Assigned: evanxd)

References

Details

(Whiteboard: [reserve-html])

Attachments

(1 file, 13 obsolete files)

6.69 KB, patch
Honza
: review+
Details | Diff | Splinter Review
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.
Blocks: 1283847
Whiteboard: [devtools-html] [triage]
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [devtools-html] [triage] → [devtools-html]
could you help elaborate more about this bug?
Flags: needinfo?(lclark)
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.
Depends on: 1264682
Flags: needinfo?(lclark)
Priority: P2 → P3
Whiteboard: [devtools-html] → [reserve-html]
> 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.
Flags: needinfo?(lclark)
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.
Flags: needinfo?(lclark)
Hi Lin,

Got it, thanks.
Assignee: nobody → evan
Status: NEW → ASSIGNED
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.
Flags: needinfo?(odvarko)
(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
Flags: needinfo?(odvarko)
Attached patch bug-1284838.patch (obsolete) — Splinter Review
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
Attachment #8773658 - Flags: feedback?(lclark)
Flags: needinfo?(odvarko)
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
Flags: needinfo?(odvarko)
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.
Attachment #8773658 - Flags: feedback?(lclark)
According to Comment 12 and Comment 13, this bug needs the patch of Bug 1282791 to continue to work.
Depends on: 1282791
Attachment #8773658 - Attachment is obsolete: true
Attached patch wip-bug-1284838.patch (obsolete) — Splinter Review
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
Blocks: 1289062
Attachment #8774268 - Attachment is obsolete: true
Attached patch bug-1284838.patch (obsolete) — Splinter Review
Attachment #8774594 - Attachment is obsolete: true
Attached patch bug-1284838.patch (obsolete) — Splinter Review
Hi Lin,

Could you help to review the patch?

Thanks.
Attachment #8774595 - Flags: review?(lclark)
Attachment #8774595 - Attachment is obsolete: true
Attachment #8774595 - Flags: review?(lclark)
Attached patch bug-1284838.patch (obsolete) — Splinter Review
(patch updated)

Hi Lin,

Could you help to review the patch?

Thanks.
Attachment #8774596 - Flags: review?(lclark)
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.
Attachment #8774596 - Flags: review?(lclark) → review-
Attachment #8774596 - Attachment is obsolete: true
Attached patch bug-1284838.patch (obsolete) — Splinter Review
Hi Lin,

I updated the patch for your comments.
Please help to review the patch.

Thanks.
Attachment #8775475 - Flags: review?(lclark)
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?
Attachment #8775475 - Flags: review?(lclark) → review-
Attachment #8775475 - Attachment is obsolete: true
Attached patch bug-1284838.patch (obsolete) — Splinter Review
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.
Attachment #8776597 - Attachment is obsolete: true
Attached patch bug-1284838.patch (obsolete) — Splinter Review
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. ^
Attachment #8776607 - Flags: review?(lclark)
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.
Attachment #8776607 - Flags: review?(lclark) → review-
Attachment #8776607 - Attachment is obsolete: true
Attached patch bug-1284838.patch (obsolete) — Splinter Review
Attachment #8778186 - Attachment is obsolete: true
Attached patch bug-1284838.patch (obsolete) — Splinter Review
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
Attachment #8778195 - Flags: review?(lclark)
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.
Attachment #8778195 - Flags: review?(lclark) → review-
Honza, could you give this a look when you get back.
Flags: needinfo?(odvarko)
(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.
Attachment #8778195 - Attachment is obsolete: true
Attached patch bug-1284838.patch (obsolete) — Splinter Review
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.
Attachment #8778745 - Flags: review?(odvarko)
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))? )
Attachment #8778745 - Flags: review?(odvarko)
@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
Flags: needinfo?(odvarko) → needinfo?(lclark)
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
Flags: needinfo?(lclark)
Attachment #8778745 - Attachment is obsolete: true
(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
Attached patch bug-1284838.patch (obsolete) — Splinter Review
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
Attachment #8780003 - Flags: review?(odvarko)
(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
Attachment #8780003 - Flags: review?(odvarko)
Attachment #8780003 - Attachment is obsolete: true
Attached patch bug-1284838.patch (obsolete) — Splinter Review
Hi Honza,

I removed Immutable. Pleas review it again.

Thanks.
Attachment #8780038 - Flags: review?(odvarko)
Comment on attachment 8780038 [details] [diff] [review]
bug-1284838.patch

Review of attachment 8780038 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great now, thanks!

Honza
Attachment #8780038 - Flags: review?(odvarko) → review+
Thanks for the review, Honza.

And patch is r+, and the try is passed. Do checkin-needed.
Keywords: checkin-needed
Keywords: 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.
Attachment #8781013 - Flags: review?(odvarko)
Comment on attachment 8781013 [details] [diff] [review]
bug-1284838.patch v2

Review of attachment 8781013 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Thanks,
Honza
Attachment #8781013 - Flags: review?(odvarko) → review+
Comment on attachment 8780038 [details] [diff] [review]
bug-1284838.patch

This one is now obsolete, I guess.
Honza
Attachment #8780038 - Attachment is obsolete: true
(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.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/1837517f42e4
Return Grip in Events render method. r=Honza
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1837517f42e4
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Iteration: --- → 51.1 - Aug 15
Priority: P3 → P1
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.