cache l10n values in security-panel

RESOLVED FIXED in Firefox 58

Status

enhancement
P3
normal
RESOLVED FIXED
2 years ago
10 months ago

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
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 security-panel
http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/security-panel.js#46

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,
I have taken a stab at this issue and have created a mozreview-request.
Does this look ok?
(Reporter)

Updated

2 years ago
Assignee: nobody → abhinav.koppula
(Reporter)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8917482 [details]
Bug 1407550 - Cache L10N values in security-panel.

https://reviewboard.mozilla.org/r/188450/#review193954

The change looks good. Thanks for taking care of this bug.
For code style consistency we'd prefer declare const name in UPPER_CASE.

::: devtools/client/netmonitor/src/components/security-panel.js:20
(Diff revision 1)
>  // Components
>  const TreeViewClass = require("devtools/client/shared/components/tree/TreeView");
>  const PropertiesView = createFactory(require("./properties-view"));
>  
>  const { div, input, span } = DOM;
> +const errorLabel = L10N.getStr("netmonitor.security.error");

Please use UPPER_CASE as constant variable name, such as `ERROR_LABEL`

::: devtools/client/netmonitor/src/components/security-panel.js:45
(Diff revision 1)
>    const notAvailable = L10N.getStr("netmonitor.security.notAvailable");
>    let object;
>  
>    if (securityInfo.state === "secure" || securityInfo.state === "weak") {
>      const { subject, issuer, validity, fingerprint } = securityInfo.cert;
>      const enabledLabel = L10N.getStr("netmonitor.security.enabled");

please also rename these const variables in UPPER_CASE
Attachment #8917482 - Flags: review?(gasolin)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

2 years ago
mozreview-review-reply
Comment on attachment 8917482 [details]
Bug 1407550 - Cache L10N values in security-panel.

https://reviewboard.mozilla.org/r/188450/#review193954

Thanks for the review, Fred. Have addressed the comments.
(Reporter)

Comment 6

2 years ago
mozreview-review
Comment on attachment 8917482 [details]
Bug 1407550 - Cache L10N values in security-panel.

https://reviewboard.mozilla.org/r/188450/#review194848

::: devtools/client/netmonitor/src/components/security-panel.js:45
(Diff revision 2)
>    const notAvailable = L10N.getStr("netmonitor.security.notAvailable");
>    let object;
>  
>    if (securityInfo.state === "secure" || securityInfo.state === "weak") {
>      const { subject, issuer, validity, fingerprint } = securityInfo.cert;
> -    const enabledLabel = L10N.getStr("netmonitor.security.enabled");
> +    const ENABLED_LABEL = L10N.getStr("netmonitor.security.enabled");

Sorry for not find this issue at first time. Define const in SecurityPanel does not give any performance gain at all.

But we could move `L10N.getStr` related string after WARNING_CIPHER_LABEL (out of function), which does cache strings and gain performance a little bit.

You can leave L10N.getFormatStr inline if it does changed dynamically.
Attachment #8917482 - Flags: review?(gasolin)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

2 years ago
mozreview-review-reply
Comment on attachment 8917482 [details]
Bug 1407550 - Cache L10N values in security-panel.

https://reviewboard.mozilla.org/r/188450/#review194848

> Sorry for not find this issue at first time. Define const in SecurityPanel does not give any performance gain at all.
> 
> But we could move `L10N.getStr` related string after WARNING_CIPHER_LABEL (out of function), which does cache strings and gain performance a little bit.
> 
> You can leave L10N.getFormatStr inline if it does changed dynamically.

I have made the changes. I have defined `const HOST_HEADER_LABEL` inside the function just for consistency sake and have moved `NOT_AVAILABLE` constant outside.
(Reporter)

Comment 9

2 years ago
mozreview-review
Comment on attachment 8917482 [details]
Bug 1407550 - Cache L10N values in security-panel.

https://reviewboard.mozilla.org/r/188450/#review195372

Looks good, Thanks for contribution!
Attachment #8917482 - Flags: review?(gasolin) → review+

Comment 10

2 years ago
Pushed by flin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/afba28bafc9d
Cache L10N values in security-panel. r=gasolin
https://hg.mozilla.org/mozilla-central/rev/afba28bafc9d
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58

Updated

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