Closed Bug 1265790 Opened 3 years ago Closed 3 years ago

replace inIDOMUtils.isInheritedProperty

Categories

(DevTools :: Framework, enhancement, P1)

enhancement

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Iteration:
49.3 - Jun 6
Tracking Status
firefox49 --- fixed

People

(Reporter: tromey, Assigned: gregtatum)

References

Details

(Whiteboard: [devtools-html])

Attachments

(1 file, 2 obsolete files)

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.
Flags: qe-verify-
Priority: -- → P1
Assignee: nobody → gtatum
Status: NEW → ASSIGNED
Iteration: --- → 49.1 - May 9
Iteration: 49.1 - May 9 → 49.2 - May 23
Iteration: 49.2 - May 23 → 49.3 - Jun 6
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+
This patch changes the format of the DB to be an object instead of lists of
properties.
Attachment #8758749 - Attachment is obsolete: true
Fixing a broken test from try.
Attachment #8759184 - Attachment is obsolete: true
Keywords: checkin-needed
Hi Greg! Looks like this needs a rebase.
Flags: needinfo?(gtatum)
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)
https://hg.mozilla.org/mozilla-central/rev/7556b084d9e1
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.