1036949 - New API: MarkupView customization

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

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

I tried to extend/customize the existing MarkupView (used to render content of the Inspector panel) and append (as a test) an xpath tooltip for every element.

There isn't currently a way how to do it properly and so here is a suggestion for new hooks.

Use case:
- Append a tooltip for ever element in the Inspector panel
- modify/replace generated content that is used to render individual DOM nodes in the panel

API:

1) Firing an event when rendering happens
Rendering of the MarkupView is done through MarkupView.template() method. This methods gets proper template from markup-view.xhtml, clones the DOM structure and fills it with passed data (using Templater.jsm)

This method could fire an event that can be handled by extensions to modify generated DOM structure. See an example:

template: function(aName, aDest, aOptions={stack: "markup-view.xhtml"}) {
  let node = this.doc.getElementById("template-" + aName).cloneNode(true);
  node.removeAttribute("id");
  template(node, aDest, aOptions);

  // Fire a new event
  this._inspector.emit("render-node", node, aName, aDest, aOptions);

  return node;
},

It's fired through the InspectorPanel object, since it might be a bit more stable API to handle events on the panel than on objects that represent its internal structure.


2) Registering listeners to the Inspector panel
In order to handle the "render-node" event, extensions need to register a listener to the InspectorPanel soon enough - before any rendering happens. Ideally just after the panel's constructor is executed and panel instance available.

It isn't easy to handle this moment since panels are instantiated within Tool.build method and the reference is not even returned from this method. Return value of the method is a promise that might be resolved too late (it actually is resolved too late) and extensions might miss some events.

I can see the following scenario as a solution:

1) The build method return directly a reference to the instantiated panel object. 
2) Toolbox.loadTool() fires an event and passes the panel object to it.
3) Toolbox.loadTool() method calls panel.open()

See an example:

let panel = definition.build(iframe.contentWindow, this);

// Extensions can handle this event and register any
// listeners to created panel (or other initialization steps that should
// happen at the very beginning).
this.emit(id + "-build", panel);

let built = panel.open();
promise.resolve(built).then((panel) => {
  ...
  // An existing event, fired when panel's iframe is loaded.
  this.emit(id + "-ready", panel);
  ...
);


Thoughts?
Honza

Comment 1

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

Here is related commit in Firebug.next:
https://github.com/firebug/firebug.next/commit/2565ff1130bfe74bc6a91791b7a4cbd4285a8b9a
(hacking the current API)

Honza

Comment 2

[Patrick Brosset <:pbro>]

On the specific subject of tooltips, know that there is a whole API related to tooltips in place already. It hasn't been made with extensions in mind for now, and it's not easily possible to add new types of tooltips to the markup-view yet, but that's all fixable with minimal changes.

I think we should take a rather generic approach to when/which/how tooltips are displayed throughout the inspector (maybe throughout the devtools). If you take a look at the style-inspector (rule-view, computed-view), it's actually pretty ok, there's a couple of event listeners added to the whole sidebar that detect mouse movements on nodes of the sidebar. For each node hovered, a generic 'getNodeInfo' function is called which returns an object like: {type: 'selector', value: 'div.something'} or {type: 'property', name: 'color', value: 'blue'}, etc ...
Based on the node info returned, we then have a simple logic that decides which tooltip to show (if any) for that hovered node.
That's what we use for the image background tooltip, font-family tooltip, soon the cubic-bezier tooltip, but we're also using it for the css transform highlighter. See style-inspector-overlays.js.

If we did something like this but for the whole inspector, and added a hook to the logic that decided which tooltip to show, it would be very easy for an extension to add, for example, a text tooltip that contained the xpath to the node when the mouse would hover over tag names for instance.

Such a hook could look like this:

function onHover(nodeInfo, tooltip) {
  if (nodeInfo.context === "markupview" && nodeInfo.type === "tag") {
    tooltip.setTextContent(...the xpath...);
  }
}

Now, when it comes to altering the content of the markup-view, I'm not sure what's the best API. What you proposed seems ok to me so far.
Isn't backward compatibility going to become a tough challenge if we need to make changes to the structure of the markup-view? I'm not that familiar with the firefox extensions ecosystem but I'm sure that's the kind of questions that's been asked and answered a million times.

On one hand, it'd be really cool if extensions would be able to change almost everything that makes up a node in the markup-view, almost provide their own template, but on the other hand, if they did, a lot of our code would break as it makes assumptions about what's in there.
I think we need to rework a little bit how the MarkupContainer class, ElementEditor class and the view itself interact together so that it would be easy to replace the view with something else, and just re-wire all the calls to the ElementEditor so that you can still support things like editing attributes.

Maybe I'm sidetracking a bit here, but I'd like to provide as much customization capabilities as possible without constraining how much we can evolve this tool in the future.

Comment 3

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

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #2)
> I think we should take a rather generic approach to when/which/how tooltips
> are displayed throughout the inspector (maybe throughout the devtools). If
Yes, it makes perfect sense.

> you take a look at the style-inspector (rule-view, computed-view), it's
> actually pretty ok, there's a couple of event listeners added to the whole
> sidebar that detect mouse movements on nodes of the sidebar. For each node
> hovered, a generic 'getNodeInfo' function is called which returns an object
> like: {type: 'selector', value: 'div.something'} or {type: 'property', name:
> 'color', value: 'blue'}, etc ...
> Based on the node info returned, we then have a simple logic that decides
> which tooltip to show (if any) for that hovered node.
> That's what we use for the image background tooltip, font-family tooltip,
> soon the cubic-bezier tooltip, but we're also using it for the css transform
> highlighter. See style-inspector-overlays.js.
I am also seeing TooltipsOverlay and Tooltip objects that could be
used as the basics for the final extensible APIs.

> If we did something like this but for the whole inspector, and added a hook
> to the logic that decided which tooltip to show, it would be very easy for
> an extension to add, for example, a text tooltip that contained the xpath to
> the node when the mouse would hover over tag names for instance.
> 
> Such a hook could look like this:
> 
> function onHover(nodeInfo, tooltip) {
>   if (nodeInfo.context === "markupview" && nodeInfo.type === "tag") {
>     tooltip.setTextContent(...the xpath...);
>   }
> }
Nit: I would also pass the target node (the current one under the cursor).

> Now, when it comes to altering the content of the markup-view, I'm not sure
> what's the best API. What you proposed seems ok to me so far.
> Isn't backward compatibility going to become a tough challenge if we need to
> make changes to the structure of the markup-view? I'm not that familiar with
> the firefox extensions ecosystem but I'm sure that's the kind of questions
> that's been asked and answered a million times.
Precisely. My feeling is that, of course, we should always try keep API
backward compatible, but at the same time we shouldn't be afraid
to change it and evolve into better infrastructure if there is a reason
for it. Extension developers who want 100% stable API should solely use
Add-ons SDK high level API that are there for that matter.

> On one hand, it'd be really cool if extensions would be able to change
> almost everything that makes up a node in the markup-view, almost provide
> their own template, but on the other hand, if they did, a lot of our code
> would break as it makes assumptions about what's in there.
> I think we need to rework a little bit how the MarkupContainer class,
> ElementEditor class and the view itself interact together so that it would
> be easy to replace the view with something else, and just re-wire all the
> calls to the ElementEditor so that you can still support things like editing
> attributes.
We can wait for specific use cases and requirements from extension developers,
but I would be curious what kind of improvements do you have in mind (just making
the code more loosely coupled?).
It would be nice if e.g. inline editors are also extensible
(e.g. allowing creating of new visual editors, auto-completion, etc.).

> Maybe I'm sidetracking a bit here, but I'd like to provide as much
> customization capabilities as possible without constraining how much we can
> evolve this tool in the future.
I created a new bug specifically for tooltips: bug 1037386

Honza

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Jan Honza Odvarko [:Honza] from comment #0)
> 2) Registering listeners to the Inspector panel
> In order to handle the "render-node" event, extensions need to register a
> listener to the InspectorPanel soon enough - before any rendering happens.
> Ideally just after the panel's constructor is executed and panel instance
> available.
> 
> It isn't easy to handle this moment since panels are instantiated within
> Tool.build method and the reference is not even returned from this method.
> Return value of the method is a promise that might be resolved too late (it
> actually is resolved too late) and extensions might miss some events.
> 
> I can see the following scenario as a solution:
> 
> 1) The build method return directly a reference to the instantiated panel
> object. 
> 2) Toolbox.loadTool() fires an event and passes the panel object to it.
> 3) Toolbox.loadTool() method calls panel.open()
> 
> See an example:
> 
> let panel = definition.build(iframe.contentWindow, this);
> 
> // Extensions can handle this event and register any
> // listeners to created panel (or other initialization steps that should
> // happen at the very beginning).
> this.emit(id + "-build", panel);
> 
> let built = panel.open();
> promise.resolve(built).then((panel) => {
>   ...
>   // An existing event, fired when panel's iframe is loaded.
>   this.emit(id + "-ready", panel);
>   ...
> );

What happens if the extension is registered after the initial event has fired?  This will hit an early condition in loadTool and not fire the event again.  Is this a situation that will come up?  If so, it seems like the API that an extension would really want is:

onceToolOpened("inspector").then((panel) => {
  /* Either inspector has never been opened and this will fire when it does, or it has already been opened and it will fire immediately */
});

And this would fire earlier than we currently do as you suggest (before the open() finishes).

Comment 5

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

(In reply to Brian Grinstead [:bgrins] from comment #4)
> What happens if the extension is registered after the initial event has
> fired?  This will hit an early condition in loadTool and not fire the event
> again.  Is this a situation that will come up?
Good question, this could happen when bootstrapped extensions are installed
(or enabled) in the middle of Firefox session.

> If so, it seems like the API
> that an extension would really want is:
> 
> onceToolOpened("inspector").then((panel) => {
>   /* Either inspector has never been opened and this will fire when it does,
> or it has already been opened and it will fire immediately */
> });
> 
> And this would fire earlier than we currently do as you suggest (before the
> open() finishes).
Yes, I really like the concept of API designed and based on promises like:
getItWhenReady(something).then(onReady);

---

I think we should still support events that allow customization at the right moment (usually doing it soon enough is the key). Event driven programming is well known and understood concept and btw also used as a simple asynchronous programing model (like e.g. NodeJS).

In this particular case, it's about initialization sequence. Extensions that are already installed need to perform various initialization steps (like, e.g. registering hooks and listeners) and those need to be usually done on time. For example, an extension hooking console logs shouldn't miss any logs - not even those that happen at the very beginning.

Extensions that are installed in the middle of the Firefox session are suppose to miss events, of course. Using the previous example, Console logs will be customized since the extension is installed. These extensions need to be able to access existing API and register all hooks and listeners a little bit later.

Initialization is usually done in more steps (the more asynchronous the initialization is the more steps). Let's see the current toolbox panel as an example, I believe that it should be possible to handle following moments:

1) get the panel instance as soon as it's instantiated. This allows to register listeners related to the panel and also set various flags and properties/objects on the panel instance - in order to customize it.

2) get panel frame element as soon as it's created. This allows to move the element on another place in the DOM and register listeners (e.g. load).

