Bug 100858

Created attachment 171819 [details] Patch

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 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?

> > 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.

Created attachment 171834 [details] Patch Take Kenneth's feedback into consideration.

Comment on attachment 171834 [details] Patch Attachment 171834 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/14697307

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.

Once Bug 99914 is fixed, we can probably share the WebCore code with GTK port.

Comment on attachment 171845 [details] Patch LGTM

Comment on attachment 171845 [details] Patch Clearing flags on attachment: 171845 Committed r133177: <http://trac.webkit.org/changeset/133177>

All reviewed patches have been landed. Closing bug.

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?

(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).

(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.