Used in webconsole, canvas debugger, and performance. Should move this to a shared utility in shared/ somewhere, or in gDevTools. Newest version (to be replicated) is in performance. webconsole seems to have an additional feature of allowing opening in source view?

f? as this is a pretty global change. New utilites for viewing source in debugger, style editor, scratchpad, and with the native View Source. Currently standalone (requiring a target), but maybe this would make more sense as toolbox methods (namespaced)? Only case I saw this outside of the toolbox was in the webconsole, but not sure what would trigger that case. Currently, to fetch gViewSourceUtils[0], there are many magic ways throughout the tools to do this, but this file is included in the inspector, and the inspector exposes this for others to consume (style-inspector), so the utilities start up the inspector if it's not already and use it. Super lazy, so should be ok, but if there's a cleaner way to do this (so inspector doesn't have to be in control of this) that'd be ideal. Feel free to f? anyone else who you think would be interested (esp for web console?) [0]http://mxr.mozilla.org/mozilla-central/source/toolkit/components/viewsource/content/viewSourceUtils.js https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5ea1fe71c5c

Comment on attachment 8590109 [details] [diff] [review] 1134778-view-source.patch Review of attachment 8590109 [details] [diff] [review]: ----------------------------------------------------------------- Overall, it seems logical to me, but it would nice to move ownership to the toolbox for real, I think, instead of living oddly in the inspector but also shared. (In reply to Jordan Santell [:jsantell] [@jsantell] from comment #1) > Created attachment 8590109 [details] [diff] [review] > 1134778-view-source.patch > > f? as this is a pretty global change. > > New utilites for viewing source in debugger, style editor, scratchpad, and > with the native View Source. Currently standalone (requiring a target), but > maybe this would make more sense as toolbox methods (namespaced)? Only case > I saw this outside of the toolbox was in the webconsole, but not sure what > would trigger that case. Where is this "out of toolbox" case? ::: browser/devtools/framework/toolbox.js @@ +1828,5 @@ > + if (this._gViewSourceUtils) { > + return this._gViewSourceUtils; > + } > + let inspectorPanel = yield this.loadTool("inspector"); > + return this._gViewSourceUtils = inspectorPanel.viewSourceUtils; It seems odd to me to make this available the toolbox level, but then have the toolbox pull a reference from the inspector panel... Could we just load the |viewSourceUtils.js| file in the toolbox frame instead? Really, it would be much better if |gViewSourceUtils| were instead an require()able module, but I realize that could be too much to ask for.

Comment on attachment 8590109 [details] [diff] [review] 1134778-view-source.patch Review of attachment 8590109 [details] [diff] [review]: ----------------------------------------------------------------- The "outside the toolbox" case was the webconsole checking if the toolbox exists, and then handling that case, and not sure if that can exist or what, so whether or not we need to support a scenario where there is no toolbox -- which would affect whether or not we can attach it to toolbox. I'll move these methods to toolbox instance, also with the shared lib, and see if i can pull out inspector exposing its own version, with the toolbox.xul hosting the viewSourceUtils in the first place. Thanks! ::: browser/devtools/framework/toolbox.js @@ +1828,5 @@ > + if (this._gViewSourceUtils) { > + return this._gViewSourceUtils; > + } > + let inspectorPanel = yield this.loadTool("inspector"); > + return this._gViewSourceUtils = inspectorPanel.viewSourceUtils; Agreed -- this was something I wanted to ask you, since I wasn't quite sure how it worked. Seemed that the inspector had it, as well as the browser toolbox. +1 on the requireable, but this is just a direct translation, can get fancy in the future IMO

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #3) > Comment on attachment 8590109 [details] [diff] [review] > 1134778-view-source.patch > > Review of attachment 8590109 [details] [diff] [review]: > ----------------------------------------------------------------- > > The "outside the toolbox" case was the webconsole checking if the toolbox > exists, and then handling that case, and not sure if that can exist or what, > so whether or not we need to support a scenario where there is no toolbox -- > which would affect whether or not we can attach it to toolbox. Ah, I think the "no toolbox" case is the Browser Console. There, the lonely console is all by itself, and so there is no guarantee of access to other tools. But, it seems your translation of the code would handle this and fall back to plain view source as before.

Comment on attachment 8590109 [details] [diff] [review] 1134778-view-source.patch Review of attachment 8590109 [details] [diff] [review]: ----------------------------------------------------------------- omg finally

This should land after bug 1077464 lands

Comment on attachment 8591113 [details] [diff] [review] 1134778-view-source.patch Review of attachment 8591113 [details] [diff] [review]: ----------------------------------------------------------------- A few remaining things to fix, looks good overall. I'll be gone for the next week, so I trust Victor to handle any further rounds. :) ::: browser/devtools/framework/toolbox.js @@ +165,5 @@ > CUSTOM: "custom" > }; > > Toolbox.prototype = { > + _gViewSourceUtils: null, Nit: This is kind of surprising to see at the top of the prototype, since it's kind of an ancillary feature... But, no great suggestions either. :) @@ +1824,5 @@ > + > + /** > + * Returns gViewSourceUtils for viewing source. > + */ > + get gViewSourceUtils() { Wait, what is |_gViewSourceUtils| for then? Were you going to cache it? ::: browser/devtools/shared/source-utils.js @@ +123,5 @@ > + * @return {Promise} > + */ > +exports.viewSource = Task.async(function *(toolbox, sourceURL, sourceLine) { > + let utils = toolbox.gViewSourceUtils; > + // XXX does it matter what document we use for this? I don't believe it matters. ::: browser/devtools/shared/timeline/marker-details.js @@ +218,5 @@ > hbox.appendChild(aNode); > > aNode.addEventListener("click", (event) => { > event.preventDefault(); > + this.emit("view-source", url, line); Hmm, where is the listener for this?

Comment on attachment 8591113 [details] [diff] [review] 1134778-view-source.patch Review of attachment 8591113 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/framework/toolbox.js @@ +1824,5 @@ > + > + /** > + * Returns gViewSourceUtils for viewing source. > + */ > + get gViewSourceUtils() { Good catch -- yeah, was cached when it involved talking to the inspector -- removed the _gViewSourceUtils ::: browser/devtools/shared/source-utils.js @@ +123,5 @@ > + * @return {Promise} > + */ > +exports.viewSource = Task.async(function *(toolbox, sourceURL, sourceLine) { > + let utils = toolbox.gViewSourceUtils; > + // XXX does it matter what document we use for this? +1 ::: browser/devtools/shared/timeline/marker-details.js @@ +218,5 @@ > hbox.appendChild(aNode); > > aNode.addEventListener("click", (event) => { > event.preventDefault(); > + this.emit("view-source", url, line); In performance/views/details-waterfall.js

Waiting to land after bug 1077464, which will require rebasing https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7533fc425b6

Rebased after the console.profile patch was backed out and relanded. https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d21cccfc820

Is this ready to land again?

browser/devtools/framework/test/browser_toolbox_view_source_04.js is still failing only on try

Rebased