Bug 85576 – [BlackBerry] AutofillManager implementation upstream

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit BlackBerry

[Jonathan Dong]

RIM PR: 136405

The form auto fill feature by my current implementation is pretty much like the behavior of desktop browsers, when user submits a form, the name attribute and the value of text input fields(exclude password) will be stored in the autofill database as a name-value pair. Next time when the user fills any form on any site, the browser will search the db to find out if there's any matched name-value pair to fill the current editing input text field. We will find all the values which has the same name attribute and starts with the character that user has input, and pops up a context dialog to notify user those candidates used by him. If user chooses one from the context dialog, the value will fill the current input text field.

This PR is about the implementation of class AutofillManager, which handles autofill interactions.

[Jonathan Dong]

Created attachment 140460 [details]
Patch

[Rob Buis]

Comment on attachment 140460 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140460&action=review

This can be cleaned up some more.

> Source/WebKit/blackberry/WebCoreSupport/AutofillManager.h:26
> +class String;

Indent here, this is WebKit style.

> Source/WebKit/blackberry/WebCoreSupport/AutofillManager.h:31
> +class WebPagePrivate;

Ditto.

> Source/WebKit/blackberry/WebCoreSupport/AutofillManager.h:37
> +class HTMLInputElement;

The whole block in namespace should be indented.

> Source/WebKit/blackberry/WebCoreSupport/AutofillManager.h:50
> +    AutofillManager(BlackBerry::WebKit::WebPagePrivate* page):m_webPagePrivate(page), m_element(0) { }

Watch the ':', it should have space characters around it.

[Jonathan Dong]

Comment on attachment 140460 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140460&action=review

>> Source/WebKit/blackberry/WebCoreSupport/AutofillManager.h:37
>> +class HTMLInputElement;
> 
> The whole block in namespace should be indented.

Thanks Rob, for those indentation things, I guess many of our porting's codes are not following this guideline, especially in WebKit/blackberry. Do we need to fix them when we are about to make some changes?
Anyway I'll follow your suggestion with my next patch.

[Jonathan Dong]

(In reply to comment #3)
> (From update of attachment 140460 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140460&action=review
> 
> >> Source/WebKit/blackberry/WebCoreSupport/AutofillManager.h:37
> >> +class HTMLInputElement;
> > 
> > The whole block in namespace should be indented.
> 
> Thanks Rob, for those indentation things, I guess many of our porting's codes are not following this guideline, especially in WebKit/blackberry. Do we need to fix them when we are about to make some changes?
> Anyway I'll follow your suggestion with my next patch.

oh, the changes with the indentation in namespace are rejected by check-webkit-style, and I also checked the coding style of webkit.org in http://www.webkit.org/coding/coding-style.html which implies that we should not indent in this situation.

[Jonathan Dong]

Created attachment 140625 [details]
Patch

[Rob Buis]

Comment on attachment 140625 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140625&action=review

Looks good, can be improved some more.

> Source/WebKit/blackberry/WebCoreSupport/AutofillManager.cpp:45
> +    Vector<String> candidates = autofillBackingStore().get(element->getAttribute("name").string(), element->value());

Can you use HTMLNames::nameAttr here? Also is this line needed since you are not using candidates.

> Source/WebKit/blackberry/WebCoreSupport/AutofillManager.cpp:66
> +            || element->isAutofilled() || !element->shouldAutocomplete())

You are doing these checks here and in didChangeInTextField, can you make a static function for this in this .cpp?

> Source/WebKit/blackberry/WebCoreSupport/AutofillManager.cpp:68
> +        autofillBackingStore().add(element->getAttribute("name").string(), element->value());

Can you use HTMLNames::nameAttr here?

[Jonathan Dong]

Created attachment 140642 [details]
Patch

[Rob Buis]

Comment on attachment 140642 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140642&action=review

Sorry, needs one more iteration :)

> Source/WebKit/blackberry/WebCoreSupport/AutofillManager.cpp:31
> +static bool isNotAutofillable(HTMLInputElement* element)

I think this is better to reverse this logic, ie. make it isAutofillable, and test for ! isNotAutofillable().

[Rob Buis]

(In reply to comment #8)
> (From update of attachment 140642 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140642&action=review
> 
> Sorry, needs one more iteration :)
> 
> > Source/WebKit/blackberry/WebCoreSupport/AutofillManager.cpp:31
> > +static bool isNotAutofillable(HTMLInputElement* element)
> 
> I think this is better to reverse this logic, ie. make it isAutofillable, and test for ! isNotAutofillable().

Sorry I meant test for !isAutofillable()

[Jonathan Dong]

Comment on attachment 140642 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140642&action=review

>>> Source/WebKit/blackberry/WebCoreSupport/AutofillManager.cpp:31
>>> +static bool isNotAutofillable(HTMLInputElement* element)
>> 
>> I think this is better to reverse this logic, ie. make it isAutofillable, and test for ! isNotAutofillable().
> 
> Sorry I meant test for !isAutofillable()

thanks, will fix soon. :)

[Jonathan Dong]

Created attachment 140649 [details]
Patch

[Rob Buis]

Comment on attachment 140649 [details]
Patch

I like it!

[WebKit Review Bot]

Comment on attachment 140649 [details]
Patch

Rejecting attachment 140649 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/12650264

[Jonathan Dong]

Created attachment 140673 [details]
Patch

[WebKit Review Bot]

Comment on attachment 140673 [details]
Patch

Rejecting attachment 140673 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/12644374

[Jonathan Dong]

Created attachment 140677 [details]
Patch

[Charles Wei]

Comment on attachment 140677 [details]
Patch

Help to commit which was already reviewed by Rob.

[WebKit Review Bot]

Comment on attachment 140677 [details]
Patch

Clearing flags on attachment: 140677

Committed r116400: <http://trac.webkit.org/changeset/116400>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.