Bug 84214 – [BlackBerry] Loading media data with http authentication

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit BlackBerry

[Jonathan Dong]

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.

[Jonathan Dong]

Created attachment 137820 [details]
Patch

[Antonio Gomes]

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?

[Jonathan Dong]

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.

[Jonathan Dong]

Created attachment 138047 [details]
Patch

[Rob Buis]

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?

[Jonathan Dong]

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.

[Ryuan Choi]

Created attachment 144512 [details]
WIP:minimum

[Ryuan Choi]

Comment on attachment 144512 [details]
WIP:minimum

Sorry, I mistake number :(

[Jonathan Dong]

Created attachment 144671 [details]
Patch

[WebKit Review Bot]

Comment on attachment 144671 [details]
Patch

Clearing flags on attachment: 144671

Committed r118887: <http://trac.webkit.org/changeset/118887>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.