Bug 85575 - [BlackBerry] Autofill backing store implementation upstream
Summary: [BlackBerry] Autofill backing store implementation upstream
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: 85576
  Show dependency treegraph
 
Reported: 2012-05-03 21:09 PDT by Jonathan Dong
Modified: 2012-05-06 18:50 PDT (History)
8 users (show)

See Also:


Attachments
Patch (11.33 KB, patch)
2012-05-03 23:57 PDT, Jonathan Dong
no flags Details | Formatted Diff | Diff
Patch (11.36 KB, patch)
2012-05-04 09:26 PDT, Jonathan Dong
no flags Details | Formatted Diff | Diff
Patch (21.85 KB, patch)
2012-05-06 03:22 PDT, Jonathan Dong
no flags Details | Formatted Diff | Diff
Patch (21.95 KB, patch)
2012-05-06 16:53 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-05-03 21:09:45 PDT
RIM PR: 136405

The form auto fill feature by my current implementation is pretty much like the behavior of desktop browsers, when user submits a form, the name attribute and the value of text input fields(exclude password) will be stored in the autofill database as a name-value pair. Next time when the user fills any form on any site, the browser will search the db to find out if there's any matched name-value pair to fill the current editing input text field. We will find all the values which has the same name attribute and starts with the character that user has input, and pops up a context dialog to notify user those candidates used by him. If user chooses one from the context dialog, the value will fill the current input text field.

This PR is about the implementation of class AutofillBackingStore, which takes over all the database interaction for AutofillManager.
Comment 1 Jonathan Dong 2012-05-03 23:57:12 PDT
Created attachment 140169 [details]
Patch
Comment 2 Rob Buis 2012-05-04 04:49:44 PDT
Comment on attachment 140169 [details]
Patch

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

Can be cleaned up some more.

> Source/WebCore/platform/network/blackberry/AutofillBackingStore.cpp:24
> +#include "NotImplemented.h"

Can be removed?

> Source/WebCore/platform/network/blackberry/AutofillBackingStore.cpp:163
> +    int numOfRow = m_hasStatement->getColumnInt(0);

Do you mean numberOfRows here or rowNumber? 
Better not abbreviate.

> Source/WebCore/platform/network/blackberry/AutofillBackingStore.cpp:170
> +    return false;

You can combine these into one return statement.

> Source/WebCore/platform/network/blackberry/AutofillBackingStore.h:23
> +#include <wtf/Vector.h>

Does not seem like we need this include here.

> Source/WebCore/platform/network/blackberry/AutofillBackingStore.h:40
> +    bool has(const String& name, const String& value);

has is an unclear name for a function. How about contains?

> Source/WebCore/platform/network/blackberry/AutofillBackingStore.h:41
> +

Please check if any of the functions are const.
Comment 3 Jonathan Dong 2012-05-04 09:26:12 PDT
Created attachment 140251 [details]
Patch
Comment 4 Rob Buis 2012-05-04 09:34:43 PDT
Comment on attachment 140251 [details]
Patch

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

> Source/WebCore/platform/network/blackberry/AutofillBackingStore.cpp:56
> +        m_database.close();

Should the dtor here do similar things like close()? Can you assure somewhow close() is called before the dtor is called? Is the dtor called at all?
Comment 5 Build Bot 2012-05-04 09:47:42 PDT
Comment on attachment 140251 [details]
Patch

Attachment 140251 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12480935
Comment 6 Jonathan Dong 2012-05-04 09:53:41 PDT
Comment on attachment 140251 [details]
Patch

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

>> Source/WebCore/platform/network/blackberry/AutofillBackingStore.cpp:56
>> +        m_database.close();
> 
> Should the dtor here do similar things like close()? Can you assure somewhow close() is called before the dtor is called? Is the dtor called at all?

well that's a good question. I think as AutofillBackingStore works as a singleton, close() should make as a private function and called from dtor, we shouldn't let people call close() explicitly.
dtor should be called after exit main(), I'll make a build and debug it tomorrow to see if it works as expected, too tired today.
I guess CredentialBackingStore has the same issue, sorry for that. Will fix it together with next patch. Thanks.
Comment 7 Rob Buis 2012-05-04 10:44:56 PDT
Hi Jonathan,

(In reply to comment #6)
> (From update of attachment 140251 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140251&action=review
> 
> >> Source/WebCore/platform/network/blackberry/AutofillBackingStore.cpp:56
> >> +        m_database.close();
> > 
> > Should the dtor here do similar things like close()? Can you assure somewhow close() is called before the dtor is called? Is the dtor called at all?
> 
> well that's a good question. I think as AutofillBackingStore works as a singleton, close() should make as a private function and called from dtor, we shouldn't let people call close() explicitly.
> dtor should be called after exit main(), I'll make a build and debug it tomorrow to see if it works as expected, too tired today.
> I guess CredentialBackingStore has the same issue, sorry for that. Will fix it together with next patch. Thanks.

Sounds good. Also if you use DEFINE_STATIC_LOCAL for the singleton it should call the dtor, maybe you can debug that.
If you only call close() from the dtor then maybe you do not even need that function but just put the code in the dtor.
Looking forward to the patch :)
Cheers,

Rob.
Comment 8 Rob Buis 2012-05-04 10:47:29 PDT
Comment on attachment 140251 [details]
Patch

Clearing review flag since Jonathan will fix the dtor issues in a new patch.
Comment 9 Jonathan Dong 2012-05-06 03:22:07 PDT
Created attachment 140421 [details]
Patch
Comment 10 Rob Buis 2012-05-06 16:30:16 PDT
Comment on attachment 140421 [details]
Patch

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

Looks good. I don't really see the need to move away from instance() concept, but ok.

> Source/WebCore/platform/network/blackberry/AutofillBackingStore.cpp:185
> +

One newline too much.
Comment 11 Jonathan Dong 2012-05-06 16:53:57 PDT
Created attachment 140445 [details]
Patch
Comment 12 Rob Buis 2012-05-06 16:58:37 PDT
Comment on attachment 140445 [details]
Patch

Looks good.
Comment 13 WebKit Review Bot 2012-05-06 18:50:01 PDT
Comment on attachment 140445 [details]
Patch

Clearing flags on attachment: 140445

Committed r116258: <http://trac.webkit.org/changeset/116258>
Comment 14 WebKit Review Bot 2012-05-06 18:50:08 PDT
All reviewed patches have been landed.  Closing bug.