1137285 - Refactor inspector tests in order to be remote friendly and be run on luciddream

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Alexandre Poirot [:ochameau]]

Today, inspector tests are e10s friendly, but not remote friendly as they are still using CPOW and a frame script.
The frame script abstraction looks like an actor but isn't. There isn't that much to add in order to be able to be remote friendly.
We could convert the frame script to a dynamically registered actor and stop using CPOW.
Then we would only need just a couple of abstraction in the test in order to connect to any arbitrary target: local tab, e10s or non-e10s one, remote app, remote tab, chrome document, ...

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: wip patch

Here is a work in progress patch, converting just a few tests.
I can already split this patch in meaningful subset,
but I would like some feedback from inspector peers in order 
to ensure I'm sane and that this contribution will be accepted ;)

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Run inspector test on luciddream

Here is the luciddream magic to make it run.
I'm not really satisfied by this as it contains
a list of whitelisted files to run in the Javascript helper file.

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8569957 [details] [diff] [review]
wip patch

Review of attachment 8569957 [details] [diff] [review]:
-----------------------------------------------------------------

I'm all for this. It's indeed going to be a long refactor (I know this for a fact, I've had to re-write almost all of these tests at least once), but the added value is big, and the code doesn't look more complex at all, in fact some parts of tests are even simplified a little bit.

Could you rename 'actor' into something more self-explanatory? Like maybe 'pageActor' or something better. I'm worried about new contributors that only work on the front-end and haven't had to learn about what actors are yet. They'll be using this a lot when writing tests, and need to be able to know immediately what this is for when reading other tests.

::: browser/devtools/inspector/test/actor.js
@@ +2,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +

I'm going to assume that this file contains almost exactly the code we had in the frame-script and some of the code we had in head.js. So, that looks good to me.

@@ +3,5 @@
> + http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +// A helper frame-script for brower/devtools/inspector tests.

s/frame-script/actor

::: browser/devtools/inspector/test/browser_inspector_highlight_after_transition.js
@@ +8,1 @@
>  

Ok that's weird, this test doesn't seem to test anything at all! Not blaming your patch, it looks like it's already the case without it.
I was expecting it to test the position of the nodeinfobar, but it doesn't.

@@ +19,5 @@
> +function* checkDivHeight(inspector, actor) {
> +  let div = yield getNodeFront("div", inspector);
> +  console.log(div);
> +  let mod = div.startModifyingAttributes();
> +  mod.setAttribute("visible", "true");

I think you should add a setAttribute method on the test actor, just because it's simpler than this attribute modification batching method and because we will need to test assert changing attributes outside of the inspector code too.

::: browser/devtools/inspector/test/head.js
@@ +259,2 @@
>        info("Toolbox and inspector already open");
> +      throw new Error("Inspector is already opened, please use getActiveInspector");

Why do you want to throw here? I seems to me that calling openInspector several times should be harmless.

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Convert addTab+openInspector to openInspectorForURL v1

Ok, so I tried to split this patch is multiple independant ones...
Here is a first naive one.
addTab doesn't mean anything when speaking about random remote target like apps.
So I'm trying to ensure that all tests (most, as some really want to test addTab),
are using openInspectorForURL.

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Replace content.location = ... by a navigateTo request v1

Another really simple one. We use content.location = "http://..." to change the target document.
That's not compatible with remote targets. Instead I'm now calling TabActor.navigateTo request.

Comment 6

[Alexandre Poirot [:ochameau]]

Here is a try run for these two patches:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a98274b8922

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: wip patch

And the leftover. It is still pretty significant patch, but I don't see how to split it even more.
There is still some failures, but most tests are working locally.

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: wip patch

Fixed keybinding tests.
Now I only get one failing test, locally: 
  TEST-UNEXPECTED-FAIL | browser/devtools/inspector/test/browser_inspector_select-last-selected.js | #id4 is selected after navigation. - Got [Front for domnode/server1.conn58.domnode76], expected [Front for domnode/server1.conn58.domnode79]

Comment 9

[Alexandre Poirot [:ochameau]]

And a try with all patches to see if I get the same results on CI vs locally:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=b57f8f234f97

Patrick, do not hesitate to throw additional comments to the "wip patch", it is almost there, I'm waiting to have all tests green to request the official review.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #3)
> Comment on attachment 8569957 [details] [diff] [review]
> Could you rename 'actor' into something more self-explanatory?

Renamed to testActor!

> 
> ::: browser/devtools/inspector/test/actor.js
> @@ +2,5 @@
> > +/* Any copyright is dedicated to the Public Domain.
> > + http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> > +"use strict";
> > +
> 
> I'm going to assume that this file contains almost exactly the code we had
> in the frame-script and some of the code we had in head.js. So, that looks
> good to me.

Yes. I tried to rename the frame script into test-actor.js in the last revision,
but it failed, git still considers it as a deleted+new file :(

> 
> @@ +19,5 @@
> > +function* checkDivHeight(inspector, actor) {
> > +  let div = yield getNodeFront("div", inspector);
> > +  console.log(div);
> > +  let mod = div.startModifyingAttributes();
> > +  mod.setAttribute("visible", "true");
> 
> I think you should add a setAttribute method on the test actor, just because
> it's simpler than this attribute modification batching method and because we
> will need to test assert changing attributes outside of the inspector code
> too.

Done.

> 
> ::: browser/devtools/inspector/test/head.js
> @@ +259,2 @@
> >        info("Toolbox and inspector already open");
> > +      throw new Error("Inspector is already opened, please use getActiveInspector");
> 
> Why do you want to throw here? I seems to me that calling openInspector
> several times should be harmless.

Just to be more explicit, reusing an "already tested" inspector can introduce some randomness in our tests given which test is executed before... We have getActiveInspector to explicitely reuse one.

Comment 10

[Alexandre Poirot [:ochameau]]

Oh try is failing as I based my patches on top of bug 1134180 and forgot to push it to try.
bug 1134180 helps but I'm not sure that's a strong dependency...

Comment 11

[Patrick Brosset <:pbro>]

(In reply to Alexandre Poirot [:ochameau] from comment #7)
> Created attachment 8576770 [details] [diff] [review]
> wip patch
> 
> And the leftover. It is still pretty significant patch, but I don't see how
> to split it even more.
You could create a patch that only contains the test actor and its registration on the remote device, even if none of the tests use it yet.
And then another (big) patch removing the duplicated functions in head.js and migrating all test files.
This would make it easier to review the most important patch in this bug I guess.

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8576776 [details] [diff] [review]
wip patch

Review of attachment 8576776 [details] [diff] [review]:
-----------------------------------------------------------------

I have one request: could you measure how much time it takes to run the tests with this new setup compared to how much it takes without (locally, not with a remote device).
We've had problems in the past when servers on TRY were getting slower at times and had some of our tests failing because they were taking too much time. We've had to split some tests in 2 or more parts because of this. I'd like to make sure this doesn't slow down the tests too much when running them against a local tab as today.

Otherwise, as discussed earlier, I like the approach of going through an actor to access the test page. May I suggest that we start adding a comment block in each head.js file in directories that use this technique (or have a README somewhere) that explains the setup? Also, this page will need to be updated: https://wiki.mozilla.org/DevTools/Hacking#Running_the_Developer_Tools_Tests
Indeed, people working on the devtools for the first time may be surprised if they've been writing browser mochitests before, so it needs to be obvious why we do this, and what it is.

I haven't looked at all the details in this patch yet. I have looked at head.js, test-actor.js and doc_frame_script.js changes mostly. Those changes look good to me.
For the rest, it seems like boring, repetitive changes across all tests to move from local helpers or mm messages to testActor methods. So I'll look at those when you think it's ready for review. Also, TRY will be a lot better than me at finding problems for this type of changes.

::: browser/devtools/inspector/test/browser_inspector_select-last-selected.js
@@ +62,1 @@
>        yield selectNode(nodeToSelect, inspector);

selectNode already waits for the "inspector-updated" event, so why did you need to add another step to wait for "new-node-front" here?

@@ +65,4 @@
>      }
>  
> +    let onNewNodeFront = inspector.selection.once("new-node-front");
> +    yield navigateToAndWaitForNewRoot(toolbox, testActor, url);

Same question here.

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #12)
> May I suggest that we start adding a comment
> block in each head.js file in directories that use this technique (or have a
> README somewhere) that explains the setup?

Yes, some kind of in-tree docs sound quite helpful for this!  Maybe a docs folder with a Markdown file like we have for some server-side things already.

Comment 14

[Alexandre Poirot [:ochameau]]

Comment on attachment 8569959 [details] [diff] [review]
Run inspector test on luciddream

I'm going to move the work of actually running the tests on luciddream to bug 1149479.
I spend more time in maintaining my patch and rebasing it, rather than making it work on LD...

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: Convert addTab+openInspector to openInspectorForURL v1

Rebased.

Comment 16

[Alexandre Poirot [:ochameau]]

Attachment: Replace content.location = ... by a navigateTo request v1

Comment 17

[Alexandre Poirot [:ochameau]]

Attachment: Rename frame script to actor

In order to help reviewing the next patch, here is one that just renames doc_frame_script.js to test-actor.js.
(I don't know why, but git can't correctly handle git mv + modification correctly,
 it consider as a removal+new file)

Comment 18

[Alexandre Poirot [:ochameau]]

Attachment: Convert inspector test to be remote friendly v1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a67b89f2c1c

Tests are working locally.
I would like to land this significant refactoring,
as it is very time consuming for me to maintain this patch.
Tests do not all pass on luciddream yet,
so I'll most likely need to patch head.js again,
but at least tests are going to start using the new test actor.

About this patch, note that the TestActorFront is associated to the toolbox.
So that we don't need to pass toolbox references to many methods.
Also, when it comes to highlighters, I default to the toolbox default one,
unless we pass as argument a specific highlighter front from which we retrieve the actorID.

Overall, this patch is about replacing all usages of `content` and executeInContent
with the test actor. I haven't mapped all `content` usage to a very specific actor method,
sometimes I just use a magic `eval` method that allows doing a lot of very random things.
We should be careful with that and only uses it for the rare and test-specific action/checks.

Also, I'm considering merging all these patches into a single one before landing,
the split is here to simplify the review.

Comment 19

[Alexandre Poirot [:ochameau]]

Attachment: Replace content.location = ... by a navigateTo request v2

I don't know why I ended up modifying markup view tests... but I shouldn't.

Comment 20

[Alexandre Poirot [:ochameau]]

Comment on attachment 8586077 [details] [diff] [review]
Convert addTab+openInspector to openInspectorForURL v1

Carry over

Comment 21

[Alexandre Poirot [:ochameau]]

Comment on attachment 8586087 [details] [diff] [review]
Convert inspector test to be remote friendly v1

Review of attachment 8586087 [details] [diff] [review]:
-----------------------------------------------------------------

New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=89e4c9ab7f1b

Please read comment 17,18 before reviewing, if you want to test this patch you need to apply all the patches
and also apply bug 1145049 before.

Comment 22

[Alexandre Poirot [:ochameau]]

Comment on attachment 8586087 [details] [diff] [review]
Convert inspector test to be remote friendly v1

Review of attachment 8586087 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/inspector/test/browser_inspector_highlight_after_transition.js
@@ +17,4 @@
>  });
>  
> +function* checkDivHeight(inspector, testActor) {
> +  yield testActor.setAttribute("div", "visible", "true");

This throws, but we are not running the test.
Should I just remove it, or, (fix this test and) register it in browser.ini ?
BTW if you see tests that are pointless, please shoot!
Better removing them than refactoring them for nothing.

Comment 23

[Patrick Brosset <:pbro>]

(In reply to Alexandre Poirot [:ochameau] from comment #22)
> Comment on attachment 8586087 [details] [diff] [review]
> Convert inspector test to be remote friendly v1
> 
> Review of attachment 8586087 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> browser/devtools/inspector/test/browser_inspector_highlight_after_transition.
> js
> @@ +17,4 @@
> >  });
> >  
> > +function* checkDivHeight(inspector, testActor) {
> > +  yield testActor.setAttribute("div", "visible", "true");
> 
> This throws, but we are not running the test.
> Should I just remove it, or, (fix this test and) register it in browser.ini ?
> BTW if you see tests that are pointless, please shoot!
> Better removing them than refactoring them for nothing.
This test is obsolete, it used to test that the nodeinfobar would not appear outside of the content area, but now that the highlighter markup is part of the content document, this can't happen anymore.
So, please remove this test.

Comment 24

[Patrick Brosset <:pbro>]

Comment on attachment 8586087 [details] [diff] [review]
Convert inspector test to be remote friendly v1

Review of attachment 8586087 [details] [diff] [review]:
-----------------------------------------------------------------

That's a lot of tests to rework. Congrats for not going crazy over them.
Only minor comments below, but I'd still like to see a new version before R+.

::: browser/devtools/inspector/test/browser_inspector_highlight_after_transition.js
@@ +17,4 @@
>  });
>  
> +function* checkDivHeight(inspector, testActor) {
> +  yield testActor.setAttribute("div", "visible", "true");

As commented earlier, please remove the test.

::: browser/devtools/inspector/test/browser_inspector_initialization.js
@@ +27,5 @@
>    content.document.body.innerHTML = DOCUMENT_HTML;
>    content.document.title = "Inspector Initialization Test";
>  
> +  let deferred = promise.defer();
> +  content.setTimeout(deferred.resolve, 2000);

2000 seems like a random time to wait here, and on particularly slow test VMs might sometimes not be enough. This is an intermittent waiting to happen. Isn't there an event we could wait instead?
In any case, this at least requires a big comment saying why we need to do it.
(would disabling this test for LD be an option?)

::: browser/devtools/inspector/test/browser_inspector_pseudoclass-lock.js
@@ +8,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "DebuggerServer",
> +  "resource://gre/modules/devtools/dbg-server.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "DebuggerClient",
> +  "resource://gre/modules/devtools/dbg-client.jsm");

These getters don't seem like they're needed.

::: browser/devtools/inspector/test/head.js
@@ +169,3 @@
>  });
>  
> +// Register a test actor that can operate on the remote document

Can you move this block of code (up until _request) to a new file in /browser/devtools/shared/ ? 
Maybe in a file named test-actor-registry.js or something better.
And maybe move test-actor.js next to it.
I'm suggesting this directory because that's where we already have frame-script-utils.js which we use in several mochitest suites.
The reason why I'm asking is that so far, everytime we've added a generic test-related helper/util/function/whatever, we've added it to a head.js file, and then copy/pasted it to other head.js files over and over again.
We've been discussing lately about putting them back in common in one file (see bug1147012).
I'd hate to see this (rather complex) logic be copied to other head.js files when we want to run other tests on LD.
You can use something like this to load this new file:
Services.scriptloader.loadSubScript(testDir + "/" + filePath, this);

@@ +178,5 @@
> +  let { ActorRegistryFront } = require("devtools/server/actors/actor-registry");
> +  let front = ActorRegistryFront(client, response);
> +
> +  // Fetch the test actor sources
> +  let source = yield _request(ACTOR_URL);

What about exporting request from actor-registry and adding an option to bypass the resource:// uri check instead of having to duplicate the code here?

@@ +191,5 @@
> +  return actorActorFront;
> +});
> +
> +// Load the test actor in a custom sandbox
> +// as we can't use SDK module loader with URIs

I have to trust you on this as I know nothing about this. But it does sound sad that we need to load the test-actor source one more time and evaluate here. Do we really not have any other way to load the module?

@@ +212,5 @@
> +  let client = toolbox.target.client;
> +  return _getTestActor(client, toolbox.target.tab, toolbox);
> +});
> +
> +

nit: just one empty line to separate functions.

@@ +236,5 @@
> +  // We may have to update the form in order to get the dynamically registered
> +  // test actor.
> +  let form = yield _getUpdatedForm(client, tab);
> +
> +  let source = yield _request(ACTOR_URL);

Having to load the source once again and evaluate it is an implementation detail of _loadFront, so this line should be moved inside _loadFront.

@@ +248,5 @@
> +  let deferred = promise.defer();
> +  try {
> +    uri = Services.io.newURI(uri, null, null);
> +  } catch (e) {
> +    reject(e);

reject is undefined here I think. You probably wanted to call deferred.reject(e) instead.

::: browser/devtools/inspector/test/test-actor.js
@@ +21,4 @@
>  
> +let dumpn = msg => {
> +  dump(msg + "\n");
> +  Cu.reportError(msg);

Why report an error when we're just logging a message?

@@ +25,3 @@
>  }
>  
> +

nit: just one empty line.

@@ +54,2 @@
>  
> +  // `selector` parameter is either just a regular selector string

nit: use a jsdoc-style comment block here:

/**
 * desc
 * @param ...
 * @return ...
 */

@@ +383,5 @@
> +   * @param {String} selector selector identifier to select the DOM node
> +   * @param {String} event event name to listen to
> +   * @return {json} the DOM event object
> +   */
> +  addEventListener: protocol.method(function (selector, event) {

Maybe rename to addOneTimeEventListener since we're just listening once.

@@ +521,5 @@
> +   * @param {Object} highlighter Optional custom highlither to target
> +   * @return {String} value
> +   */
> +  getSelectorHighlighterBoxNb: protocol.custom(function(highlighter) {
> +    return this._getSelectorHighlighterBoxNb((highlighter || this.toolbox.highlighter).actorID);

The main toolbox highlighter is a BoxModelHighlighter type, it can never be a SelectorHighlighter, so for this particular method, there's no need to default to this.toolbox.highlighter if highlighter is missing, and so, no need for a custom protocol method here.

@@ +557,5 @@
> +    return this.getHighlighterNodeAttribute("box-model-elements", "hidden")
> +      .then(value => value === null);
> +  },
> +
> +  assertHighlitNode: Task.async(function* (selector) {

nit: Since you're working on this, can you take the opportunity to rename this to 'assertHighlightedNode'.

@@ +650,5 @@
>  
> +    let points = polygons[0].trim().split(" ").map(i => {
> +      return i.replace(/M|L/, "").split(",")
> +    });
> + 

nit: trailing whitespace.

@@ +758,5 @@
> +    }
> +
> +    return {d, points};
> +  })
> +

nit: extra empty line.

Comment 25

[Alexandre Poirot [:ochameau]]

Attachment: Allow ressource uri for dynamically registered actors

This isn't really useful to forbid using resource:// URIs,
as we can easily bypass the checks by calling front._registerActor
with a string fetched from a XHR or something...

So I removed this check as it prevents from using ActorActor
easily from tests!

Comment 26

[Alexandre Poirot [:ochameau]]

Attachment: Convert inspector test to be remote friendly v2

https://treeherder.mozilla.org/#/jobs?repo=try&revision=50bc2dc688e8

Comment 27

[Alexandre Poirot [:ochameau]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #24)
> Comment on attachment 8586087 [details] [diff] [review]
> 
> That's a lot of tests to rework. Congrats for not going crazy over them.

I'm especially going crazy when I get 2-3 merge to manually resolve
and new test to convert, *per week* :-/

> 
> ::: browser/devtools/inspector/test/browser_inspector_initialization.js
> @@ +27,5 @@
> >    content.document.body.innerHTML = DOCUMENT_HTML;
> >    content.document.title = "Inspector Initialization Test";
> >  
> > +  let deferred = promise.defer();
> > +  content.setTimeout(deferred.resolve, 2000);
> 
> 2000 seems like a random time to wait here, and on particularly slow test
> VMs might sometimes not be enough. This is an intermittent waiting to
> happen. Isn't there an event we could wait instead?

Replaced with a executeSoon, we just need to wait a tick here.

> ::: browser/devtools/inspector/test/head.js
> @@ +169,3 @@
> >  });
> >  
> > +// Register a test actor that can operate on the remote document
> 
> Can you move this block of code (up until _request) to a new file in
> /browser/devtools/shared/ ? 

done, in shared/tests/, test-actor.js and test-actor-registry.js

> 
> @@ +178,5 @@
> > +  let { ActorRegistryFront } = require("devtools/server/actors/actor-registry");
> > +  let front = ActorRegistryFront(client, response);
> > +
> > +  // Fetch the test actor sources
> > +  let source = yield _request(ACTOR_URL);
> 
> What about exporting request from actor-registry and adding an option to
> bypass the resource:// uri check instead of having to duplicate the code
> here?

I removed the ressouce:// uri check in a separate patch, it makes everything simplier!

> 
> @@ +191,5 @@
> > +  return actorActorFront;
> > +});
> > +
> > +// Load the test actor in a custom sandbox
> > +// as we can't use SDK module loader with URIs
> 
> I have to trust you on this as I know nothing about this. But it does sound
> sad that we need to load the test-actor source one more time and evaluate
> here. Do we really not have any other way to load the module?

The SDK loader is really only about paths,
we would have to hack the loader itself to be able to load test ressources.
Note that ActorActor doesn't use the SDK loader because of that.
May be we could something steal the front reference from ActorActor.registerActor...
I haven't looked into that yet and kept the code as-is.


> ::: browser/devtools/inspector/test/test-actor.js
> @@ +21,4 @@
> >  
> > +let dumpn = msg => {
> > +  dump(msg + "\n");
> > +  Cu.reportError(msg);
> 
> Why report an error when we're just logging a message?

It was just more handy in luciddream to see the errors in the browser console... I removed it.

> @@ +521,5 @@
> > +   * @param {Object} highlighter Optional custom highlither to target
> > +   * @return {String} value
> > +   */
> > +  getSelectorHighlighterBoxNb: protocol.custom(function(highlighter) {
> > +    return this._getSelectorHighlighterBoxNb((highlighter || this.toolbox.highlighter).actorID);
> 
> The main toolbox highlighter is a BoxModelHighlighter type, it can never be
> a SelectorHighlighter, so for this particular method, there's no need to
> default to this.toolbox.highlighter if highlighter is missing, and so, no
> need for a custom protocol method here.

Done. But I have to manually pass the actorID now. I don't know if you expected that.
It means that the test now has to do: testActor.getSelectorHighlighterBoxNb(highlighterFront.actorID)

> 
> @@ +557,5 @@
> > +    return this.getHighlighterNodeAttribute("box-model-elements", "hidden")
> > +      .then(value => value === null);
> > +  },
> > +
> > +  assertHighlitNode: Task.async(function* (selector) {
> 
> nit: Since you're working on this, can you take the opportunity to rename
> this to 'assertHighlightedNode'.

done

Comment 28

[Patrick Brosset <:pbro>]

(In reply to Alexandre Poirot [:ochameau] from comment #27)
> > @@ +521,5 @@
> > > +   * @param {Object} highlighter Optional custom highlither to target
> > > +   * @return {String} value
> > > +   */
> > > +  getSelectorHighlighterBoxNb: protocol.custom(function(highlighter) {
> > > +    return this._getSelectorHighlighterBoxNb((highlighter || this.toolbox.highlighter).actorID);
> > 
> > The main toolbox highlighter is a BoxModelHighlighter type, it can never be
> > a SelectorHighlighter, so for this particular method, there's no need to
> > default to this.toolbox.highlighter if highlighter is missing, and so, no
> > need for a custom protocol method here.
> 
> Done. But I have to manually pass the actorID now. I don't know if you
> expected that.
> It means that the test now has to do:
> testActor.getSelectorHighlighterBoxNb(highlighterFront.actorID)
Ah I see, so if for consistency reason you prefer to have tests never deal with IDs, and let the front do this, then I'm fine to keep the custom front method to do
this._getSelectorHighlighterBoxNb(highlighter.actorID);
but we just don't need this.toolbox.highlighter in this method, that's for sure.

Comment 29

[Patrick Brosset <:pbro>]

Comment on attachment 8589308 [details] [diff] [review]
Convert inspector test to be remote friendly v2

Review of attachment 8589308 [details] [diff] [review]:
-----------------------------------------------------------------

I have only looked at the most important files: test-actor.js, test-actor-registry.js and head.js, and have assumed the changes to the test files were similar to the ones I had reviewed last time (hard without a proper interdiff mechanism, btw, I've been using mozreview for a while, and their interdiff works like a charm so far, I can only encourage you to switch over to it).
Anyway, those latest changes do look great to me. Thanks for fixing everything I commented about.
I only have a couple of remaining comments, but other than this, r=me for this.

::: browser/devtools/shared/test/browser.ini
@@ +12,5 @@
>    doc_options-view.xul
>    head.js
>    leakhunt.js
> +  test-actor.js
> +  test-actor-registry.js

I might be wrong, but I don't think you need to register these 2 files in this browser.ini file.

::: browser/devtools/shared/test/test-actor-registry.js
@@ +18,5 @@
> +let ROOT_TEST_DIR = getRootDirectory(gTestPath);
> +let ACTOR_URL = ROOT_TEST_DIR + "test-actor.js";
> +
> +// Register a test actor that can operate on the remote document
> +exports.registerTestActor = Task.async(function* (client) {

Don't you need to import Task.jsm for this to work?

@@ +29,5 @@
> +  let front = ActorRegistryFront(client, response);
> +
> +  // Then ask to register our test-actor to retrieve its front
> +  let actorActorFront = yield front.registerActor(ACTOR_URL, { type: { tab: true }, constructor: "TestActor", prefix: "testActor" });
> +  return actorActorFront;

Wow, actorActorFront! So meta.

Comment 30

[Brian Grinstead [:bgrins]]

We need to make inspector test changes (including adding a new in-page-content function) in Bug 901250.  Marking this as blocking so it doesn't require another rebase, but hoping this lands asap so work there can proceed

Comment 31

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8589307 [details] [diff] [review]
Allow ressource uri for dynamically registered actors

(In reply to Alexandre Poirot [:ochameau] from comment #25)
> Created attachment 8589307 [details] [diff] [review]
> Allow ressource uri for dynamically registered actors
> 
> This isn't really useful to forbid using resource:// URIs,
> as we can easily bypass the checks by calling front._registerActor
> with a string fetched from a XHR or something...
> 
> So I removed this check as it prevents from using ActorActor
> easily from tests!

Looks ok to me. Nick, I know you've appended the check at some point and I left it there. Did you have any special reason to do it?

Honza

Comment 32

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

IIRC, disallowing resource URIs was done because it was a security concern, but I don't really remember the details. Passing ni on to Mark.

Comment 33

[Mark Goodwin [:mgoodwin]]

The intent was to make it hard(er) for untrusted content to find its way into an actor - I'm not sure whether it was possible to register actors from a string (as per comment 25) at the time.

If there are ways around this, there may not be benefits in doing this.

Comment 34

[Alexandre Poirot [:ochameau]]

Yes, I had a patch that worked around this check by calling the original actor method, that expects a string.
We have to use a string as the child process can be remote, on another device and won't have access to chrome/resource URIs. So the client has to pipe the JS source to the server.

Comment 35

[Alexandre Poirot [:ochameau]]

(In reply to Brian Grinstead [:bgrins] from comment #30)
> We need to make inspector test changes (including adding a new
> in-page-content function) in Bug 901250.  Marking this as blocking so it
> doesn't require another rebase, but hoping this lands asap so work there can
> proceed

Thanks a lot for waiting for my changes! It helps a lot. I'm almost there...
I'm currently stuck on some issue with bug 1145049.
It looks like the refactoring isn't green if I don't pull bug 1145049.
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbe877e5903b

But I'll try to land the green patch from bug 1145049, may be that will be enough.

Comment 36

[Alexandre Poirot [:ochameau]]

(In reply to Alexandre Poirot [:ochameau] from comment #35)
> But I'll try to land the green patch from bug 1145049, may be that will be
> enough.

Looks like it isn't enough, I got a leak reported and new other test failure if I don't take all bug patches:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=d885d6a3c503
  (this try doesn't take the InspectorActor.destroy/disconnect patch)

So I think I really have to take all the patches from bug 1145049.
But I don't get why browser/devtools/shared/test/browser_toolbar_tooltip.js fails with all the patches:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a3fd9064c38
I can't easily reproduce it localy wheread it is a perma-fail on try.

I'm starting to consider blacklisting this test in order to allow landing this and figuring this out in a followup.

Comment 37

[Alexandre Poirot [:ochameau]]

Attachment: Fix browser_toolbar_webconsole_errors_count when run independently

Comment 38

[Alexandre Poirot [:ochameau]]

Attachment: Cleanup correctly tests run from browser/devtools/shared/test

Comment 39

[Alexandre Poirot [:ochameau]]

Attachment: Fix exception when using SimpleTest.expectUncaughtException

Comment 40

[Brian Grinstead [:bgrins]]

Comment on attachment 8590505 [details] [diff] [review]
Fix browser_toolbar_webconsole_errors_count when run independently

Review of attachment 8590505 [details] [diff] [review]:
-----------------------------------------------------------------

Please add a note to the commit message about how DeveloperToolbar.show returns a promise

Comment 41

[Alexandre Poirot [:ochameau]]

Comment on attachment 8590507 [details] [diff] [review]
Fix exception when using SimpleTest.expectUncaughtException

Wrong bug.

Comment 42

[Alexandre Poirot [:ochameau]]

New try, with new patches, hopefully green now:
  https://hg.mozilla.org/try/pushloghtml?changeset=1744b443412d

Comment 43

[Brian Grinstead [:bgrins]]

(In reply to Alexandre Poirot [:ochameau] from comment #42)
> New try, with new patches, hopefully green now:
>   https://hg.mozilla.org/try/pushloghtml?changeset=1744b443412d

Looks like there is just one more.. a leak in inspector/test/browser_inspector_highlighter-by-type.js: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1744b443412d

Comment 44

[Alexandre Poirot [:ochameau]]

(In reply to Brian Grinstead [:bgrins] from comment #43)
> (In reply to Alexandre Poirot [:ochameau] from comment #42)
> > New try, with new patches, hopefully green now:
> >   https://hg.mozilla.org/try/pushloghtml?changeset=1744b443412d
> 
> Looks like there is just one more.. a leak in
> inspector/test/browser_inspector_highlighter-by-type.js:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=1744b443412d

Finally identified the leak and many others... I'll attach yet another new patch on the pile shortly.

Comment 45

[Alexandre Poirot [:ochameau]]

Attachment: Allow ressource uri for dynamically registered actors.

Rebased.

Comment 46

[Alexandre Poirot [:ochameau]]

This leaks comes from a patch in bug 1145049.
Here is a new try push with updated patch:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f274933ca9c

Comment 47

[Alexandre Poirot [:ochameau]]

Crazy! When I fix leaks, tbpl reports me new ones...

Comment 48

[Alexandre Poirot [:ochameau]]

I finally got a green try with all the patches!
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=7737aa9f7b26

But here is another one, I had to rebase the refactoring patch again this morning...
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=1352523e3b98

Comment 49

[Alexandre Poirot [:ochameau]]

Attachment: Convert inspector test to be remote friendly v3

Rebased.

Comment 50

[Alexandre Poirot [:ochameau]]

New leaks identified in styles.js, yet another new fix and new try:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=de24e28b692d

Comment 51

[Alexandre Poirot [:ochameau]]

Comment on attachment 8592136 [details] [diff] [review]
Convert inspector test to be remote friendly v3

Try is green!

Comment 52

[Alexandre Poirot [:ochameau]]

Green try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=90fc5e34d026

Landed on fx-team:
https://hg.mozilla.org/integration/fx-team/rev/145f6347bb11
https://hg.mozilla.org/integration/fx-team/rev/120d272971a6
https://hg.mozilla.org/integration/fx-team/rev/998c4e53c1f8
https://hg.mozilla.org/integration/fx-team/rev/3570dbae06e2

Comment 53

[Carsten Book [:Tomcat]]

backd this out since the depending bug  bug 1145049 was backed out

Comment 54

[Alexandre Poirot [:ochameau]]

New try with a simplier patch to cleanup inspector actor:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a0f3d3286a4
Let's see if that's enough to make the new remote test to pass.

Comment 55

[Alexandre Poirot [:ochameau]]

I'll try to at least flush the patch that don't break anything:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a22fd59097c

Comment 56

[Alexandre Poirot [:ochameau]]

Bug 1155168 seems to help, at least it fixes the leak locally:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=b204731a312a

Comment 57

[Pulsebot]

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

Comment 58

[Alexandre Poirot [:ochameau]]

Just landed attachment 8591632 [details] [diff] [review], that to unlock bug 1155168, that itself should allow landid the other patches in this bug...

Comment 59

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fdb02f71f91d

Comment 60

[(not currently active) Ted Mielczarek]

If there are techniques here we could factor out into the luciddream harness to make writing these sorts of tests easier I would be all for that.

Comment 61

[Alexandre Poirot [:ochameau]]

Oops should have set leave-open... Still miss some patches to land.

Comment 62

[Alexandre Poirot [:ochameau]]

Attachment: Convert addTab+openInspector to openInspectorForURL v3

Just some rebase.

Comment 63

[Alexandre Poirot [:ochameau]]

Attachment: Replace content.location = ... by a navigateTo request v3

Comment 64

[Alexandre Poirot [:ochameau]]

Attachment: Convert inspector test to be remote friendly v4

Comment 65

[Alexandre Poirot [:ochameau]]

Comment on attachment 8650452 [details] [diff] [review]
Convert addTab+openInspector to openInspectorForURL v3

Carrying over review after rebase for these two simple patches.

Comment 66

[Alexandre Poirot [:ochameau]]

Attachment: Fix browser/devtools/inspector/test/browser_inspector_delete-selected-node-01.js

I had to patch this test in order to prevent a race on e10s.
Markup-view call showBrieflyBoxModel, which spawn showBoxModel/hideBoxModel requests.
And that doesn't delay `inspector-updated` event. See onNewSelection.
The test correctly waits for inspector-updated, we should ensure
that this event is fired *after* the requests are all done.

In this patch, I took care of note delaying the call to showNode()
by waiting on showBrieflyBoxModel. Instead both run in parallel.

Comment 67

[Alexandre Poirot [:ochameau]]

Comment on attachment 8650454 [details] [diff] [review]
Convert inspector test to be remote friendly v4

You already r+ this patch, and this is just a rebase. But this one is not naive and I had to resolve merge conflict since April...
Feel free to take a look at other rebased patches as well.

This is now green:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=08bb43bf4a51
(with the additional fix I just asked for review)

(Note that this is based on top of bug 1195825 if you want to apply it.)

Also, this refactoring isn't enough to make all tests to work on luciddream,
I'll most likely need another refactoring step (or a lot of random fixes) to get them all up on running there.

Comment 68

[Patrick Brosset <:pbro>]

Comment on attachment 8650457 [details] [diff] [review]
Fix browser/devtools/inspector/test/browser_inspector_delete-selected-node-01.js

Review of attachment 8650457 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Thanks Alex for cleaning this up!

Comment 69

[Patrick Brosset <:pbro>]

Comment on attachment 8650454 [details] [diff] [review]
Convert inspector test to be remote friendly v4

Review of attachment 8650454 [details] [diff] [review]:
-----------------------------------------------------------------

I've not spent a huge amount of time reviewing this rebased patch to be honest. I had already R+'d it and looked at it thoroughly in the past. And I'm not seeing a lot of differences in this new version, so I'm going to R+ it again.

::: browser/devtools/shared/test/test-actor-registry.js
@@ +48,5 @@
> +  Cu.evalInSandbox(sourceText, sandbox, "1.8", ACTOR_URL, 1);
> +  return sandbox.exports;
> +});
> +
> +let _getUpdatedForm = function (client, tab) {

Just a remark, you don't have to do this: since we're explicitly exporting public members on 'exports', I don't see a big reason to prefix private members with an underscore.

Comment 70

[Alexandre Poirot [:ochameau]]

Comment on attachment 8650457 [details] [diff] [review]
Fix browser/devtools/inspector/test/browser_inspector_delete-selected-node-01.js

Moving this patch to bug 1195825 as this race seems to be related to promise change rather than the refactoring.

Comment 71

[Alexandre Poirot [:ochameau]]

Attachment: Convert inspector test to be remote friendly v5

Removed `_` prefixes.

Comment 72

[Alexandre Poirot [:ochameau]]

All green but a leak from dependency bug 1195825.

Comment 73

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f72b3cb19b5d
https://hg.mozilla.org/integration/fx-team/rev/0c0b99881870
https://hg.mozilla.org/integration/fx-team/rev/06e01800d4fe

Comment 74

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f72b3cb19b5d
https://hg.mozilla.org/mozilla-central/rev/0c0b99881870
https://hg.mozilla.org/mozilla-central/rev/06e01800d4fe

Comment 75

[Alexandre Poirot [:ochameau]]

\o/ Now, the tests still don't run correctly on luciddream, but at least they theoretically can.
Some already pass, may we can start running some.