Currently the highlighters are all in the same module; that makes hard to maintain them, introduce new highlighters, and work on highlighters in parallel. There are also strict dependencies that should be avoided, the highlighters should be as much as stand alone module as possible, and make them easier to create it would help the contributors too.

Comment on attachment 8660583 [details] [diff] [review] Refactor the highlighters into separate modules Notice that this patch is built on top of Bug 1132475's patch. There are still some improvements I'd like to do (e.g. the highlighters.css), but I think this is already a good base to start. All the highlighters now are under `toolkit/devtools/server/actors/highlighters/`; and each of them has it's own module. Here the try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=05ed69e2c460 I don't think the exceptions are caused by this patch.

Comment on attachment 8660583 [details] [diff] [review] Refactor the highlighters into separate modules Review of attachment 8660583 [details] [diff] [review]: ----------------------------------------------------------------- Not many remarks. Looks good to me. Thanks for doing this, it'll be a lot easier to work with the various highlighters. ::: toolkit/devtools/layout/utils.js @@ +8,5 @@ > const { memoize } = require("sdk/lang/functional"); > > +loader.lazyRequireGetter(this, "setIgnoreLayoutChanges", > + "devtools/server/actors/layout", true); > +exports.setIgnoreLayoutChanges = (...args) => Why is it a good idea to add this extra level of setIgnoreLayoutChanges in layout/utils.js rather than have the highlighter directly require it from the layout actor? My point of view is this adds one more level for people to go over when trying to understand what this function does. ::: toolkit/devtools/server/actors/highlighters/simple-outline.js @@ +8,5 @@ > + addPseudoClassLock, removePseudoClassLock } = require("./utils/markup"); > + > +// SimpleOutlineHighlighter's stylesheet > +const HIGHLIGHTED_PSEUDO_CLASS = ":-moz-devtools-highlighted"; > +const SIMPLE_OUTLINE_SHEET = ".__fx-devtools-hide-shortcut__ {" + Since you're moving this code, maybe you could take this opportunity to change this to a template string `` This way we could deal with multi-lines without having to use + ::: toolkit/devtools/server/actors/highlighters/utils/markup.js @@ +44,5 @@ > + */ > +function isXUL(window) { > + return window.document.documentElement.namespaceURI === XUL_NS; > +} > +exports.isXUL = isXUL; nit: add a newline after this.

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #3) > ::: toolkit/devtools/layout/utils.js > @@ +8,5 @@ > > const { memoize } = require("sdk/lang/functional"); > > > > +loader.lazyRequireGetter(this, "setIgnoreLayoutChanges", > > + "devtools/server/actors/layout", true); > > +exports.setIgnoreLayoutChanges = (...args) => > > Why is it a good idea to add this extra level of setIgnoreLayoutChanges in > layout/utils.js rather than have the highlighter directly require it from > the layout actor? For a couple of reasons: first, it seems odd to me that the highlighter needs to require two different module for utilities related to layout. Ignoring the layout changes can be definitely part of the `layout/utils`, so we keep the highlighter code narrow. Also, in writing the `highlighter` the developer doesn't need to use a different `require` (a lazy one) for the `setIgnoreLayoutChanges`, 'cause it comes lazy by default, and can be used only in that one. Plus, it doesn't need to set a "globals" for the linter too. > My point of view is this adds one more level for people to go over when > trying to understand what this function does. Personally I find the advantages to compensate that; I think we could also copy / add a comment that explains what the method does to reduce this issue. > ::: toolkit/devtools/server/actors/highlighters/simple-outline.js > @@ +8,5 @@ > > + addPseudoClassLock, removePseudoClassLock } = require("./utils/markup"); > > + > > +// SimpleOutlineHighlighter's stylesheet > > +const HIGHLIGHTED_PSEUDO_CLASS = ":-moz-devtools-highlighted"; > > +const SIMPLE_OUTLINE_SHEET = ".__fx-devtools-hide-shortcut__ {" + > > Since you're moving this code, maybe you could take this opportunity to > change this to a template string `` > This way we could deal with multi-lines without having to use + I'll update the patch, thanks. Yeah, there are a tons of changes and improvements I want to do, as I wrote in comment 2, but I wanted at least have the separation in modules landed, so also the improvement can be done easier compared to now.

