834878 - link arrows go nowhere

Status: VERIFIED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Description

[Rob Campbell [:rc] (:robcee)]

The link arrows next to the source name in the profiler don't do anything. They should at least point to view-source: for chrome files. For content, they should bring you to the appropriate source:line in the debugger.

Comment 1

[Markus Stange [:mstange]]

In the original cleopatra, their purpose is to restrict the view to the selected subtree of the profile. I don't know why they don't do that in the Firefox version.

Comment 2

[Anton Kovalyov (:anton)]

Attachment: Make link arrows open files

We decided to change link arrows. In the original Cleopatra they focus on a call stack but we decided to make them open files instead. If file is content we open it in the debugger, otherwise we use view-source. I also removed link arrows from stuff like (total) because there's no file associated with it.

Comment 3

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 713750 [details] [diff] [review]
Make link arrows open files

Review of attachment 713750 [details] [diff] [review]:
-----------------------------------------------------------------

change a method name. Otherwise it's good (bro).

::: browser/devtools/profiler/ProfilerPanel.jsm
@@ +243,5 @@
> +    let type = win.document.documentElement.getAttribute("windowtype");
> +
> +    if (type !== "navigator:browser") {
> +      win = Services.wm.getMostRecentWindow("navigator:browser");
> +    }

guessing you need this when the devtools window is detached?

@@ +445,5 @@
> +   *    - uri
> +   *    - line
> +   *    - isChrome (chrome files are opened via view-source)
> +   */
> +  openFile: function PP_openFile(data, onOpen=function() {}) {

can we rename this to something more like "displaySource"? This sounds like we'd use it to open a filesystem file.

@@ +459,5 @@
> +      panelWin.editor.setCaretPosition(data.line - 1);
> +      onOpen();
> +    }
> +
> +    if (data.isChrome) {

you could probably guess this correctly in here rather than having to pass in as an argument.

@@ +464,5 @@
> +      return void this.browserWindow.gViewSourceUtils.
> +        viewSource(data.uri, null, this.document, data.line);
> +    }
> +
> +    gDevTools.showToolbox(this.target, "jsdebugger").then(function (toolbox) {

it's good, bro.

::: browser/devtools/profiler/cleopatra/js/tree.js
@@ +466,4 @@
>        '<span class="resourceIcon" data-resource="' + node.library + '"></span> ' +
>        '<span class="functionName">' + nodeName + '</span>' +
>        '<span class="libraryName">' + libName + '</span>' +
> +      (nodeName === '(total)' ? '' :

that's a pretty long line.

::: browser/devtools/profiler/cleopatra/js/ui.js
@@ +169,5 @@
>      self._onContextMenuClick(e);
>    });
>    this.treeView.addEventListener("focusCallstackButtonClicked", function (frameData) {
> +    // NOTE: Not in the original Cleopatra source code.
> +    notifyParent("openfile", {

probably want to change the notification to reflect whatever the method name gets changed to. e.g., "displaysource".

Comment 4

[Markus Stange [:mstange]]

Maybe we should change the icon then. Otherwise Instruments users will be very confused to find a button with the same icon but a different meaning.

Comment 5

[Anton Kovalyov (:anton)]

https://hg.mozilla.org/integration/fx-team/rev/f627b7d881c8

Comment 6

[Anton Kovalyov (:anton)]

(In reply to Markus Stange from comment #4)
> Maybe we should change the icon then. Otherwise Instruments users will be
> very confused to find a button with the same icon but a different meaning.

I agree, I'll open a followup for this since we I can't draw icons myself.

Comment 7

[Girish Sharma [:Optimizer]]

What about if the file is not chrome but still not available to debugger ? There are cases when the files get GC'ed or just take a very long time to load. According to the discussions in bug 766001, the same flow should also be applied here, which is to wait for some time and then open view source instead, even for content scripts.

Also, I wonder if the tests will pass this easily if they involved 2 content scripts, instead of one.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/f627b7d881c8

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 713750 [details] [diff] [review]
Make link arrows open files

Review of attachment 713750 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/profiler/ProfilerPanel.jsm
@@ +469,5 @@
> +      let dbg = toolbox.getCurrentPanel();
> +      panelWin = dbg.panelWin;
> +
> +      let view = dbg.panelWin.DebuggerView;
> +      if (view.Source && view.Sources.selectedValue === data.uri) {

You have a typo here (Source -> Sources) that breaks scrolling to another line when the right script is already selected in the debugger.

Comment 10

[Victor Porof [:vporof][:vp]]

(In reply to Panos Astithas [:past] from comment #9)
> Comment on attachment 713750 [details] [diff] [review]
> Make link arrows open files
> 
> Review of attachment 713750 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/profiler/ProfilerPanel.jsm
> @@ +469,5 @@
> > +      let dbg = toolbox.getCurrentPanel();
> > +      panelWin = dbg.panelWin;
> > +
> > +      let view = dbg.panelWin.DebuggerView;
> > +      if (view.Source && view.Sources.selectedValue === data.uri) {
> 
> You have a typo here (Source -> Sources) that breaks scrolling to another
> line when the right script is already selected in the debugger.

I don't even think the extra check is necessary. The view.Sources object should already be created at this point.

Comment 11

[Anton Kovalyov (:anton)]

(In reply to Girish Sharma [:Optimizer] from comment #7)
> What about if the file is not chrome but still not available to debugger ?
> There are cases when the files get GC'ed or just take a very long time to
> load. According to the discussions in bug 766001, the same flow should also
> be applied here, which is to wait for some time and then open view source
> instead, even for content scripts.
> 
> Also, I wonder if the tests will pass this easily if they involved 2 content
> scripts, instead of one.

Yeah I saw that. The 350 milliseconds timeout you used in your patch is not enough since during my manual tests view-source was being triggered every single time. But the real problem is that, in case of a timeout, you get both view-source and partially opened debugger panel. In my case, if it takes a long time for the debugger to open a file user will see exactly that: debugger taking long time to open a file. Pretty explicit IMHO.

Comment 12

[Anton Kovalyov (:anton)]

(In reply to Victor Porof [:vp] from comment #10)
> (In reply to Panos Astithas [:past] from comment #9)
> > Comment on attachment 713750 [details] [diff] [review]
> > Make link arrows open files
> > 
> > Review of attachment 713750 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/devtools/profiler/ProfilerPanel.jsm
> > @@ +469,5 @@
> > > +      let dbg = toolbox.getCurrentPanel();
> > > +      panelWin = dbg.panelWin;
> > > +
> > > +      let view = dbg.panelWin.DebuggerView;
> > > +      if (view.Source && view.Sources.selectedValue === data.uri) {
> > 
> > You have a typo here (Source -> Sources) that breaks scrolling to another
> > line when the right script is already selected in the debugger.
> 
> I don't even think the extra check is necessary. The view.Sources object
> should already be created at this point.

Yeah, thanks Panos and Victor. I totally screwed up yesterday by not uploading the final patch here. Doing it right now. Also will follow-up with a typo fix.

Comment 13

[Anton Kovalyov (:anton)]

Attachment: Make link arrows open files

Comment 14

[Mihaela Velimiroviciu (:mihaelav)]

Verified the fix on latest Nightly (22.0a1) and Aurora(21.0a2) builds across main platforms (Win 7, Ubuntu 12.10, Mac 10.8): the arrows launch either the debugger, or the source file