1283123 - Reps: make it possible to pass in a component to handle object links

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect, P1)

Description

[Lin Clark [:linclark]]

This is useful for making objects link to variables view.

Comment 1

[Lin Clark [:linclark]]

Attachment: Testing PR linking

Comment 2

[Lin Clark [:linclark]]

Attachment: Bug 1283123 - Reps: make it possible to pass in a component to handle object links.

Review commit: https://reviewboard.mozilla.org/r/61420/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61420/

Comment 3

[Lin Clark [:linclark]]

Comment on attachment 8766586 [details]
Bug 1283123 - Reps: make it possible to pass in a component to handle object links.

https://reviewboard.mozilla.org/r/61420/#review58562

This is passing all committed tests, so r+ from me.

Comment 4

[Lin Clark [:linclark]]

Actually, one thing I thought of... we might want the property passed in to be "grip" rather than "objectActor". Honza, what do you think?

Comment 5

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

Comment on attachment 8766586 [details]
Bug 1283123 - Reps: make it possible to pass in a component to handle object links.

https://reviewboard.mozilla.org/r/61420/#review58590

Great work here!

(In reply to Lin Clark [:linclark] from comment #4)
> Actually, one thing I thought of... we might want the property passed in to
> be "grip" rather than "objectActor". Honza, what do you think?

Yes agree the name isn't clear (an actor is living on the backend not on the front-end).
I think we can use yet more generic name: `object`. The value that is passed to the objectLink component is always the `object` property from the parent rep. It's the same thing and it should have the same name.

Pleas fix it and I'll quickly re-review again.

Thanks!
Honza

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1283123 - Reps: make it possible to pass in a component to handle object links.

Review commit: https://reviewboard.mozilla.org/r/61956/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61956/

Comment 7

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

Comment on attachment 8767484 [details]
Bug 1283123 - Reps: make it possible to pass in a component to handle object links.

https://reviewboard.mozilla.org/r/61956/#review58796

Looks good to me.

Honza

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8767484 [details]
Bug 1283123 - Reps: make it possible to pass in a component to handle object links.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61956/diff/1-2/

Comment 9

[Lin Clark [:linclark]]

Comment on attachment 8767484 [details]
Bug 1283123 - Reps: make it possible to pass in a component to handle object links.

https://reviewboard.mozilla.org/r/61956/#review59350

This looks good to me, but unfortunately will need an update. The changes to the DatePreview component in new console frontend won't apply because that has been changed with Bug 1283870.

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8767484 [details]
Bug 1283123 - Reps: make it possible to pass in a component to handle object links.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61956/diff/2-3/

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #10)
> Comment on attachment 8767484 [details]
> Bug 1283123 - Reps: make it possible to pass in a component to handle object
> links.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/61956/diff/2-3/

Pulled and rebased.
date-preview.js should not be in the patch anymore

Comment 12

[Lin Clark [:linclark]]

Comment on attachment 8767484 [details]
Bug 1283123 - Reps: make it possible to pass in a component to handle object links.

https://reviewboard.mozilla.org/r/61956/#review59370

LGTM, thanks!

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

TRY is green https://treeherder.mozilla.org/#/jobs?repo=try&revision=54bb82c5ba1b&selectedJob=23386583 except an Android checkstyle (sucessfully retriggered though) which does not seem caused by my patch.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/79d1579dee2e
Reps: make it possible to pass in a component to handle object links. r=linclark,honza

Comment 15

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=10338699&repo=fx-team

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/fa4355015e36
Backed out changeset 79d1579dee2e for test failures in test_reps_date-time.html

Comment 17

[Nicolas Chevobbe [:nchevobbe]]

Okay, I see what's wrong in my patch, I'll push again in a couple hours. 
Any idea why my TRY push ( https://treeherder.mozilla.org/#/jobs?repo=try&revision=54bb82c5ba1b&selectedJob=23386583 ) didn't show this failure ? Does the try syntax doesn't include this test ?

Comment 18

[Carsten Book [:Tomcat]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #17)
> Okay, I see what's wrong in my patch, I'll push again in a couple hours. 
> Any idea why my TRY push (
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=54bb82c5ba1b&selectedJob=23386583 ) didn't show this
> failure ? Does the try syntax doesn't include this test ?

yeah seems the test failures where in  [TC] Linux64 mochitest-chrome-1  and seems that was not included in the try, also don't worry that kind of things happen :))))

Comment 19

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8767484 [details]
Bug 1283123 - Reps: make it possible to pass in a component to handle object links.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61956/diff/3-4/

Comment 20

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8767484 [details]
Bug 1283123 - Reps: make it possible to pass in a component to handle object links.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61956/diff/4-5/

Comment 21

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8767484 [details]
Bug 1283123 - Reps: make it possible to pass in a component to handle object links.

My patch did not pass the Rep's tests (which are not in dt- , didn't knew that). I made some changes in order to pass the test, which it does now.

Comment 22

[Lin Clark [:linclark]]

Comment on attachment 8767484 [details]
Bug 1283123 - Reps: make it possible to pass in a component to handle object links.

https://reviewboard.mozilla.org/r/61956/#review59804

Comment 23

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8767484 [details]
Bug 1283123 - Reps: make it possible to pass in a component to handle object links.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61956/diff/5-6/

Comment 24

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8767484 [details]
Bug 1283123 - Reps: make it possible to pass in a component to handle object links.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61956/diff/6-7/

Comment 25

[Nicolas Chevobbe [:nchevobbe]]

TRY is green https://treeherder.mozilla.org/#/jobs?repo=try&revision=d95bca8ad9fc , except from some known intermittents, unrelated to my patch.

Comment 26

[Carsten Book [:Tomcat]]

has problems to apply:

patching file devtools/client/shared/components/reps/grip-array.js
Hunk #4 FAILED at 125
1 out of 4 hunks FAILED -- saving rejects to file devtools/client/shared/components/reps/grip-array.js.rej

nicolas, could you take a look again ? Thanks!

Comment 27

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8767484 [details]
Bug 1283123 - Reps: make it possible to pass in a component to handle object links.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61956/diff/7-8/

Comment 28

[Nicolas Chevobbe [:nchevobbe]]

Rebased and merged, should be good

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/5b767b7a32d4
Reps: make it possible to pass in a component to handle object links. r=linclark,honza

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5b767b7a32d4