3) get 'ready' event saying that the panel instance finished its initialization process and is ready to use. This step can have further sub-steps depending on the panel e.g. the Console panel is doing a few more asynchronous things. Extensions need to be able to register listeners for all these panel-related asynchronous steps.


So, to handle these initialization steps, extensions should be able to register listeners for:

toolbox.on(panelId + "-created", PanelOverlay.onCreated);
toolbox.on(panelId + "-build", PanelOverlay.onBuild);
toolbox.on(panelId + "-ready", PanelOverlay.onReady);

let PanelOverlay = {
  onCreated: function(panel) {}
  onFrame: function(panel, frame) {}
  onReady: function(panel) {}
}

This model is simple and familiar to developers.


Of course, experienced developers (especially with promises concept) can build (or use an existing) additional API on top of the events if necessary:

getPanelWhenCreated(panelId).then(PanelOverlay.onCreated);
getFrameWhenCreated(panelId).then(PanelOverlay.onFrame);
getPanelWhenReady(panelId).then(PanelOverlay.onReady);


Honza

Comment 6

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

I created a new bug report for implementing the new toolbox panel events (point #2 in comment #0).
Bug 1059727 - New API: Toolbox panel initialization events

It's blocking the markup view customization API,
but out of scope of this report.

Honza

Comment 7

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

As part of the work on FireQuery [1] I need to customize element rendering in the Inspector panel and append info about jQuery data [2]. You can see a screenshot with working prototype on the home page.

I had to do two things to display custom data in the Inspector panel:
1) I needed an event that is fired from MarkupView.prototype.template (at the end of the method, when the element is rendered). Handling the event allowed to append the custom UI.

2) In order to render additional data I had to also make sure they are sent from the backend. I fired another event from NodeActor.prototype.form (again at the end) that allows to create new fields in the actor 'form' object (jQuery data in this specific case).

What do you think?
Is there a better way how to design the API?

We could potentially get some inspiration from the Console panel API.
Specifically, ObjectRenderers (front end) and Previewers (back end) [3]

Honza


[1] https://github.com/firebug/firequery
[2] http://api.jquery.com/data/
[3] https://developer.mozilla.org/en-US/docs/Tools/Web_Console/Custom_output

Comment 8

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

Patrick, what do you think?

Honza

Comment 9

[Patrick Brosset <:pbro>]

(In reply to Jan Honza Odvarko [:Honza] from comment #7)
> As part of the work on FireQuery [1] I need to customize element rendering
> in the Inspector panel and append info about jQuery data [2]. You can see a
> screenshot with working prototype on the home page.
> 
> I had to do two things to display custom data in the Inspector panel:
> 1) I needed an event that is fired from MarkupView.prototype.template (at
> the end of the method, when the element is rendered). Handling the event
> allowed to append the custom UI.
This approach sounds good, but I have one worry: there are discussions about refactoring our panels so they all share the same way of generating and updating their UIs. One candidate for this is React, but it could go any other way.
If we went for React after Firequery (and others) started using this approach, we'd have to find a way to still support this extension API, and frankly, I don't see a way to do this.
I'm not saying we should wait until we refactor all the things™, but maybe that we should try and find an API that couples inspector addons less with the way the inspector generates its markup.
What about having events *after* the markup has been generated. This way, no matter how we do generate it or update it, we can always trigger that event for addons to use, and pass a list of the nodes that have been rendered.
Would that work for you?
We already have the inspector event "inspector-updated" that is fired every time something changes in the inspector, or the event "markupmutation" fired after markup changes.
I don't think it should be too hard to either use one of them or add a new one that fires once at page load and then every time a new node is displayed, and pass a reference to the corresponding markup-view DOM along.

> 2) In order to render additional data I had to also make sure they are sent
> from the backend. I fired another event from NodeActor.prototype.form (again
> at the end) that allows to create new fields in the actor 'form' object
> (jQuery data in this specific case).
That would work indeed, how do you access this new property from the client-side though? I don't think you have access to the NodeFront in the template method. How do you know which node is being rendered?
Also, I guess you have to do something like nodeFront._form.myJqueryData to access the property, right?
I see 2 different options:
- we keep your approach, which I think is good, but we make it a little more "official" by adding some form property called "extraProperties" or something like this, where addons are free to add their stuff. And we make a getter for this in NodeFront,
- or we don't make devtools actors extensible but instead require addons to register their own actors to return the data they need. That's more code, and more processing time (one more round-trip), but also probably more flexible. In this case, you'd have to register a FireQueryActor that has one method: getJQueryDataForNode(nodeActor). It would take a NodeActor as input, and using it, would just return the jquery data you need to the client.
This means, this would be an extra async call from the client, and the jquery data would appear a bit after the node has been rendered in the UI.

I'm not particularly for one or the other solution. I think I like your proposal to make actors extensible.

Comment 10

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

