[Webkit-unassigned] [Bug 174394] Expose way to set values of classified form controls as {Legacy WebKit, WebKit} SPI

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=174394

--- Comment #32 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 320266
  --> https://bugs.webkit.org/attachment.cgi?id=320266
Patch

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

New patch looks really good! Just some stylistic issues. Please rebase it agains trunk.

> Source/WebCore/editing/ios/AutofillElements.cpp:34
> +static inline bool isAutofillableElement(Element* node)

This function should take Element& instead of Element*.

> Source/WebCore/editing/ios/AutofillElements.cpp:36
> +    if (is<HTMLInputElement>(node)) {

Nit: We refer an early return to nested structure.
so:
if (~)
  return false;
~
return true;

> Source/WebCore/editing/ios/AutofillElements.cpp:37
> +        RefPtr<HTMLInputElement> inputElement = &downcast<HTMLInputElement>(*node);

Use `auto inputElement`. There's no need to ref-churn here.

> Source/WebCore/editing/ios/AutofillElements.cpp:43
> +static inline RefPtr<Element> nextAutofillableElement(Node* startNode, Page& page)

This function should probably return RefPtr<HTMLInputElement>.

> Source/WebCore/editing/ios/AutofillElements.cpp:72
> +    if (is<HTMLInputElement>(start.get())) {

Please explicitly check whether start is null or not as in:
if (start && is<HTMLInputElement>(*start))

> Source/WebCore/editing/ios/AutofillElements.cpp:75
> +            RefPtr<Element> previousElement = previousAutofillableElement(&inputElement, *start->document().page());

We should check that page isn't null, and exit early when it is.
In fact, just store FocusController& as a local variable and use pass that to these helper functions instead.

> Source/WebCore/editing/ios/AutofillElements.cpp:76
> +            if (is<HTMLInputElement>(previousElement.get())) {

Ditto.

> Source/WebCore/editing/ios/AutofillElements.cpp:77
> +                RefPtr<HTMLInputElement> potentialUsernameField = &downcast<HTMLInputElement>(*previousElement);

There's no need to use RefPtr here since we know it can't be null.
Remove & before downcast and use Ref<HTMLInputElement>
But really, you should just use `auto` instead.

> Source/WebCore/editing/ios/AutofillElements.cpp:87
> +            RefPtr<Element> nextElement = nextAutofillableElement(&inputElement, *start->document().page());
> +            if (is<HTMLInputElement>(nextElement.get())) {
> +                RefPtr<HTMLInputElement> potentialPasswordField = &downcast<HTMLInputElement>(*nextElement);

Same issues explicitly checking the nullity and using `auto` (which makes RefPtr in line 87 for example) instead of RefPtr.

> Source/WebCore/editing/ios/AutofillElements.h:26
> +#include "Element.h"

There's no need to include Element since HTMLInputElement includes Element.

> Source/WebCore/editing/ios/AutofillElements.h:31
> +class AutofillElements {

We should probably use WTF_MAKE_FAST_ALLOCATED here.

> Source/WebCore/editing/ios/AutofillElements.h:36
> +    RefPtr<HTMLInputElement> username;
> +    RefPtr<HTMLInputElement> password;

It looks like these two member variables are never accessed outside AutofillElements class.
Make them private and prefix them with m_.
If not, please add an explicit getter & setter instead of exposing them as public member variables.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170914/7a072d15/attachment.html>