The default bug view has changed. See this FAQ.

don't cache gStringBundle in nsAccessNode

RESOLVED FIXED in mozilla15

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: tbsaunde, Assigned: capella)

Tracking

unspecified
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

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

Comment 1

5 years ago
Created attachment 621405 [details] [diff] [review]
Patch (v1)

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)
(Assignee)

Updated

5 years ago
Depends on: 750287
(Assignee)

Comment 2

5 years ago
Created attachment 621406 [details] [diff] [review]
Patch (v2)

(Forgot to qrefresh the patch ...)
Attachment #621405 - Attachment is obsolete: true
Attachment #621405 - Flags: feedback?(trev.saunders)
Attachment #621406 - Flags: feedback?(trev.saunders)
(Reporter)

Comment 3

5 years ago
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)
(Assignee)

Comment 4

5 years ago
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

5 years ago
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).
(Reporter)

Comment 6

5 years ago
> @@ +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

5 years ago
(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.
(Assignee)

Comment 8

5 years ago
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 :)
Attachment #621406 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #621643 - Flags: review?(surkov.alexander)

Updated

5 years ago
Attachment #621643 - Attachment is patch: true

Comment 9

5 years ago
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+
(Assignee)

Comment 10

5 years ago
Created attachment 621925 [details] [diff] [review]
Patch (v4)
Attachment #621643 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #621925 - Flags: feedback?(trev.saunders)
(Assignee)

Comment 11

5 years ago
FYI
https://tbpl.mozilla.org/?tree=Try&rev=17852a7b64c2
(Reporter)

Comment 12

5 years ago
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)
(Assignee)

Comment 13

5 years ago
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.
(Reporter)

Comment 14

5 years ago
(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.
(Assignee)

Comment 15

5 years ago
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.
(Assignee)

Comment 16

5 years ago
Created attachment 622128 [details] [diff] [review]
Patch (v5)

Here's the revisions ....
Attachment #621925 - Attachment is obsolete: true
Attachment #622128 - Flags: feedback?(trev.saunders)
(Reporter)

Comment 17

5 years ago
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+
(Assignee)

Comment 18

5 years ago
Created attachment 622341 [details] [diff] [review]
Patch (v6)

Final nits addressed with early returns.
Attachment #622128 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Blocks: 748719
(Assignee)

Updated

5 years ago
No longer blocks: 748719
(Assignee)

Updated

5 years ago
Blocks: 748719

Comment 19

5 years ago
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

5 years ago
(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

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/806aed03eb1b
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/806aed03eb1b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.