Attachment: bug1036949-1.patch

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #9)
> If we went for React after Firequery (and others) started using this
> approach, we'd have to find a way to still support this extension API, and
> frankly, I don't see a way to do this.
> I'm not saying we should wait until we refactor all the things™, but maybe
> that we should try and find an API that couples inspector addons less with
> the way the inspector generates its markup.
> What about having events *after* the markup has been generated. This way, no
> matter how we do generate it or update it, we can always trigger that event
> for addons to use, and pass a list of the nodes that have been rendered.
> Would that work for you?
> We already have the inspector event "inspector-updated" that is fired every
> time something changes in the inspector, or the event "markupmutation" fired
> after markup changes.
> I don't think it should be too hard to either use one of them or add a new
> one that fires once at page load and then every time a new node is
> displayed, and pass a reference to the corresponding markup-view DOM along.
I have been looking at how we could fire an event after the inspector generates the markup and pass list of rendered nodes. I though that instead of firing "markupview-render" event (from within MarkupView.template method, see my patch) we could collect these nodes and use it in "inspector-updated". But, it seems that this event is fired *before* nodes are rendered - before the "markupview-render" event (in some cases e.g. when a container is expanded). 

"markupmutation" is not useful since it's fired only if mutation happens, while we need to handle every node 'render' (include initial rendering).

So, the question is, is there a chance to collect all rendered nodes (within MarkupView.template) and fired a new event (with the list of collected nodes) immediately (synchronously) at the end of every rendering process?

In any case, refactoring the entire view on top of ReactJS (and I support it) will represent significant change and I think that even the way how the view should be customized will change. I.e. other modules/extensions should also use ReactJS API to customize the rendering and not manipulate the DOM directly. So, perhaps we shouldn't wait (it could be pretty long time till the view is ReactJS based anyway) and offer working API now and see what the future brings...

> 
> > 2) In order to render additional data I had to also make sure they are sent
> > from the backend. I fired another event from NodeActor.prototype.form (again
> > at the end) that allows to create new fields in the actor 'form' object
> > (jQuery data in this specific case).
> That would work indeed, how do you access this new property from the
> client-side though? I don't think you have access to the NodeFront in the
> template method. How do you know which node is being rendered?
> Also, I guess you have to do something like nodeFront._form.myJqueryData to
> access the property, right?
> I see 2 different options:
> - we keep your approach, which I think is good, but we make it a little more
> "official" by adding some form property called "extraProperties" or
> something like this, where addons are free to add their stuff. And we make a
> getter for this in NodeFront,
> - or we don't make devtools actors extensible but instead require addons to
> register their own actors to return the data they need. That's more code,
> and more processing time (one more round-trip), but also probably more
> flexible. In this case, you'd have to register a FireQueryActor that has one
> method: getJQueryDataForNode(nodeActor). It would take a NodeActor as input,
> and using it, would just return the jquery data you need to the client.
> This means, this would be an extra async call from the client, and the
> jquery data would appear a bit after the node has been rendered in the UI.
> 
> I'm not particularly for one or the other solution. I think I like your
> proposal to make actors extensible.

I actually used both approaches in FireQuery and both work great together.

- FireQuery backend actor adds one custom prop into the NodeActor's form. A boolean indicating whether there are jQuery data or not.
https://github.com/firebug/firequery/blob/master/lib/firequery-actor.js#L315

- The front access the boolean prop and renders an icon if it's true.
https://github.com/firebug/firequery/blob/master/lib/inspector-overlay.js#L207

- The actual jQuery data are fetched from the backend using a custom FireQuery actor on-demand when the user clicks the icon to see a tooltip with details.
https://github.com/firebug/firequery/blob/master/lib/inspector-overlay.js#L285


There are simple new API for setting custom properties on NodeActor's form.

Please see the attached patch that shows what I have in mind.


Honza

Comment 11

[Patrick Brosset <:pbro>]

(In reply to Jan Honza Odvarko [:Honza] from comment #10)
> I have been looking at how we could fire an event after the inspector
> generates the markup and pass list of rendered nodes. I though that instead
> of firing "markupview-render" event (from within MarkupView.template method,
> see my patch) we could collect these nodes and use it in
> "inspector-updated". But, it seems that this event is fired *before* nodes
> are rendered - before the "markupview-render" event (in some cases e.g. when
> a container is expanded). 
Alright, didn't know this.

> "markupmutation" is not useful since it's fired only if mutation happens,
> while we need to handle every node 'render' (include initial rendering).
That's right.

> So, the question is, is there a chance to collect all rendered nodes (within
> MarkupView.template) and fired a new event (with the list of collected
> nodes) immediately (synchronously) at the end of every rendering process?
Yes there is another way.
The right function to instrument here is MarkupView.prototype.importNode.
It starts with 2 if conditions that do early returns, we don't need to worry about those, and after this, it creates a MarkupContainer (which type depends on the type of the node).

If, right after this, we did: 'this.emit("container-imported", container);' then it'd be easy for extensions to listen to that event and be alerted whenever a new node was displayed.
This should work for initial page load, expanding nodes, new node insertions, ...

Extensions that are installed after the markup-view was displayed should first loop over 'markupView._containers' (for which we could expose a getter).

Each MarkupContainer object returned will have a 'elt' property that points to the DOM element of that given node. So extensions could use this to alter it.

Also, MarkupContainer objects have a 'node' property that points to the NodeFront object, which is useful too.

> In any case, refactoring the entire view on top of ReactJS (and I support
> it) will represent significant change and I think that even the way how the
> view should be customized will change. I.e. other modules/extensions should
> also use ReactJS API to customize the rendering and not manipulate the DOM
> directly. So, perhaps we shouldn't wait (it could be pretty long time till
> the view is ReactJS based anyway) and offer working API now and see what the
> future brings...
Yeah, I agree, let's not block this for potential long-term refactoring.

Comment 12

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

Attachment: bug1036949-2.patch

Thanks for the feedback Patrick, utilizing importNode() method works well.

New patch that introduces "container-created" event and helper API for setting custom form properties attached.

Honza

Comment 13

[Patrick Brosset <:pbro>]

Comment on attachment 8606265 [details] [diff] [review]
bug1036949-2.patch

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

::: toolkit/devtools/server/actors/inspector.js
@@ +284,5 @@
> +    };
> +
> +    // Fire an event so, other modules can create its own properties
> +    // that should be passed to the client (within the form.props field).
> +    DebuggerServer.emit("domnode-form", this, form);

Is this the agreed way to hook server-side addons? I can see one other occurrence of this in main.js:
DebuggerServer.emit("disconnected-from-child:"...)
but it's the first time I encounter this and I just want to make sure DebuggerServer is the right module we want addons to require to catch these events.

Shouldn't we update protocol.js instead to make it emit events when forms are being created there? I'm just thinking we'll most likely need to do something similar with other types of actors, and we'll end up adding the same helper and event in many places.

@@ +899,5 @@
>    }),
>  
> +  // Accessors for custom properties.
> +
> +  getProperty: function(name) {

Can you rename these accessors to:
getFormProperty
hasFormProperty
get formProperties
to avoid confusing these with DOM properties.

@@ +904,5 @@
> +    return this._form.props ? this._form.props[name] : null;
> +  },
> +
> +  hasProperty: function(name) {
> +    return this._form.props ? (name in this._form.props) : null;

s/null/false

Comment 14

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

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #13)
> Comment on attachment 8606265 [details] [diff] [review]
> bug1036949-2.patch
> 
> Review of attachment 8606265 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/server/actors/inspector.js
> @@ +284,5 @@
> > +    };
> > +
> > +    // Fire an event so, other modules can create its own properties
> > +    // that should be passed to the client (within the form.props field).
> > +    DebuggerServer.emit("domnode-form", this, form);
> 
> Is this the agreed way to hook server-side addons? I can see one other
> occurrence of this in main.js:
> DebuggerServer.emit("disconnected-from-child:"...)
> but it's the first time I encounter this and I just want to make sure
> DebuggerServer is the right module we want addons to require to catch these
> events.
> 
> Shouldn't we update protocol.js instead to make it emit events when forms
> are being created there? I'm just thinking we'll most likely need to do
> something similar with other types of actors, and we'll end up adding the
> same helper and event in many places.

Good point, NIing Brian...

Honza

Comment 15

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

Brian: quick context info. The "domnode-form" event allows appending new properties into actor's form. These custom properties can be appended by other modules/extensions. It's currently done only for the "domnode" (NodeActor) actor, but perhaps we could have more generic event that allows to append custom actor form properties to any registered actor...?

Honza

Comment 16

[Brian Grinstead [:bgrins]]

(In reply to Jan Honza Odvarko [:Honza] from comment #15)
> Brian: quick context info. The "domnode-form" event allows appending new
> properties into actor's form. These custom properties can be appended by
> other modules/extensions. It's currently done only for the "domnode"
> (NodeActor) actor, but perhaps we could have more generic event that allows
> to append custom actor form properties to any registered actor...?
> 
> Honza

Redirecting to jryans / dcamp to see if they have any ideas (see also Comment 14)

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I think it could be quite powerful for protocol.js to offer a generic way for any actor method to be intercepted / modified / ignored.

The current event mechanism in the patch feels a little odd, mostly since I usually expect events to be one way "this happened" communication, not a two way mutable communication where you make changes to an object from the event.  But, it works in a pinch when there's nothing else.

Honza, do you (or others) anticipate needing to extend / override many actor methods like this in the future?

Comment 18

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

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #17)
> I think it could be quite powerful for protocol.js to offer a generic way
> for any actor method to be intercepted / modified / ignored.
Yes

> The current event mechanism in the patch feels a little odd, mostly since I
> usually expect events to be one way "this happened" communication, not a two
> way mutable communication where you make changes to an object from the
> event.  But, it works in a pinch when there's nothing else.
Are there any other options (in terms of API design)?

Btw. isn't this 'two way communication' already in place anyway? (every
time an object argument is passed into an event handler)

> Honza, do you (or others) anticipate needing to extend / override many actor
> methods like this in the future?
Yes, I think this could happen relatively often, basically every time a module/extension
wants to get additional data from the backend without introducing whole new
actor (that would send that info).

Besides attaching new props to the NodeActor I can imagine e.g. TabActor or SourceActor, etc. having new data in the form responses...

How the protocol should help here? I don't see a central place where the 'form' method of individual actors would be executed. E.g. SourceActor.from is called from SourceActor.onNewSource, TabActor.form is called from RootActor.onGetTab, etc.

Of course, it's already possible to intercept sent/received packets, analyze them and append new stuff, but it sounds too low level...

Honza

