Last Comment Bug 750287 - don't cache is form fill enabled pref
: don't cache is form fill enabled pref
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: 750283
Blocks: 750295
  Show dependency treegraph
 
Reported: 2012-04-30 08:38 PDT by Trevor Saunders (:tbsaunde)
Modified: 2012-05-08 11:17 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (6.16 KB, patch)
2012-05-02 19:30 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v2) (4.47 KB, patch)
2012-05-03 06:53 PDT, Mark Capella [:capella]
tbsaunde+mozbugs: feedback+
Details | Diff | Splinter Review
Patch (v3) (4.49 KB, patch)
2012-05-07 10:53 PDT, Mark Capella [:capella]
surkov.alexander: review+
Details | Diff | Splinter Review

Description Trevor Saunders (:tbsaunde) 2012-04-30 08:38:08 PDT
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] 2012-05-02 19:30:21 PDT
Created attachment 620564 [details] [diff] [review]
Patch (v1)

Initial patch attempt ... builds and tests locally.
Comment 2 Trevor Saunders (:tbsaunde) 2012-05-03 04:09:32 PDT
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] 2012-05-03 06:53:12 PDT
Created attachment 620687 [details] [diff] [review]
Patch (v2)

Cool tip! Much cleaner code.
Comment 4 Trevor Saunders (:tbsaunde) 2012-05-07 02:08:43 PDT
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 2012-05-07 04:24:36 PDT
Mark, are you going to file new patch addressing comments or should I fix them before landing?
Comment 6 Mark Capella [:capella] 2012-05-07 04:51:22 PDT
I'll fix this one .... I'm working you hard enough already   :)
Comment 7 alexander :surkov 2012-05-07 08:24:43 PDT
(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] 2012-05-07 10:53:32 PDT
Created attachment 621666 [details] [diff] [review]
Patch (v3)

Final nits addressed and re-tested ...
Comment 9 alexander :surkov 2012-05-07 17:43:37 PDT
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 11 Ed Morley [:emorley] 2012-05-08 11:17:45 PDT
https://hg.mozilla.org/mozilla-central/rev/87bc642d7328

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