Closed Bug 456121 Opened 16 years ago Closed 16 years ago

nsApplicationAccessible::GetName does not return a default value when brand.properties does not exist

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: arno, Assigned: arno)

Details

(Keywords: access)

Attachments

(1 file, 4 obsolete files)

updated mochitest (again0
4.63 KB, patch
MarcoZ
: review+
Details | Diff | Splinter Review 
Hi,

nsApplicationAccessible::GetName returns application brandShortName (as defined in chrome://branding/locale/brand.properties). But if brand.properties does not exist, or if brandShortName is not defined, no default value is returned.


after calling

bundleService->CreateBundle("chrome://branding/locale/brand.properties",  getter_AddRefs(bundle));

function tests

 if (bundle) {
}

but createBundle creates a bundle even if file does not exist (I was told on irc that url is loaded lazily).

Instead, it would be better to return value of  bundle->GetStringFromName. I'll attach a patch proposal.

Because of that problem, ldtp[1] does not work with an application without a brand.properties file.

[1]: http://ldtp.freedesktop.org/wiki/
Keywords: access
I could not think of an automated test. Does someone has an idea ?
Attachment #339526 - Flags: review?(aaronleventhal)
(In reply to comment #1)
> Created an attachment (id=339526) [details]
> test for bundle->GetStringFromName return value
> 
> I could not think of an automated test. Does someone has an idea ?

What kind of difficulties do you see with mochitests?
(In reply to comment #2)

> What kind of difficulties do you see with mochitests?

Firstly, because I don't known how to get nsIAccessible related to application. Then, because brandShortName is defined in mochitests, so I cannot test the case when it's not defined.
(In reply to comment #3)
> (In reply to comment #2)
> 
> > What kind of difficulties do you see with mochitests?
> 
> Firstly, because I don't known how to get nsIAccessible related to application.

You could get any accessible by DOM node (look at getAccessible() function in chrome://mochikit/content/a11y/accessible/common.js) and get parents until you get null. The topmost parent will be root accessible.

> Then, because brandShortName is defined in mochitests, so I cannot test the
> case when it's not defined.

When brandShortName is not defined?
>When brandShortName is not defined?

This can happen if someone writing an application based on the Mozilla platform, decides not to put that brandName in.
Attachment #339526 - Flags: review?(aaronleventhal) → review?(surkov.alexander)
Comment on attachment 339526 [details] [diff] [review]
test for bundle->GetStringFromName return value

Alex, I don't see a problem with the patch, but you go ahead and review since you had some questions.
Comment on attachment 339526 [details] [diff] [review]
test for bundle->GetStringFromName return value

><html><body><pre style="word-wrap: break-word; white-space: pre-wrap;">diff -r b375e5cab4dc accessible/src/base/nsApplicationAccessible.cpp
>--- a/accessible/src/base/nsApplicationAccessible.cpp	Wed Sep 17 22:53:09 2008 +0200
>+++ b/accessible/src/base/nsApplicationAccessible.cpp	Sat Sep 20 00:26:24 2008 +0200
>@@ -109,25 +109,25 @@ nsApplicationAccessible::GetName(nsAStri
> 
>   nsCOMPtr<nsIStringBundleService> bundleService =
>     do_GetService(NS_STRINGBUNDLE_CONTRACTID);
> 
>   NS_ASSERTION(bundleService, "String bundle service must be present!");
>   NS_ENSURE_STATE(bundleService);
> 
>   nsCOMPtr<nsIStringBundle> bundle;
>-  bundleService->CreateBundle("chrome://branding/locale/brand.properties",
>+  nsresult rv = bundleService->CreateBundle("chrome://branding/locale/brand.properties",
>                               getter_AddRefs(bundle));

nit: please make right indent

>+  NS_ENSURE_SUCCESS(rv, rv);
> 
>   nsXPIDLString appName;
>-  if (bundle) {
>-    bundle->GetStringFromName(NS_LITERAL_STRING("brandShortName").get(),
>+  rv = bundle->GetStringFromName(NS_LITERAL_STRING("brandShortName").get(),
>                               getter_Copies(appName));

nit: the same

>-  } else {
>-    NS_WARNING("brand.properties not present, using default app name");
>+  if (NS_FAILED(rv) || !appName) {

appName.IsEmpty() ?
please add mochitests also:
1) get root accessibles and get the name
2) get properties and compare it with the name
Comment on attachment 339526 [details] [diff] [review]
test for bundle->GetStringFromName return value


>+    NS_WARNING("brandShortName not found, using default app name");

>     appName.AssignLiteral("Mozilla");

possibly "Gecko based application" because it seems every application should define brand properties.

canceling review until comments are addressed
Attachment #339526 - Flags: review?(surkov.alexander)
Attached patch patch v2 (obsolete) — — Splinter Review 
> possibly "Gecko based application" because it seems every application should
> define brand properties.

Yes, every application should define brand properties. But application works fine without  defining apart a few minor things. So, I had an application with brand properties, and I was wondering why it didn't appear in at-poke. So I supposed it can happen to other people, and may be a fallback is therefore a good idea.

Also, should "Gecko based application" be localized ?
Attachment #339526 - Attachment is obsolete: true
Attachment #339826 - Flags: review?(surkov.alexander)
(In reply to comment #10)

> Also, should "Gecko based application" be localized ?

I think it should be but let's cc Alex and Neil to see what they think.
Comment on attachment 339826 [details] [diff] [review]
patch v2


> 		moz.png \
> 		longdesc_src.html \
> 		common.js \
> 		nsIAccessible_actions.js \
> 		nsIAccessible_name.css \
> 		nsIAccessible_name.js \
> 		nsIAccessible_name.xbl \
> 		nsIAccessibleEditableText.js \
>+		test_application_name.html \

possibly test_nsIAccessible_applicationacc.html or something like this? Marco, what do you think?

>+<head>
>+  <title>application accessible name</title>
>+  <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css" />
>+  <script type="application/javascript" src="chrome://mochikit/content/MochiKit/packed.js"></script>
>+  <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>
>+

please include common.js file

>+  <script type="application/javascript">
>+    function doTest()
>+    {
>+        var bundleServ = Components.classes["@mozilla.org/intl/stringbundle;1"].getService(Components.interfaces.nsIStringBundleService);
>+        var bundle = bundleServ.createBundle("chrome://branding/locale/brand.properties");
>+        var brandShortName = bundle.GetStringFromName("brandShortName");
>+
>+        var retrieval = Components.classes["@mozilla.org/accessibleRetrieval;1"].getService(Components.interfaces.nsIAccessibleRetrieval);
>+        var accessible = retrieval.getAccessibleFor(document);

just use getAccessible(document) defined in common.js

>+        while (accessible.parent) {
>+            accessible = accessible.parent;
>+        }
>+        is (accessible.name, brandShortName);

should you write

if (! brandShortName)
  is (accessible.name, "Gecko based application")

for the case when "gecko based application" will run our mochitests?
Attachment #339826 - Flags: review?(marco.zehe)
Comment on attachment 339826 [details] [diff] [review]
patch v2

r=me
Attachment #339826 - Flags: review?(surkov.alexander) → review+
Comment on attachment 339826 [details] [diff] [review]
patch v2

In addition to what Alexander said:
1. Please ensure that the bundle is being created successfully by checking if the bundle is anything other than "null".
2. In the BODY section, please leave the infrastructure in place that is found in other mochitest files (everything up to the closing PRE tag). Otherwise, the tests won't be able to generate proper output.
3. The file name should indeed be something like test_nsApplicationAccessible_name.html, so anyone coming into this folder can see what class is being tested.
Attachment #339826 - Flags: review?(marco.zehe) → review-
(In reply to comment #14)

> test_nsApplicationAccessible_name.html, so anyone coming into this folder can
> see what class is being tested.

btw, we have test_nsIAccessible_name to test nsIAccessible.name attribute so we could use this file to test application accessible name. But if we don't want this then I think we should follow names like test_nsIAccessible_selects or test_nsIAccessible_editablebody, so from this point it should be test_nsIAccessible_applicationAccessible or test_nsIAccessible_applicationAcc or something similar.
I don't think we should localize this. The right way to expose this name to l10n is to ship a brand.properties, if app-authors like their stuff to be broken, we shouldn't half-way fix it.

I wonder, how verbose is the NS_WARNING? If someone develops a xulrunner app with the sdk, are they ever going to see that warning?
Attached patch patch v2 + better mochitest (obsolete) — — Splinter Review 
I've update mochitest:
name is test_nsIAccessibleApplication_name.html
and it fall backs to "Gecko based application" if stringbundle cannot be resolved.

> I wonder, how verbose is the NS_WARNING? If someone develops a xulrunner app
> with the sdk, are they ever going to see that warning?

NS_WARNING does nothing on non debug builds. So may be it's not useful here.
http://mxr.mozilla.org/mozilla/source/xpcom/glue/nsDebug.h#169
Attachment #339826 - Attachment is obsolete: true
Attachment #339998 - Flags: review?(surkov.alexander)
Comment on attachment 339998 [details] [diff] [review]
patch v2 + better mochitest


> nsApplicationAccessible::GetRole(PRUint32 *aRole)
>diff -r ade9f6b95169 accessible/tests/mochitest/Makefile.in
>--- a/accessible/tests/mochitest/Makefile.in	Mon Sep 22 23:07:21 2008 +0100
>+++ b/accessible/tests/mochitest/Makefile.in	Tue Sep 23 22:04:46 2008 +0200
>@@ -64,16 +64,17 @@ _TEST_FILES =\
> 		test_groupattrs.xul \
> 	$(warning test_table_indexes.html temporarily disabled) \
> 		test_nsIAccessible_actions.html \
> 		test_nsIAccessible_actions.xul \
> 		test_nsIAccessible_name.html \
> 		test_nsIAccessible_name.xul \
>  		test_nsIAccessible_selects.html \
> 		test_nsIAccessible_focus.html \
>+		test_nsIAccessibleApplication_name.html \

concerning the name, we don't have nsIAccessibleApplication, see comment #15

>+<head>
>+  <title>application accessible name</title>
>+  <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css" />
>+  <script type="application/javascript" src="chrome://mochikit/content/MochiKit/packed.js"></script>
>+  <script type="application/javascript" src="chrome://mochikit/content/a11y/accessible/common.js" ></script>

nit: move down common.js, put if after SimpleText.js

>+  <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>

>+
>+        var bundleServ = Components.classes["@mozilla.org/intl/stringbundle;1"].getService(Components.interfaces.nsIStringBundleService);
>+        var bundle = bundleServ.createBundle("chrome://branding/locale/brand.properties");
>+        try {
>+            var appliAccessName = bundle.GetStringFromName("brandShortName");

define appliAccessName variable before try/catch expression

>+        }  catch(e) {
>+            var appliAccessName = "Gecko based application";

in GetName() we use a bit different logic, as well we check empty string
Attachment #339998 - Flags: review?(surkov.alexander)
Attachment #339998 - Flags: review?(marco.zehe)
Attachment #339998 - Flags: review+
Comment on attachment 339998 [details] [diff] [review]
patch v2 + better mochitest

>+  <script type="application/javascript" src="chrome://mochikit/content/MochiKit/packed.js"></script>
>+  <script type="application/javascript" src="chrome://mochikit/content/a11y/accessible/common.js" ></script>
>+  <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>

Nit: Please make sure lines are not longer than 80 characters. Please put the SRC attribute on the next line and align with "type".

>+        var accessible = getAccessible(document);
>+        while (accessible.parent) {

Please check if acc is actually not NULL and give an error message if the accessible was for some reason not successfully retrieved.
if (!accessible) {
ok(false, "No document accessible obtained!");
simpleTest.finish();
}

>+        is (accessible.name, appliAccessName);

Please provide an error message as the third parameter of the "is" function. Otherwise if the test fails, people won't know what the test is about.
Attachment #339998 - Flags: review?(marco.zehe) → review-
Also, please define the variable that holds either the bundle property or the default string outside of the try...except block and initialize it with an empty string "". Then, simply do the two assignments. Since you are then using the variable outside that try...except block, scoping is cleaner.
(In reply to comment #19)

> 
> >+        var accessible = getAccessible(document);
> >+        while (accessible.parent) {
> 
> Please check if acc is actually not NULL and give an error message if the
> accessible was for some reason not successfully retrieved.
> if (!accessible) {
> ok(false, "No document accessible obtained!");
> simpleTest.finish();
> }

Actually I can't see a case when there is no accessible for document but if we want to check this anyway then you should use
if (!accessible) {
  SimpleTest.finish();
  return;
}
because getAccessible() will call ok() function if there is no accessible.

Also, since accessible.parent may throw an exception (though I believe it's not our case) then you can protect yourself by getAccessible(accessible.parent) as well.
btw, I'm not sure "appliAccessName" is really good name, probably just applicationName?
(In reply to comment #11)
> (In reply to comment #10)
> > Also, should "Gecko based application" be localized ?
> I think it should be but let's cc Alex and Neil to see what they think.
Localising a missing localisation string sounds wrong ;-)

How about using the user agent string?
Thanks for fixing this bug.
I found this bug some days ago, but I forgot to file it. Sorry.

But if brand.properties does not exist, properly you running a application that embeds Gecko, e.g. yelp.
It was wrong that nsApplicationAccessible::GetName is being called.
Because the application is yelp not Gecko.
For a11y hierarchy, Gecko is just a child of a widget. We should not hijack atk_get_root().
It's not easy to fix.
On Gecko side, some events of nsRootAccessible/nsDocAccessible relies on its parent, nsApplicationAccessible.
How do we know it is being embedded? 
Perhaps we need to a special case for GetParent().

I expect similar issues on Windows, but I never tried it.
Bug 456722 was filed for comment #24.
Attached patch updated mochitest (obsolete) — — Splinter Review 

        Attachment #339998 -
        Attachment is obsolete: true

        Attachment #340325 -
        Flags: review?

        Attachment #340325 -
        Flags: review? → review?(marco.zehe)

    
    

    
  
Comment on attachment 340325 [details] [diff] [review]
updated mochitest


>+
>+<head>
>+  <title>application accessible name</title>
>+  <link rel="stylesheet" type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css" />
>+  <script type="application/javascript" 
>+          src="chrome://mochikit/content/MochiKit/packed.js"></script>
>+  <script type="application/javascript" 
>+          src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>

nit: new line please

>+  <script type="application/javascript" 
>+          src="chrome://mochikit/content/a11y/accessible/common.js" ></script>
>+
>+  <script type="application/javascript">
>+    function doTest()
>+    {
>+        var accessible = getAccessible(document);
>+        while (accessible && accessible.parent) {
>+            accessible = accessible.parent;
>+        }
>+
>+        if (!accessible) {
>+            simpleTest.finish();

SimpleTest, not simpeTest I think



>+            return;
>+        }
>+
>+        var bundleServ = Components.classes["@mozilla.org/intl/stringbundle;1"]
>+                         .getService(Components.interfaces.nsIStringBundleService);
>+        var bundle = bundleServ.createBundle("chrome://branding/locale/brand.properties");
>+
>+        var appliAccessName = "";
>+

nit: I would like more "applicationName" rather than "appliAccessName" because "appli" isn't associated with "application" for me.

    
    

    
  
(In reply to comment #23)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > Also, should "Gecko based application" be localized ?
> > I think it should be but let's cc Alex and Neil to see what they think.
> Localising a missing localisation string sounds wrong ;-)
> 
> How about using the user agent string?

that's possibly better. Marco, what do you think?

    
    

    
  
I think the user agent string is OK. But be aware that, at least for Firefox and Thunderbird, this user agent string is dependent on the locale. So, a German Firefox produces a different user agent string than an English one AFAIK.

    
  

        Attachment #340325 -
        Flags: review?(marco.zehe) → review+

    
    

    
  
Comment on attachment 340325 [details] [diff] [review]
updated mochitest

Two nits:
>+        var appliAccessName = "";

As Alexander said, this name is a bit confusing. AppAccessName would be OK. App is a common abbreviation for application.

>+        if (appliAccessName === "")

One = too many.

With that fixed, r=me.

    
    

    
  


  

        Attachment #340325 -
        Attachment is obsolete: true

        Attachment #340994 -
        Flags: review?

    
  

        Attachment #340994 -
        Flags: review? → review?(surkov.alexander)

    
    

    
  
Comment on attachment 340994 [details] [diff] [review]
updated mochitest (again0

r=me. Thanks!

        Attachment #340994 -
        Flags: review?(surkov.alexander) → review+

    
    

    
  
who should I ask for super review ?

    
    

    
  
Arno, no sr+ needed for this module.

    
    

    
  
I'm waiting for the tree to reopen after beta 1 was tagged. Once that's done, I will check this one in for Arno.

Arno, what's your last name? In the checkin comment, I need to supply the full name of the patch contributor. Thanks!

    
  
Keywords: checkin-needed

    
    

    
  
arno renevier

    
    

    
  
(In reply to comment #35)
> In the checkin comment, I need to supply the full name of the patch contributor.
You mean 'hg commit -u "contributor <address@domain>"' right? ;-)

    
    

    
  
(In reply to comment #37)
> (In reply to comment #35)
> > In the checkin comment, I need to supply the full name of the patch contributor.
> You mean 'hg commit -u "contributor <address@domain>"' right? ;-)

Indeed, that's what I meant! I just didn't want to make it too complicated for those not familiar with HG that far in. :-)
Flags: in-testsuite+

    
    

    
  
checked in, http://hg.mozilla.org/mozilla-central/rev/05ebcc185813
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: