Closed Bug 1124246 Opened 5 years ago Closed 5 years ago

Visually differentiate XHR network logs in the webconsole

Categories

(DevTools :: Console, enhancement, P3)

enhancement

Tracking

(firefox38 fixed)

RESOLVED FIXED
Firefox 38
Tracking Status
firefox38 --- fixed

People

(Reporter: Kwan, Assigned: Kwan)

References

Details

Attachments

(2 files, 6 obsolete files)

Attached patch WIP patch (obsolete) — Splinter Review
With my patch for bug 972655 XHR messages now have a filter. It would also be helpful for there to be a visual distinction between them and normal netlogs, both for when all messages are viewed, and especially when only errors are.

Attached is a rough patch implementing one idea of how this could be achieved, which shamelessly rips off the styling of the event listener indicator from the inspector.

My first thought on positioning was after the URL, but I wasn't a fan of how that means it moves around depending on the length of the URL.  The current patch thus puts it before the method name.  I'll shortly attach a image showing the other positions I tested out.
Attached image Positioning experiments
Depends on: 972655
+1 for this feature. The screenshots attached look pretty nice. I have a preference for having the XHR label just before the status.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #2)
> +1 for this feature. The screenshots attached look pretty nice. I have a
> preference for having the XHR label just before the status.

I think that's my #2, with my #1 being the label right next to the URL. Stephen, what do you think?
Flags: needinfo?(shorlander)
Just to share my thinking behind my preferences:

+ 1–3, next to properties of the request
− 4&5, next to properties of the response

+ 1&5, don't move, one place to visually scan to to find it
− 2–4, move about, takes more effort to visually find them (looks "messy")

− 1, moves the method type from its usual location (minor)
Assignee: nobody → moz-ian
I think it's less important to keep the XHR label from moving, as you can filter out non-XGR requests if that's what you are looking for, than it is to keep the request type (GET, POST, etc.) from moving, which you can't filter on.
(In reply to Panos Astithas [:past] from comment #5)
> I think it's less important to keep the XHR label from moving, as you can
> filter out non-XGR requests if that's what you are looking for, than it is
> to keep the request type (GET, POST, etc.) from moving, which you can't
> filter on.

Ah, good point.  In that case I'd favour option 2, between method and URL, both visually (still moves, but less than at the end of URL) and logically.
Attached patch WIP patch v2 (obsolete) — Splinter Review
Update of my WIP patch.
+ Switch padding and line-height to px to fix the text shifting up 1px on later messages and looking horribly off-center
+ Now with localised text
+ Switch to positioning 2, between method and URL
Attachment #8552519 - Attachment is obsolete: true
Attached patch patch v2.1 (obsolete) — Splinter Review
Updated the patch header, and caught a typo in the localisation note.
Attachment #8553847 - Attachment is obsolete: true
Attachment #8561441 - Flags: review?(past)
Comment on attachment 8561441 [details] [diff] [review]
patch v2.1

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

Looks good to me, but I'll ask Brian to take an additional look at the style changes. I also think it would be a good idea to have a test for this (something simple, like ensuring the extra node is present in one of the existing tests).

::: browser/themes/shared/devtools/webconsole.inc.css
@@ +261,5 @@
>    color: -moz-nativehyperlinktext;
>    margin: 0 6px;
>  }
>  
> +.message .xhr {

Can't we use the child selector instead of the descendant selector here?
Attachment #8561441 - Flags: review?(past)
Attachment #8561441 - Flags: review+
Attachment #8561441 - Flags: feedback?(bgrinstead)
(In reply to Panos Astithas [:past] from comment #5)
> I think it's less important to keep the XHR label from moving, as you can
> filter out non-XGR requests if that's what you are looking for, than it is
> to keep the request type (GET, POST, etc.) from moving, which you can't
> filter on.

Yeah, I think 1 looks nicer visually but it breaks scanning the request "column". 3 is probably the least disruptive to the flow but doesn't make the most logical sense. I agree 2 seems to be the best trade-off.

Another option to help with alignment would be to indent everything and put the XHR tag first: http://cl.ly/image/3A0m0g2y3J2H but it has other drawbacks and is probably more complicated.
Flags: needinfo?(shorlander)
Comment on attachment 8561441 [details] [diff] [review]
patch v2.1

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

::: browser/themes/shared/devtools/webconsole.inc.css
@@ +261,5 @@
>    color: -moz-nativehyperlinktext;
>    margin: 0 6px;
>  }
>  
> +.message .xhr {

We can't use the child selector on .message, but could on .message-body.  I don't have a strong preference, but seeing how existing network message styling is using selectors like:

.message[category=network] .status

I'd suggest:

.message[category=network] .xhr

@@ +266,5 @@
> +  background-color: var(--theme-body-color-alt);
> +  color: var(--theme-body-background);
> +  border-radius: 3px;
> +  font-weight: bold;
> +  font-size: .9em;

Since you are using line-height to vertically center this, I'd switch font-size to 10px as well to make sure it is always centered across platforms / different default font sizes.
Attachment #8561441 - Flags: feedback?(bgrinstead) → feedback+
Attached patch patch v3 with tests (obsolete) — Splinter Review
(In reply to Panos Astithas [:past] from comment #9)
> Comment on attachment 8561441 [details] [diff] [review]
> patch v2.1
> 
> Review of attachment 8561441 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me, but I'll ask Brian to take an additional look at the style
> changes. I also think it would be a good idea to have a test for this
> (something simple, like ensuring the extra node is present in one of the
> existing tests).
Done.  At the moment this just adds positive checks to the same two tests I had to s/SEVERITY_LOG/SEVERITY_INFO/ when adding the filter.  Negative tests run on every call of waitForMessages() (and trigger without those two isXhr: true changes).

(In reply to Panos Astithas [:past] from comment #9)
> Can't we use the child selector instead of the descendant selector here?
(In reply to Brian Grinstead [:bgrins] from comment #11)
> We can't use the child selector on .message, but could on .message-body.  I
> don't have a strong preference, but seeing how existing network message
> styling is using selectors like:
> 
> .message[category=network] .status
> 
> I'd suggest:
> 
> .message[category=network] .xhr
Done.

(In reply to Stephen Horlander [:shorlander] from comment #10)
> Another option to help with alignment would be to indent everything and put
> the XHR tag first: http://cl.ly/image/3A0m0g2y3J2H but it has other
> drawbacks and is probably more complicated.
This is actually pretty easy to do, by having the label there for all network messages and visibility: hidden it for non-XHR requests (since trying to allocate space on non-XHR messages seems to be a losing proposition with the vagaries of font sizing and the possibility of the label being translated). However having the extra space in front of all-non xhr net requests doesn't look too good, especially when interleaved with other non-network messages. (My original-original plan had been to have the label in the icon spot, but that was quickly scuppered when I realised that wouldn't work for XHR errors since they already have the X there.)

(In reply to Brian Grinstead [:bgrins] from comment #11)
> @@ +266,5 @@
> > +  background-color: var(--theme-body-color-alt);
> > +  color: var(--theme-body-background);
> > +  border-radius: 3px;
> > +  font-weight: bold;
> > +  font-size: .9em;
> 
> Since you are using line-height to vertically center this, I'd switch
> font-size to 10px as well to make sure it is always centered across
> platforms / different default font sizes.
Done.
Attachment #8561441 - Attachment is obsolete: true
Attachment #8562947 - Flags: review?(past)
Attached patch patch v3.1 with tests (obsolete) — Splinter Review
(In reply to Ian Moody [:Kwan] from comment #12)
> Created attachment 8562947 [details] [diff] [review]
> patch v3 with tests
> 
> (In reply to Panos Astithas [:past] from comment #9)
> > Comment on attachment 8561441 [details] [diff] [review]
> > patch v2.1
> > 
> > Review of attachment 8561441 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks good to me, but I'll ask Brian to take an additional look at the style
> > changes. I also think it would be a good idea to have a test for this
> > (something simple, like ensuring the extra node is present in one of the
> > existing tests).
> Done.  At the moment this just adds positive checks to the same two tests I
> had to s/SEVERITY_LOG/SEVERITY_INFO/ when adding the filter.  Negative tests
> run on every call of waitForMessages() (and trigger without those two isXhr:
> true changes).
Actually, if we're going to test this, lets be thorough and test SEVERITY_ERROR with an XHR label.  Added one to the browser_console.js test.  Running it without isXhr: true triggers error as expected.  With it we get an extra pass.
Is there an easy way to test SEVERITY_WARNING with an XHR request?  I'm not sure what triggers warnings, security/cert problems?  Is there an easy way to simulate that in the tests? (If so, feel free to leave the review until a v3.2 with such a test)
Attachment #8562947 - Attachment is obsolete: true
Attachment #8562947 - Flags: review?(past)
Attachment #8563184 - Flags: review?(past)
(In reply to Ian Moody [:Kwan] from comment #14)
> Is there an easy way to test SEVERITY_WARNING with an XHR request?  I'm not
> sure what triggers warnings, security/cert problems?  Is there an easy way
> to simulate that in the tests? (If so, feel free to leave the review until a
> v3.2 with such a test)

Mark would know best, but I think fetching a resource that triggers a mixed content warning should do it. Not sure about low-grade crypto connections (RC4, SHA-1), are they warnings or errors?
Flags: needinfo?(mgoodwin)
(In reply to Panos Astithas [:past] from comment #15)
> (In reply to Ian Moody [:Kwan] from comment #14)
> > Is there an easy way to test SEVERITY_WARNING with an XHR request?  I'm not
> > sure what triggers warnings, security/cert problems?  Is there an easy way
> > to simulate that in the tests? (If so, feel free to leave the review until a
> > v3.2 with such a test)
> 
> Mark would know best, but I think fetching a resource that triggers a mixed
> content warning should do it. Not sure about low-grade crypto connections
> (RC4, SHA-1), are they warnings or errors?

The security messages (crypto warnings, HPKP / HSTS parse errors etc) are warnings. You can create one of these by having your test request a resource from e.g. https://sha1ee.example.com

One thing to watch out for; some of the warnings relate to deprecation - you don't want your stuff to break when they actually get removed.

An HSTS parse error is probably a good one to go for. At the moment, these only work for top level requests but when bug 1092055 lands you can maybe use one of the resources from its tests.
Flags: needinfo?(mgoodwin)
Attached patch patch v4 with tests (obsolete) — Splinter Review
Attachment #8563184 - Attachment is obsolete: true
Attachment #8563184 - Flags: review?(past)
Attachment #8563371 - Flags: review?(past)
Comment on attachment 8563371 [details] [diff] [review]
patch v4 with tests

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

::: browser/devtools/webconsole/test/head.js
@@ +1116,5 @@
>  
>      return true;
>    }
>  
> +  function checkXhrLabel(aElement) {

Nit: how about hasXhrLabel, since it returns a boolean?
Attachment #8563371 - Flags: review?(past) → review+
(In reply to Panos Astithas [:past] from comment #18)
> Nit: how about hasXhrLabel, since it returns a boolean?
Good call, done.
Review carried forward.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=32ead0b1fffe
Attachment #8563371 - Attachment is obsolete: true
Attachment #8563432 - Flags: review+
(In reply to Ian Moody [:Kwan] from comment #13)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=514c20fb8dd8
(In reply to Ian Moody [:Kwan] from comment #19)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=32ead0b1fffe
All green again, except for infra issues with OS X 10.8 opt.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/af2139805585
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
Depends on: 1133030
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.