Closed Bug 1290437 Opened 8 years ago Closed 8 years ago

Fix and land the console netlogging tests

Categories

(DevTools :: Console, defect)

Product:
DevTools
Component:
Console
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox51 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: jsnajdr, Assigned: rickychien)

References

Details

(Whiteboard: [good taipei bug])

Attachments

(2 files)

Fix and land the console netlogging tests
26.56 KB, patch
Details | Diff | Splinter Review
Bug 1290437 - Fix and land the console netlogging tests
58 bytes, text/x-review-board-request
jsnajdr
: review+
Details 
For some reason, the patch with tests (attachment 8735424 [details] [diff] [review]) in bug 1211525 was never finished and landed.

At this moment, the browser_net_cookies.js and browser_net_post.js tests fail. The reason for both failures is that the information in the UI is checked before it arrives from the server.

The workflow for every network request is this:
- when the request is initiated, the server sends a networkEvent with the little info that is available at that time
- as more info is available, server sends networkEventUpdate events with "updateType" field that says "responseHeaders" etc. Very little info is sent as part of this event. For example, the headers count and size, but not their values
- client sends a "getResponseHeaders" request (or "getAnythingElse") and receives the complete data (if available)

The browser_net_cookies test fails because the cookies are not present when check at [1] is performed and when they arrive, there is no code that would update the component state.

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/net/components/net-info-body.js#132
I think the best way to fix this is to refactor the netlogging code to Redux. The current code does suspicious things like triggering side effects (actions.requestData) in render() methods.

The Redux netlogging could also be trivially added to the new console frontend. Lin, what do you think?
Blocks: 1211525, 1287172
Flags: needinfo?(lclark)
After rebasing and removing the irrelevant parts, this is the up-to-date version of attachment 8735424 [details] [diff] [review].
Yes, we should move any calls to things such as actions.requestData out of render. Using Redux shouldn't be a requirement for that, though. We should be able to fix that in the existing frontend with componentDidMount. See https://facebook.github.io/react/tips/initial-ajax.html

