The defaultRep for Grip's PropRep should default to grip

RESOLVED FIXED in Firefox 50

Status

defect
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: linclark, Assigned: dalimil)

Tracking

Trunk
Firefox 50

Firefox Tracking Flags

(firefox49 affected, firefox50 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

3 years ago
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?
Reporter

Updated

3 years ago
Flags: needinfo?(odvarko)
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
Flags: needinfo?(odvarko)
Assignee

Comment 2

3 years ago
Posted patch Bug1275890.patch - rev1 (obsolete) — Splinter Review
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...
Assignee: nobody → dalimilhajek
Status: NEW → ASSIGNED
Attachment #8773712 - Flags: review?(lclark)
Reporter

Comment 3

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

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

Looks good to me
Attachment #8773712 - Flags: review?(lclark) → review+
Assignee

Comment 5

3 years ago
Posted patch Bug1275890.patch - rev2 (obsolete) — Splinter Review
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.
Attachment #8773712 - Attachment is obsolete: true
Attachment #8774472 - Flags: review?(lclark)
Reporter

Comment 6

3 years ago
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.
Attachment #8774472 - Flags: review?(lclark) → review-
Assignee

Comment 7

3 years ago
Posted patch Bug1275890.patch - rev3 (obsolete) — Splinter Review
OK, fixed.
Attachment #8774472 - Attachment is obsolete: true
Attachment #8774484 - Flags: review?(lclark)
Reporter

Comment 8

3 years ago
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.
Attachment #8774484 - Flags: review?(lclark) → feedback+
Assignee

Comment 9

3 years ago
OK, renamed.

Push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=59e4e8da827b
Attachment #8774484 - Attachment is obsolete: true
Attachment #8774878 - Flags: review?(lclark)
Reporter

Comment 10

3 years ago
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.
Attachment #8774878 - Flags: review?(lclark) → review+
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 11

3 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/1e25c7300f13
The defaultRep for Grip's PropRep should default to grip. r=lclark
Keywords: checkin-needed

Comment 12

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1e25c7300f13
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.