Bug 80401 - [BlackBerry] Credential save and autofill implemetation
Summary: [BlackBerry] Credential save and autofill implemetation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-06 02:09 PST by Jonathan Dong
Modified: 2012-03-21 08:23 PDT (History)
7 users (show)

See Also:


Attachments
Patch (11.51 KB, patch)
2012-03-06 02:44 PST, Jonathan Dong
no flags Details | Formatted Diff | Diff
Patch (19.25 KB, patch)
2012-03-15 19:35 PDT, Jonathan Dong
no flags Details | Formatted Diff | Diff
Patch (19.14 KB, patch)
2012-03-20 19:19 PDT, Jonathan Dong
no flags Details | Formatted Diff | Diff
Patch (19.41 KB, patch)
2012-03-20 20:43 PDT, Jonathan Dong
no flags Details | Formatted Diff | Diff
Patch (19.34 KB, patch)
2012-03-21 06:59 PDT, Jonathan Dong
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Dong 2012-03-06 02:09:50 PST
Implement credential autofill feature for blackberry porting.

Including http authentication dialog save and autofill, as well as in-form authentication save and autofill.
Comment 1 Jonathan Dong 2012-03-06 02:44:31 PST
Created attachment 130343 [details]
Patch
Comment 2 Antonio Gomes 2012-03-10 17:06:27 PST
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.
Comment 3 Jonathan Dong 2012-03-15 19:35:27 PDT
Created attachment 132177 [details]
Patch
Comment 4 Rob Buis 2012-03-20 11:25:28 PDT
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.
Comment 5 Jonathan Dong 2012-03-20 19:19:52 PDT
Created attachment 132948 [details]
Patch
Comment 6 Rob Buis 2012-03-20 19:24:35 PDT
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 7 Charles Wei 2012-03-20 19:27:35 PDT
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.
Comment 8 Leo Yang 2012-03-20 19:29:20 PDT
You also need to address comment #4
Comment 9 Rob Buis 2012-03-20 19:34:13 PDT
(In reply to comment #8)
> You also need to address comment #4

Ah, good catch...
Cheers,

Rob.
Comment 10 Jonathan Dong 2012-03-20 20:43:50 PDT
Created attachment 132956 [details]
Patch

Sorry for the carelessness and thanks for the help
Comment 11 Rob Buis 2012-03-21 04:26:09 PDT
Comment on attachment 132956 [details]
Patch

Looks good.
Comment 12 WebKit Review Bot 2012-03-21 04:27:51 PDT
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 13 Rob Buis 2012-03-21 04:31:24 PDT
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.
Comment 14 Jonathan Dong 2012-03-21 06:59:43 PDT
Created attachment 133036 [details]
Patch
Comment 15 Rob Buis 2012-03-21 08:15:56 PDT
Comment on attachment 133036 [details]
Patch

Lets retry.
Comment 16 WebKit Review Bot 2012-03-21 08:23:50 PDT
Comment on attachment 133036 [details]
Patch

Clearing flags on attachment: 133036

Committed r111546: <http://trac.webkit.org/changeset/111546>
Comment 17 WebKit Review Bot 2012-03-21 08:23:55 PDT
All reviewed patches have been landed.  Closing bug.