Closed
Bug 711447
Opened 13 years ago
Closed 13 years ago
Close connection in nsPermissionManager
Categories
(Core :: Networking: Cookies, defect)
Tracking
()
RESOLVED
FIXED
mozilla12
People
(Reporter: espindola, Assigned: espindola)
References
Details
(Whiteboard: [Snappy:p1])
Attachments
(1 file, 2 obsolete files)
No description provided.
Assignee | ||
Comment 1•13 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=06d000ca95bf
Comment 2•13 years ago
|
||
Comment on attachment 582267 [details] [diff] [review] Close connection in nsPermissionManager Review of attachment 582267 [details] [diff] [review]: ----------------------------------------------------------------- looks ok
Attachment #582267 -
Flags: review?(mak77) → review+
Comment 3•13 years ago
|
||
Comment on attachment 582267 [details] [diff] [review] Close connection in nsPermissionManager Review of attachment 582267 [details] [diff] [review]: ----------------------------------------------------------------- ::: extensions/cookie/nsPermissionManager.cpp @@ +797,5 @@ > mStmtInsert = nsnull; > mStmtDelete = nsnull; > mStmtUpdate = nsnull; > + mozilla::DebugOnly<nsresult> rv = mDBConn->Close(); > + MOZ_ASSERT(NS_SUCCEEDED(rv)); You should add a helper method to this and make this and `RemoveAllInternal` call the helper. We were doing it right in one place, but not the other.
Attachment #582267 -
Flags: review-
Updated•13 years ago
|
Component: Storage → Networking: Cookies
Product: Toolkit → Core
QA Contact: storage → networking.cookies
Version: unspecified → Trunk
Assignee | ||
Comment 4•13 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=6601609921ae
Attachment #582267 -
Attachment is obsolete: true
Attachment #582835 -
Flags: review?(sdwilsh)
Attachment #582835 -
Flags: review?(mak77)
Comment 5•13 years ago
|
||
Comment on attachment 582835 [details] [diff] [review] Close connection in nsPermissionManager Review of attachment 582835 [details] [diff] [review]: ----------------------------------------------------------------- ::: extensions/cookie/nsPermissionManager.cpp @@ +620,5 @@ > return rv; > } > > +void > +nsPermissionManager::CloseDBInternal() I don't like the Internal suffix here, we usually use it when there's another exposed method with the same name, that's not this case, just CloseDB() will be fine afaict. @@ +646,1 @@ > mDBConn = nsnull; you can move this inside the helper "if (mDBConn)" scope ::: extensions/cookie/nsPermissionManager.h @@ +221,5 @@ > PRUint32 aExpireType, > PRInt64 aExpireTime, > const PRUnichar *aData); > void NotifyObservers(nsIPermission *aPermission, const PRUnichar *aData); > + void CloseDBInternal(); this may like a nice but simple javadoc explaining what it does.
Assignee | ||
Comment 6•13 years ago
|
||
This implement all the comments, except for the javadoc, since the file had no javadoc I just added a plain comment. https://tbpl.mozilla.org/?tree=Try&rev=8e4a8b9a5073
Attachment #582835 -
Attachment is obsolete: true
Attachment #582835 -
Flags: review?(sdwilsh)
Attachment #582835 -
Flags: review?(mak77)
Attachment #582972 -
Flags: review?(mak77)
Comment 7•13 years ago
|
||
Comment on attachment 582972 [details] [diff] [review] Close connection in nsPermissionManager Review of attachment 582972 [details] [diff] [review]: ----------------------------------------------------------------- I don't have further comments, I'm forwarding final review to either swilsh or dwitte, on a first come first serve handling, since sdwilsh appears to be on vacation
Attachment #582972 -
Flags: review?(sdwilsh)
Attachment #582972 -
Flags: review?(mak77)
Attachment #582972 -
Flags: review?(dwitte)
Attachment #582972 -
Flags: feedback+
Comment 8•13 years ago
|
||
Comment on attachment 582972 [details] [diff] [review] Close connection in nsPermissionManager Review of attachment 582972 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine to me too. I can't technically r+ this, so f+ at taras's request.
Attachment #582972 -
Flags: feedback+
Comment 9•13 years ago
|
||
Comment on attachment 582972 [details] [diff] [review] Close connection in nsPermissionManager Looks good, thanks. r=dwitte
Attachment #582972 -
Flags: review?(sdwilsh)
Attachment #582972 -
Flags: review?(dwitte)
Attachment #582972 -
Flags: review+
Assignee | ||
Comment 10•13 years ago
|
||
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=ed120f1a9dea
Comment 11•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ed120f1a9dea
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in
before you can comment on or make changes to this bug.
Description
•