1265798 - replace inIDOMUtils.cssPropertyIsShorthand

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Tom Tromey :tromey]

Replace inIDOMUtils.cssPropertyIsShorthand for the devtools de-chrome-ification
project.

Comment 1

[Greg Tatum [:gregtatum]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d53a98a8dd2e

Comment 2

[Greg Tatum [:gregtatum]]

Attachment: Implement CSS database to query css properties

This is my first pass at separating out the server code. I've not gotten a clear
sense of a direction for creating a more global approach, so I went ahead and
attached my dependency injection version. Let me know if we want to go with
a different approach. This approach did touch quite a few files.

I ended up creating a `devtools/server/css-properties.js` file that duplicates
the functionality of the css-properties actor, but only on the server side. My
thought was that the css-properties.js file and the CssPropertiesActor could
collect any additional CSS synchronous querying that we will need.

Let me know if I need to change this approach or anything else I need to fix up.
I have a try run still going since the tree was closed yesterday, but the relevant
tests seem to be passing locally.

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8751271 [details] [diff] [review]
Implement CSS database to query css properties

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

My main concern with this is something we've already discussed, but I guess it became clearer to me by looking at the code:
here we're adding arguments, passing things around through functions just because some piece of code somewhere happens to need the isKnown function, while all the others parts don't, but they happen to be leading to that code.
So, having something more global (as you said you had discussed about with Tom) seems like a better approach.
We don't have a global devtools object we can easily add this to. Like, there's nothing in the module's scope we can use for this.
Sticking this on toolbox feels wrong because it isn't related to the toolbox. Keeping it on the inspector as you did is better, but it means we need to have access to the inspector everywhere we need this, which we don't, and so we need to pass the reference around as function parameters.

One thing to keep in mind too, is that the list of css properties depends on the debuggee. So, if you have 2 toolboxes opened at the same time in Firefox, remotely connected to 2 different debuggees that support a different set of css properties, we need each toolbox to reference the right list. So, storing the list as a global in a module would need to be done with a WeakMap indexed by toolboxes. So we need a reference to the toolbox anywhere we need the list anyway.

If I'm not mistaken, in this patch, we only need the isKnown function, and we only need it in 2 places: text-property.js and css-parsing-utils.js (to know if a comment should be parsed).
css-parsing-utils.js is the problematic one here, because it has no knowledge of toolboxes, and I don't think it should. It's just a collection of helper functions that parse strings into a CSS model.
So, for this particular case, I think it makes sense to pass a reference to the isKnown function around, as you've done.

For the rest though, it feels odd to have to store this.cssProperties un rule-editor, text-property-editor, text-property, etc... 
All of the classes defined there have access to the toolbox one way or another (like this.rule.elementStyle.ruleView.inspector.toolbox), and so, I'd like to limit the number of places we store cssProperties and avoid adding parameters to functions if we can.
For this, we could do something that I had suggested to you on IRC: have a separate module (which could be the same as where the front is stored, since we now have to separate actors/specs/fronts). This could work somewhat like this:

var cssProperties = new WeakMap();

exports.initCssDatabase = Task.async(function* (toolbox) {
  // Initialize the front, and call the method to retrieve the list.
  let front = CssPropertiesFront(toolbox.target.client, toolbox.target.form);
  let properties = yield front.getCSSDatabase();
  cssProperties.set(toolbox, properties);
});

exports.getCssDatabase = function (toolbox) {
  if (!cssProperties.has(toolbox)) {
    throw new Error("The CSS Database has not been initialized, please make sure initCssDatabase was called once before for this toolbox");
  }
  return cssProperties.get(toolbox);
}

This way, we can have the inspector-panel be in charge of doing this once and for all for this toolbox (after all, the inspector is the one which needs to deal with css properties), as you've done.
But then, whenever we need the DB, consumers can just synchronously get it from anywhere, as long as they have a reference to the toolbox:

let properties = require("/path/to/this/new/module").getCssDatabase(toolbox);

We also have to think about backward compatibility here:
You're introducing a new actor, which means that if you use the toolbox (with your patch in), and remotely connect it to another (older) Firefox instance that doesn't have this actor yet in the server, then there's going to be a problem.
You need to check this first with toolbox.target.hasActor("cssProperties")
And if this returns false, then keep some of the old code around. For instance, you could still pass isKnown to css-parsing-utils functions, but if the actor doesn't exist, then make isKnown be something that uses DOMUtils.

::: devtools/client/fronts/styles.js
@@ +128,5 @@
>     * trait is true; otherwise a RuleModificationList will be
>     * returned.
> +   *
> +   * @param {CssPropertiesFront} cssProperties
> +                                 This is needed by the RuleRewriter.

If you're adding jsdoc for this new param, you should also add some fo the missing @return

Comment 4

[Greg Tatum [:gregtatum]]

Thanks! All of your feedback is really clear. I'll work on a new patch with your feedback.

Comment 5

[Greg Tatum [:gregtatum]]

Attachment: Implement CSS database to query css properties

More progress.

Comment 6

[Greg Tatum [:gregtatum]]

pbro: Ok, this is a start on the approach with the WeakMap keyed on the toolbox. What do you think about:

> /devtools/client/inspector/rules/models/rule.js
> 
> +  const toolbox = this.elementStyle.ruleView.inspector.toolbox;
> +  this.cssProperties = getCssProperties(toolbox);

Here I've saved the reference to a property on the object from the constructor. Then down in a method I used:

> +      for (let cssProp of parseDeclarations(this.cssProperties.isKnown,
> +                                            this.style.authoredText)) {

Should I get the cssProperties object each time the method is called instead of saving it as a property? There would be one less property on the object.


> -function EditingSession(doc, rules) {
> +function EditingSession(cssProperties, doc, rules) {
>    this._doc = doc;
>    this._rules = rules;
>    this._modifications = new Map();
> +  this._cssProperties = cssProperties;
>  }

Also, on the EditingSession I couldn't find the toolbox in the doc or the rules, so I ended up having to still pass in the cssProperties.

I still have a few other things to address from your comments.

Comment 7

[Greg Tatum [:gregtatum]]

(In reply to Patrick Brosset <:pbro> from comment #3)

> We also have to think about backward compatibility here:
> You're introducing a new actor, which means that if you use the toolbox
> (with your patch in), and remotely connect it to another (older) Firefox
> instance that doesn't have this actor yet in the server, then there's going
> to be a problem.
> You need to check this first with toolbox.target.hasActor("cssProperties")
> And if this returns false, then keep some of the old code around. For
> instance, you could still pass isKnown to css-parsing-utils functions, but
> if the actor doesn't exist, then make isKnown be something that uses
> DOMUtils.

I already have devtools/server/css-properties.js with a cssPropertyIsKnown in there, but wouldn't this pollute our client-only code with some server-side code? I thought we were trying to separate the two. The only way I could think to not do this would be to keep a static list of these properties in the source code to support legacy servers.

Comment 8

[Patrick Brosset <:pbro>]

(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #6)
> > /devtools/client/inspector/rules/models/rule.js
> > 
> > +  const toolbox = this.elementStyle.ruleView.inspector.toolbox;
> > +  this.cssProperties = getCssProperties(toolbox);
> 
> Here I've saved the reference to a property on the object from the
> constructor. Then down in a method I used:
> 
> > +      for (let cssProp of parseDeclarations(this.cssProperties.isKnown,
> > +                                            this.style.authoredText)) {
> 
> Should I get the cssProperties object each time the method is called instead
> of saving it as a property? There would be one less property on the object.
Saving a property in the constructor sounds OK to me here. No problem.
> 
> > -function EditingSession(doc, rules) {
> > +function EditingSession(cssProperties, doc, rules) {
> >    this._doc = doc;
> >    this._rules = rules;
> >    this._modifications = new Map();
> > +  this._cssProperties = cssProperties;
> >  }
> 
> Also, on the EditingSession I couldn't find the toolbox in the doc or the
> rules, so I ended up having to still pass in the cssProperties.
I've been looking at this more. You're right, there's no access to a toolbox here.
I'm wondering if we shouldn't pass the inspector instead of the cssProperties here. It might make a bit more sense. Or better, pass the LayoutView instance directly"

function EditingSession({inspector, doc, elementRules}) {
  this._toolbox = inspector.toolbox;
  this._doc = doc;
  this._rules = elementRules;
}

new EditingSession(this);

But, I've been thinking about this again, and the only reason we need to care about this is because RuleRewriter (in css-parsing-utils) needs the isKnown function.
It would be so much better if we could just retrieve the function from RuleRewriter instead of having to pass it around.
I was discussing this on IRC with :ochameau, and he pointed me to bug 1222047 which would be awesome.
In RuleRewriter, we have a rule object, which is a StyleRuleFront. From a front, we can (sort of) easily get a client object. Bug 1222047 could make this even easier by the way.
So if we could get access the right cssProperties front instance from a client, then we wouldn't need to pass anything around anymore.
As today, we would just instantiate the front in the inspector, and load the database, because we still want to do this only once. But then, we could more easily get access to this front from other places, as long as we have a client. And we have a client when we have a toolbox, or target, or another front.

Just to illustrate, today in RuleRewriter, we have this.rule.
From this object, we can sort of get the client:

let client = [...this.rule.conn._clients][0][1].client;
(I'm not sure why there's a a map of _clients here, and which one is the right one, but I'm sure we could find a better way to access the client from this.rule).

let toolbox = [...gDevTools._toolboxes].filter(([target, toolbox]) => target._client === client)[0][1];

So, again, this is just to illustrate what I've been saying. That from a front, we can get a reference to a toolbox. Of course this code is hackish, and forces to loop through toolboxes, but hopefully bug 1222047 can bring a better solution to this.

In the meantime, I think we can still progress with this bug. No need to block on bug 1222047, especially because nobody is working on it now (unless you're interested to take it on).

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8752336 [details] [diff] [review]
Implement CSS database to query css properties

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

::: devtools/shared/specs/css-properties.js
@@ +15,5 @@
> +
> +  methods: {
> +    getCSSDatabase: {
> +        request: {},
> +        response: RetVal("json"),

I was thinking about this just now: it's great that you've chosen the JSON return type here, and we should keep it this way, because it's very likely that the format of what we send back changes over time. So, using the generic JSON type here means that dealing with backward compatibility will be easier. The protocol just won't care about the type, and we'll just have to handle compatibility on the front.

I was thinking about this specifically because of bug 1268082. In that bug, I think one option would be to augment the CSS database with value types that each property supports. I'll drop a comment there about this.

Comment 10

[Greg Tatum [:gregtatum]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=343145950f5e

Comment 11

[Greg Tatum [:gregtatum]]

Attachment: Implement CSS database to query css properties

Ok, let me know if anything else needs to be tweaked on this. I pushed up to try
but haven't seen the results yet. I ended up adding a list of CSS properties
in the code as a fallback, that way the client can have a clean break from server
code, but still work if the actor isn't supported.

Also the WeakMap off of the toolbox was causing some issues with duplicating the
actor front off of a single actor. I instead keyed off of the client that is
on the toolbox.

Comment 12

[Greg Tatum [:gregtatum]]

Attachment: Implement CSS database to query css properties

Woops, missed a few es-lint issues, fixed and re-attached.

Comment 13

[Patrick Brosset <:pbro>]

Comment on attachment 8754836 [details] [diff] [review]
Implement CSS database to query css properties

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

Almost there. I'd like to see one more version before R+.
Also, I think it would be nice to add a comment somewhere in the code that says that bug 1222047 would avoid passing cssProperties around. Maybe in RuleRewriter, the jsdoc comment should mention that when that bug is fixed, we will be able to not require cssProperties to be passed in.

::: devtools/client/inspector/inspector-panel.js
@@ +110,5 @@
>     * open is effectively an asynchronous constructor
>     */
>    open: function () {
>      return this.target.makeRemote().then(() => {
> +      return initCssProperties(this.toolbox)

nit: missing semicolon.

::: devtools/server/css-properties.js
@@ +10,5 @@
> +loader.lazyGetter(this, "DOMUtils", () => {
> +  return Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils);
> +});
> +
> +exports.createCssDatabase = function cssPropertyIsKnown (propertyName) {

I'd like to suggest moving this inside devtools/server/actors/css-properties.js
It is duplicated code and I think we should just have a getCssDatabase in the actor file that is exported and can be used both by the actor and tests if they need it.
In this patch there are 5 new files all named css-properties.js, each doing a different thing. If we can stick with the 3 (front, spec, actor) only, I think that would be better. And there's no reason an actor file can only define an actor class, it can also expose helper methods/classes/whatever if needed.

::: devtools/shared/css-properties.js
@@ +1,1 @@
> +/**

Same comment about this file than in devtools/server/css-properties.js : I think we could make it work without adding this new file. We're already adding css-properties.js in 3 locations (actor, front, spec) and adding this extra helper complexifies things a bit. At least that's my impression.

My suggestion is the following:

isCssVariable is only useful to parseDeclarations in css-parsing-utils.js. So I would move the regexp and isCssVariable there. And instead of having the isKnown function call it here, I would just make css-parsing-utils.js call it. Instead of just doing:
cssPropertyIsKnown(lastProp.name)
it would do:
cssPropertyIsKnown(lastProp.name) || isCssVariable(lastProp.name)

Once done, we are left with just the isKnown function here in this module. But all it does now is this.properties.includes(property), which is simple enough that we don't care about duplicating code here. So, the css-properties front could do it, and the style actor could do it too since we need to check if a property is known in both these locations. I just don't think it's worth it to introduce a new shared module with a class and method for it.

If, later, we end up having to ask more and more questions from the property list, then maybe we could revisit this decision and introduce this shared helper again. I'm thinking that it could even just be a list of pure functions though, like:
function isKnown(properties, name) {
  return properties.includes(name);
}
function otherHelperFunction(properties) {
  ...
}

::: devtools/shared/fronts/css-properties.js
@@ +6,5 @@
> +const {
> +  FrontClassWithSpec,
> +  Front,
> +  custom
> +} = require("devtools/shared/protocol");

nit: maybe you've wrapped this on multiple lines because it's easier to read, or maybe because it didn't fit in under 80 chars. If that is the case, know that we have exceptions to the 80-chars rule. Anything that contains require( for example will be exempt. So if you'd rather have that all on 1 line, then you can do it.

Comment 14

[Greg Tatum [:gregtatum]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba493f9459d8

Comment 15

[Greg Tatum [:gregtatum]]

Attachment: Implement CSS database to query css properties

Taking out the shared/css-properties.js wasn't too crazy since isKnown can be
implemented using the DOMUtils, as it was before. I think it will be nice to
maintain the CssProperties class, since it provides an easy interface to ask
general CSS properties related questions. There is a little bit of complexity
in checking for support with the actor, which the CssProperties interface can
hide away. I did reduce the new file count by putting it all in the front file.

Currently we only have 1 very simple question, but I'm planning on using this
class to implement synchronous calls for
inIDOMUtils.getSubpropertiesForCSSProperty (Bug 1265796) and
inIDOMUtils.isInheritedProperty (Bug 1265790) after this patch lands. Also, with
pure functions, we still need to pass around the database of properties, so it
seems like we'll have less code if we pass it all as one package, rather than
passing the database, and then requiring some functions as well.

It looks like the isCssVariable function is required in
devtools/client/inspector/rules/models/text-property.js, not just in
css-parsing-utils.js. If I don't check it in text-property.js then CSS variables
are crossed out in the inspector panel.

In my previous patch I missed that the CssPropertiesFront was not being
destroyed when the toolbox was destroyed. I added that here.

Comment 16

[Patrick Brosset <:pbro>]

Comment on attachment 8755568 [details] [diff] [review]
Implement CSS database to query css properties

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

Mostly nits, and a comment regarding consistent return values and inspector destroy sequence (by the way, we should be careful touching the inspector-panel destroy code, I suggest you do some manual tests by opening/closing the inspector quickly, or during a page load, etc... just to make sure it doesn't break).

I don't think this require a new round of reviews.

::: devtools/client/inspector/inspector-panel.js
@@ +115,5 @@
> +      panel._cssPropertiesLoaded = initCssProperties(panel.toolbox);
> +      yield panel._cssPropertiesLoaded;
> +      yield panel.target.makeRemote();
> +      yield panel._getPageStyle();
> +      return yield panel._deferredOpen(yield panel._getDefaultNodeForSelection());

Or use Task.async to avoid having to set panel to this:

open: Task.async(function *() {
  this._cssPropertiesLoaded = initCssProperties(panel.toolbox);
  yield this._cssPropertiesLoaded;
  yield panel.target.makeRemote();
  yield panel._getPageStyle();
  let defaultSelection = yield panel._getDefaultNodeForSelection();
  return yield panel._deferredOpen(defaultSelection);
}),

@@ +650,5 @@
>        this.layoutview.destroy();
>      }
>  
> +    if (this._cssPropertiesLoaded) {
> +      this._cssPropertiesLoaded.then(front => {

You might want to store this promise in this._panelDestroyer:
In fact, can this._cssPropertiesLoaded be falsy? It's the first thing that gets initialized when the panel opens. So, even if the inspector is closed quickly during its opening sequence, this._cssPropertiesLoaded should still exist and be a promise, right?

I think this if should be replaced with:

let cssPropertiesDestroyer = this._cssPropertiesLoaded.then(front => {
  if (front) {
    front.destroy();
  }
});

And then later:

    this._panelDestroyer = promise.all([
      sidebarDestroyer,
      markupDestroyer,
      cssPropertiesDestroyer
    ]);

::: devtools/client/shared/css-parsing-utils.js
@@ +151,5 @@
>   * of comment text.  This wraps a recursive call to parseDeclarations
>   * with the processing needed to ensure that offsets in the result
>   * refer back to the original, unescaped, input string.
>   *
> + * @param {Function} cssPropertyIsKnown

nit: maybe rename to isCssPropertyKnown, to match better our general convention of prefixing booleans (or boolean returning functions) with is/has

@@ +254,5 @@
>   *
>   * The return value and arguments are like parseDeclarations, with
>   * these additional arguments.
>   *
> + * @param {Function} cssPropertyIsKnown

Same here.

@@ +409,5 @@
>   * The input string is assumed to only contain declarations so { and }
>   * characters will be treated as part of either the property or value,
>   * depending where it's found.
>   *
> + * @param {Function} cssPropertyIsKnown

And here.

@@ +467,5 @@
>   * Additionally, editing will set the |changedDeclarations| property
>   * on this object.  This property has the same form as the |changed|
>   * property of the object returned by |getResult|.
>   *
> + * @param {Function} cssPropertyIsKnown

And here.

@@ +1079,5 @@
>  /**
>   * Expects a single CSS value to be passed as the input and parses the value
>   * and priority.
>   *
> + * @param {Function} cssPropertyIsKnown

And here :)

::: devtools/server/actors/css-properties.js
@@ +31,5 @@
> +    return { propertiesList };
> +  }
> +});
> +
> +function cssPropertyIsKnown(name) {

As commented before, please rename to isCssPropertyKnown
Also, please add some jsdoc here, at least for the @param and @return, no need for a long description, the name is self-explanatory.

::: devtools/shared/fronts/css-properties.js
@@ +25,5 @@
> + * @param {String} input
> + * @return {Boolean}
> + */
> +function isCssVariable(input) {
> +  return Boolean(input.match(IS_VARIABLE_TOKEN));

Or !!input.match(IS_VARIABLE_TOKEN) which, arguably, is less readable, but might be more often used in our codebase.

@@ +83,5 @@
> + * The front is returned only with this function so that it can be destroyed
> + * once the toolbox is destroyed.
> + *
> + * @param {Toolbox} The current toolbox.
> + * @returns {CssPropertiesFront|undefined}

Actually, this returns a promise that resolves to the Front.

@@ +95,5 @@
> +
> +  // Get the list dynamically if the cssProperties exists.
> +  if (toolbox.target.hasActor("cssProperties")) {
> +    front = CssPropertiesFront(toolbox.target.client,
> +                                     toolbox.target.form);

nit: odd formatting here for the second line. Please vertically align the second line below toolbox.target.client on the previous line.

@@ +106,5 @@
> +    propertiesList = db.propertiesList;
> +  }
> +  const cssProperties = new CssProperties(propertiesList);
> +  cachedCssProperties.set(toolbox.target.client, cssProperties);
> +  return front;

The early return at the top of this function returns the CssProperties instance, but here you return the front. You should be consistent and always return the same thing.
It looks like except for the inspector-panel, no one needs to use this function or its return value. So this function should always return the front.
I tend to think that it would also be useful for this function to return the CssProperties instance, because that's what it says it initializes. So maybe return both?

cachedCssProperties.set(toolbox.target.client, {cssProperties, front});
return {cssProperties, front);

@@ +113,5 @@
> +/**
> + * Synchronously get a cached and initialized CssProperties.
> + *
> + * @param {Toolbox} The current toolbox.
> + * @returns {CssPropertiesFront}

This doesn't return a CssPropertiesFront instance but a CssProperties.

Comment 17

[Greg Tatum [:gregtatum]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=285bec782b0f

Comment 18

[Greg Tatum [:gregtatum]]

Attachment: Implement CSS database to query css properties

This patch addresses pbro's previous comments.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4a7a28982402

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4a7a28982402