Closed Bug 461199 (async-visited-check) Opened 12 years ago Closed 10 years ago

Create an API for asynchronous isVisited checks that content/layout can use

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a2

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

(Depends on 2 open bugs)

Details

(Keywords: perf, Whiteboard: [tsnap])

Attachments

(31 files, 78 obsolete files)

15.38 KB, patch
Details | Diff | Splinter Review
43.50 KB, patch
Details | Diff | Splinter Review
2.45 KB, patch
Details | Diff | Splinter Review
4.73 KB, patch
Details | Diff | Splinter Review
1.51 KB, patch
Details | Diff | Splinter Review
850 bytes, patch
Details | Diff | Splinter Review
2.10 KB, patch
Details | Diff | Splinter Review
10.38 KB, patch
Details | Diff | Splinter Review
20.27 KB, patch
Details | Diff | Splinter Review
4.11 KB, patch
Details | Diff | Splinter Review
4.18 KB, patch
Details | Diff | Splinter Review
1.96 KB, patch
Details | Diff | Splinter Review
1.89 KB, patch
Details | Diff | Splinter Review
3.38 KB, patch
Details | Diff | Splinter Review
14.03 KB, patch
Details | Diff | Splinter Review
4.46 KB, patch
Details | Diff | Splinter Review
2.39 KB, patch
Details | Diff | Splinter Review
6.71 KB, patch
Details | Diff | Splinter Review
2.20 KB, patch
Details | Diff | Splinter Review
12.48 KB, patch
Details | Diff | Splinter Review
3.31 KB, patch
Details | Diff | Splinter Review
894 bytes, patch
Details | Diff | Splinter Review
5.52 KB, patch
Details | Diff | Splinter Review
3.93 KB, patch
Details | Diff | Splinter Review
2.63 KB, patch
Details | Diff | Splinter Review
1.88 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
2.24 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
10.93 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
4.19 KB, patch
sicking
: review+
Details | Diff | Splinter Review
8.69 KB, patch
Details | Diff | Splinter Review
1.15 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Attached file transcript (obsolete) —
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...
Duplicate of this bug: 511960
Assignee: nobody → sdwilsh
Depends on: 511962
Summary: Investigate adding an async IsVisited API → Create an API for asynchronous isVisited checks that content/layout can use
Target Milestone: Future → mozilla1.9.3a1
Depends on: 515494
Attached patch New API v1.0 (obsolete) — Splinter Review
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.
Attachment #399621 - Flags: superreview?(benjamin)
Attachment #399621 - Flags: review?(dietrich)
Attachment #399621 - Flags: review?(bzbarsky)
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.
Status: NEW → ASSIGNED
Attached patch v0.1 (obsolete) — Splinter Review
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.
Attachment #399621 - Flags: superreview?(benjamin)
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!
Attached patch New API v1.1 (obsolete) — Splinter Review
Tests are hard to write.  This is just a checkpoint so I don't lose a days worth of work.
Attachment #399621 - Attachment is obsolete: true
Attachment #399621 - Flags: review?(dietrich)
Attachment #399621 - Flags: review?(bzbarsky)
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?
Attached patch New API v1.2 (obsolete) — Splinter Review
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.
Attachment #399905 - Attachment is obsolete: true
> 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.
Blocks: 516728
Attached patch New API v1.3 (obsolete) — Splinter Review
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.
Attachment #400678 - Attachment is obsolete: true
Attachment #401522 - Flags: superreview?(benjamin)
Attachment #401522 - Flags: review?(mak77)
Attachment #401522 - Flags: review?(bzbarsky)
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
Attachment #401522 - Flags: review?(mak77) → review-
(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.
Depends on: 517604
Attached patch New API v1.4 (obsolete) — Splinter Review
Per review comments.
Attachment #401522 - Attachment is obsolete: true
Attachment #401562 - Flags: superreview?(benjamin)
Attachment #401562 - Flags: review?(mak77)
Attachment #401562 - Flags: review?(bzbarsky)
Attachment #401522 - Flags: superreview?(benjamin)
Attachment #401522 - Flags: review?(bzbarsky)
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.
Attachment #401562 - Flags: review?(mak77) → review+
No longer depends on: 511962
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
Attachment #401562 - Flags: superreview?(benjamin) → superreview+
Whiteboard: [tsnap]
(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.
Whiteboard: [tsnap]
Whiteboard: [tsnap]
Depends on: 209275
Attached patch New API v1.5 (obsolete) — Splinter Review
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.
Attachment #401562 - Attachment is obsolete: true
Attachment #402619 - Flags: review?(bzbarsky)
Attachment #401562 - Flags: review?(bzbarsky)
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.
Attached patch New API v1.6 (obsolete) — Splinter Review
Use nsTHashtable instead of nsClassHashtable.

I'll start work on using this API once it gets review.
Attachment #344329 - Attachment is obsolete: true
Attachment #402619 - Attachment is obsolete: true
Attachment #403622 - Flags: review?(bzbarsky)
Attachment #402619 - Flags: review?(bzbarsky)
Whiteboard: [tsnap] → [needs review bz][tsnap]
Keywords: perf
Attached patch Refactor v1.0 (obsolete) — Splinter 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.
Attachment #406091 - Flags: superreview?(jst)
Attachment #406091 - Flags: review?(jonas)
Whiteboard: [needs review bz][tsnap] → [needs review bz][needs review sicking][needs sr jst][tsnap]
Attached patch Refactor v1.1 (obsolete) — Splinter Review
I forgot to qrefresh before submitting that patch.
Attachment #406091 - Attachment is obsolete: true
Attachment #406092 - Flags: superreview?(jst)
Attachment #406092 - Flags: review?(jonas)
Attachment #406091 - Flags: superreview?(jst)
Attachment #406091 - Flags: review?(jonas)
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.
Attachment #406092 - Flags: review?(jonas) → review+
Whiteboard: [needs review bz][needs review sicking][needs sr jst][tsnap] → [needs review bz][needs sr jst][tsnap]
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.
Attachment #406092 - Flags: superreview?(jst) → superreview+
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.
Whiteboard: [needs review bz][needs sr jst][tsnap] → [needs review bz][tsnap]
> 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.
Attachment #406092 - Attachment is obsolete: true
Comment on attachment 410066 [details] [diff] [review]
Part 1 - Refactor v1.2 (checked in)

http://hg.mozilla.org/mozilla-central/rev/d89369248cbf
Attachment #410066 - Attachment description: Refactor v1.2 → Refactor v1.2 (checked in)
(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?
Attached patch New API v1.7 (obsolete) — Splinter Review
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).
Attachment #403622 - Attachment is obsolete: true
Attachment #410556 - Flags: review?(mak77)
Attachment #410556 - Flags: review?(bzbarsky)
Attachment #403622 - Flags: review?(bzbarsky)
Whiteboard: [needs review bz][tsnap] → [needs review bz][needs review mak][tsnap]
> 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.
Attachment #410646 - Flags: review?(jonas)
Attachment #399752 - Attachment is obsolete: true
Whiteboard: [needs review bz][needs review mak][tsnap] → [needs review bz][needs review mak][needs review sicking][tsnap]
Whiteboard: [needs review bz][needs review mak][needs review sicking][tsnap] → [needs review bz][needs review mak][tsnap]
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.
Attachment #410556 - Flags: review?(mak77) → review+
Whiteboard: [needs review bz][needs review mak][tsnap] → [needs review bz][tsnap]
(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.
Attached patch New API v1.8 (obsolete) — Splinter Review
Addresses mak's review comments.  All this needs now is bz's review.
Attachment #410556 - Attachment is obsolete: true
Attachment #411213 - Flags: review?(bzbarsky)
Attachment #410556 - Flags: review?(bzbarsky)
Attached patch New API v1.8 (obsolete) — Splinter Review
I uploaded the wrong patch...
Attachment #411213 - Attachment is obsolete: true
Attachment #411457 - Flags: review?(bzbarsky)
Attachment #411213 - Flags: review?(bzbarsky)
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.
Attachment #411457 - Flags: review?(bzbarsky) → review+
Whiteboard: [needs review bz][tsnap] → [tsnap]
Attached patch Part 2 - New API v1.9 (obsolete) — Splinter Review
Per review comments.  I added an @note documenting that SetLinkState cannot register or unregister, and added an assertion in History::NotifyVisited.
Attachment #411457 - Attachment is obsolete: true
Attachment #410066 - Attachment description: Refactor v1.2 (checked in) → Part 1 - Refactor v1.2 (checked in)
Attachment #414533 - Attachment description: New API v1.9 → Part 2 - New API v1.9
Attachment #410646 - Attachment description: Part 3 - add IHistory getting to nsContentUtils → Part 3 - add IHistory getter to nsContentUtils
Attachment #414548 - Flags: review? → review?(bzbarsky)
Adds Link::LinkState to be called by a subclass's IntrinsicState.
Attachment #414549 - Flags: superreview?(bzbarsky)
Attachment #414549 - Flags: review?(jonas)
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.
Attachment #414548 - Flags: review?(bzbarsky) → review-
nsSVGAElement needs to implement GetHrefURI so so Link can call that to cache the URI (Part 7).
Attachment #414550 - Flags: review?(jwatt)
Attachment #414548 - Attachment description: Part 4 - Add NS_EVENT_STATE_UNVISITED to nsIEventStateManager → Part 4 - Add NS_EVENT_STATE_UNVISITED to nsIEventStateManager v1.0
Attachment #414549 - Attachment description: Part 5 - Add Link::LinkState → Part 5 - Add Link::LinkState v1.0
Attachment #414550 - Attachment description: Part 6 - Implement nsSVGAElement::GetHrefURI → Part 6 - Implement nsSVGAElement::GetHrefURI v1.0
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
Attachment #414549 - Flags: superreview?(bzbarsky) → superreview+
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).
Attachment #414551 - Flags: review?(bzbarsky)
We no longer need eLazyURIValue.  When getting the URI, we should call Link::GetURI instead (or recompute).
Attachment #414553 - Flags: superreview?(bzbarsky)
Attachment #414553 - Flags: review?(jonas)
Comment on attachment 414551 [details] [diff] [review]
Part 7 - Link should cache its URI v1.0

Looks ok, but see comments on next patch.
Attachment #414551 - Flags: review?(bzbarsky) → review+
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?
Attachment #414553 - Flags: superreview?(bzbarsky) → superreview-
(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.
Attachment #414548 - Attachment is obsolete: true
Attachment #414560 - Flags: review?(bzbarsky)
Per review comments.
Attachment #414549 - Attachment is obsolete: true
Attachment #414561 - Flags: review?(jonas)
Attachment #414549 - Flags: review?(jonas)
Move helper methods on nsGenericHTMLElement to Link and use the cached URI.
Attachment #414562 - Flags: superreview?(bzbarsky)
Attachment #414562 - Flags: review?(jst)
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.
Attachment #414560 - Flags: review?(bzbarsky) → review+
Make sure we call Link::ResetLinkCache in Bind and Unbind
Attachment #414563 - Flags: review?(jwatt)
Attachment #414563 - Flags: review?(bzbarsky)
I somehow managed to miss a few in the last patch, so I'm asking for review for a sanity check.
Attachment #414560 - Attachment is obsolete: true
Attachment #414583 - Flags: review?(bzbarsky)
Attachment #414583 - Flags: review?(bzbarsky) → review+
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?
Attachment #414563 - Flags: review?(bzbarsky) → review+
Attachment #414550 - Flags: review?(jwatt) → review+
Attachment #414563 - Flags: review?(jwatt) → review+
(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.
Attachment #414553 - Flags: superreview- → superreview?(bzbarsky)
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.
Attachment #414553 - Flags: superreview?(bzbarsky) → superreview+
Addresses comment 73.  The code and comments are all the same now (with style/grammar fixes).
Attachment #414562 - Attachment is obsolete: true
Attachment #415202 - Flags: superreview?(bzbarsky)
Attachment #415202 - Flags: review?(jst)
Attachment #414562 - Flags: superreview?(bzbarsky)
Attachment #414562 - Flags: review?(jst)
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.
Attachment #415202 - Flags: superreview?(bzbarsky) → superreview+
(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.
Attachment #415202 - Attachment is obsolete: true
Attachment #415405 - Flags: review?(jst)
Attachment #415202 - Flags: review?(jst)
Attached patch Part 2 - New IHistory API v1.10 (obsolete) — Splinter Review
Unbitrot IHistory API and add missing files.
Attachment #414533 - Attachment is obsolete: true
Attachment #416801 - Flags: review?(bzbarsky)
Attachment #416802 - Flags: superreview?(bzbarsky)
Attachment #416802 - Flags: review?(jonas)
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.
Attachment #416802 - Flags: superreview?(bzbarsky)
Attachment #416802 - Flags: review?(jonas)
Attachment #416802 - Flags: review-
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.
Attachment #416802 - Attachment is obsolete: true
Attachment #416835 - Flags: superreview?(bzbarsky)
Attachment #416835 - Flags: review?(jonas)
Needed to build this in more than just content/base.  Move the destructor to protected: instead of private:
Attachment #416835 - Attachment is obsolete: true
Attachment #416848 - Flags: superreview?(bzbarsky)
Attachment #416848 - Flags: review?(jonas)
Attachment #416835 - Flags: superreview?(bzbarsky)
Attachment #416835 - Flags: review?(jonas)
(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.
Attachment #416803 - Flags: review?(bzbarsky) → review?(jonas)
This contains the changes to places test code for compiling to still work.
Attachment #416848 - Attachment is obsolete: true
Attachment #416930 - Flags: superreview?(bzbarsky)
Attachment #416848 - Flags: superreview?(bzbarsky)
Attachment #416801 - Attachment description: Part 11 - Register with IHistory in Link::VisitedState → Part 11 - Register with IHistory in Link::VisitedState v1.0
Attachment #416803 - Attachment description: Part 13 - Link::SetLinkState should notify document on change → Part 13 - Link::SetLinkState should notify document on change v1.0
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
Attachment #416803 - Flags: review?(jonas) → review-
use mozAutoDocUpdate with UPDATE_STYLE.
Attachment #416803 - Attachment is obsolete: true
Attachment #416943 - Flags: review?(jonas)
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
Attachment #416943 - Flags: review?(jonas) → review+
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.
Attachment #416943 - Flags: review-
Attachment #416801 - Flags: review?(bzbarsky) → review+
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.
Attachment #416930 - Flags: superreview?(bzbarsky) → superreview+
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.
Attachment #416995 - Flags: review?(bzbarsky)
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).
Attachment #416995 - Flags: review?(bzbarsky) → review-
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).
Attachment #416801 - Attachment is obsolete: true
Per sr comments.
Attachment #416930 - Attachment is obsolete: true
Per review comments.
Attachment #416943 - Attachment is obsolete: true
Attachment #417130 - Flags: review?(bzbarsky)
(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.
Attachment #416995 - Attachment is obsolete: true
Attachment #417138 - Flags: superreview?(dbaron)
Attachment #417138 - Flags: review?(bzbarsky)
> Still need the IsLink check, but only for the active state.

Ah, indeed.
Attachment #417130 - Flags: review?(bzbarsky) → review+
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.
Attachment #417138 - Flags: review?(bzbarsky) → review+
> 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.
Attachment #417138 - Attachment is obsolete: true
Attachment #417142 - Flags: superreview?(dbaron)
Attachment #417138 - Flags: superreview?(dbaron)
Pure code removal here.
Attachment #417155 - Flags: superreview?(dbaron)
Attachment #417155 - Flags: review?(bzbarsky)
API change so sr needed.  This code is no longer used after Part 17.
Attachment #417157 - Flags: superreview?(jonas)
Attachment #417157 - Flags: review?(bzbarsky)
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
Attachment #417142 - Flags: superreview?(dbaron) → superreview+
Attachment #417155 - Flags: superreview?(dbaron) → superreview+
(In reply to comment #115)
> 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.
I think instead of matching unvisited nodes in that case, it makes sense to match no nodes.  This is the behavior of this patch.  Asking for bz's review again because I'm not sure he'll like the solution I have for checking the preference.
Attachment #417142 - Attachment is obsolete: true
Attachment #417161 - Flags: review?(bzbarsky)
Er, that patch is not right.  New one coming.
Actually, supporting the preference is hard.  The patch tried to modify the macro, but that just sets the value in a static global array, so it won't update as the value changes (and would always be true).  There's no way to support the pref without treating :link and :visited special again.  I'm open to ideas.
How about changing RuleProcessorData::ContentState to contain:

if (!gSupportVisitedLinks & (mContentState & NS_EVENT_STATE_VISITED)) {
  // change the visited state to unvisited
  mContentState = (mContentState & ~PRUint32(NS_EVENT_STATE_VISITED)) |
                  NS_EVENT_STATE_UNVISITED;
}
Oops, the first & should have been &&.
Comment on attachment 417155 [details] [diff] [review]
Part 16 - Remove now unused link testin code in nsStyleUtils v1.0

Looks fine; you can also nuke GetLinkState on nsILinkHandler and its impl in docshell, right?  Possibly as a followup bug?
Attachment #417155 - Flags: review?(bzbarsky) → review-
Comment on attachment 417157 [details] [diff] [review]
Part 17 - Remove unused code from webshell and docshell v1.0

Ah, here we go.  Excellent.  ;)
Attachment #417157 - Flags: review?(bzbarsky) → review+
Attachment #417157 - Flags: superreview?(jonas) → superreview+
Attachment #417155 - Flags: review- → review+
Comment on attachment 417161 [details] [diff] [review]
Part 15 - Layout should call IntrinsicState instead of GetLinkState for style information v1.2

I'd like to see a version of this that does comment 119.
Attachment #417161 - Flags: review?(bzbarsky) → review-
Part 2 is presently broken (test does not pass) at least on windows.  I'm working on fixing it.
Attached patch Part 2 - New IHistory API v1.11 (obsolete) — Splinter Review
This actually builds and passes tests on windows.  Had a NS_RELEASE_THIS before I used some member variables, which caused all sorts of fun.
Attachment #416169 - Attachment is obsolete: true
per comments
Attachment #417161 - Attachment is obsolete: true
Attachment #417756 - Flags: superreview?(dbaron)
Attachment #417756 - Flags: review?(bzbarsky)
Turns out, calling a virtual method that is only implemented in a subclass in the base classes constructor is a bad idea.  This was crashing on windows (but not os x!).  Removes the assertion in the constructor.
Attachment #414551 - Attachment is obsolete: true
Comment on attachment 417756 [details] [diff] [review]
Part 15 - Layout should call IntrinsicState instead of GetLinkState for style information v1.3

Move the |if (!gSupportVisitedPseudo| block into the !mGotContentState block, and looks good.
Attachment #417756 - Flags: review?(bzbarsky) → review+
Adding the assertion in this code to make sure we have an nsIContent (since I'm not doing it in the constructor anymore).
Attachment #417762 - Flags: review?(jst)
Adding the assertion in this code to make sure we have an nsIContent (since I'm
not doing it in the constructor anymore).
Attachment #417130 - Attachment is obsolete: true
One additional change to part 15.  We no longer need to observe NS_LINK_VISITED_EVENT_TOPIC.
Attachment #417792 - Flags: superreview?(dbaron)
Comment on attachment 417756 [details] [diff] [review]
Part 15 - Layout should call IntrinsicState instead of GetLinkState for style information v1.3

Did you mean to mark this patch obsolete?
Attachment #417756 - Flags: superreview?(dbaron)
Comment on attachment 417792 [details] [diff] [review]
Part 15 - Layout should call IntrinsicState instead of GetLinkState for style information v1.4

>+    // If we are not supposed to mark visited links as such, be sure to flip the

Could you wrap at slightly under 80 characters?  (I tend to wrap at 72.)

sr=dbaron
Attachment #417792 - Flags: superreview?(dbaron) → superreview+
(In reply to comment #133)
> (From update of attachment 417756 [details] [diff] [review])
> Did you mean to mark this patch obsolete?
sure did - you sr'd the patch that replaced it.
Attachment #416931 - Flags: superreview?(jst) → superreview?(bzbarsky)
Attachment #415405 - Flags: review?(jst) → review?(jonas)
Attachment #415405 - Attachment is obsolete: true
Attachment #415405 - Flags: review?(jonas)
Attachment #417762 - Flags: review?(jst) → review?(jonas)
Comment on attachment 417762 [details] [diff] [review]
Part 9 - Move helper methods to Link v1.3

Can you comment on which, if any, parts of this patch isn't simply moving code from nsGenericHTMLElement to nsLink?
Attachment #416931 - Flags: superreview?(bzbarsky) → superreview+
(In reply to comment #100)
> (From update of attachment 416930 [details] [diff] [review])
> 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.
I'm pretty sure we can guarantee that, but I'm getting a crash right now because this isn't true in some tests.  Tracking it down.
Also, Link::GetURIToMutate was returning nsnull if mCachedURI was nsnull.  However, it looks like the old code would always tried to get the URI, if possible.  I'm changing this code to call GetURI (which will return the cached URI if it is available), and then clone it.  We need to do this since it's possible for someone to create a node, not yet insert it in the document, and then try to modify the href (node.href = "someurl").  We would previously return nsnull, whereas now we'll get the href attribute on the node, and then clone it as opposed to strictly clonding mCachedURI.
Fix a crash during node destruction.  We ended up in a state where mRegistered was true, but mCachedURI was null because we ended up registering a node that was not in the document.  The fix is to assume that nodes not in the document are not visited.
Attachment #417123 - Attachment is obsolete: true
Attachment #421347 - Flags: review?(bzbarsky)
Attachment #421347 - Flags: review?(bzbarsky) → review+
Adds an API that dispatches an observer notification when the lookup completes.  This will help in writing/updating test cases to this new API.
Attachment #421542 - Flags: review?(mak77)
Alias: async-visited-check
Attached patch Part 2 - New IHistory API v1.12 (obsolete) — Splinter Review
Updated some formatting and bit rot with this patch.  Does not need to be reviewed again.
Comment on attachment 421542 [details] [diff] [review]
Part 18 - Notification about lookup completion v1.0

out of curiosity, any reason for using "not visited" instead of "unvisited"?

>diff --git a/toolkit/components/places/src/History.cpp b/toolkit/components/places/src/History.cpp
>+      const PRUnichar* status = mIsVisited ?
>+        NS_LITERAL_STRING(URI_VISITED).get() :
>+        NS_LITERAL_STRING(URI_NOT_VISITED).get();

nit: you could include NS_LITERAL_STRING().get() in the define and make this more readable, they are used just here after all

>diff --git a/toolkit/components/places/tests/cpp/test_IHistory.cpp b/toolkit/components/places/tests/cpp/test_IHistory.cpp

>+namespace test_observer_topic_dispatched_helpers {
>+  #define URI_VISITED "visited"
>+  #define URI_NOT_VISITED "not visited"
>+  #define URI_VISITED_RESOLUTION_TOPIC "visited-status-resolution"
>+  class statusObserver : public nsIObserver

nit: nl after defines would help readability

btw, looks good.
Attachment #421542 - Flags: review?(mak77) → review+
> nit: you could include NS_LITERAL_STRING().get() in the define and make this
> more readable, they are used just here after all

Er... the code quoted right above that is unsafe.  It just shouldn't exist to start with.  In particular, on platforms where NS_LITERAL_STRING creates a temporary object |status| will point at the memory from inside a now-destroyed object.
Er... yes.  Please file a bug on that!  That one is particularly silly, since it should just use PRUnichar(',') and be done with it.
filed bug 539710, i'll try to check if we have other cases around in the codebase.
(In reply to comment #143)
> Er... the code quoted right above that is unsafe.  It just shouldn't exist to
> start with.  In particular, on platforms where NS_LITERAL_STRING creates a
> temporary object |status| will point at the memory from inside a now-destroyed
> object.
Bah, I know better.  Fixed locally.
(In reply to comment #42)
> > 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).
Hey look, I think I was wrong.  The link-visited topic should probably be dispatched *after* we've notified the nodes.  At least, for testing purposes, that means that once the observer topic has been dispatched, we know we can test link nodes for that URI for their visitedness.
(In reply to comment #148)
> Hey look, I think I was wrong.  The link-visited topic should probably be
> dispatched *after* we've notified the nodes.  At least, for testing purposes,
> that means that once the observer topic has been dispatched, we know we can
> test link nodes for that URI for their visitedness.
Actually, no.  docshell notifies before addVisit ever gets called, so we need to keep the ordering in the current patches.  *sigh*
Attached patch Fix docshell tests v1.0 (obsolete) — Splinter Review
This is a wdiff of my changes to fix the only docshell test impacted by this change.  I also removed a bunch of trailing whitespace, so the actually diff looks more complicated.
Attachment #421684 - Flags: review?(bzbarsky)
Attachment #421684 - Attachment description: Fix docshell tests → Fix docshell tests v1.0
I found that we were not calling Link::ResetLinkState as we should have been in nsSVGAElement.
Attachment #421711 - Flags: superreview?(bzbarsky)
Attachment #421711 - Flags: review?(jwatt)
ehsan noticed a typo in the test.
Attachment #421684 - Attachment is obsolete: true
Attachment #421712 - Flags: review?(bzbarsky)
Attachment #421684 - Flags: review?(bzbarsky)
So, an interesting problem has come up with my investigating why pseudo-classes-02.svg was failing.  The svg:athat were failing contained an empty href attribute:
 <a xlink:href="">
However, we have code in Link::LinkState that discards elements that have no href as not a link due to bug 23209.

The spec has this to say about :link:
The document language determines which elements are hyperlink source anchors. For example, in HTML4, the link pseudo-classes apply to A elements with an "href" attribute

I couldn't find anything in the SVG spec (granted, I've never looked at it before, so I could have missed something), however, bug 268135 indicates that all svg:a elements are to be styled as :link (unless they are :visited).

So, I could modify part 5 to only perform the empty href test if content->IsHTML returns true or not.
Per comment 153, which fixes pseudo-classes-02.svg locally.  Sorry bz, but I'm going to have you look at this patch again (at least it should be getting very familiar...).  Pushing to the try server with today's work as well to see what progress was made overall.  ehsan is also working on the private browsing test that this broke.
Attachment #421347 - Attachment is obsolete: true
Attachment #421720 - Flags: review?(bzbarsky)
Wait.  href="" and "no href attribute" are not the same thing at all.  The latter should be "not a link" (at least for HTML); the former is a link (pointing to the current page).  

> however, bug 268135 indicates that all svg:a elements are to be styled as :link

Uh... where, exactly?
And which of the multiple diffs in this bug contains the code in LinkState mentioned in comment 153?
Comment on attachment 421712 [details] [diff] [review]
Part 24 - Fix docshell tests v1.1

>+++ mozilla-central.21b1de231c4b\docshell\test\chrome\bug293235_window.xul	2010-01-14 15:22:10 -0800
>@@ -9,16 +9,19 @@
>+      // Because of LAZY_ADD, we will not be notified for three seconds
>+      // Wait for four seconds just to be safe.
>+      setTimeout(nextTest, 4000);

This is setting off alarm bells.  Why do we think that this timeout will definitely fire after the other one?  What if someone changes the other one?  What's this LAZY_ADD thing anyway?
Attachment #421711 - Flags: superreview?(bzbarsky) → superreview+
(In reply to comment #155)
> Wait.  href="" and "no href attribute" are not the same thing at all.  The
> latter should be "not a link" (at least for HTML); the former is a link
> (pointing to the current page).  


> Uh... where, exactly?
I thought comment 23 indicated that, and then comment 34 landed tests that also indicated that.  But then there is no <svg:a /> element in the test, so maybe I'm wrong.

(In reply to comment #156)
> And which of the multiple diffs in this bug contains the code in LinkState
> mentioned in comment 153?
attachment 421347 [details] [diff] [review] (Part 11 v1.2).  It is supposed to be doing exactly what this code was doing here:
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleUtil.cpp#442
GetHrefURI would call nsGenericHTMLElement::GetHrefURIForAnchors, which ends up calling nsGenericHTMLElement::GetURIAttr, which gets the lazy URI value or tries to resolve everything:
http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsGenericHTMLElement.cpp#2141

The current code calls Link::GetURI, which calls nsIContent::GetHrefURI, which calls nsGenericHTMLElement::GetHrefURIForAnchors, which calls nsGenericHTMLElement::GetURIAttr, which has all the same logic as before, so the behavior is identical (*phew*).

For SVG, nsIContent::GetHrefURI ends up calling nsSVGAElement::IsLink to get the URI.  Upon closer inspection, that looks correct too, so I'm not sure why pseudo-classes-02.svg was failing anymore.
(In reply to comment #157)
> This is setting off alarm bells.  Why do we think that this timeout will
> definitely fire after the other one?  What if someone changes the other one? 
> What's this LAZY_ADD thing anyway?
Yup, and it should.  There's no way to really work around the LAZY_ADD issue, short of using a timeout.  I should make it double the value though, like we do in other tests.  The places team controls that value, and we know if we change it, we have to update a bunch of tests too.  FWIW, it has not changed since it was first added.


It's a crappy hack that was added before Firefox 3.0 to move adding visits, which involve a bunch of database reads and writes, away from page load (it was a workaround to a page load regression that places caused when it was enabled as I understand it).  It was before my time, so I don't know all the details though.  We plan on getting rid of it (tracked in bug 499828), but we really need bug 539706 before we could even consider doing so.
> so I'm not sure why pseudo-classes-02.svg was failing anymore

OK.  I'm not either; fully agreed with the rest of comment 158.

Which part of the test fails?
Per discussion on irc, it's failing because the two bits that are failing are looking for visited styling, so we need to wait for it to happen (sadfaces).  I need to fix the test, so the code is right.  Part 11 v1.2 is good, and already has review.
Attachment #421720 - Attachment is obsolete: true
Attachment #421720 - Flags: review?(bzbarsky)
Attachment #421347 - Attachment is obsolete: false
Part 19: "kNameSpaceID_None == kNameSpaceID_XLink" doesn't look right.
(In reply to comment #159)
> (In reply to comment #157)
> The places team controls that value, and we know if we change
> it, we have to update a bunch of tests too.  FWIW, it has not changed since it
> was first added.

afaict, if you reduce it (like to 2s instead of 3s), there won't be problems with other tests.
Attachment #421711 - Flags: review?(jwatt) → review+
> Part 19: "kNameSpaceID_None == kNameSpaceID_XLink" doesn't look right.

Er... yes.  That needs fixing.
I've fixed that namespace blunder locally.  Not sure what I was thinking there...

After fixing all the unit tests, there are still two things that need to done with this bug:
1) Remove this code (http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#7383) and unhook the presshell from listening to the "link-visited" topic.  The IHistory implementation handles this now.
2) fix Link::SetLinkState to call ResetLinkState when we are passed eLinkState_Unknown
Attached patch Part 27 - Fix layout tests v1.0 (obsolete) — Splinter Review
This is the method that David and I had talked about before.  I could probably rewrite it to be more like the next patch if we don't want to use the thread manager.
Attachment #421905 - Flags: review?(dbaron)
Attached patch Part 25 - Fix SVG tests v1.0 (obsolete) — Splinter Review
Disable the one test (bz tells me it is bug 534526), and fix the other to work with the new behavior.
Attachment #421914 - Flags: review?(jwatt)
This patch ensures that the visited link coloring is turned off in private browsing mode, and rewrites the respective test to be compatible with the new async API.
Attachment #422142 - Flags: review?(sdwilsh)
Attachment #422142 - Flags: review?(bzbarsky)
Comment on attachment 422142 [details] [diff] [review]
Part 20 - Turn off visited link coloring in private browsing mode

I don't think this is acceptable from a performance standpoint.  I would prefer either the private browsing stuff setting this pref, or this code having and observer that updates a boolean so that the cost is only paid on transitions in/out of private browsing, or something.

In any case, this doesn't seem related to this bug directly does it?
Attachment #422142 - Flags: review?(bzbarsky) → review-
(In reply to comment #169)
> (From update of attachment 422142 [details] [diff] [review])
> I don't think this is acceptable from a performance standpoint.  I would prefer
> either the private browsing stuff setting this pref, or this code having and
> observer that updates a boolean so that the cost is only paid on transitions
> in/out of private browsing, or something.

If the former approach is acceptable, then I would prefer that as well.  I'll submit a patch with that approach in a bit.

> In any case, this doesn't seem related to this bug directly does it?

It is, in the sense that the current patch set will break the visited link coloring test in PB mode, and hence cannot land on mozilla-central!  :-)
(In reply to comment #169)
> (From update of attachment 422142 [details] [diff] [review])
> I don't think this is acceptable from a performance standpoint.  I would prefer
> either the private browsing stuff setting this pref, or this code having and
> observer that updates a boolean so that the cost is only paid on transitions
> in/out of private browsing, or something.
This is simple enough to do like gSupportVisitedPseudo and use an observer, which we can do.

> In any case, this doesn't seem related to this bug directly does it?
Yes.  I asked ehsan to help me out here because I didn't fully understand what the private browsing test was trying to do, and he had recently wrote it.  As of right now, this code in this bug does not handle private browsing properly (and we fail tests, which he fixed).
This patch manipulates the visited link coloring from inside the private browsing service.
Attachment #422142 - Attachment is obsolete: true
Attachment #422143 - Flags: review?(sdwilsh)
Attachment #422143 - Flags: review?(mconnor)
Attachment #422142 - Flags: review?(sdwilsh)
You probably want to check with dbaron whether he's ok with messing with the pref like this, but other than that looks ok to me.
Comment on attachment 422143 [details] [diff] [review]
Part 20 - Turn off visited link coloring in private browsing mode

Requesting dbaron's review as per comment 173.
Attachment #422143 - Flags: review?(dbaron)
Comment on attachment 422143 [details] [diff] [review]
Part 20 - Turn off visited link coloring in private browsing mode

r=me on the /browser bits, since I assume that's what's being asked of me here!
Attachment #422143 - Flags: review?(mconnor) → review+
Comment on attachment 421914 [details] [diff] [review]
Part 25 - Fix SVG tests v1.0

>+fails == dynamic-link-style-01.svg pass.svg

Make that:

>+fails == dynamic-link-style-01.svg pass.svg # https://bugzilla.mozilla.org/show_bug.cgi?id=461199#c167

please. With that, r=jwatt.
Attachment #421914 - Flags: review?(jwatt) → review+
Comment on attachment 422143 [details] [diff] [review]
Part 20 - Turn off visited link coloring in private browsing mode

>+const VISITED_LINK_COLORING_PREF = "layout.css.visited_links_enabled";
In a previous bug (don't recall number offhand), you had told me that changing the preference would be bad because that leaves a trace on disk that the user was in private browsing mode.  What is wrong with taking attachment 422142 [details] [diff] [review], adding gInPrivateBrowsing, and listening for NS_PRIVATE_BROWSING_SWITCH_TOPIC just like we do with gSupportVisitedPseudo?
(In reply to comment #176)
> please. With that, r=jwatt.
Fixed locally.
This patch implements the behavior suggested in comment 117.  It doesn't have the performance implications of the previous version of the patch.
Attachment #422426 - Flags: review?(bzbarsky)
Attachment #422143 - Attachment is obsolete: true
Attachment #422143 - Flags: review?(sdwilsh)
Attachment #422143 - Flags: review?(dbaron)
Attachment #422426 - Flags: review?(sdwilsh)
The previous patch missed a hunk :/
Attachment #422426 - Attachment is obsolete: true
Attachment #422429 - Flags: review?(sdwilsh)
Attachment #422429 - Flags: review?(bzbarsky)
Attachment #422426 - Flags: review?(sdwilsh)
Attachment #422426 - Flags: review?(bzbarsky)
Comment on attachment 422429 [details] [diff] [review]
Part 20 - Turn off visited link coloring in private browsing mode

>+++ b/layout/style/nsCSSRuleProcessor.cpp
>+  gPrivateBrowsingObserver = new nsPrivateBrowsingObserver();

Need to addref this.  I'm surprised this didn't crash on you....

>+nsCSSRuleProcessor::Shutdown()
>+{
>+  FreeSystemMetrics();
>+  delete gPrivateBrowsingObserver;

And this should just be a NS_RELEASE, not a delete.  Someone might still be holding a ref to it.

> +        (gPrivateBrowsingObserver && gPrivateBrowsingObserver->InPrivateBrowsing())) &&

Why is the null-check needed?
Attachment #422429 - Flags: review?(bzbarsky) → review-
Comment on attachment 422429 [details] [diff] [review]
Part 20 - Turn off visited link coloring in private browsing mode

>+    setTimeout(handleLoad, 4000);
You should set this to LAZY_ADD_TIMER * 2 (and define that constant) like we do in other tests.

r=sdwilsh on the places changes
Attachment #422429 - Flags: review?(sdwilsh) → review+
(In reply to comment #181)
> (From update of attachment 422429 [details] [diff] [review])
> > +        (gPrivateBrowsingObserver && gPrivateBrowsingObserver->InPrivateBrowsing())) &&
> 
> Why is the null-check needed?

In case the memory allocation for the object has failed for some reason...

Addressed the rest of the review comments.
Attachment #422429 - Attachment is obsolete: true
Attachment #422452 - Flags: review?(bzbarsky)
> In case the memory allocation for the object has failed for some reason...

Is there a reason to not detect that at allocation time and just fail module init instead of tossing this check into performance-sensitive code?
Depends on: 540896
From http://groups.google.com/group/mozilla.dev.tech.layout/msg/0159f3cb91c58ffd:
The other issue I ran into is that IntrinsicState() can actually be
somewhat expensive if called repeatedly without caching the result (e.g.
the QI+addref+release that Link::LinkState does accounts for about 10%
of the time on reframe of the HTML5 page with the patches in my tree).
Attachment #422586 - Flags: review?(bzbarsky)
Attachment #422586 - Flags: review?(bzbarsky) → review+
Attachment #421712 - Flags: review?(bzbarsky) → review+
Per comment 165, make Link::SetLinkState call Link::ResetLinkState when passed eLinkState_Unknown.
Attachment #422591 - Flags: review?(bzbarsky)
(In reply to comment #184)
> > In case the memory allocation for the object has failed for some reason...
> 
> Is there a reason to not detect that at allocation time and just fail module
> init instead of tossing this check into performance-sensitive code?

You're right.  This new patch does that.
Attachment #422452 - Attachment is obsolete: true
Attachment #422597 - Flags: review?(bzbarsky)
Attachment #422452 - Flags: review?(bzbarsky)
Comment on attachment 422591 [details] [diff] [review]
Part 22 - Reset when given an unknown state v1.0

The callers notify themselves, right?
Attachment #422591 - Flags: review?(bzbarsky) → review+
Attachment #422597 - Flags: review?(bzbarsky) → review+
(In reply to comment #188)
> (From update of attachment 422591 [details] [diff] [review])
> The callers notify themselves, right?
The only current caller is http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#7468, and it does not want to notify.
Attached patch Part 26 - Fix content tests v1.0 (obsolete) — Splinter Review
Finally got the content tests passing on the try server today.
Attachment #422682 - Flags: review?(bzbarsky)
Update: Still need to do point (1) from comment 165, and answer Jonas (comment 136).  Then, I'll land on the Places branch (blocked on bug 540896) to see perf numbers and make sure unit tests pass on mozilla-central's configuration.
Attachment #422682 - Flags: review?(bzbarsky) → review+
(In reply to comment #136)
> (From update of attachment 417762 [details] [diff] [review])
> Can you comment on which, if any, parts of this patch isn't simply moving code
> from nsGenericHTMLElement to nsLink?
There is one bug I found in Link::GetSearch, which I'll be uploading a new patch for in just a moment.  Otherwise, the only difference is with these two methods:
Link::GetURIToMutate()
Link::SetHrefAttribute
Attachment #417762 - Attachment is obsolete: true
Attachment #423032 - Flags: review?(jonas)
Attachment #417762 - Flags: review?(jonas)
Do note that attachment 423103 [details] is a high-level overview of what goes on between Link and IHistory for those interested (in graphical form for ease of understanding).
So, the try server was very unhappy with all these patches crashing in nsCSSRuleProcessor::Startup or nsCSSRuleProcessor::Shutdown or somewhere randomly in layout code (that didn't used to).  I didn't run this locally, but it's on all three platforms:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1264203669.1264211739.18626.gz&fulltext=1
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1264203666.1264209580.26352.gz&fulltext=1
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1264203666.1264214750.19809.gz&fulltext=1

Any ideas ehsan?  Your patch is the only thing that touched that function.
Non-crashy version of part 20 to address comment 195.
Attachment #422597 - Attachment is obsolete: true
Attachment #423412 - Flags: review?(bzbarsky)
(In reply to comment #165)
> 1) Remove this code
> (http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#7383)
> and unhook the presshell from listening to the "link-visited" topic.  The
> IHistory implementation handles this now.
nsPresShell code was removed in Part 15.  I'll kill the dead code in nsDocument (which that link no longer points to) in Part 23.
So this is it as far as I can tell.
Attachment #423429 - Flags: review?(bzbarsky)
Attachment #423412 - Flags: review?(bzbarsky) → review+
Comment on attachment 423429 [details] [diff] [review]
Part 23 - Remove dead code in nsDocument v1.0

This looks ok; is there a followup bug to reevaluate mLinkMap in light of this change?
Attachment #423429 - Flags: review?(bzbarsky) → review+
(In reply to comment #199)
> (From update of attachment 423429 [details] [diff] [review])
> This looks ok; is there a followup bug to reevaluate mLinkMap in light of this
> change?
I think we need it still for base URI changes.  There are a few optimizations I think we could make though:
1) change the link map to hold mozilla::dom::Link objects
2) have Link::LinkState add to the document map
3) have DNS prefetch use Link and then call GetURI (so we used the cached URI).
attachment 423429 [details] [diff] [review] (part 23) fails in a full build.  bah.

So,  nsDocument::OnPageShow calls UpdateLinkMap.  I know bz and I talked about this, but that was over a week ago and I can't recall why that was needed.  Anybody know if it is still needed, or can it go?  If it can't go, we can't do part 22 and that code isn't actually dead.  If it can, I need to update that patch to also remove those bits.
> I think we need it still for base URI changes.

Sure, but it's not obvious that it's needed in its current form (i.e. a hashtable keyed by URI, etc) for that.

> I know bz and I talked about this, but that was over a week ago and I can't
> recall why that was needed. 

Because notifications on links were queued up (see NotifyURIVisitednessChanged) while in bfcache.  Sounds like you should remove that call and the mVisitednessChangedURIs member, right?  Because your new code will just notify on the links in bfcache and rely on bug 540531 to not do too much work?
(In reply to comment #202)
> Sure, but it's not obvious that it's needed in its current form (i.e. a
> hashtable keyed by URI, etc) for that.
Yes, let's do that in a follow-up, along with some of the other things I commented on in comment 200 (yikes, 200 comments in this bug).

> Because notifications on links were queued up (see NotifyURIVisitednessChanged)
> while in bfcache.  Sounds like you should remove that call and the
> mVisitednessChangedURIs member, right?  Because your new code will just notify
> on the links in bfcache and rely on bug 540531 to not do too much work?
Ah, right.  It's all coming back to me now.  I'll have a patch up for that first thing tomorrow morning (or maybe even tonight).
Depends on: 540531
Part 23 for real this time.  This one builds!
Attachment #423562 - Flags: review?(bzbarsky)
Attachment #423429 - Attachment description: Part 23 - Remove dead code in nsDocument → Part 23 - Remove dead code in nsDocument v1.0
Attachment #423562 - Flags: review?(bzbarsky) → review+
No longer depends on: 515494
And in case anybody wants to try a build with these changes:
https://build.mozilla.org/tryserver-builds/sdwilsh@shawnwilsher.com-try-1cb71ad2cb5c/
Attachment #421905 - Attachment description: Fix layout tests v1.0 → Part 24 - Fix layout tests v1.0
Attachment #421905 - Attachment description: Part 24 - Fix layout tests v1.0 → Part 27 - Fix layout tests v1.0
Attachment #421712 - Attachment description: Fix docshell tests v1.1 → Part 24 - Fix docshell tests v1.1
Attachment #421914 - Attachment description: Fix SVG tests v1.0 → Part 25 - Fix SVG tests v1.0
Attachment #422682 - Attachment description: Fix content tests v1.0 → Part 26 - Fix content tests v1.0
Some of this code has bit rotted quite a bit since I last uploaded it, so I'm going to attach new patches for everything.  Sorry about the next 26 e-mails...
r=mak, r=bz, sr=bsmedberg
Attachment #421549 - Attachment is obsolete: true
Attachment #410646 - Attachment description: Part 3 - add IHistory getter to nsContentUtils → Part 3 - add IHistory getter to nsContentUtils v1.0
r=sicking
Attachment #410646 - Attachment is obsolete: true
r=bz
Attachment #414583 - Attachment is obsolete: true
Attachment #417723 - Attachment is obsolete: true
r=sicking, sr=bz
r=jwatt
Attachment #414550 - Attachment is obsolete: true
Attachment #414561 - Attachment is obsolete: true
r=bz
Attachment #417761 - Attachment is obsolete: true
r=sicking, sr=bz
Attachment #414553 - Attachment is obsolete: true
r=sicking, sr=bz
Attachment #423032 - Attachment is obsolete: true
Attachment #417155 - Attachment description: Part 16 - Remove now unused link testin code in nsStyleUtils → Part 16 - Remove now unused link testin code in nsStyleUtils v1.0
r=bz, sr=sicking
Attachment #417157 - Attachment is obsolete: true
Attachment #421542 - Attachment description: Part 18 - Notification about lookup completion → Part 18 - Notification about lookup completion v1.0
Attachment #421711 - Attachment description: Part 19 - nsSVGAElement needs to override SetAttr and UnsetAttr → Part 19 - nsSVGAElement needs to override SetAttr and UnsetAttr v1.0
r=mconnor, r=sdwilsh, r=bz
Attachment #423412 - Attachment is obsolete: true
Attachment #422586 - Attachment description: Part 21 - Cache a pointer to nsIContent in Link → Part 21 - Cache a pointer to nsIContent in Link v1.0
Attachment #422591 - Attachment description: Part 22 - Reset when given an unknown state. → Part 22 - Reset when given an unknown state v1.0
r=bz
Attachment #423429 - Attachment is obsolete: true
Attachment #423562 - Attachment is obsolete: true
r=bz
Attachment #421712 - Attachment is obsolete: true
r=jwatt
Attachment #421914 - Attachment is obsolete: true
Attached patch Part 26 - Fix content tests v1.1 (obsolete) — Splinter Review
r=bz
Attachment #422682 - Attachment is obsolete: true
Attachment #421905 - Attachment is obsolete: true
Attachment #423681 - Flags: review?(dbaron)
Attachment #421905 - Flags: review?(dbaron)
Blocks: 542592
(In reply to comment #199)
> (From update of attachment 423429 [details] [diff] [review])
> This looks ok; is there a followup bug to reevaluate mLinkMap in light of this
> change?
Filed bug 542592.
Blocks: 542632
This makes us inform the document after we've successfully registered with IHistory.  We already tell the document to forget about us in ResetLinkState.  With this, and a revised Part 26, we are in much better shape dealing with href changes.  We'll crash until I kill off part 22, which is going to need bug 542592 first (these two bugs will have to land at the same time I think).
Attachment #423886 - Flags: review?(bzbarsky)
Attached patch Part 26 - Fix content tests v1.2 (obsolete) — Splinter Review
Fixed tests that mostly pass after part 28 (needs bug 542592 as well).
Attachment #423680 - Attachment is obsolete: true
Attachment #423888 - Flags: review?(bzbarsky)
Comment on attachment 423886 [details] [diff] [review]
Part 28 - Tell the document after we register v1.0

So this will add the link to the link map for all sorts of cases where it didn't use to before (specifically any attr changes on links in display:none subtrees and querySelector matching).  Those might be rare cases, so this is probably ok, esp. since we're figuring we'll make the link map cheaper.  But something to watch out for.
Attachment #423886 - Flags: review?(bzbarsky) → review+
Comment on attachment 423888 [details] [diff] [review]
Part 26 - Fix content tests v1.2

Hmm.  So now that I know this is the base-changing test, it seems like the fill thing should in fact work.  Why doesn't it?