246964 - Special-case 'if (document.all)...'-style object detection in JSClass.resolve

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Brendan Eich [:brendan]]

To handle IE content that uses document.all without testing for it, we could add
a mode to resolve, signaled by JSRESOLVE_DETECTING being set in JSNewREsolveOp's
flags argument, that the unresolved property reference is the condition of an if
or ?: condition (possibly parenthesized).  Thus, we could fail to resolve when
pages are object-detecting document.all, but willingly resolve when pages use
document.all[id].

I do not propose to handle &&, ||, or ! combinations.

Comments?

/be

Comment 1

[Johnny Stenback (:jst)]

Worth a shot, definately. I can hack up the other side of this once you've got
the flag for me to use...

Comment 2

[Brendan Eich [:brendan]]

Attachment: proposed enhancement

Simple and direct, including js shell it.noisy test fun.

/be

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 150885 [details] [diff] [review]
proposed enhancement

jst, feel free to try this and take advantage of it in dom code....

/be

Comment 4

[Brendan Eich [:brendan]]

jst: probably want to insist on JSRESOLVE_QUALIFIED in order to even consider
resolving document.all, but then bail on the resolution (return with null in
*objp) if JSRESOLVE_DETECTING is set.

/be

Comment 5

[Bob Clary [:bc] (inactive)]

This is an interesting approach which might be helpful for cases where an App is
written solely for MSIE without impacting us in cases where the author has coded
for both MSIE and us. It is not clear to me how much this will get you without
supporting more of the MSIE DOM especially MSIE's exposure of elements w/ID as
global vars.

jst, will you do just document.all and not HTMLElement.all ? How much of the
other parts like indexing, tags, item, etc? I can run some scans to see how this
plays out when you have a preliminary patch.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #5)
> It is not clear to me how much this will get you without
> supporting more of the MSIE DOM especially MSIE's exposure of elements w/ID as
> global vars.

doesn't MSIE also have a global |event| variable, contrary to mozilla, which
pages often use?

Comment 7

[Bob Clary [:bc] (inactive)]

(In reply to comment #6)
> 
> doesn't MSIE also have a global |event| variable, contrary to mozilla, which
> pages often use?

yes.

Comment 8

[Johnny Stenback (:jst)]

I think the best approach here is to make incremental changes and test and see
how far it gets us, if we find that doing document.all gets us far, but taking
one more step would get a significant improvement, then lets think about that,
separately for each case we run into.

Comment 9

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 150885 [details] [diff] [review]
proposed enhancement

r=shaver

Comment 10

[Brendan Eich [:brendan]]

Fix is in.  jst, can you file the DOM followup bug?

/be

Comment 11

[Brendan Eich [:brendan]]

Filed bug 248549 on DOM: Level 0 for followup.

/be

Comment 12

[Boris Zbarsky [:bzbarsky]]

I assume that with this patch something like:

  var isIE = document.all;
  if (isIE) {
  }

will continue to test false in Gecko?

Comment 13

[Brendan Eich [:brendan]]

bz: yes, as close reading of bug 248549 should make clear (bug 248549 comment 3
says so directly, and bug 248549 comment 17 says more).

/be

Comment 14

[Brendan Eich [:brendan]]

I should have said that this bug's patch doesn do what bz wants, but the second
attachment in bug 248549 does.

/be

Comment 15

[Johnny Stenback (:jst)]

Landed on the aviary branch too.

Comment 16

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-246964.js

Thanks to Brendan, Boris and Steve. See bug 248549, 253150, 259935.

Steve, with your permission this will be included in the javascript test
library.

Comment 17

[Steve Chapel]

Yes, you have my permission. Thanks for listing me as a contributor.

Comment 18

[Bob Clary [:bc] (inactive)]

js1_5/Regress/regress-246964.js checked in.

Comment 19

[Bob Clary [:bc] (inactive)]

verified fixed.