1147826 - Stop exposing the tabActor to commands

Status: RESOLVED FIXED

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

Description

[Joe Walker [:jwalker]]

The highlight command uses the tabActor [1] which is exposed by the GCLI actor [2]. We should stop exposing the tabActor, but instead fake it up inside the command (or the highlighter)

[1]: https://github.com/joewalker/gecko-dev/blob/pr10-tip/toolkit/devtools/gcli/commands/highlight.js#L115
[2]: https://github.com/joewalker/gecko-dev/blob/pr10-tip/toolkit/devtools/server/actors/gcli.js#L234

Comment 2

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1147826 - Remove deprecated TabActor reference in runAt:server GCLI commands; r=jwalker; r=miker

Bug 1147826 - Remove deprecated TabActor reference in runAt:server GCLI commands; r=jwalker

We used to rely on the TabActor a lot in
/toolkit/devtools/server/actors/highlighter.
This object was being passed around to various highlighter classes, helper
classes and functions.
It was a useful way to get access to the window the highlighters are shown,
listen to DOM events in that window, and also listen to tabActor's will-navigate
events.

Using this object isn't the best idea because it assumes the debugger server is
started and attached to a tab, which makes re-using highlighters outside of this
context impossible (or harder).
Plus we wanted to get rid of the tabActor reference in gcli command contexts.

This change introduces a HighlighterEnvironment that fulfills the requirements
above without relying on a tabActor.
It needs to be initialized either with a tabActor, when we have one (when using
the highlighter from the inspector panel), or with a window (which is what gcli
commands will use).

This change also fixes the highlight command and the rulers command (which
didn't work well with e10s either).

Comment 3

[Patrick Brosset <:pbro>]

Joe, here is a first try at fixing this. It seems to work fine so far.
The commit message explains what I've done (mozreview has apparently imported it in comment 2 too).

Some more notes:
- to fix the rulers command, I've introduced a rulers_server command that displays the highlighter, while the rulers command (client only) manages the button state,
- I've also fixed a problem in the eslint config and cleaned up the highlight and rulers command code a little bit so they don't generate any eslint errors anymore.

Comment 4

[Joe Walker [:jwalker]]

Comment on attachment 8612279 [details]
MozReview Request: Bug 1147826 - Remove deprecated TabActor reference in runAt:server GCLI commands; r=jwalker; r=miker

https://reviewboard.mozilla.org/r/9549/#review8535

::: toolkit/devtools/gcli/commands/highlight.js:11
(Diff revision 1)
> +  require("devtools/server/actors/highlighter");

Policy Nit: Does the eslintrc ask for 2 space indents?

My gripe with 2 space indents is that it can force you to read to the ends of line to work out what's happening:

if (long.condition.that.might.wrap.but.not.sure &&
  take.some.action.which.maybe.conditional) {
  take.another.action();
}

Comment 5

[Joe Walker [:jwalker]]

https://reviewboard.mozilla.org/r/9549/#review8537

::: toolkit/devtools/gcli/commands/highlight.js:23
(Diff revision 1)
> +let hEnvironment;

Minor: Initial thought: "What's the h prefix for? Aren't we cured from our aPrefix gObsession?"

Is 'highlighterEnv' worse?

::: toolkit/devtools/server/actors/highlighter.js:3108
(Diff revision 1)
> +  initFromWindow: function(window) {

If we're going to move our code into content at some stage, maybe we should avoid using 'window' as a local variable name?

We're probably doing this in so many places that it's swimming against the tide somewhat, and it's not an error even with a 'window' global. So this is more of a thinking-out-loud than a comment.

Comment 6

[Joe Walker [:jwalker]]

Comment on attachment 8612279 [details]
MozReview Request: Bug 1147826 - Remove deprecated TabActor reference in runAt:server GCLI commands; r=jwalker; r=miker

https://reviewboard.mozilla.org/r/9549/#review8539

Ship It!

Comment 7

[Joe Walker [:jwalker]]

It's r+ but from me, but I'm not sure I can give a proper review of the highlighter.js changes without spending more time on it than I have this week. Maybe it's a good idea to ask someone else?

Comment 8

[Patrick Brosset <:pbro>]

(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #4)
> Comment on attachment 8612279 [details]
> MozReview Request: Bug 1147826 - Remove deprecated TabActor reference in
> runAt:server GCLI commands; r=jwalker
> 
> https://reviewboard.mozilla.org/r/9549/#review8535
> 
> ::: toolkit/devtools/gcli/commands/highlight.js:11
> (Diff revision 1)
> > +  require("devtools/server/actors/highlighter");
> 
> Policy Nit: Does the eslintrc ask for 2 space indents?
Yes I've configured eslintrc to check for 2 spaces indents. That's what we've been advertising here https://wiki.mozilla.org/DevTools/CodingStandards#Code_style (which I reformatted last week, but the 2 spaces indentation has always been there).
> My gripe with 2 space indents is that it can force you to read to the ends
> of line to work out what's happening:
> 
> if (long.condition.that.might.wrap.but.not.sure &&
>   take.some.action.which.maybe.conditional) {
>   take.another.action();
> }
fwiw, I usually format like this:
if (long.condition.that.might.wrap.but.not.sure &&
    take.some.action.which.maybe.conditional) {
  take.another.action();
}

Not a whole lot better, but I believe it helps, and I think that's a popular pattern in the code base.

(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #5)
> https://reviewboard.mozilla.org/r/9549/#review8537
> 
> ::: toolkit/devtools/gcli/commands/highlight.js:23
> (Diff revision 1)
> > +let hEnvironment;
> 
> Minor: Initial thought: "What's the h prefix for? Aren't we cured from our
> aPrefix gObsession?"
> 
> Is 'highlighterEnv' worse?
Changed to highlighterEnv which is, indeed, a lot better.

> ::: toolkit/devtools/server/actors/highlighter.js:3108
> (Diff revision 1)
> > +  initFromWindow: function(window) {
> 
> If we're going to move our code into content at some stage, maybe we should
> avoid using 'window' as a local variable name?
> 
> We're probably doing this in so many places that it's swimming against the
> tide somewhat, and it's not an error even with a 'window' global. So this is
> more of a thinking-out-loud than a comment.
Yes we're doing this all over the place. Somehow it always feels strange when I do it because window might be a global as you said, even if it's not in our case (most of the time at least). I guess win is just as good and we could probably refrain from inserting new instances of window in the code. I will change it.

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8612279 [details]
MozReview Request: Bug 1147826 - Remove deprecated TabActor reference in runAt:server GCLI commands; r=jwalker; r=miker

Bug 1147826 - Remove deprecated TabActor reference in runAt:server GCLI commands; r=jwalker; r=miker

We used to rely on the TabActor a lot in
/toolkit/devtools/server/actors/highlighter.
This object was being passed around to various highlighter classes, helper
classes and functions.
It was a useful way to get access to the window the highlighters are shown,
listen to DOM events in that window, and also listen to tabActor's will-navigate
events.

Using this object isn't the best idea because it assumes the debugger server is
started and attached to a tab, which makes re-using highlighters outside of this
context impossible (or harder).
Plus we wanted to get rid of the tabActor reference in gcli command contexts.

This change introduces a HighlighterEnvironment that fulfills the requirements
above without relying on a tabActor.
It needs to be initialized either with a tabActor, when we have one (when using
the highlighter from the inspector panel), or with a window (which is what gcli
commands will use).

This change also fixes the highlight command and the rulers command (which
didn't work well with e10s either).

Comment 10

[Patrick Brosset <:pbro>]

Mike, Joe has reviewed that patch already, it'd be great if you could take a closer look at the changes in highlighter.js (which are described in comment 9).

Comment 11

[Joe Walker [:jwalker]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #8)
> (In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc)
> from comment #4)
> > Comment on attachment 8612279 [details]
> > MozReview Request: Bug 1147826 - Remove deprecated TabActor reference in
> > runAt:server GCLI commands; r=jwalker
> > 
> > https://reviewboard.mozilla.org/r/9549/#review8535
> > 
> > ::: toolkit/devtools/gcli/commands/highlight.js:11
> > (Diff revision 1)
> > > +  require("devtools/server/actors/highlighter");
> > 
> > Policy Nit: Does the eslintrc ask for 2 space indents?
> Yes I've configured eslintrc to check for 2 spaces indents. That's what
> we've been advertising here
> https://wiki.mozilla.org/DevTools/CodingStandards#Code_style (which I
> reformatted last week, but the 2 spaces indentation has always been there).
> > My gripe with 2 space indents is that it can force you to read to the ends
> > of line to work out what's happening:
> > 
> > if (long.condition.that.might.wrap.but.not.sure &&
> >   take.some.action.which.maybe.conditional) {
> >   take.another.action();
> > }
> fwiw, I usually format like this:
> if (long.condition.that.might.wrap.but.not.sure &&
>     take.some.action.which.maybe.conditional) {
>   take.another.action();
> }

Right, so personally I distinguish between follow-on indents which are 4+ spaces and block indents which are 2 spaces.
The commented line had a 2 space follow-on indent, and I wasn't sure if this was a policy.

Comment 12

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=45b51d729217

Comment 13

[Tim Nguyen :ntim]

[Tracking Requested - why for this release]: Page rulers (relnoted for 40) will not work, and some other commands as well.

Comment 14

[Kate Glazko]

Tracking for 40, 41 because reference comment #13. Affects 40, 41.

Comment 15

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

https://reviewboard.mozilla.org/r/9547/#review9085

Simple fix that also fixes the ruler, r+.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/33cc16699f79

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/33cc16699f79

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Patrick, could you fill the uplift request to aurora? Merci!

Comment 19

[Patrick Brosset <:pbro>]

Attachment: rulers-e10s-aurora.patch

The patch that landed on m-c doesn't apply cleanly on aurora, so here's a rebased one for aurora.
Along with the approval request:


Approval Request Comment
[Feature/regressing bug #]: 1144163
[User impact if declined]: If declined, the new "rulers" button in the devtools, which allows to toggle rulers on the page (useful for designers), won't work with e10s windows.
[Describe test coverage new/current, TreeHerder]: There are automated tests in the tree already for the rulers command, and the patch has been run on try, fx-team and m-c.
[Risks and why]: That patch has been on m-c for a few days, we would have already realized if it caused any side-effects, so risk is low. Having said this, I've changed quite substantially some of the devtools highlighter classes (not the architecture though, just some data object passed around), so there could always be things we've missed.
[String/UUID change made/needed]: None

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Oh, I didn't realized that the patch was that big... :/

So, I am going to take it because:
* it is fresh, it is easier to take it now than waiting for the next merge (aurora = 41) and fix the regression in 3 or 4 weeks
* it has tests
* didn't cause regressions after 4 days in m-c

However, if complex regressions are found, we will probably backout this fix.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9f264bf59621