Closed Bug 461199 (async-visited-check) Opened 16 years ago Closed 14 years ago

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

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

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...
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+
Comment 119 sounds good to me.
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?
(In reply to comment #237)
> (From update of attachment 423888 [details] [diff] [review])
> 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?
Not really sure.  Do SVG documents do something different here, or do they just use nsDocument as well?
The should be using nsDocument afaik.
OK then.  Rebuilding with DOMi to try and figure out what is going on in the SVG case.
Ah, test_bug209275.xhtml was totally bogus in the past, and it was luck as to why it was passing.  Fixing, and will attach a new version for it.
Lots of context here should make my fixes obvious.  The test was clearly wrong int the past...
Attachment #423888 - Attachment is obsolete: true
Attachment #424682 - Flags: review?(bzbarsky)
Attachment #423888 - Flags: review?(bzbarsky)
Comment on attachment 423681 [details] [diff] [review]
Part 27 - Fix layout tests v1.1 (checked in)

r=dbaron if you tested that the test still fails if you remove the code that implements this pref
Attachment #423681 - Flags: review?(dbaron) → review+
(In reply to comment #243)
> (From update of attachment 423681 [details] [diff] [review])
> r=dbaron if you tested that the test still fails if you remove the code that
> implements this pref
It falls flat on its face.
Comment on attachment 424682 [details] [diff] [review]
Part 26 - Fix content tests v1.3 (checked in)

So the key new change here is s/ellipse/ellipselink/?  If so, r=bzbarsky
Attachment #424682 - Flags: review?(bzbarsky) → review+
(In reply to comment #245)
> So the key new change here is s/ellipse/ellipselink/?  If so, r=bzbarsky
Yup.  An element with id ellipse never actually existed, so the test only passed because of that.
Try server runs still failing on one test.  Timeout; last output is:
32530 INFO TEST-PASS | /tests/content/html/content/test/test_bug209275.xhtml | link 5 test 2
Will look into this tomorrow since I thought this was passing locally.
(In reply to comment #247)
> Try server runs still failing on one test.  Timeout; last output is:
> 32530 INFO TEST-PASS | /tests/content/html/content/test/test_bug209275.xhtml |
> link 5 test 2
> Will look into this tomorrow since I thought this was passing locally.
So, it passes locally when running by itself, but not when it's running with other tests.  Investigating.
(In reply to comment #248)
> So, it passes locally when running by itself, but not when it's running with
> other tests.  Investigating.
Even better, if I attach a debugger to it and make sure things are getting called, it passes :(
OK, narrowed it down to an invalidation issue.  The test passes by itself, no problem.  The test does not pass the part where I check for the visited color on colorlink, but will pass once I move my mouse over the ellipse and the colored link the test is checking.  Any suggestions on what could be wrong here?  I'm guessing it has to do with it being in the iframe?
So the issue is that the colorlink never ends up with the visited color, right?  It should end up with that whenever the ContentStatesChanged from SetLinkState happens...
(In reply to comment #251)
> So the issue is that the colorlink never ends up with the visited color, right?
>  It should end up with that whenever the ContentStatesChanged from SetLinkState
> happens...
Yes, this is the issue.  It does end up getting this color if the test is ran by itself.  It does not when ran inside the iframe unless I mouse over it.  Any ideas on why this is the case?
Everything looks good, except for a few tests issues that have popped up:
test_bug500328.html was added in bug 500328, and I'll have to fix it like the rest of the tests.
363247-1.html is failing, but it's not clear yet how these changes even come close to impacting it.  It's possible I also had a bad m-c pull.
Fixes test_bug500328.html that was recently added.
Attachment #426595 - Flags: review?(jonas)
Comment on attachment 426595 [details] [diff] [review]
Part 29 - Fix dom tests (checked in)

r=me
Attachment #426595 - Flags: review?(jonas) → review+
I cannot reproduce the failure in 363247-1.html locally, so I'm going to push to the Places branch and see if I was just unlucky on try.
(In reply to comment #256)
> I cannot reproduce the failure in 363247-1.html locally, so I'm going to push
> to the Places branch and see if I was just unlucky on try.
But the places branch is hitting it too.  The test (as seen at http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/363247-1.html?force=1) changes the stylesheet URI of a link node.  Now, AFAICT, the code properly handles this.  The only way this could fail is if the new stylesheet URI is not parsed.  Hrm...
OK, this should make us green across the board.  There was a cache invalidation issue that needed to be fixed.
Attachment #426736 - Flags: review?(bzbarsky)
Attachment #426736 - Flags: review?(bzbarsky) → review+
Attachment #426736 - Attachment description: Part 30 - Fix failing reftest + new test bz added → Part 30 - Fix failing reftest + new test bz added v1.0
Attachment #426736 - Attachment is obsolete: true
This passes the tests that we've been having issues with locally.
Attachment #427153 - Flags: review?(bzbarsky)
Comment on attachment 427153 [details] [diff] [review]
Part 30 - Fix link invalidation issues

r=bzbarsky
Attachment #427153 - Flags: review?(bzbarsky) → review+
Places tree looks greenish, although not everything has appeared to have cycled yet.
Can confirm with a recent merge today.  It's all green!  Now I just need a window to land on mozilla-central...
I think this may have caused a new intermittent reftest failure.... see bug 546898.
Depends on: 546913
Blocks: 546938
Comment on attachment 423641 [details] [diff] [review]
Part 2 - New IHistory API v1.13 (checked in)

http://hg.mozilla.org/mozilla-central/rev/ed47ac6b707e
Attachment #423641 - Attachment description: Part 2 - New IHistory API v1.13 → Part 2 - New IHistory API v1.13 (checked in)
Comment on attachment 423643 [details] [diff] [review]
Part 4 - Add NS_EVENT_STATE_UNVISITED to nsIEventStateManager v1.3 (checked in)

http://hg.mozilla.org/mozilla-central/rev/850c88a0f147
Attachment #423643 - Attachment description: Part 4 - Add NS_EVENT_STATE_UNVISITED to nsIEventStateManager v1.3 → Part 4 - Add NS_EVENT_STATE_UNVISITED to nsIEventStateManager v1.3 (checked in)
While I didn't get a chance to test this before leaving for work, I thought I would point out that at least 1 person as noticed something while running Acid3 test with this patch that was in the build for awhile yesterday before being backed out: 
http://forums.mozillazine.org/viewtopic.php?p=8742205#p8742205
OK< just tested with an hourly build prior to the backout, and I do see in big red letters : LINKTEST FAILED , looks the failed tests, I don't see any that relate to links.. 

Failed 3 tests.
Test 26 passed, but took 100ms (less than 30fps)
Test 77 failed: expected '4776' but got '5560' - getComputedTextLength failed.
Test 78 failed: expected '90' but got '0' - getRotationOfChar(0) failed.
Test 79 failed: expected '34' but got '33' - SVGSVGTextElement.getNumberOfChars() incorrect
Total elapsed time: 1.31s
Presumably the issue is that acid3 uses some sort of :visited styling which is now applied asynchronously.
The LINKTEST FAILED bit goes away before completion too...
The file "test_IHistory.cpp" (added for this bug in changeset ed47ac6b707e, and still in m-c even after the backout) seems to cause build failures on certain mac OS configurations, due to an ambiguity with the test's (namespace-protected) global-variable "link" vs the function "link()" defined in unistd.h.
{
> test_IHistory.cpp: In function 'void test_unvisted_does_not_notify_part1()':
> test_IHistory.cpp:103: error: reference to 'link' is ambiguous
> /usr/include/unistd.h:468: error: candidates are: int link(const char*, const char*)
> test_IHistory.cpp:86: error:                 nsCOMPtr<mozilla::dom::Link> test_unvisited_does_not_notify::link
}

I get this error for lines 103, 107, 141, and 146 (everywhere we use the namespaced 'link' variable), and it busts the build.

I initially assumed it was something wonky on my machine, but minutes after I saw it, gandalf commented on it in IRC.  So it's not just me.

FWIW, the conflicting 'link' in /usr/include/unistd.h on my machine is:
> int      link(const char *, const char *);

There's a simple fix: just rename 'link' in the test file to 'testLink', which doesn't conflict with anything.
(In reply to comment #270)
> There's a simple fix: just rename 'link' in the test file to 'testLink', which
> doesn't conflict with anything.

With sheriff ddahl's permission, I just pushed a bustage fix to make this change:
 http://hg.mozilla.org/mozilla-central/rev/c6567e3efafb
So the issue is that this style is not applied immediately:
#linktest:visited { display: none; }
which is used in test 48:
    function () {
      // test 48: :link and :visited
      var iframe = document.getElementById("selectors");
      var number = (new Date()).valueOf();
      var a = document.createElement('a');
      a.appendChild(document.createTextNode('LINKTEST FAILED'));
      a.setAttribute('id', 'linktest');
      a.setAttribute('class', 'pending');
      a.setAttribute('href', iframe.getAttribute('src') + "?" + number);
      document.getElementsByTagName('map')[0].appendChild(a);
      iframe.setAttribute("onload", "document.getElementById('linktest').removeAttribute('class')");
      iframe.src = a.getAttribute("href");
      return 3;
    },
Trying to figure out this regression still and here is some data.  The regression does not appear to be related to the number of Link-inheriting elements on a page.  The number of canvas reflows appears to be the same (or sometimes lower with the new code) with or without the changes.  The number of restyles (calls to nsCSSFrameConstructor::RestyleElement) actually drops with the changes.

roc had also asked for timings of the IsVisited (sync) check and then RegisterVisitedCallback (async) to compare old and new.  I ran each function 1000 times, timing that loop, with the following timings:
*** Sync visited total = 9.400000e-002 (.094 seconds)
*** Sync unvisited total = 9.200000e-002 (.092 seconds)
*** Async visited total = 3.000000e-003 (.003 seconds)
*** Async unvisited total = 4.000000e-003 (.004 seconds)
These numbers were fairly consistent.


At this point, I don't know what could be causing the regression.  Anybody have any ideas of what else I could benchmark to try and pin a cause?
I revised my benchmark to make sure I use a unique URI (pre generated before the timings) each time so I don't take advantage of an optimization in IHistory that wouldn't send a message to the other thread.  The new timings look like this (and the numbers are still pretty consistent):
Sync visited total = 9.600000e-002 (.096 seconds)
Sync unvisited total = 9.400000e-002 (.094 seconds)
Async visited total = 4.900000e-002 (.049 seconds)
Async unvisited total = 5.000000e-002 (.05 seconds)
So the new method is about twice as fast as the old method.

And if I reverse the order of running the the tests (the use the same query, so the second one should have a cached result), the numbers for async are about the same, but sync goes up a bit:
Sync visited total = 1.620000e-001 (.162 seconds)
Sync unvisited total = 1.480000e-001 (.148 seconds)
Async visited total = 5.100000e-002 (.051 seconds)
Async unvisited total = 4.900000e-002 (.049 seconds)
Number of NS_NewURI calls?
(In reply to comment #275)
> Number of NS_NewURI calls?
I added logging to nsIOService::NewURI and those numbers certainly did go up:
youtube - 1398 -> 1567 (-169) with 123 links and a 3.4% win
mail.ru - 1582 -> 1792 (-210) with 143 links and a 37% loss
imdb - 1387 -> 1628 (-241) with 250 links and a 26.4% loss

These numbers were taken by starting and then stopping the browser and passing the page to load to the command line.

We are clearing the link cache more often than we did in the past (in Bind and Unbind), which we didn't do at all before (only when [Un]SetAttr was called, or the document told us to forget our link state).  Perhaps that is it?
Huh.  What does the NewURI number look like (either before or after or both) for about:blank?

If you comment out the cache-clearing in Unbind and Bind, what do the numbers look like?
(In reply to comment #277)
> Huh.  What does the NewURI number look like (either before or after or both)
> for about:blank?
After numbers: 937 consistently
OK.  So the number of newURI calls for the content pages has gone from about 450-600 to about 600-800, right?

I really wish we could get useful performance data other than by landing on m-c.  :(  Do you want to try seeing how bind/unbind affect the newuri numbers?
(In reply to comment #279)
> I really wish we could get useful performance data other than by landing on
> m-c.  :(  Do you want to try seeing how bind/unbind affect the newuri numbers?
Yeah I just had to leave right after the build finished last night so I didn't get the numbers yet.
(In reply to comment #277)
> If you comment out the cache-clearing in Unbind and Bind, what do the numbers
> look like?
I'm seeing virtually no change in numbers, and we hit some assertions now about adding/removing not happening as expected.
>  I'm seeing virtually no change in numbers,

OK.  Any idea why we're seeing more newURI calls, then?

You could maybe add an NS_ERROR or some such to newURI, run with XPCOM_DEBUG_BREAK=stack, then post-process the result to get some idea of where the calls are coming from...
Updated part 30 per irc conversation with bz.  This gets rid of our extra NewURI calls that got added in.  r=bz over irc.

Going to land this on places first, to make sure tests pass, and then we'll go from there.
Attachment #427153 - Attachment is obsolete: true
Attachment #428567 - Flags: review?(bzbarsky)
Comment on attachment 428567 [details] [diff] [review]
test for part 30 (checked in)

r=bzbarsky
Attachment #428567 - Flags: review?(bzbarsky) → review+
Blocks: 510202
Depends on: 534526
bug 461199 has landed so it should no longer be necessary to disable layout/reftests/svg/dynamic-link-style-01.svg
content/html/content/test/test_bug209275.xhtml (see bug 461199 comment 23)
oops, comment 286 should have been...

bug 534526 has landed so it should no longer be necessary to disable
layout/reftests/svg/dynamic-link-style-01.svg
content/html/content/test/test_bug209275.xhtml (see bug 534526 comment 23)
No longer depends on: 534526
The latest patches here don't disable those anyway.
(In reply to comment #267)
> OK< just tested with an hourly build prior to the backout, and I do see in big
> red letters : LINKTEST FAILED , looks the failed tests, I don't see any that
> relate to links.
This is because we don't style it right away.  Can you please file a new bug on this issue please?
I don't think the regression went away at all at least on OS X.  We had a slight win from a different bug, which lowered our baseline.  Factoring that in, it looks like the same height.  So, I ran some new logging:
We call ForgetLink less often with the new code.
We call AddStyleReleventLink the same amount of times.
Links do not participate in cycle collection anymore.
Checking the visitedness of a link (IHistory::RegisterVisistedCallback or nsIGlobalHistory2::IsVisited) Goes from 154 -> 153, so essentially no change.

I don't really know what else to check here at this point.  Everything I can think of is either doing less work, or the same work as before with the exception of Resetting on Bind and UnBind, but we don't see us doing that during tp anyway.  Once again, I'm open to ideas on what could possibly be regressing this right now.
Is the new regression happening on the same pages as the old regression?
Patches for the following two bugs: bug 546098 and bug 54584 landed the same morning and caused the regression of bug 547128.

Considering that Dao can't see how his patches could have caused bug 547128 (see bug 547128 comment 1) -- even though the regression range matches his check-ins, is it possible that those two check-ins might also be responsible for the regression you people are seeing?

I'm not sure myself, since I'm not sure how to test the regression you're seeing, but I thought I'd at least mention it.
(In reply to comment #292)
> Patches for the following two bugs: bug 546098 and bug 54584 landed the same
> morning and caused the regression of bug 547128.

Apparently you're referring to another morning.
(In reply to comment #292)
> Considering that Dao can't see how his patches could have caused bug 547128
> (see bug 547128 comment 1) -- even though the regression range matches his
> check-ins, is it possible that those two check-ins might also be responsible
> for the regression you people are seeing?
No.
(In reply to comment #291)
> Is the new regression happening on the same pages as the old regression?
Other than a few sites changing around, it doesn't look like it.

This regression only showed up on os X as well :/
Pushed a followup to fix the g++ warning "ISO C++ does not support the 'z' printf length modifier" in places_test_harness_tail.h, a file that was added by this bug.
(rs=sdwilsh granted via IRC) http://hg.mozilla.org/mozilla-central/rev/17da12c3ab04
OK.  So it sounds like this regression is not specific to particular pages or something....  I assume the mac machines running talos are in fact multi-core?
(In reply to comment #297)
> OK.  So it sounds like this regression is not specific to particular pages or
> something....  I assume the mac machines running talos are in fact multi-core?
They are.  Should we/me try and make the case that we should just take this mac only regression on dev.tree-management at this point?
Or at least that it can be addressed in a followup and may be accepted in the end, yes.
Comment on attachment 423642 [details] [diff] [review]
Part 3 - dd IHistory getter to nsContentUtils v1.1 (checked in)

http://hg.mozilla.org/mozilla-central/rev/98a35ca2f5a1
Attachment #423642 - Attachment description: Part 3 - dd IHistory getter to nsContentUtils v1.1 → Part 3 - dd IHistory getter to nsContentUtils v1.1 (checked in)
Comment on attachment 423644 [details] [diff] [review]
Part 5 - Add Link::LinkState v1.2 (checked-in)

http://hg.mozilla.org/mozilla-central/rev/2363629e31ce
Attachment #423644 - Attachment description: Part 5 - Add Link::LinkState v1.2 → Part 5 - Add Link::LinkState v1.2 (checked-in)
Comment on attachment 423645 [details] [diff] [review]
Part 6 - Implement nsSVGAElement::GetHrefURI v1.1 (checked in)

http://hg.mozilla.org/mozilla-central/rev/464faa386b92
Attachment #423645 - Attachment description: Part 6 - Implement nsSVGAElement::GetHrefURI v1.1 → Part 6 - Implement nsSVGAElement::GetHrefURI v1.1 (checked in)
Comment on attachment 423648 [details] [diff] [review]
Part 7 - Link should cache its URI v1.2 (checked in)

http://hg.mozilla.org/mozilla-central/rev/2bf4da33e541
Attachment #423648 - Attachment description: Part 7 - Link should cache its URI v1.2 → Part 7 - Link should cache its URI v1.2 (checked in)
Comment on attachment 423649 [details] [diff] [review]
Part 8 - Remove eLazyURIValue v1.1 (checked in)

http://hg.mozilla.org/mozilla-central/rev/28605d6dbb55
Attachment #423649 - Attachment description: Part 8 - Remove eLazyURIValue v1.1 → Part 8 - Remove eLazyURIValue v1.1 (checked in)
Comment on attachment 423650 [details] [diff] [review]
Part 9 - Move helper methods to Link v1.5 (checked in)

http://hg.mozilla.org/mozilla-central/rev/74f08c7a58cc
Attachment #423650 - Attachment description: Part 9 - Move helper methods to Link v1.5 → Part 9 - Move helper methods to Link v1.5 (checked in)
Comment on attachment 423651 [details] [diff] [review]
Part 10 - Ensure we call ResetLinkState on BindFromTree and UnbindFromTree v1.1 (checked in)

http://hg.mozilla.org/mozilla-central/rev/8e4fb9ef39c7
Attachment #423651 - Attachment description: Part 10 - Ensure we call ResetLinkState on BindFromTree and UnbindFromTree v1.1 → Part 10 - Ensure we call ResetLinkState on BindFromTree and UnbindFromTree v1.1 (checked in)
Comment on attachment 423653 [details] [diff] [review]
Part 11 - Register with IHistory in Link::VisitedState v1.4 (checked in)

http://hg.mozilla.org/mozilla-central/rev/d013cef514a1
Attachment #423653 - Attachment description: Part 11 - Register with IHistory in Link::VisitedState v1.4 → Part 11 - Register with IHistory in Link::VisitedState v1.4 (checked in)
Comment on attachment 423655 [details] [diff] [review]
Part 12 - Link::~Link should call Link::UnregisterFromHistory v2.3 (checked in)

http://hg.mozilla.org/mozilla-central/rev/adc5d77e82f8
Attachment #423655 - Attachment description: Part 12 - Link::~Link should call Link::UnregisterFromHistory v2.3 → Part 12 - Link::~Link should call Link::UnregisterFromHistory v2.3 (checked in)
Comment on attachment 423656 [details] [diff] [review]
Part 13 - Link::SetLinkState should notify document on change v1.4 (checked in)

http://hg.mozilla.org/mozilla-central/rev/938cf5c0b16a
Attachment #423656 - Attachment description: Part 13 - Link::SetLinkState should notify document on change v1.4 → Part 13 - Link::SetLinkState should notify document on change v1.4 (checked in)
Comment on attachment 423657 [details] [diff] [review]
Part 14 - Subclasses of Link must implement IntrinsicState and call Link::LinkState v1.1 (checked in)

http://hg.mozilla.org/mozilla-central/rev/ddfb63f3bccd
Attachment #423657 - Attachment description: Part 14 - Subclasses of Link must implement IntrinsicState and call Link::LinkState v1.1 → Part 14 - Subclasses of Link must implement IntrinsicState and call Link::LinkState v1.1 (checked in)
Comment on attachment 423659 [details] [diff] [review]
Part 15 - Layout should call IntrinsicState instead of GetLinkState for style information v1.5 (checked in)

http://hg.mozilla.org/mozilla-central/rev/70b16597aaf6
Attachment #423659 - Attachment description: Part 15 - Layout should call IntrinsicState instead of GetLinkState for style information v1.5 → Part 15 - Layout should call IntrinsicState instead of GetLinkState for style information v1.5 (checked in)
Comment on attachment 423661 [details] [diff] [review]
Part 16 - Remove now unused link testin code in nsStyleUtils v1.1 (checked in)

http://hg.mozilla.org/mozilla-central/rev/d725b28f0116
Attachment #423661 - Attachment description: Part 16 - Remove now unused link testin code in nsStyleUtils v1.1 → Part 16 - Remove now unused link testin code in nsStyleUtils v1.1 (checked in)
Comment on attachment 423663 [details] [diff] [review]
Part 17 - Remove unused code from webshell and docshell v1.1 (checked in)

http://hg.mozilla.org/mozilla-central/rev/4ae1f164e1f0
Attachment #423663 - Attachment description: Part 17 - Remove unused code from webshell and docshell v1.1 → Part 17 - Remove unused code from webshell and docshell v1.1 (checked in)
Comment on attachment 423665 [details] [diff] [review]
Part 18 - Notification about lookup completion v1.1 (checked in)

http://hg.mozilla.org/mozilla-central/rev/7f7492f34faf
Attachment #423665 - Attachment description: Part 18 - Notification about lookup completion v1.1 → Part 18 - Notification about lookup completion v1.1 (checked in)
Comment on attachment 423667 [details] [diff] [review]
Part 19 - nsSVGAElement needs to override SetAttr and UnsetAttr v1.1 (checked in)

http://hg.mozilla.org/mozilla-central/rev/11b5924dccd6
Attachment #423667 - Attachment description: Part 19 - nsSVGAElement needs to override SetAttr and UnsetAttr v1.1 → Part 19 - nsSVGAElement needs to override SetAttr and UnsetAttr v1.1 (checked in)
Comment on attachment 423668 [details] [diff] [review]
 Part 20 - Turn off visited link coloring in private browsing mode v2.6 (checked in)

http://hg.mozilla.org/mozilla-central/rev/db0c3219ed3d
Attachment #423668 - Attachment description: Part 20 - Turn off visited link coloring in private browsing mode v2.6 → Part 20 - Turn off visited link coloring in private browsing mode v2.6 (checked in)
Comment on attachment 423669 [details] [diff] [review]
Part 21 - Cache a pointer to nsIContent in Link v1.1 (checked in)

http://hg.mozilla.org/mozilla-central/rev/c8b472547655
Attachment #423669 - Attachment description: Part 21 - Cache a pointer to nsIContent in Link v1.1 → Part 21 - Cache a pointer to nsIContent in Link v1.1 (checked in)
Comment on attachment 423671 [details] [diff] [review]
Part 22 - Reset when given an unknown state v1.1 (checked in)

http://hg.mozilla.org/mozilla-central/rev/ae0a2a95651e
Attachment #423671 - Attachment description: Part 22 - Reset when given an unknown state v1.1 → Part 22 - Reset when given an unknown state v1.1 (checked in)
Comment on attachment 423676 [details] [diff] [review]
Part 23 - Remove dead code in nsDocument v1.2 (checked in)

http://hg.mozilla.org/mozilla-central/rev/b86245842847
Attachment #423676 - Attachment description: Part 23 - Remove dead code in nsDocument v1.2 → Part 23 - Remove dead code in nsDocument v1.2 (checked in)
Comment on attachment 423678 [details] [diff] [review]
Part 24 - Fix docshell tests v1.2 (checked in)

http://hg.mozilla.org/mozilla-central/rev/c95b8cf81ec3
Attachment #423678 - Attachment description: Part 24 - Fix docshell tests v1.2 → Part 24 - Fix docshell tests v1.2 (checked in)
Comment on attachment 423679 [details] [diff] [review]
Part 25 - Fix SVG tests v1.1 (checked in)

http://hg.mozilla.org/mozilla-central/rev/ddd82a95e68c
Attachment #423679 - Attachment description: Part 25 - Fix SVG tests v1.1 → Part 25 - Fix SVG tests v1.1 (checked in)
Comment on attachment 423681 [details] [diff] [review]
Part 27 - Fix layout tests v1.1 (checked in)

http://hg.mozilla.org/mozilla-central/rev/270092d27184
Attachment #423681 - Attachment description: Part 27 - Fix layout tests v1.1 → Part 27 - Fix layout tests v1.1 (checked in)
Comment on attachment 424682 [details] [diff] [review]
Part 26 - Fix content tests v1.3 (checked in)

http://hg.mozilla.org/mozilla-central/rev/554bbd46a491
Attachment #424682 - Attachment description: Part 26 - Fix content tests v1.3 → Part 26 - Fix content tests v1.3 (checked in)
Comment on attachment 426595 [details] [diff] [review]
Part 29 - Fix dom tests (checked in)

http://hg.mozilla.org/mozilla-central/rev/79462e4e234d
Attachment #426595 - Attachment description: Part 29 - Fix dom tests → Part 29 - Fix dom tests (checked in)
Comment on attachment 428522 [details] [diff] [review]
Part 30 - Fix link invalidation issues v1.2 (checked in)

http://hg.mozilla.org/mozilla-central/rev/639b98ae11a8
Attachment #428522 - Attachment description: Part 30 - Fix link invalidation issues v1.2 → Part 30 - Fix link invalidation issues v1.2 (checked in)
Comment on attachment 428567 [details] [diff] [review]
test for part 30 (checked in)

http://hg.mozilla.org/mozilla-central/rev/639b98ae11a8
Attachment #428567 - Attachment description: test for part 30 → test for part 30 (checked in)
All patches have landed, so this is now fixed.  Hurray!
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9.3a1 → mozilla1.9.3a2
So this breaks all "--disable-places" products which use Gecko, right?

According to nsContentUtils::Init(),
nsContentUtils::sImgLoader and nsContentUtils::sImgCache can be null.
What's wrong with nsContentUtils::sHistory is null?
(In reply to comment #328)
> So this breaks all "--disable-places" products which use Gecko, right?
Yes, and this is meant to be intentional at this point (bug 546913 comment 18).
Depends on: 549797
Depends on: 549887
Depends on: 550661
This also means that visited link colouring works in SeaMonkey mail when it didn't before because its browser uses disablehistory="true". Intentional?
Depends on: 550778
(In reply to comment #330)
> This also means that visited link colouring works in SeaMonkey mail when it
> didn't before because its browser uses disablehistory="true". Intentional?
I have no idea how that attribute works, and there were clearly no tests in the tree about it.  It wasn't intentional, and I know nothing about it.  File a new bug?
> I have no idea how that attribute works

It affects whether the relevant docshell has an nsIGlobalHistory.  Docshells that do not don't save entries to it, and didn't do visited styling (since they had nothing to ask for the visited state).

Since we now directly ask the IHistory, instead of going through docshell, we still don't save entries to history, but do read the visited state from it.  Definitely worth filing a bug to see whether we want that behavior.
Depends on: 550785
Depends on: 551838
Depends on: 559612
Depends on: 560089
No longer blocks: 516728
Blocks: 599978
Depends on: 718936
You need to log in before you can comment on or make changes to this bug.