Summary: | [BlackBerry] Autofill backing store implementation upstream | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jonathan Dong <jonathan.dong.webkit> | ||||||||||
Component: | WebKit BlackBerry | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | charles.wei, leo.yang, mstaikos, rakuco, rwlbuis, staikos, tonikitoo, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 85576 | ||||||||||||
Attachments: |
|
Description
Jonathan Dong
2012-05-03 21:09:45 PDT
Created attachment 140169 [details]
Patch
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. Created attachment 140251 [details]
Patch
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 on attachment 140251 [details] Patch Attachment 140251 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12480935 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. 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 on attachment 140251 [details]
Patch
Clearing review flag since Jonathan will fix the dtor issues in a new patch.
Created attachment 140421 [details]
Patch
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. Created attachment 140445 [details]
Patch
Comment on attachment 140445 [details]
Patch
Looks good.
Comment on attachment 140445 [details] Patch Clearing flags on attachment: 140445 Committed r116258: <http://trac.webkit.org/changeset/116258> All reviewed patches have been landed. Closing bug. |