Bug 73280 - Upstream credential storage files of BlackBerry porting
Summary: Upstream credential storage files of BlackBerry porting
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 73144
  Show dependency treegraph
 
Reported: 2011-11-28 21:38 PST by Jonathan Dong
Modified: 2011-12-01 07:56 PST (History)
5 users (show)

See Also:


Attachments
Patch (9.30 KB, patch)
2011-11-28 22:03 PST, Jonathan Dong
no flags Details | Formatted Diff | Diff
Patch (9.31 KB, patch)
2011-11-30 19:49 PST, Jonathan Dong
no flags Details | Formatted Diff | Diff
Patch (9.72 KB, patch)
2011-11-30 21:55 PST, 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 2011-11-28 21:38:27 PST
files: WebCore/platform/network/blackberry/CredentialStorageBlackBerry.(cpp|h),  WebCore/platform/network/blackberry/CredentialBackingStore.(cpp|h)
Comment 1 Jonathan Dong 2011-11-28 21:39:40 PST
WebCore/platform/network/blackberry/CredentialStorageBlackBerry.cpp,  WebCore/platform/network/blackberry/CredentialBackingStore.(cpp|h)

sorry.
Comment 2 Jonathan Dong 2011-11-28 22:03:27 PST
Created attachment 116891 [details]
Patch
Comment 3 Leo Yang 2011-11-30 19:08:31 PST
Comment on attachment 116891 [details]
Patch

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

This is an informal review. LGTM, but some minor style issues.

> Source/WebCore/platform/network/blackberry/CredentialBackingStore.cpp:35
> +CredentialBackingStore* CredentialBackingStore::instance()
> +{
> +    static CredentialBackingStore *backingStore = 0;

The star in the above line should be near the type.

> Source/WebCore/platform/network/blackberry/CredentialBackingStore.cpp:47
> +CredentialBackingStore::CredentialBackingStore()
> +    : m_addStatement(0),
> +    m_updateStatement(0),
> +    m_hasStatement(0),
> +    m_getStatement(0),
> +    m_removeStatement(0)
> +{

comma should be at the beginning of each member variable.

> Source/WebCore/platform/network/blackberry/CredentialBackingStore.cpp:103
> +String CredentialBackingStore::encryptedString(const String& plainText) const
> +{
> +    // FIXME: need encrypt plain_text here

Nit: need --> Need

> Source/WebCore/platform/network/blackberry/CredentialBackingStore.cpp:110
> +String CredentialBackingStore::decryptedString(const String& cipherText) const
> +{
> +    // FIXME: need decrypt cipher_text here

Ditto.

> Source/WebCore/platform/network/blackberry/CredentialBackingStore.h:36
> +    static CredentialBackingStore* instance();
> +    ~CredentialBackingStore();
> +

Blank line is not necessary.

> Source/WebCore/platform/network/blackberry/CredentialBackingStore.h:39
> +    bool open(const String& dbPath);
> +    void close();
> +

Ditto

> Source/WebCore/platform/network/blackberry/CredentialBackingStore.h:45
> +    Credential getLogin(const ProtectionSpace&);
> +    bool removeLogin(const ProtectionSpace&);
> +

Ditto
Comment 4 Jonathan Dong 2011-11-30 19:49:22 PST
Created attachment 117315 [details]
Patch
Comment 5 Antonio Gomes 2011-11-30 20:44:07 PST
Comment on attachment 117315 [details]
Patch

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

it looks good.

r- because we need another round to fix the nits.

> Source/WebCore/platform/network/blackberry/CredentialBackingStore.cpp:53
> +bool CredentialBackingStore::open(const String& dbPath)

UNUSED_PARAM(dbPath) missing.

> Source/WebCore/platform/network/blackberry/CredentialBackingStore.cpp:68
> +bool CredentialBackingStore::addLogin(const KURL& url, const ProtectionSpace& protectionSpace, const Credential& credential)
> +{
> +    notImplemented();
> +    return false;
> +}

ditto

> Source/WebCore/platform/network/blackberry/CredentialBackingStore.cpp:74
> +bool CredentialBackingStore::updateLogin(const KURL& url, const ProtectionSpace& protectionSpace, const Credential& credential)
> +{
> +    notImplemented();
> +    return false;
> +}

ditto

> Source/WebCore/platform/network/blackberry/CredentialBackingStore.cpp:86
> +Credential CredentialBackingStore::getLogin(const ProtectionSpace& protectionSpace)
> +{
> +    notImplemented();
> +    return Credential();
> +}

ditto

> Source/WebCore/platform/network/blackberry/CredentialBackingStore.cpp:92
> +{
> +    notImplemented();
> +    return false;
> +}

ditto

> Source/WebCore/platform/network/blackberry/CredentialBackingStore.h:55
> +    SQLiteStatement *m_addStatement;
> +    SQLiteStatement *m_updateStatement;
> +    SQLiteStatement *m_hasStatement;
> +    SQLiteStatement *m_getStatement;
> +    SQLiteStatement *m_removeStatement;

* position on the left

> Source/WebCore/platform/network/blackberry/CredentialStorageBlackBerry.cpp:35
> +#else
> +    return Credential();
> +#endif

UNUSED_PARAM needed here too.
Comment 6 Jonathan Dong 2011-11-30 21:55:27 PST
Created attachment 117334 [details]
Patch
Comment 7 Rob Buis 2011-12-01 06:40:37 PST
Comment on attachment 117334 [details]
Patch

Antonio's nits are fixed, so r+.
Comment 8 WebKit Review Bot 2011-12-01 07:56:28 PST
Comment on attachment 117334 [details]
Patch

Clearing flags on attachment: 117334

Committed r101668: <http://trac.webkit.org/changeset/101668>
Comment 9 WebKit Review Bot 2011-12-01 07:56:33 PST
All reviewed patches have been landed.  Closing bug.