In the new console frontend, we do use Redux and this data will probably end up in the Redux store. But I don't think we want to introduce Redux into the current frontend.
Flags: needinfo?(lclark)
(In reply to Lin Clark [:linclark] from comment #3)
> Yes, we should move any calls to things such as actions.requestData out of
> render. Using Redux shouldn't be a requirement for that, though.We should
> be able to fix that in the existing frontend with componentDidMount.
Yes, totally agree.

Honza
Whiteboard: [good taipei bug]
Assignee: nobody → rchien
Status: NEW → ASSIGNED
After applying rebased patch from comment 2, it ends up showing the URL_ROOT is not defined error:

33 INFO TEST-UNEXPECTED-FAIL | devtools/client/webconsole/net/test/mochitest/browser_net_basic.js | Exception thrown - at chrome://mochitests/content/browser/devtools/client/webconsole/net/test/mochitest/browser_net_basic.js:8 - ReferenceError: URL_ROOT is not defined
34 INFO TEST-UNEXPECTED-FAIL | devtools/client/webconsole/net/test/mochitest/browser_net_cookies.js | Exception thrown - at chrome://mochitests/content/browser/devtools/client/webconsole/net/test/mochitest/browser_net_cookies.js:8 - ReferenceError: URL_ROOT is not defined

Jarda, do you have any idea?
Flags: needinfo?(jsnajdr)
(In reply to Ricky Chien [:rickychien] from comment #5)
> After applying rebased patch from comment 2, it ends up showing the URL_ROOT
> is not defined error:

These horrible errors usually happen when the head.js script fails to load. For some reason, it fails silently without reporting any errors and the test execution continues. But the variables that it was supposed to define are not defined and you get the ReferenceError about URL_ROOT.

The head.js fails to load because it tries to load common subscripts from other directories, and these subscripts are not declared in its browser.ini. Because of this, they are not copied into your build directory, which causes a "file not found" error.

You need to add these lines to the browser.ini:

--- a/devtools/client/webconsole/net/test/mochitest/browser.ini
+++ b/devtools/client/webconsole/net/test/mochitest/browser.ini
@@ -9,6 +9,8 @@ support-files =
   test.txt
   test.xml
   test.xml^headers^
+  !/devtools/client/webconsole/test/head.js
+  !/devtools/client/framework/test/shared-head.js
Flags: needinfo?(jsnajdr)
For the test of browser_net_cookies, I noticed that it doesn't receive "request.cookies" or "response.cookies" from backend so that test case was stuck. I suspect that there is something wrong in "updateBody" call at net-request.js or networkEventUpdate event.

Jarda, do you have a clue about that?
Flags: needinfo?(jsnajdr)
(In reply to Ricky Chien [:rickychien] from comment #7)
> For the test of browser_net_cookies, I noticed that it doesn't receive
> "request.cookies" or "response.cookies" from backend so that test case was
> stuck. I suspect that there is something wrong in "updateBody" call at
> net-request.js or networkEventUpdate event.

That's exactly the issue I describe in comment 0. When the CookiesTab is selected, it asks for the cookies data (calling actions.requestData), but it can happen that the cookies are not available yet. When they later get available (networkEventUpdate arrives from the server), then updateBody updates the view, but that's long after the test checked for the data and failed.
Flags: needinfo?(jsnajdr)
browser_net_cookies.js fixed by removing cookies tab check so that cookies tab always appear on panel (if that meets requirement?)

browser_net_post.js failure since there is an unexpected return result at isJson() in post-tab.js. It's simple fix by accessing "text" property from postData.

In current patch, I haven't fixed any "actions.requestData out of render" things. If we'd like to do that in this bug, should I move all "actions.requestData" call for every tab components (such as header-tab, response-tab, params-tab...etc)?
Flags: needinfo?(jsnajdr)
Comment on attachment 8782804 [details]
Bug 1290437 - Fix and land the console netlogging tests

Cancel review and I'll re-submit patch again once "actions.requestData out of render() method" done.
Attachment #8782804 - Flags: review?(jsnajdr)
Comment on attachment 8782804 [details]
Bug 1290437 - Fix and land the console netlogging tests

https://reviewboard.mozilla.org/r/72854/#review70586

::: devtools/client/webconsole/net/components/net-info-body.js
(Diff revision 1)
>  
>    onTabChanged(index) {
>      this.setState({tabActive: index});
>    },
>  
> -  hasCookies() {

I'd rather fix the feature (not showing Cookies tab if there are no cookies) instead of removing it.

You'll need to figure out why the Cookies tab isn't there when the test is looking for it.

::: devtools/client/webconsole/net/components/post-tab.js:41
(Diff revision 1)
>    },
>  
>    displayName: "PostTab",
>  
>    isJson(file) {
> -    let postData = file.request.postData;
> +    let postData = file.request.postData.text;

nit: Let's rename this variable to "text", as it doesn't contain "postData" any more.

::: devtools/client/webconsole/net/test/mochitest/head.js:36
(Diff revision 1)
> + * @return a promise that resolves to the tab object when the url is loaded
> + */
> +function addTestTab(url) {
> +  info("Adding a new JSON tab with URL: '" + url + "'");
> +
> +  let deferred = promise.defer();

In bug 1283869, we are removing usages of "defer" and replacing them with plain Promises.

In new files like this, we shouldn't add new usages of "defer". Please convert them to Promises, or even better, to Task.js. All async functions in this file are going to benefit a lot from the Task.js style.

::: devtools/client/webconsole/net/test/mochitest/head.js:116
(Diff revision 1)
> +  if (!element.hasAttribute("loading")) {
> +    deferred.resolve();
> +    return deferred.promise;
> +  }
> +
> +  let onReady = event => {

This is a common code pattern - wait for only one instance of an event, resolve a promise and unsubscribe when it fires.

There is a utility function "once" that does exactly this at [1]. You would use it like this:

yield once(element, "netlog-no-pending-requests", true);

[1] http://searchfox.org/mozilla-central/source/devtools/client/framework/test/shared-head.js#255-257

::: devtools/client/webconsole/net/test/mochitest/head.js:192
(Diff revision 1)
> +/**
> + * Wait for specified number of milliseconds.
> + * @param delay Number of milliseconds to wait.
> + * @returns
> + */
> +function waitForTime(delay) {

The shared-head.js already has a "wait" function for this at [1]. Let's not duplicate.

[1] http://searchfox.org/mozilla-central/source/devtools/client/framework/test/shared-head.js#293-297

::: devtools/client/webconsole/net/test/unit/xpcshell.ini:6
(Diff revision 1)
> +[DEFAULT]
> +tags = devtools
> +head =
> +tail =
> +firefox-appdir = browser
> +skip-if = toolkit == 'android' || toolkit == 'gonk'

This "skip-if" shouldn't be there without a reason. Like the tests actually being unable to run on Android or Gonk. Utility functions for JSON and header parsing should work everywhere, however.

It was probably blindly copied from another test suite.
Attachment #8782804 - Flags: review-
(In reply to Ricky Chien [:rickychien] from comment #10)
> In current patch, I haven't fixed any "actions.requestData out of render"
> things. If we'd like to do that in this bug, should I move all
> "actions.requestData" call for every tab components (such as header-tab,
> response-tab, params-tab...etc)?

That would be very nice to fix, of course, but feel free to do it in a separate followup bug.

The right place for triggering the actions is componentDidMount(), as this article posted by Lin says:

https://facebook.github.io/react/tips/initial-ajax.html

I'm not sure about one thing, however: shouldn't the example in the article trigger the request also in componentWillReceiveProps, in case this.props.source changes? Otherwise, the state won't get updated, will it?
Flags: needinfo?(jsnajdr) → needinfo?(lclark)
In their hypothetical use case source might not change... I'm not sure. But if it did need to update, it would probably be in response to an interaction, so the request could be triggered in the handler for that interaction.
Flags: needinfo?(lclark)
Review issues and test failures are fixed but I'm still finding why we don't receive cookies data to display the Cookies Tab.
I notice that there is no requestHeaders / responseHeaders method triggered [1] at the first time XHR panel opened. After clicking Headers tab manually, cookies data appears after a requestHeaders method request is done and Cookies tab shows up as expected.

How this requestHeaders method request be triggered and why it doesn't emit when first time XHR panel is opened?

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/net/net-request.js#127
Flags: needinfo?(jsnajdr)
The reason requestHeaders method is triggered at `actions.requestData("requestHeaders");` in headers-tab.js [1], so that we are able to receive a cookies header [2] and show up Cookies Tab after clicking Headers tab.

And if we don't display Cookies tab by default, we never trigger `actions.requestData("requestCookies");` in cookies-tab.js [3] and so there is no way to show up the Cookies tab since cookies data will not arrive.

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/net/components/headers-tab.js#37
[2] https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/net/components/net-info-body.js#68
[3] https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/net/components/cookies-tab.js#36
The request for requestHeaders and responseHeaders is triggered only after the "Headers" tab is selected. By default, tab with index 1 (i.e., the second one) is selected, so the "Cookies" tab is never shown until you actually click on the "Headers". That's an obvious bug. I'm not sure about the default tabActive being 1 instead of 0 - is it really a feature, or just an off-by-one bug, Honza?

You need to change how hasCookies determines its value. Rather that look at the request and response headers, you should watch for the "requestCookies" and "responseCookies" update packets in updateBody. These packets contain the cookie count, and you should display the Cookies tab if either of them is > 0. Add new properties with these values to the state and update the state in the updateBody method. This will cause a UI update as soon as the information about cookies arrives.

There seems to be another bug in the cookies-tab.js component causing that response cookies are never displayed. See how only the "requestCookies" are requested at [1], but not "responseCookies"? Please add a test case to browser_net_cookies.js that will catch this bug.

[1] http://searchfox.org/mozilla-central/source/devtools/client/webconsole/net/components/cookies-tab.js#36
Flags: needinfo?(jsnajdr) → needinfo?(odvarko)
(In reply to Jarda Snajdr [:jsnajdr] from comment #19)
> The request for requestHeaders and responseHeaders is triggered only after
> the "Headers" tab is selected. By default, tab with index 1 (i.e., the
> second one) is selected, so the "Cookies" tab is never shown until you
> actually click on the "Headers". That's an obvious bug. I'm not sure about
> the default tabActive being 1 instead of 0 - is it really a feature, or just
> an off-by-one bug, Honza?
This is regression bug (the indexing changed, it was originally 1-based).
So, the 'Headers' tab should be selected by default.

Honza

diff --git a/devtools/client/webconsole/net/components/net-info-body.js b/devtoo--- a/devtools/client/webconsole/net/components/net-info-body.js
+++ b/devtools/client/webconsole/net/components/net-info-body.js
@@ -40,17 +40,17 @@ var NetInfoBody = React.createClass({
       response: PropTypes.object.isRequired
     })
   },

   displayName: "NetInfoBody",

   getDefaultProps() {
     return {
-      tabActive: 1
+      tabActive: 0
     };
   },

   getInitialState() {
     return {
       data: {
         request: {},
         response: {}
Flags: needinfo?(odvarko)
Thank you Honza, problem solved after apply your patch!
(In reply to Jarda Snajdr [:jsnajdr] from comment #19)
> You need to change how hasCookies determines its value. Rather that look at
> the request and response headers, you should watch for the "requestCookies"
> and "responseCookies" update packets in updateBody. These packets contain
> the cookie count, and you should display the Cookies tab if either of them
> is > 0. Add new properties with these values to the state and update the
> state in the updateBody method. This will cause a UI update as soon as the
> information about cookies arrives.

Since it just a regression bug as Honza mentioned on comment 20, do we still need to do above modification?
Flags: needinfo?(jsnajdr)
(In reply to Ricky Chien [:rickychien] from comment #22)
> Since it just a regression bug as Honza mentioned on comment 20, do we still
> need to do above modification?

If you do it, it will increase the quality of the code. The visibility of the "Cookies" tab won't depend on the "Headers" being selected by default, which is somewhat arbitrary and looks like a Rube Goldberg machine.

But I agree it's not needed to finish this bug. Please file a separate followup bug if you decide not to do it.

The issue with the "responseCookies" not being requested and displayed should definitely be fixed here. If it's confirmed and I'm not just overlooking something, of course.
Flags: needinfo?(jsnajdr)
(In reply to Jarda Snajdr [:jsnajdr] from comment #19) 
> You need to change how hasCookies determines its value. Rather that look at
> the request and response headers, you should watch for the "requestCookies"
> and "responseCookies" update packets in updateBody. These packets contain
> the cookie count, and you should display the Cookies tab if either of them
> is > 0. Add new properties with these values to the state and update the
> state in the updateBody method. This will cause a UI update as soon as the
> information about cookies arrives.

updateBody() has already watched for the "requestCookies" and "responseCookies".
Check updateBody() -> this.requestData() -> this.onRequestData(). Once a new cookies data arrives, it will invoke refresh() to update top level React component and update following child components. These things seems like a temporary Redux working flow as far as I know, and it will be replaced by Redux soon.

In cookies-tab.js, requestData("requestCookies") & requestData("responseCookies") will not be invoked until CookiesTab is created [1]. However, hasCookies() will always be false if above methods never get called. Thus, there must be a place to initialize data and that invoke requestData("requestCookies") & requestData("responseCookies") at the beginning and the place might be in NetInfoBody itself.

Does it make sense?

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/net/components/net-info-body.js#138
Flags: needinfo?(jsnajdr)
(In reply to Ricky Chien [:rickychien] from comment #24)
> updateBody() has already watched for the "requestCookies" and
> "responseCookies".
> Check updateBody() -> this.requestData() -> this.onRequestData(). Once a new
> cookies data arrives, it will invoke refresh() to update top level React
> component and update following child components. These things seems like a
> temporary Redux working flow as far as I know, and it will be replaced by
> Redux soon.

It seems you're mixing two different concepts together:

1. updateBody watches for networkEventUpdate packets from the server. These updates don't contain the full data, but only notifications: hey, client, the response arrived and "responseCookies" data are available. If you want to see them, ask me.

2. requestData("responseCookies") asks for the full cookies data.

In order to decide about the "Cookies" tab visibility, you need only the update packets. You don't need to ask for them, they always arrive and trigger an updateBody call.

To actually display the data in the "Cookies" tab, you need to requestData(). It's perfectly OK to call it inside the cookies-tab component, when it's getting rendered. The decision about showing the "Cookies" tab has already been made.

There is a bug in the cookies-tab component - it never asks for "responseCookies" and doesn't display them. You can easily test it: look at any response that has Set-Cookie header. Netmonitor will display the cookies, but Console netlogging won't.

> In cookies-tab.js, requestData("requestCookies") & requestData("responseCookies")
> will not be invoked until CookiesTab is created [1].

Yes, that's correct. Don't ask for the data until you really need them. I.e., until the user has clicked the "Cookies" tab.

> However, hasCookies() will always be false if above methods never get called.

This is not true - hasCookies() should use the information from the cookie-related networkEventUpdate packets.

Do you have RDP inspector add-on installed? It's very useful to watch the protocol packet and inspect what they contain. You'll see all the packets and requests I'm talking about in real time.

https://addons.mozilla.org/en-US/firefox/addon/rdp-inspector/
Flags: needinfo?(jsnajdr)
Thanks! I'll check. But RDP-inspector is broken on my latest nightly and threw errors:

console.error: rdpinspector:
  Message: TypeError: actorProto is undefined
  Stack:
    ActorClass@resource://devtools/shared/protocol.js:1106:3
(In reply to Ricky Chien [:rickychien] from comment #26)
> Thanks! I'll check. But RDP-inspector is broken on my latest nightly and
> threw errors:

That's fixed in bug 1295171. It landed in m-c less than 24 hours ago. If you do your own build, you'll have it. Otherwise, the next Nightly should be OK.
(In reply to Jarda Snajdr [:jsnajdr] from comment #25)
> In order to decide about the "Cookies" tab visibility, you need only the
> update packets. You don't need to ask for them, they always arrive and
> trigger an updateBody call.

I still have no idea how to update packets. Do you mean I have to trigger a update packets event manually? 

My understand is that this.onRequestData() listener has already set, so CookiesTab will be visible when a new networkEventUpdate packets comes. We don't have to do any code modification AFAIK. Is that right?

Patch has submitted without adding new test case since I'd like to hear your feedback to ensure everything is on the right way.
Flags: needinfo?(jsnajdr)
(In reply to Ricky Chien [:rickychien] from comment #29)
> My understand is that this.onRequestData() listener has already set, so
> CookiesTab will be visible when a new networkEventUpdate packets comes. We
> don't have to do any code modification AFAIK. Is that right?

You still don't get the difference between networkEventUpdate packets (handled by updateBody), and responses to requests triggered by requestData (with replies handled by onRequestData).
Flags: needinfo?(jsnajdr)
Comment on attachment 8782804 [details]
Bug 1290437 - Fix and land the console netlogging tests

https://reviewboard.mozilla.org/r/72854/#review71682

::: devtools/client/webconsole/net/components/net-info-body.js:69
(Diff revisions 1 - 2)
>    },
>  
> +  hasCookies() {
> +    let {request, response} = this.state.data;
> +    return NetUtils.getHeaderValue(request.headers, "Cookie") ||
> +      NetUtils.getHeaderValue(response.headers, "Cookie");

If you continue to use response.headers here, then the value tested should be "Set-Cookie". That's the name of the cookie header in a response.

This is a bug in Honza's implementation, it's not new.

::: devtools/client/webconsole/net/test/mochitest/head.js:194
(Diff revisions 1 - 2)
>    let mm = gBrowser.selectedBrowser.messageManager;
>  
> -  let def = promise.defer();
> +  return Task.spawn(function* () {
> -  mm.addMessageListener(name, function onMessage(msg) {
> +    mm.addMessageListener(name, function onMessage(msg) {
> -    mm.removeMessageListener(name, onMessage);
> +      mm.removeMessageListener(name, onMessage);
> -    def.resolve(msg.data);
> +      yield msg.data;

If the msg.data is to be returned from Task.spawn (as a promise), there should be "return" here instead of "yield". Otherwise, the generator function will implicitly return undefined.
Attachment #8782804 - Flags: review?(jsnajdr) → review-
Comment on attachment 8782804 [details]
Bug 1290437 - Fix and land the console netlogging tests

https://reviewboard.mozilla.org/r/72854/#review70586

> This "skip-if" shouldn't be there without a reason. Like the tests actually being unable to run on Android or Gonk. Utility functions for JSON and header parsing should work everywhere, however.
> 
> It was probably blindly copied from another test suite.

Yes, I just applied the patch on comment 2 and didn't aware of these duplicate code. thanks!
Comment on attachment 8782804 [details]
Bug 1290437 - Fix and land the console netlogging tests

https://reviewboard.mozilla.org/r/72854/#review71710

::: devtools/client/webconsole/net/components/net-info-body.js:70
(Diff revision 3)
>  
>    hasCookies() {
>      let {request, response} = this.state.data;
>      return NetUtils.getHeaderValue(request.headers, "Cookie") ||
> -      NetUtils.getHeaderValue(response.headers, "Cookie");
> +      NetUtils.getHeaderValue(response.headers, "Cookie") ||
> +      (request.cookies && request.cookies.length > 0) ||

I guess that what you said is to check the cookies.length here.

::: devtools/client/webconsole/net/test/mochitest/head.js:194
(Diff revision 3)
> +  let mm = gBrowser.selectedBrowser.messageManager;
> +
> +  return new Promise((resolve) => {
> +    mm.addMessageListener(name, function onMessage(msg) {
> +      mm.removeMessageListener(name, onMessage);
> +      resolve(msg.data);

I wasn't aware of there is a callback wrapping around yield so that makes it unreachable.

Promise is the only way to resolve and return result in deeper callback.
(In reply to Ricky Chien [:rickychien] from comment #34)
> I guess that what you said is to check the cookies.length here.

You would have to check the cookies.length from the networkEventUpdate for this to work.

> I wasn't aware of there is a callback wrapping around yield so that makes it
> unreachable.
> 
> Promise is the only way to resolve and return result in deeper callback.

Task.spawn() returns a promise that will resolve with the return value of the inner function:

let n = Task.spawn(function* () {
  yield somethingAsync();
  return 10;
});

Here, "n" is a promise that will resolve to "10" once the task is done.

However, if you do:

let n = Task.spawn(function* () {
  yield somethingAsync();
  yield 10;
  // there's implicit "return undefined" here!
});

then "n" will resolve to undefined, because that's the return value.
Attachment #8782804 - Flags: review?(jsnajdr)
(In reply to Jarda Snajdr [:jsnajdr] from comment #36)
> (In reply to Ricky Chien [:rickychien] from comment #34)
> > I guess that what you said is to check the cookies.length here.
> 
> You would have to check the cookies.length from the networkEventUpdate for
> this to work.
> 
> > I wasn't aware of there is a callback wrapping around yield so that makes it
> > unreachable.
> > 
> > Promise is the only way to resolve and return result in deeper callback.
> 
> Task.spawn() returns a promise that will resolve with the return value of
> the inner function:
> 
> let n = Task.spawn(function* () {
>   yield somethingAsync();
>   return 10;
> });
> 
> Here, "n" is a promise that will resolve to "10" once the task is done.
> 
> However, if you do:
> 
> let n = Task.spawn(function* () {
>   yield somethingAsync();
>   yield 10;
>   // there's implicit "return undefined" here!
> });
> 
> then "n" will resolve to undefined, because that's the return value.

Thank your for explanation. But what I mentioned that is if there is a yield in generator function's another callback:

let n = Task.spawn(function* () {
  doSync(function callback() {
    yield 10;
  });
});

it doesn't work. So convert to promise is the only way to resolve and return result correctly.
(In reply to Ricky Chien [:rickychien] from comment #38)
> Thank your for explanation. But what I mentioned that is if there is a yield
> in generator function's another callback:
> 
> let n = Task.spawn(function* () {
>   doSync(function callback() {
>     yield 10;
>   });
> });
> 
> it doesn't work. So convert to promise is the only way to resolve and return
> result correctly.

That's right, because you're in a completely different function there. You need to convert the callback into a Promise:

yield new Promise(resolve => {
  doSync(function callback() {
    resolve(10);
  });
});
Comment on attachment 8782804 [details]
Bug 1290437 - Fix and land the console netlogging tests

https://reviewboard.mozilla.org/r/72854/#review71948

This finally looks pretty good, thanks for the effort. Let's fix the remaining small issues and we're done!

::: devtools/client/webconsole/net/components/net-info-body.js:70
(Diff revisions 3 - 4)
>  
>    hasCookies() {
>      let {request, response} = this.state.data;
> -    return NetUtils.getHeaderValue(request.headers, "Cookie") ||
> -      NetUtils.getHeaderValue(response.headers, "Cookie") ||
> -      (request.cookies && request.cookies.length > 0) ||
> +    return this.state.hasCookies ||
> +      NetUtils.getHeaderValue(request.headers, "Cookie") ||
> +      NetUtils.getHeaderValue(response.headers, "Set-Cookie");

Checking the headers is not needed here. this.state.hasCookies already has all the information.

::: devtools/client/webconsole/net/net-request.js:116
(Diff revision 4)
>      } else {
>        this.closeBody();
>      }
>    },
>  
> +  updateCookies: function(method, response) {

Very nice, now the cookie detection will work perfectly :)

Only one little issue: check if the method is one of "requestCookies" or "responseCookies". Otherwise, you're checking "response.cookies > 0" on any update packet, and you have no guarantee what the "cookies" property means there.
Comment on attachment 8782804 [details]
Bug 1290437 - Fix and land the console netlogging tests

https://reviewboard.mozilla.org/r/72854/#review71962

Some suggestions to improve comments and code style, but it's r+. Please do a try run before landing.

::: devtools/client/webconsole/net/net-request.js:119
(Diff revisions 4 - 5)
>    },
>  
>    updateCookies: function(method, response) {
>      // TODO: This code will be part of a reducer.
>      let result;
> -    if (response.cookies > 0) {
> +    if (response.cookies > 0 && method.indexOf("Cookies") !== -1) {

nit: ["requestCookies", "responseCookies"].includes(method) would be more readable.

::: devtools/client/webconsole/net/test/mochitest/browser_net_cookies.js:43
(Diff revisions 4 - 5)
>    ok(cookieValue, "Cookie value must exist");
>    is(cookieValue.textContent, "foo",
>      "The cookie value must have proper value");
> +
> +  cookieName = tabBody.querySelector(
> +    ".netInfoParamName > span[title='test']");

This selector could be improved to match only request or response cookies:

".netInfoGroup.requestCookies .netInfoParamName > span[title='test']"

::: devtools/client/webconsole/net/test/mochitest/browser_net_cookies.js:45
(Diff revisions 4 - 5)
>      "The cookie value must have proper value");
> +
> +  cookieName = tabBody.querySelector(
> +    ".netInfoParamName > span[title='test']");
> +
> +  // Verify "bar" cookie (name and value)

nit: "bar" cookie is not verified there - the name is "test".

Maybe add comment that you're testing response vs request cookies?
Attachment #8782804 - Flags: review?(jsnajdr) → review+
Updated patch to fix ^M character.
Blocks: 1298048
Pushed by jsnajdr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a89334d982bd
Fix and land the console netlogging tests r=jsnajdr
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a89334d982bd
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
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: