Closed Bug 1356536 Opened 3 years ago Closed Last year

Render an icon in the File column indicating request type

Categories

(DevTools :: Netmonitor, enhancement, P3)

enhancement

Tracking

(firefox64 fixed)

RESOLVED FIXED
Firefox 64
Tracking Status
firefox64 --- fixed

People

(Reporter: gasolin, Assigned: amychan331, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug, Whiteboard: [netmonitorl])

User Story

See comment #26 for detailed description of what should be done in this report.

Honza

Attachments

(6 files, 2 obsolete files)

Currently the request list in Network Monitor has image thumbnails when this request is an image. Hover on that thumbnail will show the preview image in tooltip preview window.

The Network Monitor will get images and resize it as thumbnails after it received each request.

Chrome has thumbnails in list but does not have the tooltip preview window. (Firefox and Chrome both have preview tab in sidebar)

When this list item is an image, it will append the thumbnail in file column. For performance concern it will trigger the UI reflow.

Dev team want remove the image thumbnail and tooltip in the request list to
1. prevent separate quests to the backend when the request is received
2. avoid UI reflow
3. removed all tooltip on list
We also have some thought about the alternative approach:

* remove the thumbnail, put a file-type icon before each file, so we can keep the image preview tooltip
(no reflow, but still need 2 quests to backend)



Blake, is that UI change sounds fine from the UX side?
Flags: needinfo?(bwinton)
Flags: qe-verify+
Priority: -- → P1
Whiteboard: [netmonitorl]
It seems to me that the alternative wouldn't remove the tooltip, so it misses point three, and if we put a file-type icon, but replaced it with the image preview when it was available, we would get all the benefits of the current approach, but avoid the UI reflow…  What do you think of that approach?
Flags: needinfo?(bwinton)
Remove both thumbnail & tooltip could improve the performance most, because with that approach we don't need to fetch the image when we receive the request. So if UX does not have a strong opinion to keep the thumbnail and tooltip, we are planing to remove them.


Though we can have a placeholder and update when the image is ready, it will generate same amount of quests to the backend. But to avoid reflow, we need assign a file-type icon for each request item (or just a blank icon). It means we need not only image file-type icon, but (js, html, css), and un-recognized file-type icon (total 2~5+ icons).

FYI, Debugger.html already use the icons from https://vorillaz.github.io/devicons/#/dafont
Flags: needinfo?(bwinton)
But we're already fetching the image in the request so we shouldn't need a second request, and it seems more useful to have the preview than to go a little faster.  Have people been complaining about the performance of the network monitor, or is this part of a more generic "let's try to make stuff faster" initiative?  Is there any data showing that this is an important bottleneck, and that it would make people's lives easier to show less information?  It seems like it would be a false savings to not show the thumbnails in an effort to speed things up, just to force people to copy and paste and load every image url to see what images they were loading.

Maybe there's a lot of context I'm missing here, but I don't really see how this change makes things better for web developers.
Flags: needinfo?(bwinton)
Actually there's a second request in frontend.

Network -> (firefox) backend -> frontend get main request
frontend ask image content from backend
backend -> send image uri to frontend
frontend render the thumbnail


If this request item is an image, People can see the preview in the Response Header. 
Though I get your point.

