Replace each string bundle creation for browser.properties and brand.properties with lazy getters in nsBrowserGlue

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bgrins, Assigned: kartiksomani, Mentored)

Tracking

unspecified
Firefox 40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
There are lots of places in nsBrowserGlue.js that call Services.strings.createBundle("chrome://branding/locale/brand.properties") or Services.strings.createBundle("chrome://browser/locale/browser.properties").

This should be refactored to instead use a lazy getter for each at the top of the file.

Here is a list of each of the current references:
* https://dxr.mozilla.org/mozilla-central/search?q=path%3Abrowser%2Fcomponents%2Fnsbrowserglue.js+chrome%3A%2F%2Fbranding%2Flocale%2Fbrand.properties&redirect=true
* https://dxr.mozilla.org/mozilla-central/search?q=path%3Abrowser%2Fcomponents%2Fnsbrowserglue.js+chrome%3A%2F%2Fbrowser%2Flocale%2Fbrowser.properties&redirect=true
(Reporter)

Updated

4 years ago
Mentor: gijskruitbosch+bugs
Summary: Replace each string bundle creation for browser.properties and brand.properties with lazy getters in nsBrowserGlu → Replace each string bundle creation for browser.properties and brand.properties with lazy getters in nsBrowserGlue
Whiteboard: [good first bug][lang=js]

Comment 1

4 years ago
Hey!
I'm ready to take this up.

If I understand it right, I need to make two global variables: 'brandBundle' & 'browserBundle'  
defined as null at the beginning, and at every reference throughout the file, I'll simply call createBundle in case the required variable is null.

Something like this : 

> brandBundle = brandBundle || Services.strings.createBundle("chrome://branding/locale/brand.properties");
> browserBundle = browserBundle || Services.strings.createBundle("chrome://branding/locale/browser.properties");

Is that right ?
Flags: needinfo?(bgrinstead)

Comment 2

4 years ago
(In reply to bogas04 from comment #1)
> Hey!
> I'm ready to take this up.
> 
> If I understand it right, I need to make two global variables: 'brandBundle'
> & 'browserBundle'  
> defined as null at the beginning, and at every reference throughout the
> file, I'll simply call createBundle in case the required variable is null.
> 
> Something like this : 
> 
> > brandBundle = brandBundle || Services.strings.createBundle("chrome://branding/locale/brand.properties");
> > browserBundle = browserBundle || Services.strings.createBundle("chrome://branding/locale/browser.properties");
> 
> Is that right ?

Not quite. You basically want to add something along the lines of:

http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#132

132 XPCOMUtils.defineLazyGetter(this, "ShellService", function() {
133   try {
134     return Cc["@mozilla.org/browser/shell-service;1"].
135            getService(Ci.nsIShellService);
136   }
137   catch(ex) {
138     return null;
139   }
140 });

Except instead of "ShellService" it should be "gBrandBundle" and "gBrowserBundle", respectively, and the functions that you pass as the third argument should return the stringbundle (Services.strings...).

Then you should update the reference sites to reference gBrand/gBrowserBundle instead. They shouldn't need to create them themselves.
Flags: needinfo?(bgrinstead)
(Assignee)

Comment 3

4 years ago
if @bogas04 is not working on it now, I would like to work on it.
(Assignee)

Comment 4

4 years ago
Created lazygetters for browserbundle and brandbundle, and refactored them in browser/components/nsBrowserGlue.js

Comment 5

4 years ago
Comment on attachment 8583333 [details] [diff] [review]
Created Lazy getters for browserBundle and brandBundle, and refactored

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

It seems bogas is working on bug 1139026, so maybe Kartik can work on this one.

Kartik, can you format your patch to include a commit message ( https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#Commit_Message_Conventions )?

Then I have some comments on the patch content. When you've addressed all of these, please attach a new patch and set the "review" field to "?", and put ":Gijs" in the textfield that appears, and you should be able to get me to review the patch.

::: browser/components/nsBrowserGlue.js
@@ +140,5 @@
>  });
>  
> +XPCOMUtils.defineLazyGetter(this, "gBrandBundle", function() {
> +  return Services.strings.createBundle('
> +  chrome://branding/locale/brand.properties');

Please create a temporary variable for the URL and pass that as the argument. Whatever you do, don't split up the actual string with the URL that you're passing here across multiple lines. Now there's whitespace in the URL - I'd be surprised if it worked. Did you test this change? How?

@@ +145,5 @@
> +});
> +
> +XPCOMUtils.defineLazyGetter(this, "gBrowserBundle", function() {
> +  return Services.strings.createBundle('
> +  chrome://browser/locale/browser.properties');

Same here.

@@ +1203,2 @@
>  
>      let appName = brandBundle.GetStringFromName("brandShortName");

This is the only line using brandBundle in this function, so please just update this line and remove the definition.

@@ +1277,5 @@
>        return;
>  
>      var formatter = Cc["@mozilla.org/toolkit/URLFormatterService;1"].
>                      getService(Ci.nsIURLFormatter);
> +    var browserBundle = gBrowserBundle;

There are only 2 callsites here, so let's just update those and remove this line.

@@ +1283,1 @@
>      var appName = brandBundle.GetStringFromName("brandShortName");

You can just update this line to use gBrandBundle directly, and remove the previous line.

@@ +1618,1 @@
>      var applicationName = brandBundle.GetStringFromName("brandShortName");

Again, remove the previous line, update this line.

@@ +2276,5 @@
>      function onFullScreen() {
>        popup.remove();
>      }
>  
> +    var browserBundle = gBroserBundle;

Typo...

@@ +2365,5 @@
>    },
>  
>    _promptGeo : function(aRequest) {
>      var secHistogram = Services.telemetry.getHistogramById("SECURITY_UI");
> +    var browserBundle = gBroserBundle;

Typo. But also, we might as well remove this line and update the two callsites below.

@@ +2418,5 @@
>                       "geo-notification-icon", options);
>    },
>  
>    _promptWebNotifications : function(aRequest) {
> +    var browserBundle = gBrowserBundle);

You left the closing brace here.

Again, there's only 1 callsite, so might as well update that instead of this line.

@@ +2452,5 @@
>    },
>  
>    _promptPointerLock: function CPP_promtPointerLock(aRequest, autoAllow) {
>  
> +    let browserBundle = gBrowserBundle;

Same thing here.
(Assignee)

Comment 6

4 years ago
"Please create a temporary variable for the URL and pass that as the argument"

Gijs, I saw other lazy getters were using the string directly. So using a variable is optional or do I have to do it?
(Assignee)

Comment 7

4 years ago
"Please create a temporary variable for the URL and pass that as the argument"

Gijs, I saw other lazy getters were using the string directly. So using a variable is optional or do I have to do it?
(Assignee)

Comment 8

4 years ago
Also, i tested by building and checking whether the browser was starting up. Any specific test case(s) that I should test?
(Assignee)

Comment 9

4 years ago
I have the patch ready, need the previous queries to be answered before submitting, or should I just submitting the patch?
Flags: needinfo?(gijskruitbosch+bugs)

Comment 10

4 years ago
(In reply to Kartik Somani from comment #7)
> "Please create a temporary variable for the URL and pass that as the
> argument"
> 
> Gijs, I saw other lazy getters were using the string directly. So using a
> variable is optional or do I have to do it?

I mean, either you should wrap the call correctly, or use a temp variable. I don't mind much which you prefer.

(In reply to Kartik Somani from comment #9)
> I have the patch ready, need the previous queries to be answered before
> submitting, or should I just submitting the patch?

Just submitting the patch is fine, we can continue to iterate if more changes are necessary.

(apologies for the delay in responding, I was away somewhere with no internet access)
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 11

4 years ago
Posted patch bug_1147398.patch (obsolete) — Splinter Review
Created lazy getters for browserBundle and brandBundle, and made corrections after comments from Gijs
Attachment #8585475 - Flags: review?(gijskruitbosch+bugs)

Comment 12

4 years ago
Comment on attachment 8585475 [details] [diff] [review]
bug_1147398.patch

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

This looks OK, I just have some whitespace nits. Can you upload the new version and finalize the commit's comment to have "r=gijs" at the end, and request another review so I see it and can run tests on it before landing it? Thanks!

::: browser/components/nsBrowserGlue.js
@@ +1286,2 @@
>                                                           aPropData.stringParams,
>                                                           aPropData.stringParams.length);

Nit: please reindent these lines so they keep matching with the previous line.

@@ +2377,1 @@
>                                                     [requestingURI.path], 1);

Nit: same here.

@@ +2380,1 @@
>                                                     [requestingURI.host], 1);

and here.

@@ +2416,1 @@
>                                                       [requestingURI.host], 1);

