1592763 - ChangesView should use TargetList API to list to changes coming from actors from all the frames

Status: RESOLVED FIXED

(DevTools :: Inspector: Changes, defect, P1)

Description

[Gabriel Luong [:gl] (ΦωΦ)]

We need to make use of the TargetList API to listen to changes coming from actors from all frames. In the current setup, only changes from the ChangesFront associated with the inspector.currentTarget at the time of Changes panel init would be listening for changes.

Proposed changes in https://searchfox.org/mozilla-central/source/devtools/client/inspector/changes/ChangesView.js#103.

TargetList.watchFrontEvent([TargetList.TYPES.FRAME], "changes", ["add-change", "clear-changes"], (targetType, targetFront, changesFront, eventType, event) {
  if (eventType == "add-change") {
  
  } else if (eventType == "clear-changes") {

  }
});

See https://phabricator.services.mozilla.com/D50973 for prior discussions

Comment 1

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1592763: Use TargetList api for ChangesView. r?ochameau!,r?rcaliman!

Comment 2

[Julian Descottes [:jdescottes]]

Hi Alex, Razvan,

We were discussing about this bug with Daisuke, and I was not sure about the scope of this bug.
Is it to make the ChangesView handle several targets, or simply to make sure it has the correct "currentTarget" when the top-level target changes (similar to what was done for the ReflowTracker).

Thanks!

Comment 3

[Alexandre Poirot [:ochameau]]

It is about using the TargetList, which implies:

• supporting target switching, by:
  • no longer caching the "current target". You may refer to targetList.currentTarget if you need it. But in many case, just switching to use watchTargets helps you drop this reference. (like here, you stop refering to inspector.currentTarget)
  • handling the special case in onTargetAvailable/onTargetDestroyed when isTopLevel is true (like here, adding the will-navigate listener)
• supporting multiple targets:
  • handling the other cases in onTargetAvailable/onTargetDestroyed when isTopLevel is false (like here, register the add-change listener to all the targets)

Note that, the second bullet point (supporting multiple targets) may be done, in some cases, in a followup patch, if that helps.
i.e. you may first refactor existing code to use targetList.watchTargets, and only consider isTopLevel=true case.
I may highlight this option when working on the other less important panels, as a preliminary, easier work to only support target switching.
But here, for the inspector, it is important to support multiple targets.

Conclusion: the patch submitted on phabricator looks great and does what I was expecting :)

Comment 4

[Julian Descottes [:jdescottes]]

(In reply to Alexandre Poirot [:ochameau] from comment #3)

It is about using the TargetList, which implies:

• supporting target switching, by:
  • no longer caching the "current target". You may refer to targetList.currentTarget if you need it. But in many case, just switching to use watchTargets helps you drop this reference. (like here, you stop refering to inspector.currentTarget)
  • handling the special case in onTargetAvailable/onTargetDestroyed when isTopLevel is true (like here, adding the will-navigate listener)
• supporting multiple targets:
  • handling the other cases in onTargetAvailable/onTargetDestroyed when isTopLevel is false (like here, register the add-change listener to all the targets)

Note that, the second bullet point (supporting multiple targets) may be done, in some cases, in a followup patch, if that helps.
i.e. you may first refactor existing code to use targetList.watchTargets, and only consider isTopLevel=true case.
I may highlight this option when working on the other less important panels, as a preliminary, easier work to only support target switching.
But here, for the inspector, it is important to support multiple targets.

Conclusion: the patch submitted on phabricator looks great and does what I was expecting :)

(clearing the ni for Razvan)
Thanks for taking a look! I must say I was confused this morning, I thought this was about the StyleChangeTracker (Bug 1588868). That's why I was more expecting something similar to the ReflowTracker changes, I didn't realize this was another bug :)

Comment 5

[Daisuke Akatsuka (:daisuke)]

Thank you very much for your feedback, Alex!
I'll update the patch a bit to use watchFronts and unwatchFronts, then request reviewing.

Comment 6

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1592763 - Ensure ChangesActor is ready to track CSS changes for each frame. r?ochameau,r?pbro

Depends on D54256

In order for CSS changes to be tracked, the ChangesActor needs to be created and listening. This is done by requesting its corresponding front on the client.

With Fission, we may have multiple iframes, each with its own ChangesActor isolated to the frame's process. In order to ensure there is a corresponding ChangesFront for each frame we move from eager instantiation in the inspector client with _getChangesFront() (this was ok pre-Fission since there was just one ChangesActor/Front) to instantiating a ChangesActor per frame on-demand when necesary.

To do so, we ensure a ChangesActor exists for the respective frame's target before attempting to send the CSS changes to be applied on the server via the corresponding methods on StyleRuleFront.

This change makes it so ChangesActors/Fronts get lazily instantiated only when they're about to be used and avoids needless creation of multiple actors/fronts for each frame if they're not going to be used.

Comment 7

[Phabricator Automation]

Comment on attachment 9111589 [details]
Bug 1592763 - Ensure ChangesActor is ready to track CSS changes for each frame. r?ochameau,r?pbro

Revision D54725 was moved to bug 1599728. Setting attachment 9111589 [details] to obsolete.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bce288d63380
Use TargetList api for ChangesView. r=ochameau,rcaliman

Comment 9

[Cristian Brindusan [:cbrindusan]]

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