Last Comment Bug 722853 - Determine Private Browsing state from nearest docshell
: Determine Private Browsing state from nearest docshell
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla14
Assigned To: Josh Matthews [:jdm]
:
Mentors:
Depends on: 822928 722840 723353 762345
Blocks: PBnGen
  Show dependency treegraph
 
Reported: 2012-01-31 13:48 PST by Josh Matthews [:jdm]
Modified: 2012-12-18 18:37 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Derive private browsing status from docshell when assigning element states. (5.54 KB, patch)
2012-02-01 01:56 PST, Josh Matthews [:jdm]
no flags Details | Diff | Splinter Review
Derive private browsing status from docshell when assigning element states. (5.55 KB, patch)
2012-02-01 12:54 PST, Josh Matthews [:jdm]
no flags Details | Diff | Splinter Review
Derive private browsing status from docshell when assigning element states. (12.29 KB, patch)
2012-02-01 13:42 PST, Josh Matthews [:jdm]
no flags Details | Diff | Splinter Review
Derive private browsing status from docshell when assigning element states. (12.33 KB, patch)
2012-02-01 13:45 PST, Josh Matthews [:jdm]
dbaron: review+
Details | Diff | Splinter Review
Derive private browsing status from docshell when assigning element states. (12.34 KB, patch)
2012-02-21 08:25 PST, Josh Matthews [:jdm]
no flags Details | Diff | Splinter Review

Description Josh Matthews [:jdm] 2012-01-31 13:48:14 PST
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] 2012-02-01 01:56:03 PST
Created attachment 593347 [details] [diff] [review]
Derive private browsing status from docshell when assigning element states.
Comment 2 Josh Matthews [:jdm] 2012-02-01 12:54:11 PST
Created attachment 593578 [details] [diff] [review]
Derive private browsing status from docshell when assigning element states.
Comment 3 Josh Matthews [:jdm] 2012-02-01 13:42:39 PST
Created attachment 593595 [details] [diff] [review]
Derive private browsing status from docshell when assigning element states.
Comment 4 Josh Matthews [:jdm] 2012-02-01 13:45:05 PST
Created attachment 593596 [details] [diff] [review]
Derive private browsing status from docshell when assigning element states.
Comment 5 :Ehsan Akhgari 2012-02-06 14:35:14 PST
Is this ready for review Josh?
Comment 6 Josh Matthews [:jdm] 2012-02-06 14:37:42 PST
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) (Exited; email my personal email if necessary) 2012-02-06 14:59:39 PST
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 :Ehsan Akhgari 2012-02-06 16:23:12 PST
(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+8 (review requests must explain patch) 2012-02-13 15:56:24 PST
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 [:bz] 2012-02-13 16:28:28 PST
Hmm.  Can we end up there with a data document of some sort?  Or do those have containers?
Comment 11 Mozilla RelEng Bot 2012-02-18 02:45:20 PST
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
Comment 12 Josh Matthews [:jdm] 2012-02-21 08:25:27 PST
Created attachment 599188 [details] [diff] [review]
Derive private browsing status from docshell when assigning element states.
Comment 13 Mozilla RelEng Bot 2012-02-21 17:03:01 PST
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
Comment 16 Josh Matthews [:jdm] 2012-03-29 23:13:39 PDT
Now passes with bug 723353 landed.
Comment 18 Ed Morley [:emorley] 2012-03-30 12:55:54 PDT
https://hg.mozilla.org/mozilla-central/rev/cf941140cded

Note You need to log in before you can comment on or make changes to this bug.