Last Comment Bug 660742 - accessible should use mozilla::Preferences
: accessible should use mozilla::Preferences
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
:
Mentors:
Depends on: 656826
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-30 21:22 PDT by Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
Modified: 2011-09-01 08:29 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1.0 (7.02 KB, patch)
2011-05-30 21:22 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
roc: review+
dbolter: review+
Details | Diff | Review
Patch v1.1 (7.79 KB, patch)
2011-06-02 07:09 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
Patch v1.2 (7.80 KB, patch)
2011-06-02 07:19 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
roc: review+
dbolter: review+
Details | Diff | Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-05-30 21:22:31 PDT
Created attachment 536224 [details] [diff] [review]
Patch v1.0
Comment 1 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-30 22:06:23 PDT
Comment on attachment 536224 [details] [diff] [review]
Patch v1.0

Review of attachment 536224 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 2 Blake Winton (:bwinton) (:☕️) 2011-05-31 16:51:15 PDT
Comment on attachment 536224 [details] [diff] [review]
Patch v1.0

I'm _really_ not the right person for this review.  Perhaps David Bolter would be a better choice (or would re-direct it to someone more useful).

Thanks,
Blake.
Comment 3 David Bolter [:davidb] 2011-05-31 18:31:57 PDT
Comment on attachment 536224 [details] [diff] [review]
Patch v1.0

r=me. nice. thanks.
Comment 4 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-06-01 18:09:52 PDT
http://hg.mozilla.org/mozilla-central/rev/f81b4d9534f5
Comment 5 :Ms2ger 2011-06-02 04:19:39 PDT
Comment on attachment 536224 [details] [diff] [review]
Patch v1.0

>diff --git a/accessible/src/base/nsAccessible.cpp b/accessible/src/base/nsAccessible.cpp
>--- a/accessible/src/base/nsAccessible.cpp
>+++ b/accessible/src/base/nsAccessible.cpp
>   // use ui.key.generalAccessKey (unless it is -1)
>-  PRInt32 accessKey;
>-  nsresult rv = prefBranch->GetIntPref("ui.key.generalAccessKey", &accessKey);
>+  PRInt32 accessKey = -1;
>+  nsresult rv = Preferences::GetInt("ui.key.generalAccessKey", accessKey);

