Closed
Bug 1265790
Opened 9 years ago
Closed 9 years ago
replace inIDOMUtils.isInheritedProperty
Categories
(DevTools :: Framework, enhancement, P1)
DevTools
Framework
Tracking
(firefox49 fixed)
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: tromey, Assigned: gregtatum)
References
Details
(Whiteboard: [devtools-html])
Attachments
(1 file, 2 obsolete files)
50.62 KB,
patch
|
Details | Diff | Splinter Review |
Replace uses of inIDOMUtils.isInheritedProperty for devtools de-chrome-ification project.
Comment 1•9 years ago
|
||
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•9 years ago
|
Flags: qe-verify-
Priority: -- → P1
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → gtatum
Updated•9 years ago
|
Status: NEW → ASSIGNED
Iteration: --- → 49.1 - May 9
Updated•9 years ago
|
Iteration: 49.1 - May 9 → 49.2 - May 23
Updated•9 years ago
|
Iteration: 49.2 - May 23 → 49.3 - Jun 6
Assignee | ||
Comment 2•9 years ago
|
||
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8758749 -
Flags: review?(pbrosset)
Comment 4•9 years ago
|
||
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 5•9 years ago
|
||
Assignee | ||
Comment 6•9 years ago
|
||
This patch changes the format of the DB to be an object instead of lists of
properties.
Assignee | ||
Updated•9 years ago
|
Attachment #8758749 -
Attachment is obsolete: true
Assignee | ||
Comment 7•9 years ago
|
||
Assignee | ||
Comment 8•9 years ago
|
||
Fixing a broken test from try.
Assignee | ||
Updated•9 years ago
|
Attachment #8759184 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 10•9 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
Comment 11•9 years ago
|
||
(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•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Updated•7 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•