1407552 - cache l10n values in toolbar

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P3)

Description

[[e-mail]]

+++ This bug was initially created as a clone of Bug #1407548 +++

as bug 1404130 we found L10N APIs has high cost so we'd like cache as much strings in each component as possible

plenty of strings can be cached in toolbar
http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/toolbar.js#101

you can refer the `mdn-link` component, define L10N strings as `const` like
http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/mdn-link.js#17

Comment 1

[Abhinav Koppula]

Attachment: Bug 1407552 - Caching L10N values in netmonitor toolbar.

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

Comment 2

[Abhinav Koppula]

Hi Fred,
Have created a review-request for this.
Can you check if I missed anything here?

Comment 3

[[e-mail]]

Comment on attachment 8917527 [details]
Bug 1407552 - Caching L10N values in netmonitor toolbar.

https://reviewboard.mozilla.org/r/188484/#review193936

Thanks for the patch, it did the job and it will be nice if we can make the code more compact.

::: devtools/client/netmonitor/src/components/toolbar.js:41
(Diff revision 1)
>  const TOOLBAR_CLEAR = L10N.getStr("netmonitor.toolbar.clear");
>  
>  const DEVTOOLS_DISABLE_CACHE_PREF = "devtools.cache.disabled";
>  const DEVTOOLS_ENABLE_PERSISTENT_LOG_PREF = "devtools.netmonitor.persistlog";
> +const TOOLBAR_FILTER_LABELS = {
> +  all: L10N.getStr(`netmonitor.toolbar.filter.all`),

The list reoccured in 
http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/reducers/filters.js#15 and 
http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/statistics-panel.js#171

To prevent repeating, please define FILTER_TAGS = ["html", "css", "js", "xhr", "fonts", "images", "media", "flash", "ws", "other"]; in constants http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/constants.js#284 and generate the list/dict from these filters tags.

Comment 4

[Abhinav Koppula]

Comment on attachment 8917527 [details]
Bug 1407552 - Caching L10N values in netmonitor toolbar.

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

Comment 5

[Abhinav Koppula]

Comment on attachment 8917527 [details]
Bug 1407552 - Caching L10N values in netmonitor toolbar.

https://reviewboard.mozilla.org/r/188484/#review193936

> The list reoccured in 
> http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/reducers/filters.js#15 and 
> http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/statistics-panel.js#171
> 
> To prevent repeating, please define FILTER_TAGS = ["html", "css", "js", "xhr", "fonts", "images", "media", "flash", "ws", "other"]; in constants http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/constants.js#284 and generate the list/dict from these filters tags.

Hi Fred,
Thanks for the feedback. I have incorporated the review comments.

Comment 6

[[e-mail]]

Comment on attachment 8917527 [details]
Bug 1407552 - Caching L10N values in netmonitor toolbar.

https://reviewboard.mozilla.org/r/188484/#review194666

Thanks for update! Just need address some nit after these change.

::: devtools/client/netmonitor/src/components/statistics-panel.js:8
(Diff revision 2)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
>  const ReactDOM = require("devtools/client/shared/vendor/react-dom");
> +const { REQUESTS_WATERFALL } = require("../constants");

please import `FILTER_TAGS` from the top level constants

::: devtools/client/netmonitor/src/components/toolbar.js:44
(Diff revision 2)
>  const DEVTOOLS_ENABLE_PERSISTENT_LOG_PREF = "devtools.netmonitor.persistlog";
> +const TOOLBAR_FILTER_LABELS = REQUESTS_WATERFALL.FILTER_TAGS
> +                              .concat("all")
> +                              .reduce((o, tag) =>
> +                                Object.assign(o, {
> +                                  [tag]: L10N.getStr(`netmonitor.toolbar.filter.` + tag)

could be `[tag]: L10N.getStr(`netmonitor.toolbar.filter.${tag}`)`

And for current indent, the line might be too long, you could just put `.concat("all")...` and the rest with 2 spaces indent.

::: devtools/client/netmonitor/src/constants.js:282
(Diff revision 2)
>    TICKS_COLOR_OPACITY: 192,
>    HEADER_TICKS_MULTIPLE: 5, // ms
>    HEADER_TICKS_SPACING_MIN: 60, // px
>    // Reserve extra space for rendering waterfall time label
>    LABEL_WIDTH: 50, // px
> +  FILTER_TAGS: ["html", "css", "js", "xhr", "fonts", "images",

Since these tags are not just for waterfall, I'd expect its a top level constant like `FILTER_FLAGS`. And don't forget to add the trailing comma,

Comment 7

[Abhinav Koppula]

Comment on attachment 8917527 [details]
Bug 1407552 - Caching L10N values in netmonitor toolbar.

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

Comment 8

[Abhinav Koppula]

Comment on attachment 8917527 [details]
Bug 1407552 - Caching L10N values in netmonitor toolbar.

https://reviewboard.mozilla.org/r/188484/#review194666

Thanks Fred, Have updated the review-request.

>> And don't forget to add the trailing comma

Out of curiosity, why is  trailing comma so important?

Comment 9

[[e-mail]]

> >> And don't forget to add the trailing comma
> 
> Out of curiosity, why is  trailing comma so important?

It's not that important, you can still see no trailing comma in some places.
It's just a diff-friendly coding style (and is supported from ES 2018).
With trailing comma, the git diff looks more clear.

Comment 10

[[e-mail]]

Comment on attachment 8917527 [details]
Bug 1407552 - Caching L10N values in netmonitor toolbar.

https://reviewboard.mozilla.org/r/188484/#review194690

thanks!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8363beb49fd3
Caching L10N values in netmonitor toolbar. r=gasolin

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8363beb49fd3