Add flags for new Autofill work.

Add flags for new Autofill work.

Also:
 * Rename the kExternalAutofillPopup flag to the more general kEnableNewAutofillUi flag, since we'll be working on multiple new pieces of UI.
 * Remove an Android override with a flag; use a #if defined(OS_ANDROID) macro instead, so that fewer bits of the code need to worry about the flag.
 * Remove a dead flag.
 * Expand a comment in the about_flags.cc code.

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

[Ilya Sherman, 2012-10-19 05:02:03]

+joth for the Android changes

[Ilya Sherman, 2012-10-19 07:25:29]

+csharp as an fyi

[Albert Bodenhamer, 2012-10-19 16:46:52]

https://chromiumcodereview.appspot.com/11193052/diff/1/chrome/browser/ui/tab_...
File chrome/browser/ui/tab_contents/tab_contents.cc (right):

https://chromiumcodereview.appspot.com/11193052/diff/1/chrome/browser/ui/tab_...
chrome/browser/ui/tab_contents/tab_contents.cc:120: #if !defined(OS_ANDROID)
Wrapping an if() in a conditional compile like this is a bit hard to read. 
Consider:
#if defined(OS_ANDROID)
  bool enable_new_autofill_ui = true;
#else
  bool enable_new_autofill_ui = CommandLine::ForCurrentProcess()->HasSwitch(   
switches::kEnableNewAutofillUi))
#endif

if(enable_new_autofill_ui) {

[joth, 2012-10-19 22:04:06]

+aurimas FYI

https://chromiumcodereview.appspot.com/11193052/diff/1/chrome/browser/ui/tab_...
File chrome/browser/ui/tab_contents/tab_contents.cc (right):

https://chromiumcodereview.appspot.com/11193052/diff/1/chrome/browser/ui/tab_...
chrome/browser/ui/tab_contents/tab_contents.cc:120: #if !defined(OS_ANDROID)
On 2012/10/19 16:46:52, Albert Bodenhamer wrote:
> Wrapping an if() in a conditional compile like this is a bit hard to read. 
> Consider:
> #if defined(OS_ANDROID)
>   bool enable_new_autofill_ui = true;
> #else
>   bool enable_new_autofill_ui = CommandLine::ForCurrentProcess()->HasSwitch(  

> switches::kEnableNewAutofillUi))
> #endif
> 
> if(enable_new_autofill_ui) {

Android is just about to stop using tab_contents.cc altogether, so you could
TODO(nileshagrawal) to remove this ifdef  real soon now.

[Ilya Sherman, 2012-10-19 22:16:26]

https://chromiumcodereview.appspot.com/11193052/diff/1/chrome/browser/ui/tab_...
File chrome/browser/ui/tab_contents/tab_contents.cc (right):

https://chromiumcodereview.appspot.com/11193052/diff/1/chrome/browser/ui/tab_...
chrome/browser/ui/tab_contents/tab_contents.cc:120: #if !defined(OS_ANDROID)
On 2012/10/19 22:04:06, joth wrote:
> On 2012/10/19 16:46:52, Albert Bodenhamer wrote:
> > Wrapping an if() in a conditional compile like this is a bit hard to read. 
> > Consider:
> > #if defined(OS_ANDROID)
> >   bool enable_new_autofill_ui = true;
> > #else
> >   bool enable_new_autofill_ui = CommandLine::ForCurrentProcess()->HasSwitch(
 
> 
> > switches::kEnableNewAutofillUi))
> > #endif
> > 
> > if(enable_new_autofill_ui) {
> 
> Android is just about to stop using tab_contents.cc altogether, so you could
> TODO(nileshagrawal) to remove this ifdef  real soon now.

Given joth's comment, I've left the code in android/chrome_startup_flags.cc and
added a TODO for Nilesh to remove the code once the dependency on
tab_contents.cc is gone.

[joth, 2012-10-19 22:22:10]

(+nileshagrawal as we're citing him in a TODO)
lgtm for android.

[Albert Bodenhamer, 2012-10-23 21:01:55]

lgtm

[commit-bot: I haz the power, 2012-10-23 21:23:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/11193052/6002

[commit-bot: I haz the power, 2012-10-23 21:24:14]

Presubmit check for 11193052-6002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    chrome/browser/ui/tab_contents

Presubmit checks took 5.0s to calculate.

[Ilya Sherman, 2012-10-23 21:24:54]

Nico, can you stamp for OWNERship?

[Avi (use Gerrit), 2012-10-23 21:26:11]

tab_contents LGTM though you're quite out of date and will have to rebase.

[Nico, 2012-10-23 21:47:33]

lgtm except for that bug comment below

https://chromiumcodereview.appspot.com/11193052/diff/6002/chrome/app/generate...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/11193052/diff/6002/chrome/app/generate...
chrome/app/generated_resources.grd:5992: Enable the new Autofill UI (feature in
progress).
Everything on flags is in progress, no need to call that out. Also, this just
repeats the title, maybe something more describing what this is ("The new UI
shows a blargle instead of the old behavior of foobling")

https://chromiumcodereview.appspot.com/11193052/diff/6002/chrome/common/chrom...
File chrome/common/chrome_switches.cc (right):

https://chromiumcodereview.appspot.com/11193052/diff/6002/chrome/common/chrom...
chrome/common/chrome_switches.cc:593: // Enables the new Autofill UI, which is
part of the browser process rather than
If you're doing a new autofill UI, you should have a tracking bug for that and
mention that in the BUG= line. (Probably even one of those launch bugs)

[Ilya Sherman, 2012-10-23 21:58:08]

https://chromiumcodereview.appspot.com/11193052/diff/6002/chrome/app/generate...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/11193052/diff/6002/chrome/app/generate...
chrome/app/generated_resources.grd:5992: Enable the new Autofill UI (feature in
progress).
On 2012/10/23 21:47:33, Nico wrote:
> Everything on flags is in progress, no need to call that out. Also, this just
> repeats the title, maybe something more describing what this is ("The new UI
> shows a blargle instead of the old behavior of foobling")

Done, though the new descriptions are perhaps a little too verbose; and at the
same time a little imprecise, because making them precise would require even
more verbosity.

https://chromiumcodereview.appspot.com/11193052/diff/6002/chrome/common/chrom...
File chrome/common/chrome_switches.cc (right):

https://chromiumcodereview.appspot.com/11193052/diff/6002/chrome/common/chrom...
chrome/common/chrome_switches.cc:593: // Enables the new Autofill UI, which is
part of the browser process rather than
On 2012/10/23 21:47:33, Nico wrote:
> If you're doing a new autofill UI, you should have a tracking bug for that and
> mention that in the BUG= line. (Probably even one of those launch bugs)

Done.

[commit-bot: I haz the power, 2012-10-23 21:59:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/11193052/11003

[commit-bot: I haz the power, 2012-10-24 00:12:27]

Failed to apply patch for chrome/browser/autofill/autofill_manager_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/autofill/autofill_manager_unittest.cc
  Hunk #1 FAILED at 3118.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/autofill/autofill_manager_unittest.cc.rej

Patch:       chrome/browser/autofill/autofill_manager_unittest.cc
Index: chrome/browser/autofill/autofill_manager_unittest.cc
diff --git a/chrome/browser/autofill/autofill_manager_unittest.cc
b/chrome/browser/autofill/autofill_manager_unittest.cc
index
e032b3e464b11893308c283a1f31ac35db84c527..672288622c3ffb149d6fdca1cb491435a1dedf8c
100644
--- a/chrome/browser/autofill/autofill_manager_unittest.cc
+++ b/chrome/browser/autofill/autofill_manager_unittest.cc
@@ -3118,7 +3118,7 @@ TEST_F(AutofillManagerTest, TestExternalDelegate) {
 // an external delegate was set in the proper structures.
 TEST_F(AutofillManagerTest, TestTabContentsWithExternalDelegate) {
   CommandLine::ForCurrentProcess()->AppendSwitch(
-      switches::kExternalAutofillPopup);
+      switches::kEnableNewAutofillUi);
 
   // Setting the contents creates a new TabContents.
   WebContents* contents = CreateTestWebContents();

[Albert Bodenhamer, 2012-10-24 00:14:38]

Remember how we talked about rebasing?

I just ran a few try bots to see if this would work ok and it failed to
apply, so we'll need to sync and rebase this. :-/


On Tue, Oct 23, 2012 at 5:12 PM, <commit-bot@chromium.org> wrote:

> Failed to apply patch for chrome/browser/autofill/**
> autofill_manager_unittest.cc:
> While running patch -p1 --forward --force --no-backup-if-mismatch;
>   patching file chrome/browser/autofill/**autofill_manager_unittest.cc
>   Hunk #1 FAILED at 3118.
>   1 out of 1 hunk FAILED -- saving rejects to file
> chrome/browser/autofill/**autofill_manager_unittest.cc.**rej
>
> Patch:       chrome/browser/autofill/**autofill_manager_unittest.cc
>
> Index: chrome/browser/autofill/**autofill_manager_unittest.cc
> diff --git a/chrome/browser/autofill/**autofill_manager_unittest.cc
> b/chrome/browser/autofill/**autofill_manager_unittest.cc
> index
> e032b3e464b11893308c283a1f31ac**35db84c527..**
> 672288622c3ffb149d6fdca1cb4914**35a1dedf8c
>
> 100644
> --- a/chrome/browser/autofill/**autofill_manager_unittest.cc
> +++ b/chrome/browser/autofill/**autofill_manager_unittest.cc
> @@ -3118,7 +3118,7 @@ TEST_F(AutofillManagerTest, TestExternalDelegate) {
>
>  // an external delegate was set in the proper structures.
>  TEST_F(AutofillManagerTest, TestTabContentsWithExternalDel**egate) {
>    CommandLine::**ForCurrentProcess()->**AppendSwitch(
> -      switches::**kExternalAutofillPopup);
> +      switches::**kEnableNewAutofillUi);
>
>    // Setting the contents creates a new TabContents.
>    WebContents* contents = CreateTestWebContents();
>
>
>
https://chromiumcodereview.**appspot.com/11193052/<https://chromiumcodereview...
>



-- 
Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com>
org

[Avi (use Gerrit), 2012-10-24 00:16:43]

This was a race condition. We both had patches that were mutually conflicting
and we both CQ-ed. I won. (https://codereview.chromium.org/11185011)

No hard feelings. :)

[commit-bot: I haz the power, 2012-10-24 00:35:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/11193052/17001

[commit-bot: I haz the power, 2012-10-24 01:44:55]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-24 03:02:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/11193052/17001

[commit-bot: I haz the power, 2012-10-24 03:47:17]

Change committed as 163760