linux: Implement the new AutoFill section of the Options dialog.

linux: Implement the new AutoFill section of the Options dialog.

* Updated the text of the radio buttons.
* Added a button that opens the AutoFill dialog.

BUG=33025
TEST=manually

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

[tfarina (gmail-do not use), 2010-02-06 05:18:37]

Hi James, could you review this to me?

[James Hawkins, 2010-02-08 19:45:54]

http://codereview.chromium.org/579020/diff/1/3
File chrome/browser/gtk/options/content_page_gtk.cc (right):

http://codereview.chromium.org/579020/diff/1/3#newcode410
chrome/browser/gtk/options/content_page_gtk.cc:410: ShowAutoFillDialog(
This won't always work.  You have to check-load-callback the
PersonalDataManager.  That is, you have to check if the PDM is loaded, load it
if not, and wait for the callback to then ShowAutoFillDialog.  Check out how
AutoFillManager does the same thing.

[tfarina (gmail-do not use), 2010-02-08 20:05:44]

http://codereview.chromium.org/579020/diff/1/3
File chrome/browser/gtk/options/content_page_gtk.cc (right):

http://codereview.chromium.org/579020/diff/1/3#newcode410
chrome/browser/gtk/options/content_page_gtk.cc:410: ShowAutoFillDialog(
On 2010/02/08 19:45:54, James Hawkins wrote:
> This won't always work.  You have to check-load-callback the
> PersonalDataManager.  That is, you have to check if the PDM is loaded, load it
> if not, and wait for the callback to then ShowAutoFillDialog.  Check out how
> AutoFillManager does the same thing.

I will fix this. But, maybe the windows side needs to be fixed too?
At line 111 of chrome/browser/views/options/content_page_view.cc, we are doing
the same thing.

[James Hawkins, 2010-02-08 20:08:24]

On 2010/02/08 20:05:44, tfarina wrote:
> http://codereview.chromium.org/579020/diff/1/3
> File chrome/browser/gtk/options/content_page_gtk.cc (right):
> 
> http://codereview.chromium.org/579020/diff/1/3#newcode410
> chrome/browser/gtk/options/content_page_gtk.cc:410: ShowAutoFillDialog(
> On 2010/02/08 19:45:54, James Hawkins wrote:
> > This won't always work.  You have to check-load-callback the
> > PersonalDataManager.  That is, you have to check if the PDM is loaded, load
it
> > if not, and wait for the callback to then ShowAutoFillDialog.  Check out how
> > AutoFillManager does the same thing.
> 
> I will fix this. But, maybe the windows side needs to be fixed too?
> At line 111 of chrome/browser/views/options/content_page_view.cc, we are doing
> the same thing.

Yes, George is aware of the problem now and is working on a fix.  Please stick
to the gtk side.

[tfarina (gmail-do not use), 2010-02-09 01:57:44]

Hi James, I fixed the problem with ShowAutoFillDialog.

Also I included in this change a fix for the wrong labels in AutoFill dialog.
See screenshot: http://imgur.com/hK61A

Please, have another look.

-Thiago

[tfarina (gmail-do not use), 2010-02-09 20:00:45]

Ping

[James Hawkins, 2010-02-09 20:06:45]

http://codereview.chromium.org/579020/diff/6001/6004
File chrome/browser/gtk/options/content_page_gtk.cc (right):

http://codereview.chromium.org/579020/diff/6001/6004#newcode248
chrome/browser/gtk/options/content_page_gtk.cc:248: // Autofill button.
If |personal_data_| is NULL, we're OTR and the button should be disabled. 
Please add this.

[tfarina (gmail-do not use), 2010-02-10 00:22:08]

New patchset uploaded.

http://codereview.chromium.org/579020/diff/6001/6004
File chrome/browser/gtk/options/content_page_gtk.cc (right):

http://codereview.chromium.org/579020/diff/6001/6004#newcode248
chrome/browser/gtk/options/content_page_gtk.cc:248: // Autofill button.
On 2010/02/09 20:06:45, James Hawkins wrote:
> If |personal_data_| is NULL, we're OTR and the button should be disabled. 
> Please add this.

Done.

[James Hawkins, 2010-02-10 00:27:49]

http://codereview.chromium.org/579020/diff/7002/9004
File chrome/browser/gtk/options/content_page_gtk.cc (right):

http://codereview.chromium.org/579020/diff/7002/9004#newcode185
chrome/browser/gtk/options/content_page_gtk.cc:185:
personal_data_->RemoveObserver(this);
DCHECK on personal_data_.

http://codereview.chromium.org/579020/diff/7002/9004#newcode432
chrome/browser/gtk/options/content_page_gtk.cc:432: 
nit: There's an unnecessary blank line here.

http://codereview.chromium.org/579020/diff/7002/9004#newcode435
chrome/browser/gtk/options/content_page_gtk.cc:435: if
(!page->personal_data_->IsDataLoaded())
You should DCHECK on personal_data_.

http://codereview.chromium.org/579020/diff/7002/9005
File chrome/browser/gtk/options/content_page_gtk.h (right):

http://codereview.chromium.org/579020/diff/7002/9005#newcode19
chrome/browser/gtk/options/content_page_gtk.h:19: public AutoFillDialogObserver,
PersonalDataManager is also an AutoFillDialogObserver, so just send in the
PersonalDataManager instance and let it handle the business of saving profiles
directly.

http://codereview.chromium.org/579020/diff/7002/9005#newcode156
chrome/browser/gtk/options/content_page_gtk.h:156: PersonalDataManager*
personal_data_;
You should note that this can be NULL.

[tfarina (gmail-do not use), 2010-02-10 01:13:40]

> http://codereview.chromium.org/579020/diff/7002/9005
> File chrome/browser/gtk/options/content_page_gtk.h (right):
> 
> http://codereview.chromium.org/579020/diff/7002/9005#newcode19
> chrome/browser/gtk/options/content_page_gtk.h:19: public
AutoFillDialogObserver,
> PersonalDataManager is also an AutoFillDialogObserver, so just send in the
> PersonalDataManager instance and let it handle the business of saving profiles
> directly.
> 
Inheriting PersonalDataManager leds us in this error:

chrome/browser/gtk/options/content_page_gtk.cc:547: error: request for member
‘profile’ is ambiguous
./chrome/browser/autofill/personal_data_manager.h:103: error: candidates are:
Profile* PersonalDataManager::profile()
./chrome/browser/options_page_base.h:40: error:                 Profile*
OptionsPageBase::profile() const

Without inheriting AutoFillDialogObserver leds in this error:

chrome/browser/gtk/options/content_page_gtk.cc:547: error: request for member
‘profile’ is ambiguous
./chrome/browser/autofill/personal_data_manager.h:103: error: candidates are:
Profile* PersonalDataManager::profile()
./chrome/browser/options_page_base.h:40: error:                 Profile*
OptionsPageBase::profile() const

I'm fixing the others.

[James Hawkins, 2010-02-10 01:15:06]

On 2010/02/10 01:13:40, tfarina wrote:
> > http://codereview.chromium.org/579020/diff/7002/9005
> > File chrome/browser/gtk/options/content_page_gtk.h (right):
> > 
> > http://codereview.chromium.org/579020/diff/7002/9005#newcode19
> > chrome/browser/gtk/options/content_page_gtk.h:19: public
> AutoFillDialogObserver,
> > PersonalDataManager is also an AutoFillDialogObserver, so just send in the
> > PersonalDataManager instance and let it handle the business of saving
profiles
> > directly.
> > 
> Inheriting PersonalDataManager leds us in this error:
> 
> chrome/browser/gtk/options/content_page_gtk.cc:547: error: request for member
> ‘profile’ is ambiguous
> ./chrome/browser/autofill/personal_data_manager.h:103: error: candidates are:
> Profile* PersonalDataManager::profile()
> ./chrome/browser/options_page_base.h:40: error:                 Profile*
> OptionsPageBase::profile() const
> 
> Without inheriting AutoFillDialogObserver leds in this error:
> 
> chrome/browser/gtk/options/content_page_gtk.cc:547: error: request for member
> ‘profile’ is ambiguous
> ./chrome/browser/autofill/personal_data_manager.h:103: error: candidates are:
> Profile* PersonalDataManager::profile()
> ./chrome/browser/options_page_base.h:40: error:                 Profile*
> OptionsPageBase::profile() const
> 
> I'm fixing the others.

Inheriting PersonalDataManager is not what I advised.  PersonalDataManager
inherits from AutoFillDialogObserver, so just send in the PersonalDataManager to
ShowAutoFillDialog() as the observer.

[tfarina (gmail-do not use), 2010-02-10 02:52:14]

Sorry for the delay James. I had problems during the upload, so I had to fix
them before uploading the fix again.

Changes made per comments. Please have another look.

http://codereview.chromium.org/579020/diff/7002/9004
File chrome/browser/gtk/options/content_page_gtk.cc (right):

http://codereview.chromium.org/579020/diff/7002/9004#newcode185
chrome/browser/gtk/options/content_page_gtk.cc:185:
personal_data_->RemoveObserver(this);
On 2010/02/10 00:27:49, James Hawkins wrote:
> DCHECK on personal_data_.

Done.

http://codereview.chromium.org/579020/diff/7002/9004#newcode432
chrome/browser/gtk/options/content_page_gtk.cc:432: 
On 2010/02/10 00:27:49, James Hawkins wrote:
> nit: There's an unnecessary blank line here.

Done.

http://codereview.chromium.org/579020/diff/7002/9004#newcode435
chrome/browser/gtk/options/content_page_gtk.cc:435: if
(!page->personal_data_->IsDataLoaded())
On 2010/02/10 00:27:49, James Hawkins wrote:
> You should DCHECK on personal_data_.

Done.

http://codereview.chromium.org/579020/diff/7002/9005
File chrome/browser/gtk/options/content_page_gtk.h (right):

http://codereview.chromium.org/579020/diff/7002/9005#newcode19
chrome/browser/gtk/options/content_page_gtk.h:19: public AutoFillDialogObserver,
On 2010/02/10 00:27:49, James Hawkins wrote:
> PersonalDataManager is also an AutoFillDialogObserver, so just send in the
> PersonalDataManager instance and let it handle the business of saving profiles
> directly.

Done.

http://codereview.chromium.org/579020/diff/7002/9005#newcode156
chrome/browser/gtk/options/content_page_gtk.h:156: PersonalDataManager*
personal_data_;
On 2010/02/10 00:27:49, James Hawkins wrote:
> You should note that this can be NULL.

Done.

[tfarina (gmail-do not use), 2010-02-10 22:43:29]

Ping

[James Hawkins, 2010-02-10 22:57:55]

LGTM

[tfarina (gmail-do not use), 2010-02-10 23:32:05]

On 2010/02/10 22:57:55, James Hawkins wrote:
> LGTM

I rebased to ToT and fixed some conflicts with the erg patch. Could you land
this to me? I don't have commit access.

[tfarina (gmail-do not use), 2010-02-12 00:14:28]

Ping

[James Hawkins, 2010-02-12 00:30:41]

On 2010/02/12 00:14:28, tfarina wrote:
> Ping

I'm sorry, this is going to have to wait till Tuesday.  I'm out of town
tomorrow, and we have a holiday on Monday, and I'm swamped today.  I wouldn't
mind if you got someone else to commit it in the meantime.

[James Hawkins, 2010-02-17 00:01:14]

http://codereview.chromium.org/610003

Waiting for green trybots.

[James Hawkins, 2010-02-17 00:20:28]

On 2010/02/17 00:01:14, James Hawkins wrote:
> http://codereview.chromium.org/610003
> 
> Waiting for green trybots.

Compile fails on the mac bot:

distcc[692] ERROR: compile
/b/slave/mac/build/src/chrome/browser/cocoa/ui_localizer.mm on
jega3.jail/10,lzo,cpp failed
distcc[692] (dcc_build_somewhere) Warning: remote compilation of
'/b/slave/mac/build/src/chrome/browser/cocoa/ui_localizer.mm' failed, retrying
locally
distcc[692] Warning: failed to distribute
/b/slave/mac/build/src/chrome/browser/cocoa/ui_localizer.mm to
jega3.jail/10,lzo,cpp, running locally instead
In file included from
/b/slave/mac/build/src/chrome/browser/cocoa/ui_localizer.mm:64:
/b/slave/mac/build/src/chrome/../xcodebuild/chrome.build/DerivedSources/Debug/ui_localizer_table.h:
In function 'NSString* -[ChromeUILocalizer
localizedStringForString:](ChromeUILocalizer*, objc_selector*, NSString*)':
/b/slave/mac/build/src/chrome/../xcodebuild/chrome.build/DerivedSources/Debug/ui_localizer_table.h:254:
error: 'IDS_OPTIONS_AUTOFILL_NEVERSAVE' was not declared in this scope
/b/slave/mac/build/src/chrome/../xcodebuild/chrome.build/DerivedSources/Debug/ui_localizer_table.h:255:
error: 'IDS_OPTIONS_AUTOFILL_SAVE' was not declared in this scope
distcc[692] ERROR: compile
/b/slave/mac/build/src/chrome/browser/cocoa/ui_localizer.mm on localhost failed

[tfarina (gmail-do not use), 2010-02-17 01:02:52]

On 2010/02/17 00:20:28, James Hawkins wrote:
> On 2010/02/17 00:01:14, James Hawkins wrote:
> > http://codereview.chromium.org/610003
> > 
> > Waiting for green trybots.
> 
> Compile fails on the mac bot:
> 
> distcc[692] ERROR: compile
> /b/slave/mac/build/src/chrome/browser/cocoa/ui_localizer.mm on
> jega3.jail/10,lzo,cpp failed
> distcc[692] (dcc_build_somewhere) Warning: remote compilation of
> '/b/slave/mac/build/src/chrome/browser/cocoa/ui_localizer.mm' failed, retrying
> locally
> distcc[692] Warning: failed to distribute
> /b/slave/mac/build/src/chrome/browser/cocoa/ui_localizer.mm to
> jega3.jail/10,lzo,cpp, running locally instead
> In file included from
> /b/slave/mac/build/src/chrome/browser/cocoa/ui_localizer.mm:64:
>
/b/slave/mac/build/src/chrome/../xcodebuild/chrome.build/DerivedSources/Debug/ui_localizer_table.h:
> In function 'NSString* -[ChromeUILocalizer
> localizedStringForString:](ChromeUILocalizer*, objc_selector*, NSString*)':
>
/b/slave/mac/build/src/chrome/../xcodebuild/chrome.build/DerivedSources/Debug/ui_localizer_table.h:254:
> error: 'IDS_OPTIONS_AUTOFILL_NEVERSAVE' was not declared in this scope
>
/b/slave/mac/build/src/chrome/../xcodebuild/chrome.build/DerivedSources/Debug/ui_localizer_table.h:255:
> error: 'IDS_OPTIONS_AUTOFILL_SAVE' was not declared in this scope
> distcc[692] ERROR: compile
> /b/slave/mac/build/src/chrome/browser/cocoa/ui_localizer.mm on localhost
failed

Sorry James, I don't have a mac here to test. But looking at these errors it
doesn't mean it needs a clobber?

Those IDS_* strings are declared at chrome/app/generated_resources.grd.

[tfarina (gmail-do not use), 2010-02-17 02:02:54]

Hi James,

I uploaded the fix for the mac side. Hope that is the right fix.

Thanks to dhollowa for the help.

[James Hawkins, 2010-02-17 19:38:09]

Committed as r39251.

[tfarina (gmail-do not use), 2010-02-17 20:14:35]

On 2010/02/17 19:38:09, James Hawkins wrote:
> Committed as r39251.

Thanks James.