750295 - don't cache gStringBundle in nsAccessNode

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

move logic for getting the string bundle from nsAccessNode::InitXPAccessibility to nsAccessible::TranslateString() then remove gStringBundle stuff from accessible/src/base/nsAccessNode.cpp/h

Comment 1

[Mark Capella [:capella]]

Attachment: Patch (v1)

First patch, builds and tests ok locally... let me know what you think -- mark

Comment 2

[Mark Capella [:capella]]

Attachment: Patch (v2)

(Forgot to qrefresh the patch ...)

Comment 3

[Trevor Saunders (:tbsaunde)]

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.

Comment 4

[Mark Capella [:capella]]

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 5

[alexander :surkov (:asurkov)]

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).

Comment 6

[Trevor Saunders (:tbsaunde)]

> @@ +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

Comment 7

[alexander :surkov (:asurkov)]

(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.

Comment 8

[Mark Capella [:capella]]

Attachment: Patch (v3)

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 :)

Comment 9

[alexander :surkov (:asurkov)]

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)

Comment 10

[Mark Capella [:capella]]

Attachment: Patch (v4)

Comment 11

[Mark Capella [:capella]]

FYI
https://tbpl.mozilla.org/?tree=Try&rev=17852a7b64c2

Comment 12

[Trevor Saunders (:tbsaunde)]

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...

Comment 13

[Mark Capella [:capella]]

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.

Comment 14

[Trevor Saunders (:tbsaunde)]

(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.

Comment 15

[Mark Capella [:capella]]

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.

Comment 16

[Mark Capella [:capella]]

Attachment: Patch (v5)

Here's the revisions ....

Comment 17

[Trevor Saunders (:tbsaunde)]

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

Comment 18

[Mark Capella [:capella]]

Attachment: Patch (v6)

Final nits addressed with early returns.

Comment 19

[alexander :surkov (:asurkov)]

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

Comment 20

[alexander :surkov (:asurkov)]

(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

Comment 21

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/806aed03eb1b

Comment 22

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/806aed03eb1b