Page Info should lookup the real getters of objects it uses

RESOLVED WORKSFORME

Status

P1
normal
RESOLVED WORKSFORME
17 years ago
13 years ago

People

(Reporter: caillon, Assigned: db48x)

Tracking

Trunk
mozilla1.4final

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Comment 2

17 years ago
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.
Created attachment 98709 [details] [diff] [review]
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++.
Attachment #98699 - Attachment is obsolete: true
Created attachment 98876 [details] [diff] [review]
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.
Attachment #98709 - Attachment is obsolete: true
s/Attr/TagName/ in my last comment.
Created attachment 99024 [details] [diff] [review]
Patch v4

Address issues from jst via IRC
Attachment #98876 - Attachment is obsolete: true
Created attachment 99025 [details] [diff] [review]
The real v4 :-)

Next time remember to save before diffing :-)
Attachment #99024 - Attachment is obsolete: true
Attachment #99025 - Flags: superreview+
Comment on attachment 99025 [details] [diff] [review]
The real v4 :-)

sr=jst
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 :)
(Assignee)

Comment 11

16 years ago
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).
(Assignee)

Updated

16 years ago
Attachment #99025 - Flags: needs-work+
(Assignee)

Comment 12

16 years ago
Created attachment 104263 [details] [diff] [review]
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.
(Assignee)

Updated

16 years ago
Attachment #104263 - Flags: needs-work+
(Assignee)

Comment 13

16 years ago
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? 
(Assignee)

Comment 14

16 years ago
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? 
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

16 years ago
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.
Status: NEW → ASSIGNED
OS: Linux → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.4beta
Target Milestone: mozilla1.4beta → mozilla1.4final
Seems likely that caillon won't get to work on this more. Any chance you could
look at it, Daniel?
Assignee: caillon → db48x
Status: ASSIGNED → NEW
Product: Browser → Seamonkey

Comment 18

13 years ago
I can't reproduce any longer.
2005111707-trunk/Linux.
Marking worksforme.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.