Closed Bug 1281206 Opened 3 years ago Closed 3 years ago

Remove ObjectLink and ObjectBox reps

Categories

(DevTools :: Shared Components, defect, P1)

defect

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Iteration:
50.4 - Aug 1
Tracking Status
firefox50 --- fixed

People

(Reporter: linclark, Assigned: dalimil)

References

Details

(Whiteboard: [reserve-html])

Attachments

(1 file, 2 obsolete files)

Discussed with Honza at the all hands and in IRC today.

We definitely want to remove ObjectLink. We're planning to allow the parent component to pass in a component to be used for linking.

Removing ObjectBox isn't as clear, but I believe we should do it. Currently it just serves as a place to put a class name, but a div can do this in the same way.

The argument for keeping it is that it might be useful in the future as a place to hang things like contextual menus.

However, for now it results in a small performance hit (about 5-7% increase in mountComponent in my back of the napkin performance tests). It also adds an abstraction to the reps system that is currently unnecessary, and which I found confusing when I first approached the API. And once we get to implementing contextual menus, we may find that this is not the approach we want to use.

Since it's not providing a real benefit at this point, I'd say we should remove it too. We can reintroduce it later if we have a use for it.
Honza, based on the above would you be ok with removing ObjectBox?
Flags: needinfo?(odvarko)
(In reply to Lin Clark [:linclark] from comment #1)
> Honza, based on the above would you be ok with removing ObjectBox?
Ok, you convinced me.

Honza
Flags: needinfo?(odvarko)
Hi Lin, should this bug be added to the Track #2 backlog for the HTML Project?
Flags: needinfo?(lclark)
Yes, that makes sense
Flags: needinfo?(lclark)
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [devtools-html]
Priority: P2 → P3
Whiteboard: [devtools-html] → [reserve-html]
There is now both an objectLink property and and ObjectLink component. We want to remove the ObjectLink component, not the objectLink property and its related code.
Attribute.js seems to be the only component directly using object-link.js - are we going to be passing the ObjectLink to Attribute via this.props.objectLink (for example as in object.js)?
Flags: needinfo?(lclark)
I believe that attribute.js should be using ObjectBox, not ObjectLink. It may have just been missed when we did the pass through. However, it should also have the objectLink property handling logic that is in the rest of the reps. Could you take care of that as part of this issue?
Flags: needinfo?(lclark)
Assignee: nobody → dalimilhajek
Status: NEW → ASSIGNED
There are quite a few rules in reps.css for various data types: "objectBox-"+className and also "objectLink-"+className 
How should I treat those? Do you want me to keep the objectBox-* classes for the new span elements replacing the ObjectBox rep?
Flags: needinfo?(lclark)
Yes, for now let's keep the objectBox-* classes.
Flags: needinfo?(lclark)
Attached patch Bug1281206.patch - rev1 (obsolete) — Splinter Review
So I removed the ObjectBox and ObjectLink reps. See the code changes and tell me what you think...

I only tested locally so far - the only test that is failing is for the Undefined rep. ->   is(renderedComponent.getAttribute("role"), "presentation", "Undefined rep has expected aria role");

I think that's because I didn't add the 'role' parameter...

I'm not sure how the 'role': "presentation" parameter is used. Should I attach it to every span element? ( Same as with the 'objectBox-*' classes)
Attachment #8775169 - Flags: review?(lclark)
Comment on attachment 8775169 [details] [diff] [review]
Bug1281206.patch - rev1

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

This looks great to me, thanks! We are going to remove the presentation role anyway in Bug 1272364, so you can just remove that from the test.

If this passes on try, then it will be ready for checkin-needed
Attachment #8775169 - Flags: review?(lclark) → review+
Attached patch Bug1281206.patch - rev2 (obsolete) — Splinter Review
I removed that single test case and pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=05f14322dba1
Attachment #8775169 - Attachment is obsolete: true
Fixed ESLint issues.
Push to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7663e5391868
Attachment #8776254 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/31f91a3da534
Remove ObjectLink and ObjectBox reps. r=lclark
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/31f91a3da534
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Iteration: --- → 50.4 - Aug 1
Priority: P3 → P1
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.