553489 - make setAndLoadFaviconForPage completely async

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

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.

Comment 1

[Marco Bonardo [:mak]]

Attachment: Part 1: IDL changes

Comment 2

[Marco Bonardo [:mak]]

Attachment: Part 2: AsyncFaviconStepper implementation

Comment 3

[Marco Bonardo [:mak]]

Attachment: Part 3: getEffectivePageStep

Comment 4

[Marco Bonardo [:mak]]

Attachment: Part 4: fetchDatabaseIconStep

Comment 5

[Marco Bonardo [:mak]]

Attachment: Part 5: EnsureEntryStep

Comment 6

[Marco Bonardo [:mak]]

Attachment: Part 6: FetchNetworkIconStep

Comment 7

[Marco Bonardo [:mak]]

Attachment: Part 7: SetFaviconDataStep

Comment 8

[Marco Bonardo [:mak]]

Attachment: Part 8: AssociateIconWithPageStep

Comment 9

[Marco Bonardo [:mak]]

Attachment: Part 9: NotifyStep

Comment 10

[Marco Bonardo [:mak]]

Attachment: Part 10: setAndLoadFaviconForPage implementation

Comment 11

[Marco Bonardo [:mak]]

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.

Comment 12

[Marco Bonardo [:mak]]

- note: i also need a success test for the callback.

Comment 13

[Shawn Wilsher :sdwilsh]

Comment on attachment 433484 [details] [diff] [review]
Part 1: IDL changes

r=sdwilsh

Comment 14

[Shawn Wilsher :sdwilsh]

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

Comment 15

[Shawn Wilsher :sdwilsh]

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

Comment 16

[Shawn Wilsher :sdwilsh]

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

Comment 17

[Shawn Wilsher :sdwilsh]

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

Comment 18

[Shawn Wilsher :sdwilsh]

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

Comment 19

[Shawn Wilsher :sdwilsh]

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

Comment 20

[Shawn Wilsher :sdwilsh]

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

Comment 21

[Shawn Wilsher :sdwilsh]

Comment on attachment 433492 [details] [diff] [review]
Part 9: NotifyStep

>+void
>+NotifyStep::Run()
>+{
Yay!  this one was right!

r=sdwilsh

Comment 22

[Shawn Wilsher :sdwilsh]

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

[Dietrich Ayala (:dietrich)]

Comment on attachment 433485 [details] [diff] [review]
Part 2: AsyncFaviconStepper implementation

looks good, r=me.

Comment 24

[Marco Bonardo [:mak]]

note to self, latest patch should rev uuid.

Comment 25

[Robert Strong (they/them - no direct email)]

(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.

Comment 26

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 433493 [details] [diff] [review]
Part 10: setAndLoadFaviconForPage implementation

looks fine, reminder to rev the iid on the interface

Comment 27

[Marco Bonardo [:mak]]

Attachment: Part 2: AsyncFaviconStepper implementation

Comment 28

[Marco Bonardo [:mak]]

Attachment: Part 3: getEffectivePageStep

Comment 29

[Marco Bonardo [:mak]]

Attachment: Part 4: fetchDatabaseIconStep

Comment 30

[Marco Bonardo [:mak]]

Attachment: Part 5: EnsureEntryStep

Comment 31

[Marco Bonardo [:mak]]

Attachment: Part 6: FetchNetworkIconStep

Comment 32

[Marco Bonardo [:mak]]

Attachment: Part 7: SetFaviconDataStep

Comment 33

[Marco Bonardo [:mak]]

Attachment: Part 8: AssociateIconWithPageStep

Comment 34

[Marco Bonardo [:mak]]

Attachment: Part 9: NotifyStep

Comment 35

[Marco Bonardo [:mak]]

Attachment: Part 10: setAndLoadFaviconForPage implementation

Comment 36

[Marco Bonardo [:mak]]

Attachment: Part 11: Test for cases where we should not set an icon

Comment 37

[Shawn Wilsher :sdwilsh]

Comment on attachment 434588 [details] [diff] [review]
Part 11: Test for cases where we should not set an icon

r=sdwilsh

Comment 38

[Marco Bonardo [:mak]]

hm, through tryserver i've found a couple tests that need updating. looking into them.

Comment 39

[Marco Bonardo [:mak]]

Attachment: Part 11: Test for cases where we should not set an icon

just added a check that moz_favicons has only 1 entry.

Comment 40

[Marco Bonardo [:mak]]

Attachment: Part 7: SetFaviconDataStep

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.

Comment 41

[Marco Bonardo [:mak]]

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