Closed Bug 916086 Opened 6 years ago Closed 6 years ago

Remove some more enablePrivilege calls

Categories

(Testing :: Mochitest, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla27

People

(Reporter: martijn.martijn, Assigned: martijn.martijn)

References

Details

Attachments

(1 file, 3 obsolete files)

Again some removal of enablePrivilege calls, in content/dom/layout.
Summary: Remove some more enablePrivilige call → Remove some more enablePrivilige calls
Summary: Remove some more enablePrivilige calls → Remove some more enablePrivilege calls
Attached patch 916086.diff (obsolete) — Splinter Review
Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=8848e2b865c3
Assignee: nobody → martijn.martijn
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.
Attached patch 916086.diff (obsolete) — Splinter Review
Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=e671b5161e44
Attachment #804419 - Attachment is obsolete: true
Comment on attachment 804720 [details] [diff] [review]
916086.diff

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

Ok, try all green.
Attachment #804720 - Flags: review?(jmaher)
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?
Attachment #804720 - Flags: review?(jmaher) → review+
(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.
no need to change anything, those were just questions I had, thanks for explaining.
Attached patch 916086.diff (for check-in) (obsolete) — Splinter Review
Ok, this has the indentation thing fixed and Olli is ok with the contextmenu code in Eventutils.
Attachment #804720 - Attachment is obsolete: true
Sorry, this is the correct patch for check-in.
Attachment #805528 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/37dd897d1c32
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Depends on: 918981
You need to log in before you can comment on or make changes to this bug.