1353057 - Split requests summary button into multiple labels

Status: VERIFIED FIXED

(DevTools :: Netmonitor, enhancement, P1)

Description

[Tim Nguyen :ntim]

See bug 1350228 comment 5

Comment 2

[Vangelis Katsikaros]

Could I have this assigned? I think I know what to do and I can give it a go, without input.

Comment 3

[Vangelis Katsikaros]

Attachment: Bug 1353057 - Split requests summary button into multiple labels.

Review commit: https://reviewboard.mozilla.org/r/129072/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129072/

Comment 4

[Vangelis Katsikaros]

Attachment: status_bar.png

Comment 5

[Tim Nguyen :ntim]

Comment on attachment 8857141 [details]
Bug 1353057 - Split requests summary button into multiple labels.

https://reviewboard.mozilla.org/r/129072/#review131620

Looks good, but we want to move the labels outside of the button.

::: devtools/client/netmonitor/src/components/status-bar.js:55
(Diff revision 1)
> -        span({ className: "summary-info-text" }, text),
> +        span({ className: "status-bar-label summary-info-count" }, countText),
> +        span({ className: "status-bar-label summary-info-content-size" }, contentText),
> +        span({ className: "status-bar-label summary-info-transfer-size" }, transferText),
> +        span({ className: "status-bar-label summary-info-millis" }, millisText),

These should be moved outside the button, just before DOMContentLoaded > -1 &&

Comment 6

[Vangelis Katsikaros]

Attachment: status_bar.png

Comment 7

[Vangelis Katsikaros]

Comment on attachment 8857141 [details]
Bug 1353057 - Split requests summary button into multiple labels.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129072/diff/1-2/

Comment 8

[Ricky Chien [:rickychien] (inactive)]

