645197 - History is initialized and used even when useGlobalHistory is disabled

Status: RESOLVED INACTIVE

(Toolkit :: Places, defect, P5)

Description

[Benjamin Stover (:stechz)]

I'm not sure if this is an actual problem, but during an experiment I ran across this. History and nsNavHistory will get initialized if useGlobalHistory is set to false on the docshell.

Maybe this is expected behavior? I thought I'd file to make sure.

Comment 1

[Benjamin Stover (:stechz)]

Attachment: History is initialized and used even when useGlobalHistory is disabled

Comment 2

[Boris Zbarsky [:bzbarsky]]

Hmm.  So this styles the links but that's it, right?  No writes to history?

I wonder what behavior we want here...

Comment 3

[Marco Bonardo [:mak]]

so, this appears as a remaining part of bug 625712.

As Boris said, this is only about links styling, for new history this is useless work done, but this will style links for old existing history entries. I guess what's the case where useGlobalHistory changes to false when previously was true... I think the less suprising thing is to ignore existing history, if no global history should be used, there should be no way to know about visited status.

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 521986 [details] [diff] [review]
History is initialized and used even when useGlobalHistory is disabled

>diff --git a/content/base/src/Link.cpp b/content/base/src/Link.cpp

>+    nsIDocument *doc = content->GetCurrentDoc();
> 
>-      // Assume that we are not visited until we are told otherwise.
>-      self->mLinkState = eLinkState_Unvisited;
>+    // Check to see if History is being used for this link.
>+    PRBool useGlobalHistory = PR_FALSE;

nit: I'd init *doc just before its use (so after useGlobalHistory)

>+    if (doc) {
>+      nsPIDOMWindow *window = doc->GetWindow();
>+      if (window) {
>+        nsIDocShell* docShell = window->GetDocShell();
>+        if (docShell) {
>+          nsCOMPtr<nsIDocShellHistory> webnav =
>+            do_QueryInterface(docShell);
>+          webnav->GetUseGlobalHistory(&useGlobalHistory);
>+        }
>+      }
>+    }

I'm a bit (perf) worried that we have to do this for each link in the page, when actually it would be needed just once. But at the same time modifying nsIDocument to cache it is not worth it. I guess we'll have to live with it.

>+    if (useGlobalHistory) {
>+      self->mHistory = services::GetHistoryService();
>+      nsresult rv = mHistory->RegisterVisitedCallback(hrefURI, self);
>+      if (NS_SUCCEEDED(rv)) {

Just to be on the safe side, assert mHistory and then: if (mHistory && NS_SUCCEEDED(mHistory->

Globally sounds fine, I don't think there is much to work around having to check for each link, maybe Boris has ideas or can guess if that's just fast enough to not care.

Comment 5

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.