1607148 - Reps bundle was reverted to a previous version (but original files were not)

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect, P3)

Description

[Nicolas Chevobbe [:nchevobbe]]

In Bug 1602804 the reps bundle file was directly modified, without modifying the original file.
It means that when doing a node bin/bundle.js now, the reps file will be updated to reflect what's in the original file.

Jaril, was the change intentional? It reverted a change that was made for performance reason, so I'm not sure

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Jason, you reviewed that patch, could you tell me more about what it was for?

Comment 2

[Jason Laster [:jlast]]

Yes, the change was intentional. We no longer need to release the object actors .

Watchpoints are now managed in a Map and not on Object actors, which makes a big difference for quality.

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

okay thanks for the answer

Comment 4

[Stepan Stava [:stepan]]

Attachment: Bug 1607148 - Reps bundle was reverted to a previous version (but original files were not) r=nchevobbe

removed part of devtools-reps/src/object-inspector/actions.js as suggested in the patch D61086

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17263a32d12e
Reps bundle was reverted to a previous version (but original files were not) r=nchevobbe

Comment 6

[Noemi Erli[:noemi_erli]]

Backed out changeset 17263a32d12e (Bug 1607148) for causing debugger failures CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=288237474&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&searchStr=2e46ec81ee916d1b2c9092aabde10f59d8e37190&revision=17263a32d12e15a390b4bb0cf0e9e3914b784fc1

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=288237474&repo=autoland&lineNumber=426

Backout: https://hg.mozilla.org/integration/autoland/rev/cf1e77de13c485234dc2999c5369a15ca36d41e0

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

jest test failing, asked Stepan if he can look what's going on

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Jason Laster [:jlast] from comment #2)

Yes, the change was intentional. We no longer need to release the object actors .
Watchpoints are now managed in a Map and not on Object actors, which makes a big difference for quality.

Looking at the patch again and it looks weird to me.
We should still want to reach devtools/client/debugger/src/client/firefox/commands.js#90-99 for elements that are displayed in the ObjectInspector right (like the scopes panel, preview, watch expression, ...)?

Unless we're releasing those from some debugger function? But in that case we should be able to remove the releaseActor command, and remove the tests that are asserting the release of those actors.

Comment 9

[Jason Laster [:jlast]]

we can safely remove the watchpoint logic, but should keep the rest

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Stepan, looks like we have a clear view of what's needed now, do you want to pick that up?
No worries if you don't or can!

Comment 11

[Stepan Stava [:stepan]]

I'd prefer not to work on this one if it is ok. I am not that familiar with this topic.

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

sure, no worries, I'll take it then :)

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1607148 - Fix ObjectInspector releaseActors action. r=jlast.

The Reps bundle was directly edited, but the change
wasn't made in the original action file
Furthermore, changes made in Bug 1602804 were too
strict, as we still need to release actors when
we can.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d9287eba3207
Fix ObjectInspector releaseActors action. r=jlast.

Comment 15

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/d9287eba3207

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Is this something we need on Beta for 74 or can this ride 75 to release?

Comment 17

[Nicolas Chevobbe [:nchevobbe]]

not sure, what do you think Jason?

Comment 18

[Pascal Chevrel:pascalc]

P3 and we shipped our last beta, so wontfix for 74.