* caillon wishes venkman could inspect xbl <rginda> caillon: file a bug against hyatt to include line numbers <rginda> caillon: and fix his "urls" <rginda> "chrome://bar.xml#whatever (getter)" is hardly a url

Passing a start line number of 0 prevented the debugger from even seeing the bogus urls. This patch passes a start line of 1, which isn't right either, but in this case it's better than 0. You'll be able to debug XBL with Pretty Print enabled. The URLs aren't exactly right either, but it's closer than it was.

heh..the chrome URLs were my bad.. I welcome any suggestions :) I'll take a look at this patch. I think it used to be that a null string would prevent the start line issue..

Comment on attachment 93128 [details] [diff] [review] first cut at a patch this seems like a good start, anyway :) sr=alecf

My suggestion is to pass the correct URL, without any "#foo" or " (getter)" cruft appended to it, and to pass the correct starting line number. I imagine you've seen the maze of functions that need to be fixed in order to get the line number to the correct place, it's enough to make a person cry :( This patch represents a better-than-nothing fix. I dropped the bogus " (getter)" suffix, and would have dropped the "#foo" if I could have found an easy way to do it. I also pass 1 as the start line, instead of 0. Passing 0 causes the js debugger to choke on the script (because JS line numbers are 1 based, 0 indicates some sort of API misuse.) The start line of 1 makes it possible to debug XBL with Venkman's pretty print mode, which doesn't need to map PC -> source lines. I'd like to figure out how to drop the "#foo" stuff before trying to check this in.

yep, and you'll need to wade through the other mess that is finding line numbers from the htmlparser (i.e. via expat) - I spent some time chasing my tail on that one a few months ago and finally gave up.

Are you wanting to check this in now and then keep working on the rest?

No, the hash in the urls causes the debugger to display the file multiple times. You end up with browser.xml#foo, browser.xml#bar, etc. Except, the debugger strips the hash from the filename before displaying it, and it just looks like browser.xml loaded multiple times. Users won't know what to do with themselves. Having debuggable XBL cause a few other hardships for Venkman. Before (or very shortly after) any of this lands there needs to be a way to disable XBL suppor in the debugger (a-la "Debug->Exclude Browser Files".) Has anyone else tried this patch out? Any comments?

I have started looking into using XBL in contexts where MS is using behaviors. This could be a big win for us however the lack of the ability to debug XBL is a truly monumental PITA. I can try to apply this patch to a CVS build but if I am going to promote XBL to MSIE shops which already use behaviors this would be a blocker for them. Any news?

*** Bug 209118 has been marked as a duplicate of this bug. ***

The one concern I have here is the additional bloat... someone more familiar with XBL than I will need to evaluate how much of a problem that is.

Comment on attachment 125708 [details] [diff] [review] Something that covers a lot of cases... The key problem here (that necessitates the bloat) is that we know the line number at a point far before we actually need to pass it to the JS engine... so we have to stash it somewhere until we compile the scripts.

Ah, I see you've found the XXXbe comment in nsJSEnvironment.cpp nsJSContext::CompileEventHandler. I wouldn't worry about bloat yet. /be

Comment on attachment 125708 [details] [diff] [review] Something that covers a lot of cases... IMO, this bloat is warranted. sr=alecf "JS event handler do not have line numbers associated to them" ../with/ them. maybe you want to mention that the line number should be the current line in the HTML/XML file?

Taking. Once this patch is in, I'll do a second one for the event handler stuff, per conversation with Brendan. Then all we need to do is to get normal HTML event handlers to pass line numbers... ;)

typo: you meant |newHandler->SetLineNumber| here? if (newHandler) { + mHandler->SetLineNumber(aLineNumber); + // Add this handler to our chain of handlers. if (mHandler) mHandler->SetNextHandler(newHandler); //...

Yes, I did. Thank you for catching that; fixed locally.

This also fixes up the bloat issue by keeping the line number in a data structure that is deleted when the script is compiled.

Comment on attachment 126183 [details] [diff] [review] Merge to some changes that landed Alec, could you take another look? Brian, could you review?

Comment on attachment 126183 [details] [diff] [review] Merge to some changes that landed sr=alecf

Comment on attachment 126183 [details] [diff] [review] Merge to some changes that landed Patch checked in (marking alec's review). I guess I'll work on the handlers next....

Comment on attachment 127338 [details] [diff] [review] Also fix handlers and fields Thoughts?

Comment on attachment 127338 [details] [diff] [review] Also fix handlers and fields Never mind me. There is no good reason to use nsXBLTextWithLineNumber here, since these classes never let go of the text....

Comment on attachment 127340 [details] [diff] [review] This is more like it.... Looks great to me -- thanks for the 80th column sanctity fixes too. One nit, occurs in two places: + * @param aLineNo the starting line number for the script for error messages "of the script for error messages" would be better than "for ... for ...." /be

Comment on attachment 127340 [details] [diff] [review] This is more like it.... >+nsXBLProtoImplField::InstallMember(nsIScriptContext* aContext, >+ nsIContent* aBoundElement, >+ void* aScriptObject, >+ void* aTargetClassObject, >+ const nsCString& aClassStr) > { > if (mFieldTextLength == 0) > return NS_OK; // nothing to do. > > JSContext* cx = (JSContext*) aContext->GetNativeContext(); > JSObject * scriptObject = (JSObject *) aScriptObject; > NS_ASSERTION(scriptObject, "uh-oh, script Object should NOT be null or bad things will happen"); > if (!scriptObject) > return NS_ERROR_FAILURE; > >+ nsCAutoString bindingURI(aClassStr); >+ PRInt32 hash = bindingURI.RFindChar('#'); >+ if (hash != kNotFound) { >+ bindingURI.Truncate(hash); >+ } >+ The style for this file seems to be that single-line |if| blocks aren't braced. Looks good otherwise, r=bryner.

Checked in.