859051 - There's no caching information available in the monitor

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P3)

Description

[Victor Porof [:vporof][:vp]]

Cached requests (304) should be marked out differently in the requests menu, and more detailed caching information should be shown somewhere in the details pane. For example, firebug shows something like this:

Data Size       20035
Device          disk
Expires         Sat Apr 05 2014 19:44:49 GMT+0300 (EEST)
Fetch Count     4
Last Fetched    Sun Apr 07 2013 12:45:31 GMT+0300 (EEST)
Last Modified   Sun Apr 07 2013 12:45:31 GMT+0300 (EEST)

Comment 1

[Victor Porof [:vporof][:vp]]

Moving into Developer Tools: Netmonitor component. Filter on NETMONITORAMA.

Comment 2

[Sebastian Zartner [:sebo]]

FWIW, Firebug showed this informatino within the 'Cache' tab of an expanded request.

Sebastian

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: cache.png

Firebug's Net panel (cache tab) screenshot.

Honza

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Here is how Firebug fetches cache data from the platform:
https://github.com/firebug/firebug/blob/master/extension/content/firebug/net/netCacheReader.js#L218

Honza

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Some instructions for the implementation:

1) There should be a new panel "Cache" implemented alongside other panels like, Headers, Params, etc.

2) Here is implementation of existing panels. Implementation of the new Cache panel will be very similar
* https://dxr.mozilla.org/mozilla-central/rev/1e0e193b0812f68a12fbd69198552af62347af1e/devtools/client/netmonitor/shared/components/params-panel.js#37
* https://dxr.mozilla.org/mozilla-central/rev/1e0e193b0812f68a12fbd69198552af62347af1e/devtools/client/netmonitor/shared/components/headers-panel.js#42

3) The new Cache panel should also utilize `PropertiesView` just like other panels. It should simplify the work.

4) The Cache panel doesn't probably need filter input box so, the `enableInput` property (for the PropertiesView component) should be set to false.

5) The cache data should be fetched from the platform using CacheStorageService (introduced in Firefox 32). See Firebug's implementation as an example:
https://github.com/firebug/firebug/blob/master/extension/content/firebug/net/netCacheReader.js#L32

6) Fetching the data should happen on the backend. See e.g. how response content is fetched: https://dxr.mozilla.org/mozilla-central/rev/1e0e193b0812f68a12fbd69198552af62347af1e/devtools/server/actors/webconsole.js#2076. Back compatibility should be preserved so, client should be able to work with backend that doesn't support this new feature (ie fetching cache data)

Here is how the Client fetches the response content
https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/netmonitor-controller.js?q=this.webConsoleClient.getResponseContent%28actor%2C&redirect_type=single#618

---

Btw. Since the backend implementation is involved in this bug, I am removing good-first-bug flag. The patch doesn't have to be so simple.

Honza

Comment 6

[David McCurry]

Could I hop on this as my first bug?

It looks like the flag was removed, but I should be able to handle - feel free to ping me if you'd like more information.  Thanks!

Comment 7

[[e-mail]]

David, welcome to pick up any bugs you want to contribute!

As Comment 5 said, this bug involves some backend change, so this bug is not that simple.
If you are new to network monitor's code base, it's better to get a start on some smaller scale bugs.

Comment 8

[David McCurry]

Sounds good.  

I'm new to the code base, but this seems like a good way to dive in and get to know it.  Comment 5 seems to provide me with enough instruction, so I'd like to try it out.  It seems like something I can definitely handle.  (plus I like "not that simple" :))

Comment 9

[David McCurry]

Attachment: patch-add-cachepanel

Comment 10

[David McCurry]

Just added a patch for the initial implementation (no tests yet).  Could you see if I'm on the right track?

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to David McCurry from comment #10)
> Just added a patch for the initial implementation (no tests yet).  Could you
> see if I'm on the right track?
Thanks for working on this!
Sure I'll look.

Note that you can't set the review field to + by yourself. You need to set it to ? and wait for review. Consequently it's up to the reviewer if he says ok/not ok i.e. +/-

Also, there is no commit message in the patch. It should be something like as follows:

Bug 859051 - Implement cache panel; r=Honza

You might see MDN:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Landing_a_patch
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

Honza

Comment 12

[David McCurry]

Attachment: Bug859051.patch

Sorry!  Adding properly formatted patch (I hope).

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8951598 [details] [diff] [review]
Bug859051.patch

Review of attachment 8951598 [details] [diff] [review]:
-----------------------------------------------------------------

This is great, thanks for working on this!

First round of comments sent.

Also:
1) I am not seeing the tab for 304 responses (screenshot comming)
2) The way how cache info is rendered within the panel should be the same as in Security tab (see the screenshot)

Honza

::: devtools/client/locales/en-US/netmonitor.properties
@@ +1044,5 @@
> +expires = Expires
> +fetchCount = Fetch Count
> +lastFetched = Last Fetched
> +lastModified = Last Modified
> +device = Device