Comment 19

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Jan Honza Odvarko [:Honza] from comment #18)
> > The current event mechanism in the patch feels a little odd, mostly since I
> > usually expect events to be one way "this happened" communication, not a two
> > way mutable communication where you make changes to an object from the
> > event.  But, it works in a pinch when there's nothing else.
> Are there any other options (in terms of API design)?

Well, I think a general filtering / overriding mechanism is a cleaner option, but it doesn't exist yet!  So, in terms of things that exist, this seems okay for now.

> Btw. isn't this 'two way communication' already in place anyway? (every
> time an object argument is passed into an event handler)

I suppose so.  It was just more of a feeling.  I don't recall seeing much of this *in DevTools code* so far, that's all.  "Typically" in most DevTools cases I've seen, the object receiving the event just does its own work in response and doesn't need to mutate part of the event data.

I guess the part that feels strange is that we're attaching a whole new method purely to receive data from anyone who might listen to the event.  I'm not saying it's bad...  Just surprising to see, I guess.

I hope we never decide to change to an async event emitter (like we did with promises), since that would clearly break this pattern.  But, I don't think anyone is planning to do that... :)

> > Honza, do you (or others) anticipate needing to extend / override many actor
> > methods like this in the future?
> Yes, I think this could happen relatively often, basically every time a
> module/extension
> wants to get additional data from the backend without introducing whole new
> actor (that would send that info).
> 
> Besides attaching new props to the NodeActor I can imagine e.g. TabActor or
> SourceActor, etc. having new data in the form responses...
> 
> How the protocol should help here? I don't see a central place where the
> 'form' method of individual actors would be executed. E.g. SourceActor.from
> is called from SourceActor.onNewSource, TabActor.form is called from
> RootActor.onGetTab, etc.
> 
> Of course, it's already possible to intercept sent/received packets, analyze
> them and append new stuff, but it sounds too low level...

I was imagining that protocol.js could offer a kind of "filtering extension" API as part of every protocol.js actor.  Using this case, as an example:

1. Add-on registers that it wants to filter "NodeActor.form"
2. Each time NodeActor.form is called, protocol.js also calls the add-on's code, giving it a chance to extend / modify the result

It's not specific to forms or NodeActor at all, as this would be available for any protocol.js supported method.  It would support any number of filtering users / add-ons that want to override the same thing.

Anyway, this current patch only affects the server side (not the RDP), so I think the current version is okay for now.  In the future if we add a better actor extension model (like the one I mention above), we can convert this case to use it as well (though add-ons would also need to be updated).  I filed bug 1166315 about my idea.

Comment 20

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

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #19)
> Anyway, this current patch only affects the server side (not the RDP), so I
> think the current version is okay for now.
Great!

One more question, the "domnode-form" event is fired through DebuggerServer object. Does this also sound ok? I am not sure if there is a better object we could emit the event on, but it's definitely easy to access the DebuggerServer instance for extensions...

> +    // Fire an event so, other modules can create its own properties
> +    // that should be passed to the client (within the form.props field).
> +    DebuggerServer.emit("domnode-form", this, form);

Comment 21

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Jan Honza Odvarko [:Honza] from comment #20)
> One more question, the "domnode-form" event is fired through DebuggerServer
> object. Does this also sound ok? I am not sure if there is a better object
> we could emit the event on, but it's definitely easy to access the
> DebuggerServer instance for extensions...
> 
> > +    // Fire an event so, other modules can create its own properties
> > +    // that should be passed to the client (within the form.props field).
> > +    DebuggerServer.emit("domnode-form", this, form);

Hmm, it's kind of an odd choice, since the DebuggerServer itself has little to do with the event, and it just happens to be easy for add-ons to reach.  Since actors are created lazily and there are separate instances per connection, I can see how it would be hard to get the specific instance of an actor from the add-on.

But, you could at least emit from the Actor constructor, right?  That keeps things off of the server more closely tied to the actor involved.  So, something like:

// in this patch:

NodeActor.emit("domnode-form", this, form);

// in the add-on:

const { NodeActor } = require("devtools/server/actors/inspector");

NodeActor.on("domnode-form", ...);

I would think that would still be easy enough to do from the add-on.

Add-ons will have to be careful to keep actor instances from multiple connections separate from each other if the event handlers keep any per-actor state, but it's at least possible to tell actors apart since you're emitting "this".

Comment 22

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

Attachment: bug1036949-3.patch

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #21)
> But, you could at least emit from the Actor constructor, right?
Yes, excellent, I have been also thinking about this.

The only thing is that instead of:

NodeActor.emit("event-type", ...);

The actor needs something like:

events.emit(NodeActor, "event-type", ...);

(actors is not an emitter atm and since the event name isn't a registered RDP event it won't be propagated to the client)


New patch attached:
- Event renamed to "form" and emitted through NodeActor ctor
- API for custom properties renamed

Try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e443e48f3ba8


Honza

Comment 23

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8608703 [details] [diff] [review]
bug1036949-3.patch

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

The actor extension approach seems reasonable enough for now, but that's all I've reviewed.

Patrick review is still needed here to be sure it makes sense for the markup view's needs.

