1275890 - The defaultRep for Grip's PropRep should default to grip

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect)

Description

[Lin Clark [:linclark]]

If a Grip is the parent, is there any case where a child object would not also be a Grip? 

Grips have metadata properties (such as actor) that we don't want to show to the user. The current way this is avoided is by passing a defaultRep into the parent. Then, there is implicit passing of props going on which sets the PropRep's default grip to Grip, so it is also handed as a Grip element.

However, we could potentially just hardcode this in PropRep. This means consumers of Reps don't need to know about defaultRep. I think this would make a more ergonomic API, especially since Reps are mostly going to be used to render grips. Honza, can you see any cases where this would not work as expected?

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Good point (In reply to Lin Clark [:linclark] from comment #0)
> If a Grip is the parent, is there any case where a child object would not
> also be a Grip? 
Good point. I don't know about such cases.

> Grips have metadata properties (such as actor) that we don't want to show to
> the user. The current way this is avoided is by passing a defaultRep into
> the parent. Then, there is implicit passing of props going on which sets the
> PropRep's default grip to Grip, so it is also handed as a Grip element.
> 
> However, we could potentially just hardcode this in PropRep. This means
Yes, I like that idea.

Note that PropRep is second component in the file and we should move
it out into a separate file/module at some point (to match our code-style rules).
Also, Object rep and Grip rep are both using their own PropRep, which is almost
identical and should be shared.
So, I think we should pass the hardcoded defaultRep from within the Grip
into PropRep (not within the PropRep).

I am thinking about something like as follows:

// Show only interesting properties.
if (filter(type, value)) {
  props.push(PropRep(Object.assign({}, this.props, {
    key: name,
    mode: "tiny",
    name: name,
    object: value,
    equal: ": ",
    delim: ", ",
    defaultRep: Grip  //<<<<<<<
  })));
}

Honza

Comment 2

[Dalimil Hajek [:dalimil]]

Attachment: Bug1275890.patch - rev1

PropRep is currently an individual component that is shared by both object.js and grip.js. Therefore I modified the construction of new PropRep instances in Grip (as suggested) instead of PropRep itself. 
Named-node-map is still using its own (almost identical) implementation of PropRep, but there seem to be some blocking patches for this component at the moment, so I didn't change that...

Comment 3

[Lin Clark [:linclark]]

Comment on attachment 8773712 [details] [diff] [review]
Bug1275890.patch - rev1

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

Looks good to me

Comment 4

[Dalimil Hajek [:dalimil]]

Pushing to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6034e2120033

Comment 5

[Dalimil Hajek [:dalimil]]

Attachment: Bug1275890.patch - rev2

I was passing the wrong Grip object... I updated the code (+ also rebased) and uploaded to the try server again ( https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f93e79f137a ). The failed tests seem to be unrelated to this bug.

Comment 6

[Lin Clark [:linclark]]

Comment on attachment 8774472 [details] [diff] [review]
Bug1275890.patch - rev2

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

::: devtools/client/shared/components/reps/grip.js
@@ +118,5 @@
>            name: name,
>            object: value,
>            equal: ": ",
>            delim: ", ",
> +          defaultRep: exports.Grip

We shouldn't need to use the exports object here. Exports is just for exporting variables out of the file.

Instead, you should create a local variable. Then you can use that to assign to exports and also use it here.

Comment 7

[Dalimil Hajek [:dalimil]]

Attachment: Bug1275890.patch - rev3

OK, fixed.

Comment 8

[Lin Clark [:linclark]]

Comment on attachment 8774484 [details] [diff] [review]
Bug1275890.patch - rev3

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

I see the issue now. Instead of calling it gripModule, let's change Grip to GripRep and gripModule to Grip.

Comment 9

[Dalimil Hajek [:dalimil]]

Attachment: Bug1275890.patch - rev4

OK, renamed.

Push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=59e4e8da827b

Comment 10

[Lin Clark [:linclark]]

Comment on attachment 8774878 [details] [diff] [review]
Bug1275890.patch - rev4

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

Looks good to me. If there are no relevant failures on try, this should be good to go.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/1e25c7300f13
The defaultRep for Grip's PropRep should default to grip. r=lclark

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1e25c7300f13