We end up calling IsVisited a lot, and any changes to the code in that method can have a big impact on Tp. Currently, this data is read from a database, and this reading blocks the main thread until it is completed. With the addition of the async storage API, we can now read without blocking the main thread. This means we can query the database and get results back on the main thread without blocking the main thread, which could lead to better Tp times. I've attached a transcript of a conversation bz and I had over irc detailing one possible approach. The general idea of his suggestion is to make an async call immediately when the content node is created. Luckily for us, events on the main thread are processed before the style of the node ends up getting resolved, so we can hopefully get results back by then. If we do not, we end up using the sync call of IsVisited since we need to know (in theory, that should be in the database cache at that point too so it should be faster).

I should note that we're possibly adding code to get the anchor URI during BindToTree for bug 453403, in any case. Let's see how that plays out...

This is the new API and the implementation in places for it. I could write a native code test for it, but I'd have to mock an nsIContent node which I don't exactly want to do yet. I suspect I'll have a test before I'm ready to commit this. Note: This doesn't contain any of the changes to actually use this new API yet. I'll have that in a patch later when I work through the test failures and make sure my approach makes sense.

Comment on attachment 399621 [details] [diff] [review] New API v1.0 >+class IHistory This is a strange name for the interface, and too close, IMO, to nsISHistory. The interface doesn't seem to be about history, but link state.

Calling this interface IVisitHistory might be better. Or something like that. Not sure about the right naming. Unregistering means needing to know the URI you had when you registered, right? Will you be adding such a member to consumers, then? I see no provisions for removing things from mObservers. Should there be some? If multiple links with the same URI are added, it looks like we call Start() multiple times. Is that desired?

(In reply to comment #5) > Calling this interface IVisitHistory might be better. Or something like that. > Not sure about the right naming. As I understand things, this interface is intended to replace nsIGlobalHistory* with asynchronous methods. This is just the first step. I suppose it'd be trivial to change the name to something more generic down the line though. > Unregistering means needing to know the URI you had when you registered, right? > Will you be adding such a member to consumers, then? Every element I've seen has some method (usually GetHrefURI) that I can get that URI from. As long as I get that before I update/unset the href, I should be OK, right? > I see no provisions for removing things from mObservers. Should there be some? Whoops - meant to remove them in History::NotifyVisited. Thanks for catching that (a test would have caught the leak too :/) > If multiple links with the same URI are added, it looks like we call Start() > multiple times. Is that desired? I hadn't really thought about that case, but that optimization is trivial to add. If we find the URI already in the hashtable, we just shouldn't call Start.

This is the code that uses the new API. This isn't ready for review by any means yet.

> As long as I get that before I update/unset the href, I should be OK, right? For the href set case, yes, though it's not necessarily cheap. For the "someone just changed the base URI of the document" case, not so much.

dunno but QueryAndRegisterHistoryCallback looks a too "wide" name to just set visited status of an uri... you're talking about a History callback, history is not just visited or not. I guess if this should instead use a smaller-scope name like QueryAndRegisterURIVisitedCallback or just QueryAndRegisterVisitedCallback. or whatever.

(In reply to comment #6) > Whoops - meant to remove them in History::NotifyVisited. Thanks for catching > that (a test would have caught the leak too :/) That was actually a lie. A test would have crashed though!

Tests are hard to write. This is just a checkpoint so I don't lose a days worth of work.

So looking at the consumer patch.... Other than the copy/paste issue (which should probably be addressed via a superclass that has all those helper methods or something), it looks like it can leak, no? We register nodes when their link state is looked up, but only unregister in DestroyContent; the latter only gets called on nodes that are in a document when the document gets torn down. So nodes that are removed from a document before it's torn down, say, will stay alive for the life of the app. A separate issue is the more complicated hashing this is doing on links (in particular involving nsIURI::Equals); I hope that doesn't turn into a performance problem.

(In reply to comment #12) > So looking at the consumer patch.... Other than the copy/paste issue (which > should probably be addressed via a superclass that has all those helper methods > or something), it looks like it can leak, no? We register nodes when their > link state is looked up, but only unregister in DestroyContent; the latter only > gets called on nodes that are in a document when the document gets torn down. > So nodes that are removed from a document before it's torn down, say, will stay > alive for the life of the app. Even when a node loses all references, that method won't get called? I suppose I can call that in the destructor too. As for the copy/paste issue, I'd love to have a better place to put all that identical code, but wasn't sure where to put it. Suggestions gladly welcome from folks who know this code better than I. > A separate issue is the more complicated hashing this is doing on links (in > particular involving nsIURI::Equals); I hope that doesn't turn into a > performance problem. We can always change the hashing algorithm, yes?

With a bunch of tests. I'm not done yet, but I'm feeling more confident about this new API. Have one XXX in this about API on Unregister - should we fail if the content node was not registered before? I'm thinking that we probably shouldn't and just return NS_OK. That would make consumers lives a lot easier.

> Even when a node loses all references, that method won't get called? Correct. > I suppose I can call that in the destructor too. That won't help, since the destructor will never be called (until app shutdown). The history service is holding a ref, after all... > but wasn't sure where to put it nsStyledElement is a superclass of all the relevant things, right? Would utility methods in that work? Or in nsGenericElement? Or we could make up a new shared superclass along the lines of nsImageLoadingContent to manage "link stuff". > We can always change the hashing algorithm, yes? True, yeah.

The history service should *not* be holding a ref: this API intends for the nsIContent to track its lifetime and register/unregister itself appropriately, and the history service will not hold strong refs.

Sure. More precisely, the nsIContent does NS_ADDREF_THIS and then gives a pointer to itself to the history service. The corresponding NS_RELEASE_THIS is currently in a function that's not guaranteed to be called, and it's pretty easy to write a page on which it's NOT called for a given content node. That's my concern. Calling the function from the destructor wouldn't do any good, since the destructor can't fire until that NS_RELEASE_THIS happens.

(In reply to comment #15) > nsStyledElement is a superclass of all the relevant things, right? Would > utility methods in that work? Or in nsGenericElement? Or we could make up a > new shared superclass along the lines of nsImageLoadingContent to manage "link > stuff". I think a new superclass is cleaner, so I'll probably end up going with that. Anybody have a good idea as to where that code should live? (In reply to comment #17) > Sure. More precisely, the nsIContent does NS_ADDREF_THIS and then gives a > pointer to itself to the history service. The corresponding NS_RELEASE_THIS is > currently in a function that's not guaranteed to be called, and it's pretty > easy to write a page on which it's NOT called for a given content node. That's > my concern. Calling the function from the destructor wouldn't do any good, > since the destructor can't fire until that NS_RELEASE_THIS happens. This would just involve some script that would remove a node from the document, then close itself, yes? I'll add a test for this behavior to make sure we don't leak.

What is the NS_ADDREF_THIS for? Part of the point originally was that you wouldn't have to have a strong ref at all. It seems that currently it's still a strong ref, just managed differently and oddly.

Put the new superclass in content/base/src. > This would just involve some script that would remove a node from the document, > then close itself, yes? I think so, yes. Check that doing that leaks with your patch as-is?

(In reply to comment #19) > What is the NS_ADDREF_THIS for? Part of the point originally was that you > wouldn't have to have a strong ref at all. It seems that currently it's still a > strong ref, just managed differently and oddly. Seemed dangerous to not do it, but thinking about it more, as long as we unregistere at the right time, it is fine to not addref or release it. This will also make some of the logic simpler.

If you don't have strong refs you can unregister in the destructor.

Alright - I think this is in a reviewable state or at least enough so that people can start commenting on things. I have one XXX comment and I've addressed everything else that has been brought up in this bug.

Comment on attachment 401522 [details] [diff] [review] New API v1.3 Not a lot of stuff touches Places so i don't have lot of comments. i still don't like much method names of the API, and i would like the cpp tests being in a new folder rather then in tests. Just r- for this, but looks good otherwise. >diff --git a/docshell/base/IHistory.h b/docshell/base/IHistory.h >+class IHistory : public nsISupports >+{ >+public: >+ NS_DECLARE_STATIC_IID_ACCESSOR(IHISTORY_IID) >+ >+ /** >+ * Registers and queries history for the visited status of aURI. I expand this comment as "Registers history and queries history for...", and "Registers" history does not make sense, maybe rephrase. the name QueryAndRegisterVisitedCallback does not help a lot the comment since there is no callback here around. >+ /** >+ * Unregisters a previously registered nsIContent object. >+ * >+ * @param aURI >+ * The URI of the page to check. >+ * @param aContent >+ * The content object to unregister for aURI. >+ */ >+ NS_IMETHOD UnregisterVisitedCallback(nsIURI *aURI, >+ nsIContent *aContent) = 0; I'm not sure if the Query prefix of the first method is really useful if it would just be RegisterVisitedCallback, with a better comment to explain the queyr part, would probably be better. >diff --git a/toolkit/components/places/src/History.cpp b/toolkit/components/places/src/History.cpp >+class VisitedQuery : public mozIStorageStatementCallback >+{ >+public: >+ NS_DECL_ISUPPORTS >+ >+ static nsresult Start(nsIURI *aURI) >+ { >+ NS_ASSERTION(aURI, "Don't pass a null URI!"); >+ >+ mozIStorageStatement *stmt = >+ nsNavHistory::GetHistoryService()->DBGetIsVisited(); Should you check if nsNavHistory::GetHistoryService() give you something useful? If some db corruption locks its startup this looks like a null reference What happens if this runs after shutdown when history has already finalized the statement? >+ NS_IMETHOD HandleResult(mozIStorageResultSet *aResults) >+ { >+ // If this method is called, we've gotten results. >+ mIsVisited = true; i think the name of the method is self-explaining... Maybe you want to tell that if we got results the page is visited? >+ NS_IMETHOD HandleCompletion(PRUint16 aReason) >+ { >+ if (mIsVisited) >+ History::GetService()->NotifyVisited(mURI); a failure in getting the service here should be covered by the NS_ABORT_IF_FALSE in GetService? >+NS_IMETHODIMP >+History::QueryAndRegisterVisitedCallback(nsIURI *aURI, >+ nsIContent *aContent) >+{ >+ NS_ENSURE_ARG(aURI); >+ NS_ENSURE_ARG(aContent); >+ >+ // Ensure that our hash table is setup first. s/ first// >diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp >--- a/toolkit/components/places/src/nsNavHistory.cpp >+++ b/toolkit/components/places/src/nsNavHistory.cpp >@@ -55,6 +55,7 @@ > #include "nsPlacesTriggers.h" > #include "nsPlacesMacros.h" > #include "SQLFunctions.h" >+#include "History.h" > > #include "nsIArray.h" > #include "nsTArray.h" >@@ -2805,6 +2806,9 @@ nsNavHistory::AddVisit(nsIURI* aURI, PRT > obsService->NotifyObservers(aURI, NS_LINK_VISITED_EVENT_TOPIC, nsnull); > } > >+ // Because we implement IHistory, we always have to notify about the visit. >+ History::GetService()->NotifyVisited(aURI); same concern >diff --git a/toolkit/components/places/tests/Makefile.in b/toolkit/components/places/tests/Makefile.in >--- a/toolkit/components/places/tests/Makefile.in >+++ b/toolkit/components/places/tests/Makefile.in >@@ -72,6 +72,10 @@ DIRS = \ > browser \ > $(NULL) > >+CPP_UNIT_TESTS = \ >+ test_IHistory.cpp \ >+ $(NULL) uargh, could you move all of this into a cpptests subfolder? I don't like all cpp and header files to pollute main tests folder since all other tests are grouped in folders >diff --git a/toolkit/components/places/tests/places_test_harness_tail.h b/toolkit/components/places/tests/places_test_harness_tail.h >+int >+main(int aArgc, >+ char **aArgv) >+{ >+ ScopedXPCOM xpcom(TEST_NAME); >+ >+ do_test_pending(); >+ run_next_test(); >+ >+ // Spin the event loop until we've run out of tests to run. >+ while (gPendingTests) >+ (void)NS_ProcessNextEvent(); >+ >+ // And let any other events finish before we quit. >+ while (NS_HasPendingEvents()) >+ (void)NS_ProcessNextEvent(); >+ >+ // Dispatch quit-applicaiton, and then let events process one last time. >+ nsCOMPtr<nsIObserverService> os = >+ do_GetService("@mozilla.org/observer-service;1"); >+ (void)os->NotifyObservers(nsnull, "quit-application", nsnull); >+ while (NS_HasPendingEvents()) >+ (void)NS_ProcessNextEvent(); is this to mimic our current cleanup behavior in tests? Actually i think i'm near getting rid of all of this crap with a couple bugs, btw, do we leak without this? >+ >+ >+ if (gPassedTests == gTotalTests) >+ passed(TEST_FILE); double new line above these >+ >+ (void)printf("%zu of %zu tests passed\n", gPassedTests, gTotalTests); >+ >+ return gPassedTests == gTotalTests ? 0 : -1; is this automatically reporting to tinderboxes or not yet hooked up? >diff --git a/toolkit/components/places/tests/test_IHistory.cpp b/toolkit/components/places/tests/test_IHistory.cpp >+// These variables are shared between part 1 and part 2 of the test. Part 2 >+// set the nsCOMPtr's to nsnull, freeing the reference. s/set/sets/ >+ // We would have had a fail at this point had the content node been told it >+ // was visited. Therefore, it is safe to unregister our content node. I'm not english mother-tongue, i admit. The first phrase has caused me a bad rant :) >+void >+test_unregistered_visited_does_not_notify() >+{ >+ (void)printf("Running test_unregistered_visited_does_not_notify.\n"); >+ >+ // This test must have a test that has a successful notification after it so >+ // we can ensure that this would have been notified by now if we notified >+ // unregistered content nodes (due to request serialization). put some commas in the middle, and make this understandable by my cat, please! >+void >+test_new_visit_notifies_waiting_content() >+{ >+ (void)printf("Running test_new_visit_notifies_waiting_content.\n"); >+ >+ // First, we need a test URI. We'll add it to history after we've registered >+ // our content node. >+ nsCOMPtr<nsIURI> testURI(new_test_uri()); fine, so why do you create it here rather than later neat the point where you need it? >+ // Ensure that calling Unregister for a URI that was not registered does not >+ // return a failure. >+ // XXX is this the API we want? >+ nsresult rv = history->UnregisterVisitedCallback(testURI, content); what's the alternative? a failure? That for sure would force users of the API to be less lazy, but, well i think this asserts in debug no? so devs are notified >+/** >+ * Note: for tests marked "Order Important!", please see the test for details. >+ */ >+void (*gTests[])(void) = { >+ test_null_args_fail, >+ test_unvisted_does_not_notify_part1, // Order Important! >+ test_visited_notifies, >+ test_unvisted_does_not_notify_part2, // Order Important! >+ test_same_uri_notifies_both, >+ test_unregistered_visited_does_not_notify, // Order Important! >+ test_new_visit_notifies_waiting_content, >+ test_unregister_nonregistered_content, I'ts not much comfortable to go up and down seeing details, maybe you could tell "should be before test_XXX", or "should be after test_XXX" and send to details for further information. Not a big deal though, one can just enqueue new tests. >diff --git a/xpcom/tests/TestHarness.h b/xpcom/tests/TestHarness.h >+ // If we created a profile directory, we need to remove it. >+ if (mProfD) { >+ if (NS_FAILED(mProfD->Remove(PR_TRUE))) >+ NS_WARNING("Problem removing profile direrctory"); typo: direrctory

(In reply to comment #24) > I expand this comment as "Registers history and queries history for...", and > "Registers" history does not make sense, maybe rephrase. > the name QueryAndRegisterVisitedCallback does not help a lot the comment since > there is no callback here around. I've updated the comment, but the content node is the callback. We'll see what bsmedberg says (the sr) about the API name. > I'm not sure if the Query prefix of the first method is really useful if it > would just be RegisterVisitedCallback, with a better comment to explain the > queyr part, would probably be better. It was asked for in initial e-mail threads prototyping this API, and I prefer to have it. Having a method name that actually describes what it's doing is good (and even encourage as a good coding practice!). > What happens if this runs after shutdown when history has already finalized the > statement? We shouldn't be creating content nodes after shutdown, but with the added safety checks, the storage API will return an error that we will propagate. > a failure in getting the service here should be covered by the > NS_ABORT_IF_FALSE in GetService? Correct. > >+ // Ensure that our hash table is setup first. > > s/ first// rephrased, but I want first in there (it's the first thing we need to do). > is this to mimic our current cleanup behavior in tests? Sorta. Part of this is from head.js (xpcshell test harness), and some of it is because of how we finalize statements. > Actually i think i'm near getting rid of all of this crap with a couple bugs, > btw, do we leak without this? We don't leak, but we assert due to non-finalized statements. > is this automatically reporting to tinderboxes or not yet hooked up? make check tests are noted, but they are like xpcshell - the test either passes or fails. It's really just useful output at the end so it's clear that the test has passed. > I'm not english mother-tongue, i admit. The first phrase has caused me a bad > rant :) Changed fail to failure, but that's valid English and I'm not sure how else to say that. > what's the alternative? a failure? Yes. I think letting consumers assume they were registered and just let them call unregister whenever is an easier API, so we should probably go with this. > I'ts not much comfortable to go up and down seeing details, maybe you could > tell "should be before test_XXX", or "should be after test_XXX" > and send to details for further information. The idea is that people don't change the order by that comment, and if someone really wants to know why the order is important, they can see the test. I'd prefer to keep this this way. > >diff --git a/xpcom/tests/TestHarness.h b/xpcom/tests/TestHarness.h whoops - meant to pull this out into a different bug.

Per review comments.

Comment on attachment 401562 [details] [diff] [review] New API v1.4 diff --git a/docshell/base/IHistory.h b/docshell/base/IHistory.h +class IHistory : public nsISupports +{ +public: + NS_DECLARE_STATIC_IID_ACCESSOR(IHISTORY_IID) + + /** + * Registers the content node to be notified about visited sate and queries + * history for the visited status of aURI. typo: visited sate my concerns are fixed, fine afaict.

Comment on attachment 401562 [details] [diff] [review] New API v1.4 >diff --git a/docshell/base/IHistory.h b/docshell/base/IHistory.h >+ /** >+ * Registers the content node to be notified about visited sate and queries >+ * history for the visited status of aURI. >+ * >+ * @param aURI >+ * The URI of the page to check. >+ * @param aContent >+ * The content to update whenever the history status changes. >+ */ >+ NS_IMETHOD QueryAndRegisterVisitedCallback(nsIURI *aURI, >+ nsIContent *aContent) = 0; Please be specific that this holds a raw references to the nsIContent node, and that unregistration is absolutely required. >diff --git a/docshell/base/Makefile.in b/docshell/base/Makefile.in >+EXPORTS_mozilla = \ >+ IHistory.h \ >+ $(NULL) nit, please use two-space indentation here >diff --git a/toolkit/components/places/src/History.cpp b/toolkit/components/places/src/History.cpp >+NS_IMETHODIMP >+History::QueryAndRegisterVisitedCallback(nsIURI *aURI, >+ nsIContent *aContent) >+{ >+ NS_ENSURE_ARG(aURI); >+ NS_ENSURE_ARG(aContent); nit, make these assertions not runtime checks >+ // Then, see if we already have an array for this URI. We will append to it if >+ // we can. >+ ObserverArray *array; >+ if (!mObservers.Get(aURI, &array)) { >+ // Since we don't have an array added for this URI yet, add it. >+ array = new ObserverArray(); >+ NS_ENSURE_TRUE(array, NS_ERROR_OUT_OF_MEMORY); >+ NS_ENSURE_TRUE(mObservers.Put(aURI, array), NS_ERROR_OUT_OF_MEMORY); hrm, the Get/Put thing is unfortunate. If this were a nsTHashtable you could avoid the extra hash/lookup with a single PutEntry call. >+NS_IMETHODIMP >+History::UnregisterVisitedCallback(nsIURI *aURI, >+ nsIContent *aContent) >+{ >+ NS_ENSURE_ARG(aURI); >+ NS_ENSURE_ARG(aContent); more assertions instead of runtime checks >+ // Get the array, and remove the item from it. >+ ObserverArray *array; >+ if (!mObservers.Get(aURI, &array)) >+ return NS_OK; // Don't return failure if this URI isn't registered. This really ought to be an assertion: if somebody is calling UnregisterVisitedCallback it's because they registered one already. Not finding it seems like a programming error we ought to catch. >diff --git a/toolkit/components/places/src/History.h b/toolkit/components/places/src/History.h >+ * The Initial Developer of the Original Code is >+ * Mozilla Corporation. Here and elsewhere, Mozilla Foundation, not Corp (all copyright is assigned to the foundation). >+ static History *mService; gService, not mService (it's not a member!) sr=me with nits fixed

(In reply to comment #28) > hrm, the Get/Put thing is unfortunate. If this were a nsTHashtable you could > avoid the extra hash/lookup with a single PutEntry call. But I need to always Get first to know if I have and entry automatically, right? If I already have one, I just reuse it.

Addresses everything but the hashtable changes. I need to do the Get followed by a Put when the Get fails because I allocate the entry to place in the hash table. I think it's worse to allocate and then free than it is to have a Get then Put call.

bsmedberg - you gave me a pastebin link of some code to make the nsTHashTable work, but I can't find it in my history. Do you happen to have it handy still?

something like class KeyClass : public nsURIHashKey { KeyClass(KeyTypePointer* foo) : nsURIHashKey(foo), mArray(NULL) { } KeyClass(const KeyClass& toCopy) { NS_NOTREACHED("Don't call me"); } nsAutoTArray<nsIContent*, 1>* mArray; }; and then nsTHashtable<KeyClass>

Thanks - for some reason I was thinking it was more complicated than that :/ Should get back to this bug later this week to address that review comment and anything bz might say if he gets to the review before I address the hash table stuff.

Use nsTHashtable instead of nsClassHashtable. I'll start work on using this API once it gets review.

In an effort to make the changes need to this simpler and easier to understand, I'm refactoring out some common code for links into a superclass per comment 12. I'm pretty sure the html stuff is right, but not so sure about the svg bits. Hopefully the name and location of the superclass are in OK.

I forgot to qrefresh before submitting that patch.

It should be noted I had to make one minor change to the patch (currently has a compile error). Not going to bother uploading a new version, however. With this refactoring, I could change the History API to take a mozilla::content::Link instead of an nsIContent object. bsmedberg - do you want me to make that change?

bz or somebody from content should decide.

Comment on attachment 406092 [details] [diff] [review] Refactor v1.1 Though I think i'd prefer to call the namespace mozilla::dom rather than mozilla::content. We'll likely have all the content code in the dom/ directory before long.

Comment on attachment 406092 [details] [diff] [review] Refactor v1.1 >+++ b/content/html/content/src/nsHTMLAreaElement.cpp > nsHTMLAreaElement::UnbindFromTree(PRBool aDeep, PRBool aNullParent) > GetCurrentDoc()->ForgetLink(this); You can nix that call, right? ResetLinkState will handle it? Fix sicking's namespace nit, and looks good.

Comment on attachment 403622 [details] [diff] [review] New API v1.6 >+++ b/docshell/base/IHistory.h >+ * Registers the content node to be notified about visited sate and queries >+ * history for the visited status of aURI. s/sate/state/ What does this comment mean in practice? From the consumer's point of view, what will happen? Every time the visited state of aURI changes presumably the consumer will be notified. How will this notification be sent? Or something else? There will also presumably be a notification as a result of this QueryAndRegisterVisitedCallback call. What sort of notification? Synchronous or asynchronous? >+ * @param aURI >+ * The URI of the page to check. s/of the page// >+ * @param aContent >+ * The content to update whenever the history status changes. This >+ * is a raw reference. Presumably this should say that the history implementation will hold a raw pointer to the nsIContent, so the caller must make sure to call UnregisterVisitedCallback if the node is about to be destroyed. >+ NS_IMETHOD QueryAndRegisterVisitedCallback(nsIURI *aURI, >+ nsIContent *aContent) = 0; How about RegisterVisitedCallback or RegisterVisitedObserver, with the "query" part (by which I assume you mean the initial notification it sends) described in the comments? >+ * @param aURI >+ * The URI of the page that aContent was registered for. s/of the page// >+ NS_IMETHOD UnregisterVisitedCallback(nsIURI *aURI, If we decide on RegisterVisitedObserver, this should be renamed likewise. But generally, I'm fine with Register/UnregisterVisitedCallback. This should document what guarantees, if any, UnregisterVisitedCallback actually makes in terms of the notifications that would normally be delivered as a result of RegisterVisitedCallback. Hard to say what it should say until I see the documentation for that. >+++ b/toolkit/components/places/src/History.cpp >@@ -0,0 +1,262 @@ >+/* vim: sw=2 ts=2 et lcs=trail\:.,tab\:>~ : Want to add an emacs modeline too? ;) So if I read this implementation correctly, here's what RegisterVisitedObserver provides: A single callback if the node has been visited, nothing otherwise. The callback is guaranteed to happen asynchronously. The callback will call SetLinkState on the content node. This callback is guaranteed to come after a "link-visited" observer notification for the visit, if one happens before the database query completes. Does that about sum things up?

(In reply to comment #41) > What does this comment mean in practice? From the consumer's point of view, > what will happen? Every time the visited state of aURI changes presumably the > consumer will be notified. How will this notification be sent? Or something > else? I'll make sure the next iteration makes this more clear. > There will also presumably be a notification as a result of this > QueryAndRegisterVisitedCallback call. What sort of notification? Synchronous > or asynchronous? As well as this. > How about RegisterVisitedCallback or RegisterVisitedObserver, with the "query" > part (by which I assume you mean the initial notification it sends) described > in the comments? Going to stick with Callback name instead of Observer to avoid confusion with nsIObserver. > Want to add an emacs modeline too? ;) Tell me what it should be. I don't use emacs, so I have no way to test it. > So if I read this implementation correctly, here's what RegisterVisitedObserver > provides: A single callback if the node has been visited, nothing otherwise. > The callback is guaranteed to happen asynchronously. The callback will call > SetLinkState on the content node. This callback is guaranteed to come after a > "link-visited" observer notification for the visit, if one happens before the > database query completes. This sums things up well, but I don't think I want to make it set in stone that this comes after "link-visited". I don't think the order is important, and I'd hate to tie our hands like that for future changes (like maybe getting rid of that topic too). Also, thoughts on changing this API to take the Link class instead of the nsIContent? We'd have to then export Link as well, but I don't think that's a big deal.

> Tell me what it should be. Copy the one from IHistory.h except s/4/2/ ? > I don't think the order is important Well, that was what I was failing to convince myself of... Why does the order not matter? I think making the API take Link is a fine idea.

(In reply to comment #39) > (From update of attachment 406092 [details] [diff] [review]) > Though I think i'd prefer to call the namespace mozilla::dom rather than > mozilla::content. We'll likely have all the content code in the dom/ directory > before long. Done (In reply to comment #40) > You can nix that call, right? ResetLinkState will handle it? Correct I've pushed this to the tryserver, and assuming all goes well, will push this to mozilla-central tomorrow. I think this patch stands alone from this work. If you have any objections, please voice them.

Comment on attachment 410066 [details] [diff] [review] Part 1 - Refactor v1.2 (checked in) http://hg.mozilla.org/mozilla-central/rev/d89369248cbf

(In reply to comment #43) > Well, that was what I was failing to convince myself of... Why does the order > not matter? I can't think of a case where a consumer would need to use both, so I think it's best to leave it unspecified. Do you have a case in mind?

I'm pretty sure that this addresses the review comments so far. I've added a test as well, and the testing code has gotten simpler since I don't need to mock a full nsIContent object (just a mozilla::dom::Link).

> I can't think of a case where a consumer would need to use both Anchors are affected by both (this one directly, and the other via nsIDocument). Is the interaction between the two ok?

(In reply to comment #48) > Anchors are affected by both (this one directly, and the other via > nsIDocument). Is the interaction between the two ok? Oh, hrm. Originally, I was going to make the nsIDocument code not use that topic anymore (and not track links for that matter), but we can't do that anymore due to some other code now needing that. I think the interaction is fine, I'm still hesitant to spec it out. We should probably make sure there are tests for the right behavior before we land everything though.

Going to try and do this in lots of small steps to making reviewing this easier. We'll see how it goes. This patch adds a getter to nsContentUtils for IHistory.

Comment on attachment 410556 [details] [diff] [review] New API v1.7 >diff --git a/docshell/base/IHistory.h b/docshell/base/IHistory.h >+#define IHISTORY_IID \ >+ {0xaf27265d, 0x5672, 0x4d23, {0xa0, 0x75, 0x34, 0x8e, 0xb9, 0x73, 0x5a, 0x9a}} all the file has 4 spaces indentation but this line, could be wanted, just pointing it out >diff --git a/toolkit/components/places/src/History.cpp b/toolkit/components/places/src/History.cpp >+namespace { >+ >+class VisitedQuery : public mozIStorageStatementCallback >+{ >+public: >+ NS_DECL_ISUPPORTS >+ >+ static nsresult Start(nsIURI *aURI) >+ { >+ NS_ASSERTION(aURI, "Don't pass a null URI!"); >+ >+ nsNavHistory *navHist = nsNavHistory::GetHistoryService(); >+ NS_ENSURE_TRUE(navHist, NS_ERROR_FAILURE); >+ mozIStorageStatement *stmt = navHist->DBGetIsVisited(); >+ NS_ENSURE_TRUE(stmt, NS_ERROR_UNEXPECTED); nit: fwiw this is NS_ENSURE_STATE(stmt) >+ NS_IMETHOD HandleResult(mozIStorageResultSet *aResults) >+ { >+ // If this method is called, we've gotten results which means we have a >+ // visit. nit: comma after "results" and not after "called"? >diff --git a/toolkit/components/places/src/nsNavHistory.h b/toolkit/components/places/src/nsNavHistory.h >--- a/toolkit/components/places/src/nsNavHistory.h >+++ b/toolkit/components/places/src/nsNavHistory.h >@@ -253,6 +253,8 @@ public: > mozIStorageStatement* DBGetTags() { return mDBGetTags; } > PRInt64 GetTagsFolder(); > >+ mozIStorageStatement *DBGetIsVisited() { return mDBIsPageVisited; } >+ is there a possibility that this gets called when mDBIsPageVisited has been already finalized (e.g. after or during xpcom-shutdown)? since all of this is async, i assume it could happen, what happens in such a case? >diff --git a/toolkit/components/places/tests/cpp/places_test_harness_tail.h b/toolkit/components/places/tests/cpp/places_test_harness_tail.h >+#ifndef TEST_NAME >+#error "Must #define TEST_NAME before including places_test_harness_tail.h" >+#endif i don't know if all compilers will appreciate that #define in the string, i assume so >+ ScopedXPCOM xpcom(TEST_NAME); >+ >+ do_test_pending(); >+ run_next_test(); >+ >+ // Spin the event loop until we've run out of tests to run. >+ while (gPendingTests) >+ (void)NS_ProcessNextEvent(); >+ >+ // And let any other events finish before we quit. >+ while (NS_HasPendingEvents()) >+ (void)NS_ProcessNextEvent(); >+ >+ // Dispatch quit-applicaiton, and then let events process one last time. >+ nsCOMPtr<nsIObserverService> os = >+ do_GetService("@mozilla.org/observer-service;1"); >+ (void)os->NotifyObservers(nsnull, "quit-application", nsnull); >+ while (NS_HasPendingEvents()) >+ (void)NS_ProcessNextEvent(); actually, calling quit-application is completely useless on trunk after we moved all shutdown down the line, and so could be also the previous events spinning could be completely useless, please check if you can get rid of all of this. if it's not called already, you should at last notify xpcom-shutdown. you should also check os is valid.

(In reply to comment #51) > is there a possibility that this gets called when mDBIsPageVisited has been > already finalized (e.g. after or during xpcom-shutdown)? > since all of this is async, i assume it could happen, what happens in such a > case? No, and getting the statement is synchronous (we do it right away). > i don't know if all compilers will appreciate that #define in the string, i > assume so Storage has had this structure for some time checked in. Should be fine.

Addresses mak's review comments. All this needs now is bz's review.

I uploaded the wrong patch...

Comment on attachment 411457 [details] [diff] [review] New API v1.8 >+++ b/docshell/base/IHistory.h >+ mozilla::dom::Link *aLink) = 0; Do you need the "mozilla::" part there? If not, take it out for both methods? We should probably document that SetLinkState must not do anything that might modify the arrays here, right? Either that or use nsTObserverArray so we can deal with such modifications. r=bzbarsky with that.

Per review comments. I added an @note documenting that SetLinkState cannot register or unregister, and added an assertion in History::NotifyVisited.

Adds Link::LinkState to be called by a subclass's IntrinsicState.

Comment on attachment 414548 [details] [diff] [review] Part 4 - Add NS_EVENT_STATE_UNVISITED to nsIEventStateManager v1.0 These are bits, not numeric flags. We should really have written them as (1 << whatever), I guess... So this should become 0x00000800, and the following ones get renumbered.

nsSVGAElement needs to implement GetHrefURI so so Link can call that to cache the URI (Part 7).

Comment on attachment 414549 [details] [diff] [review] Part 5 - Add Link::LinkState v1.0 Drop the else after return and document "or 0 if this is not actually a link" or something along those lines? sr=bzbarsky with that

Add Link::GetURI which will lazily cache the URI (and gets the URI by calling GetHrefURI) which will be used to register and unregister the Link with History (Part 11).

We no longer need eLazyURIValue. When getting the URI, we should call Link::GetURI instead (or recompute).

Comment on attachment 414551 [details] [diff] [review] Part 7 - Link should cache its URI v1.0 Looks ok, but see comments on next patch.

Comment on attachment 414553 [details] [diff] [review] Part 8 - Remove eLazyURIValue v1.0 This drops use of the URI cache in the places that use it now (particularly the HTMLAnchorElement mutation methods). That'll cause performance regressions. It seems to me that we should move to using the Link cache if we have one, no? For example, move nsGenericHTMLElement::GetURIAttr to Link or something?

(In reply to comment #65) > (From update of attachment 414553 [details] [diff] [review]) > This drops use of the URI cache in the places that use it now (particularly the > HTMLAnchorElement mutation methods). That'll cause performance regressions. > It seems to me that we should move to using the Link cache if we have one, no? > For example, move nsGenericHTMLElement::GetURIAttr to Link or something? So, I moved the helpers that I think you are referring to in Part 9 of this bug. I'll be uploading that shortly, and you can take a look to see if I've addressed that.

Per review comments and irc conversation.

Per review comments.

Move helper methods on nsGenericHTMLElement to Link and use the cached URI.

Comment on attachment 414560 [details] [diff] [review] Part 4 - Add NS_EVENT_STATE_UNVISITED to nsIEventStateManager v1.1 Make those look like: (1 << 0) (so add parens) and looks good.

Make sure we call Link::ResetLinkCache in Bind and Unbind

I somehow managed to miss a few in the last patch, so I'm asking for review for a sanity check.

Comment on attachment 414562 [details] [diff] [review] Part 9 - Move helper methods to Link v1.0 I haven't read this in detail yet, but at first glance: 1) This changes some code (removes comments, changes nsresults to ints, etc) instead of just moving it. Why? Some of it was the way it was for a reason. 2) This makes us serialize and reparse the URI underLink::SetHref (which should be SetURI anyway, I'd think), right? We do that sometimes already, per bug 486806 but not always. Can we either avoid it or file a followup bug on avoiding it?

Oh, and I assume that SetAttr does reset the mCachedURI on the Link?

Comment on attachment 414563 [details] [diff] [review] Part 10 - Ensure we call ResetLinkState on BindFromTree and UnbindFromTree v1.0 This looks fine, but do we need the Link:: qualifier there?

(In reply to comment #73) > 1) This changes some code (removes comments, changes nsresults to ints, etc) > instead of just moving it. Why? Some of it was the way it was for a reason. Hmm, what bits matter? I was only changing things to clean up code and make it conform to the style in the Link.cpp file. Anything dropped or changed in behavior would be unexpected. If I did that, it was unintentional. > 2) This makes us serialize and reparse the URI underLink::SetHref (which > should be SetURI anyway, I'd think), right? We do that sometimes already, per > bug 486806 but not always. Can we either avoid it or file a followup bug on > avoiding it? We were already doing this work, correct? Wouldn't the followup be bug 486806? We could call it SetURI if you'd like. It was originally named SetHrefToURI, so I could go back to that too if you want.

(In reply to comment #74) > Oh, and I assume that SetAttr does reset the mCachedURI on the Link? All subclasses call Link::ResetLinkState which clears the cache, so yes.

(In reply to comment #75) > (From update of attachment 414563 [details] [diff] [review]) > This looks fine, but do we need the Link:: qualifier there? Probably not, but 1) we've done it everywhere else we've called the Link methods and 2) I think it makes the code clearer that the method we are calling is on a different class. Since these objects tend to inherit from multiple classes, being explicit about which class has the method we are calling seems like a win to readability to me. I can change this if you want though.

Comment on attachment 414553 [details] [diff] [review] Part 8 - Remove eLazyURIValue v1.0 Re-requesting sr here since the comment 65 (review comment) should be addressed by attachment 414562 [details] [diff] [review] (as indicated in comment 66)

> Hmm, what bits matter? The comments explaining the behavior? The fact that the "int" that the string methods output as a result really is an nsresult (the string api lies; the implementation sticks nsresults in that out param)? > We were already doing this work, correct? Good question. We were _sometimes_ doing it. Specifically, we were doing it in cases when the node is in the document. I guess we don't cache the URI in other cases, so that might be ok. So yes, followup in bug 486806 is fine. > It was originally named SetHrefToURI Oh, I see. This isn't actually setting mCachedURI; it really is setting the href. In that case SetHref is better than SetURI; SetHrefAttribute might be even better, I think. > I can change this if you want though. Nah, leaving the qualifier there seems ok.

Addresses comment 73. The code and comments are all the same now (with style/grammar fixes).

Comment on attachment 415202 [details] [diff] [review] Part 9 - Move helper methods to Link v1.1 At least the truncate call in GetPort has gone AWOL. Please capture it and put it in its rightful place, and check to make sure none of the others escaped? sr=bzbarsky with that.

(In reply to comment #82) > (From update of attachment 415202 [details] [diff] [review]) > At least the truncate call in GetPort has gone AWOL. Please capture it and put > it in its rightful place, and check to make sure none of the others escaped? Bah, that was the only one. New patch with that fix coming up.

Unbitrot IHistory API and add missing files.

Comment on attachment 416802 [details] [diff] [review] Part 12 - All children of Link must call ResetLinkState in DestroyContent r- As discussed, I don't think DestroyContent is the right place to unregister. Doing it in the destructor is both more correct and more reliable. DestroyContent is there as a hack to fix leaks. I don't want to add to it.

Per conversation with sicking. This should work, but having bz sr to make sure this still fits our mental model of what's going on in this bug.

Needed to build this in more than just content/base. Move the destructor to protected: instead of private:

(In reply to comment #91) > Needed to build this in more than just content/base. Move the destructor to > protected: instead of private: Note to self: update places test that implements its own Link too.

This contains the changes to places test code for compiling to still work.

Comment on attachment 416803 [details] [diff] [review] Part 13 - Link::SetLinkState should notify document on change v1.0 You need to call BeginUpdate/EndUpdate as well. Preferably by using an mozAutoDocUpdate

use mozAutoDocUpdate with UPDATE_STYLE.

Comment on attachment 416943 [details] [diff] [review] Part 13 - Link::SetLinkState should notify document on change v1.1 >From: Shawn Wilsher <sdwilsh@shawnwilsher.com> > >Make Link::SetLinkState notify the document about changes in state. Also adds a >whole bunch of assertions to check our assumptions/invariants. > >diff --git a/content/base/src/Link.cpp b/content/base/src/Link.cpp >--- a/content/base/src/Link.cpp >+++ b/content/base/src/Link.cpp >@@ -46,6 +46,7 @@ > #include "nsEscape.h" > #include "nsGkAtoms.h" > #include "nsString.h" >+#include "mozAutoDocUpdate.h" > > #include "mozilla/IHistory.h" > >@@ -82,7 +83,22 @@ Link::SetLinkState(nsLinkState aState) > { > NS_ASSERTION(mRegistered, > "Setting the link state of an unregistered Link!"); >+ NS_ASSERTION(mLinkState != aState, >+ "Setting state to the currently set state!"); >+ >+ // Set our current state as appropriate. > mLinkState = aState; >+ >+ // Notify the document that our visited state has changed. >+ nsCOMPtr<nsIContent> content(do_QueryInterface(this)); >+ nsIDocument *doc = content->GetCurrentDoc(); >+ NS_ASSERTION(doc, "Registered but we have no document?!"); >+ PRInt32 state = LinkState(); >+ NS_ASSERTION(state == NS_EVENT_STATE_VISITED || >+ state == NS_EVENT_STATE_UNVISITED, >+ "Unexpected state obtained from LinkState()!"); >+ mozAutoDocUpdate update(doc, UPDATE_STYLE, PR_TRUE); You want to use UPDATE_CONTENT_STATE rather than UPDATE_STYLE >+ doc->ContentStatesChanged(content, nsnull, LinkState()); You want to use "state" rather than "LinkState()" r=me with those things fixed

Comment on attachment 416943 [details] [diff] [review] Part 13 - Link::SetLinkState should notify document on change v1.1 The bits passed to the document need to be the bits that changed. In particular, they need to be oldState ^ newState.

Comment on attachment 416801 [details] [diff] [review] Part 11 - Register with IHistory in Link::VisitedState v1.0 I'd prefer UnregisterFromHistory. r=bzbarsky with that.

Comment on attachment 416930 [details] [diff] [review] Part 12 - Link::~Link should call Link::UnregisterWithHistory v2.2 Are we guaranteed that if mRegistered then mCachedURI non-null? If so, can we just use it directly in UnregisterWithHistory? That would guarantee that we don't try to call subclass methods in the dtor here, which I think is a good thing. sr=me with that.

bz tells me this has bugs, but I should put it up here. Layout should call IntrinsicState instead of GetLinkState.

Comment on attachment 416995 [details] [diff] [review] Part 15 - Layout should call IntrinsicState instead of GetLinkState for style information v0.1 >+++ b/layout/style/nsCSSRuleProcessor.cpp > RuleProcessorData::IsLink() >+ return (state & NS_EVENT_STATE_VISITED) || >+ (state & NS_EVENT_STATE_UNVISITED); return (state & (NS_EVENT_STATE_VISITED || NS_EVENT_STATE_UNVISITED)) != 0; or so. That said, are there many IsLink() callers left? >@@ -1857,34 +1825,16 @@ static PRBool SelectorMatches(RuleProces Should probably change the PseudoClassInfo mBit member to be mBits. >@@ -2459,23 +2409,16 @@ PRBool IsStateSelector(nsCSSSelector& aS >- if (sIsStatePseudoClass[pseudoClass->mType] || That part needs to stay (and actually get merged to the stuff I actually ended up landing). You can remove all the :link/:visited code in nsCSSRuleProcessor::HasAttributeDependentStyle, right? >+++ b/layout/style/nsHTMLStyleSheet.cpp >+ switch (aData->ContentState()) { This is wrong, since other state bits can also be set. You want an if cascade here (and probably no longer need the IsLink() check).

As far as part 13.... who calls SetLinkState? It it only called from our history callback?

(In reply to comment #103) > As far as part 13.... who calls SetLinkState? It it only called from our > history callback? Yes, that is the only place. In two or so patches (depends if I want to split some up more), I'll be removing SetLinkState from nsIContent (along with a few other methods).

Rename UnregisterWithHistory to UnregisterFromHistory (per bz's review comment).

Per sr comments.

Per review comments.

(In reply to comment #102) > or so. That said, are there many IsLink() callers left? Only three, as far as I can tell. > You can remove all the :link/:visited code in > nsCSSRuleProcessor::HasAttributeDependentStyle, right? Yup, including the SVG bits! Yay code removal! > This is wrong, since other state bits can also be set. You want an if cascade > here (and probably no longer need the IsLink() check). Still need the IsLink check, but only for the active state. Refactored that code to make it look a lot simpler and avoid the IsLink check unless mActiveRule is true.

> Still need the IsLink check, but only for the active state. Ah, indeed.

Comment on attachment 417138 [details] [diff] [review] Part 15 - Layout should call IntrinsicState instead of GetLinkState for style information v1.0 >+++ b/layout/style/nsCSSRuleProcessor.cpp > RuleProcessorData::IsLink() >+ return (state & (NS_EVENT_STATE_VISITED || NS_EVENT_STATE_UNVISITED)) != 0; That should use bitwise or, not boolean or. (Sorry for my typo in comment 102...) > PRBool IsStateSelector(nsCSSSelector& aSelector) >+ if (sPseudoClassInfo[pseudoClass->mType].mBits || > pseudoClass->mAtom == nsCSSPseudoClasses::link || > pseudoClass->mAtom == nsCSSPseudoClasses::visited) { > return PR_TRUE; The :link/:visited checks here _can_ go away. It's just the mBits check that needs to stay. r=me with those changes.

> That should use bitwise or, not boolean or. (Sorry for my typo in comment > 102...) I noticed that, and still managed to copy it verbatim. Arg.

Pure code removal here.

API change so sr needed. This code is no longer used after Part 17.

Comment on attachment 417142 [details] [diff] [review] Part 15 - Layout should call IntrinsicState instead of GetLinkState for style information v1.1 You should document in nsCSSPseudoClassList.h and in the definition of struct PseudoClassInfo that when multiple bits are set, only one of them needs to match for the pseudo-class to match. Could you rename |stateToCheck| to |statesToCheck| as well? Also, please remove the now-unused functions mozAnyLinkMatches, linkMatches, and visitedMatches. I suppose this approach makes :-moz-any-link on its own a little slower; however, almost all links are matched against :link or :visited anyway, so it doesn't matter. This patch means we're ignoring gSupportVisitedPseudo. Is that pref handled in one of the other patches? (It seems like the way we'd want to implement it know is changing NS_EVENT_STATE_VISITED to NS_EVENT_STATE_LINK.) I think we should keep support for that pref. sr=dbaron with those things fixed