Closed Bug 220215 Opened 17 years ago Closed 16 years ago

use the xpcnativewrapper for the current callers of components.lookupmethod

Categories

(SeaMonkey :: General, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: caillon, Assigned: neil)

Details

Attachments

(3 files, 4 obsolete files)

Comment on attachment 132135 [details] [diff] [review]
Editor portion of patch

glazou says moa if I test it 10 times but I can only think of 6 tests so far
:-/
Attachment #132135 - Flags: superreview?(bzbarsky)
Attachment #132135 - Flags: review?(caillon)
Comment on attachment 132135 [details] [diff] [review]
Editor portion of patch

sr=bzbarsky
Attachment #132135 - Flags: superreview?(bzbarsky) → superreview+
Attachment #132135 - Flags: review?(caillon) → review+
Attached patch Xpfe portion of patch (obsolete) — Splinter Review
Attachment #132190 - Flags: superreview?(bzbarsky)
Attachment #132190 - Flags: review?(caillon)
There's an inspector use too, although I didn't realize the style property on an
element is replaceable (or is it?)
Comment on attachment 132190 [details] [diff] [review]
Xpfe portion of patch

Hmmm how come just importing the JS in contentAreaContextOverlay makes it
available in all those JS files (eg navigator.js)?  Have you tested all the
callsites?
Attachment #132345 - Flags: superreview?(bzbarsky)
Attachment #132345 - Flags: review?(caillon)
Attachment #132345 - Flags: review?(caillon) → review+
Comment on attachment 132190 [details] [diff] [review]
Xpfe portion of patch

Does JS Console etc use any of these files, especially contentAreaUtils and
will it care about these functions?  I think editor also uses that file...
Attachment #132190 - Flags: review?(caillon)
Comment on attachment 132190 [details] [diff] [review]
Xpfe portion of patch

mailNavigatorOverlay.xul and navigator.js are only used by navigator, which
already uses contentAreaContextOverlay.xul

contentAreaUtils.js and nsContextMenu.js are only used by
contentAreaContextOverlay.xul which (surprise) is where I am also including
XPCNativeWrapper.js

Sorry for not being more obvious.
Attachment #132190 - Flags: review?(caillon)
Comment on attachment 132190 [details] [diff] [review]
Xpfe portion of patch

sr=bzbarsky
Attachment #132190 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 132190 [details] [diff] [review]
Xpfe portion of patch

>Index: communicator/resources/content/nsContextMenu.js
>+          var spec = new XPCNativeWrapper(window.content.opener, 
"location").location;

accessing .opener is already not safe, no?  The wrapper needs to be on
window.content (this was buggy in the code you changed already).

>+          var characterSet = new XPCNativeWrapper(this.target.ownerDocument, "characterSet").characterSet;

Accessing ownerDocument is already not safe; the wrapper needs to be on
this.target.  Although you may also need to rewrap the ownerDocument object
returned; please test.

>Index: browser/resources/content/navigator.js
>+      var spec = new XPCNativeWrapper(window.content.opener, "location").location;

Accessing opener is unsafe.

> Index: browser/resources/content/mailNavigatorOverlay.xul

>+                                 new XPCNativeWrapper(window._content.document, 'title').title);"/>

Accessing .document is not safe; you want the wrapper on window._content....
Attachment #132190 - Flags: superreview+ → superreview-
Comment on attachment 132345 [details] [diff] [review]
Inspector portion of patch

sr=bzbarsky on this one; the other sr was a case of mistaken patch identity.
Attachment #132345 - Flags: superreview?(bzbarsky) → superreview+
Attached patch Improved xpfe portion of patch (obsolete) — Splinter Review
I've hacked Send Frame/Link/Page around a bit but they still work.
Attachment #132190 - Attachment is obsolete: true
Attachment #132355 - Flags: superreview?(bzbarsky)
Attachment #132355 - Flags: review?(caillon)
Comment on attachment 132355 [details] [diff] [review]
Improved xpfe portion of patch

Hm, I'm wondering why I put some of these wrapers on fooWin.location.href... 
That should be completely safe.

See:
http://lxr.mozilla.org/mozilla/source/dom/src/base/nsDOMClassInfo.cpp#3144
http://lxr.mozilla.org/mozilla/source/dom/src/base/nsDOMClassInfo.cpp#3182
http://lxr.mozilla.org/mozilla/source/dom/src/base/nsDOMClassInfo.cpp#3209
http://lxr.mozilla.org/mozilla/source/dom/src/base/nsDOMClassInfo.cpp#4145
etc.

Also, this is kind of obtuse enough, could you please not nest |new
XPCNativeWrapper|s?  Just use a temporary local.  You aren't gaining that much
by not doing that, and you're losing readability.
Attachment #132190 - Flags: review?(caillon)
Attachment #132355 - Attachment is obsolete: true
Attachment #132543 - Flags: superreview?(bzbarsky)
Attachment #132543 - Flags: review?(caillon)
Attachment #132355 - Flags: superreview?(bzbarsky)
Attachment #132355 - Flags: review?(caillon)
Comment on attachment 132543 [details] [diff] [review]
Removed unnecessary wrappers on window.document/.location

>Index: browser/resources/content/navigator.js

>     gFocusedDocument = focusedWindow.document;

Is this safe? sr=me if caillon is happy with it.
Attachment #132543 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 132543 [details] [diff] [review]
Removed unnecessary wrappers on window.document/.location

>--- browser/resources/content/mailNavigatorOverlay.xul	30 May 2003 05:08:36 -0000	1.4
>+++ browser/resources/content/mailNavigatorOverlay.xul	2 Oct 2003 20:28:46 -0000
>@@ -120,12 +120,10 @@
>     {
>       if (!aDocument)
>         aDocument = window._content.document;

This looks like it needs wrapping.

>-        
>-      var pageUrl = aDocument.URL;
>-      var pageTitle = Components.lookupMethod(aDocument, 'title').call(aDocument);
> 
>       try {
>-        openComposeWindow(pageUrl, pageTitle, 1);
>+        aDocument = new XPCNativeWrapper(aDocument, "URL", "title");
>+        openComposeWindow(aDocument.URL, aDocument.title, 1);
>       } catch(ex) { dump("Cannot Send Page: " + ex + "\n"); }


>--- browser/resources/content/navigator.js	23 Sep 2003 06:49:05 -0000	1.517
>+++ browser/resources/content/navigator.js	2 Oct 2003 20:28:48 -0000
>@@ -271,7 +271,7 @@ function contentAreaFrameFocus()
> {
>   var focusedWindow = document.commandDispatcher.focusedWindow;
>   if (isContentFrame(focusedWindow)) {
>-    gFocusedURL = Components.lookupMethod(focusedWindow, 'location').call(focusedWindow).href;
>+    gFocusedURL = focusedWindow.location.href;
>     gFocusedDocument = focusedWindow.document;

This, too.
Attachment #132543 - Flags: review?(caillon) → review+
Attachment #132543 - Attachment is obsolete: true
Comment on attachment 135169 [details] [diff] [review]
Actually it's document.location that's safe, window is totally unsafe

Re-requesting reviews as the patch is significantly different.
Attachment #135169 - Flags: superreview?(bz-vacation)
Attachment #135169 - Flags: review?(caillon)
I'll probably take me about a week to get to this.
Comment on attachment 135169 [details] [diff] [review]
Actually it's document.location that's safe, window is totally unsafe

sr=bzbarsky
Attachment #135169 - Flags: superreview?(bz-vacation) → superreview+
Comment on attachment 135169 [details] [diff] [review]
Actually it's document.location that's safe, window is totally unsafe

Both suck.  r=me if you put back the document change.  I will try hacking
classinfo and caps stuff to see if I can get a better all around solution....
Attachment #135169 - Flags: review?(caillon) → review+
Attachment #135169 - Attachment is obsolete: true
Attachment #136014 - Flags: review?(caillon)
Comment on attachment 136014 [details] [diff] [review]
wrapping .location again

> 
>-    function sendLink(pageUrl, pageTitle)
>+    function sendLink(aDocument)
>     {
>+      if (!aDocument)
>+        aDocument = window._content.document;

Nit: Use |window.content| instead?  It's slightly faster.

r=me.  I hope to come up with a better generic solution for this whole issue
sometime soon.	It really annoys me that we need to use the XPCWrapper; though
it is better than not, I will agree.
Attachment #136014 - Flags: review?(caillon) → review+
Fix checked in.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Sometime between 2004-05-20 09:00:00 and 2004-05-21 08:00:00 "save as" stopped
working fromm the View Source windows. Could this checkin have caused that?
See bug 248964
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.