Closed Bug 1225254 Opened 9 years ago Closed 8 years ago

Refactor css-logic.js

Categories

(DevTools :: Framework, defect, P1)

defect

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Iteration:
50.2 - Jul 4
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
CssSheet.forSomeRules isn't used anywhere.
(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.
Blocks: 1266844
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
Flags: qe-verify-
Whiteboard: [devtools-html] [triage]
(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
Iteration: --- → 50.2
Priority: -- → P1
Whiteboard: [devtools-html] [triage] → [devtools-html]
Attachment #8764944 - Flags: review?(pbrosset) → review+
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!
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+
(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.
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.
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
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/
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/
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/d6addbcbd85d
https://hg.mozilla.org/mozilla-central/rev/b111182d1cd2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: