722853 - Determine Private Browsing state from nearest docshell

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Josh Matthews [:jdm]]

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.

Comment 1

[Josh Matthews [:jdm]]

Attachment: Derive private browsing status from docshell when assigning element states.

Comment 2

[Josh Matthews [:jdm]]

Attachment: Derive private browsing status from docshell when assigning element states.

Comment 3

[Josh Matthews [:jdm]]

Attachment: Derive private browsing status from docshell when assigning element states.

Comment 4

[Josh Matthews [:jdm]]

Attachment: Derive private browsing status from docshell when assigning element states.

Comment 5

[(no longer active)]

Is this ready for review Josh?

Comment 6

[Josh Matthews [:jdm]]

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 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 8

[(no longer active)]

(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 9

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

Comment 11

[Mozilla RelEng Bot]

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/[e-mail]-ad8586ccbe14

Comment 12

[Josh Matthews [:jdm]]

Attachment: Derive private browsing status from docshell when assigning element states.

Comment 13

[Mozilla RelEng Bot]

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/[e-mail]-9e829ab62cd0

Comment 14

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f40a17213344

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c0553bc5bc9

Comment 16

[Josh Matthews [:jdm]]

Now passes with bug 723353 landed.

Comment 17

[Josh Matthews [:jdm]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/cf941140cded

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cf941140cded