Summary: | [BlackBerry] Loading media data with http authentication | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jonathan Dong <jonathan.dong.webkit> | ||||||||||
Component: | WebKit BlackBerry | Assignee: | 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
Jonathan Dong
2012-04-17 17:05:35 PDT
Created attachment 137820 [details]
Patch
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 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. Created attachment 138047 [details]
Patch
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? 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. Created attachment 144512 [details]
WIP:minimum
Comment on attachment 144512 [details]
WIP:minimum
Sorry, I mistake number :(
Created attachment 144671 [details]
Patch
Comment on attachment 144671 [details] Patch Clearing flags on attachment: 144671 Committed r118887: <http://trac.webkit.org/changeset/118887> All reviewed patches have been landed. Closing bug. |