Please add LOCALIZATION_NOTE for every string.

You should also use a prefix "netmonitor.cache"

netmonitor.cache.emptyText
netmonitor.cache.dataSize
netmonitor.cache.expires
etc.

::: devtools/client/netmonitor/src/components/CachePanel.js
@@ +5,5 @@
> + "use strict";
> +
> + const { Component, createFactory } = require("devtools/client/shared/vendor/react");
> + const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
> + const { connect } = require("devtools/client/shared/vendor/react-redux");

connect isn't use anywhere in this module, you can remove it.

@@ +8,5 @@
> + const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
> + const { connect } = require("devtools/client/shared/vendor/react-redux");
> + const dom = require("devtools/client/shared/vendor/react-dom-factories");
> + const { L10N } = require("../utils/l10n");
> + const { fetchCacheDescriptor, fetchNetworkUpdatePacket } = require("../utils/request-utils");

fetchCacheDescriptor is not used

@@ +9,5 @@
> + const { connect } = require("devtools/client/shared/vendor/react-redux");
> + const dom = require("devtools/client/shared/vendor/react-dom-factories");
> + const { L10N } = require("../utils/l10n");
> + const { fetchCacheDescriptor, fetchNetworkUpdatePacket } = require("../utils/request-utils");
> + const { sortObjectKeys } = require("../utils/sort-utils");

sortObjectKeys is not used

@@ +10,5 @@
> + const dom = require("devtools/client/shared/vendor/react-dom-factories");
> + const { L10N } = require("../utils/l10n");
> + const { fetchCacheDescriptor, fetchNetworkUpdatePacket } = require("../utils/request-utils");
> + const { sortObjectKeys } = require("../utils/sort-utils");
> + const Actions = require("../actions/index");

Actions is not used

@@ +17,5 @@
> + const PropertiesView = createFactory(require("./PropertiesView"));
> + 
> + const { div, input } = dom;
> +
> + const CACHE_EMPTY_TEXT = L10N.getStr("cacheEmptyText");

CACHE_EMPTY_TEXT is not used, is itsupposed to be used?

@@ +31,5 @@
> +   LAST_MODIFIED,
> +   FETCH_COUNT,
> +   EXPIRES,
> +   DEVICE,
> + ];

Please add one empty line

@@ +33,5 @@
> +   EXPIRES,
> +   DEVICE,
> + ];
> + /*
> +  * Cache panel component

The comment structure should be (two stars at the beginning):

/**
 *
 */

@@ +44,5 @@
> +       openLink: PropTypes.func,
> +       request: PropTypes.object.isRequired,
> +     };
> +   }
> + 

Remove the space char on the empty line

@@ +49,5 @@
> +   componentDidMount() {
> +     let { connector, request } = this.props;
> +     fetchNetworkUpdatePacket(connector.requestData, request, ["responseCache"]);
> +   }
> + 

Remove space on the empty line

@@ +60,5 @@
> +     return (
> +       div({ className: "tabpanel-summary-container cache-summary"},
> +        div({
> +          className: "tabpanel-summary-label cache-summary-label",
> +        }, label + ':'),

Use quotes for strings. ie ":"

@@ +71,5 @@
> +     );
> +   }
> +
> +   getProperties(responseCache) {
> +    let responseCacheObj;

Fix indentation

@@ +85,5 @@
> +       cacheObj = Object.assign({}, responseCacheObj);
> +     } catch (e) {
> +        return null;
> +     }
> +   

Remove whitespaces

@@ +92,5 @@
> +
> +  getDate(timestamp) {
> +    let d = new Date(parseInt(timestamp) * 1000);
> +
> +    return d.toLocaleDateString() + ' ' + d.toLocaleTimeString();

Use quotes for strings

@@ +102,5 @@
> +       openLink,
> +     } = this.props;
> +     let { responseCache } = request;
> +     
> +

Remove whitespaces and one empty line

@@ +110,5 @@
> +     let summaryFetchCount = object.fetchCount ? this.renderSummary(FETCH_COUNT, object.fetchCount): null;
> +     let summaryDataSize = object.dataSize ? this.renderSummary(DATA_SIZE, object.dataSize): null;
> +     let summaryLastModified = object.lastModified ? this.renderSummary(LAST_MODIFIED, this.getDate(object.lastModified)): null;
> +     let summaryExpires = object.expires ? this.renderSummary(EXPIRES, this.getDate(object.expires)): null;
> +     let summaryDevice = object.device ? this.renderSummary(DEVICE, object.device) : null;

Lines are too long. Maximum length of a line is 90 chars.

::: devtools/client/netmonitor/src/components/TabboxPanel.js
@@ +87,5 @@
>          title: RESPONSE_TITLE,
>        },
>          ResponsePanel({ request, openLink, connector }),
>        ),
> +      request.fromCache && 

Remove whitespaces at the end of the line

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js
@@ +574,5 @@
> +  
> +  /**
> +   * 
> +   * @param {object} response 
> +   */

Looks like unfinished comment

::: devtools/server/actors/webconsole.js
@@ +2156,5 @@
>    },
>  
>    /**
> +   * The "getResponseCache" packet type handler.
> +   * 

Remove whitespaces at the end of the line

::: devtools/shared/webconsole/client.js
@@ +551,5 @@
>    },
>  
>    /**
> +   * Retrieve the response cache from the given NetworkEventActor
> +   * 

Remove whitespaces at the end of the line

::: devtools/shared/webconsole/network-helper.js
@@ +505,5 @@
>      }
>    },
> +  /**
> +   * Gets a cache entry for a request.
> +   * 

Remove whitespaces at the end of the line

@@ +517,5 @@
> +    if (CacheStorageService) {
> +      let cacheService = CacheStorageService.getService(Ci.nsICacheStorageService);
> +      let loadContext = Services.loadContextInfo.default;
> +      let cacheSession = cacheService.diskCacheStorage(loadContext, false);
> +      cacheSession.asyncOpenURI(this.nsIURL(request.URI.spec), "", Ci.nsICacheStorage.OPEN_NORMALLY, 

Remove whitespaces at the end of the line

@@ +538,5 @@
> +            onCacheDescriptorAvailable(cdescriptor);
> +          } else {
> +            onCacheDescriptorAvailable(null);
> +          }
> +          

Remove whitespaces

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: cache-panel.png

Comment 15

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I just noticed that the bug # in commit message is wrong:
Bug 85901 - Implement cache panel; r=Honza

Should be:
Bug 859051 - Implement cache panel; r=Honza

Honza

Comment 16

[David McCurry]

Attachment: Bug859051.patch

Adding updated patch - all code review comments should be addressed (view is updated, and 304 responses should now show the cache panel.)

Comment 17

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Can you please re-upload the patch?
The Splinter review is broken (I don't see content of the files).
Also it needs to be rebased on top of m-c

Thanks!
Honza

Comment 18

[Jan Honza Odvarko [:Honza] (always need-info? me)]

It looks like the patch is not generated in usual format.
You can check whether your hgrc file has:

[diff]
git = 1
showfunc = 1
unified = 8


Honza

Comment 19

[David McCurry]

Attachment: Bug859051.patch

Fixing patch.

Comment 20

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8955570 [details] [diff] [review]
Bug859051.patch

Review of attachment 8955570 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working on this, it's great new feature!

There are many issues related to the coding standards
(indentation, white-spaces, new lines, etc.)

You might use eslint to catch all these issues before
submitting a patch:
https://wiki.mozilla.org/DevTools/CodingStandards

Please fix all eslint problems and I'll continue
with the review.

Honza

::: devtools/client/locales/en-US/netmonitor.properties
@@ +1045,5 @@
>  netmonitor.label.dropHarFiles = Drop HAR files here
> +
> +# LOCALIZATION NOTE (netmonitor.cache.cache): This is the label text for the parent
> +# node in the TreeView.
> +netmonitor.cache.cache = Cache

There shouldn't be white-spaces around the `=` character

Please fix all occurrences of this in this file.

::: devtools/client/netmonitor/src/components/CachePanel.js
@@ +7,5 @@
> + const { Component, createFactory } = require("devtools/client/shared/vendor/react");
> + const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
> + const dom = require("devtools/client/shared/vendor/react-dom-factories");
> + const { L10N } = require("../utils/l10n");
> + const { fetchNetworkUpdatePacket } = require("../utils/request-utils");

Remove white-spaces at the beginning of the line

@@ +8,5 @@
> + const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
> + const dom = require("devtools/client/shared/vendor/react-dom-factories");
> + const { L10N } = require("../utils/l10n");
> + const { fetchNetworkUpdatePacket } = require("../utils/request-utils");
> + 

Remove trailing white-spaces

@@ +12,5 @@
> + 
> + // Components
> +const TreeViewClass = require("devtools/client/shared/components/tree/TreeView");
> +const PropertiesView = createFactory(require("./PropertiesView"));
> + 

Remove trailing white-spaces

@@ +23,5 @@
> +const LAST_FETCHED = L10N.getStr("netmonitor.cache.lastFetched");
> +const LAST_MODIFIED = L10N.getStr("netmonitor.cache.lastModified");
> +const DEVICE = L10N.getStr("netmonitor.cache.device");
> +const NOT_AVAILABLE = L10N.getStr("netmonitor.cache.notAvailable");
> +const EMPTY = L10N.getStr("netmonitor.cache.empty");

Insert one empty line here

@@ +28,5 @@
> +/**
> + * Cache panel component
> + * This tab lists full details of any cache information of the response.
> + */
> +class CachePanel extends Component {

Please fix indentation of the entire file

@@ +80,5 @@
> +    return cacheObj;
> +  }
> +
> +  getDate(timestamp) {
> +    if (!timestamp) return null;

Body of the `if` statement needs to be on separate line and enclosed by `{}`

@@ +83,5 @@
> +  getDate(timestamp) {
> +    if (!timestamp) return null;
> +    let d = new Date(parseInt(timestamp) * 1000);
> +    return d.toLocaleDateString() + " " + d.toLocaleTimeString();
> +  }

Methods are separated by an empty line

@@ +95,5 @@
> +    let object;
> +    let cache = this.getProperties(responseCache);
> +
> +    if (cache.lastFetched || cache.fetchCount || cache.dataSize
> +        || cache.lastModified | cache.expires || cache.device) {    

Remove trailing spaces

@@ +99,5 @@
> +        || cache.lastModified | cache.expires || cache.device) {    
> +      object = {
> +        [CACHE]: {
> +          [LAST_FETCHED]: this.getDate(cache.lastFetched) || NOT_AVAILABLE,
> +          [FETCH_COUNT]: cache.fetchCount || NOT_AVALIABLE,

TYPO: NOT_AVALIABLE -> NOT_AVAILABLE

@@ +122,5 @@
> +      })
> +    );
> +   }
> +}
> +module.exports = CachePanel;
\ No newline at end of file

New line required at the end of the file

::: devtools/client/netmonitor/src/constants.js
@@ +95,5 @@
>  
>    // When stack-trace finishes receiving.
>    RECEIVED_EVENT_STACKTRACE: "NetMonitor:NetworkEventUpdated:StackTrace",
>  
> +  UPDATING_RESPONSE_CACHE: "NetMonitor:NetwworkEventUpdating:ResponseCache",

Typo: NetwworkEventUpdating -> NetworkEventUpdating

::: devtools/server/actors/webconsole.js
@@ +2160,5 @@
> +   *
> +   * @return object
> +   *         The cache packet - network cache information.
> +   */
> +  onGetResponseCache: function() {

There must be space between `function` and `()`
(the same for addResponseCache)

@@ +2434,5 @@
> +
> +    this.conn.send(packet);
> +  },
> +
> +  addResponseCache: function(content) {

`addResponseCache` is defined twice

::: devtools/shared/webconsole/client.js
@@ +159,5 @@
>          };
>          networkInfo.response.bodySize = packet.contentSize;
>          networkInfo.response.transferredSize = packet.transferredSize;
>          networkInfo.discardResponseBody = packet.discardResponseBody;
> +

Remove empty line

@@ +555,5 @@
> +   *
> +   * @param string actor
> +   *        The NetworkEventActor ID.
> +   * @param function onResponse
> +   *        The function invoked when the response is recieved.

Typo: recieved -> received

@@ +557,5 @@
> +   *        The NetworkEventActor ID.
> +   * @param function onResponse
> +   *        The function invoked when the response is recieved.
> +   * @return request
> +   *         Request object that implements both Promise and EventEmitter interfaces.

Fix indentation

