1265790 - replace inIDOMUtils.isInheritedProperty

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Tom Tromey :tromey]

Replace uses of inIDOMUtils.isInheritedProperty for devtools de-chrome-ification project.

Comment 1

[Patrick Brosset <:pbro>]

This is used in \devtools\client\inspector\rules\models\rule.js in order to only show inherited properties in inherited rules when an element is selected in the inspector.
I think this information should come from the backend with each rule instead. This would require non-backward compatible protocol changes though, but they can be dealt with: if the rule contains this information then use it, if not, then use isInheritedProperty.

Comment 2

[Greg Tatum [:gregtatum]]

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

Comment 3

[Greg Tatum [:gregtatum]]

Attachment: replace inIDOMUtils.isInheritedProperty

Comment 4

[Patrick Brosset <:pbro>]

Comment on attachment 8758749 [details] [diff] [review]
replace inIDOMUtils.isInheritedProperty

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

This looks good to me. Just a suggestion to maybe reduce the size of the packet, but I'll let you be judge on that one. And a concern about landing this really quick. I believe merge date is Monday (so the thing should land by Friday).

::: devtools/server/actors/css-properties.js
@@ +33,5 @@
> +    propertiesList.forEach(prop => {
> +      inheritedList[prop] = DOMUtils.isInheritedProperty(prop);
> +    });
> +
> +    return { propertiesList, inheritedList };

Should we be concerned with the packet size growing here? One way to make it smaller would be to avoid repeating each and every property name in inheritedList, and just have one list: the propertiesList, except that instead of just being an array of strings, it would be an array of objects that contains the name + a boolean.

::: devtools/shared/fronts/css-properties.js
@@ +106,5 @@
>  
>    // Get the list dynamically if the cssProperties exists.
>    if (toolbox.target.hasActor("cssProperties")) {
>      front = CssPropertiesFront(client, toolbox.target.form);
> +    db = yield front.getCSSDatabase();

If this patch lands after the 49 timeframe (so, not at the same time as the original patch that added the css-properties actor), then we'll need one more level of backward compatibility. Because it's possible that a toolbox running FF50 connects to a backend running FF49. In which case the actor will exist, but it won't be sending the inherited properties.

We don't need to worry about this if this hits FF49. Otherwise, we'll need here something that tests the db to see if the inherited properties exist, ad if not, get the ones from the local db.

Comment 5

[Greg Tatum [:gregtatum]]

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

Comment 6

[Greg Tatum [:gregtatum]]

Attachment: replace inIDOMUtils.isInheritedProperty

This patch changes the format of the DB to be an object instead of lists of
properties.

Comment 7

[Greg Tatum [:gregtatum]]

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

Comment 8

[Greg Tatum [:gregtatum]]

Attachment: replace inIDOMUtils.isInheritedProperty

Fixing a broken test from try.

Comment 9

[Lina Butler [:lina]]

Hi Greg! Looks like this needs a rebase.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/7556b084d9e1
replace inIDOMUtils.isInheritedProperty r=pbro

Comment 11

[Tim Nguyen :ntim]

(In reply to Kit Cambridge [:kitcambridge] from comment #9)
> Hi Greg! Looks like this needs a rebase.

Rebased and landed in comment 10.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7556b084d9e1