Closed Bug 388436 Opened 15 years ago Closed 15 years ago

FUEL: Extension tests started failing on OSX

Categories

(Firefox :: General, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: mfinkle, Assigned: mfinkle)

Details

Attachments

(1 file, 2 obsolete files)

Extension tests in browser_Extensions.js started failing to get .prefs or .storage child objects, causing the tinderbox to go orange. The tests were disabled until the problem could be reproduced and addressed.
This patch adds QueryInterface methods to all JS objects in case it helps XPConnect. I also found a bug in Extension.prefs.events that kept a "change" event from firing. I found the bug because I was adding more tests i an effort to determine the orange breakage on tinderbox.

This patch passes all tests on Windows, Linux and Mac
Assignee: nobody → mark.finkle
Status: NEW → ASSIGNED
Attachment #272669 - Flags: review?(gavin.sharp)
Everything is the same as the previous patch, but this patch uses XPCOMUtils.generateQI to make the QueryInterface methods
Attachment #272669 - Attachment is obsolete: true
Attachment #272685 - Flags: review?(gavin.sharp)
Attachment #272669 - Flags: review?(gavin.sharp)
Comment on attachment 272685 [details] [diff] [review]
same as previous patch but using XPCOMUtils.jsm

I don't think the change to the PreferenceBranch constructor is correct - I'd like to better understand why it fixes any problems, at least. The rest looks OK - I can quickly stamp a patch that adds the function names and XPCOMUtils/QI stuff only.
Attachment #272685 - Flags: review?(gavin.sharp) → review-
Comment on attachment 272685 [details] [diff] [review]
same as previous patch but using XPCOMUtils.jsm

We readjust the branch so everything is relative to the whatever the _root is. Therefore we need to pass in "" to monitor changes on the branch.
Attachment #272685 - Flags: review- → review?(gavin.sharp)
Comment on attachment 272685 [details] [diff] [review]
same as previous patch but using XPCOMUtils.jsm

>Index: src/fuelApplication.js

>+  _event : function bt_event(aEvent) {
>+    if (aEvent.type == "load") {
>+      if (!aEvent.originalTarget instanceof Ci.nsIDOMHTMLDocument)
>+        return;

I missed pointing out this problem in the original review, but "!" binds tighter than "instanceof", so this is wrong, you need to parenthesize. This also points out that this code specifically could use some unit tests :)

I would have preferred having three separate patches for the three changes being made here (Extension pref events and tests, add QI/function names, and this load event filtering code), for what it's worth. Reviewing the XPCOMUtils/function name patch is much easier than the other two changes, and we could have gotten that landed quickly :)
Attachment #272685 - Flags: review?(gavin.sharp) → review-
Attachment #272685 - Attachment is obsolete: true
Attachment #272794 - Flags: review?(gavin.sharp)
(In reply to comment #5)
> (From update of attachment 272685 [details] [diff] [review])
> >Index: src/fuelApplication.js
> 
> >+  _event : function bt_event(aEvent) {
> >+    if (aEvent.type == "load") {
> >+      if (!aEvent.originalTarget instanceof Ci.nsIDOMHTMLDocument)
> >+        return;
> 
> I missed pointing out this problem in the original review, but "!" binds
> tighter than "instanceof", so this is wrong, you need to parenthesize. This
> also points out that this code specifically could use some unit tests :)
> 
Done. And I changed the logic to check for frameElement based on your IRC comments:
+      if (aEvent.originalTarget.defaultView instanceof Ci.nsIDOMWindowInternal &&
+          aEvent.originalTarget.defaultView.frameElement)
+        return;

I also added a unit test to check this code. When loading a page with frames, the load event should only be fired once.

> I would have preferred having three separate patches for the three changes
> being made here (Extension pref events and tests, add QI/function names, and
> this load event filtering code), for what it's worth. Reviewing the
> XPCOMUtils/function name patch is much easier than the other two changes, and
> we could have gotten that landed quickly :)
> 

I agree and I'm sorry. This patch sorta snowballed as I started working on it. I do better next time.
Comment on attachment 272794 [details] [diff] [review]
updated based on review comments

Sorry for the delay, I was seeing mysterious failures for the extension pref observer tests, but things seem to be working OK now.
Attachment #272794 - Flags: review?(gavin.sharp) → review+
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.