Closed Bug 750295 Opened 9 years ago Closed 9 years ago

don't cache gStringBundle in nsAccessNode

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: tbsaunde, Assigned: capella)

References

Details

(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(1 file, 5 obsolete files)

move logic for getting the string bundle from nsAccessNode::InitXPAccessibility to nsAccessible::TranslateString() then remove gStringBundle stuff from accessible/src/base/nsAccessNode.cpp/h
Attached patch Patch (v1) (obsolete) — Splinter Review
First patch, builds and tests ok locally... let me know what you think -- mark
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #621405 - Flags: feedback?(trev.saunders)
Depends on: 750287
Attached patch Patch (v2) (obsolete) — Splinter Review
(Forgot to qrefresh the patch ...)
Attachment #621405 - Attachment is obsolete: true
Attachment #621405 - Flags: feedback?(trev.saunders)
Attachment #621406 - Flags: feedback?(trev.saunders)
Comment on attachment 621406 [details] [diff] [review]
Patch (v2)


> void nsAccessNode::InitXPAccessibility()

you could remove this method all together if you like

> nsAccessible::TranslateString(const nsAString& aKey, nsAString& aStringOut)
> {
>   nsXPIDLString xsValue;
> 
>-  gStringBundle->GetStringFromName(PromiseFlatString(aKey).get(), getter_Copies(xsValue));
>+  nsIStringBundle* accStringBundle = 0;
>+  nsCOMPtr<nsIStringBundleService> stringBundleService =
>+    mozilla::services::GetStringBundleService();
>+  if (stringBundleService) {
>+    // Static variables are released in ShutdownAllXPAccessibility();
>+    stringBundleService->CreateBundle(ACCESSIBLE_BUNDLE_URL, 
>+                                      &accStringBundle);

I'm not sure what to say about this other than please think a little bit more about what you do here.
Attachment #621406 - Flags: feedback?(trev.saunders)
Ok, I'll lose the InitXP method. I wasn't sure if maybe you'd like to leave a placeholder there.

For the second item, if you're asking about releasing string bundles... that's an open question I had, so I left the comment there. I originally planned to do that code in the objects' destructer, but then I did some searching and thought I found that releasing the bundle wasn't always required in a module.. so I'll have to ask you how to handle that.

If there's something else I need to be seeing in the block of code, I might suggest not having to init the var to 0.

I also wondered if we need to check the result of the CreateBundle() before calling GetStringFromName(), but that wasn't done in the original code.
Comment on attachment 621406 [details] [diff] [review]
Patch (v2)

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

::: accessible/src/base/nsAccessible.cpp
@@ +634,5 @@
> +  nsIStringBundle* accStringBundle = 0;
> +  nsCOMPtr<nsIStringBundleService> stringBundleService =
> +    mozilla::services::GetStringBundleService();
> +  if (stringBundleService) {
> +    // Static variables are released in ShutdownAllXPAccessibility();

remove the comment

@@ +636,5 @@
> +    mozilla::services::GetStringBundleService();
> +  if (stringBundleService) {
> +    // Static variables are released in ShutdownAllXPAccessibility();
> +    stringBundleService->CreateBundle(ACCESSIBLE_BUNDLE_URL, 
> +                                      &accStringBundle);

you leak (http://mxr.mozilla.org/mozilla-central/source/intl/strres/src/nsStringBundle.cpp#732), use nsCOMPtr instead raw pointer

@@ +639,5 @@
> +    stringBundleService->CreateBundle(ACCESSIBLE_BUNDLE_URL, 
> +                                      &accStringBundle);
> +  }
> +
> +  accStringBundle->GetStringFromName(PromiseFlatString(aKey).get(), getter_Copies(xsValue));

I'd say you need to null check it

also it'd be nice to check with somebody (Neil?) if you need PromiseFlatString (the necessity is not evident from http://mxr.mozilla.org/mozilla-central/source/intl/strres/src/nsStringBundle.cpp#175).
> @@ +636,5 @@
> > +    mozilla::services::GetStringBundleService();
> > +  if (stringBundleService) {
> > +    // Static variables are released in ShutdownAllXPAccessibility();
> > +    stringBundleService->CreateBundle(ACCESSIBLE_BUNDLE_URL, 
> > +                                      &accStringBundle);
> 
> you leak
> (http://mxr.mozilla.org/mozilla-central/source/intl/strres/src/
> nsStringBundle.cpp#732), use nsCOMPtr instead raw pointer

giving away answers ;-)

> also it'd be nice to check with somebody (Neil?) if you need
> PromiseFlatString (the necessity is not evident from
> http://mxr.mozilla.org/mozilla-central/source/intl/strres/src/nsStringBundle.
> cpp#175).

well, the function takes a nsAString which I'm pretty sure doesn't have .get() so
(In reply to Trevor Saunders (:tbsaunde) from comment #6)

> well, the function takes a nsAString which I'm pretty sure doesn't have
> .get() so

it has .data() or something like this but internally they rely on length so it should be ok but I didn't read the code deeply.
Attached patch Patch (v3) (obsolete) — Splinter Review
Thanks for the tip re:nsCOMPtr and how it handles release and such. It's something I knew I needed to read up on, and I can't get enough help from mentors in either case :)
Attachment #621406 - Attachment is obsolete: true
Attachment #621643 - Flags: review?(surkov.alexander)
Attachment #621643 - Attachment is patch: true
Comment on attachment 621643 [details] [diff] [review]
Patch (v3)

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

r=me, please attach new patch and request feedback from Trevor.

::: accessible/src/base/nsAccessible.cpp
@@ +626,5 @@
>    return *aIndexInParent != -1 ? NS_OK : NS_ERROR_FAILURE;
>  }
>  
>  void 
>  nsAccessible::TranslateString(const nsAString& aKey, nsAString& aStringOut)

change TranslateString to use nsString& for key argument please so that you don't need PromiseFlatString stuffs

@@ +632,5 @@
> +  nsCOMPtr<nsIStringBundle> stringBundle;
> +  nsCOMPtr<nsIStringBundleService> stringBundleService =
> +    mozilla::services::GetStringBundleService();
> +  if (stringBundleService) {
> +    stringBundleService->CreateBundle(ACCESSIBLE_BUNDLE_URL, 

move ACCESSIBLE_BUNDLE_URL from nsAccessNode.h  (you can keep it as local static const or use the string directly or keep it in the beginning of file).

@@ +640,4 @@
>    nsXPIDLString xsValue;
> +  nsresult rv = stringBundle->GetStringFromName(PromiseFlatString(aKey).get(),
> +                                                getter_Copies(xsValue));
> +  if (!NS_FAILED(rv))

NS_SUCCEEDED(rv)
Attachment #621643 - Flags: review?(surkov.alexander) → review+
Attached patch Patch (v4) (obsolete) — Splinter Review
Attachment #621643 - Attachment is obsolete: true
Attachment #621925 - Flags: feedback?(trev.saunders)
Comment on attachment 621925 [details] [diff] [review]
Patch (v4)

> 
>-#define ACCESSIBLE_BUNDLE_URL "chrome://global-platform/locale/accessible.properties"
> #define PLATFORM_KEYS_BUNDLE_URL "chrome://global-platform/locale/platformKeys.properties"

btw nit, this one isn't needed here either, but probably best delt with in a follow up

>+nsAccessible::TranslateString(const nsString& aKey, nsAString& aStringOut)
> {
>+  nsCOMPtr<nsIStringBundle> stringBundle;
>+  nsCOMPtr<nsIStringBundleService> stringBundleService =
>+    mozilla::services::GetStringBundleService();

nit, mozilla:: shouldn't be needed.

>+  if (stringBundleService) {
>+    stringBundleService->CreateBundle("chrome://global-platform/locale/accessible.properties", 
>+                                      getter_AddRefs(stringBundle));
>+  }
>+
>   nsXPIDLString xsValue;
>-
>-  gStringBundle->GetStringFromName(PromiseFlatString(aKey).get(), getter_Copies(xsValue));
>-  aStringOut.Assign(xsValue);
>+  nsresult rv = stringBundle->GetStringFromName(aKey.get(), getter_Copies(xsValue));

consider what happens if you weren't successful in getting the service, its not a big deal since if that happened something is horribly wrong anyway but...
Attachment #621925 - Flags: feedback?(trev.saunders)
Re: consider what happens if you weren't successful in getting the service, its not a big deal since if that happened something is horribly wrong anyway but...

I mentioned this in comment4 ... the original code Creates the bundle in AccessNode but then uses it to get the string name in Accessible without error checking. 

So my code preserves state, but could be cleaner. Other nits I'll fix and repost.
(In reply to Mark Capella [:capella] from comment #13)
> Re: consider what happens if you weren't successful in getting the service,
> its not a big deal since if that happened something is horribly wrong anyway
> but...
> 
> I mentioned this in comment4 ... the original code Creates the bundle in
> AccessNode but then uses it to get the string name in Accessible without
> error checking. 
> 
> So my code preserves state, but could be cleaner. Other nits I'll fix and
> repost.

Alex asked you to add the null check in comment 3, and I'd agree since its easy.
Oh, I thought his request in comment was to ull check the result of GetStringFromName and yours was to also check the result of CreateBundle.

In either case, I'm fixing both.
Attached patch Patch (v5) (obsolete) — Splinter Review
Here's the revisions ....
Attachment #621925 - Attachment is obsolete: true
Attachment #622128 - Flags: feedback?(trev.saunders)
Comment on attachment 622128 [details] [diff] [review]
Patch (v5)

>+nsAccessible::TranslateString(const nsString& aKey, nsAString& aStringOut)
> {
>+  nsCOMPtr<nsIStringBundle> stringBundle;
>+  nsCOMPtr<nsIStringBundleService> stringBundleService =
>+    services::GetStringBundleService();
>+  if (stringBundleService) {
>+    stringBundleService->CreateBundle(
>+      "chrome://global-platform/locale/accessible.properties", 
>+      getter_AddRefs(stringBundle));
>+  }
>+  if (!stringBundle)
>+    return;

I'd probably make both early returns, but atleast a blank line between the } and the next if
Attachment #622128 - Flags: feedback?(trev.saunders) → feedback+
Attached patch Patch (v6)Splinter Review
Final nits addressed with early returns.
Attachment #622128 - Attachment is obsolete: true
Blocks: 748719
No longer blocks: 748719
Blocks: 748719
Comment on attachment 622341 [details] [diff] [review]
Patch (v6)

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

::: accessible/src/base/nsAccessible.cpp
@@ +636,5 @@
> +
> +  nsCOMPtr<nsIStringBundle> stringBundle;
> +  stringBundleService->CreateBundle(
> +    "chrome://global-platform/locale/accessible.properties", 
> +    getter_AddRefs(stringBundle));

nit: wrong indentation, I'll fix before landing
(In reply to alexander :surkov from comment #19)
> Comment on attachment 622341 [details] [diff] [review]
> Patch (v6)
> 
> Review of attachment 622341 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/base/nsAccessible.cpp
> @@ +636,5 @@
> > +
> > +  nsCOMPtr<nsIStringBundle> stringBundle;
> > +  stringBundleService->CreateBundle(
> > +    "chrome://global-platform/locale/accessible.properties", 
> > +    getter_AddRefs(stringBundle));
> 
> nit: wrong indentation, I'll fix before landing

(per irc) ignore that, " and g looks as they are at different offsets
https://hg.mozilla.org/mozilla-central/rev/806aed03eb1b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.