(In reply to Vangelis Katsikaros from comment #6)
> Created attachment 8857364 [details]
> status_bar.png

r+ it looks great!

Comment 9

[Tim Nguyen :ntim]

Comment on attachment 8857141 [details]
Bug 1353057 - Split requests summary button into multiple labels.

https://reviewboard.mozilla.org/r/129072/#review131920

LGTM!

Comment 10

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

Great work here!

Couple of UI comments :

* (transferred XX) label should not be separated from the size (by toolbar-separator). There should be just one section/label as follows: `35 KB (transferred: 25 KB)` or even better: `35 KB / 25 KB transferred` (to be consistent with Chrome DevTools)

* The time slot should use label: `Finish: 1.19 s` (again consistency)

* It would be so great to have explanatory tooltip for every label! Can be done as separate bug though.

* Having learn more icon (in case we have MDN page to link to) in the status bar would be great too. We should file separate bug for this.


Thanks!
Honza

Comment 11

[Vangelis Katsikaros]

Comment on attachment 8857141 [details]
Bug 1353057 - Split requests summary button into multiple labels.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129072/diff/2-3/

Comment 12

[Vangelis Katsikaros]

Attachment: status_bar.png

Comment 13

[Vangelis Katsikaros]

Thanks everyone!

@Honza, I took care of
> * (transferred XX) label should not be separated from the size (by toolbar-separator). There should be just one section/label as follows: `35 KB (transferred: 25 KB)` or even better: `35 KB / 25 KB transferred` (to be consistent with Chrome DevTools)
> * The time slot should use label: `Finish: 1.19 s` (again consistency)

RE the last two UI comments, I think that if I open these tickets they will have things like my user agent etc in the description cluttering the content, so should I leave the opening to you?

Comment 14

[Tim Nguyen :ntim]

(In reply to Vangelis Katsikaros from comment #12)
> Created attachment 8857629 [details]
> status_bar.png

I would remove the tooltip altogether, it doesn't seem to provide any extra information from what's already available.

Comment 15

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

Comment on attachment 8857141 [details]
Bug 1353057 - Split requests summary button into multiple labels.

https://reviewboard.mozilla.org/r/129072/#review132368

Thanks for the update!

There are some problems with the localizatin, please see my comments.

Honza

::: devtools/client/locales/en-US/netmonitor.properties:147
(Diff revision 3)
>  # LOCALIZATION NOTE (networkMenu.sortedDesc): This is the tooltip displayed
>  # in the network table toolbar, for any column that is sorted descending.
>  networkMenu.sortedDesc=Sorted descending
>  
> -# LOCALIZATION NOTE (networkMenu.empty): This is the label displayed
> -# in the network table footer when there are no requests available.
> +# LOCALIZATION NOTE (networkMenu.summaryRequestsCount: This label is displayed
> +# in the network table footer providing the number of requests

The localization note doesn't corresponds to the string key and there is missing bracket at the end:

It should be: 

# LOCALIZATION NOTE (networkMenu.summary.requestsCount):

::: devtools/client/locales/en-US/netmonitor.properties:152
(Diff revision 3)
> -# LOCALIZATION NOTE (networkMenu.summary3): Semi-colon list of plural forms.
>  # See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
> -# This label is displayed in the network table footer providing concise
> -# information about all requests. Parameters: #1 is the number of requests,
> -# #2 is the size, #3 is the transferred size, #4 is the number of seconds.
> -networkMenu.summary3=One request, #2 (transferred: #3), #4;#1 requests, #2 (transferred: #3), #4
> +# LOCALIZATION NOTE (networkMenu.summaryRequestsCountEmpty: This label is displayed
> +# in the network table footer when there are no requests
> +networkMenu.summary.requestsCount=One request;%S requests
> +networkMenu.summary.requestsCountEmpty=No requests

Please create new localization note for networkMenu.summary.requestsCountEmpty

::: devtools/client/locales/en-US/netmonitor.properties:154
(Diff revision 3)
> -# This label is displayed in the network table footer providing concise
> -# information about all requests. Parameters: #1 is the number of requests,
> -# #2 is the size, #3 is the transferred size, #4 is the number of seconds.
> -networkMenu.summary3=One request, #2 (transferred: #3), #4;#1 requests, #2 (transferred: #3), #4
> +# LOCALIZATION NOTE (networkMenu.summaryRequestsCountEmpty: This label is displayed
> +# in the network table footer when there are no requests
> +networkMenu.summary.requestsCount=One request;%S requests
> +networkMenu.summary.requestsCountEmpty=No requests
> +
> +# LOCALIZATION NOTE (networkMenu.transferred: This label is displayed

The localization note is wrong (wrong key + missing end bracket), it should be:

# LOCALIZATION NOTE (networkMenu.summary.transferred):

::: devtools/client/locales/en-US/netmonitor.properties:156
(Diff revision 3)
> -# #2 is the size, #3 is the transferred size, #4 is the number of seconds.
> -networkMenu.summary3=One request, #2 (transferred: #3), #4;#1 requests, #2 (transferred: #3), #4
> +networkMenu.summary.requestsCount=One request;%S requests
> +networkMenu.summary.requestsCountEmpty=No requests
> +
> +# LOCALIZATION NOTE (networkMenu.transferred: This label is displayed
> +# in the network table footer providing the transferred size.
> +networkMenu.summary.transferred=%S / %S

Should be:
networkMenu.summary.transferred=%S / %S transferred

::: devtools/client/locales/en-US/netmonitor.properties:158
(Diff revision 3)
> +
> +# LOCALIZATION NOTE (networkMenu.transferred: This label is displayed
> +# in the network table footer providing the transferred size.
> +networkMenu.summary.transferred=%S / %S
> +
> +# LOCALIZATION NOTE (networkMenu.summaryRequestsCount: This label is displayed

The localization note is wrong (wrong key + missing bracket)

::: devtools/client/netmonitor/src/components/status-bar.js:61
(Diff revision 3)
>          span({ className: "summary-info-icon" }),
> -        span({ className: "summary-info-text" }, text),
>        ),
> +      span({ className: "status-bar-label summary-info-count" }, countText),
> +      span({ className: "status-bar-label summary-info-transferred" }, transferText),
> +      span({ className: "status-bar-label summary-info-fiished" }, finishedText),

Typo:
summary-info-fiished => summary-info-finished

Comment 16

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

Attachment: summary.png

One more thing. In case there are no requests - the summary says "No Requests" and two toolbar-separators added (see the attached screenshot).
Please remove these two separators.

Thanks!
Honza

Comment 17

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

(In reply to Tim Nguyen :ntim from comment #14)
> (In reply to Vangelis Katsikaros from comment #12)
> > Created attachment 8857629 [details]
> > status_bar.png
> 
> I would remove the tooltip altogether, it doesn't seem to provide any extra
> information from what's already available.
Agreed (we can provide better tooltips for individual status bar sections later)

Honza

Comment 18

[Vangelis Katsikaros]

Comment on attachment 8857141 [details]
Bug 1353057 - Split requests summary button into multiple labels.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129072/diff/3-4/

Comment 19

[Vangelis Katsikaros]

Comment on attachment 8857141 [details]
Bug 1353057 - Split requests summary button into multiple labels.

https://reviewboard.mozilla.org/r/129070/#review132624

::: devtools/client/netmonitor/src/components/status-bar.js:58
(Diff revision 4)
>        },
>          span({ className: "summary-info-icon" }),
> -        span({ className: "summary-info-text" }, text),
>        ),
> +      span({ className: "status-bar-label" }, countText),
> +      !(count === 0) && span({ className: "status-bar-label" }, transferText),

I understand that "!==" is not the inverse of "===", this is why I used "!( foo === bar )". Let me know if this is indeed correct.

Comment 20

[Vangelis Katsikaros]

Rebased and resolved conflicts.

>::: devtools/client/netmonitor/src/middleware/prefs.js:38
>(Diff revision 2)
>>        case RESET_COLUMNS:
>> -        Prefs.hiddenColumns = [];
>> +        Prefs.hiddenColumns = ["protocol"];
>>          break;
>
>I would remove this altogether, and use TOGGLE_COLUMN's code: https://bugzilla.mozilla.org/show_bug.cgi?id=1344523#c27

Took care of this now that the other bug has landed.

>::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:1329
>(Diff revision 3)
>>  
>>    .requests-list-security-and-domain {
>>      width: 16vw;
>>    }
>>  
>> +  .requests-list-protocol {
>
>Why it's here again?

I see all classes are redefined in the @media section, so I just copied it.

Comment 21

[Vangelis Katsikaros]

Attachment: status_bar.png

Comment 22

[Vangelis Katsikaros]

Comment https://bugzilla.mozilla.org/show_bug.cgi?id=1353057#c20 was pasted by mistake from another. Please ignore

Comment 23

[Vangelis Katsikaros]

> There are some problems with the localizatin, please see my comments.

Oh, how sloppy of me! I apologize for this back and forth.

> One more thing. In case there are no requests - the summary says "No Requests" and two toolbar-separators added (see the attached screenshot).
> Please remove these two separators.

Done

> (In reply to Vangelis Katsikaros from comment #12)
> > Created attachment 8857629 [details]
> > status_bar.png
> 
> I would remove the tooltip altogether, it doesn't seem to provide any extra
> information from what's already available.

Done

I did 2 additional things, let me know about these too:
* I removed css class "summary-info-text" that is no longer used.
* I didn't add new css classes "summary-info-*" for the new spans, as they aren't used somewhere.

Comment 24

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

Comment on attachment 8857141 [details]
Bug 1353057 - Split requests summary button into multiple labels.

https://reviewboard.mozilla.org/r/129072/#review132630

Great work here, thanks!

R+

Honza

Comment 25

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8857141 [details]
Bug 1353057 - Split requests summary button into multiple labels.

https://reviewboard.mozilla.org/r/129072/#review133184

::: devtools/client/netmonitor/src/components/status-bar.js:35
(Diff revision 4)
>    let {
>      DOMContentLoaded,
>      load,
>    } = timingMarkers;
>  
> -  let text = (count === 0) ? L10N.getStr("networkMenu.empty") :
> +  let countText = (count === 0) ? L10N.getStr("networkMenu.summary.requestsCountEmpty") :

nit: We can remove parenthesis for count === 0. There are several places use (count === 0), I think we can remove them safely.

::: devtools/client/netmonitor/src/components/status-bar.js:58
(Diff revision 4)
> +      !(count === 0) && span({ className: "status-bar-label" }, transferText),
> +      !(count === 0) && span({ className: "status-bar-label" }, finishText),

nit: count !== 0

Comment 26

[Vangelis Katsikaros]

Comment on attachment 8857141 [details]
Bug 1353057 - Split requests summary button into multiple labels.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129072/diff/4-5/

Comment 27

[Tim Nguyen :ntim]

Comment on attachment 8857141 [details]
Bug 1353057 - Split requests summary button into multiple labels.

https://reviewboard.mozilla.org/r/129072/#review133194

::: devtools/client/netmonitor/src/components/status-bar.js:39
(Diff revision 5)
> -    .replace("#3", getFormattedSize(transferredSize))
> -    .replace("#4", getFormattedTime(millis));
> +  let transferText = count === 0 ? "" :
> +    L10N.getFormatStrWithNumbers(
> +      "networkMenu.summary.transferred", getFormattedSize(contentSize),
> +      getFormattedSize(transferredSize)
> +  );
> +  let finishText = count === 0 ? "" :
> +    L10N.getFormatStrWithNumbers(
> +      "networkMenu.summary.finish", getFormattedTime(millis)
> +  );

No need to handle the case where count === 0 because the element is not appended in this case.

Comment 28

[Vangelis Katsikaros]

Comment on attachment 8857141 [details]
Bug 1353057 - Split requests summary button into multiple labels.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129072/diff/5-6/

Comment 29

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

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2bd891216dd33d542c679da33f42fbd218402d8e&selectedJob=92240914

I am seeing one failure:

TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_footer-summary.js | Uncaught exception - at resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/l10n.js:91 - Error: No localization found for [networkMenu.empty]

Honza

Comment 30

[Vangelis Katsikaros]

Comment on attachment 8857141 [details]
Bug 1353057 - Split requests summary button into multiple labels.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129072/diff/6-7/

Comment 31

[Vangelis Katsikaros]

Honza, thanks for the test! It looks like the test needs tweaking now that the summary is split in multiple labels, so I pushed a change.

From now on, I understand it's better if I:
* test locally `devtools/client/netmonitor/*` and `devtools/client/shared/*`
* trigger a try build after an R+. I see you used `try: -b d -p linux64 -u mochitest-dt -t none`, Ricky has used `try: -b o -p linux64,macosx64,win32 -u mochitest-clipboard,mochitest-dt,mochitest-e10s-devtools-chrome -t none --artifact` in another bug of mine. Is there a preference between the 2?

Comment 32

[Tim Nguyen :ntim]

(In reply to Vangelis Katsikaros from comment #31)
> Honza, thanks for the test! It looks like the test needs tweaking now that
> the summary is split in multiple labels, so I pushed a change.
> 
> From now on, I understand it's better if I:
> * test locally `devtools/client/netmonitor/*` and `devtools/client/shared/*`
Yes, if you feel like the change can break tests, in doubt, it's better to test locally.

> * trigger a try build after an R+. I see you used `try: -b d -p linux64 -u
> mochitest-dt -t none`, Ricky has used `try: -b o -p linux64,macosx64,win32
> -u mochitest-clipboard,mochitest-dt,mochitest-e10s-devtools-chrome -t none
> --artifact` in another bug of mine. Is there a preference between the 2?

The second one covers more tests. Note that you can trigger a try build before getting an R+ to save time.

Comment 33

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

Thanks for fixing the test, it runs well on my machine!

I am yet triggering a try build, if it's green it's ready to land.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=523c3e591d2b65c8f31cdcdcdcfb5716f2f41515

Honza

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b0554383fc5
Split requests summary button into multiple labels. r=Honza,ntim

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4b0554383fc5

Comment 36

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

We might want to update the docs (+ screenshots)

Honza

Comment 37

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

Bug 1358013 reported for tooltips support.

Honza

Comment 38

[Vangelis Katsikaros]

@Honza I am not sure on the process of updating the docs. Happy to assist if/when needed.

Comment 39

[Ciprian Georgiu, Desktop QA]

Reproduced the issue on Nightly from 2017-04-18 on W10.

This is verified fixed on latest Nightly 55.0a1 (2017-04-20) under Windows 10 x64 and Mac OS X 10.11.6.

Comment 40

[Matthew N. [:MattN]]

Screenshots of the change: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=a34919b3d942cfd4f0737d432742b0ffa9929389&newProject=mozilla-central&newRev=e9a5d4f62461ee0db07a41f59b73163ec106bc3e

Comment 41

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

(In reply to Vangelis Katsikaros from comment #38)
> @Honza I am not sure on the process of updating the docs. Happy to assist
> if/when needed.
The MDN team should spot the 'dev-doc-needed' keyword and act accordingly :-)


But, please see the comment #40. It shows visual difference between the new UI and the old one. It might be a detail, but I have been also thinking about it. I think we should remove the first separator (the vertical line between the perf button and the first label) and make the space about the same as before. Can you please file a new bug for it a look into it?

Thanks,
Honza

Comment 42

[Vangelis Katsikaros]

(In reply to Jan Honza Odvarko [:Honza] from comment #41)
> (In reply to Vangelis Katsikaros from comment #38)
> But, please see the comment #40. It shows visual difference between the new
> UI and the old one. It might be a detail, but I have been also thinking
> about it. I think we should remove the first separator (the vertical line
> between the perf button and the first label) and make the space about the
> same as before. Can you please file a new bug for it a look into it?

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1358956