253405 - Warning for document.all doesn't list source file or line number

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Steve Chapel]

Steps to reproduce:

1. Go to http://www.rohan.co.uk/
2. Click on "Store locator" on the top menu bar
3. Click on an area of the map


Actual results:

The JavaScript console says:

Non-standard document.all property was used. Use W3C standard
document.getElementById() instead.


Expected Results:

The JavaScript console says:

Non-standard document.all property was used. Use W3C standard
document.getElementById() instead.
Source File: http://www.rohan.co.uk/Static/en-GB/script/storelocation.js
Line: 56

This will help web designers find the source of the problem.

Comment 1

[timeless]

Attachment: get the source file and line number

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 155287 [details] [diff] [review]
get the source file and line number

Brendan would be a much more reasonable sr for this than I (especially given
that note on reviewers.html).

Comment 3

[Brendan Eich [:brendan]]

How about cc'ing DOM peers, if not asking jst to r= the code he just wrote?

/be

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 155287 [details] [diff] [review]
get the source file and line number

>-static void
>-PrintDocumentAllWarningOnConsole()
>+static nsresult
>+PrintDocumentAllWarningOnConsole(JSContext *cx)
> {
>+  nsresult rv;
>   nsCOMPtr<nsIStringBundleService>
>-    stringService(do_GetService(NS_STRINGBUNDLE_CONTRACTID));
>-  if (!stringService) {
>-    return;
>-  }

jst wrote this to return void, and the caller ignores the new nsresult rv
you've added.  Don't waste cycles fussing about failure codes if they're
ignored (as they should be).  Please revert all this, and so on below.

>+  JSStackFrame *fp, *iterator = nsnull;
>+  fp = JS_FrameIterator(cx, &iterator);

Nit, and below: most of the DOM code uses ::JS_* to call the global (C) JS API.
 Do likewise here for "when in Rome" style coherence?

>+      if (filename)
>+        CopyUTF8toUTF16(nsDependentCString(filename), sourcefile);

Is the filename argument passed into the JS API from DOM code
(dom/src/base/nsJSEnvironment.cpp at least) always UTF-8?  It's const char *,
so it could be something else.	Worth a look to be sure (my memory fails me,
and things may have changed since I last looked).

>+      jsbytecode* pc = JS_GetFramePC(cx, fp);
>+      if (pc) {
>+        lineno = JS_PCToLineNumber(cx, script, pc);
>+      }
>+    }
>   }
>+  rv = errorObject->Init(msg.get(),
>+                         sourcefile.get(), /* file name */
>+                         EmptyString().get(), /* source line */
>+                         lineno, /* line number */
>+                         0, /* column number */
>+                         nsIScriptError::warningFlag,
>+                         "DOM:HTML");

Why all the ragged C comments?	How about at least column-aligning // comments,
if we must have comments here (some of these are vacuous).

Thanks for undertaking to fix this, it will help developers (although it fires
only once per doc).

/be

Comment 5

[Brendan Eich [:brendan]]

Safe fix, part of the document.all work that's already in aviary branch, and
that per drivers should be sync'ed with the 1.7 branch so developers get the
same Gecko back end (as far as they can tell) with aviary 1.0 products and all
future 1.7.x products.

/be

Comment 6

[timeless]

Attachment: dom style'd

Comment 7

[Bob Clary [:bc] (inactive)]

Can we change this to report each occurrence where undetected document.all is
used rather than just the first time? Reporting only the first occurrence is
much less useful to a developer looking to remove uses of document.all than
reporting all such uses.

Comment 8

[timeless]

bc: i'm not going to in this patch (and given the concise nature of this bug,
i'd like to close it as soon as this patch is committed, people are welcome to
file nother bug and fight over it there).

i think that a web developer could (should?) simply disable document.all
[user_pref("browser.dom.document.all.disabled", true);] and enable strict warnings.

the reason i'm working on this patch is that i need to be able to get feedback
from my analysts about a site, and they only need to tell me that it's being
used at all for a given page.

Comment 9

[Mike Kaply [:mkaply]]

Please don't ask for approval until r and sr is done - otherwise this will get
lost in the shuffle.

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 155876 [details] [diff] [review]
dom style'd

>+  nsCOMPtr<nsIScriptError> errorObject =
>+    do_CreateInstance(NS_SCRIPTERROR_CONTRACTID);
>+  if (!errorObject) {
>+    return;
>+  }

Can we call this scriptError instead of errorObject?  Yes, I see that
practically everything else in the source tree calls it errorObject.  I'd like
to see the proponents of that naming scheme try to sneak a jsObjectObject or a
cssStructStruct in somewhere.

> 
>-  if (consoleService) {
>-    consoleService->LogStringMessage(msg.get());
>+  JSStackFrame *fp, *iterator = nsnull;
>+  fp = ::JS_FrameIterator(cx, &iterator);
>+  PRUint32 lineno = 0;
>+  const char* filename = nsnull;
>+  nsAutoString sourcefile;
>+  if (fp) {
>+    JSScript* script = ::JS_GetFrameScript(cx, fp);
>+    if (script) {
>+      const char* filename = ::JS_GetScriptFilename(cx, script);
>+      if (filename)
>+        CopyUTF8toUTF16(nsDependentCString(filename), sourcefile);

Everything else uses K&R bracing for one-line blocks except above.  Please fix.

>+      jsbytecode* pc = ::JS_GetFramePC(cx, fp);
>+      if (pc) {
>+        lineno = ::JS_PCToLineNumber(cx, script, pc);
>+      }
>+    }
>   }
>+  nsresult rv = errorObject->Init(msg.get(),
>+                                  sourcefile.get(),
>+                                  EmptyString().get(),
>+                                  lineno,
>+                                  0,

Unsure how hard it would be to get a real column number here.  I can't seem to
find a way at a cursory glance thru spidermonkey.  Also, I think here it is
appropriate to add a C++ style comment that 0 is a column number at the least
since it is non-obvious without pulling up the nsIScriptError header.


>+                                  nsIScriptError::warningFlag,


Should this really be a warning?  I think perhaps a notice may be more
appropriate.  See getSelection() for precedence.


>+                                  "DOM:HTML");
>+  if (NS_FAILED(rv)){
>+    return;
>+  }
>+
>+  consoleService->LogMessage(errorObject);

I'd be somewhat inclined to do:

  if (NS_SUCCEEDED(rv)) {
    consoleService->LogMessage(errorObject);
  }

and save on the extra return, but it doesn't matter really.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 155876 [details] [diff] [review]
dom style'd

So can you tweak the console service to omit the ", Column: %d" or whatever it
is if the column number passed in is zero?

I don't see why this should hold up 1.8a3 for this.

/be

Comment 12

[timeless]

Attachment: scriptError, K&R, commented 0, succeeded

should it really be a warning? well, nsIScriptError doesn't offer message, but
I actually think it should be a warning. And I really hate getSelection's
output, it's as useless as this is. I intend to fix it at some point.

Look at the message, i think it's a perfectly reasonable warning:
Warning: Non-standard document.all property was used. Use W3C standard
document.getElementById() instead.
Source File: javascript:document.all.foo
Line: 1

brendan: as you can see from the above, jsconsole already is smart enough not
to bother listing the column number when it's 0.

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 156402 [details] [diff] [review]
scriptError, K&R, commented 0, succeeded

Sure -- how about that patch to the console service to avoid printing the
column when zero is passed? That would entail patching two tines of a
consoleBindings.xml fork:

http://lxr.mozilla.org/mozilla/search?string=%22errLineCol%22

/be

Comment 14

[Brendan Eich [:brendan]]

Timeless: duh, overlapped bug mail and bug comment reading -- thanks for
checking into the Column 0 suppression -- all's well.

/be

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 156402 [details] [diff] [review]
scriptError, K&R, commented 0, succeeded

I already said 1.8a3 is done, and shouldn't hold for this bug anyway.

/be

Comment 16

[Johnny Stenback (:jst)]

This snuk into the aviary branch with the changes in bug 256932. Oops. Marking
fixed-aviary1.0

Comment 17

[timeless]

Comment on attachment 156402 [details] [diff] [review]
scriptError, K&R, commented 0, succeeded

mozilla/dom/src/base/nsDOMClassInfo.cpp 	1.24

Comment 18

[timeless]

jst: that's great to know. especially given that I have been unable to get
approval for 1.7 branch.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 156402 [details] [diff] [review]
scriptError, K&R, commented 0, succeeded

approved per brendan and branch sync policy

Comment 20

[Brendan Eich [:brendan]]

timeless, hold on: has any document.all emulation landed on the 1.7 branch?  Not
in js/src, last I checked.  Please don't land anything yet.  Chofmann has an
idea for a better way to handle this, but drivers haven't heard it yet -- or
have they?  No, not yet.

/be

Comment 21

[timeless]

i'm already holding because the patch didn't apply (relevant document.all
changes weren't there).

note that i am resolving this bug as fixed (i meant to earlier, but you can't
resolve a bug as fixed while editting an attachment).

Comment 22

[Brendan Eich [:brendan]]

Ok, drivers evaluated chofmann's idea, and still favor sync'ing 1.7 branch with
aviary branch.  FYI, I've synced all of js/src in 1.7 from aviary just now. 
I'll run around adding fixed1.7 to the bugs I can find or think of, but if I
miss one (timeless!), feel free to add that flag for me.

/be

Comment 23

[Brendan Eich [:brendan]]

Although I should talk to jst first, since he would need to sync dom in 1.7 from
aviary, and that's more work.  So I take back the fixed1.7, and I'll wait till
jst has more time to do the dom merges.

/be

Comment 24

[Johnny Stenback (:jst)]

Fixed on the 1.7 branch.