Last Comment Bug 750295 - don't cache gStringBundle in nsAccessNode
: don't cache gStringBundle in nsAccessNode
Status: RESOLVED FIXED
[good first bug][mentor=trev.saunders...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Mark Capella [:capella]
:
Mentors:
Depends on: 750287
Blocks: 748719
  Show dependency treegraph
 
Reported: 2012-04-30 08:51 PDT by Trevor Saunders (:tbsaunde)
Modified: 2012-05-10 18:42 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (4.30 KB, patch)
2012-05-06 05:05 PDT, Mark Capella [:capella]
no flags Details | Diff | Review
Patch (v2) (4.30 KB, patch)
2012-05-06 05:09 PDT, Mark Capella [:capella]
no flags Details | Diff | Review
Patch (v3) (7.26 KB, patch)
2012-05-07 10:01 PDT, Mark Capella [:capella]
surkov.alexander: review+
Details | Diff | Review
Patch (v4) (8.28 KB, patch)
2012-05-08 03:07 PDT, Mark Capella [:capella]
no flags Details | Diff | Review
Patch (v5) (9.08 KB, patch)
2012-05-08 13:39 PDT, Mark Capella [:capella]
tbsaunde+mozbugs: feedback+
Details | Diff | Review
Patch (v6) (9.08 KB, patch)
2012-05-09 04:17 PDT, Mark Capella [:capella]
no flags Details | Diff | Review

Description Trevor Saunders (:tbsaunde) 2012-04-30 08:51:58 PDT
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] 2012-05-06 05:05:33 PDT
Created attachment 621405 [details] [diff] [review]
Patch (v1)

First patch, builds and tests ok locally... let me know what you think -- mark
Comment 2 Mark Capella [:capella] 2012-05-06 05:09:31 PDT
Created attachment 621406 [details] [diff] [review]
Patch (v2)

(Forgot to qrefresh the patch ...)
Comment 3 Trevor Saunders (:tbsaunde) 2012-05-06 23:42:51 PDT
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] 2012-05-07 00:01:23 PDT
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 2012-05-07 02:24:49 PDT
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) 2012-05-07 06:20:43 PDT
> @@ +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 2012-05-07 07:56:32 PDT
(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] 2012-05-07 10:01:07 PDT
Created attachment 621643 [details] [diff] [review]
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 2012-05-08 00:49:05 PDT
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] 2012-05-08 03:07:24 PDT
Created attachment 621925 [details] [diff] [review]
Patch (v4)
Comment 11 Mark Capella [:capella] 2012-05-08 04:01:04 PDT
FYI
https://tbpl.mozilla.org/?tree=Try&rev=17852a7b64c2
Comment 12 Trevor Saunders (:tbsaunde) 2012-05-08 07:26:07 PDT
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] 2012-05-08 07:44:30 PDT
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) 2012-05-08 08:09:35 PDT
(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] 2012-05-08 08:13:57 PDT
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] 2012-05-08 13:39:21 PDT
Created attachment 622128 [details] [diff] [review]
Patch (v5)

Here's the revisions ....
Comment 17 Trevor Saunders (:tbsaunde) 2012-05-08 16:51:06 PDT
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] 2012-05-09 04:17:21 PDT
Created attachment 622341 [details] [diff] [review]
Patch (v6)

Final nits addressed with early returns.
Comment 19 alexander :surkov 2012-05-10 03:49:13 PDT
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 2012-05-10 03:56:51 PDT
(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 22 Joe Drew (not getting mail) 2012-05-10 18:42:37 PDT
https://hg.mozilla.org/mozilla-central/rev/806aed03eb1b

Note You need to log in before you can comment on or make changes to this bug.