Bug 44496 – [EFL] Password manager for WebKit-Efl

• Status: RESOLVED
• Resolution: LATER
• Component: WebKit EFL

[Grzegorz Czajkowski]

Password manager emits signal "submit,clciked" when user has clicked submit button. Is also allows auto fill username and password to forms.

[Grzegorz Czajkowski]

Created attachment 65229 [details]
Password manager for WebKit-Efl

[Rafael Antognolli]

(In reply to comment #1)
> Created an attachment (id=65229) [details]
> Password manager for WebKit-Efl

Hi Grzegorz,

Could you also update the comment section inside ewk_frame.h describing this new signal "submit,clicked"? It's in the beginning of that file, listing all available signals.

+void FrameLoaderClientEfl::submitClicked(const String &userName, const String &password, const KURL &url)
+{
+    ewk_frame_submit_clicked(m_frame, userName.latin1().data(), password.latin1().data(), url.string().latin1().data());
+}

Any reason for using latin1()? I see utf8() instead of that at least inside WebKit-EFL. Not sure which one would be correct, though.

And are you aware that this code will only work if the password field is declared after the username field? Ok, I know that this probably covers 99% of the cases, just asking.

Anyway, have you looked at the other ports implementation? I didn't find this, so couldn't compare.

I would also put a better description on the ChangeLog. Maybe something like:
"Added a signal "submit,clicked" when user clicks on a submit form.
Also adding a function that fills username and password on the current view."

And this patch also just works for the main_frame. What about pages with inner frames? Maybe this function should go to ewk_frame, and work on the called frame. What do you think about this?

[Grzegorz Czajkowski]

Created attachment 65713 [details]
Password manager for WebKit-Efl

[Grzegorz Czajkowski]

Hi Rafael,

(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=65229) [details] [details]
> > Password manager for WebKit-Efl
> Hi Grzegorz,
> Could you also update the comment section inside ewk_frame.h describing this new signal "submit,clicked"? It's in the beginning of that file, listing all available signals.

Good suggesetion. Added

> +void FrameLoaderClientEfl::submitClicked(const String &userName, const String &password, const KURL &url)
> +{
> +    ewk_frame_submit_clicked(m_frame, userName.latin1().data(), password.latin1().data(), url.string().latin1().data());
> +}
> Any reason for using latin1()? I see utf8() instead of that at least inside WebKit-EFL. Not sure which one would be correct, though.

You're right. Method latin1() has been changed to utf8()

> And are you aware that this code will only work if the password field is declared after the username field? Ok, I know that this probably covers 99% of the cases, just asking.

Yes. I am but as you noticed the scenario (username field and then password) covers the most cases. Frankly speaking I didn't see 

> Anyway, have you looked at th
e other ports implementation? I didn't find this, so couldn't compare.

I was looking at others ports implementation but I didn't find them. There isn't password manager for WebCore too. I will try to implement password manager for WebCore too.

> I would also put a better description on the ChangeLog. Maybe something like:
> "Added a signal "submit,clicked" when user clicks on a submit form.
> Also adding a function that fills username and password on the current view."

Changed

> And this patch also just works for the main_frame. What about pages with inner frames? Maybe this function should go to ewk_frame, and work on the called frame. What do you think about this?

That's a good idea.
I think it can resolve scenario when web page has more than one submit button. When user filled username and password to form and then he clicks submit button we will know from which frame it comes, right? 
Currently I marked this as FIXME 

Regards,
Grzegorz

[Rafael Antognolli]

Ok, thanks for the changes.

I saw your implementation on bug 44423 to get the username and password is similar to the way you set the fields with ewk_view_autofill_personal_data. So if the official reviewers are ok with that, I think this patch is also good to go.

Regards,
Rafael

[Adam Barth]

Comment on attachment 65713 [details]
Password manager for WebKit-Efl

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

> WebKit/efl/ewk/ewk_view.cpp:4046
> +    for (unsigned i = 0; i < forms->length(); i++) {
> +        WebCore::HTMLFormElement *formElement = static_cast<WebCore::HTMLFormElement *> (forms->item(i));
> +        for (unsigned j = 0; j < formElement->associatedElements().size(); j++) {   
> +            WebCore::HTMLFormControlElement *element = formElement->associatedElements()[j];
> +            if (!element->hasLocalName(WebCore::HTMLNames::inputTag))
Why just the localName?  Seems like you'd want to match the namespace too.

> WebKit/efl/ewk/ewk_view.cpp:4051
> +            WebCore::HTMLInputElement *inputElement = static_cast<WebCore::HTMLInputElement *>(element);
> +            if (!inputElement->isEnabledFormControl())
> +                continue;
This not memory safe.  Something can have a localName of "input" without being an HTMLInputElement.

> WebKit/efl/ewk/ewk_view.cpp:4069
> +                    if ((inputElement->inputType() == WebCore::HTMLInputElement::TEXT)) {
> +                        inputElement->setValue(username);
> +                        return true;
> +                    }
This whole thing looks like overly much poking at WebCore from the WebKit layer.  If this logic is right, does every port need to copy/paste this method?  Does this code exist in other ports?  Should we move the correct implementation to WebCore proper?

[Lucas De Marchi]

Hi  Grzegorz,

Are you planning to re-submit this?

[Grzegorz Czajkowski]

(In reply to comment #7)
> Hi  Grzegorz,
> 
> Are you planning to re-submit this?

This patch is depends on 44423 that was closed because of design conflict. So I suggest to close this bug too.

I will try to prepare a new patches for password manager in the near feature :)