::: devtools/shared/webconsole/network-helper.js
@@ +519,5 @@
> +  cacheStorageSession: null,
> +  /**
> +   * Initializes our cache session / cache storage session.
> +   */
> +  initializeCacheSession: function() {

Space between `function` and `()`
(please fix all occurrences in the patch)

@@ +526,5 @@
> +
> +    if (CacheStorageService) {
> +      let cacheService = CacheStorageService.getService(Ci.nsICacheStorageService);
> +      let loadContext = Services.loadContextInfo.default;
> +      NetworkHelper.cacheStorageSession = cacheService.diskCacheStorage(loadContext, false);

Line too long (max 90 chars for one line)
Please fix all occurrences in the patch)

@@ +533,5 @@
> +    }
> +
> +    if (CacheService) {
> +      let oldICache = Ci["ns" + "ICache"];
> +      let cacheService = CacheService.getService(Ci.nsICacheService);

It is not recommended to use nsICacheService - it's old API. It will be removed (bug 913828).

You can also see:
https://developer.mozilla.org/en-US/docs/Mozilla/HTTP_cache

The patch should use nsICacheStorageService only

Honza

@@ +540,5 @@
> +    }
> +  },
> +  /**
> +   * Parses a cache descriptor returned from the backend into a
> +   * useable object.

Typo: useable -> usable

@@ +585,5 @@
> +      NetworkHelper.initializeCacheSession();
> +    }
> +    if (NetworkHelper.cacheStorageSession) {
> +      NetworkHelper.cacheStorageSession.asyncOpenURI(uri, "", Ci.nsICacheStorage.OPEN_NORMALLY, {
> +        onCacheEntryCheck: function(entry, appcache) {

Please use arrow functions for these callbacks 

() => {}

(and again the line is too long)

@@ +589,5 @@
> +        onCacheEntryCheck: function(entry, appcache) {
> +          return Ci.nsICacheEntryOpenCallback.ENTRY_WANTED;
> +        }, onCacheEntryAvailable: function(descriptor, isnew, appacache, status) {
> +          if (descriptor) {
> +            let cdescriptor = NetworkHelper.parseCacheDescriptor(descriptor);;

Double semicolon

::: devtools/shared/webconsole/network-monitor.js
@@ +582,5 @@
>    },
>  
> +  _fetchCacheInformation: function() {
> +    let httpActivity = this.httpActivity;
> +    NetworkHelper.getCacheEntry(this.httpActivity.url, function(descriptor) {

Please use arrow function for the callback

@@ +1916,5 @@
>  (function () {
>    // Listeners for new network event data coming from the NetworkMonitor.
>    let methods = ["addRequestHeaders", "addRequestCookies", "addRequestPostData",
>                   "addResponseStart", "addSecurityInfo", "addResponseHeaders",
> +                 "addResponseCookies", "addResponseContent", "addResponseCache", "addEventTimings"];

The line is too long

Comment 21

[David McCurry]

Attachment: Bug859051.patch

Thanks for your patience with this!

I've updated my lint settings, and think everything is addressed now - I also removed the deprecated CacheService calls.

Comment 22

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8956052 [details] [diff] [review]
Bug859051.patch

Review of attachment 8956052 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, the code looks a lot better now!

A few more inline comments.

Honza

::: devtools/client/netmonitor/src/components/CachePanel.js
@@ +125,5 @@
> +        openLink,
> +      })
> +    );
> +   }
> + }

nit: insert an empty line here

::: devtools/shared/webconsole/network-helper.js
@@ +506,5 @@
>    },
>  
>    /**
> +   * Flag to indicate if our session is created.
> +   */

nit: insert new line here

@@ +510,5 @@
> +   */
> +  isCacheSessionInitialized: false,
> +  /**
> +   * Cache storage session.
> +   */

nit: insert new line here

@@ +514,5 @@
> +   */
> +  cacheStorageSession: null,
> +  /**
> +   * Initializes our cache session / cache storage session.
> +   */

nit: insert new line here

@@ +525,5 @@
> +      NetworkHelper.cacheStorageSession =
> +        cacheService.diskCacheStorage(loadContext, false);
> +      NetworkHelper.isCacheSessionInitialized = true;
> +    }
> +  },

nit: insert new line here

