Closed Bug 1644193 Opened 4 years ago Closed 4 years ago

Implement StyleSheets listening via the ResourceWatcher API on the actor side

Categories

(DevTools :: Style Editor, enhancement)

Product:
DevTools
Component:
Style Editor
Type:
enhancement

Tracking

(Fission Milestone:M6c, firefox82 fixed)

Status:
RESOLVED FIXED
Milestone:
82 Branch
Project Flags:
Fission Milestone M6c
Tracking Flags:
Tracking Status
firefox82 --- fixed

People

(Reporter: ochameau, Assigned: daisuke)

References

(Blocks 1 open bug)

Details

(Whiteboard: dt-fission-m2-reserve)

Attachments

(7 files, 13 obsolete files)

Bug 1644193: Implement basic server-side stylesheets watcher.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1644193: Make media rules to be resource based.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1644193: Implement update function.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1644193: Reflect added stylesheets.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1644193: Implement addStyleSheet function.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1644193: Make walker.getStyleSheetOwnerNode() to reflect resource watcher.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1644193: Update stylesheeet watcher test.
47 bytes, text/x-phabricator-request
Details | Review

The goal here is to implement the STYLESHEET resource from the actor side.
This is about replicating the current behavior, implemented in bug 1625930.
But instead of having a wrapper on the client side to morph the legacy StyleSheetsFront methods (i.e. the legacy listener code),
we would implement an actor API matching ResourceWatcher API (i.e. watch and unwatch).

This would be about implementing a server side equivalent of this legacy listener code:
https://searchfox.org/mozilla-central/source/devtools/shared/resources/legacy-listeners/stylesheets.js
In a new server side module:
https://searchfox.org/mozilla-central/source/devtools/server/actors/resources/stylesheets.js

This module would typicaly look like this:

const { TYPES } = require("devtools/server/actors/resources/index");

class MyResourceWatcher {
  /**
   * Start watching for all ${MY_RESOURCE_TYPE} related to a given Target Actor.
   * This will notify about existing ${MY_RESOURCE_TYPE}, but also the one created in future.
   *
   * @param TargetActor targetActor
   *        The target actor from which we should observe console messages
   * @param Object options
   *        Dictionary object with following attributes:
   *        - onAvailable: mandatory function
   *          This will be called for each resource.
   */
  constructor(targetActor, { onAvailable }) {
    // In most cases, we already have some helper class which helps observing one resource
    // that we can spawn like this:
    // Note that it may often be easier to merge such `MyResourceListener` into this `MyResourceWatcher` class!
    const listener = new MyResourceListener(
      targetActor.browsingContextID,
      targetActor.window,
      ...  /* whatever is useful for your observation */
    );
    
    // Forward all future resources being observed to the upper layer calling this module,
    // via `onAvailable` callback argument.
    // I'm using EventEmitter API here, but the API may different,
    // based on the platform API we have to use to observe the resource.
    listener.on("one-of-my-resource-is-created", resource => {
      // We have to ensure that each resource object has a valid `resourceType` attribute
      resource.resourceType = TYPES.MY_RESOURCE_TYPE;
      onAvailable([resource]);
    });
    
    // Also forward all resources which already exist when we are calling this method
    // (if any exists)
    const cachedResources = listener.getAllAlreadyExistingOrCachedResources();
    for(const resource of cachedResources) {
      resource.resourceType = TYPES.MY_RESOURCE_TYPE;
    }
    onAvailable(cachedResources);
    
    // Save the listener in order to destroy/stop watching later on.
    this.listener = listener;
  }

  /**
   * Stop watching for ${MY_RESOURCE_TYPE}.
   */
  destroy() {
    if (this.listener) {
      this.listener.destroy();
    }
  }
}
module.exports = MyResourceWatcher;

An important goal here is to emit the exact same resource object that the legacy listener is passing to its onAvailable callback.
Same attributes, same values, ...

Bug 1644185 could be used as a template. As it did this work for PLATFORM_MESSAGE resource type.

The main reason to do this is to be able to start listening to the resource before the page starts loading.
Thanks to the framework work done in bug 1620243, this MyResourceWatcher class will be instantiated before the page starts
loading and possibly as early as the content process just started.
This wasn't the case with legacy actor APIs like WebConsoleActor.startListeners, ThreadActor.attachThread, ...
We were calling these methods too late, only after the frontend is notify about the existance of the target, so, late after the page started loading.

You will also have to register this new module in this registry:
https://searchfox.org/mozilla-central/source/devtools/server/actors/resources/index.js

  • Add a new entry in TYPES object.
  • Register your new resource watcher module into Resources object.

Last but not least, it is probably a good time to review the existing tests for this Resource:
https://searchfox.org/mozilla-central/source/devtools/shared/resources/tests
And ensure that it has a good coverage.
You would especially have to migrate all Client/Front tests, which were testing the backend behavior via targetFront.getFront("myfront").
All these tests will be removed, once we drop the legacy listeners. Because we are going to drop the server API that we no longer use.
Like WebConsoleActor/Front.getCachedMessage(), WebConsoleActor/Front.startListeners(), ThreadActor/Front.sources(), ThreadActor/Front.new-source, ...

Tracking dt-fission-m2 bugs for Fission Nightly (M6c)

Fission Milestone: --- → M6c
Assignee: nobody → daisuke
Status: NEW → ASSIGNED

Hello Alex!

I have started to work on this though, have one question.
I think one of the goal here is removing the stylesheet/stylesheets actor used so far.
As the current legacy-listener for stylesheets is using the stylesheet actor as it is as resource, first, I am thinking about avoid using the actor at the resource.
https://searchfox.org/mozilla-central/rev/ab81b8552f4aa9696a2524f97fdfeb59d4dc31c1/devtools/shared/resources/legacy-listeners/stylesheet.js#18-23

Then, I took a look at the actor, it seems that we can cover the following events if we use onResourceUpdated mechanism.
https://searchfox.org/mozilla-central/rev/ab81b8552f4aa9696a2524f97fdfeb59d4dc31c1/devtools/shared/specs/stylesheets.js#31-46

On the other hand, I’m thinking about how we should approach methods such as toggleDisabled.
https://searchfox.org/mozilla-central/rev/ab81b8552f4aa9696a2524f97fdfeb59d4dc31c1/devtools/shared/specs/stylesheets.js#48-69
For example, adding methods to the resource, or adding an actor that is particular for inputting, or something new mechanism to edit the stylesheet etc etc..
Do you have any ideas?

Incidentally, now, we are calling the methods via the actor since the resource has the actor.
e.g. https://searchfox.org/mozilla-central/rev/ab81b8552f4aa9696a2524f97fdfeb59d4dc31c1/devtools/client/styleeditor/StyleSheetEditor.jsm#573

Flags: needinfo?(poirot.alex)

