1081245 - Make the call tree text select and copy-able.

Status: VERIFIED FIXED

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

Description

[Alon Zakai (:azakai)]

It would be great if in the firefox profiler we could save the visible results, or at least control-A then control-C to copy them all manually.

There is an option to save the entire json data, but it would also be nice I think to be able to get just the high-level data - that is, function name, time spent in it, and time spend in it + all children. Basically what is visible to the user.

The use case I have in mind is using the profiler on a codebase, then saving the results and using that in emscripten for something like PGO.

Comment 1

[Jukka Jylänki]

Geckoprofiler has the ability to actually share the result pages as links so that others can view the same data. That will not obviously replace the need to copy-paste, but would also be very useful to have.

Comment 2

[Alon Zakai (:azakai)]

Nick, what do you think about this?

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

This is to make the call-tree select + copy-able?

Could we just add `user-select: text` in the CSS?

Comment 4

[Alon Zakai (:azakai)]

I don't know enough CSS to tell myself, but that sounds right.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

This is a good first bug for a new contributor.

We need to make the contents of the profiler's call tree view[0] select-able and copy-able. This should be possible by adding the user-select[1] CSS property to the appropriate rule in the profiler's stylesheet[2].

Learn more about hacking on devtools here: https://wiki.mozilla.org/DevTools/Hacking

[0] http://dxr.mozilla.org/mozilla-central/source/browser/devtools/profiler/utils/tree-view.js#143
[1] https://developer.mozilla.org/en-US/docs/Web/CSS/user-select
[2] http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/profiler.inc.css#278

Comment 6

[Jesal]

Hi I am interested in helping with this bug, does a patch need to be written first for it to be assigned?
Thanks 
Jesal

Comment 7

[Jesal]

Hi, Thanks for the assignment, I will read into this and try and help.

Thanks

Jesal

Comment 8

[Jesal]

(In reply to Nick Fitzgerald [:fitzgen] from comment #5)
> This is a good first bug for a new contributor.
> 
> We need to make the contents of the profiler's call tree view[0] select-able
> and copy-able. This should be possible by adding the user-select[1] CSS
> property to the appropriate rule in the profiler's stylesheet[2].
> 
> Learn more about hacking on devtools here:
> https://wiki.mozilla.org/DevTools/Hacking
> 
> [0]
> http://dxr.mozilla.org/mozilla-central/source/browser/devtools/profiler/
> utils/tree-view.js#143
> [1] https://developer.mozilla.org/en-US/docs/Web/CSS/user-select
> [2]
> http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/
> profiler.inc.css#278

Hi Nick, 
I have had a look at the materials suggested. 
Where can I see this bug in action? How do I locate Firefox profiler? This so that I can code on my build and test it out?
Also I understand that the syntax needs to be added to the last-child and header items within the css?
But are do we also need to use user-select: all? as that refers to the highest ancestor in the tree? or are we defining each element with the css?

Thanks.

Jesal

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

You can open the profiler via the menu: Tools > Web Developer > Performance

When you record a profile of a page executing JS code, you get a call tree summary of the code. http://news.google.com is doing stuff a lot of the time, and tends to be a good test case if you just need activity. You can try it out there.

I don't think you need to do any last-child stuff with the selector, just match anything that has the .call-tree-cell or .call-tree-header classes.

I haven't actually done too much with user-select; I recommend trying both "text" and "all" and seeing which behavior feels more natural.

Comment 10

[Jesal]

(In reply to Nick Fitzgerald [:fitzgen] from comment #9)
> You can open the profiler via the menu: Tools > Web Developer > Performance
> 
> When you record a profile of a page executing JS code, you get a call tree
> summary of the code. http://news.google.com is doing stuff a lot of the
> time, and tends to be a good test case if you just need activity. You can
> try it out there.
> 
> I don't think you need to do any last-child stuff with the selector, just
> match anything that has the .call-tree-cell or .call-tree-header classes.
> 
> I haven't actually done too much with user-select; I recommend trying both
> "text" and "all" and seeing which behavior feels more natural.

Thanks for the advice Nick, I will work on this and let you know how I get on. Hopefully all going well I will upload a patch within the next few days.

Jesal

Comment 11

[Jesal]

Hi Nick. 

I have tried editing the css file. However I don't understand what needs to be selectable. As in the performance tab, where all the graphs are and the fields for total time and cost etc. These elements are already selectable? Which elements do you want to be copied. Is it where the function tree is ? this needs to be copied from the root and not the json data?

Thanks Jesal

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Yes, the tree of functions.

Comment 13

[Alon Zakai (:azakai)]

Any updates here? This would be very useful to have.

Comment 14

[Tommaso Visconti [:tommyblue]]

Hi Nick, I followed the Mozilla DevRoom during the Fosdem and, as your friends suggested, I'd like to try to fix a simple bug. This seems to be a good one. 
Is it still assigned to Jesal or I can try to work on it?

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Yeah, no word or patches from Jesal, feel free to work on this. Will wait to assign it until you have a work-in-progress patch attached.

See the backlog for context and relevant parts of the code base :)

Comment 16

[Tommaso Visconti [:tommyblue]]

Attachment: added -moz-user-select, without effects

I added the `-moz-user-select: text` rule to the function cell, but the text is still not selectable. The selector seems ok, I used the browser toolbox to check it. Screenshot here: http://cl.ly/image/351g3J222W1d/call-tree-cell%20selection.png

Comment 17

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Tommaso Visconti [:tommyblue] from comment #16)
> Created attachment 8560066 [details] [diff] [review]
> added -moz-user-select, without effects
> 
> I added the `-moz-user-select: text` rule to the function cell, but the text
> is still not selectable. The selector seems ok, I used the browser toolbox
> to check it. Screenshot here:
> http://cl.ly/image/351g3J222W1d/call-tree-cell%20selection.png

Strange, I'd think that would work. Unfortunately, my CSS-fu is not great. I'm needinfo'ing some people who hopefully understand this stuff better than me.

Comment 18

[Tim Nguyen :ntim]

My guess is that the child labels don't use the "normal" text frame but the xul textbox frame, and that's because we're setting the value attribute. A simple workaround is to set the textcontent instead of the value at [0].
I'll check later today to be sure.

[0] : https://dxr.mozilla.org/mozilla-central/source/browser/devtools/profiler/utils/tree-view.js#215

Comment 19

[Tim Nguyen :ntim]

(In reply to Tim Nguyen [:ntim] from comment #18)
> ...
> I'll check later today to be sure.

So, this solution didn't work. I'm guessing XUL doesn't allow text selection. A solution would be to convert the layout to xhtml. I tried to use a <span> element with html namespace, but that didn't work.

Boris, do you know if there's a way to make xul text selectable ?

Comment 20

[Boris Zbarsky [:bzbarsky]]

> I'm guessing XUL doesn't allow text selection

Which sort of XUL?  There are all sorts of different kinds of "XUL" things, some of which are more amenable to selection than others.  A standalone testcase with the sort of "XUL" involved here would probably enable me to answer the question...

Comment 21

[Tim Nguyen :ntim]

(In reply to Boris Zbarsky [:bz] from comment #20)
> > I'm guessing XUL doesn't allow text selection
> 
> Which sort of XUL?  There are all sorts of different kinds of "XUL" things,
> some of which are more amenable to selection than others.  A standalone
> testcase with the sort of "XUL" involved here would probably enable me to
> answer the question...

XUL labels in this case. I'll attach a testcase later.

Comment 22

[Boris Zbarsky [:bzbarsky]]

For a <xul:label>, using a child text node instead of a "value" attribute and styling it with "-moz-user-select: text" should work.

Comment 23

[Tim Nguyen :ntim]

(In reply to Boris Zbarsky [:bz] from comment #22)
> For a <xul:label>, using a child text node instead of a "value" attribute
> and styling it with "-moz-user-select: text" should work.

Weird, suggested this earlier (comment 18),  and it didn't seem to work. Maybe that doesn't work in chrome documents ? Or some JS is conflicting here ?

Comment 24

[Tim Nguyen :ntim]

(In reply to Tim Nguyen [:ntim] from comment #23)
> (In reply to Boris Zbarsky [:bz] from comment #22)
> > For a <xul:label>, using a child text node instead of a "value" attribute
> > and styling it with "-moz-user-select: text" should work.
> 
> Weird, suggested this earlier (comment 18),  and it didn't seem to work.
> Maybe that doesn't work in chrome documents ? Or some JS is conflicting here
> ?

Nevermind, that works perfectly, thanks !

Comment 25

[Tim Nguyen :ntim]

So what needs to be done here :

- In [0], change mousedown to click (mousedown event is conflicting with text selection)
- Do what I mentioned in comment 18
- Set -moz-user-select: text; as well (what your patch does).

[0] : https://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/AbstractTreeItem.jsm#358

Victor, is there an issue to change from mousedown to click ?

Comment 26

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

Nah. Mousedown was used because it felt faster.

Comment 27

[Brian Grinstead [:bgrins]]

Looks like this got sorted out

Comment 28

[Tommaso Visconti [:tommyblue]]

Attachment: textSelection.patch

Thanks to the comments (thank you guys!) I found how to select the text in the "Function" column. I also changed the background color of the selected text or it's impossible to recognize it when the row is selected (the background color was the same).

Nick, is this the expected result?

Probably people expect to select the whole table with CTRL+a or using the mouse (as it's possible to select an HTML table), I can do it replicating the edit for all the cells?

Comment 29

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Tommaso Visconti [:tommyblue] from comment #28)
> Probably people expect to select the whole table with CTRL+a or using the
> mouse (as it's possible to select an HTML table), I can do it replicating
> the edit for all the cells?

Let's keep this bug simple and land it quickly. We can file follow up bugs for things like ctrl+a :)

Comment 30

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8560924 [details] [diff] [review]
textSelection.patch

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

Looks good! Flag me for review (in the attachment details) when you combine all of your changes into one patch.

::: browser/devtools/shared/widgets/AbstractTreeItem.jsm
@@ +354,5 @@
>  
>      let targetNode = this._targetNode = this._displaySelf(document, arrowNode);
>      targetNode.style.MozUserFocus = "normal";
>  
> +    targetNode.addEventListener("click", this._onClick);

So the issue is that onClick preventsDefault therefore killing our text selection? I think we can keep the snappy behavior that binding to mousedown gives if we simply stop calling preventDefault in onClick. Can you try this?

::: browser/themes/shared/devtools/profiler.inc.css
@@ +304,5 @@
>  .call-tree-item:focus .call-tree-cell {
>    -moz-border-end-color: var(--focus-cell-border-color);
>  }
>  
> +.call-tree-item .call-tree-cell[type=function] label {

Just double checking: this makes every cell select-able, not just the function cell, right? The function cell just needs special casing because of the label?

@@ +309,2 @@
>    cursor: text;
>    -moz-user-select: text;

This stuff is part of the changes as well, right? Make sure you are uploading all of the changes at once -- perhaps you need to squash patches together in mq?

@@ +310,5 @@
>    -moz-user-select: text;
>  }
>  
> +.call-tree-item .call-tree-cell[type=function] label::-moz-selection {
> +  background-color: var(--theme-highlight-orange);

Perfect use of the theme variables :)

Comment 31

[Jordan Santell [:jsantell] [@jsantell]]

Hey Tommaso, have you had any time to work on this further?

Comment 32

[Tommaso Visconti [:tommyblue]]

Hey Jordan, I'm not sure what Nick asked me to do. It's already a single patch..
I tried to contact him on IRC some months ago without success.
Now I'm on a different field, but if I can help, please explain me what you think I should do :)

Comment 33

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Which part of comment 30 are you not sure about?

Comment 34

[Tommaso Visconti [:tommyblue]]

Hi :fitzgen, when you say "when you combine all of your changes into one patch", what do you mean? The patch I submitted isn't already one patch?

Comment 35

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Tommaso Visconti [:tommyblue] from comment #34)
> Hi :fitzgen, when you say "when you combine all of your changes into one
> patch", what do you mean? The patch I submitted isn't already one patch?

Maybe I was wrong, but it seemed to me that the patch that you uploaded was based on top of some mystery patch that was not uploaded. There were things that were not marked as additions in your patch that did not exist in mozilla-central or fx-team and appeared to be additions for the purpose of fixing this bug. That led me to believe that you split the work into two or more patches (perhaps accidentally?) and only attached one of the patches.

Comment 36

[Jordan Santell [:jsantell] [@jsantell]]

I think fitzgen answered my ni?

Comment 37

[Tommaso Visconti [:tommyblue]]

Hi :fitzgen, if that happened, I don't know why :)
I made the patch using instruction from Mozilla docs and using the code from fx-team repo. The patch seems ok to me, but I'm for sure not an expert...

Comment 38

[Alon Zakai (:azakai)]

This would still be very useful for Emscripten. Any updates here?

If it's tricky to get the data via copy-paste, maybe Emscripten should just process the json info from the profiler? Is there documentation on the structure of that, and of how we would compute the high-level data (what is visible in the profiler) from it?

Comment 39

[Jordan Santell [:jsantell] [@jsantell]]

Alon, what kind of data are you looking to get? CSV-style text of each call tree row? Saving the profile will get you a giant JSON object, and requires a handful of manipulation and expansion to get useful data out of it (we have utilities for this, but some overhead of learning how to use it)

Comment 40

[Alon Zakai (:azakai)]

Just how much CPU time is spent in each method should be enough. Then Emscripten can do PGO optimization, by knowing which methods are cold and can be optimized less aggressively.

Comment 41

[Jordan Santell [:jsantell] [@jsantell]]

Tommaso, are you still interested in working on this?

Comment 42

[Tommaso Visconti [:tommyblue]]

Sorry Jordan,
I'm really busy at the moment

Comment 43

[Hernán Rodriguez Colmeiro (:peregrino)]

Attachment: Updated patch

I tried to update the patch as the code seems to have moved. Let me know if this is what's needed and/or any comments you may have.

Comment 44

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8681082 [details] [diff] [review]
Updated patch

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

This makes the text copy-able and select-able, but when I copy-paste the text it all comes out on a single line.

I guess we could land this and format the text nicely as a follow up...

Comment 45

[Hernán Rodriguez Colmeiro (:peregrino)]

yeah, I saw that everything came out as a single line, but didn't know how the output was expected. Is there a way to add line breaks and tabulations for the selected text?

Comment 46

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Hernán Rodriguez Colmeiro (:peregrino) from comment #45)
> yeah, I saw that everything came out as a single line, but didn't know how
> the output was expected. Is there a way to add line breaks and tabulations
> for the selected text?

Unfortunately, I don't know. Maybe someone else does.

Comment 47

[Hernán Rodriguez Colmeiro (:peregrino)]

Alon, how do you expect the selected text to be copied? With the patch I updated the result is that all the columns and rows can be selected and copied, but the text comes as a single line without tabulations between column values nor line breaks between rows.

Comment 48

[Alon Zakai (:azakai)]

As long as there is a way to figure out what is what, it should be fine - I can write a little parser. Just needs to be possible to know where indentation increases and decreases, I guess.

Comment 49

[Hernán Rodriguez Colmeiro (:peregrino)]

Attachment: Make the call tree text select and copy-able

Aded tabulations to indicate columns and line breaks for rows. Also, nested rows have spaces in the front of the description to indicate that.

Comment 50

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8685819 [details] [diff] [review]
Make the call tree text select and copy-able

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

Thanks Hernan!

::: devtools/client/performance/modules/widgets/tree-view.js
@@ +283,5 @@
> +    lastDescrtiption.textContent = lastDescrtiption.textContent + "\n";
> +
> +    // Add spaces as frameLevel indicators in case the row is selected and
> +    // copied. These spaces won't be displayed in the cell content.
> +    let firstDescrtiption = cell.querySelector('description:first-of-type');

Nit: typos all through this new block, "descrtiption" should be "description"

Comment 51

[Hernán Rodriguez Colmeiro (:peregrino)]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #50)
> Nit: typos all through this new block, "descrtiption" should be "description"

Heh, my bad. Was a bit late in the night and didn't see it ;) I'll fix it when I get back home. Is the format ok?

Comment 52

[Hernán Rodriguez Colmeiro (:peregrino)]

Attachment: Make the call tree text select and copy-able

Fixed typos.

Comment 53

[Pulsebot]

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

Comment 54

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/integration/fx-team/rev/fda060cdf44aaa98116d4ed21d6043155ef81892
Backed out changeset 5c98c715b5c7 (bug 1081245) for devtools bustage

Comment 55

[Pas]

Hi, I am new to fixing bugs and I would like to work on the bug please.

Comment 56

[Hernán Rodriguez Colmeiro (:peregrino)]

(In reply to Pas from comment #55)
> Hi, I am new to fixing bugs and I would like to work on the bug please.

I didn't abandon this one, it was backed out because I broke some tests. Was planning to fix them over this weekend :)

Comment 57

[Hernán Rodriguez Colmeiro (:peregrino)]

Attachment: Updated tests

Updated the broken tests that caused the backout. Re-requesting review as the chenges to the tests where quite a few.

Comment 58

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8696861 [details] [diff] [review]
Updated tests

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

r=me

Comment 59

[Pulsebot]

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

Comment 60

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6b31012ac6d2

Comment 61

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED, FIXED -> VERIFIED

Component: 
Name 			Firefox
Version 		46.0b4
Build ID 		20160322075646
Update Channel 	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS                      Windows 7 SP1 x86_64

STR:
It would be great if in the firefox profiler we could save the visible results, or at least control-A then control-C to copy them all manually.

There is an option to save the entire json data, but it would also be nice I think to be able to get just the high-level data - that is, function name, time spent in it, and time spend in it + all children. Basically what is visible to the user.

The use case I have in mind is using the profiler on a codebase, then saving the results and using that in emscripten for something like PGO.

ER:
1. It would be great if in the firefox profiler we could save the visible results, or at least control-A then control-C to copy them all manually.

2. There is an option to save the entire json data, but it would also be nice I think to be able to get just the high-level data - that is, function name, time spent in it, and time spend in it + all children. Basically what is visible to the user.

3. The use case I have in mind is using the profiler on a codebase, then saving the results and using that in emscripten for something like PGO.

AR:

1. Verified. Ctrl+A and Ctrl+C working.
2. Not clear.
3. Not clear.