See bug 533592.

Comment on attachment 423298 [details] [diff] [review] I think this should fix the one consumer that needs raw object access in Inspector r=sdwilsh

Comment on attachment 423298 [details] [diff] [review] I think this should fix the one consumer that needs raw object access in Inspector This doesn't look right to me. There doesn't seem to be a way of knowing whether the random object that you're viewing is an XPCNativeWrapper or not, nor whether any of the random subproperties are XPCNativeWrappers or not (which would for instance hold true for inspecting the "content" property of a browser's chrome window).

> There doesn't seem to be a way of knowing whether the random object that you're > viewing is an XPCNativeWrapper or not Is there a way to get into this viewer other than selecting "js object view" on a DOM node? > nor whether any of the random subproperties are XPCNativeWrappers or not Why does that matter? Subproperties are handled by opening up the relevant tree branches, not setting another root object on the viewer, right? I can use XPCNativeWrapper.unwrap() here instead, but that would make the code m-c-only. Is that preferable?

(In reply to comment #4) > > There doesn't seem to be a way of knowing whether the random object that you're > > viewing is an XPCNativeWrapper or not > Is there a way to get into this viewer other than selecting "js object view" on > a DOM node? You're supposed to be able to, but I think that feature seems to have a bug. Then again, chrome DOM nodes don't have XPCNativeWrappers either. > > nor whether any of the random subproperties are XPCNativeWrappers or not > Why does that matter? Subproperties are handled by opening up the relevant > tree branches, not setting another root object on the viewer, right? But we still don't want "automatic" XPCNativeWrappers on subproperties. I guess we'll have to XPCNativeWrapper.unwrap() each one, just in case. > I can use XPCNativeWrapper.unwrap() here instead, but that would make the code > m-c-only. Is that preferable? XPCNativeWrapper.unwrap is on 1.9.2 too, and wanted for 1.9.1 as well, no? (I don't know how far backward compatible current DOMi development should be.)

(In reply to comment #5) > XPCNativeWrapper.unwrap is on 1.9.2 too, and wanted for 1.9.1 as well, no? > (I don't know how far backward compatible current DOMi development should be.) It missed Firefox 3.6, but should be in Gecko 1.9.2.2 (whatever that ends up being). I hope it ends up on 1.9.1, but I'm not sure if that's going to happen.

(In reply to comment #5) > XPCNativeWrapper.unwrap is on 1.9.2 too, and wanted for 1.9.1 as well, no? > (I don't know how far backward compatible current DOMi development should be.) There was a bug fix recently (this year, I think) that set DOM Inspector's minimum support to 1.9.0. I think that still holds, mostly by consequence, but I don't know if Alexander Surkov's recent changes on the accessibility viewers might have changed that. I do know that we are still +fx3.0 on AMO, and for bug 384811, I did add an explicit check to make sure it would work on 1.9.0.

Heh, turns out 384811 is an attachment id I was looking at. That should be bug 212754 (the very bug where that attachment resides).

> It missed Firefox 3.6, but should be in Gecko 1.9.2.2 Right. The key being that: 1) Making this change _and_pushing_it_out_ blocks mrbkap doing work that needs to happen on trunk ASAP. 2) If we do it using unwrap() right now it'll immediately break Inspector for all existing non-trunk users for several weeks. > Then again, chrome DOM nodes don't have XPCNativeWrappers either. Good point. I should test whether there's a wrappedJSObject in the given node. Would you be ok with that? > But we still don't want "automatic" XPCNativeWrappers on subproperties. Ah, for the chrome case when we start off with something that is just a raw object, not a SJOW? Blake, is there a sane way to work around that without sprinkling unwraps on all the getters (so force things to get SJOWs instead of XPCNW even in cases when I'm going from some XUL iframe to its contentDocument or something)? Would expliciting sticking a SJOW around my initial object do the trick? Or should I just bite the bullet and put in unwraps everywhere?

(In reply to comment #9) > > It missed Firefox 3.6, but should be in Gecko 1.9.2.2 > 2) If we do it using unwrap() right now it'll immediately break Inspector for > all existing non-trunk users for several weeks. Perhaps using run-time unwrap detection, plus attempting to register both with and without xpcnativewrappers=no, would be the most compatible (with 1.9.0, which appears to be what we want to be compatible with at the moment) solution?

How do you register both with and without xpcnativewrappers=no, exactly?

Comment on attachment 423298 [details] [diff] [review] I think this should fix the one consumer that needs raw object access in Inspector >-% content inspector %content/inspector/ xpcnativewrappers=no >+% content inspector %content/inspector/ I believe that by including both lines in the above order Geckos supporting xpcnativewrappers=no will disable them for DOM Inspector and ignore the duplicate registration required by Geckos not supporting xpcnativewrappers=no.

My fault, it appears to be the other way around: you specify it first without the flag, and then again with the flag, for use with supported Geckos.

bz, could you do something like if (!XPCNativeWrapper.unwrap) XPCNativeWrapper.unwrap = function(w) return Object.prototype.toString.call(w) === "XPCNativeWrapper" ? w.wrappedJSObject : w; somewhere useful?

In builds without unwrap we can simply assume xpcnativewrappers=no works.

Yeah, I was going to go with comment 15. I just need to figure out what to do with property gets per comment 9 end of comment.

Comment on attachment 423298 [details] [diff] [review] I think this should fix the one consumer that needs raw object access in Inspector > set subject(aObject) > { >+ aObject = aObject.wrappedJSObject; Maybe this should be try { aObject = XPCNativeWrapper(aObject).wrappedJSObject; } catch (e) { // not a wrappable object }

I'm OK with making our minimum support higher (but really, no higher than Firefox 3.0/Gecko 1.9.1). Also, I think we'd want to use the appversion flag to specify xpcnativewrappers properly: https://developer.mozilla.org/en/Chrome_Registration#appversion

I'd be fine with that, assuming it's the toolkit version and not the actual application version.

(In reply to comment #18) > I'm OK with making our minimum support higher (but really, no higher than > Firefox 3.0/Gecko 1.9.1). Firefox 3.0 is Gecko 1.9.0, which is where we are now.

I meant Firefox 3.5. Sorry for the confusion.

(In reply to comment #19) > I'd be fine with that, assuming it's the toolkit version and not the actual > application version. Unfortunately I think it's the application version.

In that case, it seems to be a non-option here, since DOMI is cross-application.

AFAIK, you can do this: application=toolkit@mozilla.org appversion<X Maybe I'm wrong though (hope not)!

I think you're thinking of install.rdf, not chrome.manifest, sorry.

Comment on attachment 424079 [details] [diff] [review] Updated to neil's comments r=sdwilsh

I don't understand why, but things aren't working at all right without xpcnativewrappers=no (i.e. simply removing that line from the manifest, leaving the new entry from the patch); here are some examples: 1. Inspect the browser chrome document, change from DOM Node to JS Object view, expand defaultView, notice that content is an [object XPCNativeWrapper [object Window]], when it should be an [object Window], notice that you cannot inspect its properties at all. 2. Inspect the browser chrome window, change from DOM Node to JS Object view, expand defaultView, expand gBrowser, notice that contentDocument is an [object XPCNativeWrapper [object HTMLDocument]] (current DOM Inspector does this too), expand contentDocument, note that defaultView is an [object Window]! 3. Directly inspect the browser's content document, change from DOM Node to JS Object view, notice that the subject is an [object XPCNativeWrapper [object HTMLDocument]], when it should be an [object HTMLDocument]. Note: the number of properties enumerated on an [object XPCNativeWrapper [object HTMLDocument]] seems to be random.

neil, what build are you testing in? I did do my testing in the mode with the xpcnativewrappers=no line removed, on m-c from today. In particular do you have the fix for bug 541742 in your build?

Comment on attachment 424079 [details] [diff] [review] Updated to neil's comments Ah yes, that explains things, thanks! Was there a reason why unwrap throws on primitives? (It already accepts more objects than XPCNativeWrapper() does, for instance.) I did manage to somehow get DOM Inspector into a state where many of the properties failed to return reasonable values, but I couldn't reproduce it. Odd.

(In reply to comment #30) > Was there a reason why unwrap throws on primitives? (It already accepts more > objects than XPCNativeWrapper() does, for instance.) It doesn't really make too much sense to ask to unwrap a primitive value IMO. I could change it if people think that makes more sense.

(In reply to comment #31) > It doesn't really make too much sense to ask to unwrap a primitive value IMO. I > could change it if people think that makes more sense. It may not make sense, but it makes code that uses the API easier to write, which I think is a win.

Comment on attachment 424079 [details] [diff] [review] Updated to neil's comments >+ unwrapObject: function(aObject) >+ { >+ /* unwrap() throws for primitive values, so don't call it for those */ >+ if (typeof(aObject) == "object" && aObject && >+ "unwrap" in XPCNativeWrapper) { I'd prefer === to ==. Also, looks like some tabs crept in here (the "unwrap" line and the aObject = line). >+ aObject = XPCNativeWrapper.unwrap(aObject); sr=mrbkap

Pushed http://hg.mozilla.org/dom-inspector/rev/6ce8a2901d71 with those changes.

This seems to have caused a regression: bug 567094

Ok, why? This was supposed to have no effect anywhere other than maybe m-c; see comment 13....

Hrm...maybe Dave would know.

Comment on attachment 424079 [details] [diff] [review] Updated to neil's comments (From attachment 424079 [details] [diff] [review]) >+ unwrapObject: function(aObject) >+ { >+ /* unwrap() throws for primitive values, so don't call it for those */ >+ if (typeof(aObject) == "object" && aObject && >+ "unwrap" in XPCNativeWrapper) { >+ aObject = XPCNativeWrapper.unwrap(aObject); >+ } >+ return aObject; >+ }, There are tabs in here, by the way.

> There are tabs in here, by the way. Yes, comment 33 said that too. And comment 34 said that the tabs were changed to spaces before checking in, no?

OK, looks like comment 12 was just bunk, as was comment 13. Based on nsChromeRegistry code inspection, you can't "remove" a chrome reg flag, only add them. "Use xpcnativewrappers" is such a flag. So no matter what order those lines come in the result is to "use xpcnativewrapper". I'll follow up in bug 567094.

For posterity/linearity of discussion: 08:48 < crussell> bz: bug 533599, comment 39 is rhetorical, no? 08:48 < crussell> bz: I have no problems with replying, but I'm hesitant to generate more bugmail. 08:50 < crussell> bz: FWIW, I was indeed looking at the patch, and not the changeset. 08:51 <@bz> crussell: mmm... not entirely rhetorical, but I did double-check that I didn't land the tabs, fwiw