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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Sep 19 10:24:23 PDT 2017


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

--- Comment #34 from Frederik Riedel <riedel at apple.com> ---
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

>> Source/WebCore/editing/ios/AutofillElements.cpp:34
>> +static inline bool isAutofillableElement(Element* node)
> 
> This function should take Element& instead of Element*.

Done.

>> 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;

Done.

>> Source/WebCore/editing/ios/AutofillElements.cpp:37
>> +        RefPtr<HTMLInputElement> inputElement = &downcast<HTMLInputElement>(*node);
> 
> Use `auto inputElement`. There's no need to ref-churn here.

Done.

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

Done. Also for previousAutofillableElement.

>> 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))

Done.

>> 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.

Great idea! We don't use the page variable anyway, only its focusController(). Passing it directly now. Done.

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

Done.

>> 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.

Done.

>> Source/WebCore/editing/ios/AutofillElements.cpp:87
>> +                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.

Done.

>> Source/WebCore/editing/ios/AutofillElements.h:26
>> +#include "Element.h"
> 
> There's no need to include Element since HTMLInputElement includes Element.

True, removed.

>> Source/WebCore/editing/ios/AutofillElements.h:31
>> +class AutofillElements {
> 
> We should probably use WTF_MAKE_FAST_ALLOCATED here.

Ok. Added.

>> Source/WebCore/editing/ios/AutofillElements.h:36
>> +    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.

They can be private indeed. Added m_ as well. Done.

-- 
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/20170919/836f32fd/attachment-0001.html>


More information about the webkit-unassigned mailing list