Closed Bug 702761 Opened 9 years ago Closed 9 years ago

AsyncGetFaviconURLForPage warns like crazy

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: mak, Assigned: mak)

Details

Attachments

(1 file)

if AsyncGetFaviconURLForPage can't find an icon it warns, it should not.
Attached patch patch v1.0Splinter Review
Attachment #574679 - Flags: review?(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?
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.
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 on attachment 574679 [details] [diff] [review]
patch v1.0

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

thanks for explaining! r=me
Attachment #574679 - Flags: review?(dietrich) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/33588f503963
Flags: in-testsuite-
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/33588f503963
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.