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)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
People
(Reporter: schapel, Assigned: timeless)
References
Details
(Keywords: fixed-aviary1.0, fixed1.7.5)
Attachments
(1 file, 2 obsolete files)
2.59 KB,
patch
|
timeless
:
review+
brendan
:
superreview+
roc
:
approval1.7.5+
brendan
:
approval1.8a3-
|
Details | Diff | Splinter Review |
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.
Attachment #155287 -
Flags: superreview?(bzbarsky)
Attachment #155287 -
Flags: review?(caillon)
Comment 2•20 years ago
|
||
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)
Comment 3•20 years ago
|
||
How about cc'ing DOM peers, if not asking jst to r= the code he just wrote? /be
Comment 4•20 years ago
|
||
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-
Comment 5•20 years ago
|
||
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)
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?
Comment 7•20 years ago
|
||
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.
Comment 9•20 years ago
|
||
Please don't ask for approval until r and sr is done - otherwise this will get lost in the shuffle.
Comment 10•20 years ago
|
||
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 11•20 years ago
|
||
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?
Assignee | ||
Comment 12•20 years ago
|
||
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 13•20 years ago
|
||
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+
Comment 14•20 years ago
|
||
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 15•20 years ago
|
||
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-
Comment 16•20 years ago
|
||
This snuk into the aviary branch with the changes in bug 256932. Oops. Marking fixed-aviary1.0
Keywords: fixed-aviary1.0
Assignee | ||
Comment 17•20 years ago
|
||
Comment on attachment 156402 [details] [diff] [review] scriptError, K&R, commented 0, succeeded mozilla/dom/src/base/nsDOMClassInfo.cpp 1.24
Assignee | ||
Comment 18•20 years ago
|
||
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+
Comment 20•20 years ago
|
||
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
Assignee | ||
Comment 21•20 years ago
|
||
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
Comment 22•20 years ago
|
||
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
Comment 23•20 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•