705636 - Extract Style Editor's _loadSource and friends into a utility module

Status: RESOLVED WONTFIX

(DevTools :: Style Editor, defect)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

The Script Debugger is using an older version of _loadSource and related methods for getting source scripts:

https://hg.mozilla.org/users/dcamp_campd.org/remote-debug/file/de21cda87a2c/browser/devtools/debugger/DebuggerUI.jsm#l241

It would be best to have a single version in browser/devtools/shared instead of maintaining two copies of the same code.

Comment 1

[Joe Walker [:jwalker]]

+1

Candidates for inclusion:

https://hg.mozilla.org/integration/fx-team/file/tip/browser/devtools/styleinspector/CssLogic.jsm around line 800

CssLogic.getShortName()
CssLogic.getShortNamePath()
CssLogic.isSystemStyleSheet()
CssLogic.shortSource()

(and IMHO we need a version of CssLogic.getShortNamePath() that returns a unique CSS expression for an arbitrary node)

Comment 2

[Rob Campbell [:rc] (:robcee)]

+1 from me as well for creating a CssUtils module in /shared.

(In reply to Joe Walker from comment #1)
> (and IMHO we need a version of CssLogic.getShortNamePath() that returns a
> unique CSS expression for an arbitrary node)

Yes, we need this for bug 701419.

Comment 3

[Rob Campbell [:rc] (:robcee)]

Attachment: cssutils wip

onCacheDataAvailable doesn't appear to be working yet. Still investigating, but if you have any ideas...

Comment 4

[Cedric Vivier [:cedricv]]

(In reply to Rob Campbell [:rc] (robcee) from comment #3)
> onCacheDataAvailable doesn't appear to be working yet. Still investigating,
> but if you have any ideas...

The multiple .call invocations do not look right ;)
aCallback.call(chunks.join(""));


Also, I'm all for a CssUtil we can share, but this bug title sounds a way more generic feature (loading stuff from cache - including Javascript for the debugger) that IMHO should live in another 'generic' module (devtools/shared/Util.jsm?). We could add assertions and logging to this generic module as well (and leave/implement CSS specific stuff in CSSUtil).
Thoughts?

Comment 5

[Rob Campbell [:rc] (:robcee)]

yeah, the .call()s looked fishy to me too.

I also wondered if this method was going to be more generic than simply CSS. It occurred to me while doing the refactoring that this was going to be used by the Debugger.

That makes me think that we should just use the methods from CssLogic as they are now and move the loader methods into a generic Utils method, probably as was originally intended.

This is what I get for trying to help with what I thought would be a quick, 10 minute code reorg. :)

Comment 6

[Rob Campbell [:rc] (:robcee)]

marking myself unassigned for now. Panos, you might want to just copy the methods you need for now, it doesn't seem worth it to create a utils library for just one loader function.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Rob Campbell [:rc] (robcee) from comment #6)
> marking myself unassigned for now. Panos, you might want to just copy the
> methods you need for now, it doesn't seem worth it to create a utils library
> for just one loader function.

OK, I'll file another bug for it. Feel free to WONTFIX this one, if you don't think it's useful.

Comment 8

[Cedric Vivier [:cedricv]]

Comment on attachment 577635 [details] [diff] [review]
cssutils wip

This piece of code was just not necessary... and more importantly wrong from an asynchronicity PoV.

We'll need to rather reuse channel-based retrieval as in the current StyleEditor.loadSourceFromCache

Comment 9

[Patrick Brosset <:pbro>]

Closing this old refactoring bug as a lot of the code has changed in the past 4 years. No use keeping old refactoring bugs around for a long time. If we haven't done them since, it's very unlikely we'll do them now. Let's file new ones when it makes sense.