This is wrong, isn't it? You probably wanted to pass &accessKey as that last parameter.
Comment 6 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-06-02 05:28:45 PDT
(In reply to comment #5)
> Comment on attachment 536224 [details] [diff] [review] [review]
> Patch v1.0
> 
> >diff --git a/accessible/src/base/nsAccessible.cpp b/accessible/src/base/nsAccessible.cpp
> >--- a/accessible/src/base/nsAccessible.cpp
> >+++ b/accessible/src/base/nsAccessible.cpp
> >   // use ui.key.generalAccessKey (unless it is -1)
> >-  PRInt32 accessKey;
> >-  nsresult rv = prefBranch->GetIntPref("ui.key.generalAccessKey", &accessKey);
> >+  PRInt32 accessKey = -1;
> >+  nsresult rv = Preferences::GetInt("ui.key.generalAccessKey", accessKey);
> 
> This is wrong, isn't it? You probably wanted to pass &accessKey as that last
> parameter.

Wow, exactly. I'll back it out.
Comment 7 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-06-02 05:33:26 PDT
backed out.
http://hg.mozilla.org/mozilla-central/rev/d011353d1719
Comment 8 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-06-02 07:09:57 PDT
Created attachment 536877 [details] [diff] [review]
Patch v1.1

>  GetAccessModifierMask(nsIContent* aContent)
>  {
> -  nsCOMPtr<nsIPrefBranch> prefBranch =
> -    do_GetService(NS_PREFSERVICE_CONTRACTID);
> -  if (!prefBranch)
> -    return 0;
> -
>    // use ui.key.generalAccessKey (unless it is -1)
> -  PRInt32 accessKey;
> -  nsresult rv = prefBranch->GetIntPref("ui.key.generalAccessKey", &accessKey);
> -  if (NS_SUCCEEDED(rv) && accessKey != -1) {
> -    switch (accessKey) {
> -      case nsIDOMKeyEvent::DOM_VK_SHIFT:   return NS_MODIFIER_SHIFT;
> -      case nsIDOMKeyEvent::DOM_VK_CONTROL: return NS_MODIFIER_CONTROL;
> -      case nsIDOMKeyEvent::DOM_VK_ALT:     return NS_MODIFIER_ALT;
> -      case nsIDOMKeyEvent::DOM_VK_META:    return NS_MODIFIER_META;
> -      default:                             return 0;
> -    }
> +  switch (Preferences::GetInt("ui.key.generalAccessKey", -1)) {
> +    case -1:                             break;
> +    case nsIDOMKeyEvent::DOM_VK_SHIFT:   return NS_MODIFIER_SHIFT;
> +    case nsIDOMKeyEvent::DOM_VK_CONTROL: return NS_MODIFIER_CONTROL;
> +    case nsIDOMKeyEvent::DOM_VK_ALT:     return NS_MODIFIER_ALT;
> +    case nsIDOMKeyEvent::DOM_VK_META:    return NS_MODIFIER_META;
> +    default:                             return 0;
>    }

Maybe, I broke when I changed here... Sorry for the mistake.
Comment 9 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-06-02 07:19:58 PDT
Created attachment 536878 [details] [diff] [review]
Patch v1.2

Oops, No. This is correct.

>   // use ui.key.generalAccessKey (unless it is -1)
>   switch (Preferences::GetInt("ui.key.generalAccessKey", -1)) {
>     case -1:                             break;
>     case nsIDOMKeyEvent::DOM_VK_SHIFT:   return NS_MODIFIER_SHIFT;
>     case nsIDOMKeyEvent::DOM_VK_CONTROL: return NS_MODIFIER_CONTROL;
>     case nsIDOMKeyEvent::DOM_VK_ALT:     return NS_MODIFIER_ALT;
>     case nsIDOMKeyEvent::DOM_VK_META:    return NS_MODIFIER_META;
>     default:                             return 0;
>   }
> 
>   // get the docShell to this DOMNode, return 0 on failure
>   nsCOMPtr<nsIDocument> document = aContent->GetCurrentDoc();
>   if (!document)
>     return 0;
>   nsCOMPtr<nsISupports> container = document->GetContainer();
>   if (!container)
>     return 0;
>   nsCOMPtr<nsIDocShellTreeItem> treeItem(do_QueryInterface(container));
>   if (!treeItem)
>     return 0;
> 
>   // determine the access modifier used in this context
>   nsresult rv = NS_ERROR_FAILURE;

initial value of the rv must be error here.

I hate carried over rv!
Comment 10 David Bolter [:davidb] 2011-06-02 07:20:40 PDT
Note: please always get a clean try server run before landing, and/or explicitly ask me to build and test when requesting review.
Comment 11 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-06-02 07:28:18 PDT
(In reply to comment #10)
> Note: please always get a clean try server run before landing, and/or
> explicitly ask me to build and test when requesting review.

I have question, how do I test the GetAccessModifierMask() manually? The mistake wasn't detected by automated tests.
Comment 12 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-06-02 10:15:06 PDT
Thank you, David. I succeeded to test the patch by your advice on IRC.

But I couldn't test nsAccessNodeWrap::TurnOffNewTabSwitchingForJawsAndWE() because I couldn't find a way to call it. But the change in it is very simple. And I confirmed the new APIs are work fine. So, I think they are okay.
Comment 13 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-06-02 10:17:16 PDT
FYI: I'm also testing on tryserver again.
http://tbpl.mozilla.org/?tree=Try&rev=5021b7cfeea1
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-02 15:02:33 PDT
Comment on attachment 536878 [details] [diff] [review]
Patch v1.2

Review of attachment 536878 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 15 David Bolter [:davidb] 2011-06-02 18:34:52 PDT
Comment on attachment 536878 [details] [diff] [review]
Patch v1.2

r=me if the try run looks good. thanks.
Comment 16 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-06-02 23:04:38 PDT
http://hg.mozilla.org/mozilla-central/rev/5b0986336b74
Comment 17 Simona B [:simonab] 2011-08-31 08:09:47 PDT
Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0

Is there any way I can verify this on the QA side? I visually inspected the code changes in the repo:
http://hg.mozilla.org/mozilla-central/file/922f27baed98

Is that enough to mark this as Verified Fixed?
Comment 18 alexander :surkov 2011-08-31 08:31:53 PDT
(In reply to Simona B [QA] from comment #17)
> Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0
> 
> Is there any way I can verify this on the QA side?

No.

> I visually inspected the
> code changes in the repo:
> http://hg.mozilla.org/mozilla-central/file/922f27baed98
> 
> Is that enough to mark this as Verified Fixed?

yes

Simona, thank you for looking at a11y bugs but curious whether QA needs to run through all fixed bugs for some release and verify them, especially those that are just code cleanings-up?
Comment 19 alexander :surkov 2011-08-31 08:37:19 PDT
(In reply to alexander surkov from comment #18)

> Simona, thank you for looking at a11y bugs but curious whether QA needs to
> run through all fixed bugs for some release and verify them, especially
> those that are just code cleanings-up?

I'm asking because if general practice is bugs verification (which is great of course) but obviously you shouldn't spend your time for some of them then maybe we have (or should have) whiteboard status (like needs-verification) to point this out to keep your work easier?
Comment 20 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-01 07:48:22 PDT
(In reply to alexander surkov from comment #18)
> (In reply to Simona B [QA] from comment #17)
> > Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0
> > 
> > Is there any way I can verify this on the QA side?
> 
> No.
> 
> > I visually inspected the
> > code changes in the repo:
> > http://hg.mozilla.org/mozilla-central/file/922f27baed98
> > 
> > Is that enough to mark this as Verified Fixed?
> 
> yes
> 
> Simona, thank you for looking at a11y bugs but curious whether QA needs to
> run through all fixed bugs for some release and verify them, especially
> those that are just code cleanings-up?

@Alex, it has been communicated to the QA teams to skip verifying fixes which are obvious code-level bugs and to only focus on broken/improved features/functional bugs. Adding a whiteboard tag would certainly make filtering easier but "most" devs are resistant to use yet another whiteboard tag. The solution here is to just educate ourselves about what NEEDS to get verified and what doesn't.

Thanks.
Comment 21 alexander :surkov 2011-09-01 08:29:15 PDT
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #20)

> @Alex, it has been communicated to the QA teams to skip verifying fixes
> which are obvious code-level bugs and to only focus on broken/improved
> features/functional bugs. Adding a whiteboard tag would certainly make
> filtering easier but "most" devs are resistant to use yet another whiteboard
> tag. The solution here is to just educate ourselves about what NEEDS to get
> verified and what doesn't.

Ok, fair enough. Either way has benefits and disadvantages. Thank you.

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