Fix for: State in small letters should be auto-filled from the profile....

Fix for: State in small letters should be auto-filled from the profile.
BUG=38222
TEST=in the bug

[GeorgeY, 2010-07-20 07:45:22]

Select one end fix. I hope I am not too late with that...

[James Hawkins, 2010-07-20 17:10:28]

http://codereview.chromium.org/2832064/diff/8001/9004
File chrome/renderer/form_manager.cc (right):

http://codereview.chromium.org/2832064/diff/8001/9004#newcode234
chrome/renderer/form_manager.cc:234:
field->GetOptionStringsFromElement(element);
I'd rather have this method be in the unnamed namespace of this file than on the
FormField object.  FormManager is responsible for parsing WebForm*Elements and
putting that data into our PODs, so the code to do this is more appropriate
here.

http://codereview.chromium.org/2832064/diff/8001/9006
File webkit/glue/form_field.h (right):

http://codereview.chromium.org/2832064/diff/8001/9006#newcode33
webkit/glue/form_field.h:33: const std::vector<string16> option_strings() const
{ return option_strings_; }
Return a reference.

[GeorgeY, 2010-07-20 21:33:59]

http://codereview.chromium.org/2832064/diff/8001/9004
File chrome/renderer/form_manager.cc (right):

http://codereview.chromium.org/2832064/diff/8001/9004#newcode234
chrome/renderer/form_manager.cc:234:
field->GetOptionStringsFromElement(element);
On 2010/07/20 17:10:28, James Hawkins wrote:
> I'd rather have this method be in the unnamed namespace of this file than on
the
> FormField object.  FormManager is responsible for parsing WebForm*Elements and
> putting that data into our PODs, so the code to do this is more appropriate
> here.

What about FormField(WebKit::WebFormControlElement element) constructor, should
not we fill out the option choices there as well? It would be a code duplication
if we do...

http://codereview.chromium.org/2832064/diff/8001/9006
File webkit/glue/form_field.h (right):

http://codereview.chromium.org/2832064/diff/8001/9006#newcode33
webkit/glue/form_field.h:33: const std::vector<string16> option_strings() const
{ return option_strings_; }
On 2010/07/20 17:10:28, James Hawkins wrote:
> Return a reference.

Done.

[James Hawkins, 2010-07-20 22:27:59]

http://codereview.chromium.org/2832064/diff/8001/9004
File chrome/renderer/form_manager.cc (right):

http://codereview.chromium.org/2832064/diff/8001/9004#newcode234
chrome/renderer/form_manager.cc:234:
field->GetOptionStringsFromElement(element);
On 2010/07/20 21:34:00, GeorgeY wrote:
> On 2010/07/20 17:10:28, James Hawkins wrote:
> > I'd rather have this method be in the unnamed namespace of this file than on
> the
> > FormField object.  FormManager is responsible for parsing WebForm*Elements
and
> > putting that data into our PODs, so the code to do this is more appropriate
> > here.
> 
> What about FormField(WebKit::WebFormControlElement element) constructor,
should
> not we fill out the option choices there as well? It would be a code
duplication
> if we do...

Yes, I absolutely agree that it's ugly and duplication; however, that FormField
constructor is a hack that needs to go. I need to remember to look into removing
it. For now, please duplicate the code to pull out the option elements. Feel
free to add a TODO(jhawkins) somewhere for this.

[GeorgeY, 2010-07-21 08:47:33]

On 2010/07/20 22:27:59, James Hawkins wrote:
> http://codereview.chromium.org/2832064/diff/8001/9004
> File chrome/renderer/form_manager.cc (right):
> 
> http://codereview.chromium.org/2832064/diff/8001/9004#newcode234
> chrome/renderer/form_manager.cc:234:
> field->GetOptionStringsFromElement(element);
> On 2010/07/20 21:34:00, GeorgeY wrote:
> > On 2010/07/20 17:10:28, James Hawkins wrote:
> > > I'd rather have this method be in the unnamed namespace of this file than
on
> > the
> > > FormField object.  FormManager is responsible for parsing WebForm*Elements
> and
> > > putting that data into our PODs, so the code to do this is more
appropriate
> > > here.
> > 
> > What about FormField(WebKit::WebFormControlElement element) constructor,
> should
> > not we fill out the option choices there as well? It would be a code
> duplication
> > if we do...
> 
> Yes, I absolutely agree that it's ugly and duplication; however, that
FormField
> constructor is a hack that needs to go. I need to remember to look into
removing
> it. For now, please duplicate the code to pull out the option elements. Feel
> free to add a TODO(jhawkins) somewhere for this.

done, btw, there is already TODO to remove it...

[James Hawkins, 2010-07-21 17:21:27]

LGTM, thanks.

[James Hawkins, 2010-07-21 19:54:06]

Sorry George, I accidentally typo-ed your name :-/