| Summary: | [BlackBerry] Credential save and autofill implemetation | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jonathan Dong <jonathan.dong.webkit> | ||||||||||||
| Component: | WebKit BlackBerry | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | charles.wei, joenotcharles, leo.yang, liachen, rwlbuis, tonikitoo, webkit.review.bot | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Jonathan Dong
2012-03-06 02:09:50 PST
Created attachment 130343 [details]
Patch
Comment on attachment 130343 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130343&action=review I will like we are missing an abstraction layer here :( ... the layering violation and the 'friend' class declaration makes me kinda of sad. > Source/WebCore/platform/network/blackberry/NetworkJob.cpp:841 > + Credential inputCredential = static_cast<FrameLoaderClientBlackBerry*>(m_frame->loader()->client())->authenticationChallenge(newURL, protectionSpace); That is a layering violation. Created attachment 132177 [details]
Patch
Comment on attachment 132177 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132177&action=review Looks good, see if you can fix before landing, and Leo or Charles can help cq+ or commit for you. > Source/WebKit/blackberry/Api/WebPage.cpp:2024 > + WebCore::Credential inputCredential(username, password, WebCore::CredentialPersistenceNone); You dont need WebCore:: prefix in this file. > Source/WebKit/blackberry/Api/WebPage_p.h:180 > + virtual PageClientBlackBerry::SaveCredentialType notifyShouldSaveCredential(bool); PageClientBlackBerry:: prefix might be not needed. Created attachment 132948 [details]
Patch
Comment on attachment 132948 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132948&action=review Looks good. > Source/WebCore/ChangeLog:6 > + Reviewed by Rob Buis. You probably mean internally reviewed here. Since I am doing both the internal and external review this is fine, but better keep it blank (i.e. NOBODY). Comment on attachment 132948 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132948&action=review > Source/WebCore/ChangeLog:10 > + As this class is our platform independent interface, should be "platform specific interface". > Source/WebCore/ChangeLog:12 > + FrameLoaderClient cause which might be intrusion. "cause which might be intrusion" ==>which is a platform independent interface. You also need to address comment #4 (In reply to comment #8) > You also need to address comment #4 Ah, good catch... Cheers, Rob. Created attachment 132956 [details]
Patch
Sorry for the carelessness and thanks for the help
Comment on attachment 132956 [details]
Patch
Looks good.
Comment on attachment 132956 [details] Patch Rejecting attachment 132956 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/12035387 Comment on attachment 132956 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132956&action=review > Source/WebCore/ChangeLog:6 > + Reviewed by NOBODY (OPPS!). Typo! > Source/WebKit/blackberry/ChangeLog:6 > + Reviewed by NOBODY (OPPS!). Ditto. Created attachment 133036 [details]
Patch
Comment on attachment 133036 [details]
Patch
Lets retry.
Comment on attachment 133036 [details] Patch Clearing flags on attachment: 133036 Committed r111546: <http://trac.webkit.org/changeset/111546> All reviewed patches have been landed. Closing bug. |