(In reply to Daisuke Akatsuka (:daisuke) from comment #2)

On the other hand, I’m thinking about how we should approach methods such as toggleDisabled.
https://searchfox.org/mozilla-central/rev/ab81b8552f4aa9696a2524f97fdfeb59d4dc31c1/devtools/shared/specs/stylesheets.js#48-69
For example, adding methods to the resource, or adding an actor that is particular for inputting, or something new mechanism to edit the stylesheet etc etc..
Do you have any ideas?

The resource can still involve actors.
See how we convert an actor's "form" into a front on the client side:
https://searchfox.org/mozilla-central/source/devtools/shared/resources/transformers/console-messages.js
You may also have a look at what we do for storage:
https://phabricator.services.mozilla.com/D86570#change-nMEzqhMgn4Yx

      const { types } = require("devtools/shared/protocol.js");
      if (resourceType == ResourceWatcher.TYPES.COOKIE) {
        // Instantiate the front for this given storage
        resource = types.getType("cookies").read(resource, this.watcher);
        resource.resourceType = ResourceWatcher.TYPES.COOKIE;

(Note that such code should be in a transformer)

I'm fine doing that. I imagine that's the easiest option.
But we might revisit that while working on CDP.
It may be better to have a new actor which would receive a resourceId and implement all these action methods.

Flags: needinfo?(poirot.alex)

Also, I'm open to suggestion if you want to do something else right away!

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

It may be better to have a new actor which would receive a resourceId and implement all these action methods.

Cool, I want to try this!
Thank you very much!

Depends on D87040

Depends on D87041

Depends on D87042

Depends on D87043

Depends on D87044

Depends on D87045

Depends on D87046

Hi Alex!
I have not implemented the resource watcher in server-side yet, but tried to make StyleEditor not depend on StyleSheet actor. So, we can implement the resource watcher without StyleSheet actor. What do you think about this approach? Could you check the patches a bit?
Thanks!

Flags: needinfo?(poirot.alex)

Then, regarding the server-side watcher, I'm thinking like follows.
For example when updating the sheet,

  1. get the watcher of server-side from parentActor at update method in StyleSheets actor. (https://phabricator.services.mozilla.com/D87047#change-o0w9vKkr8LnU)
  2. get the resource that has the same id as resourceId from watcher.
  3. update the stylesheet the resource has (All updating logic will be written in StyleSheets actor).

(In reply to Daisuke Akatsuka (:daisuke) from comment #14)

Hi Alex!
I have not implemented the resource watcher in server-side yet, but tried to make StyleEditor not depend on StyleSheet actor. So, we can implement the resource watcher without StyleSheet actor. What do you think about this approach? Could you check the patches a bit?
Thanks!

Sorry for the delay...

It looks great!
It is a great idea to have introduced resourceId via legacy listener.

Ohterwise, I would suggest to add only one trait, something like supportPerStylesheetRequests, or supportResourceRequests, or something similar.
We are going to land that all at once and so one trait in enough.
I thought about introducing a new actor, but I'm not sure it is a good idea given that we will keep using stylesheets.js and we already have stylesheets.js versus styles.js (and that's already confusing).

Also, do not hesitate to attach these patches to another dependent bug.
It would probably help landing this one sooner and reduce the chance of being backed out.

(In reply to Daisuke Akatsuka (:daisuke) from comment #15)

Then, regarding the server-side watcher, I'm thinking like follows.
For example when updating the sheet,

  1. get the watcher of server-side from parentActor at update method in StyleSheets actor. (https://phabricator.services.mozilla.com/D87047#change-o0w9vKkr8LnU)

About that, you can probably follow this example:
https://searchfox.org/mozilla-central/source/devtools/server/actors/utils/logEvent.js#77-85

  const consoleMessageWatcher = getResourceWatcher(
    targetActor,
    TYPES.CONSOLE_MESSAGE
  );
  if (consoleMessageWatcher) {
    consoleMessageWatcher.onLogPoint(message);
  } else {
    // Bug 1642296: Once we enable ConsoleMessage resource on the server, we should remove onConsoleAPICall
    // from the WebConsoleActor, and only support the ConsoleMessageWatcher codepath.
    targetActor._consoleActor.onConsoleAPICall(message);
  }

You will have consoleMessageWatcher only if the traits in WatcherActor is true, otherwise you will fallback to old codepath.

  1. get the resource that has the same id as resourceId from watcher.

Once we will remove the old codepath, we can probably move BrowsingContextTargetActor.._styleSheetActors to StylesheetWatcher class.
I imagine we have to keep it in the meantime as we probably need it for the old codepath.
Then, once we removed the old codepath, calls to createStyleSheetActor may be refactored into the code I copied in 1.

  1. update the stylesheet the resource has (All updating logic will be written in StyleSheets actor).

I'm still wondering how "update"/"destroy" will be working from the Watcher class. I imagine that it would require cooperation between StyleSheets actor and the Watcher class.

Flags: needinfo?(poirot.alex)

Thank you very much for the feedback, Alex!

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

Ohterwise, I would suggest to add only one trait, something like supportPerStylesheetRequests, or supportResourceRequests, or something similar.
We are going to land that all at once and so one trait in enough.

Ah, indeed! I want to use supportResourceRequests.

I thought about introducing a new actor, but I'm not sure it is a good idea given that we will keep using stylesheets.js and we already have stylesheets.js versus styles.js (and that's already confusing).

Yeah, that was where I was not sure.. As I thought that stylesheets.js is a better name for this purpose in the future, I reused that.
But, if we found a better actor name, want to rename.

Also, do not hesitate to attach these patches to another dependent bug.
It would probably help landing this one sooner and reduce the chance of being backed out.

Ah, nice idea!!
I will file a new bug and move patches to it.

(In reply to Daisuke Akatsuka (:daisuke) from comment #15)

Then, regarding the server-side watcher, I'm thinking like follows.
For example when updating the sheet,

  1. get the watcher of server-side from parentActor at update method in StyleSheets actor. (https://phabricator.services.mozilla.com/D87047#change-o0w9vKkr8LnU)

About that, you can probably follow this example:
https://searchfox.org/mozilla-central/source/devtools/server/actors/utils/logEvent.js#77-85

  const consoleMessageWatcher = getResourceWatcher(
    targetActor,
    TYPES.CONSOLE_MESSAGE
  );
  if (consoleMessageWatcher) {
    consoleMessageWatcher.onLogPoint(message);
  } else {
    // Bug 1642296: Once we enable ConsoleMessage resource on the server, we should remove onConsoleAPICall
    // from the WebConsoleActor, and only support the ConsoleMessageWatcher codepath.
    targetActor._consoleActor.onConsoleAPICall(message);
  }

You will have consoleMessageWatcher only if the traits in WatcherActor is true, otherwise you will fallback to old codepath.

  1. get the resource that has the same id as resourceId from watcher.

Once we will remove the old codepath, we can probably move BrowsingContextTargetActor.._styleSheetActors to StylesheetWatcher class.
I imagine we have to keep it in the meantime as we probably need it for the old codepath.
Then, once we removed the old codepath, calls to createStyleSheetActor may be refactored into the code I copied in 1.

  1. update the stylesheet the resource has (All updating logic will be written in StyleSheets actor).

I'm still wondering how "update"/"destroy" will be working from the Watcher class. I imagine that it would require cooperation between StyleSheets actor and the Watcher class.

Thank you very much for your advice!
I'll implement this while referring to them!

Depends on: 1659589

Comment on attachment 9169970 [details]
Bug 1644193: Handle importing stylesheet from file using resource watcher mechanism to keep consistency.

Revision D87040 was moved to bug 1659589. Setting attachment 9169970 [details] to obsolete.

Attachment #9169970 - Attachment is obsolete: true

Comment on attachment 9169971 [details]
Bug 1644193: Get style-applied event via resource watcher.

Revision D87041 was moved to bug 1659589. Setting attachment 9169971 [details] to obsolete.

Attachment #9169971 - Attachment is obsolete: true

Comment on attachment 9169972 [details]
Bug 1644193: Get property-change event via resource watcher.

Revision D87042 was moved to bug 1659589. Setting attachment 9169972 [details] to obsolete.

Attachment #9169972 - Attachment is obsolete: true

Comment on attachment 9169973 [details]
Bug 1644193: Get media-rules-changed event via resource watcher.

Revision D87043 was moved to bug 1659589. Setting attachment 9169973 [details] to obsolete.

Attachment #9169973 - Attachment is obsolete: true

Comment on attachment 9169974 [details]
Bug 1644193: Implement toggleDisabled in stylesheets actor.

Revision D87044 was moved to bug 1659589. Setting attachment 9169974 [details] to obsolete.

Attachment #9169974 - Attachment is obsolete: true

Comment on attachment 9169975 [details]
Bug 1644193: Implement getText in stylesheets actor.

Revision D87045 was moved to bug 1659589. Setting attachment 9169975 [details] to obsolete.

Attachment #9169975 - Attachment is obsolete: true

Comment on attachment 9169976 [details]
Bug 1644193: Implement getMediaRules in stylesheets actor.

Revision D87046 was moved to bug 1659589. Setting attachment 9169976 [details] to obsolete.

Attachment #9169976 - Attachment is obsolete: true

Comment on attachment 9169977 [details]
Bug 1644193: Implement update method in stylesheets actor.

Revision D87047 was moved to bug 1659589. Setting attachment 9169977 [details] to obsolete.

Attachment #9169977 - Attachment is obsolete: true

Depends on D88540

Comment on attachment 9172581 [details]
Bug 1644193: Remove error event listener that set to stylesheet actor.

Revision D88531 was moved to bug 1659589. Setting attachment 9172581 [details] to obsolete.

Attachment #9172581 - Attachment is obsolete: true
Attachment #9172965 - Attachment is obsolete: true

Depends on D88540

Depends on D89277

Depends on D89280

Depends on D89281

Depends on D89283

Whiteboard: dt-fission-m2-mvp → dt-fission-m2-reserve
Attachment #9173948 - Attachment is obsolete: true

Depends on D89283

Depends on: 1664842

Comment on attachment 9172962 [details]
Bug 1644193: Introduce async watch function.

Revision D88759 was moved to bug 1664842. Setting attachment 9172962 [details] to obsolete.

Attachment #9172962 - Attachment is obsolete: true
Blocks: 1664941
Pushed by dakatsuka.birchill@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/90db7f8e4714 Make media rules to be resource based. r=ochameau https://hg.mozilla.org/integration/autoland/rev/9ef3631f433e Implement basic server-side stylesheets watcher. r=ochameau https://hg.mozilla.org/integration/autoland/rev/67b784c6704e Implement update function. r=ochameau https://hg.mozilla.org/integration/autoland/rev/4dbdba873a86 Reflect added stylesheets. r=ochameau https://hg.mozilla.org/integration/autoland/rev/794eeb19a327 Implement addStyleSheet function. r=ochameau,devtools-backward-compat-reviewers https://hg.mozilla.org/integration/autoland/rev/387f3ff45224 Make walker.getStyleSheetOwnerNode() to reflect resource watcher. r=ochameau https://hg.mozilla.org/integration/autoland/rev/09212dec128b Update stylesheeet watcher test. r=ochameau
Regressions: 1665073

Comment on attachment 9174609 [details]
Bug 1644193: Connect style actor and stylesheet watcher.

Revision D89558 was moved to bug 1664941. Setting attachment 9174609 [details] to obsolete.

Attachment #9174609 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: