Add-on icon tooltip should contain the add-on name

[past]

...because this is not very useful:

[julienw]

@past can you please share a link to this very profile?

[julienw]

I did one here: https://perfht.ml/2jVYcZf

[julienw]

We use the same string for the title text than for the icon URL. We should change this.

Here is how we can do this:

1. in src/profile-logic/call-tree.js: https://github.com/devtools-html/perf.html/blob/cdb62d7776a617209972d93f944a9f1141e81552/src/profile-logic/call-tree.js#L255-L260 We can have 2 properties: for example iconSrc contains the URL while just icon contains the text. The name of the property to use for the title text is defined in https://github.com/devtools-html/perf.html/blob/cdb62d7776a617209972d93f944a9f1141e81552/src/components/calltree/CallTree.js#L84 But this can be changed if needed. (Note that for other columns than the icon, the same text is used for the title and the content, maybe we could change this too, but this is out of scope for this issue)

2. In src/components/calltree/NodeIcon.js, change the property used for the source URL: change displayData.icon to displayData.iconSrc as proposed above: https://github.com/devtools-https://github.com/devtools-html/perf.html/blob/cdb62d7776a617209972d93f944a9f1141e81552/src/components/calltree/NodeIcon.js#L53-L56 Note that https://github.com/devtools-html/perf.html/pull/1000 will move NodeIcon and use it elsewhere so you'll need to change the new code too if it's merged before this.

For completeness, this is where the title is actually set: https://github.com/devtools-html/perf.html/blob/cdb62d7776a617209972d93f944a9f1141e81552/src/components/shared/TreeView.js#L140-L146 But there's nothing to change here. TreeView is used in a very generic way in several locations, that's why the code to change is very disconnected from the actual rendering code.

Likely tests will need to be updated.

[julienw]

For a node coming from an add-on, here is how we can get its name: https://github.com/devtools-html/perf.html/blob/cdb62d7776a617209972d93f944a9f1141e81552/src/profile-logic/profile-data.js#L1269-L1275

[arcturusInk]

@julienw I would like to be assigned to look into this, if this is still an issue that needs to be done.

This is what I understand the problem to be: when the cursor is hovering over the addon sign, the tooltip is displaying a text. Instead, the addon's name should be displayed.

Looking through @julienw comment from May 15, these are my understandings/confusion: 1) I am a bit confused by what this means, "We use the same string for the title text than for the icon URL" 2) The variable, propName is used to hold the value of the text that's currently being displayed. 3) Found NodeIcon.js at src/components/shared/NodeIcon.js. If displayData.icon is changed to displayData.iconSrc, I am assuming the URL of the addon would be displayed instead of the text that's being displayed now? 4) Assuming the variable, origin holds the name of the addon?

@julienw Please let me know if my assumptions are incorrect, or if there is anything wrong with my "understandings"

Thanks.

[arcturusInk]

Ah, also I am having problems downloading the Gecko Profiler addon. Firefox is giving this notification, " the addon could not be downloaded because of a connection failure"

[julienw]

This is what I understand the problem to be: when the cursor is hovering over the addon sign, the tooltip is displaying a text. Instead, the addon's name should be displayed.

That's correct.

I am a bit confused by what this means, "We use the same string for the title text than for the icon URL"

What I mean is that we use the same data for the title: https://github.com/devtools-html/perf.html/blob/c4dfd6b99dbec86d83f00b5e4386d44827a9a666/src/components/shared/TreeView.js#L139 and for the icon URL: https://github.com/devtools-html/perf.html/blob/c4dfd6b99dbec86d83f00b5e4386d44827a9a666/src/components/shared/NodeIcon.js#L55

I think you found it yourself too, so I don't understand what your confusion is.

Found NodeIcon.js at src/components/shared/NodeIcon.js. If displayData.icon is changed to displayData.iconSrc, I am assuming the URL of the addon would be displayed instead of the text that's being displayed now?

What I suggest is that we should have 2 properties:

• iconSrc would hold the icon image source
• icon (or iconTitle could be better) would hold the icon title.

Note that in the NodeIcon component we don't set the title, we only display the icon.

I mark this bug assigned because you're working on this :) Thanks !

[julienw]

Ah, also I am having problems downloading the Gecko Profiler addon. Firefox is giving this notification, " the addon could not be downloaded because of a connection failure"

What happens if you try to download the file directly ? The URL is https://raw.githubusercontent.com/devtools-html/Gecko-Profiler-Addon/master/gecko_profiler.xpi.

[arcturusInk]

@julienw Thanks for answering my questions so quickly. I was able to install Gecko Profiler addon from the link!

I have changed the variable name, icon to iconSrc in this two files. https://github.com/devtools-html/perf.html/blob/cdb62d7776a617209972d93f944a9f1141e81552/src/profile-logic/call-tree.js#L255-L269 https://github.com/devtools-html/perf.html/blob/c4dfd6b99dbec86d83f00b5e4386d44827a9a666/src/components/shared/NodeIcon.js#L55

My follow-up question is how do I get the icon name from https://github.com/devtools-html/perf.html/blob/cdb62d7776a617209972d93f944a9f1141e81552/src/profile-logic/profile-data.js#L1269-L1275 and have that variable available in file CallTree.js so that I can do something like this: { propName: 'iconTitle', title: '', component: NodeIcon },

Also, how can I reproduce a profile similar to https://perfht.ml/2jVYcZf?

Please let me know if I did something wrong here. Thank you

[julienw]

My follow-up question is how do I get the icon name from

See https://github.com/devtools-html/perf.html/issues/1001#issuecomment-389100832 :) You can try to use this bit of code:

let iconSrc = null;
let iconTitle = ''; 
if (resourceType === resourceTypes.webhost) {
  iconSrc = extractFaviconFromLibname(libName);
  iconTitle = libName;
} else if (resourceType === resourceTypes.addon) {
  iconSrc = ExtensionIcon;
  const resourceNameIndex = this._resourceTable.name[resourceIndex];   
  if (resourceNameIndex !== undefined) { 
    iconTitle = this._stringTable.getString(resourceNameIndex); 
  }
}

Note I didn't try this code.

Also, how can I reproduce a profile similar to https://perfht.ml/2jVYcZf?

You don't need to reproduce it directly, as you can load the same profile with your local version by replacing https://perf-html.io by http://localhost:4242: http://localhost:4242/public/3b385fcef7147e3d218572e008acff2175005636/calltree/?globalTrackOrder=0-1-2-3-4&hiddenGlobalTracks=0-1-2-3&hiddenLocalTracksByPid=2320-0&implementation=js&localTrackOrderByPid=2320-0-1~&thread=5&transforms=f-js-zUs&v=3

FTR I produced this profile with the plugin ublock, and loading a website like cnn.com.

[arcturusInk]

@julienw

I was able to change icon to iconSrc and have the add-on(s) be displayed. Though, I had to modify additional files: icon.js and profile-derived.js to do so.

I tested the code above. Running it, iconTitle would have the following value: I parsed the string using regex: iconTitle = iconTitle.match(/"([^"]*)"/)[1]; So iconTitle has the following value:

Suggestion: it might be good to test other profiles, and see if they are giving similar outputs. Please let me know if you have feedback on what I was able to do so far.

I am assumming in CallTree.js { propName: 'icon', title: '', component: NodeIcon }, icon is getting its value from file NodeIcon.js. How can I change this so it instead gets its value from call-tree.js?

Thanks.

[julienw]

Can you please share your current code to a pull request? This makes it a lot easier to give feedback :-)

I am assumming in CallTree.js { propName: 'icon', title: '', component: NodeIcon }, icon is getting its value from file NodeIcon.js. How can I change this so it instead gets its value from call-tree.js?

NodeIcon is the component we use to render the element (see: https://github.com/devtools-html/perf.html/blob/2b61c6ed2c025024bfe037ed608d5650d3b7fca8/src/components/shared/TreeView.js#L138)

This is where we get the value in NodeIcon: https://github.com/devtools-html/perf.html/blob/624f71bce5469cf4f8b2be720e929ba69fa6bfdc/src/components/shared/NodeIcon.js#L55

But we still use propName in this case in https://github.com/devtools-html/perf.html/blob/2b61c6ed2c025024bfe037ed608d5650d3b7fca8/src/components/shared/TreeView.js#L141-L148 so it needs to be correct.

[arcturusInk]

Was able to do this.

I will submit a pull request using the instructions from GitHub; it will my first ever pull request. Any other tips on how make a PR would be appreciated.

@julienw Thank you for your patience. But I think there are still a few things I might need to change/test.

@julienw Again, thank you for guiding me through this

[julienw]

Hey @arcturusInk, thanks for your work! I think the documentation you want is creating a pull request from a fork. If you haven't created a fork yet, you can read this help page too. If you need more help, please ask here, or, better, on Slack!

[arcturusInk]

@julienw

A few things:

• I only tested my changes with this profile: http://localhost:4242/public/3b385fcef7147e3d218572e008acff2175005636/calltree/?globalTrackOrder=0-1-2-3-4&hiddenGlobalTracks=0-1-2-3&hiddenLocalTracksByPid=2320-0&implementation=js&localTrackOrderByPid=2320-0-1~&thread=5&transforms=f-js-zUs&v=3.
• Have modified tests so my changes would pass. Hope that's okay. Haven't written any new tests for new variable, iconTitle
• Kept getting this error: “Cannot assign iconTitle.match(/"([^"]*)"/)[1]; to iconTitle because an indexer property is missing in null or undefined”. So commented out this line: iconTitle = iconTitle.match(/"([^"]*)"/)[1]; to pass the test. This is the current output: There is probably other ways of getting the title. Any hints?
• I have been getting this error: Not sure how to fix it. This is the change I made to icons.tes.js Any hint on how to pass the test or what’s making it fail?

[julienw]

Hey @arcturusInk, the easiest would be to push your current changes to a pull request :)

[julienw]

hey @arcturusInk, any update from your side? I know you told me about your update on Slack, it would be very nice if we can see the code and wrap this up :) Thanks!

[julienw]

Hey @arcturusInk, I removed the assigned label because I didn't get any update from you. However I'm still interested to see your current patch even if it's not working properly yet, because it can be a good start for another person. Thanks!

[julienw]

This is still current and pretty well self-contained!

[Cheederah]

Please can i be assigned to this @julienw