Bug 84214

Summary: [BlackBerry] Loading media data with http authentication
Product: WebKit Reporter: Jonathan Dong <jonathan.dong.webkit>
Component: WebKit BlackBerryAssignee: Ryuan Choi <ryuan.choi>
Status: RESOLVED FIXED    
Severity: Normal CC: charles.wei, eric.carlson, feature-media-reviews, leo.yang, mfeil, rwlbuis, ryuan.choi, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
WIP:minimum
none
Patch none

Description Jonathan Dong 2012-04-17 17:05:35 PDT
RIM PR: 117618

Add ability to handle http authentication challenge when loading media data.
When MMRPlayer receives a http authentication challenge while loading media file, it should notify browser to find an existing credential in memory or challenge user to provide one; and when this credential is accepted by MMRPlayer, it should notify browser to store the credential in CredentialStorage if needed.
Comment 1 Jonathan Dong 2012-04-18 19:31:05 PDT
Created attachment 137820 [details]
Patch
Comment 2 Antonio Gomes 2012-04-19 21:48:16 PDT
Comment on attachment 137820 [details]
Patch

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

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:685
> +        if (frameView() && frameView()->hostWindow())
> +            isConfirmed = frameView()->hostWindow()->platformPageClient()->authenticationChallenge(url, protectionSpace, credential);

that is a layering violation: webcore/platform code should not know about anything outside webcore/platform

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:687
> +            authChallenge.setCredential(string(credential.user().utf8().data()), string(credential.password().utf8().data()), static_cast<MMRAuthChallenge::CredentialPersistence>(credential.persistence()));

do we need string() here? does not it happen implicitly?

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:690
> +        authChallenge.setCredential(string(credential.user().utf8().data()), string(credential.password().utf8().data()), static_cast<MMRAuthChallenge::CredentialPersistence>(credential.persistence()));

ditto?
Comment 3 Jonathan Dong 2012-04-19 22:38:56 PDT
Comment on attachment 137820 [details]
Patch

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

>> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:685
>> +            isConfirmed = frameView()->hostWindow()->platformPageClient()->authenticationChallenge(url, protectionSpace, credential);
> 
> that is a layering violation: webcore/platform code should not know about anything outside webcore/platform

Thanks Antonio, yes here's a layer violation, and Max has filed a bug https://bugs.webkit.org/show_bug.cgi?id=84291 which intends to solve the very issue. I guess we can let this patch in and fix this violation together with PR 84291, is that acceptable?

>> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:687
>> +            authChallenge.setCredential(string(credential.user().utf8().data()), string(credential.password().utf8().data()), static_cast<MMRAuthChallenge::CredentialPersistence>(credential.persistence()));
> 
> do we need string() here? does not it happen implicitly?

Yeah you are right, will remove this in the next patch.
Comment 4 Jonathan Dong 2012-04-19 23:08:59 PDT
Created attachment 138047 [details]
Patch
Comment 5 Rob Buis 2012-05-04 13:24:59 PDT
Comment on attachment 138047 [details]
Patch

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

Still some stuff to clean up.

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:663
> +ProtectionSpace generateProtectionSpaceFromMMRAuthChallenge(const MMRAuthChallenge& authChallenge)

Should be static.

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:667
> +        return ProtectionSpace();

Better maybe ASSERT(url.isValid()); since the call sites already check it, so it should not happen.

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.cpp:691
> +    }

How about this? :

if (credential.isEmpty()) {
 if (frameView() && frameView()->hostWindow())
     isConfirmed = frameView()->hostWindow()->platformPageClient()->authenticationChallenge(url, protectionSpace, credential);
} else
    isConfirmed = true;

if (isConfirmed)
    authChallenge.setCredential(credential.user().utf8().data(), credential.password().utf8().data(), static_cast<MMRAuthChallenge::CredentialPersistence>(credential.persistence()));

> Source/WebCore/platform/graphics/blackberry/MediaPlayerPrivateBlackBerry.h:134
> +    virtual void onAuthenticationAccepted(const BlackBerry::Platform::MMRAuthChallenge&);

Can these be const?
Comment 6 Jonathan Dong 2012-05-04 16:19:14 PDT
Thanks Rob, Joe Mason and me is working on the http authentication refactor these days, I'll update this patch after the refactor work has done.
Comment 7 Ryuan Choi 2012-05-29 04:23:36 PDT
Created attachment 144512 [details]
WIP:minimum
Comment 8 Ryuan Choi 2012-05-29 04:25:30 PDT
Comment on attachment 144512 [details]
WIP:minimum

Sorry, I mistake number :(
Comment 9 Jonathan Dong 2012-05-29 18:48:33 PDT
Created attachment 144671 [details]
Patch
Comment 10 WebKit Review Bot 2012-05-29 20:51:38 PDT
Comment on attachment 144671 [details]
Patch

Clearing flags on attachment: 144671

Committed r118887: <http://trac.webkit.org/changeset/118887>
Comment 11 WebKit Review Bot 2012-05-29 20:51:45 PDT
All reviewed patches have been landed.  Closing bug.