Closed
Bug 1225254
Opened 9 years ago
Closed 8 years ago
Refactor css-logic.js
Categories
(DevTools :: Framework, defect, P1)
DevTools
Framework
Tracking
(firefox50 fixed)
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: pbro, Assigned: tromey)
References
Details
(Whiteboard: [devtools-html])
Attachments
(2 files)
(not sure if this is the right component).
I think we ought it to ourselves to do a complete audit of css-logic.js
It's a rather old module that hasn't been modified too much since it was created, but the code around it was. My feeling (but I may be wrong, hence, the audit) is that it's not used as much as it used to, and not in the same way.
It's 2000 lines long, and really, having 2000 lines of JS to deal with stylesheets and rules and properties seems wrong, I'm pretty sure a lot of this could/should be done by platform.
Also, some code might just be dead. Taking a quick look, I couldn't find a usage of findMatchedSelectors for example (which means that it could be removed along with compareTo and specificity). There might be some others too.
Also, the actors in styles.js are the only ones that actually instantiate the CssLogic class, but I have a feeling that this class isn't nearly as useful as it used to. For instance, we keep on calling .reset() on the instance, which, I think, defeats the original purpose which was to cache a bunch of properties.
Here's a quick list of the usages of css-logic.js throughout our code (excluding the css-logic tests):
StyleSheetEditor.jsm
shortSource
prettifyCSS
computed-view.js
shortSource
FILTER
STATUS
l10n
rule-view.js
shortSource
_strings
l10n
doc_frame_script.js
isContentStyleSheet
styleinspector/test/head.js
Useless import
csscoverage.js
prettifyCSS
markup.js
getComputedStyle
getBindingElementAndPseudo
inspector.js
getComputedStyle
findCssSelector
script.js
findCssSelector
styleeditor.js
isContentStyleSheet
styles.js
new
reset
sourceFilter
FILTER
highlight
computedStyle
hasMatchedSelectors
getPropertyInfo
getShortName
getBindingElementAndPseudo
getSelectors
keyframesRules
stylesheets.js
isContentStyleSheet
Reporter | ||
Comment 1•9 years ago
|
||
CssSheet.forSomeRules isn't used anywhere.
Assignee | ||
Comment 2•9 years ago
|
||
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #0)
> Also, some code might just be dead. Taking a quick look, I couldn't find a
> usage of findMatchedSelectors for example
This one isn't dead, since matchedSelectors calls _findMatchedSelectors.
There is some dead code but not as much as I'd hoped.
Assignee | ||
Comment 3•8 years ago
|
||
I'm going to delete the dead code and, either here or in bug 1266844,
split the server-only bits of css-logic into some file in devtools/server
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Updated•8 years ago
|
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Patrick Brosset <:pbro> from comment #0)
> Also, some code might just be dead. Taking a quick look, I couldn't find a
> usage of findMatchedSelectors for example (which means that it could be
> removed along with compareTo and specificity). There might be some others
> too.
I looked into this one. It's called here:
https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/styles.js#396
Updated•8 years ago
|
Iteration: --- → 50.2
Priority: -- → P1
Whiteboard: [devtools-html] [triage] → [devtools-html]
Assignee | ||
Comment 5•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/60526/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60526/
Attachment #8764944 -
Flags: review?(pbrosset)
Attachment #8764945 -
Flags: review?(pbrosset)
Assignee | ||
Comment 6•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/60528/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60528/
Reporter | ||
Updated•8 years ago
|
Attachment #8764944 -
Flags: review?(pbrosset) → review+
Reporter | ||
Comment 7•8 years ago
|
||
Comment on attachment 8764944 [details]
Bug 1225254 - remove unused code from css-logic.js;
https://reviewboard.mozilla.org/r/60526/#review57854
Nice, a lot of code removed, love it!
Reporter | ||
Comment 8•8 years ago
|
||
Comment on attachment 8764945 [details]
Bug 1225254 - split css-logic.js into server and shared files;
https://reviewboard.mozilla.org/r/60528/#review57848
Seems like a simple enough file split to me, so I went over the changes quite quickly, since there didn't seem to be any code changes made to css-logic apart from the split.
I just have a comment about possibly this adding some confusion. See below.
::: devtools/server/actors/styleeditor.js:17
(Diff revision 1)
> const protocol = require("devtools/shared/protocol");
> const {Arg, method, RetVal} = protocol;
> const {fetch} = require("devtools/shared/DevToolsUtils");
> const {oldStyleSheetSpec, styleEditorSpec} = require("devtools/shared/specs/styleeditor");
>
> -loader.lazyGetter(this, "CssLogic", () => require("devtools/shared/inspector/css-logic").CssLogic);
> +loader.lazyGetter(this, "CssLogic", () => require("devtools/shared/inspector/css-logic"));
CssLogic is used in OldStyleSheetActor in this file, which iirc was introduced for backward compatibility reasons a long time ago. I think we could safely get rid of it now. Anyway, that's probably for another bug.
::: devtools/server/actors/styles.js:22
(Diff revision 1)
> -loader.lazyGetter(this, "CssLogic", () => require("devtools/shared/inspector/css-logic").CssLogic);
> +loader.lazyGetter(this, "CssLogic", () => require("devtools/server/css-logic").CssLogic);
> +loader.lazyGetter(this, "SharedCssLogic", () => require("devtools/shared/inspector/css-logic"));
It may be a bit confusing for people to know which css-logic does what. Why we have 2, and when adding new code, which one should it go in.
In most cases, it'll be easy enough: server-side actors require the server-side css-logic, and client-side UI code require the client-side css-logic.
But in this case, you need both.
The reason you need both is the FILTER.UA constant only though. Maybe we could move this constant (and other similar constants) somewhere else? Or duplicate them in both css-logic?
Or maybe we just need to rename the shared css-logic to something else. Now that it only contains constants and a few helper functions, it might need to be renamed to avoid confusion (or merged into css-properties-db.js ?).
If we think of our server and client as 2 really separate things that just can't require code from each other (which will be the case when we connect to other browsers), then duplicating doesn't sound as bad. But, up to you.
Attachment #8764945 -
Flags: review?(pbrosset) → review+
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to Patrick Brosset <:pbro> from comment #8)
> The reason you need both is the FILTER.UA constant only though. Maybe we
> could move this constant (and other similar constants) somewhere else? Or
> duplicate them in both css-logic?
> Or maybe we just need to rename the shared css-logic to something else. Now
> that it only contains constants and a few helper functions, it might need to
> be renamed to avoid confusion (or merged into css-properties-db.js ?).
I have an aversion to duplication so I would prefer not to take that route.
One idea would be to have the server-side css-logic include the shared one
and re-export some constants.
After thinking it over, though, I rather like the idea of moving the constants
to css-properties-db, so I think I will most likely do that. There is still
server code using the shared css-logic.js, though, so it has to remain where it is.
Assignee | ||
Comment 10•8 years ago
|
||
I looked at this again and styles.js doesn't just use the constants from
the shared css-logic, but also one function:
let isSystem = !SharedCssLogic.isContentStylesheet(domRule.parentStyleSheet);
I think I am going to leave the patches as-is on this basis.
I wouldn't mind renaming one css-logic.js file. I didn't have a better idea of a name,
though.
Assignee | ||
Comment 11•8 years ago
|
||
Assignee | ||
Comment 12•8 years ago
|
||
I was too eager in that first patch :-( ruleCount is actually used in one spot:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/styleeditor/StyleSheetEditor.jsm#275
Assignee | ||
Comment 13•8 years ago
|
||
Comment on attachment 8764944 [details]
Bug 1225254 - remove unused code from css-logic.js;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60526/diff/1-2/
Assignee | ||
Comment 14•8 years ago
|
||
Comment on attachment 8764945 [details]
Bug 1225254 - split css-logic.js into server and shared files;
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60528/diff/1-2/
Assignee | ||
Comment 15•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 16•8 years ago
|
||
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/d6addbcbd85d
remove unused code from css-logic.js; r=pbro
https://hg.mozilla.org/integration/fx-team/rev/b111182d1cd2
split css-logic.js into server and shared files; r=pbro
Keywords: checkin-needed
Comment 17•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d6addbcbd85d
https://hg.mozilla.org/mozilla-central/rev/b111182d1cd2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•