Remove some more enablePrivilege calls

RESOLVED FIXED in mozilla27

Status

Testing
Mochitest
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Martijn Wargers (zombie), Assigned: Martijn Wargers (zombie))

Tracking

unspecified
mozilla27
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
Again some removal of enablePrivilege calls, in content/dom/layout.
(Assignee)

Updated

5 years ago
Summary: Remove some more enablePrivilige call → Remove some more enablePrivilige calls
(Assignee)

Updated

5 years ago
Summary: Remove some more enablePrivilige calls → Remove some more enablePrivilege calls
(Assignee)

Updated

5 years ago
Assignee: nobody → martijn.martijn
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
Created attachment 804720 [details] [diff] [review]
916086.diff

Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=e671b5161e44
Attachment #804419 - Attachment is obsolete: true
(Assignee)

Comment 4

5 years ago
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+
(Assignee)

Comment 6

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
Created attachment 805528 [details] [diff] [review]
916086.diff (for check-in)

Ok, this has the indentation thing fixed and Olli is ok with the contextmenu code in Eventutils.
Attachment #804720 - Attachment is obsolete: true
(Assignee)

Comment 10

5 years ago
Created attachment 805536 [details] [diff] [review]
916086.diff (for check-in)

Sorry, this is the correct patch for check-in.
Attachment #805528 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/37dd897d1c32
Status: NEW → RESOLVED
Last Resolved: 5 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.