cache l10n values in toolbar

RESOLVED FIXED in Firefox 58

Status

enhancement
P3
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: gasolin, Assigned: abhinav.koppula, Mentored)

Tracking

({good-first-bug})

57 Branch
Firefox 58

Firefox Tracking Flags

(firefox57 fix-optional, firefox58 fixed)

Details

(Whiteboard: [good first bug][mentor-lang=zh][lang=js])

Attachments

(1 attachment)

Reporter

Description

2 years ago
+++ 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
Reporter

Updated

2 years ago
No longer depends on: 1407548
See Also: → 1404130
Reporter

Updated

2 years ago
Keywords: good-first-bug
Comment hidden (mozreview-request)
Assignee

Comment 2

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

Updated

2 years ago
Assignee: nobody → abhinav.koppula
Reporter

Comment 3

2 years ago
mozreview-review
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.
Attachment #8917527 - Flags: review?(gasolin)
Reporter

Updated

2 years ago
Attachment #8917527 - Flags: feedback+
Comment hidden (mozreview-request)
Assignee

Comment 5

2 years ago
mozreview-review-reply
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.
Reporter

Comment 6

2 years ago
mozreview-review
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,
Attachment #8917527 - Flags: review?(gasolin)
Comment hidden (mozreview-request)
Assignee

Comment 8

2 years ago
mozreview-review-reply
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?
Reporter

Comment 9

2 years ago
> >> 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.
Reporter

Comment 10

2 years ago
mozreview-review
Comment on attachment 8917527 [details]
Bug 1407552 - Caching L10N values in netmonitor toolbar.

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

thanks!
Attachment #8917527 - Flags: review?(gasolin) → review+

Comment 11

2 years ago
Pushed by flin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8363beb49fd3
Caching L10N values in netmonitor toolbar. r=gasolin
https://hg.mozilla.org/mozilla-central/rev/8363beb49fd3
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.