Closed Bug 253405 Opened 20 years ago Closed 20 years ago

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

Categories

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

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: schapel, Assigned: timeless)

References

Details

(Keywords: fixed-aviary1.0, fixed1.7.5)

Attachments

(1 file, 2 obsolete files)

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.
Assignee: general → timeless
Status: NEW → ASSIGNED
Attachment #155287 - Flags: superreview?(bzbarsky)
Attachment #155287 - Flags: review?(caillon)
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).
Attachment #155287 - Flags: superreview?(bzbarsky) → superreview?(brendan)
How about cc'ing DOM peers, if not asking jst to r= the code he just wrote?

/be
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
Attachment #155287 - Flags: superreview?(brendan) → superreview-
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
Flags: blocking1.7.3+
Flags: blocking-aviary1.0+
Attachment #155287 - Flags: review?(caillon)
Attached patch dom style'd (obsolete) — Splinter Review
Attachment #155287 - Attachment is obsolete: true
Attachment #155876 - Flags: superreview?(brendan)
Attachment #155876 - Flags: review?(caillon)
Attachment #155876 - Flags: approval1.8a3?
Attachment #155876 - Flags: approval1.7.3?
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.
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.
Please don't ask for approval until r and sr is done - otherwise this will get
lost in the shuffle.
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.
Attachment #155876 - Flags: review?(caillon) → review+
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
Attachment #155876 - Flags: approval1.8a3?
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.
Attachment #155876 - Attachment is obsolete: true
Attachment #156402 - Attachment description: scriptERror, K&R, commented 0, succeeded → scriptError, K&R, commented 0, succeeded
Attachment #156402 - Flags: superreview?(brendan)
Attachment #156402 - Flags: review+
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
Attachment #156402 - Flags: superreview?(brendan) → superreview+
Timeless: duh, overlapped bug mail and bug comment reading -- thanks for
checking into the Column 0 suppression -- all's well.

/be
Attachment #156402 - Flags: approval1.8a3?
Attachment #156402 - Flags: approval1.7.3?
Attachment #155876 - Flags: superreview?(brendan)
Attachment #155876 - Flags: approval1.7.3?
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
Attachment #156402 - Flags: approval1.8a3? → approval1.8a3-
This snuk into the aviary branch with the changes in bug 256932. Oops. Marking
fixed-aviary1.0
Keywords: fixed-aviary1.0
Comment on attachment 156402 [details] [diff] [review]
scriptError, K&R, commented 0, succeeded

mozilla/dom/src/base/nsDOMClassInfo.cpp 	1.24
jst: that's great to know. especially given that I have been unable to get
approval for 1.7 branch.
Comment on attachment 156402 [details] [diff] [review]
scriptError, K&R, commented 0, succeeded

approved per brendan and branch sync policy
Attachment #156402 - Flags: approval1.7.3? → approval1.7.3+
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
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).
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
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
Keywords: fixed1.7
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
Keywords: fixed1.7
Fixed on the 1.7 branch.
Keywords: fixed1.7.x
Depends on: 248549
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: