916086 - Remove some more enablePrivilege calls

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Martijn Wargers (dead)]

Again some removal of enablePrivilege calls, in content/dom/layout.

Comment 1

[Martijn Wargers (dead)]

Attachment: 916086.diff

Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=8848e2b865c3

Comment 2

[Martijn Wargers (dead)]

Try shows failures in:
layout/base/tests/test_bug450930.xhtml
dom/plugins/test/mochitest/test_cocoa_focus.html (this one shows up in mochitest and oth, interestingly)

With the unpatched version of Firefox, I already get this error on my Macbook pro on 10.7 with test_cocoa_focus.html:
 0:31.19 JavaScript error: http://mochi.test:8888/tests/dom/plugins/test/mochitest/cocoa_focus.html, line 63: Error calling method on NPObject!
So I'll leave that test file out of this patch.

I'll look at test_bug450930.xhtml, I might know what's going wrong there.

Comment 3

[Martijn Wargers (dead)]

Attachment: 916086.diff

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

Comment 4

[Martijn Wargers (dead)]

Comment on attachment 804720 [details] [diff] [review]
916086.diff

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

Ok, try all green.

Comment 5

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

Comment on attachment 804720 [details] [diff] [review]
916086.diff

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

minor details here, but overall it looks good.

::: content/smil/test/test_smilTextZoom.xhtml
@@ +48,3 @@
>  
>    // Set text zoom to 2x
> +  var origTextZoom =  SpecialPowers.getTextZoom(window);

why not use the specialpowers.wrap to get mudv?

::: content/xbl/test/test_bug378518.xul
@@ +55,3 @@
>        var wu  =
>          window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>                .getInterface(Components.interfaces.nsIDOMWindowUtils);

isn't there a specialpowers.getDOMWindowUtils you could use?

::: dom/plugins/test/mochitest/test_bug751809.html
@@ +21,4 @@
>  
>    const Ci = Components.interfaces;
>    const utils = window.QueryInterface(Ci.nsIInterfaceRequestor).
>                                      getInterface(Ci.nsIDOMWindowUtils);

can we use the SpecialPowers.getDOMWindowUtils here?  why would we not need it?

::: dom/plugins/test/mochitest/test_convertpoint.xul
@@ +32,2 @@
>    var domWindowUtils = window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>                               .getInterface(Components.interfaces.nsIDOMWindowUtils);

do we not need SpecialPowers.getDOMWindowUtils?

::: layout/base/tests/chrome/Makefile.in
@@ +6,5 @@
>  	test_bug370436.html \
>  	test_bug396367-1.html \
>  	test_bug396367-2.html \
>  	test_bug420499.xul \
> +  test_bug458898.html \

please ensure the indentation here is identical to that of the file

::: layout/forms/test/test_bug402198.html
@@ +57,4 @@
>  
>    is(0, 0, "this is a crash/assertion test, so we're ok if we survived this far");
>    SimpleTest.finish();
> +  setTimeout(function() {document.body.style.display = '';}, 400);

why is this added?

::: testing/mochitest/tests/SimpleTest/EventUtils.js
@@ +43,5 @@
>  
>  this.$ = this.getElement;
>  
>  function sendMouseEvent(aEvent, aTarget, aWindow) {
> +  if (['click', 'contextmenu', 'dblclick', 'mousedown', 'mouseup', 'mouseover', 'mouseout'].indexOf(aEvent.type) == -1) {

so we are adding in a 'contextmenu' item here?

@@ +73,5 @@
>    var ctrlKeyArg       = aEvent.ctrlKey       || false;
>    var altKeyArg        = aEvent.altKey        || false;
>    var shiftKeyArg      = aEvent.shiftKey      || false;
>    var metaKeyArg       = aEvent.metaKey       || false;
> +  var buttonArg        = aEvent.button        || (aEvent.type == 'contextmenu' ? 2 : 0);

is this compatible with all operating systems?

@@ +81,5 @@
>                         screenXArg, screenYArg, clientXArg, clientYArg,
>                         ctrlKeyArg, altKeyArg, shiftKeyArg, metaKeyArg,
>                         buttonArg, relatedTargetArg);
>  
> +  return SpecialPowers.dispatchEvent(aWindow, aTarget, event);

why do you need to return here?

Comment 6

[Martijn Wargers (dead)]

(In reply to Joel Maher (:jmaher) from comment #5)
> >    // Set text zoom to 2x
> > +  var origTextZoom =  SpecialPowers.getTextZoom(window);
> 
> why not use the specialpowers.wrap to get mudv?

Well, there is the  SpecialPowers.getTextZoom method, why not use it here, where it seems appropriate. It's much cleaner than all this queryinterfacing.

> ::: content/xbl/test/test_bug378518.xul
> ::: dom/plugins/test/mochitest/test_bug751809.html
> ::: dom/plugins/test/mochitest/test_convertpoint.xul
> do we not need SpecialPowers.getDOMWindowUtils?

For those 3 files, they are mochitest-chrome test files, so it wasn't necessary to convert those to use SpecialPowers, but I can do that if you prefer.

> > +  setTimeout(function() {document.body.style.display = '';}, 400);
> 
> why is this added?

Just for convenience, when you want to see the results after you've run this as a standalone test. Otherwise, you would look at a blank page.
 
> ::: testing/mochitest/tests/SimpleTest/EventUtils.js
> @@ +43,5 @@
> >  
> >  this.$ = this.getElement;
> >  
> >  function sendMouseEvent(aEvent, aTarget, aWindow) {
> > +  if (['click', 'contextmenu', 'dblclick', 'mousedown', 'mouseup', 'mouseover', 'mouseout'].indexOf(aEvent.type) == -1) {
> 
> so we are adding in a 'contextmenu' item here?
> 
> @@ +73,5 @@
> >    var ctrlKeyArg       = aEvent.ctrlKey       || false;
> >    var altKeyArg        = aEvent.altKey        || false;
> >    var shiftKeyArg      = aEvent.shiftKey      || false;
> >    var metaKeyArg       = aEvent.metaKey       || false;
> > +  var buttonArg        = aEvent.button        || (aEvent.type == 'contextmenu' ? 2 : 0);
> 
> is this compatible with all operating systems?

I don't know, perhaps Olli would know. Olli, I'm using this in test_bug486990.xul. I know there are a couple more places where this would be handy.
But perhaps I should move this into a new bug.

> > +  return SpecialPowers.dispatchEvent(aWindow, aTarget, event);
> 
> why do you need to return here?

It's not really needed, but regular dispatchEvent and SpecialPowers.dispatchEvent return something so it seems appropriate if this method would return that result.

Comment 7

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

no need to change anything, those were just questions I had, thanks for explaining.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

We do default button 2 to contextmenu,
http://mxr.mozilla.org/mozilla-central/source/widget/nsGUIEvent.h#1052

Comment 9

[Martijn Wargers (dead)]

Attachment: 916086.diff (for check-in)

Ok, this has the indentation thing fixed and Olli is ok with the contextmenu code in Eventutils.

Comment 10

[Martijn Wargers (dead)]

Attachment: 916086.diff (for check-in)

Sorry, this is the correct patch for check-in.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37dd897d1c32

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/37dd897d1c32