Assignee: nobody → jason_barnabe
Status: NEW → ASSIGNED
Attachment #216682 - Flags: first-review?
Attachment #216682 - Flags: first-review? → first-review?(mconnor)
Thanks for the patch. Some details: > function ViewSourceOfURL(aURL, aPageDescriptor, aDocument) Why not optionally include a line number argument here also? Like that, this function could serve extensions as part of the internal API for extensions. Otherwise, it probably wouldn't even be necessary (since it just passes on the arguments). >+ viewSource: function(aURL, aPageDescriptor, aDocument, lineNumber) nit: aLineNumber (also for openInInternalViewer and openInInternalViewer) + openInExternalEditor: function(aURL, aPageDescriptor, aDocument, aCallBack, lineNumber) Why not lineNumber before aCallBack? This would keep the arguments to openInInternalViewer and openInExternalEditor the same - except for the additional callback for the latter. >+ var lineNumberArg, goToLine; Why not default goToLine to false here instead of setting it in the else branch below? >+ goToLine = !!lineNumberArg; goToLine = lineNumberArg.length > 0 or even = lineNumberArg.indexOf("%s") > -1 would rather describe what you test for.
(In reply to comment #2) > > function ViewSourceOfURL(aURL, aPageDescriptor, aDocument) > > Why not optionally include a line number argument here also? Like that, this > function could serve extensions as part of the internal API for extensions. I'm intending on having gViewSourceUtils.viewSource be the internal API to call the appropriate View Source. I'm not removing ViewSourceOfURL in case something already calls it. I'm not giving it a line number argument because nothing in browser opens View Source to a line number. > + openInExternalEditor: function(aURL, aPageDescriptor, aDocument, aCallBack, > lineNumber) > Why not lineNumber before aCallBack? Just to keep current users of openInExternalEditor working. I'm not sure if that's necessary, but I'll leave it as is and see what the reviewer has to say about it.
(In reply to comment #3) > I'm not giving it a line number argument because nothing in > browser opens View Source to a line number. My point was that an extension might want to do so and if gViewSourceUtils.viewSource is supposed to be internal, ViewSourceOfURL would be the place for the additional argument. > Just to keep current users of openInExternalEditor working. Since viewSourceUtils.js hasn't shipped with anything else but nightlies, I doubt that backwards compatibility will be an issue here (ignoring the fact that compatibility has already been broken between releases for as little as one capitalized letter). I'd try to stick to the cleaner solution here - but of course that's just my opinion.
Updated to comments. Just to reiterate, I want view source consumers to use gViewSourceUtils.viewSource. ViewSourceOfURL in browser.js is just for backwards compatibility.
Attachment #216804 - Flags: first-review? → first-review?(mconnor)
How come no one posts SIMPLE ANSWERS to this problem? I have spent hours already trying to fix this! The only solution may be to go back to IE!!!! which displays the word YAHOO on the web page at least. Mozilla doesn't, amoungst other things that it no longer displays! I am so annoyed ....It drops the label according to JS Console.
An enhancement to a behaviour governed by a hidden pref isn't a blocker, but I'll try to get to the review in time to take the patch.
Flags: blocking-firefox2? → blocking-firefox2-
Attachment #216804 - Flags: first-review?(mconnor) → first-review?(gavin.sharp)
Comment on attachment 216804 [details] [diff] [review] patch v2 For what it's worth, posting a short comment describing the changes in the patch makes reviewing patches like this (where I'm not entirely familiar with the code) much easier. 1) For the script changes to console.xul: does viewSourceUtils depend on contentAreaUtils? For what? 2) For the consoleBindings.xml click handler: it looks to me that since it always used to launch viewSource.xul, the external editor pref didn't work for those links, and now you're fixing it so that it does, correct? 3) Components.util.reportError() should be preferred over dump(), generally 4) Seems to me that if we're going to add this ability to pass a parameter to the external editor, we should probably consider making it more generic ("view_source.editor.args"?), and using something like "%LINE%" for the line number, in case we want to add additional arguments to the editor later on.
(In reply to comment #8) > 1) For the script changes to console.xul: does viewSourceUtils depend on > contentAreaUtils? For what? Yes, for the functions used in getTemporaryFile. > 2) For the consoleBindings.xml click handler: it looks to me that since it > always used to launch viewSource.xul, the external editor pref didn't work for > those links, and now you're fixing it so that it does, correct? Yes, per comment 1. Since at the time it didn't go to the correct line number, I didn't change that when external editors was implemented. > 4) Seems to me that if we're going to add this ability to pass a parameter to > the external editor, we should probably consider making it more generic > ("view_source.editor.args"?), and using something like "%LINE%" for the line > number, in case we want to add additional arguments to the editor later on. nsIProcess accepts an array of arguments, not a string.
Attachment #216804 - Flags: first-review?(gavin.sharp) → first-review-
Trying to get this issue fixed for Firefox 3, if you don't mind, Jason... (In reply to comment #10) > I'm inclined to say that we should remove ViewSourceOfURL Agreed and done. Should any extension have relied upon ViewSourceOfURL, it'll so note the new feature(s) we now offer. > It would be nice to try and avoid having to include contentAreaUtils Unfortunately we need major parts of that file for one method. Still, we now include it ourselves where we need it (without polluting the global namespace). (In reply to comment #8) > we should probably consider making it more generic Done. The only potential issue is that we now have to always pass a line number, even when we've got none. Passing 0 for the time being which hopefully means "beginning of the file" or "invalid/default value" for all editors (passing 1 could cause more sophisticated editors to highlight the first line when we don't necessarily want that to happen).
Never mind. Neither blocking nor wanted-1.9+, thus hardly a chance of getting review in time for Firefox 3.0.
Assignee: zeniko → nobody
Status: ASSIGNED → NEW
Giving this one another try for Firefox 3.1... The patch hasn't really changed (except for some unbitrotting), so comment #11 still applies.
Attachment #335748 - Flags: review?(gavin.sharp) → review+
Comment on attachment 335748 [details] [diff] [review] unbitrotted Sorry for the delay, bug 454022 was preventing my testing.
Assignee: nobody → zeniko
Status: NEW → ASSIGNED
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1
You need to log in before you can comment on or make changes to this bug.