Closed Bug 1275939 Opened 4 years ago Closed 4 years ago

replace domUtils.getCSSValuesForProperty in inplace-editor.js

Categories

(DevTools :: Framework, defect, P1)

49 Branch
defect

Tracking

(firefox49 wontfix, firefox51 fixed)

RESOLVED FIXED
Firefox 51
Iteration:
51.1 - Aug 15
Tracking Status
firefox49 --- wontfix
firefox51 --- fixed

People

(Reporter: gregtatum, Assigned: gregtatum)

References

Details

(Whiteboard: [devtools-html])

Attachments

(1 file, 4 obsolete files)

No description provided.
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [devtools-html]
Priority: P2 → P1
Assignee: nobody → gtatum
Status: NEW → ASSIGNED
Iteration: --- → 50.1
Iteration: 50.1 → 50.2
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
Attachment #8767298 - Flags: feedback?(pbrosset)
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 on attachment 8767298 [details] [diff] [review]
replace domUtils.getCSSValuesForProperty in inplace-editor.js

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

See my previous comment.
Attachment #8767298 - Flags: feedback?(pbrosset)
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.
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
Iteration: 50.2 - Jul 4 → 50.3 - Jul 18
Depends on: 1286232, 1286238
Assignee: gtatum → nobody
Status: ASSIGNED → NEW
Iteration: 50.3 - Jul 18 → ---
Priority: P1 → P2
Assignee: nobody → gtatum
Status: NEW → ASSIGNED
Iteration: --- → 50.3 - Jul 18
Priority: P2 → P1
Iteration: 50.3 - Jul 18 → 50.4 - Aug 1
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.
Attachment #8774489 - Flags: review?(ttromey)
Attachment #8767298 - Attachment is obsolete: true
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.
Attachment #8774489 - Flags: review?(ttromey)
Ok, new test, and moved the cssProperties to the options.
Attachment #8775274 - Flags: review?(ttromey)
Attachment #8774489 - Attachment is obsolete: true
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.
Attachment #8775274 - Flags: review?(ttromey) → review+
MozReview-Commit-ID: KtuAWnNsnaq
Attachment #8776596 - Flags: review+
Attachment #8775274 - Attachment is obsolete: true
Fixed an unused variable eslint issue.

MozReview-Commit-ID: KtuAWnNsnaq
Attachment #8776616 - Flags: review+
Attachment #8776596 - Attachment is obsolete: true
Keywords: checkin-needed
Iteration: 50.4 - Aug 1 → 51.1 - Aug 15
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/c03c0ec30113
replace domUtils.getCSSValuesForProperty in inplace-editor.js r=tromey
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c03c0ec30113
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.