gasolin@mozilla.com Reporter
	Description • 7 years ago

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

	gasolin@mozilla.com Reporter
	Comment 1 • 7 years ago

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?

	Blake Winton (:bwinton) (:☕️)
	Comment 2 • 7 years ago

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?

	gasolin@mozilla.com Reporter
	Comment 3 • 7 years ago

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

	Blake Winton (:bwinton) (:☕️)
	Comment 4 • 7 years ago

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.

	gasolin@mozilla.com Reporter
	Comment 5 • 7 years ago

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.

	gasolin@mozilla.com Reporter
	Comment 6 • 7 years ago

In Bug 1349173 we keep the blank icon before each file and update the thumbnail if its an image.

	Patrick Brosset <:pbro>
	Comment 7 • 7 years ago

(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.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 8 • 7 years ago

(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

	Blake Winton (:bwinton) (:☕️)
	Comment 9 • 7 years ago

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…

	gasolin@mozilla.com Reporter
	Comment 10 • 7 years ago

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.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 11 • 7 years ago

(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

	Ricky Chien [:rickychien] (inactive)
	Comment 12 • 7 years ago

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

	Blake Winton (:bwinton) (:☕️)
	Comment 17 • 7 years ago

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.

	Ricky Chien [:rickychien] (inactive)
	Comment 18 • 7 years ago

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.

	Stefan Yohansson
	Comment 19 • 7 years ago

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?

	Tim Nguyen :ntim
	Comment 20 • 7 years ago

We want to display file-2 when the thumbnail is not available.

	gasolin@mozilla.com Reporter
	Comment 21 • 7 years ago

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.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 22 • 7 years ago

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

	Victoria Wang [:victoria]
	Comment 23 • 7 years ago

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.)

	Tim Xie
	Comment 24 • 7 years ago

Hi, is this bug open? I would like to work on this issue if possible. 

Tim from UofT

	Tim Xie
	Comment 25 • 7 years ago

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?

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 26 • 7 years ago

(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

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 27 • 6 years ago

@Tim: are you still interested in this bug?
Honza

	JT Mozingo
	Comment 28 • 6 years ago

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?

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 29 • 6 years 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

	JT Mozingo
	Comment 30 • 6 years ago

(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?

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 31 • 6 years ago

(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

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 32 • 6 years ago

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

	Mellina Y.
	Comment 33 • 6 years ago

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!

	Amy Chan (:amychan331) Assignee
	Comment 34 • 6 years ago

Hi! I noticed that Mellina is currently working on Bug 1340100. If she's no longer working on this, can I work on it?

	Tim Nguyen :ntim
	Comment 35 • 6 years ago

(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.

	Mellina Y.
	Comment 36 • 6 years ago

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.

	Amy Chan (:amychan331) Assignee
	Comment 37 • 6 years ago

Added request type icons in File column of Network Monitor

	Amy Chan (:amychan331) Assignee
	Comment 38 • 6 years ago

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.

	Amy Chan (:amychan331) Assignee
	Comment 39 • 6 years ago

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?

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 40 • 6 years ago

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

	Amy Chan (:amychan331) Assignee
	Comment 41 • 6 years ago

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.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 42 • 6 years ago

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

	Amy Chan (:amychan331) Assignee
	Comment 43 • 6 years ago

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.

	Pulsebot
	Comment 44 • 6 years ago

Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4fa33210cbc4
Add file type icons in network monitor. r=Honza

	Cosmin Sabou [:CosminS]
	Comment 45 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/4fa33210cbc4

	Oana Botisan, Desktop QA
	Comment 46 • 6 years ago

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?

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 47 • 6 years ago

Amy, can you please look at the previous comment?

Honza

	Oana Botisan, Desktop QA
	Comment 48 • 6 years ago

 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.

	Amy Chan (:amychan331) Assignee
	Comment 50 • 6 years ago

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?

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 51 • 6 years ago

(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

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 52 • 6 years ago

(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

	Amy Chan (:amychan331) Assignee
	Comment 53 • 6 years ago

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?

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 54 • 6 years ago

(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

	Oana Botisan, Desktop QA
	Comment 55 • 6 years ago

I will track this issue in bug 1502702, so I will take out the qe-verify flag.