Added switch to guard all autofill new elements work.

Added switch to guard all autofill new elements work.


BUG=157636
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=166614

[Albert Bodenhamer, 2012-10-28 17:25:36]

I generally prefer not to have flags that don't protect anything.  It would
probably be good to add some minimal real change here as well so the flag has
something to operate on.

http://codereview.chromium.org/11264053/diff/3001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/11264053/diff/3001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:6207: +      <message
name="IDS_FLAGS_ENABLE_AUTOFILL_NEW_ELEMENTS_NAME" desc="Title for the flag to
enable Autofill for new elements">
I think this string kinda misses the mark.  "Enable new experimental form
filling." might be a better fit.

http://codereview.chromium.org/11264053/diff/3001/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/11264053/diff/3001/chrome/browser/about_flags....
chrome/browser/about_flags.cc:1035: kOsWin | kOsLinux,
kOsCrOS is what you want here, not kOsLinux. The initial UI will be Views only
which means that it wont work on desktop Linux builds (building/running Chromium
OS builds is just some flag and gclient changes, so you can continue to dev on
Linux.  I'll send out instructions.)

http://codereview.chromium.org/11264053/diff/3001/chrome/common/chrome_switch...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/11264053/diff/3001/chrome/common/chrome_switch...
chrome/common/chrome_switches.cc:458: const char kEnableAutofillNewElements[]   
 = "enable-autofill-new-elements";
Switch name should be named differently as well.  Maybe "enable-autocheckout"

[Raman Kakilate, 2012-10-30 00:30:46]

On 2012/10/28 17:25:36, Albert Bodenhamer wrote:
> I generally prefer not to have flags that don't protect anything.  It would
> probably be good to add some minimal real change here as well so the flag has
> something to operate on.
> 

Not the best use of the flag. Flag is now being used to relax the criteria for
parsing the form and to know if the form is autofillable.

>
http://codereview.chromium.org/11264053/diff/3001/chrome/app/generated_resour...
> File chrome/app/generated_resources.grd (right):
> 
>
http://codereview.chromium.org/11264053/diff/3001/chrome/app/generated_resour...
> chrome/app/generated_resources.grd:6207: +      <message
> name="IDS_FLAGS_ENABLE_AUTOFILL_NEW_ELEMENTS_NAME" desc="Title for the flag to
> enable Autofill for new elements">
> I think this string kinda misses the mark.  "Enable new experimental form
> filling." might be a better fit.
> 
Done.
>
http://codereview.chromium.org/11264053/diff/3001/chrome/browser/about_flags.cc
> File chrome/browser/about_flags.cc (right):
> 
>
http://codereview.chromium.org/11264053/diff/3001/chrome/browser/about_flags....
> chrome/browser/about_flags.cc:1035: kOsWin | kOsLinux,
> kOsCrOS is what you want here, not kOsLinux. The initial UI will be Views only
> which means that it wont work on desktop Linux builds (building/running
Chromium
> OS builds is just some flag and gclient changes, so you can continue to dev on
> Linux.  I'll send out instructions.)
> 
Done.
>
http://codereview.chromium.org/11264053/diff/3001/chrome/common/chrome_switch...
> File chrome/common/chrome_switches.cc (right):
> 
>
http://codereview.chromium.org/11264053/diff/3001/chrome/common/chrome_switch...
> chrome/common/chrome_switches.cc:458: const char kEnableAutofillNewElements[] 
 
>  = "enable-autofill-new-elements";
> Switch name should be named differently as well.  Maybe "enable-autocheckout"
Renamed it to "enable-experimental-form-filling"

[Albert Bodenhamer, 2012-11-01 16:52:47]

File a bug in crbug.com and reference it here.  Add the Restrict-View-Google
flag to it if you're need to mention anything private in the bug description.

Actually, this initial use of the flag is fine.  You can make greater use of it
in later CLs.

You'll need approval from a top-level Chrome OWNER for the changes in
chrome_switches.  thakis@ approved ahutter@'s recent similar change.

http://codereview.chromium.org/11264053/diff/14002/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/11264053/diff/14002/chrome/app/generated_resou...
chrome/app/generated_resources.grd:6211: +        Enable experimental form
filling. This allows detection of additional input elements like checkboxes and
autofill them.
I'd leave off the detailed description.  Maybe something just say something like
"Enables a collection of experimental features that make form filling easier."

[Raman Kakilate, 2012-11-01 19:08:54]

On 2012/11/01 16:52:47, Albert Bodenhamer wrote:
> File a bug in http://crbug.com and reference it here.  Add the
Restrict-View-Google
> flag to it if you're need to mention anything private in the bug description.
> 

Bug is already attached.
http://code.google.com/p/chromium/issues/detail?id=157636

> Actually, this initial use of the flag is fine.  You can make greater use of
it
> in later CLs.
> 
> You'll need approval from a top-level Chrome OWNER for the changes in
> chrome_switches.  thakis@ approved ahutter@'s recent similar change.
> 

Thanks for the pointers, I added thakis@chromium.org as reviewer.
>
http://codereview.chromium.org/11264053/diff/14002/chrome/app/generated_resou...
> File chrome/app/generated_resources.grd (right):
> 
>
http://codereview.chromium.org/11264053/diff/14002/chrome/app/generated_resou...
> chrome/app/generated_resources.grd:6211: +        Enable experimental form
> filling. This allows detection of additional input elements like checkboxes
and
> autofill them.
> I'd leave off the detailed description.  Maybe something just say something
like
> "Enables a collection of experimental features that make form filling easier."

Done.

[Raman Kakilate, 2012-11-01 19:09:56]

Sorry - getting used to the review tool.

Pardon me if you see multiple messages.

http://codereview.chromium.org/11264053/diff/14002/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/11264053/diff/14002/chrome/app/generated_resou...
chrome/app/generated_resources.grd:6211: +        Enable experimental form
filling. This allows detection of additional input elements like checkboxes and
autofill them.
On 2012/11/01 16:52:47, Albert Bodenhamer wrote:
> I'd leave off the detailed description.  Maybe something just say something
like
> "Enables a collection of experimental features that make form filling easier."

Done.

[Raman Kakilate, 2012-11-02 16:35:37]

Ping !!

[Albert Bodenhamer, 2012-11-02 16:37:46]

LGTM from my end.

Nico, can you review the chrome_switches changes?

[Nico, 2012-11-02 16:42:11]

http://codereview.chromium.org/11264053/diff/20002/chrome/common/chrome_switc...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/11264053/diff/20002/chrome/common/chrome_switc...
chrome/common/chrome_switches.cc:497: // Enable autofill for new elements like
checkboxes. crbug.com/157636
Code looks fine, but the tracking bug has several issues:

1.) If this is a new feature, it should have use the 'new feature' bug template
(which has product review, ux review, security review, privacy review etc labels
to be ticked off)

2.) The bug doesn't mention anything confidential as far as I can tell, so it
shouldn't be restrict-view-google. In general, chromium is an open-source
project and almost all bugs shouldn't need that label.

[Raman Kakilate, 2012-11-02 17:23:44]

On 2012/11/02 16:42:11, Nico wrote:
>
http://codereview.chromium.org/11264053/diff/20002/chrome/common/chrome_switc...
> File chrome/common/chrome_switches.cc (right):
> 
>
http://codereview.chromium.org/11264053/diff/20002/chrome/common/chrome_switc...
> chrome/common/chrome_switches.cc:497: // Enable autofill for new elements like
> checkboxes. crbug.com/157636
> Code looks fine, but the tracking bug has several issues:
> 
> 1.) If this is a new feature, it should have use the 'new feature' bug
template
> (which has product review, ux review, security review, privacy review etc
labels
> to be ticked off)
> 
Created feature tracking bug
http://code.google.com/p/chromium/issues/detail?id=159162

> 2.) The bug doesn't mention anything confidential as far as I can tell, so it
> shouldn't be restrict-view-google. In general, chromium is an open-source
> project and almost all bugs shouldn't need that label.

Removed the restrict on this particular bug being fixed.

[Raman Kakilate, 2012-11-07 17:51:03]

On 2012/11/02 17:23:44, Raman Kakilate wrote:
> On 2012/11/02 16:42:11, Nico wrote:
> >
>
http://codereview.chromium.org/11264053/diff/20002/chrome/common/chrome_switc...
> > File chrome/common/chrome_switches.cc (right):
> > 
> >
>
http://codereview.chromium.org/11264053/diff/20002/chrome/common/chrome_switc...
> > chrome/common/chrome_switches.cc:497: // Enable autofill for new elements
like
> > checkboxes. crbug.com/157636
> > Code looks fine, but the tracking bug has several issues:
> > 
> > 1.) If this is a new feature, it should have use the 'new feature' bug
> template
> > (which has product review, ux review, security review, privacy review etc
> labels
> > to be ticked off)
> > 
> Created feature tracking bug
> http://code.google.com/p/chromium/issues/detail?id=159162

