Determine Private Browsing state from nearest docshell

RESOLVED FIXED in mozilla14

Status

()

Core
Layout
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jdm, Assigned: jdm)

Tracking

(Depends on: 1 bug)

Trunk
mozilla14
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Depends on: 722840
(Assignee)

Comment 1

6 years ago
Created attachment 593347 [details] [diff] [review]
Derive private browsing status from docshell when assigning element states.
(Assignee)

Comment 2

6 years ago
Created attachment 593578 [details] [diff] [review]
Derive private browsing status from docshell when assigning element states.
(Assignee)

Updated

6 years ago
Attachment #593347 - Attachment is obsolete: true
(Assignee)

Comment 3

6 years ago
Created attachment 593595 [details] [diff] [review]
Derive private browsing status from docshell when assigning element states.
(Assignee)

Updated

6 years ago
Attachment #593578 - Attachment is obsolete: true
(Assignee)

Comment 4

6 years ago
Created attachment 593596 [details] [diff] [review]
Derive private browsing status from docshell when assigning element states.
(Assignee)

Updated

6 years ago
Attachment #593595 - Attachment is obsolete: true
Is this ready for review Josh?
(Assignee)

Comment 6

6 years ago
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.
Attachment #593596 - Flags: feedback?(dbaron)
Attachment #593596 - Flags: review?(roc)
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?
Attachment #593596 - Flags: review?(roc)
Attachment #593596 - Flags: review?(dbaron)
Attachment #593596 - Flags: feedback?(dbaron)
(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
Attachment #593596 - Flags: review?(dbaron) → review+
Hmm.  Can we end up there with a data document of some sort?  Or do those have containers?

Comment 11

6 years ago
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
(Assignee)

Comment 12

6 years ago
Created attachment 599188 [details] [diff] [review]
Derive private browsing status from docshell when assigning element states.
(Assignee)

Updated

6 years ago
Attachment #593596 - Attachment is obsolete: true

Comment 13

6 years ago
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
(Assignee)

Updated

6 years ago
Whiteboard: [passes tests, needs landing]
(Assignee)

Updated

6 years ago
Assignee: nobody → josh
https://hg.mozilla.org/integration/mozilla-inbound/rev/f40a17213344
Target Milestone: --- → mozilla14
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c0553bc5bc9
Whiteboard: [passes tests, needs landing] → [doesn't pass tests]
Target Milestone: mozilla14 → ---
(Assignee)

Comment 16

6 years ago
Now passes with bug 723353 landed.
Depends on: 723353
Whiteboard: [doesn't pass tests]
(Assignee)

Comment 17

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/cf941140cded
https://hg.mozilla.org/mozilla-central/rev/cf941140cded
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Depends on: 762345

Updated

5 years ago
Depends on: 822928
You need to log in before you can comment on or make changes to this bug.