Tom Tromey :tromey Assignee
	Description • 8 years ago

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

	Patrick Brosset <:pbro>
	Comment 1 • 8 years ago

This is used in \devtools\client\inspector\rules\models\text-property.js in order to display the list of long-hand properties for a short-hand property in the rule-view panel.

Something like this is done:

var domUtils = Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils);
domUtils.getSubpropertiesForCSSProperty(name);

For instance, for "transition", the following list is returned:
transition-property,transition-duration,transition-timing-function,transition-delay

I think the best course of action would probably be to get this information from the back-end instead (the getApplied method should send it along), and then we can switch to using getSubpropertiesForCSSProperty if this information isn't present (connected to an old backend).

	Patrick Brosset <:pbro>
	Comment 2 • 8 years ago

I was thinking about this while working on bug 1069829 (which touches some of the same files).

The whole logic in text-property.js/updateComputed needs changing. We shouldn't create a dummy HTML element here anymore. We should add a new actor method to PageStyleActor, something like getComputedLongHandProperties (or something shorter).

The nice thing here is that these properties are only shown when you expand the short-hand, so we can totally rewrite this to be async.

@Greg: this is your first bug in the area of the code, don't hesitate to ping me or Tom about this, we can help.

	Greg Tatum [:gregtatum]
	Comment 3 • 8 years ago

Ok, here is a patch for taking out getSubpropertiesForCSSProperty. This builds upon
the CssPropertiesActor work.

	Greg Tatum [:gregtatum]
	Comment 4 • 8 years ago

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

	Patrick Brosset <:pbro>
	Comment 5 • 8 years ago

Comment on attachment 8758430 [details] [diff] [review]
replace inIDOMUtils.getSubpropertiesForCSSProperty

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

I have one concern with this approach. On one hand, I like the approach of just adding more data to the css-properties actor, that seems like a good fit here. On the other hand, I believe the getApplied method (on the PageStyleActor) should actually return this data.

This is what the chrome protocol seems to be doing: https://chromedevtools.github.io/debugger-protocol-viewer/1-1/CSS/#type-CSSStyle
and the closer we can be to that, the easier we make our lives in the future I guess.

Whenever we call pageStyleActor.getApplied to get the list of applied css rules for a node we should get, not only the list of properties in each rule, but also the list of subproperties and their values.

This could be done in StyleRuleActor.form.

I guess the good thing is that if we did this, then that would also take care of my comment in text-property.js, because we wouldn't need the dummyElement anymore.
I think since we're going to do it later anyway, we might as well do it now.

What do you think?

::: devtools/client/inspector/rules/models/text-property.js
@@ +79,2 @@
>      // and see what the computed style looks like.
>      let dummyElement = this.rule.elementStyle.ruleView.dummyElement;

I mentioned this in comment 2, we will need to get rid of this dummyElement logic. It is a DOM node created in the toolbox to get the value for sub properties. However, it depends on the browser we are currently inspecting, not the one the inspector runs in now.
This can of course be done as a separate bug.

