Closed
Bug 553489
Opened 15 years ago
Closed 15 years ago
make setAndLoadFaviconForPage completely async
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a4
People
(Reporter: mak, Assigned: mak)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-needed)
Attachments
(11 files, 11 obsolete files)
(deleted),
patch
|
sdwilsh
:
review+
vlad
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
this implements basic prototype to make all methods async, but uses it only in that mehod for now, since it's what browser uses to set icons. Further methods will follow in parent bug, the design has been done so that all methods can be made async with a minimum effort.
Assignee | ||
Comment 1•15 years ago
|
||
Assignee | ||
Comment 2•15 years ago
|
||
Assignee | ||
Comment 3•15 years ago
|
||
Assignee | ||
Updated•15 years ago
|
Attachment #433486 -
Attachment is patch: true
Attachment #433486 -
Attachment mime type: application/octet-stream → text/plain
Assignee | ||
Comment 4•15 years ago
|
||
Assignee | ||
Comment 5•15 years ago
|
||
Assignee | ||
Comment 6•15 years ago
|
||
Assignee | ||
Comment 7•15 years ago
|
||
Assignee | ||
Comment 8•15 years ago
|
||
Assignee | ||
Comment 9•15 years ago
|
||
Assignee | ||
Comment 10•15 years ago
|
||
Assignee | ||
Comment 11•15 years ago
|
||
side notes: - i can't kill LAZY_ADD till visits will be async (https://bugzilla.mozilla.org/show_bug.cgi?id=540765#c19 and following) - i decided to callback only on success or expected CANCEL, other situations (CANCEL without icon or FAIL) are really not interesting for implementers. This will make tests a bit harder, but i figured out a timed way to write a test (TODO). - current tests are good enough for success cases, i'll write a test for CANCEL cases, using a timeout. This means i could still find some glitch, but i think that should not block starting the review, i can add further patches as new parts instead. - these patches have a bit of dependency that should go away in the next days. patch in bug 546942 should be applied before them due to Makefile conflicts.
Assignee | ||
Updated•15 years ago
|
Attachment #433486 -
Attachment description: Part3: getEffectivePageStep → Part 3: getEffectivePageStep
Assignee | ||
Updated•15 years ago
|
Attachment #433491 -
Attachment description: Part8: AssociateIconWithPageStep → Part 8: AssociateIconWithPageStep
Assignee | ||
Updated•15 years ago
|
Attachment #433484 -
Flags: review?(sdwilsh)
Assignee | ||
Updated•15 years ago
|
Attachment #433485 -
Flags: review?(sdwilsh)
Assignee | ||
Updated•15 years ago
|
Attachment #433486 -
Flags: review?(sdwilsh)
Assignee | ||
Updated•15 years ago
|
Attachment #433487 -
Flags: review?(sdwilsh)
Assignee | ||
Updated•15 years ago
|
Attachment #433488 -
Flags: review?(sdwilsh)
Assignee | ||
Updated•15 years ago
|
Attachment #433489 -
Flags: review?(sdwilsh)
Assignee | ||
Updated•15 years ago
|
Attachment #433490 -
Flags: review?(sdwilsh)
Assignee | ||
Updated•15 years ago
|
Attachment #433491 -
Flags: review?(sdwilsh)
Assignee | ||
Updated•15 years ago
|
Attachment #433492 -
Flags: review?(sdwilsh)
Assignee | ||
Updated•15 years ago
|
Attachment #433493 -
Flags: review?(sdwilsh)
Assignee | ||
Comment 12•15 years ago
|
||
- note: i also need a success test for the callback.
Flags: in-testsuite?
Comment 13•15 years ago
|
||
Comment on attachment 433484 [details] [diff] [review] Part 1: IDL changes r=sdwilsh
Attachment #433484 -
Flags: superreview?(robert.bugzilla)
Attachment #433484 -
Flags: review?(sdwilsh)
Attachment #433484 -
Flags: review+
Comment 14•15 years ago
|
||
Comment on attachment 433485 [details] [diff] [review] Part 2: AsyncFaviconStepper implementation >+nsresult >+AsyncFaviconStepper::GetIconData(nsACString& aMimeType, >+ const PRUint8** aData, >+ PRUint32* aDataLen) nit: _data and _dataLen since these are out params >+AsyncFaviconStepperInternal::AsyncFaviconStepperInternal( >+ nsIFaviconDataCallback* aCallback) nit: closing paren on new line with no indent >+void >+AsyncFaviconStepperInternal::Failure() >+{ >+ mStatus = STEPPER_FAILED; >+ >+ // Break the cycles so steps are collected. >+ mSteps.Clear(); >+ >+ //XXX notify Error to caller? seems rather difficult to do, yeah (short of doing the callback for errors like you mentioned before)? >+void >+AsyncFaviconStepperInternal::Cancel(PRBool aNotify) nit: use bool, not PRBool I'd like dietrich to skim this too, but r=sdwilsh
Attachment #433485 -
Flags: review?(sdwilsh) → review+
Updated•15 years ago
|
Attachment #433485 -
Flags: review?(dietrich)
Comment 15•15 years ago
|
||
Comment on attachment 433486 [details] [diff] [review] Part 3: getEffectivePageStep >Bug 540765 - Part3: GetEffectivePageStep > >diff --git a/toolkit/components/places/src/AsyncFaviconHelpers.cpp b/toolkit/components/places/src/AsyncFaviconHelpers.cpp >--- a/toolkit/components/places/src/AsyncFaviconHelpers.cpp >+++ b/toolkit/components/places/src/AsyncFaviconHelpers.cpp >@@ -40,11 +40,45 @@ > > #include "mozilla/storage.h" > >+#include "nsNetUtil.h" >+ >+#include "nsNavHistory.h" >+#include "nsNavBookmarks.h" >+#include "nsFaviconService.h" >+ > #define TO_CHARBUFFER(_buffer) \ > reinterpret_cast<char*>(const_cast<PRUint8*>(_buffer)) > #define TO_INTBUFFER(_string) \ > reinterpret_cast<PRUint8*>(const_cast<char*>(_string.get())) > >+#ifdef DEBUG >+#define ASYNC_STATEMENT_HANDLEERROR_IMPL(_class) \ >+NS_IMETHODIMP \ >+_class::HandleError(mozIStorageError *aError) \ >+{ \ >+ PRInt32 result; \ >+ nsresult rv = aError->GetResult(&result); \ >+ NS_ENSURE_SUCCESS(rv, rv); \ >+ nsCAutoString message; \ >+ rv = aError->GetMessage(message); \ >+ NS_ENSURE_SUCCESS(rv, rv); \ >+ nsCAutoString warnMsg; \ >+ warnMsg.Append("An error occured while executing an async statement: "); \ >+ warnMsg.Append(result); \ >+ warnMsg.Append(" "); \ >+ warnMsg.Append(message); \ >+ NS_WARNING(warnMsg.get()); \ >+ FAVICONSTEP_FAIL_IF_FALSE_RV(false, NS_OK); \ >+} >+#else >+#define ASYNC_STATEMENT_HANDLEERROR_IMPL(_class) \ >+NS_IMETHODIMP \ >+_class::HandleError(mozIStorageError *aError) \ >+{ \ >+ FAVICONSTEP_FAIL_IF_FALSE_RV(false, NS_OK); \ >+} >+#endif We should be inheriting from mozilla::places::AsyncStatementCallback but override the error handler. In debug builds, we will still call the parent class's HandleError method, but in release builds just do the FAVICONSTEP_FAIL_IF_FALSE_RV. >+ mozIStorageStatement* GetStatementById( >+ enum mozilla::places::BookmarkStatementId aStatementId) nit: closing parent on new line even with "mozIStorageStatement*" indent level >+ { >+ switch(aStatementId) { >+ case mozilla::places::DB_FIND_REDIRECTED_BOOKMARK: >+ return GetStatement(mDBFindRedirectedBookmark); nit: I think you can place a using statement inside the function and not have to have all the namespacing in the switch statement. >+ mozIStorageStatement* GetStatementById( >+ enum mozilla::places::HistoryStatementId aStatementId) ditto >+ { >+ switch(aStatementId) { >+ case mozilla::places::DB_GET_PAGE_INFO: >+ return mDBGetURLPageInfo; ditto r=sdwilsh with these comments addressed
Attachment #433486 -
Flags: review?(sdwilsh) → review+
Comment 16•15 years ago
|
||
Comment on attachment 433487 [details] [diff] [review] Part 4: fetchDatabaseIconStep >+ASYNC_STATEMENT_HANDLEERROR_IMPL(FetchDatabaseIconStep) same comment from earlier patches >+void >+FetchDatabaseIconStep::Run() { isn't our style to have the opening brace on a new line? >+ if (mStepper->mPageURI) >+ rv = BindStatementURI(stmt, 1, mStepper->mPageURI); >+ else >+ rv = stmt->BindNullParameter(1); style says to always brace, right? >+ mozIStorageStatement* GetStatementById( >+ enum mozilla::places::FaviconStatementId aStatementId) nit: trailing parent on new line >+ { >+ switch(aStatementId) { >+ case mozilla::places::DB_GET_ICON_INFO_WITH_PAGE: >+ return GetStatement(mDBGetIconInfoWithPage); nit: use a using statement here r=sdwilsh with comments addressed
Attachment #433487 -
Flags: review?(sdwilsh) → review+
Comment 17•15 years ago
|
||
Comment on attachment 433488 [details] [diff] [review] Part 5: EnsureEntryStep >+#define ASYNC_STATEMENT_EMPTY_HANDLERESULT_IMPL(_class) \ >+NS_IMETHODIMP \ >+_class::HandleResult(mozIStorageResultSet* aResultSet) \ >+{ \ >+ return NS_OK; \ >+} In theory, we shouldn't ever get here, right? Add an NS_NOTREACHED here? >+ASYNC_STATEMENT_HANDLEERROR_IMPL(EnsureDatabaseEntryStep) ditto re: previous error handling comments >+void >+EnsureDatabaseEntryStep::Run() { nit: new line for opening brace >+NS_IMETHODIMP >+EnsureDatabaseEntryStep::HandleCompletion(PRUint16 aReason) >+{ >+ FAVICONSTEP_FAIL_IF_FALSE_RV(aReason == mozIStorageStatementCallback::REASON_FINISHED, NS_OK); >+ >+ // XXX Get the new icon id? Do we need it later? We don't need this comment here, right? We don't need the id AFAICT... >@@ -149,6 +150,8 @@ public: > switch(aStatementId) { > case mozilla::places::DB_GET_ICON_INFO_WITH_PAGE: > return GetStatement(mDBGetIconInfoWithPage); >+ case mozilla::places::DB_INSERT_ICON: >+ return GetStatement(mDBInsertIcon); you should be using a using statement, so update this part too please r=sdwilsh with comments addressed
Attachment #433488 -
Flags: review?(sdwilsh) → review+
Comment 18•15 years ago
|
||
Comment on attachment 433489 [details] [diff] [review] Part 6: FetchNetworkIconStep > //////////////////////////////////////////////////////////////////////////////// > // AsyncFaviconStep oops, I think I missed this in past patches, but four '/' not two on the second line > //////////////////////////////////////////////////////////////////////////////// >+// FetchNetworkIconStep nit: four, not two slashes please >+void >+FetchNetworkIconStep::Run() { you really like having strange style for this run method, don't you? :P >+ // If we did not obtain a time from the cache, or it was negative, use our cap >+ if (expiration < 0) >+ expiration = PR_Now() + MAX_FAVICON_EXPIRATION; nit: brace one line ifs please >+ nsCAutoString buffer; >+ nsresult rv = NS_ConsumeStream(aInputStream, aCount, buffer); >+ if (rv != NS_BASE_STREAM_WOULD_BLOCK && NS_FAILED(rv)) >+ return rv; nit: brace one line ifs please r=sdwilsh with comments addressed
Attachment #433489 -
Flags: review?(sdwilsh) → review+
Comment 19•15 years ago
|
||
Comment on attachment 433490 [details] [diff] [review] Part 7: SetFaviconDataStep > //////////////////////////////////////////////////////////////////////////////// >+// SetFaviconDataStep nit: four slashes please >+ASYNC_STATEMENT_HANDLEERROR_IMPL(SetFaviconDataStep) ditto per previous error handler callback comments >+void >+SetFaviconDataStep::Run() { Oh hi misplaced opening brace >+ if (!mStepper->mIconId) >+ rv = stmt->BindNullParameter(0); >+ else >+ rv = stmt->BindInt64Parameter(0, mStepper->mIconId); nit: brace if and else >+//////////////////////////////////////////////////////////////////////////////// phantom slashes? r=sdwilsh with comments addressed
Attachment #433490 -
Flags: review?(sdwilsh) → review+
Comment 20•15 years ago
|
||
Comment on attachment 433491 [details] [diff] [review] Part 8: AssociateIconWithPageStep >+ASYNC_STATEMENT_HANDLEERROR_IMPL(AssociateIconWithPageStep) ditto >+void >+AssociateIconWithPageStep::Run() { :/ >+ // The API does not say anything about what happens when we try to set an >+ // icon for a never seen page, but the service always tried to create it. >+ // Creating a new moz_places entry is a complex task that we don't want to >+ // replicate here, so, till that process will be async, we just go sync. >+ // Notice that the common case is that we add the visit before adding the >+ // icon, so this should hurt only direct and isolated API calls. so much sadfaces - file a bug and reference it in the comments here? >+//////////////////////////////////////////////////////////////////////////////// phantom slashes again? I don't recall these showing up in previous files > return GetStatement(mDBGetIconInfoWithPage); > case mozilla::places::DB_INSERT_ICON: > return GetStatement(mDBInsertIcon); >+ case mozilla::places::DB_ASSOCIATE_ICONURI_TO_PAGEURI: >+ return GetStatement(mDBAssociateFaviconURIToPageURI); using! r=sdwilsh with comments addressed
Attachment #433491 -
Flags: review?(sdwilsh) → review+
Comment 21•15 years ago
|
||
Comment on attachment 433492 [details] [diff] [review] Part 9: NotifyStep >+void >+NotifyStep::Run() >+{ Yay! this one was right! r=sdwilsh
Attachment #433492 -
Flags: review?(sdwilsh) → review+
Updated•15 years ago
|
Attachment #433493 -
Flags: superreview?(robert.bugzilla)
Attachment #433493 -
Flags: review?(sdwilsh)
Attachment #433493 -
Flags: review+
Comment 22•15 years ago
|
||
Comment on attachment 433493 [details] [diff] [review] Part 10: setAndLoadFaviconForPage implementation >-//////////////////////////////////////////////////////////////////////////////// >-//// ExpireFaviconsStatementCallbackNotifier definition >- >+// ExpireFaviconsStatementCallbackNotifier forward declaration. >+// But it's not a forward declaration - it's the definition r=sdwilsh. This needs sr for API change.
Comment 23•15 years ago
|
||
Comment on attachment 433485 [details] [diff] [review] Part 2: AsyncFaviconStepper implementation looks good, r=me.
Attachment #433485 -
Flags: review?(dietrich) → review+
Assignee | ||
Comment 24•15 years ago
|
||
note to self, latest patch should rev uuid.
Comment 25•15 years ago
|
||
(In reply to comment #22) > (From update of attachment 433493 [details] [diff] [review]) > >-//////////////////////////////////////////////////////////////////////////////// > >-//// ExpireFaviconsStatementCallbackNotifier definition > >- > >+// ExpireFaviconsStatementCallbackNotifier forward declaration. > >+// > But it's not a forward declaration - it's the definition > > r=sdwilsh. This needs sr for API change. I am a tad inundated at the moment with bug 553169, etc. so if this can't wait for around a week please find someone else to sr.
Attachment #433484 -
Flags: superreview?(robert.bugzilla) → superreview+
Comment on attachment 433493 [details] [diff] [review] Part 10: setAndLoadFaviconForPage implementation looks fine, reminder to rev the iid on the interface
Attachment #433493 -
Flags: superreview?(robert.bugzilla) → superreview+
Assignee | ||
Comment 27•15 years ago
|
||
Attachment #433485 -
Attachment is obsolete: true
Assignee | ||
Comment 28•15 years ago
|
||
Attachment #433486 -
Attachment is obsolete: true
Assignee | ||
Comment 29•15 years ago
|
||
Attachment #433487 -
Attachment is obsolete: true
Assignee | ||
Comment 30•15 years ago
|
||
Attachment #433488 -
Attachment is obsolete: true
Assignee | ||
Comment 31•15 years ago
|
||
Attachment #433489 -
Attachment is obsolete: true
Assignee | ||
Comment 32•15 years ago
|
||
Attachment #433490 -
Attachment is obsolete: true
Assignee | ||
Comment 33•15 years ago
|
||
Attachment #433491 -
Attachment is obsolete: true
Assignee | ||
Comment 34•15 years ago
|
||
Attachment #433492 -
Attachment is obsolete: true
Assignee | ||
Comment 35•15 years ago
|
||
Attachment #433493 -
Attachment is obsolete: true
Assignee | ||
Comment 36•15 years ago
|
||
Attachment #434588 -
Flags: review?(sdwilsh)
Comment 37•15 years ago
|
||
Comment on attachment 434588 [details] [diff] [review] Part 11: Test for cases where we should not set an icon r=sdwilsh
Attachment #434588 -
Flags: review?(sdwilsh) → review+
Assignee | ||
Comment 38•15 years ago
|
||
hm, through tryserver i've found a couple tests that need updating. looking into them.
Assignee | ||
Updated•15 years ago
|
Whiteboard: [fixing remaining tests, almost ready to land]
Assignee | ||
Comment 39•15 years ago
|
||
just added a check that moz_favicons has only 1 entry.
Attachment #434588 -
Attachment is obsolete: true
Assignee | ||
Comment 40•15 years ago
|
||
Actually, there was a glitch in part 7, due to a wrong storage bind index to mDBIsertIcon, minor problem though, now fixed. I should be ready to land.
Attachment #434584 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Whiteboard: [fixing remaining tests, almost ready to land] → [ready to land once m-c allows]
Assignee | ||
Comment 41•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/d0e517138232 http://hg.mozilla.org/mozilla-central/rev/c42cb3020dff http://hg.mozilla.org/mozilla-central/rev/7f3c40179eb8 http://hg.mozilla.org/mozilla-central/rev/580f64b36260 http://hg.mozilla.org/mozilla-central/rev/3eb4cc8091e4 http://hg.mozilla.org/mozilla-central/rev/0cbeb547de12 http://hg.mozilla.org/mozilla-central/rev/17a763661401 http://hg.mozilla.org/mozilla-central/rev/d1933ef63769 http://hg.mozilla.org/mozilla-central/rev/ec9bb3fb1f72 http://hg.mozilla.org/mozilla-central/rev/0703b3ba02a4 http://hg.mozilla.org/mozilla-central/rev/7119a973030d
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Whiteboard: [ready to land once m-c allows]
Target Milestone: --- → mozilla1.9.3a4
Updated•14 years ago
|
Updated•12 years ago
|
Keywords: dev-doc-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•