[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