167871 - Page Info should lookup the real getters of objects it uses

Status: RESOLVED WORKSFORME

(SeaMonkey :: Page Info, defect, P1)

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch v1

Comment 2

[Daniel Brooks [:db48x]]

how exactly is this better? Seems more complicated if you ask me. Also, noticed
a couple places where called lookupMethod rather than your getNativeProperty
function, though perhaps you did that on purpose.

Also, why change i++ to ++i? it look weird that way.

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch v2

This patch also gets the native for window.frames and frame.document

> how exactly is this better? Seems more complicated if you ask me.

It definitely is more complicated.  But this patch lets you the real page info.
 See page info on this page: http://www.returnzero.com/mozilla-test/167871.html


> Also, noticed a couple places where called lookupMethod rather than your 
> getNativeProperty function, though perhaps you did that on purpose.

That is done on purpose.  If you notice I am getting native functions as
opposed to just properties, and passing arguments to them.. slightly different.
 I think it is good to handle those on a case by case basis here.

> Also, why change i++ to ++i? it look weird that way.

Well, you already have a bunch of places with ++foo in the file.  I'm being
consistent.  But on top of that, ++i is more efficient than i++.

Comment 4

[Christopher Aillon (sabbatical, not receiving bugmail)]

Also see bug 77315.

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch v3

Duh.  window.frames === window so I can't get accurately the frames by getting
the native wrapped 'frames' property.  Instead, get the frames and iframe Elts
by Attr.

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

s/Attr/TagName/ in my last comment.

Comment 7

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch v4

Address issues from jst via IRC

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: The real v4 :-)

Next time remember to save before diffing :-)

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 99025 [details] [diff] [review]
The real v4 :-)

sr=jst

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 99025 [details] [diff] [review]
The real v4 :-)

>       case "area":
>-        linkView.addRow([++linkIndex, elem.alt, elem.href, linkArea, elem.target]);
>+        linkView.addRow([++linkIndex, elem.alt, elemHref, linkArea, elemTarget]);

elem.alt needs to be modified

>         break;
>       case "input":
>-        linkView.addRow([++linkIndex, elem.value || linkSubmit, elem.form.getAttribute("action"), linkSubmission, elem.form.getAttribute("target")]); // use getAttribute() due to bug 122128
>+        var elemValue = getNativeProperty(elem, 'value');
>+        var elemForm = getNativeProperty(elem, 'form');
>+        var elemFormGetAttribute = Components.lookupMethod(elem, 'getAttribute');

Should be Components.lookupMethod(elemForm, 'getAttribute');

>+        linkView.addRow([++linkIndex, elemValue || linkSubmit, elemFormGetAttribute.call(elem, "action"), linkSubmission, elemFormGetAttribute.call(elem, "target")]); // use getAttribute() due to bug 122128

.call(elemForm, ...)


>@@ -627,28 +658,39 @@
>   var length = imageList.length;
>   imageIndex = 0;
> 
>-  for (var i = 0; i < length; i++)
>+  for (var i = 0; i < length; ++i)
>   {
>     var elem = imageList[i];
>-    switch (elem.nodeName.toLowerCase())
>+    var elemAlt;
>+    var elemSrc;
>+    switch (getNativeProperty(elem, 'nodeName').toLowerCase())

why? you're not saving any code anyway, so why not just define them when you
need them? (or get the values up here and save a few lines further down)

>         break;
>       case "object":
>-        imageView.addRow([++imageIndex, elem.data, mediaObject]);
>+        var elemData = ;
>+        imageView.addRow([++imageIndex, elemData, mediaObject]);

nit: you could just do 
imageView.addRow([++imageIndex, getNativeProperty(elem, 'data'), mediaObject]);

same a couple of lines down where you only use one propery.

>-  document.getElementById("imagelongdesctext").value = ("longDesc" in item && item.longDesc) || notSet;
>+
>+  var longdesc = getNativeProperty(item, 'longDesc');
>+  if (longdesc)
>+    document.getElementById("imagelongdesctext").value = longdesc;
>+  else
>+    document.getElementById("imagelongdesctext").value = notSet;

nit: document.getElementById("imagelongdesctext").value = longdesc || notSet
seems smoother

>-  var length = linkNode.childNodes.length;
>-  for (var i = 0; i < length; i++)
>+
>+  var kids = getNativeProperty(linkNode, 'childNodes');
>+  var length = kids.length;

don't you have to call
getNativeProperty(kids, 'length');
?

Same in getAltText

>   // resolve everything from bottom up, starting with document location
>   var ioService = Components.classes["@mozilla.org/network/io-service;1"].getService(Components.interfaces.nsIIOService);
>-  var URL = ioService.newURI(doc.location.href, null, null);
>+  var URL = ioService.newURI(getNativeProperty(doc, 'location').href, null, null);

getNativeProperty(getNativeProperty(doc, 'location'), 'href'); ?


>+  if (frameElts)
>+    frames.concat(frameElts);
>+
>+  if (iframeElts)
>+    frames.concat(iframeElts);
>+
>+  return frames;

Does concat modify the object, or does it just return a new array?

if the former there is quite a few places where we could optimise the code in
pageInfo.js (in a separate bug of course), if the latter, then your code
doesn't work :)

Comment 11

[Daniel Brooks [:db48x]]

this is a bad thing, and I should probably make it more of a priority. I'll fix
the patch tonight (it's out of date).

Comment 12

[Daniel Brooks [:db48x]]

Attachment: getting there

timeless is right of course, this would be so much easier if we could just drop
our chrome privs for awhile while we did something that could be bad.

Comment 13

[Daniel Brooks [:db48x]]

of course, it's just my opionion and stuff, but if people weren't able to  
replace functions like toString, getters and so on for objects that are part  
of the dom, I certainly wouldn't mind. What do you say jst? 

Comment 14

[Daniel Brooks [:db48x]]

oh, and I also need to know what functions |"alt" in item| (and similar) are 
calling behind my back. Where in the source are the operators handled? 

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

Yeah, I need to get on this again.  But not this way if possible.  I'm working
on an actual API for this that avoids such ugly code, that I need to rehash.

Comment 16

[Neil Marshall]

I've come up with a test case for 2 other places where this pops up.  Right
Click and Right Click->Properties.

http://www.eightlines.com/neil/hack.html

I don't know all of the cases that are affected, but I went through all of the
methods I know of and overwrote them with alert functions.  It may be missing
stuff, I'm not sure myself.

Comment 17

[Hixie (not reading bugmail)]

Seems likely that caillon won't get to work on this more. Any chance you could
look at it, Daniel?

Comment 18

[Hideo Oshima]

I can't reproduce any longer.
2005111707-trunk/Linux.
Marking worksforme.