1275939 - replace domUtils.getCSSValuesForProperty in inplace-editor.js

Status: RESOLVED FIXED

(DevTools :: Framework, defect, P1)

Comment 1

[Greg Tatum [:gregtatum]]

https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/inplace-editor.js#1323
https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/inplace-editor.js#1349

Comment 2

[Greg Tatum [:gregtatum]]

Attachment: replace domUtils.getCSSValuesForProperty in inplace-editor.js

I'm thinking the CSS properties database is an easy approach for this bug. This is an initial
implementation without actually changing the static database.

pbro: do we have an easy way to re-generate the static database? It got pretty painful
to regenerate on my first pass at the getSubpropertiesForCSSProperty.
JSON.stringify(db, null, "\t") ends up creating a lot of newlines. I'm wondering if
I should write a script that can do it nicely for our codebase.

Locally I'm running in to a few intermittents on the `browser_inplace-editor_autocomplete_01.js`
test, that I believe are false positives, although it is in code that I'm touching.

https://brasstacks.mozilla.com/orangefactor/?display=OrangeFactor&includefiltertype=quicksearch&includefilterdetailsexcludeResolved=false&includefilterdetailsexcludeDisabled=false&includefilterdetailsquicksearch=browser_inplace-editor_autocomplete_01.js&includefilterdetailsnumbugs=0&includefilterdetailsresolvedIds=&excludefiltertype=quicksearch&excludefilterdetailsquicksearch=&excludefilterdetailsnumbugs=0&excludefilterdetailsresolvedIds=&plat=All&test=All&type=All&startday=2016-06-24&endday=2016-07-01&tree=trunk

Comment 3

[Patrick Brosset <:pbro>]

I'm worried about 2 things here:

- as you said, generating the CSS static DB is going to get more complex with this
- the amount of data we're sending over the wire is getting way bigger here, and there's a tone of duplicated data data now. For instance
DOMUtils.getCSSValuesForProperty("border-color")
and
DOMUtils.getCSSValuesForProperty("color")
return the same long list of named colors, and there are many color-taking properties that are going to return the same list.

I'm thinking that the list of possible values should instead be asynchronously retrieved from the server only when needed (when auto-completing). This would be a bigger and more complex change.

Looking at chromedevtools, they seem to have everything hard-coded in a CSSMetaData helper (which we discussed already in previous bugs):
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front_end/sdk/CSSMetadata.js?q=CSSMetadata.descriptor&sq=package:chromium&dr=C&l=185
As you can see, they have possible values for each properties (as you did), but they omit colors. And then later, when they need to access the data, they first check if the property accepts colors, and if it does, they append the list of colors to the possible values:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/devtools/front_end/sdk/CSSMetadata.js?q=CSSMetadata.descriptor&sq=package:chromium&l=651&dr=C

Just as a reference, the DOMUtils function is used:
http://searchfox.org/mozilla-central/rev/a7c8e9f3cc323fd707659175a46826ad12899cd1/devtools/client/sourceeditor/editor.js#1300
http://searchfox.org/mozilla-central/rev/a7c8e9f3cc323fd707659175a46826ad12899cd1/devtools/client/sourceeditor/css-autocompleter.js#1225
http://searchfox.org/mozilla-central/rev/a7c8e9f3cc323fd707659175a46826ad12899cd1/devtools/client/shared/inplace-editor.js#1482
http://searchfox.org/mozilla-central/rev/a7c8e9f3cc323fd707659175a46826ad12899cd1/devtools/client/shared/inplace-editor.js#1353

Comment 4

[Patrick Brosset <:pbro>]

Comment on attachment 8767298 [details] [diff] [review]
replace domUtils.getCSSValuesForProperty in inplace-editor.js

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

See my previous comment.

Comment 5

[Greg Tatum [:gregtatum]]

I think not sending the colors makes perfect sense. I'll look at some data-saving measures for this property.

I have a lot of concerns on turning things async, my other `inIDOMUtils.getSubpropertiesForCSSProperty` patch is getting mired down in fixing bugs in the tests due to assumptions about how the components internally manage their state. When I manually test that new code it appears to work correctly. Once some small part of these components turn async, I don't have a lot of confidence in the code being correct as it's hard to reason with the flow of how the components work together.

As for your concerns with the packet getting too large, I was thinking of a new solution to that. The possible configurations of client to server look like this:

 1. Firefox Client -> Firefox Server (same version)
 2. Firefox Client -> Firefox Server (different version that supports actor)
 3. Firefox Client -> Firefox Server (different version that does not support actor)
 4. Firefox Client -> Non-Gecko RDP Server

The only one where we REALLY need to talk to the server for these properties would be configuration #2. We could automate the creation and format of the client-side database to remove any complexity with keeping it up to date, and then write a test to make sure the devtools client are in sync with the platform. That way we could have 100% confidence that the client and server database match for the same version. If we encounter case #2 from above, we could send the information over RDP on booting the inspector panel to make sure our interactions in the inspector are correct.

We can continue to grow the size of the static database without too many concerns about performance, because case #2 is not the common case of usage, all the while continue to be sensitive to keeping the RDP packet small in size when we can. This gives us the added benefit of making the CSS database easier to to maintain as the CSS implementation evolves over time, since we can automatically re-generate our database.

Comment 6

[Greg Tatum [:gregtatum]]

I decided to measure the performance impact of loading the database to see
what kind of load times we're looking at.

In `devtools/shared/fronts/css-properties.js` I ran:

> console.time("Properties load")
> db = yield front.getCSSDatabase();
> console.timeEnd("Properties load")

No data sent:
> Properties load: 57ms
> Properties load: 57ms
> Properties load: 40ms
> Properties load: 44ms
> Properties load: 50ms
> Properties load: 33ms
> Properties load: 42ms

Current load 119kb:
> Properties load: 68ms
> Properties load: 66ms
> Properties load: 59ms
> Properties load: 70ms
> Properties load: 67ms

Padded with extra string data 260kb:
> Properties load: 66ms
> Properties load: 67ms
> Properties load: 71ms
> Properties load: 63ms
> Properties load: 69ms

Padded with extra string data 540kb:
> Properties load: 74ms
> Properties load: 62ms
> Properties load: 65ms
> Properties load: 86ms
> Properties load: 68ms

Comment 7

[Greg Tatum [:gregtatum]]

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

Comment 8

[Greg Tatum [:gregtatum]]

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

Comment 9

[Greg Tatum [:gregtatum]]

Attachment: replace domUtils.getCSSValuesForProperty in inplace-editor.js

Hey Tom, I'm pinging you on this review as pbro is on PTO for a couple of weeks.
This is adding the CSS property values to the CSS database file. Patrick had some
concerns about file size for the packet, which I've addressed by only sending
the packet when the client and server versions do not match. I also removed the
redundant CSS color values from the communication.

I'm running it through try right now, so hopefully nothing comes up.

Comment 10

[Tom Tromey :tromey]

Comment on attachment 8774489 [details] [diff] [review]
replace domUtils.getCSSValuesForProperty in inplace-editor.js

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

Thank you for doing this.  It's looking good.

One concern I have is that this isn't testing the case where the client and server have
different versions.  I wonder if there is some way to add that?

::: devtools/client/shared/inplace-editor.js
@@ +123,2 @@
>   */
> +function editableField(options, cssProperties) {

Rather than a new parameter, I think it's better to just add this to the long list of things in |options|.  It's only needed for certain values of |options.contentType|.

@@ +213,5 @@
>  }
>  
>  exports.getInplaceEditorForSpan = getInplaceEditorForSpan;
>  
> +function InplaceEditor(options, cssProperties, event) {

Here too.

Comment 11

[Greg Tatum [:gregtatum]]

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

Comment 12

[Greg Tatum [:gregtatum]]

Attachment: replace domUtils.getCSSValuesForProperty in inplace-editor.js

Ok, new test, and moved the cssProperties to the options.

Comment 13

[Tom Tromey :tromey]

Comment on attachment 8775274 [details] [diff] [review]
replace domUtils.getCSSValuesForProperty in inplace-editor.js

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

Thanks for doing this.
This looks good to me.

Comment 14

[Greg Tatum [:gregtatum]]

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

Comment 15

[Greg Tatum [:gregtatum]]

Attachment: replace domUtils.getCSSValuesForProperty in inplace-editor.js

MozReview-Commit-ID: KtuAWnNsnaq

Comment 16

[Greg Tatum [:gregtatum]]

Attachment: replace domUtils.getCSSValuesForProperty in inplace-editor.js

Fixed an unused variable eslint issue.

MozReview-Commit-ID: KtuAWnNsnaq

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/c03c0ec30113
replace domUtils.getCSSValuesForProperty in inplace-editor.js r=tromey

Comment 18

[Carsten Book [:Tomcat]]

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