Bug 76761 - [BlackBerry] Credential backing store implementation
Summary: [BlackBerry] Credential backing store implementation
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
Depends on:
Blocks: 73144
  Show dependency treegraph
Reported: 2012-01-20 18:02 PST by Jonathan Dong
Modified: 2012-01-30 04:50 PST (History)
3 users (show)

See Also:

Patch (11.71 KB, patch)
2012-01-20 18:13 PST, Jonathan Dong
no flags Details | Formatted Diff | Diff
Patch (11.76 KB, patch)
2012-01-29 18:42 PST, Jonathan Dong
tonikitoo: review+
Details | Formatted Diff | Diff
patch for cq after Antonio's review, also thanks Rob. (12.48 KB, patch)
2012-01-29 21:34 PST, Jonathan Dong
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
patch for cq (12.48 KB, patch)
2012-01-29 21:43 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 2012-01-20 18:02:20 PST
files: Source/WebCore/platform/network/blackberry/CredentialBackingStore.(h|cpp).
Comment 1 Jonathan Dong 2012-01-20 18:13:36 PST
Created attachment 123421 [details]
Comment 2 Rob Buis 2012-01-20 18:56:59 PST
Comment on attachment 123421 [details]

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

I think it can be improved a bit more, r- for now.

> Source/WebCore/platform/network/blackberry/CredentialBackingStore.cpp:66
> +        // create table logins

In  general: Make comments look like sentences by starting with a capital letter and ending with a period.

> Source/WebCore/platform/network/blackberry/CredentialBackingStore.cpp:104
> +    // prepare the statments.


> Source/WebCore/platform/network/blackberry/CredentialBackingStore.cpp:105
> +

Why empty line?

> Source/WebCore/platform/network/blackberry/CredentialBackingStore.cpp:144
> +    m_updateStatement =0;

= 0

> Source/WebCore/platform/network/blackberry/CredentialBackingStore.cpp:254
> +

Why empty line?

> Source/WebCore/platform/network/blackberry/CredentialBackingStore.cpp:255
> +    return credential;

Why do we need a temp var here?

> Source/WebCore/platform/network/blackberry/CredentialBackingStore.cpp:288
> +        return false;

This pattern is repeated a lot. Can you come up with macro(s)? If done correctly it will be much shorter and you don't need braces (go into macro).
Comment 3 Jonathan Dong 2012-01-29 18:42:53 PST
Created attachment 124484 [details]
Comment 4 Antonio Gomes 2012-01-29 20:49:50 PST
Comment on attachment 124484 [details]

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

it loosk good, r+

Please re-upload with the suggestions addressed if you think they are convenient, and add "Reviewed by Antonio Gomes" to the new patch/changelog. No need to another review round, only cq+ needed.

> Source/WebCore/platform/network/blackberry/CredentialBackingStore.cpp:70
> +        // Create table logins

I would drop this comment. it looks unneeded given the line below it. Also lacks a "." at the end :-)

> Source/WebCore/platform/network/blackberry/CredentialBackingStore.cpp:74
> +        // Create index for table logins


> Source/WebCore/platform/network/blackberry/CredentialBackingStore.cpp:77
> +    } else { // Initiate CredentialStorage

. in the end
Comment 5 Jonathan Dong 2012-01-29 21:34:10 PST
Created attachment 124493 [details]
patch for cq after Antonio's review, also thanks Rob.
Comment 6 WebKit Review Bot 2012-01-29 21:34:51 PST
Comment on attachment 124493 [details]
patch for cq after Antonio's review, also thanks Rob.

Rejecting attachment 124493 [details] from commit-queue.

jonathan.dong@torchmobile.com.cn does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 7 Jonathan Dong 2012-01-29 21:43:08 PST
Created attachment 124494 [details]
patch for cq

re-attach the patch as wrongly set the commit-queue flag for the last patch. :(
Comment 8 Rob Buis 2012-01-30 04:35:24 PST
Comment on attachment 124494 [details]
patch for cq

Use cq
Comment 9 WebKit Review Bot 2012-01-30 04:50:13 PST
Comment on attachment 124494 [details]
patch for cq

Clearing flags on attachment: 124494

Committed r106234: <http://trac.webkit.org/changeset/106234>
Comment 10 WebKit Review Bot 2012-01-30 04:50:20 PST
All reviewed patches have been landed.  Closing bug.