If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

304 Not Modified - Make it clear if request comes from the browser cache

VERIFIED FIXED in Firefox 55

Status

()

Firefox
Developer Tools: Netmonitor
P1
normal
VERIFIED FIXED
8 months ago
6 months ago

People

(Reporter: karlcow, Assigned: Vincent Lequertier, Mentored)

Tracking

(Blocks: 1 bug, {good-first-bug})

53 Branch
Firefox 55
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox55 verified)

Details

(Whiteboard: [netmonitor-reserve])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

(Reporter)

Description

8 months ago
Steps to reproduce:

1. Go to a site with HTTP Caching
2. Reload and check a resource which would be a 304 Not Modified.
3. Ctrl + Click on the 304 resource and do "copy response"

Actual:
* We get a payload.
* The response tab has the HTTP payload.

Expected:
* Empty Payload.
* The response tab is empty

a 304 Not Modified when done correctly is not sending a body at all. 

I understand the rationale of providing something useful for the users to play with. That said the Network panel is about the HTTP transactions. And this doesn't help the user to understand if he/she has done the right thing.

Maybe in terms of layout there's a way to convey that the body is empty and that the payload is coming from the cache. :)
Thanks for the report!

(In reply to Karl Dubost :karlcow from comment #0)
> Steps to reproduce:
> 
> 1. Go to a site with HTTP Caching
> 2. Reload and check a resource which would be a 304 Not Modified.
> 3. Ctrl + Click on the 304 resource and do "copy response"
> 
> Actual:
> * We get a payload.
> * The response tab has the HTTP payload.
> 
> Expected:
> * Empty Payload.
> * The response tab is empty
> 
> a 304 Not Modified when done correctly is not sending a body at all. 
> 
> I understand the rationale of providing something useful for the users to
> play with.
Yes, I think that "Copy Response to Clipboard" should always work regardless on the cache.

> That said the Network panel is about the HTTP transactions. And
> this doesn't help the user to understand if he/she has done the right thing.
... and I agree with this.

> Maybe in terms of layout there's a way to convey that the body is empty and
> that the payload is coming from the cache. :)
Yes, this sounds as the bug in the UI.

I am thinking about the following (to make it clear that the response body comes from disk-cache and not from the server over the wire):

1) There are two columns (in the Net panel): 'Size' (size of the response body) and Transferred (amount of bytes, perhaps compressed, sent over the wire). The 'Transferred' column should be set to 0 in case the response-body comes from disk-cache. The columns value could be set to: "(from disk-cache)" (no quotes)

2) Requests (bodies) coming from disk-cache can be rendered using gray color. Firebug did this and it worked nicely in my opinion. See attached screenshot.

What do you think?

Honza
Whiteboard: [netmonitor][triage]
Created attachment 8835950 [details]
firebug-net-panel.png
(Reporter)

Comment 3

7 months ago
+1 to all these suggestions. 
Yep that would certainly improve the understanding. :)
* (from disk-cache) 

Updated

7 months ago
Blocks: 1307743
Flags: qe-verify?

Updated

7 months ago
Flags: qe-verify? → qe-verify+
QA Contact: ciprian.georgiu

Updated

7 months ago
Priority: -- → P3
Whiteboard: [netmonitor][triage] → [netmonitor-reserve]
Some instructions for anyone who's interested in this bug:

1)
Mentor: odvarko@gmail.com
Keywords: good-first-bug
Summary: 304 Not Modified - Copy Response and Response tab → 304 Not Modified - Make it clear if request comes from the browser cache
Some instructions for anyone who's interested in this bug:

1) The Transferred column value is rendered using `TransferredColumn` component here:
https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/devtools/client/netmonitor/components/request-list-item.js#330

2) The entire requests row is rendered using `RequestListItem`:
https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/devtools/client/netmonitor/components/request-list-item.js#122

Honza
(Assignee)

Comment 6

7 months ago
Hello, can I work on this?

Vincent
(In reply to vi.le from comment #6)
> Hello, can I work on this?
Sure, assigned to you!


:-)

Honza
Assignee: nobody → vi.le
Status: NEW → ASSIGNED
(Assignee)

Comment 8

7 months ago
Thanks
Let me know if you have any questions :-)

Honza
(Assignee)

Comment 10

7 months ago
I've started to play with the code on the first part (modifications on the 'Transfered' column).