Created new public umbrella bug
https://code.google.com/p/chromium/issues/detail?id=159830. I am not sure about
the process, should each of the change have its own crbug associated with it, or
all the bugs point to single feature bug ?

I have two bugs. one for complete feature and one for just this change. both the
bugs are public now.

> 
> > 2.) The bug doesn't mention anything confidential as far as I can tell, so
it
> > shouldn't be restrict-view-google. In general, chromium is an open-source
> > project and almost all bugs shouldn't need that label.
> 
> Removed the restrict on this particular bug being fixed.

[Nico, 2012-11-07 17:57:26]

lgtm

[commit-bot: I haz the power, 2012-11-07 18:49:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ramankk@chromium.org/11264053/20002

[commit-bot: I haz the power, 2012-11-07 18:49:21]

Failed to apply patch for chrome/common/chrome_switches.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/common/chrome_switches.h
  Hunk #1 FAILED at 145.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/common/chrome_switches.h.rej

Patch:       chrome/common/chrome_switches.h
Index: chrome/common/chrome_switches.h
diff --git a/chrome/common/chrome_switches.h b/chrome/common/chrome_switches.h
index
942307a2756bf3fe4265ca2fcdecccbe9cdd9ced..aa23457500572d6180683883fb1666a079d7ceb5
100644
--- a/chrome/common/chrome_switches.h
+++ b/chrome/common/chrome_switches.h
@@ -145,6 +145,7 @@ extern const char kEnableExperimentalExtensionApis[];
 extern const char kEnableExtensionActivityLogging[];
 extern const char kEnableExtensionActivityUI[];
 extern const char kExtensionsInActionBox[];
+extern const char kEnableExperimentalFormFilling[];
 extern const char kEnableExtensionTimelineApi[];
 extern const char kEnableFramelessConstrainedDialogs[];
 extern const char kEnableFileCookies[];

[Raman Kakilate, 2012-11-07 19:13:17]

Rebased and uploaded CL.

On 2012/11/07 18:49:21, I haz the power (commit-bot) wrote:
> Failed to apply patch for chrome/common/chrome_switches.h:
> While running patch -p1 --forward --force --no-backup-if-mismatch;
>   patching file chrome/common/chrome_switches.h
>   Hunk #1 FAILED at 145.
>   1 out of 1 hunk FAILED -- saving rejects to file
> chrome/common/chrome_switches.h.rej
> 
> Patch:       chrome/common/chrome_switches.h
> Index: chrome/common/chrome_switches.h
> diff --git a/chrome/common/chrome_switches.h b/chrome/common/chrome_switches.h
> index
>
942307a2756bf3fe4265ca2fcdecccbe9cdd9ced..aa23457500572d6180683883fb1666a079d7ceb5
> 100644
> --- a/chrome/common/chrome_switches.h
> +++ b/chrome/common/chrome_switches.h
> @@ -145,6 +145,7 @@ extern const char kEnableExperimentalExtensionApis[];
>  extern const char kEnableExtensionActivityLogging[];
>  extern const char kEnableExtensionActivityUI[];
>  extern const char kExtensionsInActionBox[];
> +extern const char kEnableExperimentalFormFilling[];
>  extern const char kEnableExtensionTimelineApi[];
>  extern const char kEnableFramelessConstrainedDialogs[];
>  extern const char kEnableFileCookies[];

[commit-bot: I haz the power, 2012-11-07 23:20:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ramankk@chromium.org/11264053/32001

[commit-bot: I haz the power, 2012-11-08 06:22:24]

Change committed as 166614