295582 - Potential post-281988 cleanup

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[[e-mail]]

Because of the autowrapping functions introduced by bug 281988 for the global,
communicator and navigator packages, the references to Components.lookupMethod
and new XPCNativeWrapper from xpfe/ scripts should no longer be necessary, and
can be removed. Components.lookupMethod(foo, "bar").call(foo, args); becomes
foo.bar(args); and new XPCNativeWrapper(foo, "bar").bar. becomes foo.bar.

Comment 1

[J.P.Klippel]

Attachment: patch removing unneeded Components.lookupMethod and new XPCNativeWrapper

Also removed some helper variables where possible and readability of the code
wasn't affected.

Comment 2

[[e-mail]]

Comment on attachment 189590 [details] [diff] [review]
patch removing unneeded Components.lookupMethod and new XPCNativeWrapper

>-  var focusedWindow = new XPCNativeWrapper(document.commandDispatcher.focusedWindow, "top", "document", "location");
I'm not convinced that inlining focusedWindow is a good idea here. But I'm not
strongly opposed to leaving it like this.

>+          var ownerDocument = this.target.ownerDocument;
>+          var characterSet = ownerDocument.characterSet;
On the other hand, I would have inlined ownerDocument here ;-)

>-                onget="return Components.lookupMethod(this.contentDocument, 'title').call(this.contentDocument);"
>+                onget="return this.contentDocument.title();
Although it says components.lookupMethod, it's a getter method, so you don't
actually need the ()s to invoke it. What you do need is c closing quote!

>-              Components.lookupMethod(element, "focus").call(element);
>+              element.focus();
For comparison, this one is a real method. Confused yet?

>           <![CDATA[
>-            // mechanism for reading properties of the underlying XPCOM object
>-            // (ignoring potential getters/setters added by malicious content)
>-            var safeGetProperty = function(obj, propname) {
>-              return Components.lookupMethod(obj, propname).call(obj);
>-            };
> 
I think this blank line should go too.

Comment 3

[jag (Peter Annema)]

Comment on attachment 189590 [details] [diff] [review]
patch removing unneeded Components.lookupMethod and new XPCNativeWrapper

Nice work! Just fix the things Neil pointed out and the few things I'm adding
below, and attach a new patch.

>+++ mozilla/xpfe/browser/resources/content/navigator.js	2005-07-17 12:28:12.000000000 +0200

>@@ -273,20 +273,19 @@ function getContentAreaFrameCount()

>+  if (document.commandDispatcher.focusedWindow.top == window.content) {
>+    gFocusedURL = document.commandDispatcher.focusedWindow.location.href;
>+    gFocusedDocument = document.commandDispatcher.focusedWindow.document;
>   }

As Neil pointed out, I think the following is a bit more readable:

  const focusedWindow = document.commandDispatcher.focusedWindow;
  if (focusedWindow.top == window.content) {
    gFocusedURL = focusedWindow.location.href;
    gFocusedDocument = focusedWindow.document;
  }


>+++ mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml	2005-07-17 12:57:53.000000000 +0200

>@@ -696,40 +691,38 @@

>+            const targetDoc = event.target.ownerDocument();

>+            var uri = ioService.newURI(href, targetDoc.characterSet(), null);

>+            var origURI = ioService.newURI(targetDoc.documentURI(), targetDoc.characterSet(), null);

>                 if (contentPolicy.shouldLoad(nsIContentPolicy.TYPE_IMAGE,
>                                              uri, origURI, event.target,
>+                                             event.target.type(),
>                                              null) == nsIContentPolicy.ACCEPT) {

>@@ -781,18 +774,17 @@

>+                  var characterSet = browser.contentDocument.characterSet();

Just to be on the safe side, in addition to the |.title()| Neil pointed out,
the above cases, where you replaced safeGetProperty, are all property accesses
and should be |.foo|, not |.foo()|.

Looking forward to your new patch.

Comment 4

[J.P.Klippel]

Attachment: revised patch

added all suggestions. Thanks for the time and your review so far!

Comment 5

[[e-mail]]

Comment on attachment 189674 [details] [diff] [review]
revised patch

>-  var focusedWindow = new XPCNativeWrapper(document.commandDispatcher.focusedWindow, "top", "document", "location");
>+  const focusedWindow = document.commandDispatcher.focusedWindow;
I'm not a fan of const for computed values.

>-      var opener = new XPCNativeWrapper(window.content, "opener").opener;
>-      var location = new XPCNativeWrapper(opener, "location").location;
>-      return IOS.newURI(location.href, null, null);
>+      return IOS.newURI(window.content.location.href, null, null);
Looks like you're missing an opener here. r=me with this fixed.

Comment 6

[J.P.Klippel]

Attachment: fixed the "opener"-issue requested in the review

-      return IOS.newURI(location.href, null, null);
+      return IOS.newURI(window.content.opener.location.href, null, null);

Also it's not a big change, requesting superreview for the new patch again.

Comment 7

[[e-mail]]

Comment on attachment 190023 [details] [diff] [review]
fixed the "opener"-issue requested in the review

suite-only cleanup

Comment 8

[[e-mail]]

Fix checked in.

Comment 9

[[e-mail]]

*** Bug 299362 has been marked as a duplicate of this bug. ***