[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 #14 from Wenson Hsieh <wenson_hsieh at apple.com> ---
Comment on attachment 316993
  --> https://bugs.webkit.org/attachment.cgi?id=316993
Patch

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

> Source/WebCore/ChangeLog:3022
> +2017-07-24  Frederik Riedel  <friedel at apple.com>

You should remove the previous ChangeLog entry here.

> Source/WebKit/ChangeLog:1613
> +2017-07-24  Frederik Riedel  <friedel at apple.com>

Ditto.

> Source/WebKitLegacy/mac/ChangeLog:105
> +2017-07-24  Frederik Riedel  <friedel at apple.com>

Ditto.

> Source/WebCore/dom/Document.cpp:6897
> +Element* Document::nextFocusableElement(Element& element)

The return type here should be a RefPtr<Element>. TBH, FocusController should probably not return a raw pointer either, but that is something we should address separately (additionally, I don't see why it's an Element& instead of a const Element&, but that's also a separate issue).

> Source/WebCore/dom/Document.cpp:6899
> +    return page()->focusController().nextFocusableElement(element);

Perhaps we should add a null check here too -- it looks like the frame may not necessarily exist, and so page() is not necessarily non-null. Something like:

if (auto* page = this->page())
    return page->focusController().nextFocusableElement(element);
return nullptr;

> Source/WebCore/dom/Document.cpp:6902
> +Element* Document::previousFocusableElement(Element& element)

Ditto with the RefPtr<Element>

> Source/WebCore/dom/Document.cpp:6904
> +    return page()->focusController().previousFocusableElement(element);

Ditto with the null check.

> Source/WebCore/html/HTMLInputElement.cpp:921
> +bool HTMLInputElement::computeUsernameAndPasswordElements(Element *start, HTMLInputElement*& usernameElement, HTMLInputElement*& passwordElement)

Instead of raw pointer references as arguments, I think taking a RefPtr<HTMLInputElement>& or std::optional<HTMLInputElement>& would be better.

> Source/WebCore/html/HTMLInputElement.cpp:924
> +        HTMLInputElement* inputElement = downcast<HTMLInputElement>(start);

In WebKit, we use auto in most places when explicitly specifying the type is redundant (as in this case). Something like:

auto& inputElement = downcast<HTMLInputElement>(*start);

...would be better. (we also know that the input element definitely exists here too, since is<HTMLInputElement>(start) would have been false otherwise).

> Source/WebCore/html/HTMLInputElement.cpp:928
> +            if (previousElement && is<HTMLInputElement>(previousElement)) {

The previousElement null check here isn't needed, since is<HTMLInputElement>(previousElement) will return false anyways if previousElement is null.

> Source/WebCore/html/HTMLInputElement.cpp:939
> +            if (nextElement && is<HTMLInputElement>(nextElement)) {

Ditto with the null check.

> Source/WebCore/html/HTMLInputElement.cpp:949
> +    return false;

Let's also set usernameElement and passwordElement to nullptr here before returning false, to ensure that they are null if username/password element computation failed.

> Source/WebCore/html/HTMLInputElement.h:77
> +    WEBCORE_EXPORT URL representingPageUrl();

Could we instead ask the Document directly for the page url instead of adding a new HTMLInputElement method to do this? It seems a bit odd to specifically teach the HTMLInputElement about the document URL.

> Source/WebKit/Platform/spi/ios/UIKitSPI.h:46
> +#import <UIKit/UIKeyboardLoginCredentialsSuggestion.h>

If this is a new header, this will need to be ifdef'ed out for this to compile on older iOSes.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2764
> +        information.representingPageUrl = element.representingPageUrl();

I think it would be better to ask the Page or Document directly here, instead of going through the HTMLInputElement.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:2830
> +void WebPage::autofillLoginCredentials(String username, String password)

Let's change this to take const String&s here.

-- 
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/20170802/fd62b090/attachment-0001.html>