replace inIDOMUtils.isInheritedProperty

RESOLVED FIXED in Firefox 49

Status

P1
enhancement
RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: tromey, Assigned: gregtatum)

Tracking

unspecified
Firefox 49
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [devtools-html])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
Replace uses of inIDOMUtils.isInheritedProperty for devtools de-chrome-ification project.
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.

Updated

2 years ago
Flags: qe-verify-
Priority: -- → P1
(Assignee)

Updated

2 years ago
Assignee: nobody → gtatum

Updated

2 years ago
Status: NEW → ASSIGNED
Iteration: --- → 49.1 - May 9

Updated

2 years ago
Iteration: 49.1 - May 9 → 49.2 - May 23

Updated

2 years ago
Iteration: 49.2 - May 23 → 49.3 - Jun 6
(Assignee)

Comment 3

2 years ago
Created attachment 8758749 [details] [diff] [review]
replace inIDOMUtils.isInheritedProperty
Attachment #8758749 - Flags: review?(pbrosset)
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.
Attachment #8758749 - Flags: review?(pbrosset) → review+
(Assignee)

Comment 6

2 years ago
Created attachment 8759184 [details] [diff] [review]
replace inIDOMUtils.isInheritedProperty

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

Updated

2 years ago
Attachment #8758749 - Attachment is obsolete: true
(Assignee)

Comment 8

2 years ago
Created attachment 8759313 [details] [diff] [review]
replace inIDOMUtils.isInheritedProperty

Fixing a broken test from try.
(Assignee)

Updated

2 years ago
Attachment #8759184 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Hi Greg! Looks like this needs a rebase.
Flags: needinfo?(gtatum)

Comment 10

2 years ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/7556b084d9e1
replace inIDOMUtils.isInheritedProperty r=pbro
Keywords: checkin-needed
(In reply to Kit Cambridge [:kitcambridge] from comment #9)
> Hi Greg! Looks like this needs a rebase.

Rebased and landed in comment 10.
Flags: needinfo?(gtatum)

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7556b084d9e1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49

Updated

3 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.