702761 - AsyncGetFaviconURLForPage warns like crazy

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

if AsyncGetFaviconURLForPage can't find an icon it warns, it should not.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.0

Comment 2

[Dietrich Ayala (:dietrich)]

Comment on attachment 574679 [details] [diff] [review]
patch v1.0

Review of attachment 574679 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/AsyncFaviconHelpers.cpp
@@ +873,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  // No icon was found.
> +  if (iconSpec.IsEmpty())
> +    return NS_OK;

what kind of behaviour change will occur as a result of this? notifications where before there were none?

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

We fire the notification event that immediately bails out on an NS_ENSURE_SUCCESS. That's the exepcted behavior (notify only on success, don't notify on failure) according to the current idl, so this doesn't change the behavior, just bails out earlier.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

to clarify without the patch we dispatch an event that tries to create a nsIURI out of an empty spec, and that warns and returns. with the patch we avoid creating the event and just bail out.

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 574679 [details] [diff] [review]
patch v1.0

Review of attachment 574679 [details] [diff] [review]:
-----------------------------------------------------------------

thanks for explaining! r=me

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/33588f503963

Comment 7

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/33588f503963