There's no caching information available in the monitor

RESOLVED FIXED in Firefox 61

Status

defect
P3
normal
RESOLVED FIXED
6 years ago
10 months ago

People

(Reporter: vporof, Assigned: dmccurry, Mentored)

Tracking

(Blocks 2 bugs, {dev-doc-complete})

unspecified
Firefox 61
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(3 attachments, 11 obsolete attachments)

(Reporter)

Description

6 years ago
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)
(Reporter)

Comment 1

6 years ago
Moving into Developer Tools: Netmonitor component. Filter on NETMONITORAMA.
Component: Developer Tools → Developer Tools: Netmonitor
Summary: [netmonitor] There's no caching information available in the monitor → There's no caching information available in the monitor
(Reporter)

Updated

6 years ago
Priority: -- → P3
FWIW, Firebug showed this informatino within the 'Cache' tab of an expanded request.

Sebastian
Blocks: firebug-gaps
Keywords: dev-doc-needed
Posted image cache.png
Firebug's Net panel (cache tab) screenshot.

Honza
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
Keywords: good-first-bug
(Assignee)

Comment 6

a year ago
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!
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.
Flags: needinfo?(dmccurry)
(Assignee)

Comment 8

a year ago
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" :))
Flags: needinfo?(dmccurry)
(Assignee)

Comment 9

a year ago
Posted patch patch-add-cachepanel (obsolete) — Splinter Review
Attachment #8951331 - Flags: review+
(Assignee)

Comment 10

a year ago
Just added a patch for the initial implementation (no tests yet).  Could you see if I'm on the right track?
Flags: needinfo?(odvarko)
(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
Flags: needinfo?(odvarko)
Attachment #8951331 - Flags: review+ → review?(odvarko)
(Assignee)

Comment 12

a year ago
Posted patch Bug859051.patch (obsolete) — Splinter Review
Sorry!  Adding properly formatted patch (I hope).
Attachment #8951331 - Attachment is obsolete: true
Attachment #8951331 - Flags: review?(odvarko)
Attachment #8951598 - Flags: review?(odvarko)
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
Attachment #8951598 - Flags: review?(odvarko)
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
(Assignee)

Comment 16

a year ago
Posted patch Bug859051.patch (obsolete) — Splinter Review
Adding updated patch - all code review comments should be addressed (view is updated, and 304 responses should now show the cache panel.)
Attachment #8951598 - Attachment is obsolete: true
Attachment #8955125 - Flags: review?(odvarko)
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
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
(Assignee)

Comment 19

a year ago
Posted patch Bug859051.patch (obsolete) — Splinter Review
Fixing patch.
Attachment #8955125 - Attachment is obsolete: true
Attachment #8955125 - Flags: review?(odvarko)
Attachment #8955570 - Flags: review?(odvarko)
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
Attachment #8955570 - Flags: review?(odvarko)
(Assignee)

Comment 21

a year ago
Posted patch Bug859051.patch (obsolete) — Splinter Review
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.
Attachment #8955570 - Attachment is obsolete: true
Attachment #8956052 - Flags: review?(odvarko)
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.
Attachment #8956052 - Flags: review?(odvarko)
Comment on attachment 8956052 [details] [diff] [review]
Bug859051.patch

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

Honza
Attachment #8956052 - Flags: review?(jryans)
@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
Flags: needinfo?(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
Flags: needinfo?(nchevobbe)
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/
Attachment #8956052 - Flags: review?(jryans) → review-
(Assignee)

Comment 27

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

Thanks!
Assignee: nobody → dmccurry
Status: NEW → ASSIGNED
@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
Flags: needinfo?(dmccurry)
(Assignee)

Comment 29

a year ago
(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
Flags: needinfo?(dmccurry)
(Assignee)

Comment 31

a year ago
Posted patch Bug859051.patch (obsolete) — Splinter Review
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").
Attachment #8956052 - Attachment is obsolete: true
Attachment #8961735 - Flags: review?(odvarko)
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 `(`
Attachment #8961735 - Flags: review?(odvarko)
@David, anything I can help with?

Honza
Flags: needinfo?(dmccurry)
(Assignee)

Comment 35

a year ago
@Honza - no issues yet.  I'm going to submit a patch with the updates soon, just making sure it's all working.
Flags: needinfo?(dmccurry)
(Assignee)

Comment 36

a year ago
(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!
(Assignee)

Comment 37

a year ago
Posted patch Bug859051.patch (obsolete) — Splinter Review
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.
Attachment #8961735 - Attachment is obsolete: true
Attachment #8966558 - Flags: review?(odvarko)
(Assignee)

Comment 38

a year ago
(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.
(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
(Assignee)

Comment 40

a year ago
Posted patch Bug859051.patch (obsolete) — Splinter Review
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!
Attachment #8966558 - Attachment is obsolete: true
Attachment #8966558 - Flags: review?(odvarko)
Attachment #8967362 - Flags: review?(odvarko)
(Assignee)

Comment 41

a year ago
Posted patch Bug859051.patch (obsolete) — Splinter Review
Fixing comment that was not correct.
Attachment #8967362 - Attachment is obsolete: true
Attachment #8967362 - Flags: review?(odvarko)
Attachment #8967369 - Flags: review?(odvarko)
(Assignee)

Comment 42

a year ago
Posted patch Bug859051.patch (obsolete) — Splinter Review
Rebased for changes in TabboxPanel.js
Attachment #8967369 - Attachment is obsolete: true
Attachment #8967369 - Flags: review?(odvarko)
Attachment #8967371 - Flags: review?(odvarko)
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
Attachment #8967371 - Flags: review?(odvarko)
(Assignee)

Comment 44

a year ago
Posted patch Bug859051.patch (obsolete) — Splinter Review
Should fix failing test - added another check before incrementing number of network packets.
Attachment #8967371 - Attachment is obsolete: true
Attachment #8970152 - Flags: review?(odvarko)
(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
Flags: needinfo?(dmccurry)
@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
(Assignee)

Comment 47

a year ago
Thanks so much!

Fix is attached in latest patch.
Attachment #8970152 - Attachment is obsolete: true
Attachment #8970152 - Flags: review?(odvarko)
Flags: needinfo?(dmccurry)
Attachment #8972280 - Flags: review?(odvarko)
Comment on attachment 8972280 [details] [diff] [review]
Bug859051.patch

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

Thanks!

R+

Honza
Attachment #8972280 - Flags: review?(odvarko) → review+

Comment 50

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c56e7428cbf9
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
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)]:
relnote-firefox: --- → ?
(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).
Flags: needinfo?(odvarko)
(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
relnote-firefox: ? → ---
Flags: needinfo?(odvarko)
(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

Updated

11 months ago
Product: Firefox → DevTools
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!
Flags: needinfo?(odvarko)
(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
Flags: needinfo?(odvarko) → needinfo?(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.
Flags: needinfo?(cmills)
You need to log in before you can comment on or make changes to this bug.