750287 - don't cache is form fill enabled pref

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

remove the code in accessible/src/base/nsAccessNode.cpp/h that has to do with gIsFormFillEnabled and in accessible/src/html/HTMLFormControlAccessible.cpp just check preferences::GetBoolPref() directly.

Comment 1

[Mark Capella [:capella]]

Attachment: Patch (v1)

Initial patch attempt ... builds and tests locally.

Comment 2

[Trevor Saunders (:tbsaunde)]

Comment on attachment 620564 [details] [diff] [review]
Patch (v1)

>+#include "nsIPrefService.h"
> #include "nsISelectionController.h"
> #include "jsapi.h"
>+  if (mParent) {
>+    bool isFormFillEnabled = false;
>+    nsCOMPtr<nsIPrefBranch> prefBranch(do_GetService(NS_PREFSERVICE_CONTRACTID));
>+    if (prefBranch)
>+      prefBranch->GetBoolPref("browser.formfill.enable", &isFormFillEnabled);

since your touching this exact code already use mozilla::preferneces please, if nothing else it'll make the changes here easier to be sure are correct.

Comment 3

[Mark Capella [:capella]]

Attachment: Patch (v2)

Cool tip! Much cleaner code.

Comment 4

[Trevor Saunders (:tbsaunde)]

Comment on attachment 620687 [details] [diff] [review]
Patch (v2)

>   // is unattached from tree then we don't care.
>-  if (mParent && gIsFormFillEnabled) {
>+  if (mParent && mozilla::Preferences::GetBool("browser.formfill.enable", true)) {

add using namespace mozilla then you can just do preferences::

also keep the defautl as false.

>     // Check to see if autocompletion is allowed on this input. We don't expose
>     // it for password fields even though the entire password can be remembered
>     // for a page if the user asks it to be. However, the kind of autocomplete
>     // we're talking here is based on what the user types, where a popup of
>     // possible choices comes up.
>     nsAutoString autocomplete;
>     mContent->GetAttr(kNameSpaceID_None, nsGkAtoms::autocomplete,
>                       autocomplete);

Comment 5

[alexander :surkov (:asurkov)]

Mark, are you going to file new patch addressing comments or should I fix them before landing?

Comment 6

[Mark Capella [:capella]]

I'll fix this one .... I'm working you hard enough already   :)

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Mark Capella [:capella] from comment #6)
> I'll fix this one .... I'm working you hard enough already   :)

thank you. and it'd be great if you get an access to land your patches yourself. it'll be a big help also :)

Comment 8

[Mark Capella [:capella]]

Attachment: Patch (v3)

Final nits addressed and re-tested ...

Comment 9

[alexander :surkov (:asurkov)]

Comment on attachment 621666 [details] [diff] [review]
Patch (v3)

Review of attachment 621666 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/html/HTMLFormControlAccessible.cpp
@@ +498,5 @@
>      return state | states::SUPPORTS_AUTOCOMPLETION;
>  
>    // No parent can mean a fake widget created for XUL textbox. If accessible
>    // is unattached from tree then we don't care.
> +  if (mParent && Preferences::GetBool("browser.formfill.enable", false)) {

you don't need to pass false since it's value of default argument, I'll fix it

Comment 10

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87bc642d7328

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/87bc642d7328