and here.
Attachment #8585475 - Flags: review?(gijskruitbosch+bugs) → feedback+
(Assignee)

Comment 13

4 years ago
Made some more changes (indentation related) after comments from Gijs.
Attachment #8585748 - Flags: review?(gijskruitbosch+bugs)

Comment 14

4 years ago
Comment on attachment 8585748 [details] [diff] [review]
Patch for bug 1147398

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

Looks good to me!

Try push: 

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0c97ee4ed6b
Attachment #8585748 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 15

4 years ago
What is to be done from my end now?

Comment 16

4 years ago
(In reply to Kartik Somani from comment #15)
> What is to be done from my end now?

We wait for the try push to finish and see if there are any failing tests that indicate an issue with the patch. If not, I'll add the checkin-needed keyword or land the patch myself. So there's not much to do on your end for the moment. If the tests do fail, we'll need to figure out how you can fix the patch so it doesn't cause issues. :-)

Updated

4 years ago
Attachment #8583333 - Attachment is obsolete: true

Updated

4 years ago
Attachment #8585475 - Attachment is obsolete: true

Comment 17

4 years ago
remote:   https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=b8c1a40cea07
Assignee: nobody → kartiksomani
Status: NEW → ASSIGNED
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
(Reporter)

Updated

4 years ago
Blocks: 1148996
Backed out for crashtest assertions https://hg.mozilla.org/integration/fx-team/rev/0b0c5e555e9b

https://treeherder.mozilla.org/logviewer.html#?job_id=2499150&repo=fx-team
Flags: needinfo?(kartiksomani)
Flags: needinfo?(gijskruitbosch+bugs)
Actually, that's just an intermittent failure that struck three times on this one push.

Relanded https://hg.mozilla.org/integration/fx-team/rev/0d5114f9b9d9

Sorry for the churn.
Flags: needinfo?(kartiksomani)
Flags: needinfo?(gijskruitbosch+bugs)
https://hg.mozilla.org/mozilla-central/rev/0d5114f9b9d9
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 40
You need to log in before you can comment on or make changes to this bug.