1041563 - Remove ChromePowers.js, part 2

Status: RESOLVED DUPLICATE of bug 1561705

(Testing :: Mochitest, defect)

Description

[Martijn Wargers (dead)]

There are several places where ChromePowers are use:
http://mxr.mozilla.org/mozilla-central/search?string=ChromePowers

For docshell/test/chrome/bug293235_window.xul and docshell/test/chrome/bug396649_window.xul , I think those instances can be removed safely. I forgot those to fix, as part of bug 945781.

But ChromePowers is also used by browser-chrome and marionette ui tests on the emulator. It might be more difficult for those frameworks to accept specialpowers.js instead of ChromePowers.js.

Comment 1

[Martijn Wargers (dead)]

Attachment: 1041563.diff

This is lame, but it seems to work for browser-chrome tests. I think it should work for the marionette ui tests too.

I think it is possibly to really get rid of ChromePowers.js, though. What ChromePowers.js currently is doing, is redefining some methods of specialpowers.js that would otherwise fail in browser-chrome/chrome tests.

Comment 2

[Martijn Wargers (dead)]

Attachment: 1041563_v2.diff

With the limited testing I did, this seems to work.

Comment 3

[Martijn Wargers (dead)]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=1ebec19aab9b

Comment 4

[Martijn Wargers (dead)]

Attachment: 1041563_v3.diff

The previous try run showed all kinds of failures. The timeouts happened at the end of a mochitest run.
This was my latest attempt at fixing these failure. But I haven't been able to fix all js errors coming from specialpowers.js.

Comment 5

[Martijn Wargers (dead)]

Attachment: 1041563_small.diff

At least these instances of chromepowers can be removed safely (which I already should have done as part of bug 945781).

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8460149 [details] [diff] [review]
1041563_small.diff

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

apologies for the delay- looks great!

Comment 7

[Martijn Wargers (dead)]

Ok, some history, because I keep forgetting about how stuff works.
In bug 676274, this ChromePowers.js file was created. This was done becasue "we didn't want to special case every privileged call for specialpowers or chrome" (comment 0 of that bug).
What was done (comment 2 of that bug):
refactor the specialpowers code into:
specialpowers.js -> 
 - specialpowers.js (binding glue and message manager api)
 - ChromePowers.js (fake backend to the api)
 - specialpowersAPI.js (api the tests use)
SpecialPowersObserver.js ->
 - SpecialPowersObserver.js (binding glue and message manage backend)
 - ChromePowers.js (fake backend of the observer api)
 - SpecialPowersObserverAPI.js (api the message manager uses)

So the specialpowersAPI.js was created, because that contains the code that's used by all mochitest types.
specialpowers.js is used by mochitest-plain.
ChromePowers.js is used by chrome/browser-chrome mochitests instead of specialpowers.js.
So everything that didn't seem to work in specialpowers.js was being replaced in ChromePowers.js (those functions were rewritten to work in browser-chrome/chrome tests).

The specialpowers/mochitest code has much stabilized since then, and it should be possible to get rid of ChromePowers.js, without complicating specialpowers.js too much.

Since I wasn't able to remove ChromePowers.js directly, I could try to remove as much as possible.
That means that I would need to move the used functions in there, to specialPowersAPI.js. So those functions also need to move from specialpowers.js to specialPowersAPI.js.

The functions that at least should be easy to move from ChromePowers.js to specialpowersAPI.js are:
sanityCheck, quit, focus, executeAfterFlushingMessageQueue
The rest shouldn't be too difficult either, but might be a bit more involved.

Comment 8

[Martijn Wargers (dead)]

Attachment: 1041563_v4.diff

Ok, pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=770566113b18
This moves various specialpowers methods from specialpowers.js/chromepowers.js to specialPowersAPI.js.
Local testing on mochitest-plain, mochitest-browser, mochitest-chrome seemed to go well, fwiw.

Comment 9

[Martijn Wargers (dead)]

Comment on attachment 8462990 [details] [diff] [review]
1041563_v4.diff

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

Tryserver is green.

Comment 10

[Martijn Wargers (dead)]

What remains in ChromePowers.js is _sendSyncMessage, _sendAsyncMessage and _receiveMessage
I'm only seeing one consumer of those function and that's "_sendSyncMessage"
http://mxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowersAPI.js#705
So I think that ChromePowers._sendSyncMessage could be easily integrated into the specialpowersAPI.js one and then those functions can be removed, I think.

The __exposedProps__ stuff should be removed, because it was removed for specialpowers.js too, see bug 1038844.

Then we're left with only a small amount of code, that should be possible to integrate into specialpowers.js. Although this could be the trickiest part, with all the window type checks, I've burned myself already in my initial patch.

I think the whole SpecialPowers api needs to be test in a browser-chrome test, btw. Parts of them are working (the ones that are needed by current browser-chrome tests), but I have my doubts about the parts of the api that are making use of the specialpowersobserver api.
I think those could be made to work, if they aren't. But anyhow, a dedicated browser-chrome test, that would test most/all of the specialpowers api would be handy.

Comment 11

[Martijn Wargers (dead)]

Attachment: 1041563_tests.diff

I basically copied these tests from testing/mochitest/tests/Harness_sanity/test_SpecialPowersExtension.html
They were almost working, I only had to add "this.Components = Components;" and "ChromePowers.prototype.Components = undefined;" to ChromePowers.js to get those to work.

I tested the browser-chrome test also for e10s, they are passing there too.

Btw, I made this change in the patch, too, for ChromePowers.js:
-  if (typeof(window) == "ChromeWindow" && typeof(content.window) == "Window") {
+  if ((window instanceof ChromeWindow) && (content && content.window instanceof Window)) {

It turned out that line of code never worked correctly, typeof(window) would return 'object'.
So since it never seemed to work and that if.. statement was never reached, perhaps, that whole line of code can be removed? I'm not exactly sure what it is supposed to try to solve.

I haven't ran this patch on tryserver yet.

Comment 12

[Martijn Wargers (dead)]

Attachment: 1041563_final.diff

Ok, it turned out that the name 'SpecialPowers' as the constructor was causing the problems with removing ChromePowers.
On mochitest-plain this wasn't a problem, because there, this code is carried out in its own sandbox. But chrome/browser-chrome specialpowers is carried out in the same window.
So renaming 'SpecialPowers' constructor function to '_SpecialPowers' works, but perhaps you know a better name/way?

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8462990 [details] [diff] [review]
1041563_v4.diff

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

just one question, otherwise this looks great.

::: testing/specialpowers/content/specialpowersAPI.js
@@ +1828,5 @@
> +    return "foo";
> +  },
> +
> +  get DOMWindowUtils() {
> +    return bindDOMWindowUtils(this.window.get());

We have this, but not a local this.domWindowUtils variable defined.  Is that going to be a problem?  Does this get definition act the same?  I assume so since the try server run tells us :)

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8463150 [details] [diff] [review]
1041563_tests.diff

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

Is there anyway we can have test_specialpowers.html and browser_specialpowers.js use the same .js file?  They both are duplicated.  Is this the same code that is for the mochitest.ini (mochitest plain) version as well?

::: testing/mochitest/chrome/test_SpecialPowers.html
@@ +38,5 @@
> +dump("\nSPECIALPTEST:::Test script loaded " + (new Date).getTime() + "\n");
> +
> +var startTime = new Date();
> +
> +function starttest(){

nit: space between () and {

@@ +77,5 @@
> +  SpecialPowers.removeChromeEventListener("TestEvent", testEventListener, true);
> +  SpecialPowers.removeChromeEventListener("TestEvent", testEventListener2, true);
> +  dispatchTestEvent();
> +  is(eventCount, 2, "Shouldn't have got an event!");
> +  

nit, extra whitespace on the blank line, make it a blank line

@@ +85,5 @@
> +
> +  // Test a DOMWindowUtils method and property
> +  is(SpecialPowers.DOMWindowUtils.getClassName(window), "Proxy");
> +  is(SpecialPowers.DOMWindowUtils.docCharsetIsForced, false);
> +  

nit, extra whitespace on the blank line, make it a blank line

@@ +91,5 @@
> +  is(SpecialPowers.can_QI(SpecialPowers), false);
> +  ok(SpecialPowers.can_QI(window));
> +  ok(SpecialPowers.do_QueryInterface(window, "nsIDOMWindow"));
> +  is(SpecialPowers.getPrivilegedProps(SpecialPowers.do_QueryInterface(window, "nsIDOMWindow"), "document.nodeName"), "#document");
> +  

nit, extra whitespace on the blank line, make it a blank line

@@ +155,5 @@
> +    SpecialPowers.wrap(thrower)();
> +    ok(false, "Should have thrown");
> +  } catch (e) {
> +    ok(SpecialPowers.isWrapper(e), "Exceptions should be wrapped for call");
> +    is(e.message, 'hah', "Correct message");

lets use a different error than 'hah', maybe 'wrap and throw test'

@@ +189,5 @@
> +  ok(SpecialPowers.MockPermissionPrompt, "check mock permission prompt");
> +
> +  // Set a pref using pushPrefEnv to make sure that flushPrefEnv is called
> +  SpecialPowers.pushPrefEnv({set: [['testing.some_arbitrary_pref', true]]},
> +                            function() { SimpleTest.finish(); });

how do we verify flushprefenv?

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8463223 [details] [diff] [review]
1041563_final.diff

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

this patch looks great!

Comment 16

[Martijn Wargers (dead)]

(In reply to Joel Maher (:jmaher) from comment #13)
> > +  get DOMWindowUtils() {
> > +    return bindDOMWindowUtils(this.window.get());
> 
> We have this, but not a local this.domWindowUtils variable defined.  Is that
> going to be a problem?  Does this get definition act the same?  I assume so
> since the try server run tells us :)

I might have gotten some strict warnings because of this with the tests in browser-chrome mochitests, because those are run, in strict mode. I would have to look into it.
In the end, this acts the same, but this patch would have this getter function run, every time SpecialPowers.DOMWindowUtils is called. If you set SpecialPowers.DOMWindowUtils in the constructor, it is supposedly cached (and potentially quicker that way, but I don't think it matters much).

Comment 17

[Martijn Wargers (dead)]

(In reply to Joel Maher (:jmaher) from comment #14)
> Comment on attachment 8463150 [details] [diff] [review]
> 1041563_tests.diff
> 
> Review of attachment 8463150 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is there anyway we can have test_specialpowers.html and
> browser_specialpowers.js use the same .js file?  They both are duplicated. 
> Is this the same code that is for the mochitest.ini (mochitest plain)
> version as well?

Yeah, they are mostly the same code, also the one from mochitest-plain. I guess I should take a look, if I could share the code.

> > +  // Set a pref using pushPrefEnv to make sure that flushPrefEnv is called
> > +  SpecialPowers.pushPrefEnv({set: [['testing.some_arbitrary_pref', true]]},
> > +                            function() { SimpleTest.finish(); });
> 
> how do we verify flushprefenv?

It's not tested here, I guess I forgot to add test_SpecialPowersExtension2.html , because there it is tested that this pref would go away, correctly. So I guess I should add that one, too.
There are more tests of the SpecialPowers api that could be added to chrome/browser-chrome, but for now, I just needed some basic test, that made sure my patches were doing things correctly.

Comment 18

[Martijn Wargers (dead)]

(In reply to Joel Maher (:jmaher) from comment #15)
> Comment on attachment 8463223 [details] [diff] [review]
> 1041563_final.diff
> 
> Review of attachment 8463223 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this patch looks great!

Ok, thanks, but I thought renaming SpecialPowers to _SpecialPowers looks kind of lame. Perhaps renaming it to SpecialPowersConstructor is better?

Comment 19

[Martijn Wargers (dead)]

Attachment: 1041563_v5.diff

Sorry, I was busy with this, but I hadn't got the time to get your suggestions about the tests to do yet.
I've now merged the ChromePowers.js removal into 1 patch now.
I tried to follow your suggestions

Comment 20

[Martijn Wargers (dead)]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #11)
> Btw, I made this change in the patch, too, for ChromePowers.js:
> -  if (typeof(window) == "ChromeWindow" && typeof(content.window) ==
> "Window") {
> +  if ((window instanceof ChromeWindow) && (content && content.window
> instanceof Window)) {
> 
> It turned out that line of code never worked correctly, typeof(window) would
> return 'object'.
> So since it never seemed to work and that if.. statement was never reached,
> perhaps, that whole line of code can be removed? I'm not exactly sure what
> it is supposed to try to solve.

I just removed this line now in the latest patch.
I've now used the name "SpecialPowersConstructor" instead of "_SpecialPowers".

> > We have this, but not a local this.domWindowUtils variable defined.  Is that
> > going to be a problem?  Does this get definition act the same?  I assume so
> > since the try server run tells us :)
> 
> I might have gotten some strict warnings because of this with the tests in
> browser-chrome mochitests, because those are run, in strict mode. I would
> have to look into it.
> In the end, this acts the same, but this patch would have this getter
> function run, every time SpecialPowers.DOMWindowUtils is called. If you set
> SpecialPowers.DOMWindowUtils in the constructor, it is supposedly cached
> (and potentially quicker that way, but I don't think it matters much).

I've now reverted back to use this.DOMWindowUtils in the constructor.