::: toolkit/devtools/server/actors/inspector.js
@@ +901,5 @@
>    }),
>  
> +  // Accessors for custom form properties.
> +
> +  getFormProperty: function(name) {

This seems redundant since you can get all the properties with the `formProperties` getter below.

@@ +905,5 @@
> +  getFormProperty: function(name) {
> +    return this._form.props ? this._form.props[name] : null;
> +  },
> +
> +  hasFormProperty: function(name) {

This seems redundant since you can get all the properties with the `formProperties` getter below.

Comment 24

[Patrick Brosset <:pbro>]

Comment on attachment 8608703 [details] [diff] [review]
bug1036949-3.patch

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

Thanks JRyans for filing bug 1166315, this would be a much better (read generic) solution to this requirement. In the meantime, I don't see a reason to hold off this patch, migrating potential addons that would use this to the new way after bug 1166315 seems easy enough.

Comment 25

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

Great, thanks!

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #23)
> @@ +905,5 @@
> > +  getFormProperty: function(name) {
> > +    return this._form.props ? this._form.props[name] : null;
> > +  },
> > +
> > +  hasFormProperty: function(name) {
> 
> This seems redundant since you can get all the properties with the
> `formProperties` getter below.
Can we keep it there, it's actually little handy API that also checks the _form.props array (and the array isn't created until there is custom prop set - to safe RDP traffic). It also gives extension developers a simple way to detect this feature.

Honza

Comment 26

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Jan Honza Odvarko [:Honza] from comment #25)
> Great, thanks!
> 
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #23)
> > @@ +905,5 @@
> > > +  getFormProperty: function(name) {
> > > +    return this._form.props ? this._form.props[name] : null;
> > > +  },
> > > +
> > > +  hasFormProperty: function(name) {
> > 
> > This seems redundant since you can get all the properties with the
> > `formProperties` getter below.
> Can we keep it there, it's actually little handy API that also checks the
> _form.props array (and the array isn't created until there is custom prop
> set - to safe RDP traffic). It also gives extension developers a simple way
> to detect this feature.

Alright, if you wish.

Looking at the patch again however, wouldn't be more consistent to offer |setFormProperty| on the actor prototype like you do with |getFormProperty| and the others?  I can't see why it's needed to create the |setFormProperty| method right before delivering the form.

Comment 27

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

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #26)
> Looking at the patch again however, wouldn't be more consistent to offer
> |setFormProperty| on the actor prototype like you do with |getFormProperty|
> and the others?  I can't see why it's needed to create the |setFormProperty|
> method right before delivering the form.
The 'form' object isn't NodeActor member it's created locally within the NodeActor.form method and so, there wouldn't be access to the 'form' object. Btw. this method also initializes the 'props' array as soon as it's actually needed (when the first custom props is created).

Honza

Comment 28

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Jan Honza Odvarko [:Honza] from comment #27)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #26)
> > Looking at the patch again however, wouldn't be more consistent to offer
> > |setFormProperty| on the actor prototype like you do with |getFormProperty|
> > and the others?  I can't see why it's needed to create the |setFormProperty|
> > method right before delivering the form.
> The 'form' object isn't NodeActor member it's created locally within the
> NodeActor.form method and so, there wouldn't be access to the 'form' object.
> Btw. this method also initializes the 'props' array as soon as it's actually
> needed (when the first custom props is created).

Ah, I misread the patch the first time... I did not notice that |getFormProperty| etc. are on the front instead of the actor.

Okay, I think you are safe to proceed, sorry! :)

Comment 29

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

Attachment: bug1036949-test-1.patch

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #28)
> Okay, I think you are safe to proceed, sorry! :)

Excellent, no problem, thanks for the reviews guys!

I am also attaching a test (another patch). It works for me locally, 
but try is closed now (I've been waiting for some time, but still closed)

Honza

Comment 30

[Patrick Brosset <:pbro>]

Comment on attachment 8611227 [details] [diff] [review]
bug1036949-test-1.patch

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

I went over the code, it looks nice to me.
I realize this may require to rewrite a rather big part of the test but I don't see anything here that really requires the toolbox UI to be opened, and therefore, I think this test would be better suited as a chrome mochitest file in toolkit/devtools/server/tests/mochitest/ alongside with the other test_inspector... tests.
It's not a big deal if you decide not to do this because the wrong type of test is better than no test at all, and we can always rewrite it later.
Btw, try has re-open.

::: browser/devtools/markupview/test/browser.ini
@@ +21,5 @@
>    doc_markup_svg_attributes.html
>    doc_markup_toggle.html
>    doc_markup_tooltip.png
>    doc_markup_xul.xul
> +  browser_markupview_events_form_actor.js

Maybe rename to actor_events_form.js: let's use the actor_ prefix for all future test actors we'll need, and put it first in the support-files list.

::: browser/devtools/markupview/test/browser_markupview_events_form.js
@@ +3,5 @@
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// Testing 'form' event sent by NodeActor

Please elaborate this comment a little bit: "Testing the feature whereby custom registered actors can listen to 'form' events sent by the NodeActor to hook custom data to it" ...

::: browser/devtools/markupview/test/browser_markupview_events_form_actor.js
@@ +1,3 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +

Can you add a comment block at the beginning of this file explaining that this is a test actor used for testing the addition of form data on NodeActors, or something like this anyway.

::: browser/devtools/markupview/test/head.js
@@ +700,5 @@
>    element.dispatchEvent(evt);
>  }
> +
> +/**
> + * A helper for registering a new backend tab actor.

Please document the parameters too, client and options, listing the accepted properties in options.

Comment 31

[Patrick Brosset <:pbro>]

Comment on attachment 8611227 [details] [diff] [review]
bug1036949-test-1.patch

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

I had a second, closer, look at the test file and added a few comments.

::: browser/devtools/markupview/test/browser_markupview_events_form.js
@@ +32,5 @@
> +  info("Unregistering actor");
> +  yield unregisterActor(registrar, front);
> +});
> +
> +function openToolbox() {

Can you move this to head.js instead, making it a function that accepts a String argument which is the tool ID you want to open, then refactor the existing openInspector function in head.js so it uses it.

@@ +33,5 @@
> +  yield unregisterActor(registrar, front);
> +});
> +
> +function openToolbox() {
> +  let deferred = promise.defer();

No need to create a new promise here, you could simply return the promise returned by showToolbox because anyway you're not doing anything in the resolution handler.

::: browser/devtools/markupview/test/head.js
@@ +727,5 @@
> +      });
> +    });
> +  });
> +
> +  return deferred.promise;

What if listTabs or registerActor or getTab fail? Then the promise here will never fulfill, and the test will hang. Could you instead of creating a new promise return the chain of promises?

return client.listTabs(response => {
  ...
  return registry.registerActor(moduleUrl, config).then(registrar => {
    return client.getTab().then(response => {
      return {registrar, form: response.tab};
    });
  });
});

@@ +742,5 @@
> +      deferred.resolve();
> +    });
> +  });
> +
> +  return deferred.promise;

Same remark here.

Comment 32

[Patrick Brosset <:pbro>]

I wasn't very clear in my last comment, these remarks are only valid if you keep the test as a mochitest-devtools test in markupview of course, if you decide to rewrite it as a chrome mochitest in toolkit/devtools, then they don't apply.

Comment 33

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

Attachment: bug1036949-test-2.patch

(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #30)
> Comment on attachment 8611227 [details] [diff] [review]
> bug1036949-test-1.patch
> 
> Review of attachment 8611227 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I went over the code, it looks nice to me.
> I realize this may require to rewrite a rather big part of the test but I
> don't see anything here that really requires the toolbox UI to be opened,
> and therefore, I think this test would be better suited as a chrome
> mochitest file in toolkit/devtools/server/tests/mochitest/ alongside with
> the other test_inspector... tests.
> It's not a big deal if you decide not to do this because the wrong type of
> test is better than no test at all, and we can always rewrite it later.
I see.

Since this feature will eventually be replaced by bug 1166315 and the test is likely to be refactored as well, let's keep it as it is for now.

All the other individual points are fixed.

Updated patch attached.

Try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=510a24336443

Thanks for the review Patrick!

Honza

Comment 34

[Patrick Brosset <:pbro>]

Comment on attachment 8612254 [details] [diff] [review]
bug1036949-test-2.patch

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

LGTM, just a few comments that I'd like addressed but no new review required for this.
Thanks.

::: browser/devtools/markupview/test/actor_events_form.js
@@ +21,5 @@
> +  initialize: function() {
> +    Actor.prototype.initialize.apply(this, arguments);
> +  },
> +
> +  attach: method(function () {

nit: no whitespace between function and ()
(sorry this is very very nitpicking, but I just landed the eslint config in devtools, so now these kind of things are reported to me automatically in my editor. I seem to remember you use Eclipse, I'd be interested to know if there is a way to integrate it there).

::: browser/devtools/markupview/test/browser_markupview_events_form.js
@@ +6,5 @@
> +
> +// Testing the feature whereby custom registered actors can listen to
> +// 'form' events sent by the NodeActor to hook custom data to it.
> +// The test registers one backend actor providing custom form data
> +// and checks that the value is properly send to the client (NodeFront).

nit: s/send/sent

@@ +18,5 @@
> +add_task(function*() {
> +  info("Opening the Toolbox");
> +  let {toolbox} = yield addTab(TEST_PAGE_URL).then(tab => {
> +    return openToolbox("webconsole");
> +  });

nit: If you're going to have to format this then handler on a couple of lines, then it'd be better to just use:

let {tab} = yield addTab(TEST_PAGE_URL);
let {toolbox} = yield openToolbox("webconsole");

avoids mixing "yield promise" and "promise.then(handle)"

::: browser/devtools/markupview/test/head.js
@@ +136,3 @@
>      let inspector = toolbox.getCurrentPanel();
> +    let eventId = "inspector-updated";
> +    return promiseInvoke(inspector, inspector.once, eventId).then(() => {

Not sure if you need promiseInvoke here, inspector.once("inspector-updated") does return a promise already.

@@ +715,5 @@
>  }
> +
> +/**
> + * Registers new backend tab actor.
> + * 

nit: trailing whitespace.

@@ +777,5 @@
> +      deferred.resolve();
> +    });
> +  });
> +
> +  return deferred.promise;

Wouldn't this be simpler like:

function unregisterActor(registrar, front) {
  return front.detach().then(() => {
    return registrar.unregister();
  });
}

Or

let unregisterActor = Task.async(function*(registrar, front) {
  yield front.detach();
  yield registrar.unregister();
});

Comment 35

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

Attachment: bug1036949-test-3.patch

(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #34)
> Comment on attachment 8612254 [details] [diff] [review]
> bug1036949-test-2.patch
> 
> Review of attachment 8612254 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM, just a few comments that I'd like addressed but no new review required
> for this.
All good points, all fixed.

Honza

Comment 36

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5003b725f65c
https://hg.mozilla.org/integration/fx-team/rev/129a1f5435cc

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5003b725f65c
https://hg.mozilla.org/mozilla-central/rev/129a1f5435cc