There is already some work to display informations if the request comes from the cache here: 
https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/devtools/client/netmonitor/components/request-list-item.js#340-342

Should we behave the same way if the HTTP code is 304? This is what I'm doing.
Should the variable 'fromCache' set to true if the status is 304?

Regards,

Vincent
(Reporter)

Comment 11

7 months ago
This is a good question.

When the status is 304. There was a communication between the client and the server.

It's a conditional request. The client sent a request with specific headers such as If-None-Match or If-Modified-Since and the server is replying in case the resource has not been modified with HTTP headers but the body is empty. So the response from the server is really meaningful. The server is telling to the client use the cached copy.

And maybe it's here that it hits something from my first comment. In the response tab if we put a body, we are basically not showing what's really happening at the HTTP level. Imagine for example, your are designing an app on the server and you are implementing 304 Not Modified, but you forgot that you should NOT sent the HTTP body.

So 3 cases:

1. URI -> HTTP Request    -> 200 OK with response body and headers (or any code such as 404, etc.)
2. URI -> No HTTP Request -> Directly from the cache (age < max-age)
3. URI -> HTTP Request    -> 304 Not Modified without response body 
                             (response headers only), body from the cache


(thinking)
(wondering what Jan is thinking)
Created attachment 8839197 [details]
netpanel.png

I am attaching a screenshot to show what I have in mind.

* Requests coming from cache are grayed out (opacity: 0.5)
* File name is bold for non-cached requests
* Individual request items has bottom border (no zebra-backgrounds any more)

> So 3 cases:
> 1. URI -> HTTP Request    -> 200 OK with response body and headers (or any code such as 404, etc.)
> 2. URI -> No HTTP Request -> Directly from the cache (age < max-age)
> 3. URI -> HTTP Request    -> 304 Not Modified without response body 
                             (response headers only), body from the cache
There are also:

4. Requests coming from BFCache [1] (similar to #2, since these requests are directly coming from BFCache).
5. Partial responses (part of the response comes from the cache and the rest from the server.


I am also going to attach Firebug screenshot that shows how requests coming from BFCache are rendered.

I think that we can (in this bug report) focus only on distinguishing requests coming from the server and from the cache.

Honza



[1] http://www.softwareishard.com/blog/firebug/firebug-tip-what-the-heck-is-bfcache/
Created attachment 8839199 [details]
bfcache.png

Requests coming from BFCache have special background.

Honza

Comment 14

7 months ago
(In reply to Jan Honza Odvarko [:Honza] from comment #12)
> Created attachment 8839197 [details]
> netpanel.png
> 
> I am attaching a screenshot to show what I have in mind.
> 
> * Requests coming from cache are grayed out (opacity: 0.5)
> * File name is bold for non-cached requests
It's unclear why bold should be used. It implies those requests have a special status, which they don't. I would prefer making cached requests italic (in addition to graying them out).

> * Individual request items has bottom border (no zebra-backgrounds any more)
We should consider consistency internally: all our tools (storage, memory, perf) use a zebra striping. It would feel strange if the netmonitor moved away from it.
(Assignee)

Comment 15

7 months ago
Thank you for the helpful comments!
Some update on what I've done:

- If the request comes from the cache or is 304 Not Modified, the transfered column is set to 'cached'.

- If the request comes from the cache or is 304 Not Modified, it is grayed out (with opacity 0.6, I have found 0.5 to be a bit unreadable)

So we separate the 2nd and 3rd cases from the 1st case mentioned above. I will create a patch in the next days.
Comment hidden (mozreview-request)
@Vincent: can you please rebase on top of m-c, thanks!

Honza

patching file devtools/client/netmonitor/components/request-list-content.js
Hunk #1 FAILED at 209
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/netmonitor/components/request-list-content.js.rej
patching file devtools/client/netmonitor/components/request-list-item.js
Hunk #1 FAILED at 53
Hunk #2 FAILED at 98
Hunk #3 FAILED at 323
3 out of 3 hunks FAILED -- saving rejects to file devtools/client/netmonitor/components/request-list-item.js.rej
patching file devtools/client/themes/netmonitor.css
Hunk #1 FAILED at 603
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/themes/netmonitor.css.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh cache
Comment hidden (mozreview-request)
(Assignee)

Comment 19

7 months ago
(In reply to Jan Honza Odvarko [:Honza] from comment #17)
> @Vincent: can you please rebase on top of m-c, thanks!
> 
> Honza
> 
> patching file devtools/client/netmonitor/components/request-list-content.js
> Hunk #1 FAILED at 209
> 1 out of 1 hunks FAILED -- saving rejects to file
> devtools/client/netmonitor/components/request-list-content.js.rej
> patching file devtools/client/netmonitor/components/request-list-item.js
> Hunk #1 FAILED at 53
> Hunk #2 FAILED at 98
> Hunk #3 FAILED at 323
> 3 out of 3 hunks FAILED -- saving rejects to file
> devtools/client/netmonitor/components/request-list-item.js.rej
> patching file devtools/client/themes/netmonitor.css
> Hunk #1 FAILED at 603
> 1 out of 1 hunks FAILED -- saving rejects to file
> devtools/client/themes/netmonitor.css.rej
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working directory
> errors during apply, please fix and qrefresh cache

Should be good now

Comment 20

7 months ago
mozreview-review
Comment on attachment 8841486 [details]
Bug 1338386 - Make it clear if request comes from the browser cache;

https://reviewboard.mozilla.org/r/115708/#review117836

Looks great!

One thing. The opacity is set for the entire row including vertical time markers indicating 'DOMContentLoad' and 'load' events. The vertical line shouldn't use opacity.

See the attached screenshot.

Honza
Attachment #8841486 - Flags: review?(odvarko)
Created attachment 8842416 [details]
timeline.png
(In reply to Tim Nguyen :ntim from comment #14)
> (In reply to Jan Honza Odvarko [:Honza] from comment #12)
> > Created attachment 8839197 [details]
> > netpanel.png
> > 
> > I am attaching a screenshot to show what I have in mind.
> > 
> > * Requests coming from cache are grayed out (opacity: 0.5)
> > * File name is bold for non-cached requests
> It's unclear why bold should be used. It implies those requests have a
> special status, which they don't. I would prefer making cached requests
> italic (in addition to graying them out).
Agree, let's stick with the grey color.

> > * Individual request items has bottom border (no zebra-backgrounds any more)
> We should consider consistency internally: all our tools (storage, memory,
> perf) use a zebra striping. It would feel strange if the netmonitor moved
> away from it.
Good point, agree. Let's keep zebra-bk in place.

Honza

Updated

7 months ago
See Also: → bug 983884

Comment 23

7 months ago
mozreview-review
Comment on attachment 8841486 [details]
Bug 1338386 - Make it clear if request comes from the browser cache;

https://reviewboard.mozilla.org/r/115708/#review118564

::: devtools/client/netmonitor/components/request-list-content.js:247
(Diff revision 2)
>          tabIndex: 0,
>          onKeyDown: this.onKeyDown,
>        },
>          displayedRequests.map((item, index) => RequestListItem({
>            firstRequestStartedMillis,
> +          fromCache: item.status == 304 || item.fromCache,

please use `===` instead of `==`

::: devtools/client/netmonitor/components/request-list-item.js:397
(Diff revision 2)
>    render() {
> -    const { transferredSize, fromCache, fromServiceWorker } = this.props.item;
> +    const { transferredSize, fromCache, fromServiceWorker, status } = this.props.item;
>  
>      let text;
>      let className = "subitem-label";
> -    if (fromCache) {
> +    if (fromCache || status == 304) {

nit: use `===` instead of `==`
Comment hidden (mozreview-request)
(Assignee)

Comment 25

7 months ago
I've updated the patch. I haven't found a way to set the opacity of the line to 0.6 while setting the opacity of the vertical markers to 1. I've modified the CSS rule to set the opacity to the subitem elements, except for the column with the timings.
(Assignee)

Comment 26

7 months ago
(In reply to Fred Lin [:gasolin] from comment #23)
> Comment on attachment 8841486 [details]
> Bug 1338386 - Set the opacity on the subitems classes, except
> .requests-list-waterfall
> 
> https://reviewboard.mozilla.org/r/115708/#review118564
> 
> ::: devtools/client/netmonitor/components/request-list-content.js:247
> (Diff revision 2)
> >          tabIndex: 0,
> >          onKeyDown: this.onKeyDown,
> >        },
> >          displayedRequests.map((item, index) => RequestListItem({
> >            firstRequestStartedMillis,
> > +          fromCache: item.status == 304 || item.fromCache,
> 
> please use `===` instead of `==`
> 
> ::: devtools/client/netmonitor/components/request-list-item.js:397
> (Diff revision 2)
> >    render() {
> > -    const { transferredSize, fromCache, fromServiceWorker } = this.props.item;
> > +    const { transferredSize, fromCache, fromServiceWorker, status } = this.props.item;
> >  
> >      let text;
> >      let className = "subitem-label";
> > -    if (fromCache) {
> > +    if (fromCache || status == 304) {
> 
> nit: use `===` instead of `==`

Using '===' instead of '==' does not seem to work well. If I do so, the conditions 'item.status === 304' and 'status == 304' are always false.

Comment 27

7 months ago
If `===` not work, it means we was not matching with the right status code type.

The possible approach is try to compare with `status === "304"` which treat "304" as string. Or try to print `typeof status` to make sure we do use the right type.
Comment hidden (mozreview-request)
(Assignee)

Comment 29

7 months ago
(In reply to Fred Lin [:gasolin] from comment #27)
> If `===` not work, it means we was not matching with the right status code
> type.
> 
> The possible approach is try to compare with `status === "304"` which treat
> "304" as string. Or try to print `typeof status` to make sure we do use the
> right type.

Thanks

Comment 30

7 months ago
mozreview-review
Comment on attachment 8841486 [details]
Bug 1338386 - Make it clear if request comes from the browser cache;

https://reviewboard.mozilla.org/r/115708/#review119204

LGTM!

R+ assuming try is green.

Honza
Attachment #8841486 - Flags: review?(odvarko) → review+
Try push looks good
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4fcd02928f1d023b8dab913ff6c286ef4e91754


Honza
Keywords: checkin-needed

Comment 32

7 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/166cfeeeab65
Make it clear if request comes from the browser cache; r=Honza
Keywords: checkin-needed

Comment 33

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/166cfeeeab65
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

7 months ago
Iteration: --- → 55.1 - Mar 20
Priority: P3 → P1
(In reply to Karl Dubost :karlcow from comment #0)
> Steps to reproduce:
...
> 3. Ctrl + Click on the 304 resource and do "copy response"
...
> Expected:
> * Empty Payload.
> * The response tab is empty

I'm not seeing with latest Nightly 55.0a1 (2017-03-14) the expected result mentioned above. The Response tab is not empty, after I click "Copy response" from context menu on a 304 Not modified request. 

What do you think, am I missing something here, Vincent?
Flags: needinfo?(vi.le)
(Assignee)

Comment 35

6 months ago
(In reply to Ciprian Georgiu, QA [:ciprian_georgiu] from comment #34)
> (In reply to Karl Dubost :karlcow from comment #0)
> > Steps to reproduce:
> ...
> > 3. Ctrl + Click on the 304 resource and do "copy response"
> ...
> > Expected:
> > * Empty Payload.
> > * The response tab is empty
> 
> I'm not seeing with latest Nightly 55.0a1 (2017-03-14) the expected result
> mentioned above. The Response tab is not empty, after I click "Copy
> response" from context menu on a 304 Not modified request. 
> 
> What do you think, am I missing something here, Vincent?

We decided to not change the behavior of the response tab (and of the copy response feature) because it would confuse users. From the comment 1 :

"Yes, I think that "Copy Response to Clipboard" should always work regardless on the cache."

We stated that it would be confusing if in the netmonitor panel, for some requests you can have the payload, and for some requests you can't. Being able to see what the browser got (from cache or not) for each request is convenient. So instead, we decided to provide a graphical indicator (when the cache was used for request, the line is greyed out) that shows if the cache was used for a request.

Do not hesitate if you have other questions :-)

Vincent
Flags: needinfo?(vi.le)
(In reply to Vincent Lequertier from comment #35)
...
> We stated that it would be confusing if in the netmonitor panel, for some
> requests you can have the payload, and for some requests you can't. Being
> able to see what the browser got (from cache or not) for each request is
> convenient. So instead, we decided to provide a graphical indicator (when
> the cache was used for request, the line is greyed out) that shows if the
> cache was used for a request.

Thanks for the clarification! 

Marking this as verified fixed on latest Nightly 55.0a1 (2017-03-16), using Windows 10 x64, Mac OS X 10.11.6 and Ubuntu 16.04 x64 LTS.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.