The PB service is going away. We need to get this information from the closest docshell instead when determining whether to use information such as link visited state.

Is this ready for review Josh?

Comment on attachment 593596 [details] [diff] [review] Derive private browsing status from docshell when assigning element states. Feel free to turn this into a review request, or pass it off to someone else. I'm looking for feedback as to the kinds of tests I should run, as I have not run any yet. I'm also flailing around in the code, so confirmation that this is the right way to do this would be good.

Comment on attachment 593596 [details] [diff] [review] Derive private browsing status from docshell when assigning element states. Review of attachment 593596 [details] [diff] [review]: ----------------------------------------------------------------- I assume the PB-status of a docshell can't change during its lifetime. dbaron needs to review this. ::: layout/style/nsRuleProcessorData.h @@ +147,5 @@ > { > + nsCOMPtr<nsISupports> container = mDocument->GetContainer(); > + nsCOMPtr<nsILoadContext> loadContext = do_QueryInterface(container); > + if (loadContext) { > + loadContext->GetUsePrivateBrowsing(&mUsingPrivateBrowsing); Could there be a deCOM version of this method? @@ +150,5 @@ > + if (loadContext) { > + loadContext->GetUsePrivateBrowsing(&mUsingPrivateBrowsing); > + } else { > + //XXXjdm What's the right thing to do here? > + NS_WARNING("Couldn't get loadContext from container; assuming no private browsing."); Wouldn't it be better to default to private?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7) > Comment on attachment 593596 [details] [diff] [review] > Derive private browsing status from docshell when assigning element states. > > Review of attachment 593596 [details] [diff] [review]: > ----------------------------------------------------------------- > > I assume the PB-status of a docshell can't change during its lifetime. It can. > @@ +150,5 @@ > > + if (loadContext) { > > + loadContext->GetUsePrivateBrowsing(&mUsingPrivateBrowsing); > > + } else { > > + //XXXjdm What's the right thing to do here? > > + NS_WARNING("Couldn't get loadContext from container; assuming no private browsing."); > > Wouldn't it be better to default to private? I don't think so. That's definitely not the current behavior. Is there any case where loadContext can be null there?

Comment on attachment 593596 [details] [diff] [review] Derive private browsing status from docshell when assigning element states. Oops, I probably should have tossed this review to bz when it came in, since he's been in this code more lately (and has some patches-in-progress affecting it, I think). That said, it looks fine. Could you name the params that are of type TreeMatchContext aTreeMatchContext rather than aMatchContext -- since some functions could end up with a NodeMatchContext at some point in the future too? >+ //XXXjdm What's the right thing to do here? >+ NS_WARNING("Couldn't get loadContext from container; assuming no private browsing."); Seems like this ought to be at least an NS_ASSERTION. r=dbaron with that

Hmm. Can we end up there with a data document of some sort? Or do those have containers?

Try run for ad8586ccbe14 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=ad8586ccbe14 Results (out of 55 total builds): success: 43 warnings: 12 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josh@joshmatthews.net-ad8586ccbe14

Try run for 9e829ab62cd0 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=9e829ab62cd0 Results (out of 62 total builds): success: 54 warnings: 8 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josh@joshmatthews.net-9e829ab62cd0

Now passes with bug 723353 landed.