If UX still prefer keep thumbnail and tooltip, to avoid the reflow, the quick way we could try is to show a blank icon before each request item and update the image if available. And we can update file-type images in later bug when the file-type image is available.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached image blank file type icon
In Bug 1349173 we keep the blank icon before each file and update the thumbnail if its an image.
(In reply to Blake Winton (:bwinton) (:☕️) from comment #4)
> Have people been complaining about the performance of the
> network monitor, or is this part of a more generic "let's try to make stuff
> faster" initiative?
I am not aware of any user feedback about the netmonitor's performance yet.
But we know for a fact that our most recent version is slower than what it used to be before we upgraded it to use React. It also seems (visually) to perform slower than Firebug did.
Our refactor of the netmonitor has not yet reached Firefox release, which is where we have the vast majority of our users. This explains why we have not received complaints yet.
We want to make sure we make it as fast as we can before it reaches release.
Fred, if you could share some data about the performance degradation, that would be helpful. Did we get DAMP talos regressions?
> Is there any data showing that this is an important
> bottleneck, and that it would make people's lives easier to show less
> information?
No we don't have data about this, and I suspect we will never have any. Removing a user feature is very likely to be perceived badly, unless there is an other benefit that user get as a result of removing this. This is not the case, we're just trying to make the netmonitor as fast as it was ... So removing this feature entirely would look bad.

(In reply to Fred Lin [:gasolin] from comment #5)
> If UX still prefer keep thumbnail and tooltip, to avoid the reflow, the
> quick way we could try is to show a blank icon before each request item and
> update the image if available. And we can update file-type images in later
> bug when the file-type image is available.
By all means, showing the thumbnail should not cause a reflow! Reflows are almost always of the complete document, so we want to avoid them as much as we can since they can be expensive. It seems like here there's an  easy way to do this by showing a placeholder image that gets the real background image as soon as it is available.
Thumbnails could be retrieved a tiny bit later, with a lower priority than the list of requests itself, in a throttled loop or something.
About tooltips: I don't think there's any cost to keep them, they're useful and can be initialized lazily anyway, on mouseover.
(In reply to Blake Winton (:bwinton) (:☕️) from comment #4)
> But we're already fetching the image in the request so we shouldn't need a
> second request, and it seems more useful to have the preview than to go a
> little faster.  Have people been complaining about the performance of the
> network monitor, or is this part of a more generic "let's try to make stuff
> faster" initiative?
Performance of the Net panel has always been an issue and there are several existing reports (done yet before we started converting it to HTML). The panel is used to debug Page Load Performance and if it's slowing things down all the timing data are incorrect.

Bug 1176050 - Firefox liveload with lots of connections slow when netmonitor is open
Bug 976823 - NCIX.com sale flyer page slows Firefox to a crawl with netmonitor open
Bug 956452 - Network monitor is janky on sites with a ton of requests

If we could avoid automatic fetching of HTTP response bodies data from the backend/server we can gain performance since we don't have to send so much packets and data over RDP (DevTools Remote Debugging Protocol) plus encode/decode binaries. In general, it would be great if this could happen on user request (fetch the data when the user is actually opening the side details panel + or hovering over request to get image tooltip).

Honza
Blank file type icon seems good to me.  For reducing the impact on page-fetching, what if we only showed the thumbnail after the user hovered over the line?  That way it wouldn't impact the page load speed, until the user was looking for that information…
damp test only monitor the first time/ending time. The damp test result is no correct before Janda fixed that after react request-list is landed. Though I think Janda has improved the first loading time and is at least similar to the XUL version.

Current issues we encounter are around the request list performance when there are lots of requests, which does not have accountable data yet.

We track the progress in Bug 1350969 - [meta] [Performance] New netmonitor can be slow when a lot of requests are happening in the same time
Julien already provide a sample test page, we profiling that page and doing several performance enhancements in parallel.
(In reply to Blake Winton (:bwinton) (:☕️) from comment #9)
> Blank file type icon seems good to me.  For reducing the impact on
> page-fetching, what if we only showed the thumbnail after the user hovered
> over the line?  That way it wouldn't impact the page load speed, until the
> user was looking for that information…
Yes, I like this.

Honza
Blake,

We're going to adopt your suggestion to use file type icon instead. Only one problem is that we don't have any existing file type icon in devtools assets folder [1]. Could you help us create an image type icon here?

At least the netmonitor can show an image type icon for image and blank icon for other types.

Thanks!

[1] http://searchfox.org/mozilla-central/source/devtools/client/themes/images
Flags: needinfo?(bwinton)
Attached image file-1.svg
Attached image file-2.svg (obsolete) —
Attached image file-2.svg
Attachment #8861873 - Attachment is obsolete: true
Attached image file-3.svg
Yeah, I'm _really_ not a visual designer, so I won't be able to create an image for you.  Maybe there's one somewhere else in the Firefox codebase you could use?  Alternatively, either file-1 or file-2 from :ntim would be just fine.  (I think file-3 is a little squashed-looking… ;)  Or just using an empty square is always a good option.
Flags: needinfo?(bwinton)
Ah, thanks for the suggestion Blake!

file-2 looks good to me. I'm in favor of using file-2 as our general file icon and then find another image type icon to replace current image thumbnail.
Keywords: good-first-bug
can I take this one?

> Blank file type icon seems good to me.  For reducing the impact on
> page-fetching, what if we only showed the thumbnail after the user hovered
> over the line?  That way it wouldn't impact the page load speed, until the
> user was looking for that information…

By what I read, it's fine to use file-2 as placeholder, remove thumbnail until user triggers "mouseover" to fetch image (for tooltip display) and after this, show the thumbnail, right?

anything I am missing?
Assignee: nobody → sy.fen0
We want to display file-2 when the thumbnail is not available.
Hi Victoria, I'd like to rise this bug to make sure the change does not conflict with your design.

Currently Network Monitor load contents per request and show the thumbnails if its an image.
We'd like to make requests showing faster on Network Monitor, it saves a lot of loading time if we don't load the image content at first time.

Blake suggests in comment 9 to show a placeholder icon first. While user mouse-hover the request item, fetch the image, update it in thumbnail and show it in the tooltip preview.
Flags: needinfo?(victoria)
Blocks: 1350969
See, also positive comment (related to removing the little preview thumbnail) from Harald here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1404917#c8

Honza
Hi Fred, Thanks for the bug summary! That sounds like a good solution to show the placeholder first, and fetch the image after hover.

One thing that could make this extra nice is to show the tiny Photon loading icon (the horizontally-bouncing ball) inside the placeholder after the users triggers mouseover/fetch (if we could load that in a way that wouldn't cost anything performance-wise).

Sounds like you're saying you'll use file-1 for css/js/html, and file-2 for images that haven't loaded yet? That looks good! (Later I may update the icons for the bolder photon styling, but I haven't even gotten to this for the main toolbar images yet.)
Flags: needinfo?(victoria)
Hi, is this bug open? I would like to work on this issue if possible. 

Tim from UofT
Flags: needinfo?(odvarko)
It seems that the thumbnail is completely removed in the latest version, and hovering over the link will show the picture. Should this be closed?
(In reply to Tim Xie from comment #24)
> Hi, is this bug open? I would like to work on this issue if possible. 
Yes, assigned to you.



(In reply to Tim Xie from comment #25)
> It seems that the thumbnail is completely removed in the latest version, and
> hovering over the link will show the picture.
Correct, but this report shouldn't be closed yet.

This is what we have to do

1) The File column should render a little icon in front of the file name. You can see the `blank file type icon` attachment showing how this looked before we removed the thumbnail.
2) File-2 icon should be used for images
3) File-1 icon should be used for everything else

Additionally (might be done as part of another report)
4) Show the tiny Photon loading icon (the horizontally-bouncing ball) inside the placeholder after the users triggers mouseover/fetch. See also comment #c23

Honza
Assignee: sy.fen0 → tim.xie
Flags: needinfo?(odvarko)
@Tim: are you still interested in this bug?
Honza
Flags: needinfo?(tim.xie)
Hey there guys! Wasn't sure if this one is still open or not? Looks like last comment or work was a few months ago?
Yes, this one is still open. But, nobody is currently working on it.
I updated the bug summary to reflect more what should be done here.

I think that Comment #26 nicely summarizes what should be done here.

Honza
Assignee: tim.xie → nobody
Mentor: odvarko
User Story: (updated)
Summary: [Performance] Remove image thumbnail and tooltip in request list → Render an icon in the File column indicating request type
(In reply to Jan Honza Odvarko [:Honza] from comment #29)
> Yes, this one is still open. But, nobody is currently working on it.
> I updated the bug summary to reflect more what should be done here.
> 
> I think that Comment #26 nicely summarizes what should be done here.
> 
> Honza

Ok great! I'll get to work on this. Just curious where I can start looking over the code at? I came to this bug with the assumption that it was withing the DevTool source code. Is that correct?
(In reply to JT Mozingo from comment #30)
> Ok great! I'll get to work on this. Just curious where I can start looking
> over the code at? I came to this bug with the assumption that it was withing
> the DevTool source code. Is that correct?
Yes, that's correct.

We are using React to build Network panel UI, and so you can start exploring how RequestListColumnFile component is implemented:

https://searchfox.org/mozilla-central/rev/c621276fbdd9591f52009042d959b9e19b66d49f/devtools/client/netmonitor/src/components/RequestListColumnFile.js#18

It's the `render` method responsible for rendering the column value for every request.

Honza
Also, please use 'Need more information from' field below, if you have any questions. It increases the chance that your question isn't lost in bugzilla mail ;-)

Assigned to you!

Thanks,
Honza
Assignee: nobody → jmozingo12
Status: REOPENED → ASSIGNED
Product: Firefox → DevTools
Assignee: jmozingo12 → nobody
Status: ASSIGNED → NEW
Hi Honza!
As this is a React issue, can I try it? I'll give you an update until Sunday, if I figure it out or not.
Thank you!
Hi! I noticed that Mellina is currently working on Bug 1340100. If she's no longer working on this, can I work on it?
(In reply to Amy Chan (:amychan331) from comment #34)
> Hi! I noticed that Mellina is currently working on Bug 1340100. If she's no
> longer working on this, can I work on it?

Yes, that's fine! Please let Honza know if you have any questions.
Assignee: nobody → amy_yyc
Hi all!
I got sick this weekend I had to halt all of my Mozilla's contributions :( I'll be glad if you could take this.
Added request type icons in File column of Network Monitor
Attachment #9014315 - Attachment description: added File column icons → Bug 1356536 - Add file type icons in network monitor. r=Honza
Got started with the first 3 points stated in comment #26. Got the icons in, and got the icon to toggle on mouse hover and mouse leave. Will be working on point 4 next, but to make sure the first 3 point is done correctly.
Status: NEW → ASSIGNED
I have moved the svg files, applied the isImage method (which is accessed via Filters.images and accepts objects that contains mimeType), and create the getFileName method. Hope they looks alright.
I have also been looking at the code related the tiny Photon loading icon mentioned in point 4. It seems to be related to the tab-throbber class in /browser/base/content/tabbrowser.*?  
Also, I was re-reading the comments about it - it sounds like the Photon icon replaces the file type icon when the mouse is hovering. Is it when the mouse hover over the icon or the row? And does the Photon icon does stay as long as user hover over the target?
Flags: needinfo?(odvarko)
Comments (In reply to Amy Chan (:amychan331) from comment #39)
> I have moved the svg files, applied the isImage method (which is accessed
> via Filters.images and accepts objects that contains mimeType), and create
> the getFileName method. Hope they looks alright.
Looks good.

> I have also been looking at the code related the tiny Photon loading icon
> mentioned in point 4. It seems to be related to the tab-throbber class in
> /browser/base/content/tabbrowser.*?  
> Also, I was re-reading the comments about it - it sounds like the Photon
> icon replaces the file type icon when the mouse is hovering. Is it when the
> mouse hover over the icon or the row? And does the Photon icon does stay as
> long as user hover over the target?
Please file a new bug for this, so it isn't blocking the work we have done so far and we can land it.

Quick link to file a bug:
https://bugzilla.mozilla.org/enter_bug.cgi?product=DevTools&component=Netmonitor

Honza
Flags: needinfo?(odvarko)
The new bug have been filed at https://bugzilla.mozilla.org/show_bug.cgi?id=1498018.
While checking out the changes I made, I notice that the icons don't always changes to image icons when it should. I realized that this.props.item.mimeType used by Filters.image() is not always called. So in the latest push, I added a check for cause.type, which is still called with value "img" even if mimeType is not called.
I am seeing some test failures in the try push.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ffc8c474766c22c9118d29b350d33d1e5ddcf73

Can you please investigate it?

Here is how you can run a test locally:

./mach test devtools/client/responsive.html/test/browser/browser_toolbox_rule_view_reload.js

I any of the failing tests failing with your patch on your machine?

You might also use:

./mach test devtools/client/responsive.html/test/browser/browser_toolbox_rule_view_reload.js --verify --headless

To run the same test many times without UI (without browser window being opened).

Honza
Flags: needinfo?(tim.xie) → needinfo?(amy_yyc)
Running test locally didn't cause any error, but I was reading the treeherder failure logs, and the errors occur when the variable cause is undefined in line 36 in RequestListColumnFile.js ("if (Filters.images(this.props.item) || (cause && cause.type == "img"))"). If I change that line to "cause && cause.type == "img"", it should fix that error.
Out of curiosity, I checked what happens if I remove Filters.images(mimeType). The code runs without it, so I have removed it for now.
Flags: needinfo?(amy_yyc)
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4fa33210cbc4
Add file type icons in network monitor. r=Honza
https://hg.mozilla.org/mozilla-central/rev/4fa33210cbc4
Status: ASSIGNED → RESOLVED
Closed: 3 years agoLast year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
I managed to reproduce the issue using an older version of Nightly from 2017-04-14 on Windows 10 x64.
I retested everything on latest Nightly 64.0a1 using Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.13 and the issue it's not reproducing anymore. Every entry has either file-2 icon or file-1 icon.

However, I am not sure which one is for which files. In comment 23 and comment 26 it says that file-2 are for images and file-1 is for other types (html, css, js etc.). But if you go to https://en.wikipedia.org/wiki/Main_Page (or even on https://9gag.com), you can see that some of the images have the file-1 icon. Per comment 23 that means that the image is not loaded, but I don't think that's the case. Please look at the attached image to see what I am trying to say. Is this expected behaviour?
Amy, can you please look at the previous comment?

Honza
Flags: needinfo?(amy_yyc)
 if you go to
> https://en.wikipedia.org/wiki/Main_Page (or even on https://9gag.com), you
> can see that some of the images have the file-1 icon. Per comment 23 that
> means that the image is not loaded, but I don't think that's the case.
> Please look at the attached image to see what I am trying to say. Is this
> expected behaviour?

 I feel the need to clarify what I wrote here.

Some of the images have file-1 icon on wikipedia.com and some have file-2 icon. 
In comment 23 it says that the images that have file-2 icon means that they are not loaded, but that doesn't seem to be the case. And I think that is in contradiction with comment 26 where it's says that images should have file-2 icon.
Please don't hesitate to contact me if there is something that you don't understand.
Attached file Add more ways to detect image file (obsolete) —
Just opened a new patch to solved this since my last one was close. It seems what happened was that it was checking for cause === img, but image files can also have a cause type of imageset and beacon, so I changed the if statement to check for cause type that include img, image, and beacon. 

As for Oana's question about file-2 icon for images that are not loaded, when I was reading Victoria's comment 23, I thought she meant that user hasn't hover over the link to load the image yet, so Honza ended up just saying that image files should have file-2 in comment 26. Can Victoria please clarify?
Flags: needinfo?(amy_yyc) → needinfo?(victoria)
(In reply to Amy Chan (:amychan331) from comment #50)
> Just opened a new patch to solved this since my last one was close. It seems
> what happened was that it was checking for cause === img, but image files
> can also have a cause type of imageset and beacon, so I changed the if
> statement to check for cause type that include img, image, and beacon. 
> 
> As for Oana's question about file-2 icon for images that are not loaded,
> when I was reading Victoria's comment 23, I thought she meant that user
> hasn't hover over the link to load the image yet, so Honza ended up just
> saying that image files should have file-2 in comment 26. Can Victoria
> please clarify?

Thanks for working on this Amy!

Since this bug is already closed, can you please file a follow up and move your new patch there?
I'll do the review in the folow-up.

Please, also link the new bug here.

Thanks!
Honza
Flags: needinfo?(amy_yyc)
(In reply to Amy Chan (:amychan331) from comment #50)
> when I was reading Victoria's comment 23, I thought she meant that user
> hasn't hover over the link to load the image yet, so Honza ended up just
> saying that image files should have file-2 in comment 26. Can Victoria
> please clarify?
Taking this question.

This is correct assumption. Support for loading should be done in a follow up.

Honza
Flags: needinfo?(victoria)
Attachment #9020318 - Attachment is obsolete: true
Blocks: 1502702
Done! New bug posted at https://bugzilla.mozilla.org/show_bug.cgi?id=1502702. When you said link the new bug here, does that mean I should put this bug in the Depends on section of the new bug?
Flags: needinfo?(amy_yyc)
(In reply to Amy Chan (:amychan331) from comment #53)
> Done! New bug posted at
> https://bugzilla.mozilla.org/show_bug.cgi?id=1502702. When you said link the
Great thanks!

> new bug here, does that mean I should put this bug in the Depends on section
> of the new bug?

Adding  bug 1502702 int the Blocks field in this bug.

Honza
I will track this issue in bug 1502702, so I will take out the qe-verify flag.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.