@@ +569,5 @@
> +   *        callback function.
> +   */
> +  fetchCacheDescriptor: function (uri, onCacheDescriptorAvailable) {
> +    if (!NetworkHelper.isCacheSessionInitialized) {
> +      NetworkHelper.initializeCacheSession();

This code is within `NetworkHelper`, so you can use `this.isCacheSessionInitialized`.
Please fix also the other places in this method and in `initializeCacheSession` method.

Comment 23

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8956052 [details] [diff] [review]
Bug859051.patch

@jryans: can you please look at the backend part?

Honza

Comment 24

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Nicolas: this patch is adding a new data-type packet for fetching cache information. This means that there is one more network update packet data type sent and the following place needs to be fixed:

https://searchfox.org/mozilla-central/rev/d2b4b40901c15614fad2fa34718eea428774306e/devtools/client/webconsole/new-console-output/new-console-output-wrapper.js#283

Number of updates should be 9 now:
const NUMBER_OF_NETWORK_UPDATE = 9;

Is this the only place we need to fix?

Honza

Comment 25

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Jan Honza Odvarko [:Honza] from comment #24)
> @Nicolas: this patch is adding a new data-type packet for fetching cache
> information. This means that there is one more network update packet data
> type sent and the following place needs to be fixed:
> 
> https://searchfox.org/mozilla-central/rev/
> d2b4b40901c15614fad2fa34718eea428774306e/devtools/client/webconsole/new-
> console-output/new-console-output-wrapper.js#283
> 
> Number of updates should be 9 now:
> const NUMBER_OF_NETWORK_UPDATE = 9;
> 
> Is this the only place we need to fix?
> 

I looked around and I think that's the only place where we do listen for a specific number of requests in the console

Comment 26

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8956052 [details] [diff] [review]
Bug859051.patch

Review of attachment 8956052 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working on this! :)

Overall, I think there are a few things we'll need to adjust with the backend portion.  Let me know if more details are needed.

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js
@@ +230,5 @@
>  
> +  async fetchResponseCache(responseCache) {
> +    let payload = {};
> +    if (responseCache) {
> +      payload.responseCache = await this.getLongString(responseCache);

It doesn't seem like the cache data is being sent as a long string, so this doesn't seem like what you want here.  Honza should have a better idea about this.

As for backward compatibility with old servers...  old server won't send `ResponseCache` events.  Is that okay with code as written?  I am assuming we wait to hear about the cache being available before requesting.  Are there places where we expect a cache event to always arrive?  If so, we'll likely need to add a trait[1] to the server so we only do this for new servers where this support exists.

See more notes about this at http://docs.firefox-dev.tools/backend/backward-compatibility.html.

[1]: https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/devtools/server/actors/webconsole.js#88

::: devtools/shared/webconsole/network-helper.js
@@ +515,5 @@
> +  cacheStorageSession: null,
> +  /**
> +   * Initializes our cache session / cache storage session.
> +   */
> +  initializeCacheSession: function () {

I think we might want to move this new code to its own module, instead of using `NetworkHelper`.  The existing `NetworkHelper` methods are all stateless, static functions that are independent of each other.  This new code is a set a properties and functions that work together.  Also, `NetworkHelper` exports all of its properties, but it's not clear we want that for this new code.

So, it seems like a new module would be better here.

@@ +522,5 @@
> +    if (CacheStorageService) {
> +      let cacheService = Services.cache2;
> +      let loadContext = Services.loadContextInfo.default;
> +      NetworkHelper.cacheStorageSession =
> +        cacheService.diskCacheStorage(loadContext, false);

I don't think this part is quite right.  The default load context isn't correct for every request.  As an example, if you use the "containers" Firefox feature[1], then requests made in tabs set to a non-default container will have a different load context.

So, I think we need to use the load context from the request.  It looks like `NetworkHelper.getRequestLoadContext` will be useful for this.

[1]: https://addons.mozilla.org/en-US/firefox/addon/multi-account-containers/

Comment 27

[David McCurry]

Yeah, this makes sense.  I'm going to move this out into a separate module, and I'll fix the loadContext issue.

Thanks!

Comment 28

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@David, how is the Cache panel work coming along?

This is really great feature and it would be sad if it's forgotten.
Let me know if there is anything I can help with to move this forward.

Honza

Comment 29

[David McCurry]

(In reply to Jan Honza Odvarko [:Honza] from comment #28)
> @David, how is the Cache panel work coming along?
> 
> This is really great feature and it would be sad if it's forgotten.
> Let me know if there is anything I can help with to move this forward.
> 
> Honza

Good timing - it's going well, and just trying to knock out a few last things I had on my list.  I'll have a patch tomorrow morning for review.  Thanks!

David

Comment 30

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Excellent!

Honza

Comment 31

[David McCurry]

Attachment: Bug859051.patch

I've moved cache-entry into its own component now.

Per the review, I've also removed `getLongString` from the statement fetching cache from the backend.

In the case of the backend not supporting responseCache, I've tested by removing the `this.conn.send(packet)` call for responseCache, and the system works (it just displays "No Cache Information").

Comment 32

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8961735 [details] [diff] [review]
Bug859051.patch

Review of attachment 8961735 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the update!

Please see my inline comments.

Couple more comments:
1) I noticed that `fetchCount` is increased by 2 (not 1) after executing one cached request.
Is that expected? Could DT code-base cause the additional +1?

2) There is new networkUpdate type and we should also update:
const NUMBER_OF_NETWORK_UPDATE = 9;

See also my comment: https://bugzilla.mozilla.org/show_bug.cgi?id=859051#c24

Increasing the number should only happen when we know that the server
side is supporting this new packet type and I think that we need to
add a trait to the server. Just like described by Ryan in comment #26:
https://bugzilla.mozilla.org/show_bug.cgi?id=859051#c26

We also need to ask Nicolas to review this part.

I think this is that last part we need to solve.

Honza

::: devtools/shared/cache-entry.js
@@ +108,5 @@
> +         }
> +       );
> +     }
> +   }
> + };

Thanks for moving this into separate module.
I think that this module should be located in netmonitor/src/utils directory.

