Closed Bug 1338386 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools :: Netmonitor, defect, P1)

Product:
DevTools
Component:
Netmonitor
Version:
53 Branch
Type:
defect

Tracking

(firefox55 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 55
Iteration:
55.1 - Mar 20
Tracking Flags:
Tracking Status
firefox55 --- verified

People

(Reporter: karlcow, Assigned: vincent, Mentored)

References

(Blocks 1 open bug)

Details

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

Attachments

(5 files)

firebug-net-panel.png
30.29 KB, image/png
Details
netpanel.png
40.81 KB, image/png
Details
bfcache.png
58.55 KB, image/png
Details
Bug 1338386 - Make it clear if request comes from the browser cache;
59 bytes, text/x-review-board-request
Honza
: review+
Details
timeline.png
32.79 KB, image/png
Details 
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]
+1 to all these suggestions. 
Yep that would certainly improve the understanding. :)
* (from disk-cache)
Blocks: netmonitor-html
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
QA Contact: ciprian.georgiu
Priority: -- → P3
Whiteboard: [netmonitor][triage] → [netmonitor-reserve]
Some instructions for anyone who's interested in this bug:

1)
Mentor: odvarko
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
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
Thanks
Let me know if you have any questions :-)

Honza
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
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)
Attached image 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/
Attached image bfcache.png — 
Requests coming from BFCache have special background.

Honza
(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.
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.
@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
(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 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)
(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
See Also: → 983884
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 `==`
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.
(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.
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.
(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 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+
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
https://hg.mozilla.org/mozilla-central/rev/166cfeeeab65
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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)
(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+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: