STR: * http://www.20thingsilearned.com * Start reading, then navigate forward to the second page of the forward. * Right-click on the back button. Expected results: * Favicon appears on the back button. Actual results: * No favicon. There's also no favicon in the History menu, presumably for the same reason. Note that the page's <title> is also missing from the drop-down list and the History menu. That it's missing from the History menu is fixed in bug 646422. I'll file a separate bug that it's missing from the drop-down list.

How do we add these things to history in the first place?

The code is all here: http://hg.mozilla.org/mozilla-central/file/8d0ca70728ff/docshell/base/nsDocShell.cpp#l9478 I think we just need to set the favicon for that URI after we add it to the global history (per the patch in bug 646422).

Ah, but it looks like neither getFaviconImageForPage nor getFaviconLinkForIcon gives me a result which I'd be able to pass back to setFaviconUrlForPage. Also, this API is asynchronous, right? So if they called pushState early enough, we might not have figured out what the favicon URI is yet? Maybe the right thing to do is look up the favicon based on the page's original URI, which doesn't change with pushstate.

Oh, I read the IDL wrong; getFaviconForPage is what we'd want. But using the document's original URI still seems like the right thing to do.

... although using the original URI will break nsPlacesImportExportService::WriteItem -- if you have bookmarked a page which was pushState'ed to, you won't be able to write its favicon when you export the bookmark. I'm not sure I care so much about the import/export service, but I do care about awesomebar results for Facebook having the right favicon, and I think that has the same problem. I guess I'll go back to looking at the approach in comment 2.