::: devtools/shared/webconsole/client.js
@@ +558,5 @@
> +   *        The function invoked when the response is received.
> +   * @return request
> +   *         Request object that implements both Promise and EventEmitter interfaces.
> +   */
> +  getResponseCache: function (actor, onResponse) {

nit: remove the space between `function` and `(`

::: devtools/shared/webconsole/network-monitor.js
@@ +578,5 @@
>      this.httpActivity.owner.addResponseHeaders(openResponse.headers);
>      this.httpActivity.owner.addResponseCookies(openResponse.cookies);
>    },
>  
> +  _fetchCacheInformation: function () {

nit: remove the space between `function` and `(`

Comment 33

[Jan Honza Odvarko [:Honza] (always need-info? me)]

There are also some test failures. See this try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=27248cbd5d94f0883ccaf329376be6db866dde9b&selectedJob=170280094

Honza

Comment 34

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@David, anything I can help with?

Honza

Comment 35

[David McCurry]

@Honza - no issues yet.  I'm going to submit a patch with the updates soon, just making sure it's all working.

Comment 36

[David McCurry]

(In reply to David McCurry from comment #35)
> @Honza - no issues yet.  I'm going to submit a patch with the updates soon,
> just making sure it's all working.

Oh, and thanks for checking!

Comment 37

[David McCurry]

Attachment: Bug859051.patch

This is updated now - I moved cache-entry to netmonitor/src/utils, added a trait (and a check for it).

Hopefully this fixes the failing test - I'm still looking into the issue with the 2 fetchCount registered.

Comment 38

[David McCurry]

(In reply to David McCurry from comment #37)
> Created attachment 8966558 [details] [diff] [review]
> Bug859051.patch
> 
> This is updated now - I moved cache-entry to netmonitor/src/utils, added a
> trait (and a check for it).
> 
> Hopefully this fixes the failing test - I'm still looking into the issue
> with the 2 fetchCount registered.

Looks like _fetchCacheDescriptor is getting called too often.  I'm working on a fix.

Comment 39

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to David McCurry from comment #38)
> (In reply to David McCurry from comment #37)
> > Created attachment 8966558 [details] [diff] [review]
> > Bug859051.patch
> > 
> > This is updated now - I moved cache-entry to netmonitor/src/utils, added a
> > trait (and a check for it).
> > 
> > Hopefully this fixes the failing test - I'm still looking into the issue
> > with the 2 fetchCount registered.
> 
> Looks like _fetchCacheDescriptor is getting called too often.  I'm working
> on a fix.
The following test is failing:

// This test makes sure that the style editor does not store any content CSS
// files in the permanent cache when opened from PB mode.
devtools/client/styleeditor/test/browser_styleeditor_private_perwindowpb.js
(if I remove `_fetchCacheInformation` from `onStreamClose` it's passing again)

I am also seeing:
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/devtools/client/netmonitor/src/utils/cache-entry.js:9:19 | require(chrome) is not allowed (mozilla/reject-some-requires)

Moving the cache-entry.js file into the following dir should fix the problem.
devtools/shared/platform/

Honza

Comment 40

[David McCurry]

Attachment: Bug859051.patch

I wrapped _fetchCacheInformation in a check for request.fromCache || status == 304.  This should fix the failing test.

Also moved the cache-entry into shared/platform. 

Thanks!

Comment 41

[David McCurry]

Attachment: Bug859051.patch

Fixing comment that was not correct.

Comment 42

[David McCurry]

Attachment: Bug859051.patch

Rebased for changes in TabboxPanel.js

Comment 43

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8967371 [details] [diff] [review]
Bug859051.patch

Review of attachment 8967371 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to David McCurry from comment #40)
> Created attachment 8967362 [details] [diff] [review]
> Bug859051.patch
> 
> I wrapped _fetchCacheInformation in a check for request.fromCache || status
> == 304.  This should fix the failing test.
> 
> Also moved the cache-entry into shared/platform. 
Great, the failing test and eslint error are both fixed, thanks!

I pushed to try again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=25fcd6687aec23692fc6866be41d74a9d6406043&selectedJob=173286958

And there are two failing tests (both on timeout):
devtools/client/webconsole/test/fixtures/stub-generators/browser_webconsole_check_stubs_network_event.js
devtools/client/webconsole/test/mochitest/browser_webconsole_network_messages_status_code.js

I think that the problem (for both failures) is in this method:
https://searchfox.org/mozilla-central/rev/9f3da81290054c5b8955bb67ff98cae66676f745/devtools/client/webconsole/new-console-output-wrapper.js#326

expectedLength - is set to 10 since both requestPostData and responseCache update packets are expected, but the `responseCache` is never sent => 'batchedMessageUpdates' never executed => `network-message-updated` event never fired => tests never finished.

Is `responseCache` always sent? What if the descriptor is empty and addResponseCache not called...?

Honza

Comment 44

[David McCurry]

Attachment: Bug859051.patch

Should fix failing test - added another check before incrementing number of network packets.

Comment 45

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to David McCurry from comment #44)
> Created attachment 8970152 [details] [diff] [review]
> Bug859051.patch
> 
> Should fix failing test - added another check before incrementing number of
> network packets.
The Cache panel is always empty for me now, saying: "No cache information"
Can you see that issue?

Honza

Comment 46

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@David: I found the problem I mentioned in comment #45, see the following patch:

--- a/devtools/client/webconsole/new-console-output-wrapper.js
+++ b/devtools/client/webconsole/new-console-output-wrapper.js
@@ -335,17 +335,17 @@ NewConsoleOutputWrapper.prototype = {
     // that networkInfo.updates has all we need.
     // Note that 'requestPostData' is sent only for POST requests, so we need
     // to count with that.
     // 'fetchCacheDescriptor' will also cause a network update and increment
     // the number of networkInfo.updates
     const NUMBER_OF_NETWORK_UPDATE = 8;

     let expectedLength = NUMBER_OF_NETWORK_UPDATE;
-    if (this.jsterm.hud.proxy.webConsoleClient.traits.fetchCacheDescriptor
+    if (this.hud.proxy.webConsoleClient.traits.fetchCacheDescriptor
       && res.networkInfo.updates.includes("responseCache")) {
       expectedLength++;
     }
     if (res.networkInfo.updates.includes("requestPostData")) {
       expectedLength++;
     }

     if (res.networkInfo.updates.length === expectedLength) {


I also pushed to Try and it looks good!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=97072d9da6d44f599b4879e8ae4371fb7cf799b2

I believe that this is ready to land.

Please update your patch and I'll R+

(there is also one minor easy-to-fix conflict in network-monitor.js, so you need to rebase)

Great job here!

Honza

Comment 47

[David McCurry]

Attachment: Bug859051.patch

Thanks so much!

Fix is attached in latest patch.

Comment 48

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8972280 [details] [diff] [review]
Bug859051.patch

Review of attachment 8972280 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

R+

Honza

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c56e7428cbf9
Implement cache panel; r=Honza

Comment 50

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/c56e7428cbf9

Comment 51

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Release Note Request (optional, but appreciated)
[Why is this notable]: There is a new UI in DevTools Network panel
[Affects Firefox for Android]: no
[Suggested wording]: The Network panel has a new side panel with cache information
[Links (documentation, blog post, etc)]:

Comment 52

[Pascal Chevrel:pascalc (PTO until April 26)]

(In reply to Jan Honza Odvarko [:Honza] from comment #51)
> Release Note Request (optional, but appreciated)
> [Why is this notable]: There is a new UI in DevTools Network panel
> [Affects Firefox for Android]: no
> [Suggested wording]: The Network panel has a new side panel with cache
> information
> [Links (documentation, blog post, etc)]:

Jan, do you mean that you want this mentioned in Firefox Release Notes (https://www.mozilla.org/en-US/firefox/58.0/releasenotes/) or did you mean that you want it mentioned in the Developer Notes for the release on MDN (https://developer.mozilla.org/en-US/Firefox/Releases/58#Changes_for_Web_developers)?

If the later, then you don't need to nominate it for release notes (see https://wiki.mozilla.org/Release_Management/Release_Notes#Nomination_in_Bugzilla).

Comment 53

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Pascal Chevrel:pascalc from comment #52)
> If the later, then you don't need to nominate it for release notes (see
> https://wiki.mozilla.org/Release_Management/
> Release_Notes#Nomination_in_Bugzilla).
I see, thanks for the comment.
I think that mentioning this feature in Developer Notes is enough.
Honza

Comment 54

[Pascal Chevrel:pascalc (PTO until April 26)]

(In reply to Jan Honza Odvarko [:Honza] from comment #53)
> (In reply to Pascal Chevrel:pascalc from comment #52)
> > If the later, then you don't need to nominate it for release notes (see
> > https://wiki.mozilla.org/Release_Management/
> > Release_Notes#Nomination_in_Bugzilla).
> I see, thanks for the comment.
> I think that mentioning this feature in Developer Notes is enough.
> Honza

That's what I thought, thanks. 
Future 61 MDN page: https://developer.mozilla.org/en-US/Firefox/Releases/61

Comment 55

[Chris Mills [:cmills]]

I've documented this by adding a section about the Cache tab to the net monitor page:
https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor#Cache

And a note to the Fx61 rel notes:
https://developer.mozilla.org/en-US/Firefox/Releases/61#Developer_tools

Let me know if that looks OK. There is a comment about a bit I am not sure about at the former URL.

Thanks!

Comment 56

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #55)
> I've documented this by adding a section about the Cache tab to the net
> monitor page:
> https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor#Cache
> 
> And a note to the Fx61 rel notes:
> https://developer.mozilla.org/en-US/Firefox/Releases/61#Developer_tools
> 
> Let me know if that looks OK. There is a comment about a bit I am not sure
> about at the former URL.

Thanks for working on this!

> Device: The ... [NOT SURE ABOUT THIS ONE]

The 'Device' field is related to `deviceID` prop in `nsICacheEntryInfo` interface
The doc says: Get the client id associated with this cache entry.
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsICacheEntryInfo

You might say something like: The device where the resource was fetched from (e.g. "disk").

Honza

Comment 57

[Chris Mills [:cmills]]

(In reply to Jan Honza Odvarko [:Honza] from comment #56)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #55)
> > Device: The ... [NOT SURE ABOUT THIS ONE]
> 
> The 'Device' field is related to `deviceID` prop in `nsICacheEntryInfo`
> interface
> The doc says: Get the client id associated with this cache entry.
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> Interface/nsICacheEntryInfo
> 
> You might say something like: The device where the resource was fetched from
> (e.g. "disk").
> 
> Honza

Ah, perfect, thanks.