Bug 44496 - [EFL] Password manager for WebKit-Efl
Summary: [EFL] Password manager for WebKit-Efl
Status: RESOLVED LATER
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on: 44423
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-24 00:13 PDT by Grzegorz Czajkowski
Modified: 2011-01-12 03:29 PST (History)
3 users (show)

See Also:


Attachments
Password manager for WebKit-Efl (7.57 KB, patch)
2010-08-24 01:21 PDT, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff
Password manager for WebKit-Efl (8.27 KB, patch)
2010-08-27 07:24 PDT, Grzegorz Czajkowski
abarth: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Grzegorz Czajkowski 2010-08-24 00:13:04 PDT
Password manager emits signal "submit,clciked" when user has clicked submit button. Is also allows auto fill username and password to forms.
Comment 1 Grzegorz Czajkowski 2010-08-24 01:21:15 PDT
Created attachment 65229 [details]
Password manager for WebKit-Efl
Comment 2 Rafael Antognolli 2010-08-25 12:15:44 PDT
(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?
Comment 3 Grzegorz Czajkowski 2010-08-27 07:24:54 PDT
Created attachment 65713 [details]
Password manager for WebKit-Efl
Comment 4 Grzegorz Czajkowski 2010-08-27 08:15:06 PDT
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
Comment 5 Rafael Antognolli 2010-08-27 12:16:16 PDT
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
Comment 6 Adam Barth 2010-08-31 19:44:38 PDT
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?
Comment 7 Lucas De Marchi 2011-01-06 09:21:33 PST
Hi  Grzegorz,

Are you planning to re-submit this?
Comment 8 Grzegorz Czajkowski 2011-01-12 02:30:41 PST
(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 :)