(In reply to comment #2) > I think we just need to set the favicon for that URI after we add it to the > global history (per the patch in bug 646422). Correct. This is going to be fun since we don't expose an API for docshell to use for that...

This seems to work, but maybe I'm missing something? I'm not entirely sure how to write a test, since things appear to be happening asynchronously without observers I can hook my test code into.

(In reply to comment #7) > I'm not entirely sure how to write a test, since things appear to be > happening asynchronously without observers I can hook my test code into. Marco can give you tips on how to test the favicon code.

Comment on attachment 531081 [details] [diff] [review] Patch v1 I'm not sure if we can use nsIFaviconService inside of docshell since it's in toolkit/components/places/. Maybe that's changed in the new libxul world though... Regardless, mak is a better reviewer here.

> I'm not sure if we can use nsIFaviconService inside of docshell since it's in > toolkit/components/places/. Maybe that's changed in the new libxul world > though... fwiw, we use content/base/src/nsContentUtils.cpp in docshell now too. O brave new world! That has such linkage in it!

There was some discussion in the related (cousin?) bug 420605; There is it seemed to me that the code had to go into browser or tabbrowser. I'm not sure that the docshell always has a faviconservice, or even Places, available to it. If this patch goes ahead it would be nice to apply something similar to bug 420605.

(In reply to comment #11) > If this patch goes ahead it would be nice to apply something similar to bug > 420605. I tried to read that bug, the only thing I got is that we could change the way fragments are stored, so I don't completely understand what you mean here, could be my foreign language is not helping me :) Regarding the patch, if I see it correctly it applies on top of the patch in bug 646422. There is an interesting difference from visits and titles, getFaviconForPage and setFaviconURLForPage are synchronous, thus we can't be sure the icon for oldURI has already been stored. Actually this is even more cumbersome since the icon is stored by browser and not by the docshell, so the ordering can differ :( A better solution could be changing getFaviconForPage to async with a callback, should not be too hard to do using the existing code in AsyncFaviconHelpers.cpp. This would still base on the fact browser sets the icon on oldURI before this code runs, but at least the query would be serialized with visits. Btw, in both cases I'd prefer if you use SetAndLoadFaviconForPage that is already async. My plan is to make all the favicon service methods async, but the resources are limited. Regarding what you can listen in a test, you need a nsNavHistoryObserver, and to use onPageChanged with aProperty == "favicon"

which transition type do these visits get in moz_historyvisits?

(In reply to comment #12) > (In reply to comment #11) > > If this patch goes ahead it would be nice to apply something similar to bug > > 420605. > > I tried to read that bug, the only thing I got is that we could change the > way fragments are stored, so I don't completely understand what you mean > here, could be my foreign language is not helping me :) I was saying that it would then be nice if the docshell could also set the favicon on a _hashchange_ - which would fix bug 420605 in a similar way to the patch here.

(In reply to comment #11) > There was some discussion in the related (cousin?) bug 420605; There is it > seemed to me that the code had to go into browser or tabbrowser. I'm not > sure that the docshell always has a faviconservice, or even Places, > available to it. In the libxul-required world, my understanding is that module boundaries no longer exist. > A better solution could be changing getFaviconForPage to async with a > callback, should not be too hard to do using the existing code in > AsyncFaviconHelpers.cpp. This would still base on the fact browser sets the > icon on oldURI before this code runs, but at least the query would be > serialized with visits. This would seem to introduce a new class of race conditions. For example: * Load page at URI X. Begin loading favicon x. * Page calls pushState. New URI is Y. Register callback so that when X's favicon finishes loading, set Y's favicon to x. * Page changes its favicon to y. Register callback so that when Y's favicon finishes loading, set Y's favicon to y. We now have racing callbacks. If y finishes loading before x, then Y's favicon will incorrectly become x. This kind of thing can become arbitrarily complex, because you need to chain these (X pushStates to Y which pushStates to Z -- Z gets a favicon when Y gets one, which happens when X gets one). So solutions like canceling the stale callback may become complicated...

(In reply to comment #13) > which transition type do these visits get in moz_historyvisits? You mean the flags passed in to history->VisitURI()? I think those are 0 for pushState, unless the pushState happens in a frame. See nsDocShell::AddURIVisit, which is called by nsDocShell::AddState.

(In reply to comment #15) > We now have racing callbacks. If y finishes loading before x, then Y's favicon > will incorrectly become x. Do we actually handle a page changing it's favicon without reloading it? I don't think we do.

(In reply to comment #16) > You mean the flags passed in to history->VisitURI()? I think those are 0 for > pushState, unless the pushState happens in a frame. See > nsDocShell::AddURIVisit, which is called by nsDocShell::AddState. It's not passing in TOP_LEVEL then?

(In reply to comment #17) > Do we actually handle a page changing it's favicon without reloading it? I > don't think we do. Is the last "it" "the page" or "the favicon"? Surely a page can change its favicon without reloading the page? Otherwise I'm not sure what you mean about reloading the favicon.

(In reply to comment #15) > * Load page at URI X. Begin loading favicon x. > * Page calls pushState. New URI is Y. Register callback so that when X's > favicon finishes loading, set Y's favicon to x. You don't register a callback to be called when X favicon finishes, you do what you are doing right now, but async rather than sync (no ui locking). The callback informs you if X has a favicon and you then set it to Y. X favicon is setup by browser, my suggestion was mostly to avoid to do work synchronously, because it's not serialized with other requests and causes ui hangs. It's not a solution to the "we have to be sure browser finished loading the icon" problem. But can make that a little better because the X favicon fetching is serialized to any other async request that already started.

(In reply to comment #18) > It's not passing in TOP_LEVEL then? Sorry, I misread the code. It's: if (!IsFrame()) { visitURIFlags |= IHistory::TOP_LEVEL; } So yes, it's passing in TOP_LEVEL, and it's 0 if it *is* in a frame.

(In reply to comment #19) > Is the last "it" "the page" or "the favicon"? Surely a page can change its > favicon without reloading the page? Otherwise I'm not sure what you mean about > reloading the favicon. it is the page. Rephrasing what I said: "Do we actually handle a page changing its favicon without reloading the page? I don't think we listen for anything like that currently, and only check on page load."

Fwiw, framed visits can't have a favicon nor a title, unless they are explicitely marked as followed links (http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsIBrowserHistory.idl#158) they are not even added to the database. Doesn't seem this case though since they appear in the history menu.

(In reply to comment #22) > it is the page. Rephrasing what I said: "Do we actually handle a page > changing its favicon without reloading the page? I don't think we listen > for anything like that currently, and only check on page load." As discussed on IRC, we do. Gmail uses this for their unread-count-in-the-favicon feature. mfinkle: they just update the <link> to the favicon mfinkle: we get a DOMLinkAdded event and update the favicon in firefox

(In reply to comment #20) > my suggestion was mostly to avoid to do work > synchronously, because it's not serialized with other requests and causes ui > hangs. It's not a solution to the "we have to be sure browser finished > loading the icon" problem. But can make that a little better because the X > favicon fetching is serialized to any other async request that already > started. Cool, I understand. I'm happy to punt on the races, especially since I agree that the async solution is less racy than the sync solution. I'll see if I can make the functions async.

Cancelling review until I can have a look at making get/set favicon async.

The idea is to add a in nsIFaviconDataCallback aCallback param to getFaviconForPage, drop the return value, and use the code in AsyncFaviconHelpers.cpp to handle the method. The callback interface has been designed to be able to be used in most of favicon service methods. At than point in the callback could just use SetAndLoadFaviconForPage, we could in future update SetFaviconUrlForPage to be async as well, but that's not a show-stopper (SetAndLoad could just do some more checks). The issue could be that getFaviconForPage is used in a sync way in some other code (at first glance browser code seems easily fixable, importexport code looks hairy), if that code is not easily fixable we could have to add a new asyncGetFaviconForPage method, or make the callback optional and go the async road only if a callback is provided, otherwise go the sync road (I'd rather avoid this mess though, if possible).

Adding a testcase, since 20thingsilearned.com has other problems (e.g. bug 655328).

This will need review from a docshell peer, but I'd first like to get mak's signoff. I didn't end up putting any code in AsyncFaviconHelpers.cpp because this code looked higher level than most of the code in that file and because I thought the control flow would be easier to follow if it were all together. But I'm not wed to this decision.

Comment on attachment 531691 [details] [diff] [review] Patch v1 Review of attachment 531691 [details] [diff] [review]: ----------------------------------------------------------------- 2 important things I don't like: 1. Why adding a new callback interface when we already have one (nsIFaviconDataCallback)? I'd start by reusing the existing interface. 2. You cannot use GetFaviconSpecForPage (not sure why you need another method, just to avoid spec->nsIURI->spec?) out of mainthread, the statement can't cross threads that easily, indeed your new implementation should assert that is on mainthread, and would fail that assertion in the runnable. That's why I was suggesting to reuse where possible the existing infrastructure in AsyncFaviconHelpers, I understand it could be complicated, I could help you figuring it out if you could fix all the surroundings, I just have to refresh my mind on that piece of code. Like we have AsyncFetchAndSetIconForPage, you should aget a GetIconForPage (guessed name) runnable that you can statically ::start() with some initial data, then after fetching the icon (FetchIconInfo can be useful there) you should be able to reuse the NotifyIconObserver runnable.

> 1. Why adding a new callback interface when we already have one > (nsIFaviconDataCallback)? I'd start by reusing the existing interface. I created a new interface because nsIFaviconDataCallback seems like the wrong one to be using here. nsIFaviconDataCallback is used to inform someone that data is available for a favicon: void onFaviconDataAvailable(in nsIURI aURI, in unsigned long aDataLen, [const,array,size_is(aDataLen)] in octet aData, in AUTF8String aMimeType); nsIFaviconURLCallback serves an entirely different purpose; there, we're informing the caller that we've completed a page URI --> favicon URI lookup: void onFaviconURLAvailable(in nsIURI aPageURI, in nsIURI aFaviconURI); Although I understand that we could use onFaviconDataAvailable here, since all we really need is a single nsIURI parameter, that seems like a hack to me. Why is reusing this interface better than declaring a new one? > 2. You cannot use GetFaviconSpecForPage (not sure why you need another method, > just to avoid spec->nsIURI->spec?) You can't call NS_NewURI off the main thread, so the function needed to return a spec rather than an nsIURI. Similarly, it needed to take a spec as an argument rather than an nsIURI because nsIURI's addref/release isn't threadsafe, and thus the calling class couldn't store an nsIURI. > 2. You cannot use GetFaviconSpecForPage out of mainthread, the statement > can't cross threads that easily, indeed your new implementation should > assert that is on mainthread, and would fail that assertion in the runnable. Are you saying it should currently be failing an assertion, or that I should add an assertion? It's not asserting in my builds.

(In reply to comment #31) > Although I understand that we could use onFaviconDataAvailable here, since > all we really need is a single nsIURI parameter, that seems like a hack to > me. Why is reusing this interface better than declaring a new one? The original scope for that callback was to be made reusable by all the other methods once converted to async. Actually if you completed a lookup and there is an icon informing you have an icon sounds fine to me. If we start adding a new callback interface for each method, we end up with a lot of complication, that could have been avoided. > > 2. You cannot use GetFaviconSpecForPage (not sure why you need another method, > > just to avoid spec->nsIURI->spec?) > > You can't call NS_NewURI off the main thread, so the function needed to > return a spec rather than an nsIURI. Similarly, it needed to take a spec as > an argument rather than an nsIURI because nsIURI's addref/release isn't > threadsafe, and thus the calling class couldn't store an nsIURI. yes but you invoke an API method that is on mainthread and your callback is called back on mainthread. in the middle we store and use a spec. We do the same in history and in setAndLoadFaviconForURI > > 2. You cannot use GetFaviconSpecForPage out of mainthread, the statement > > can't cross threads that easily, indeed your new implementation should > > assert that is on mainthread, and would fail that assertion in the runnable. > > Are you saying it should currently be failing an assertion, or that I should > add an assertion? It's not asserting in my builds. I'm saying you should add an assertion, and if you do so it would be hit.

Personally, I think the One Callback To Rule Them All approach ultimately leads to: nsIVariant onCallback(nsCOMArray<nsIVariant> &args) (*) which is a dark place. As far as complexity, reusing the existing callback mechanism adds complexity because either: * I fill in only aURI and leave the rest of the arguments empty, complicating the callback's calling convention (and making it act more like (*) above), or * I fill in all the arguments when I only wanted to fill in aURI, requiring extra code and doing extra work for nothing. > If we start adding a new callback interface for each method, we end up with a > lot of complication, that could have been avoided. If each method actually returns something different and thus uses onDataAvailable in a different way, that complexity is already present, just hidden. I don't hack on this module often, so I'm happy to go with whatever interface you think is right. But as a consumer of the interface in docshell, I think registering an onDataAvailable listener when I just care about the URI is undesirable.

> yes but you invoke an API method that is on mainthread and your callback is > called back on mainthread. in the middle we store and use a spec. We do the > same in history and in setAndLoadFaviconForURI Sorry, I don't understand. I thought the question was why can't GetFaviconForPageAsyncRunnable call nsFaviconService::GetFaviconForPage? And the answer is, the runnable runs off the main thread, while GetFaviconForPage takes a URI as an argument and calls NS_NewURI, and thus is mainthread only. Maybe it doesn't matter; it sounds like I'm going to need to rework the patch anyway.

(In reply to comment #33) > * I fill in only aURI and leave the rest of the arguments empty, > complicating the callback's calling convention (and making it act more like > (*) above), or The interface definition clearly says that the invoker has to check arguments, especially data. In a @note above getFaviconForUrlAsync you can easily specify that the callback won't get any data, only the faviconURI. I absolutely understand your point about pure interfaces, but I feel like we should be more flexible, having a single callback definition allows you to have a single signature, and managing null arguments is easy (the invoker just have to check datalen btw). (In reply to comment #34) > Sorry, I don't understand. I thought the question was why can't > GetFaviconForPageAsyncRunnable call nsFaviconService::GetFaviconForPage? because any nsIFaviconService method has to run on mainthread, your runnable runs in another thread. > And the answer is, the runnable runs off the main thread, while > GetFaviconForPage takes a URI as an argument and calls NS_NewURI, and thus > is mainthread only. Sorry, it's possible my foreign language doesn't help. The path is: 1. invoke nsIFaviconService method on mainthread passing a callback 2. on mainthread convert nsIURI->spec. Actually you could just use a IconData struct. 3. spawn a runnable in another thread passing in the struct 4. the runnable on completion spawns a runnable on mainthread passing spec 5. on mainthread convert spec->nsIURI 6. on mainthread callback is notified so nsIURI always lives only on mainthread.

> The path is: > 1. invoke nsIFaviconService method on mainthread passing a callback > 2. on mainthread convert nsIURI->spec. Actually you could just use a IconData > struct. > 3. spawn a runnable in another thread passing in the struct > 4. the runnable on completion spawns a runnable on mainthread passing spec > 5. on mainthread convert spec->nsIURI > 6. on mainthread callback is notified This is exactly what my patch did, right?

(In reply to comment #36) > This is exactly what my patch did, right? Yes, similarly, apart invoking a mainthread-only method in another thread.

> because any nsIFaviconService method has to run on mainthread That function was mainthread-only only because as a rule, all functions in nsFaviconService, are mainthread-only? In terms of the code contained in the function, it was threadsafe?

It's an API, so it must be callable from main-thread, but since internally it uses a storage statement (mDBGetURL) that can't be used across different threads, it can't be used in anything other than main-thread. For queries that must be used in different threads we create a separate statement for each thread (this is handled in AsyncFaviconHelpers through a statementCache IIRC)

I see; thanks for clarifying! Can we add a debug assertion to the storage statement that it's used on only one thread?

What do you think of this patch, Marco? I ended up copy-pasting the relevant SQL statement, which seems like the Wrong Thing to do. How do you handle this sort of thing elsewhere?

copy paste is fine BUT you should do like others methods there do: nsCOMPtr<mozIStorageStatement> stmt = mFaviconSvc->mSyncStatements.GetCachedStatement(NS_LITERAL_CSTRING( "SELECT omg FROM change" )); this way you use a cached statement and favicon service takes care of destroying it on shutdown.

(In reply to comment #42) > copy paste is fine BUT you should do like others methods there do Okay. The patch follows that idiom afaict.

(In reply to comment #43) > (In reply to comment #42) > > copy paste is fine BUT you should do like others methods there do > > Okay. The patch follows that idiom afaict. I was referring only to the statement usage, I didn't look at the rest yet, I'll look at the patch along the night, now I'm going to take a couple free hours for my birthday :)

I think we're misunderstanding each other. I think that comment 42 does not apply to the patch I posted, because the patch I posted already does what comment 42 says to do. Does patch v2 do something wrong? If so, I don't understand what. In any case, I can wait until the night. Happy birthday!

(In reply to comment #40) > Can we add a debug assertion to the storage statement that it's used on only > one thread? We probably should. Can you file that bug please (Toolkit::Storage)?

In nearly every case the required URL is available as the mIconURL property on the browser - which is why the tab and location bar show the correct icon; Is it not possible to save a database query by looking there first?

Comment on attachment 531939 [details] [diff] [review] Patch v2 Review of attachment 531939 [details] [diff] [review]: ----------------------------------------------------------------- So, this is very good! Thanks for the patience to make this properly done, before r+ I'd like to discuss a small detail regarding the idl, for now I put in a f+ while we figure out the following stuff. Recently we started thinking to make completely new interfaces for async APIs in Places (where possible), the reasons are a couple: - having to prefix each new API with Async (like AsyncGetFaviconUrlForPage in this case) sucks - one day we'd like to directly deprecate all of the old interface In this case we could start defining mozIAsyncFavicons with 2 methods: GetFaviconForPage and SetAndFetchFaviconForPage The new interface would be implemented by nsFaviconService and we should just forward calls to the existing methods (where present) My doubt regards whether we should use nsIURI or just pass in the spec, as requested in various bugs (like bug 399490 and bug 636393) due to the fact any nsIURI in js has to be GCed and doing that in cpp is cheaper. On the other side using nsIURI is better for various reasons: url is parsed and verified, the security discussion in bug 399490, and there is a bug from brettw, that now I can't find, about the fact we should store in the db the nsIURI.originCharset, otherwise we are storing a wrongly coded urlstring. My impression is that for now we should stick with nsIURI but mark the interface as experimental and likely to change while we finalize it (like http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/mozIAsyncHistory.idl#141) Sdwilsh, Justin, thoughts? ::: docshell/base/nsDocShell.cpp @@ +9421,5 @@ > + NS_DECL_ISUPPORTS > + > + nsAddStateFaviconCallback(nsIURI *aNewURI) > + { > + mNewURI = NS_TryToMakeImmutable(aNewURI); what is this preventing? @@ +9445,5 @@ > + > + // nsAddStateFaviconCallback gets passed between the main thread and the > + // Places DB thread. > + NS_IMPL_THREADSAFE_ISUPPORTS1(nsAddStateFaviconCallback, > + nsIFaviconDataCallback) I don't think this is really needed, we never invoke nor addref/release the callback out of mainthread (we always swap the ownership to avoid this kind of things), so you can just define it as usual and not threadsafe. @@ +9737,5 @@ > + > + // Inform the favicon service that our old favicon applies to this new > + // URI. > + nsCOMPtr<nsIFaviconService> favSvc = > + do_GetService("@mozilla.org/browser/favicon-service;1", &rv); no need to assign rv here @@ +9741,5 @@ > + do_GetService("@mozilla.org/browser/favicon-service;1", &rv); > + if (favSvc) { > + nsCOMPtr<nsIFaviconDataCallback> callback = > + new nsAddStateFaviconCallback(newURI); > + rv = favSvc->GetFaviconForPageAsync(oldURI, callback); you're assigning rv but not using it, cast to (void) instead ::: docshell/test/browser/browser_bug655270.js @@ +32,5 @@ > + * and other provisions required by the GPL or the LGPL. If you do not delete > + * the provisions above, a recipient may use your version of this file under > + * the terms of any one of the MPL, the GPL or the LGPL. > + * > + * ***** END LICENSE BLOCK ***** */ you can use the short pd license in tests: http://www.mozilla.org/MPL/boilerplate-1.1/pd-c ::: toolkit/components/places/AsyncFaviconHelpers.cpp @@ +264,5 @@ > + "FROM moz_places h " > + "JOIN moz_favicons f ON h.favicon_id = f.id " > + "WHERE h.url = :page_url " > + "LIMIT 1" > + )); So, you only care about url, thus this should only select url, let's do: SELECT f.url FROM moz_places h JOIN moz_favicons f ON h.favicon_id = f.id WHERE h.url = :page_url (the LIMIT sounds useless, there is only one page with that url, and only one icon with that id) @@ +278,5 @@ > + // No favicon found. > + return NS_OK; > + } > + > + return stmt->GetUTF8String(1, aIconSpec); I'd rather invert here and do if (NS_SUCCEEDED && hasResult) { // Found a favicon for the page. rv = GetUTF8String NS_ENSURE_SUCCESS } return NS_OK; When you change the query, remember to change the GetUTF8String index from 1 to 0 @@ +865,5 @@ > + NS_ENSURE_ARG(aCallback); > + NS_PRECONDITION(NS_IsMainThread(), > + "This should be called on the main thread."); > + > + nsCString spec; nsCAutoString @@ +878,5 @@ > + new AsyncGetFaviconURLForPage(spec, aDBConn, fs, callback); > + > + nsCOMPtr<nsIEventTarget> target = do_GetInterface(aDBConn); > + NS_ENSURE_TRUE(target, NS_ERROR_OUT_OF_MEMORY); > + rv = target->Dispatch(event, NS_DISPATCH_NORMAL); you're not checking rv @@ +911,5 @@ > +{ > + NS_PRECONDITION(!NS_IsMainThread(), > + "This should not be called on the main thread."); > + > + nsCString iconSpec; nsCAutoString ::: toolkit/components/places/nsFaviconService.cpp @@ +720,5 @@ > +{ > + NS_ENSURE_ARG_POINTER(aPageURI); > + NS_ENSURE_ARG_POINTER(aCallback); > + > + return AsyncGetFaviconURLForPage::start(aPageURI, mDBConn, aCallback); assign rv, NS_ENSURE_SUCCESS and return NS_OK this usually produces better stack traces.

> In this case we could start defining mozIAsyncFavicons with 2 methods: > GetFaviconForPage and SetAndFetchFaviconForPage > The new interface would be implemented by nsFaviconService and we should just > forward calls to the existing methods (where present) This sounds like scope creep to me, to be honest. I'm happy to make a new interface if that's all I need to do, but what about all the existing calls to the synchronous GetFaviconForPage? > My doubt regards whether we should use nsIURI or just pass in the spec, as requested in > various bugs (like bug 399490 and bug 636393) due to the fact any nsIURI in js has to be GCed > and doing that in cpp is cheaper. I didn't see anything in those bugs to convince me that perf on the favicon service actually matters. This seems pretty unlikely to me, since we call it just once per load. > + mNewURI = NS_TryToMakeImmutable(aNewURI); > what is this preventing? I don't control that URI, so making it immutable seemed prudent. But then again, I don't know when one should or shouldn't make a URI immutable. > So, you only care about url, thus this should only select url, let's do: D'oh. I copy-pasted but didn't look at what I was copy-pasting!! > (the LIMIT sounds useless, there is only one page with that url, and only one > icon with that id) Is there a UNIQUE constraint on the URL? (Does SQLite even have that?) Do we want to get rid of the LIMIT 1 in other places too? Do you want to do that as a followup instead? > assign rv, NS_ENSURE_SUCCESS and return NS_OK > this usually produces better stack traces. Is this a Places-specific style? My understanding was that this is frowned upon; for example, see bug 560112 comment 9.

(In reply to comment #49) > > In this case we could start defining mozIAsyncFavicons with 2 methods: > > GetFaviconForPage and SetAndFetchFaviconForPage > > The new interface would be implemented by nsFaviconService and we should just > forward calls to the existing methods (where present) > > This sounds like scope creep to me, to be honest. I'm happy to make a new > interface if that's all I need to do, but what about all the existing calls > to the synchronous GetFaviconForPage? It's not scope creep, since we are adding a new method to an API. It's better to introduce a new interface today than to break all callers "tomorrow". Existing calls will keep working since they are served by the old interface. We are just adding a new one for new callers. > > + mNewURI = NS_TryToMakeImmutable(aNewURI); > > what is this preventing? > > I don't control that URI, so making it immutable seemed prudent. But then > again, I don't know when one should or shouldn't make a URI immutable. I feel like it's useless work, we control all of that code. > Is there a UNIQUE constraint on the URL? (Does SQLite even have that?) Do > we want to get rid of the LIMIT 1 in other places too? Do you want to do > that as a followup instead? yes, url has a unique constraint. we fix queries along while we touch related code, and LIMIT 1 is a limited problem (it adds a couple opcodes, nothing really critical but better not introducing new ones) > > assign rv, NS_ENSURE_SUCCESS and return NS_OK > > this usually produces better stack traces. > > Is this a Places-specific style? My understanding was that this is frowned > upon; for example, see bug 560112 comment 9. That comment says to never "return rv", that is exactly what I'm suggesting. We should always (yes in Places we follow this style): rv = XYZ NS_ENSURE_SUCCESS(rv, rv); return NS_OK;

Btw, wait on Sdwilsh feedback on the interface before fixing any idl related stuff, to avoid doing work twice.

> That comment says to never "return rv", that is exactly what I'm suggesting. The code he's commenting on does: > >+ rv = mElement->GetAttribute(normAttr, _retval); > >+ NS_ENSURE_SUCCESS(rv, rv); > >+ return NS_OK; which is exactly what you're suggesting. The comment says "avoid return rv" because that's what *another* commenter had suggested. But I think both commenters agree that the idiom above is frowned upon. Anyway I'm happy to follow Places style. > It's not scope creep, since we are adding a new method to an API. It's better > to introduce a new interface today than to break all callers "tomorrow". > Existing calls will keep working since they are served by the old interface. We > are just adding a new one for new callers. To be clear, I understood the suggestion to be to add a new interface with an asynchronous GetFaviconForPage function, but to leave the existing synchronous GetFaviconForPage function under a different interface. You don't think it'll be confusing to have two methods with the same name declared on one class which do two different things? Especially when you access the class from JS and it's not clear what interface the class implements?

(In reply to comment #52) > > That comment says to never "return rv", that is exactly what I'm suggesting. > > The code he's commenting on does: Maybe I'm reading it wrong but the original code was doing what I'm suggesting, someone commented saying "no, directly return" and he replied to this comment "no, never return rv". So to me looks like the original code was fine, the first comment was wrong. I'm not willing to discuss my english understandment though :p > You don't think it'll be confusing to have two methods with the same name > declared on one class which do two different things? Especially when you > access the class from JS and it's not clear what interface the class > implements? One is GetFaviconUrlForPage, the other one is GetFaviconForPage, so no. In js you also have always to specify a interface to get the service.

So, yes, I'm suggesting to slightly change the methods names, to avoid having same method name in 2 interfaces.

sdwilsh: please see the first part of comment 48 (the overall review comment)

(In reply to comment #48) > In this case we could start defining mozIAsyncFavicons with 2 methods: > GetFaviconForPage and SetAndFetchFaviconForPage > The new interface would be implemented by nsFaviconService and we should just > forward calls to the existing methods (where present) This sounds like the right approach here probably. > My doubt regards whether we should use nsIURI or just pass in the spec, as > requested in various bugs (like bug 399490 and bug 636393) due to the fact any > nsIURI in js has to be GCed and doing that in cpp is cheaper. > On the other side using nsIURI is better for various reasons: url is parsed and > verified, the security discussion in bug 399490, and there is a bug from > brettw, that now I can't find, about the fact we should store in the db the > nsIURI.originCharset, otherwise we are storing a wrongly coded urlstring. I'm inclined to use a spec here, but not strongly.

nsIFaviconDataCallback returns an nsIURI. Do you want to make that a spec too?

This moves to the async interface. I need to push to try to make sure this didn't mess things up.

Try results will appear at: http://tbpl.mozilla.org/?tree=Try&rev=9f148f9237d1

Well, I accidentally pushed a patch which I know was creating some orange, but I don't see any extra test failures from this patch, so it should be OK. I ran the Places test suite locally just in case, and it was fine. (Mostly, I wanted to make sure that I'd sprinkled enough QI around to get the tests to pick up the new async interface where necessary.) Requesting review on this new patch.

You should not remove any method from the old interface (We DON'T want to break old users, otherwise this new interface introduction is useless, the idea is that the old .idl stays untouched). Please follow what we discussed above, about using different method names instead. The URI vs spec issue still needs a good solution, I feel like a discussion should happen in moz.dev.platform and reach a final answer, it's open from 2007! I'm not sold that the pros overcome the cons. pros: - less js GC for nsIURI objects - easy to understand, it's just a string cons: - spec not normalized (easy to fix building nsIURI in cpp) - spec not verified (easy to fix building nsIURI in cpp) - I pass in a string, it gets normalized internally, then I'm notified for the normalized spec, that is not the string I passed in. To catch the right notification I should make a nsIURI, nullifying pros. - originCharset is lost (we don't use it, but we could in future) - security context is lost (see disc. in bug 399490) - comparison is more expensive (.equals can be less expensive than full string compare) - implementors will start adding their own uri parsers (more error-prone and slower) to get schema, path and so on, when nsIURI has its own standard ones Personally I'd stick to URI for now, mark the interface as experimental (as I linked above) and kick-off the discussion.

Sorry, it was not at all clear to me that you wanted to leave the original IDL unchanged, particularly because I didn't notice the difference between SetAndFetchFaviconForPage (new function) and SetAndLoadFaviconForPage (old function). I'm not familiar with the frozenness level you want to enforce on these APIs. Is the concern that you don't want to break extensions? I'll switch back to URIs. Is there anything else you want me to do?

(In reply to comment #62) > I'm not familiar with the frozenness level you want to enforce on these > APIs. Is the concern that you don't want to break extensions? Yes, SetAndLoadFaviconForPage is a pretty commonly used method in a lot of js extensions (I checked in the addons mxr). The frozeness level is "don't change things unless you can't do otherwise or the gain is pretty high", that is probably the common level around the codebase. We are trying to make new experimental async interfaces and when these will be considered frozen enough we'll deprecate the old sync interface, on these new APIs we expect devs to be ready to changes till we remove the experimental flag.

Fixing test_favicon.js; I forgot to change the test back to the URI API.

Comment on attachment 532550 [details] [diff] [review] Patch v4 Review of attachment 532550 [details] [diff] [review]: ----------------------------------------------------------------- Ehr, this review is on the previous version of the patch (you posted a new one while I was reviewing :p) r- because you are not releasing anymore mChannel. I'd like to get sdwilsh's feedback on the actual idl. ::: toolkit/components/places/AsyncFaviconHelpers.cpp @@ +429,5 @@ > + if (mFaviconSvc) { > + // Cast mFaviconSvc to nsIFaviconService before passing to NS_ProxyRelease > + // so NS_ProxyRelease can unambiguously cast to nsISupports. > + nsIFaviconService *faviconSvc; > + mFaviconSvc.forget(&faviconSvc); we should probably fix the service declaration to solve ambiguity at the source, something like the following may work instead of the classic IMPL_ISUPPORTS2 NS_IMPL_ADDREF(nsFaviconService) NS_IMPL_RELEASE(nsFaviconService) NS_INTERFACE_MAP_BEGIN(nsFaviconService) NS_INTERFACE_MAP_ENTRY(nsIFaviconService) NS_INTERFACE_MAP_ENTRY(mozIAsyncFavicons) NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIFaviconService) NS_INTERFACE_MAP_END @@ +578,5 @@ > { > nsCOMPtr<nsIThread> thread; > (void)NS_GetMainThread(getter_AddRefs(thread)); > if (mCallback) { > (void)NS_ProxyRelease(thread, mCallback, PR_TRUE); I think you really wanted to release mChannel here, the base class releases mCallback already ::: toolkit/components/places/AsyncFaviconHelpers.h @@ +127,5 @@ > + virtual ~AsyncFaviconHelperBase(); > + > + nsCOMPtr<mozIStorageConnection>& mDBConn; > + nsRefPtr<nsFaviconService> mFaviconSvc; > + nsCOMPtr<nsIFaviconDataCallback> mCallback; Please retain the comments from the original location of this code. @@ +280,5 @@ > + * URL of the page whose favicon's URL we're fetching > + * @param aDBConn > + * database connection to use > + * @param aCallback > + * function to be called when the fetch and associate process finishes there is no associate process here (copy/paste fail probably) @@ +296,5 @@ > + * database connection to use > + * @param aFaviconSvc > + * the favicon service to query > + * @param aCallback > + * function to be called when the fetch and associate process finishes ditto ::: toolkit/components/places/mozIAsyncFaviconService.idl @@ +46,5 @@ > + * asynchronously. It may change or even go away in the future. > + */ > + > +[scriptable, uuid(6D2B0507-245F-452D-9718-5595DCD3CD14)] > +interface mozIAsyncFaviconService : nsISupports Per code style with other async interfaces, we are dropping the Service suffix, so this ends up being mozIAsyncFavicons (with mozIAsyncHistory and future mozIAsyncBookmarks) Also add @status EXPERIMENTAL before the interface, as we do in http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/mozIAsyncHistory.idl#122 @@ +49,5 @@ > +[scriptable, uuid(6D2B0507-245F-452D-9718-5595DCD3CD14)] > +interface mozIAsyncFaviconService : nsISupports > +{ > + /** > + * This method is identical to nsIFaviconService::SetAndLoadFaviconForPage. move this to a @note @@ +79,5 @@ > + * @param aForceReload > + * If aForceReload is false, we try to reload the favicon only if we > + * don't have it or it has expired from the cache. Setting > + * aForceReload to true causes us to reload the favicon even if we > + * have a usable copy. missing a line for the optional callback. @@ +89,5 @@ > + > + /** > + * Asynchronously retrieve the URL of the favicon for the given page. The > + * lookup is done off the main thread, but the callback is on the main > + * thread. I think both Asynchronous and the callback on main-thread stuff is useless. the former is well specified in the name of the interface (plus you don't get any return result), the latter is true for any interface, unless it's declared thread-safe. Rather, specify that the callback won't get any icon data. @@ +95,5 @@ > + * @param aPageURI > + * URI of the page whose favicon's URL we're looking up > + * @param aCallback > + * Once we've found the favicon's URL, we call > + * aCallback->OnFaviconDataAvailable. Hm, I think this is confusing since the callback interface is declared also with "function". Maybe we should just say "Callback to be invoked when the fetching is done." (and same in setAndFetch...) add in both a @see nsIFaviconDataCallback in nsIFaviconService.idl ::: toolkit/components/places/tests/unit/test_favicons.js @@ +59,5 @@ > // Get favicon service > try { > var iconsvc = Cc["@mozilla.org/browser/favicon-service;1"]. > + getService(Ci.nsIFaviconService). > + QueryInterface(Ci.mozIAsyncFaviconService); jusr do: var iconsvc = PlacesUtils.favicons; PlacesUtils is already in scope for all of Places xpcshell tests.

Comment on attachment 532793 [details] [diff] [review] Patch v4.1 the test change is fine, I'll review next version

> we should probably fix the service declaration to solve ambiguity at the > source, something like the following may work instead of the classic > IMPL_ISUPPORTS2 > > NS_IMPL_ADDREF(nsFaviconService) > NS_IMPL_RELEASE(nsFaviconService) > NS_INTERFACE_MAP_BEGIN(nsFaviconService) > NS_INTERFACE_MAP_ENTRY(nsIFaviconService) > NS_INTERFACE_MAP_ENTRY(mozIAsyncFavicons) > NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIFaviconService) > NS_INTERFACE_MAP_END I think IMPL_ISUPPORTS2 does exactly that. From nsISupportsImpl.h: #define NS_IMPL_ISUPPORTS2(_class, _i1, _i2) NS_IMPL_ADDREF(_class) NS_IMPL_RELEASE(_class) NS_IMPL_QUERY_INTERFACE2(_class, _i1, _i2) #define NS_IMPL_QUERY_INTERFACE2(_class, _i1, _i2) NS_INTERFACE_TABLE_HEAD(_class) NS_INTERFACE_TABLE2(_class, _i1, _i2) NS_INTERFACE_TABLE_TAIL #define NS_INTERFACE_TABLE2(_class, _i1, _i2) NS_INTERFACE_TABLE_BEGIN NS_INTERFACE_TABLE_ENTRY(_class, _i1) NS_INTERFACE_TABLE_ENTRY(_class, _i2) NS_INTERFACE_TABLE_ENTRY_AMBIGUOUS(_class, nsISupports, _i1) NS_INTERFACE_TABLE_END > there is no associate process here (copy/paste fail probably) This should be "associated process", by the way.

(In reply to comment #68) > I think IMPL_ISUPPORTS2 does exactly that. From nsISupportsImpl.h: > NS_INTERFACE_TABLE_ENTRY_AMBIGUOUS(_class, nsISupports, _i1) well, NS_INTERFACE_MAP_ENTRY_AMBIGUOUS is slightly different from what I can tell, doesn't it change anything?

There are two ways of implementing QI: Maps and tables. Maps are the old way, tables are the new way. I think those two macros are equivalent; they just apply to different ways of implementing QI.

> there is no associate process here (copy/paste fail probably) Oh, now I understand. It should be "the fetch-and-associate process". It gets hyphenated because "fetch-and-associate" is a single modifier attached to "process". I'll fix that in the comments.

> missing a line for the optional callback [in > mozIAsyncFaviconService::setAndFetchFaviconForPage]. Would you like me to add this to nsIFaviconService::setAndLoadFaviconForPage as well?

sure, fix that comment as well.

Comment on attachment 532814 [details] [diff] [review] Patch v5 Review of attachment 532814 [details] [diff] [review]: ----------------------------------------------------------------- Seems OK to me. Sdwilsh could you please give feedback on the new idl (interface, method, arguments naming)? Once Shawn approves, I think you could ping rs or gavin for the superreview, unless you have your personal superreviewer :) ::: toolkit/components/places/mozIAsyncFavicons.idl @@ +70,5 @@ > + * This function will not save favicons for non-bookmarked URIs when > + * history is disabled. The rest of the functions > + * here will always store favicons even when history is disabled. > + > + * @note This method is identical to there is an empty line without *

Comment on attachment 532814 [details] [diff] [review] Patch v5 Review of attachment 532814 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. ::: toolkit/components/places/PlacesUtils.jsm @@ +2155,5 @@ > > +XPCOMUtils.defineLazyGetter(PlacesUtils, "favicons", function() { > + return Cc["@mozilla.org/browser/favicon-service;1"]. > + getService(Ci.nsIFaviconService). > + QueryInterface(Ci.mozIAsyncFavicons); I'm inclined to say that we should just do the nsIClassInfo bits to make this interface automagic to JS consumers. ::: toolkit/components/places/mozIAsyncFavicons.idl @@ +14,5 @@ > + * > + * The Original Code is Places code > + * > + * The Initial Developer of the Original Code is > + * Mozilla Foundation. nit: "the Mozilla Foundation." @@ +52,5 @@ > +{ > + /** > + * Same as nsIFaviconService::SetFaviconUrlForPage except that this also > + * attempts to fetch and save the icon data by loading the favicon URI > + * through an async network request. I would prefer it if we didn't reference this other method, and just said what we needed to say about it here. Having to open up another file to figure out what this does seems less than ideal. @@ +71,5 @@ > + * history is disabled. The rest of the functions > + * here will always store favicons even when history is disabled. > + > + * @note This method is identical to > + * nsIFaviconService::SetAndLoadFaviconForPage. except that it's asynchronous, yeah? ::: toolkit/components/places/nsFaviconService.h @@ +65,5 @@ > // forward definition for friend class > class FaviconLoadListener; > > +class nsFaviconService : public nsIFaviconService, > + public mozIAsyncFavicons nit: comma should go under the : like https://hg.mozilla.org/mozilla-central/annotate/d124391343a0/toolkit/components/places/History.h#l63

> except that it's asynchronous, yeah? They're both asynchronous. FWIW, this is *exactly* the kind of confusion I was concerned about.

(In reply to comment #77) > They're both asynchronous. > > FWIW, this is *exactly* the kind of confusion I was concerned about. Oh, duh. setAndLoadFaviconForPage is already set in terms of being async-ready. Make the older interface's version as [deprecated] so we can drop it there. Moving forward it will only be on the async version (after a few releases). Sound OK?

yes the old version is there for compatibility reasons (as I said lots of extensions use it, more than 10)

> Make the older interface's version as [deprecated] so we can drop it there. > Moving forward it will only be on the async version (after a few releases). > Sound OK? Sounds good to me, so long as you're OK marking an interface deprecated in favor of one marked as experimental.

(In reply to comment #80) > Sounds good to me, so long as you're OK marking an interface deprecated in > favor of one marked as experimental. I sure am!

Do I need to implement GetInterfaces, GetHelperForLanguage, GetContractID, GetClassDescription, GetClassID, GetImplementationLanguage, GetFlags, and GetClassIDNoAlloc by hand? Using NS_IMPL_THREADSAFE_CI seems wrong, since the class isn't threadsafe. I don't see any other macros to use, and I can't figure out what the "corresponding nsIFactory implementation" is in: * Macro to generate nsIClassInfo methods for classes which do not have * corresponding nsIFactory implementations.

(In reply to comment #82) > Do I need to implement GetInterfaces, GetHelperForLanguage, GetContractID, > GetClassDescription, GetClassID, GetImplementationLanguage, GetFlags, and > GetClassIDNoAlloc by hand? I've always had to roll it by hand. Sorry.

Mind if I write a macro? I guess we'd need to get that reviewed by an xpcom peer. Can I just use the same stubs in nsISupportsImpl.h, or should I actually try to implement the methods properly?

Oh, it looks like I don't actually need to inherit from nsIClassInfo and stub out the methods. I wish this was documented somewhere. (And if it is documented somewhere, the first thing I'm going to do tomorrow is put a link to those docs in nsIClassInfoImpl.h.)

Comment on attachment 533155 [details] [diff] [review] Patch v6 >diff --git a/toolkit/components/places/mozIAsyncFavicons.idl b/toolkit/components/places/mozIAsyncFavicons.idl >+ void getFaviconURLForPage(in nsIURI aPageURI, >+ in nsIFaviconDataCallback aCallback); Re-using nsIFaviconDataCallback for this is slightly unpleasant, I agree. But given its documentation, and the similarity between this method and setAndFetchFaviconForPage, I suppose it's fine.