Brian Grinstead [:bgrins] Reporter
	Description • 9 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

	bogas04
	Comment 1 • 9 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 ?

	:Gijs (he/him)
	Comment 2 • 9 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.

	Kartik Somani Assignee
	Comment 3 • 9 years ago

if @bogas04 is not working on it now, I would like to work on it.

	Kartik Somani Assignee
	Comment 4 • 9 years ago

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

	:Gijs (he/him)
	Comment 5 • 9 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.

	Kartik Somani Assignee
	Comment 6 • 9 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?

	Kartik Somani Assignee
	Comment 7 • 9 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?

	Kartik Somani Assignee
	Comment 8 • 9 years ago

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

	Kartik Somani Assignee
	Comment 9 • 9 years ago

I have the patch ready, need the previous queries to be answered before submitting, or should I just submitting the patch?

	:Gijs (he/him)
	Comment 10 • 9 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)

	Kartik Somani Assignee
	Comment 11 • 9 years ago

Created lazy getters for browserBundle and brandBundle, and made corrections after comments from Gijs

	:Gijs (he/him)
	Comment 12 • 9 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.

	Kartik Somani Assignee
	Comment 13 • 9 years ago

Made some more changes (indentation related) after comments from Gijs.

	:Gijs (he/him)
	Comment 14 • 9 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

	Kartik Somani Assignee
	Comment 15 • 9 years ago

What is to be done from my end now?

	:Gijs (he/him)
	Comment 16 • 9 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. :-)

	:Gijs (he/him)
	Comment 17 • 9 years ago

remote:   https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=b8c1a40cea07

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 18 • 9 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 19 • 9 years ago

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.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 20 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/0d5114f9b9d9