Stop exposing the tabActor to commands

RESOLVED FIXED in Firefox 40

Status

defect
RESOLVED FIXED
5 years ago
11 months ago

People

(Reporter: jwalker, Assigned: pbro)

Tracking

unspecified
Firefox 41
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox39 unaffected, firefox40+ fixed, firefox41+ fixed)

Details

Attachments

(2 attachments)

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
See Also: → 1164887
Blocks: 1169246
Duplicate of this bug: 1164887
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).
Attachment #8612279 - Flags: review?(jwalker)
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.
Attachment #8612279 - Flags: review?(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();
}
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 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!
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?
(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 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).
Attachment #8612279 - Attachment description: MozReview Request: Bug 1147826 - Remove deprecated TabActor reference in runAt:server GCLI commands; r=jwalker → MozReview Request: Bug 1147826 - Remove deprecated TabActor reference in runAt:server GCLI commands; r=jwalker; r=miker
Attachment #8612279 - Flags: review+ → review?(mratcliffe)
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).
(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.
[Tracking Requested - why for this release]: Page rulers (relnoted for 40) will not work, and some other commands as well.
Tracking for 40, 41 because reference comment #13. Affects 40, 41.
Attachment #8612279 - Flags: review?(mratcliffe) → review+
https://hg.mozilla.org/mozilla-central/rev/33cc16699f79
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Patrick, could you fill the uplift request to aurora? Merci!
Flags: needinfo?(pbrosset)
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
Flags: needinfo?(pbrosset)
Attachment #8617827 - Flags: approval-mozilla-aurora?
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.
Attachment #8617827 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.