Oh, and by the way, Thanks a lot for the quick review! Really appreciated!

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #4) > (In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #3) > > > ::: toolkit/devtools/layout/utils.js > > @@ +8,5 @@ > > > const { memoize } = require("sdk/lang/functional"); > > > > > > +loader.lazyRequireGetter(this, "setIgnoreLayoutChanges", > > > + "devtools/server/actors/layout", true); > > > +exports.setIgnoreLayoutChanges = (...args) => > > > > Why is it a good idea to add this extra level of setIgnoreLayoutChanges in > > layout/utils.js rather than have the highlighter directly require it from > > the layout actor? > > For a couple of reasons: first, it seems odd to me that the highlighter > needs to require two different module for utilities related to layout. > Ignoring the layout changes can be definitely part of the `layout/utils`, so > we keep the highlighter code narrow. The problem I have with this is that setIgnoreLayoutChanges only has meaning for the LayoutChangesObserver, which is something the ReflowActor uses. Moving this function to a general highlighter-level utils makes it seem like it's a general utility that people can use to ignore layout changes (whatever that means), whereas it's a really specific thing that is only used to avoid side-effects when the ReflowActor is intialized *and* listening. > Also, in writing the `highlighter` the developer doesn't need to use a > different `require` (a lazy one) for the `setIgnoreLayoutChanges`, 'cause it > comes lazy by default, and can be used only in that one. Yes, that's nice. > Plus, it doesn't > need to set a "globals" for the linter too. Well, this won't be needed anymore very soon anyway, so I don't think that's a problem (see bug 1203520). So, overall, not an important enough topic to cancel the review, that's why I R+'d at first, but if you decide to keep it this way, then at least comment your reasons in the layout utils file.

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #6) > (In reply to Matteo Ferretti [:matteo] [:zer0] from comment #4) > > (In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #3) > > > > > ::: toolkit/devtools/layout/utils.js > > > @@ +8,5 @@ > > > > const { memoize } = require("sdk/lang/functional"); > > > > > > > > +loader.lazyRequireGetter(this, "setIgnoreLayoutChanges", > > > > + "devtools/server/actors/layout", true); > > > > +exports.setIgnoreLayoutChanges = (...args) => > > > > > > Why is it a good idea to add this extra level of setIgnoreLayoutChanges in > > > layout/utils.js rather than have the highlighter directly require it from > > > the layout actor? > > > > For a couple of reasons: first, it seems odd to me that the highlighter > > needs to require two different module for utilities related to layout. > > Ignoring the layout changes can be definitely part of the `layout/utils`, so > > we keep the highlighter code narrow. > > The problem I have with this is that setIgnoreLayoutChanges only has meaning > for the LayoutChangesObserver, which is something the ReflowActor uses. > Moving this function to a general highlighter-level utils Well, it's not really a general highlighter-level – it's under `toolkit/devtools/layout`, not under `highlighter/utils` – and we're not moving anything, we're just creating a lazy-by-default shortcut, to make the higher code cleaner; we could also rename it a different way. I understand the concern about that `setIgnoreLayoutChanges` only has meaning for the `LayoutChangesObserver`, in fact I'm not 100% happy with my solution, but to me it's just a way to reduce the complexity on higher level code. I mean, we have to put that thing somewhere, we can either require lazy every time in any add-on, or using a shortcut to do so. > > Also, in writing the `highlighter` the developer doesn't need to use a > > different `require` (a lazy one) for the `setIgnoreLayoutChanges`, 'cause it > > comes lazy by default, and can be used only in that one. > Yes, that's nice. > So, overall, not an important enough topic to cancel the review, that's why > I R+'d at first, but if you decide to keep it this way, then at least > comment your reasons in the layout utils file. I don't have a strong feeling about that, so I can also revert that part. At the end, what I'd like to have, it's something transparent to the developer, so that `setUgnoreLayoutChanges` is not explicitly required. That was a temporary simplification.

Comment on attachment 8661327 [details] [diff] [review] Refactor the highlighters into separate modules We sorted out the last comments over IRC. Note that this patch needs to be checked in after the on in Bug 1132475, 'cause it's built on top of it.