::: devtools/client/shared/css-properties-db.js
@@ +424,5 @@
> +/**
> + * This list has the properties with only 1 subproperty removed.
> + */
> +exports.subpropertiesList = {
> +  "all": [

"all" seems to contain all longhand properties. Do we actually need it in the inspector? What uses it? Could we instead derive it from the other data here? It seems to me like it shouldn't be here, and that the css-properties actor should not send it either.

	Greg Tatum [:gregtatum]
	Comment 6 • 8 years ago

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

	Greg Tatum [:gregtatum]
	Comment 7 • 8 years ago

I really wasn't looking at the broader context of this problem. I was only looking at replacing the single function. Patrick your thoughts on addressing the dummyElement make sense to me, and I feel like the getSubpropertiesForCSSProperty should be part of making this call async and handling it on the server.

I'm going to need some broader context to how the inspector works to address this properly. I'm taking some time to go through the tool's codebase to get a better understanding of how all of this fits together.

	Greg Tatum [:gregtatum]
	Comment 8 • 8 years ago

Making this async is stumping me. I've attempted a few approaches, but it ends up touching a lot of code in ways that scare me. I have a bunch of code locally on this, but it's not anything worth sharing yet.

Approach 1: Make TextProperty.prototype.updateComputed() async. I started down this path, and I may explore it some more. The problem is that the constructor calls this function, which injects lots of async assumptions everywhere.

Approach 2: Make the `computed` array a promise that resolves to the list of computed elements. This approach so far is leading to some weird async errors. It's a little scary on knowing what is effected, but I might be able to pursue this route.

	Patrick Brosset <:pbro>
	Comment 9 • 8 years ago

(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #8)
> Approach 1: Make TextProperty.prototype.updateComputed() async. I started
> down this path, and I may explore it some more. The problem is that the
> constructor calls this function, which injects lots of async assumptions
> everywhere.
This sounds like the right approach to me, and I don't see why the TextProperty constructor calls this function. The long-hand properties should only be constructed lazily, when shown the first time.

	Greg Tatum [:gregtatum]
	Comment 10 • 8 years ago

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

	Greg Tatum [:gregtatum]
	Comment 11 • 8 years ago

Ok, this is the approach I'm taking. I've still got a few issues that I need to sort out in the tests. This will resolve bug 1266097 as well. I moved the dummyElement testing to the server. In addition I took away the hiddenDOM logic, and I'm creating the element actually on the debuggee, but not attaching it to the DOM. As far as I can tell all of the computed elements still get calculated correctly. I also probably need to add a test for my new getComputedProperties property.

The failing tests I've got locally are:

> INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_add-property-commented.js | The 'background-color' property is disabled. -
> INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_add-property-commented.js | The 'height' property is enabled. -
> INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_cycle-color.js | Color  is still displayed as a hex value. - Got blue, expected #00f
> INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_multiple-properties-unfinished_01.js | Test timed out -
> INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_pseudo-element_01.js | First added property is on back of array - Got [object Object], expected [object Object]
> INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_pseudo-element_01.js | Second added property is on back of array - Got [object Object], expected [object Object]
> INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_pseudo-element_01.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/inspector/rules/test/head.js:631 - TypeError: textProp.editor is undefined

I also pushed to try to see if anything else is breaking.

Another thing I'm not sure on is how to be backwards compatible with targets that don't have the PageStyleActor's getComputedProperties method.

	Greg Tatum [:gregtatum]
	Comment 13 • 8 years ago

Hmm... it looks like the computed properties are needed initially. As the rules are being loaded in they are checked each time for their computed values to know whether or not the value is being overridden as can be seen here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/rules/models/element-style.js?q=path%3Aelement-style&redirect_type=single#242-305

So it looks like we won't be able to refactor and only get this information when you are expanding the shorthand properties.

	Patrick Brosset <:pbro>
	Comment 14 • 8 years ago

(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #13)
> Hmm... it looks like the computed properties are needed initially. As the
> rules are being loaded in they are checked each time for their computed
> values to know whether or not the value is being overridden as can be seen
> here:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/
> rules/models/element-style.js?q=path%3Aelement-
> style&redirect_type=single#242-305
> 
> So it looks like we won't be able to refactor and only get this information
> when you are expanding the shorthand properties.
So, I suggest investigating plan B: basically what I said in comment 5: StyleRuleActor can send the computed sub-properties and their values.
1 - StyleRuleActor is server-side, which is what we want to calculate sub properties and values correctly anyway
2 - Anytime an element is selected in the inspector, a call to PageStyleActor.getApplied is made, and it returns the list of all rules that apply to the element, each one of these rule is a StyleRuleActor instance.
3 - So, if we make the StyleRuleActor.form not only contain the list of text properties (as we do today), but also, for each of them, the corresponding list of sub properties and their computed values, then we're fine: the information will be available to text-property.js right from the start, no need to fake it with a dummy DOM element, no extra async calls to make to the server.

	Eric Rahm [:erahm]
	Comment 15 • 8 years ago

Just checking in, this blocks bug 1288835, which blocks bug 1276366, which will hopefully eliminate a rather large class of window leaks. Do we plan on continuing to pursue this?

	Greg Tatum [:gregtatum]
	Comment 16 • 8 years ago

I'm still actively working on this bug, it's just taking a long time due to the complexity of adding async behavior to the call stacks.

	Tom Tromey :tromey Assignee
	Comment 20 • 8 years ago

> So, I suggest investigating plan B: basically what I said in comment 5: StyleRuleActor can send the computed sub-properties and their values.

This did seem promising, so I looked into it a bit today.
The main problem seems to be that neither PageStyleActor nor StyleRuleActor
record the node being examined.  It isn't clear to me that it would be
ok to stick this into the PageStyleActor.
So, barring new information, I think I am going to drop this in favor
of Greg's existing approach.

	Tom Tromey :tromey Assignee
	Comment 21 • 8 years ago

For browser_rules_cycle-color.js, the bug seems to be that we fetch the "blue"
value from the store, here:

TextProperty@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/rules/models/text-property.js:52:17
Rule.prototype._getTextProperties@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/rules/models/rule.js:476:22
Rule@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/rules/models/rule.js:65:20
ElementStyle.prototype._maybeAddRule<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/rules/models/element-style.js:189:14

	Tom Tromey :tromey Assignee
	Comment 22 • 8 years ago

More logging shows that perhaps the problem is that we aren't waiting for the
unit change to round-trip through the server before moving on with the test.

	Tom Tromey :tromey Assignee
	Comment 23 • 8 years ago

Waiting longer helps, but introduces a new failure, which occurs because
TextPropertyEditor._onSwatchCommit calls |this.update| before the rule
rewriting in |_onValueDone| has completed.

	Tom Tromey :tromey Assignee
	Comment 30 • 8 years ago

Attaching my current patches in case you want to see what they look like.
The earlier patches have changes relative to Greg's, as well.
I consider the "element-style" patch the worst of the bunch FWIW, so if you
just want to see the terribleness, start there.

As discussed in the meeting, I'm going to abandon this approach in favor of
a purely client-side approach for the time being.  I may file a follow-up
inspector bug and reference this bug for future work.

	Greg Tatum [:gregtatum]
	Comment 34 • 8 years ago
mozreview-review

Comment on attachment 8792118 [details]
Bug 1265796 - add CSS subproperty info to CssProperties;

https://reviewboard.mozilla.org/r/79340/#review78224

Waiting for the new solution we discussed on IRC.

	Tom Tromey :tromey Assignee
	Comment 35 • 8 years ago

Testing showed an eslint nit I overlooked.

	Greg Tatum [:gregtatum]
	Comment 36 • 8 years ago
mozreview-review

Comment on attachment 8792116 [details]
Bug 1265796 - add .eslintrc to devtools/shared/platform/content/;

https://reviewboard.mozilla.org/r/79336/#review78522

	Greg Tatum [:gregtatum]
	Comment 41 • 8 years ago
mozreview-review

Comment on attachment 8792991 [details]
Bug 1265796 - fix mach devtools-css-db for linux, debug builds;

https://reviewboard.mozilla.org/r/79804/#review78576

Thanks for getting this working on Linux!

::: devtools/shared/css/generated/generate-properties-db.js:19
(Diff revision 1)
>  // Output JSON
>  dump(JSON.stringify({
>    cssProperties: cssProperties(),
>    pseudoElements: pseudoElements()
>  }));
> +// In a debug build, xpcshell might print extra debugging information,

Nice.

	Greg Tatum [:gregtatum]
	Comment 42 • 8 years ago
mozreview-review

Comment on attachment 8792117 [details]
Bug 1265796 - remove dummy document from rule view;

https://reviewboard.mozilla.org/r/79338/#review78606

Hmm... I think we may be able to get rid dummyDocument all together here. Thoughts?

::: devtools/client/inspector/rules/rules.js:793
(Diff revision 3)
>        // ::before and ::after do not have a namespaceURI
>        let namespaceURI = this.element.namespaceURI ||
>            document.documentElement.namespaceURI;
>        this._dummyElement = document.createElementNS(namespaceURI,
>                                                     this.element.tagName);
>        document.documentElement.appendChild(this._dummyElement);

If the non-chrome side of the devtools/shared/platform code is run, then this adds a DOM element to the current document, not a dummy document, which I don't think we want. The dummy element is used in [devtools/client/inspector/rules/models/text-property.js](https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/rules/models/text-property.js#83-86) and code appears to work fine without actually adding the element to the page. Do we even need a dummy document here? Could we just get the panel's reference to the current document? It would be functionally the same unless I'm missing something.

e.g. open the webconsole and run:

```js
var dummyElement = document.createElement('div');
var dummyStyle = dummyElement.style;
dummyStyle.cssText = "";
dummyStyle.setProperty("margin", "1em", "");
console.log(dummyStyle.getPropertyValue("margin-top"));
// "1em"
```

	Greg Tatum [:gregtatum]
	Comment 43 • 8 years ago
mozreview-review

Comment on attachment 8792118 [details]
Bug 1265796 - add CSS subproperty info to CssProperties;

https://reviewboard.mozilla.org/r/79340/#review78608

LGTM!

	Tom Tromey :tromey Assignee
	Comment 44 • 8 years ago

> Do we even need a dummy document here? Could
> we just get the panel's reference to the current document? It would be
> functionally the same unless I'm missing something.

I don't know.  However, I tried removing it all and everything seems to
work fine.  In particular all the rule-view tests pass.

	Greg Tatum [:gregtatum]
	Comment 50 • 8 years ago
mozreview-review

Comment on attachment 8792117 [details]
Bug 1265796 - remove dummy document from rule view;

https://reviewboard.mozilla.org/r/79338/#review78874

::: devtools/client/inspector/rules/rules.js
(Diff revision 4)
> - * engine, we will set properties on a dummy element and observe
> - * how their .style attribute reflects them as computed values.
> - * This function creates the document in which those dummy elements
> - * will be created.
> - */
> -var gDummyPromise;

Nice, this simplifies the code quite a bit. I'm glad this looks like it will work out.

	Greg Tatum [:gregtatum]
	Comment 51 • 8 years ago
mozreview-review

Comment on attachment 8792117 [details]
Bug 1265796 - remove dummy document from rule view;

https://reviewboard.mozilla.org/r/79338/#review78876

Sweet, thanks for figuring out if that would work.

	Tom Tromey :tromey Assignee
	Comment 54 • 8 years ago

It turns out that removing the dummy element promise caused a regression...
lots of subtle ordering dependencies in this code.

This latest version restores the promise and fixes an eslint nit.
Green try run here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecdd1157c58b85409ef53392ea57a0ee178856bf

	Pulsebot
	Comment 55 • 8 years ago

Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cc97d85a2b35
fix mach devtools-css-db for linux, debug builds; r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/13215faddd07
add .eslintrc to devtools/shared/platform/content/; r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/627e15d6dd96
remove dummy document from rule view; r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/cf4dc9616855
add CSS subproperty info to CssProperties; r=gregtatum

	Carsten Book [:Tomcat]
	Comment 56 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/cc97d85a2b35
https://hg.mozilla.org/mozilla-central/rev/13215faddd07
https://hg.mozilla.org/mozilla-central/rev/627e15d6dd96
https://hg.mozilla.org/mozilla-central/rev/cf4dc9616855