Bug 100858 - [EFL][WK2] Add Ewk_Auth_Request API
Summary: [EFL][WK2] Add Ewk_Auth_Request API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks: 61838 100953
  Show dependency treegraph
 
Reported: 2012-10-31 07:20 PDT by Chris Dumez
Modified: 2012-11-03 01:27 PDT (History)
14 users (show)

See Also:


Attachments
Patch (38.67 KB, patch)
2012-11-01 05:30 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (38.78 KB, patch)
2012-11-01 06:42 PDT, Chris Dumez
gtk-ews: commit-queue-
Details | Formatted Diff | Diff
Patch (39.38 KB, patch)
2012-11-01 07:24 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2012-10-31 07:20:22 PDT
We need an API for the browser to handle HTTP authentication requests.
Comment 1 Chris Dumez 2012-11-01 05:30:07 PDT
Created attachment 171819 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2012-11-01 05:50:44 PDT
Comment on attachment 171819 [details]
Patch

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

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:863
>  }
> +#elif PLATFORM(EFL)

newline before that

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1073
>  }
> +#elif PLATFORM(EFL)

newline before

> Source/WebKit2/UIProcess/API/efl/EwkViewCallbacks.h:151
> +DECLARE_EWK_VIEW_CALLBACK(AuthRequest, "auth,request", Ewk_Auth_Request);

why not authentication,request

> Source/WebKit2/UIProcess/API/efl/ewk_auth_request.cpp:69
> +const char* Ewk_Auth_Request::realm() const

realm :-) Feels like 1600 England :-)
Comment 3 Chris Dumez 2012-11-01 06:04:58 PDT
Comment on attachment 171819 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_auth_request.cpp:69
>> +const char* Ewk_Auth_Request::realm() const
> 
> realm :-) Feels like 1600 England :-)

This is commonly used in this context though :) Would you prefer something else?
Comment 4 Kenneth Rohde Christiansen 2012-11-01 06:05:45 PDT
> > realm :-) Feels like 1600 England :-)
> 
> This is commonly used in this context though :) Would you prefer something else?

No, no, it's fine :-) it's just funny.
Comment 5 Chris Dumez 2012-11-01 06:42:21 PDT
Created attachment 171834 [details]
Patch

Take Kenneth's feedback into consideration.
Comment 6 kov's GTK+ EWS bot 2012-11-01 07:04:30 PDT
Comment on attachment 171834 [details]
Patch

Attachment 171834 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14697307
Comment 7 Chris Dumez 2012-11-01 07:24:45 PDT
Created attachment 171845 [details]
Patch

Call AuthenticationClient::receivedRequestToContinueWithoutCredential() from WebCoreSynchronousLoader::didReceiveAuthenticationChallenge() so that we don't handle authentication for synchronous XMLHTTPRequests.

This fixes a regression in:
  http/tests/xmlhttprequest/access-control-preflight-credential-sync.html

BTW, the GTK EWS seems broken and its warning is unrelated to my patch.
Comment 8 Chris Dumez 2012-11-01 07:34:14 PDT
Once Bug 99914 is fixed, we can probably share the WebCore code with GTK port.
Comment 9 Mikhail Pozdnyakov 2012-11-01 07:36:04 PDT
Comment on attachment 171845 [details]
Patch

LGTM
Comment 10 WebKit Review Bot 2012-11-01 08:33:05 PDT
Comment on attachment 171845 [details]
Patch

Clearing flags on attachment: 171845

Committed r133177: <http://trac.webkit.org/changeset/133177>
Comment 11 WebKit Review Bot 2012-11-01 08:33:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Martin Robinson 2012-11-02 17:29:10 PDT
Were the changes to ResourceHandleSoup copied from my patch at https://bugs.webkit.org/show_bug.cgi?id=99914 ? I really do appreciate that this patch fixed the stupid bugs in my earlier work, bit it would have been swell to coordinate with you guys. Now I guess I'll try to fix the merge conflicts. Be aware that you probably want to get the CredentialStorage working for EFL if you to avoid continuing to fork the libsoup backend. Do you plan to use libsecret as well?
Comment 13 Chris Dumez 2012-11-03 01:14:27 PDT
(In reply to comment #12)
> Were the changes to ResourceHandleSoup copied from my patch at https://bugs.webkit.org/show_bug.cgi?id=99914 ? I really do appreciate that this patch fixed the stupid bugs in my earlier work, bit it would have been swell to coordinate with you guys. Now I guess I'll try to fix the merge conflicts. Be aware that you probably want to get the CredentialStorage working for EFL if you to avoid continuing to fork the libsoup backend. Do you plan to use libsecret as well?

Sadly, I had been working on that patch for a while and I was made aware of your patch by Dominik only after I had it working. I wish I had known about your patch before because the WebCore changes were a bit of work (and my plan was only to write the WebKit2 EFL API but then I noticed the authentication callback was not called when running my API tests). So, to answer your question, no I did not copy your patch, I based mine on other ports' implementation and ewk_auth_soup.* from WebKit1-EFL.

By the way, I CC'd you and a few other GTK port guys as soon as I uploaded my patch since I was editing SOUP-related code.

Sorry about the merge conflicts, I should have raised the issue with you after I was made aware of your work and noticed the overlapping work. I'm hoping we can share same ResourceHandleSoup implementation now though (I don't know if this is how you handled the conflicts).

I'll look into the CredentialStorage support, thanks for notifying me. I'll need to take a look at it because taking a decision but we usually try to share as much code as possible with GTK port in that case. We certainly don't want to "fork" the libsoup backend. The authentication client code was an exception (because the GTK port did not have one yet and it seemed to be the simplest approach to get authentication working for us in WK2 EFL).
Comment 14 Chris Dumez 2012-11-03 01:27:02 PDT
(In reply to comment #13)
> (In reply to comment #12)
> > Were the changes to ResourceHandleSoup copied from my patch at https://bugs.webkit.org/show_bug.cgi?id=99914 ? I really do appreciate that this patch fixed the stupid bugs in my earlier work, bit it would have been swell to coordinate with you guys. Now I guess I'll try to fix the merge conflicts. Be aware that you probably want to get the CredentialStorage working for EFL if you to avoid continuing to fork the libsoup backend. Do you plan to use libsecret as well?
> 
> Sadly, I had been working on that patch for a while and I was made aware of your patch by Dominik only after I had it working. I wish I had known about your patch before because the WebCore changes were a bit of work (and my plan was only to write the WebKit2 EFL API but then I noticed the authentication callback was not called when running my API tests). So, to answer your question, no I did not copy your patch, I based mine on other ports' implementation and ewk_auth_soup.* from WebKit1-EFL.
> 
> By the way, I CC'd you and a few other GTK port guys as soon as I uploaded my patch since I was editing SOUP-related code.
> 
> Sorry about the merge conflicts, I should have raised the issue with you after I was made aware of your work and noticed the overlapping work. I'm hoping we can share same ResourceHandleSoup implementation now though (I don't know if this is how you handled the conflicts).
> 
> I'll look into the CredentialStorage support, thanks for notifying me. I'll need to take a look at it because taking a decision but we usually try to share as much code as possible with GTK port in that case. We certainly don't want to "fork" the libsoup backend. The authentication client code was an exception (because the GTK port did not have one yet and it seemed to be the simplest approach to get authentication working for us in WK2 EFL).

I have checked and I see that you already removed the #if PLATFORM() statements and share the implementation for all SOUP-based ports. This looks much nicer indeed, thanks a lot.