Closed
Bug 1147398
Opened 9 years ago
Closed 9 years ago
Replace each string bundle creation for browser.properties and brand.properties with lazy getters in nsBrowserGlue
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: bgrins, Assigned: kartiksomani, Mentored)
References
Details
(Whiteboard: [good first bug][lang=js])
Attachments
(1 file, 2 obsolete files)
14.62 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
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•9 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]
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•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.
Flags: needinfo?(bgrinstead)
Assignee | ||
Comment 3•9 years ago
|
||
if @bogas04 is not working on it now, I would like to work on it.
Assignee | ||
Comment 4•9 years ago
|
||
Created lazygetters for browserbundle and brandbundle, and refactored them in browser/components/nsBrowserGlue.js
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.
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?
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?
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?
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?
Flags: needinfo?(gijskruitbosch+bugs)
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)
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 11•9 years ago
|
||
Created lazy getters for browserBundle and brandBundle, and made corrections after comments from Gijs
Attachment #8585475 -
Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8585475 -
Flags: review?(gijskruitbosch+bugs) → feedback+
Assignee | ||
Comment 13•9 years ago
|
||
Made some more changes (indentation related) after comments from Gijs.
Attachment #8585748 -
Flags: review?(gijskruitbosch+bugs)
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
Attachment #8585748 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 15•9 years ago
|
||
What is to be done from my end now?
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. :-)
Updated•9 years ago
|
Attachment #8583333 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8585475 -
Attachment is obsolete: true
Comment 17•9 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]
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
Closed: 9 years ago
status-firefox40:
--- → fixed
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.
Description
•