Patrick Brosset <:pbro> Reporter
	Description • 7 years ago

In bug 1350645, we want to stop using a certain number of SDK APIs.
One of them is sdk/content/mod which the inspector uses to inject CSS files in the content page so they can be used to style our highlighters or hide elements when you press H in the markup-view.

See the usage here:
http://searchfox.org/mozilla-central/search?q=sdk%2Fcontent%2Fmod&path=devtools

We should replace this with something else.

	Matteo Ferretti [:zer0] [:matteo] Assignee
	Comment 2 • 7 years ago

A couple of notes here, that could be discussed in the review.

First of all, I kept the same logic we had before, just refactored the code without the SDK dependencies.

I have some doubt, however, not only related to that:

1. We use a WeakSet (it was a WeakMap) to decide if apply or not the stylesheet, since if it is already applied we do not want to applied twice. However, that would never be the case since the platform method would fail if we try to load the style twice; and since we catch the exception we could probably call all the time `loadSheet` directly without the need to keep references to documents in our code. My only concern in that case could be only related to performance, but I doubt it will be visible.

2. I wasn't sure were to put the `loadSheet` and `removeSheet` methods. I temporary put in `shared/layout/utils.js` however it doesn't feel totally right. Itt's just the best module I've found that we have at the moment where to put them, but they could probably belongs to a different module / new module – any suggestion are welcomed.
IMVHO they should be under `shared` anyway. I was also thinking, if we remove from `layout` we should probably have a shared module for the `DOMWindowUtils` cache: currently is only in `layout/utils` – mostly because the layout's call are the more frequent ones that needs to be faster since they're during the rendering -; but since we're using DOMWindowUtils a lot also in other part of the code, maybe we could have a separate module that actually keeps the reference to them and using across devtools as default way to obtain the window utils.

3. The default type for `loadSheet` / `removeSheet` was "user", but we're only use "agent" in our code. We could switch to that as default; especially if we're going to remove the `installHelperSheet` and call `loadSheet` directly – for the moment, as said, I kept the previous logic.

	Patrick Brosset <:pbro> Reporter
	Comment 3 • 7 years ago

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #2)
> 1. We use a WeakSet (it was a WeakMap) to decide if apply or not the
> stylesheet, since if it is already applied we do not want to applied twice.
> However, that would never be the case since the platform method would fail
> if we try to load the style twice; and since we catch the exception we could
> probably call all the time `loadSheet` directly without the need to keep
> references to documents in our code. My only concern in that case could be
> only related to performance, but I doubt it will be visible.
I doubt that we would gain any performance by having a WeakSet on our side to avoid calling loadSheet.
If loadSheet already throws when you try to load twice the same URL, it means it already has a list of URLs and it uses it to early return. So I'm pretty sure that the cost of maintaining a WeakSet on our side is actually higher than to just call loadSheet unconditionally and have a try/catch around it. So I vote for getting rid of the code on our side.

> 2. I wasn't sure were to put the `loadSheet` and `removeSheet` methods. I
> temporary put in `shared/layout/utils.js` however it doesn't feel totally
> right. Itt's just the best module I've found that we have at the moment
> where to put them, but they could probably belongs to a different module /
> new module – any suggestion are welcomed.
> IMVHO they should be under `shared` anyway. I was also thinking, if we
> remove from `layout` we should probably have a shared module for the
> `DOMWindowUtils` cache: currently is only in `layout/utils` – mostly because
> the layout's call are the more frequent ones that needs to be faster since
> they're during the rendering -; but since we're using DOMWindowUtils a lot
> also in other part of the code, maybe we could have a separate module that
> actually keeps the reference to them and using across devtools as default
> way to obtain the window utils.
I'm fine with shared/layout/utils.js for now.
If we do, in fact, use DOMWindowUtils stuff in other parts of the code then I agree we could create another shared module somewhere more central. But please do this in a separate bug, and only if we do need it. No need creating a new shared util thing that will in fact be used in just one place.

> 3. The default type for `loadSheet` / `removeSheet` was "user", but we're
> only use "agent" in our code. We could switch to that as default; especially
> if we're going to remove the `installHelperSheet` and call `loadSheet`
> directly – for the moment, as said, I kept the previous logic.
Sounds good to me, let's default to agent since that's what we use.

	Patrick Brosset <:pbro> Reporter
	Comment 4 • 7 years ago
mozreview-review

Comment on attachment 8856828 [details]
Bug 1353005 - removed sdk/content/mod and replaced with DOMWindowUtils calls;

https://reviewboard.mozilla.org/r/128748/#review132000

Thanks!
No need for an extra review. See my previous comment.

::: commit-message-a31c5:1
(Diff revision 1)
> +Bug 1353005 - removed sdk/content/mod and replaced with DOMWindowUtils calls; r= pbro

The reason mozreview didn't set the review flag correctly is because you have a space between = and pbro :)
It's really silly, but it happened to me before too. mozreview is quite sensitive to this. So you might want to correct this in your next ammended commit.

::: devtools/server/actors/highlighters/utils/markup.js:107
(Diff revision 1)
>  exports.isXUL = isXUL;
>  
>  /**
>   * Inject a helper stylesheet in the window.
>   */
> -var installedHelperSheets = new WeakMap();
> +var installedHelperSheets = new WeakSet();

As discussed in the bug, let's get rid of this.

::: devtools/server/actors/inspector.js:1855
(Diff revision 1)
>      if (!this.installedHelpers) {
> -      this.installedHelpers = new WeakMap();
> +      this.installedHelpers = new WeakSet();
>      }
>      let win = node.rawNode.ownerGlobal;
>      if (!this.installedHelpers.has(win)) {
> -      let { Style } = require("sdk/stylesheet/style");
> +      loadSheet(win, HELPER_SHEET, "agent");

And let's make "agent" the default value.

	Pulsebot
	Comment 5 • 7 years ago

Pushed by mferretti@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4c4b1375925f
removed sdk/content/mod and replaced with DOMWindowUtils calls; r=pbro pbro

	Mozilla Autoland
	Comment 7 • 7 years ago

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s bef647f7805e -d 4c4b1375925f: rebasing 391137:bef647f7805e "Bug 1353005 - removed sdk/content/mod and replaced with DOMWindowUtils calls; r=pbro" (tip)
merging devtools/server/actors/highlighters/utils/markup.js
merging devtools/server/actors/inspector.js
merging devtools/shared/layout/utils.js
warning: conflicts while merging devtools/server/actors/highlighters/utils/markup.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging devtools/server/actors/inspector.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging devtools/shared/layout/utils.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 8 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/4c4b1375925f