Last Comment Bug 722864 - nsFilePicker uses global PB service to decide whether to add choices to recent documents
: nsFilePicker uses global PB service to decide whether to add choices to recen...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla14
Assigned To: Diana Koenraadt
:
Mentors:
Depends on: 722840 744887
Blocks: PBnGen
  Show dependency treegraph
 
Reported: 2012-01-31 14:05 PST by Josh Matthews [:jdm]
Modified: 2012-04-12 12:11 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
use reference to docShell's loadContext to determine privacy mode (3.61 KB, patch)
2012-04-09 10:30 PDT, Diana Koenraadt
josh: feedback+
Details | Diff | Splinter Review
Improvements from review have been processed (4.02 KB, patch)
2012-04-09 11:40 PDT, Diana Koenraadt
jmathies: review+
Details | Diff | Splinter Review
Proposed patch: use reference to docShell's loadContext to determine privacy mode (4.49 KB, patch)
2012-04-09 13:17 PDT, Diana Koenraadt
no flags Details | Diff | Splinter Review

Description Josh Matthews [:jdm] 2012-01-31 14:05:41 PST
The global Private Browsing service is going away. The File Picker should use a reference to the nearest relevant docshell to decide instead.
Comment 1 :Ehsan Akhgari 2012-02-06 14:28:40 PST
This information should be propagated down from the callers of the file picker dialog.  The callers that we care about are the following I think:

* the file control frame implementation
* open/save dialogs invoked by the File menu entries

Firefox should treat any caller which does not pass down that information as not in PB mode.
Comment 2 Josh Matthews [:jdm] 2012-03-11 12:59:40 PDT
It looks like the file picker is initialized with an nsDOMWindow (nsBaseFilePicker::Init). Could we get an nsIWebNavigation from that, and get the nsILoadContext from there?
Comment 3 :Ehsan Akhgari 2012-03-14 17:15:55 PDT
(In reply to Josh Matthews [:jdm] from comment #2)
> It looks like the file picker is initialized with an nsDOMWindow
> (nsBaseFilePicker::Init). Could we get an nsIWebNavigation from that, and
> get the nsILoadContext from there?

Yes.  You should be able to do_GetInterface(domWindow) to get an nsIWebNavigation out of it.
Comment 4 :Ehsan Akhgari 2012-03-22 20:14:18 PDT
So what needs to be done here is to make the Windows nsFilePicker (widget/windows/nsFilePicker.cpp) override nsBaseFilePicker::Init, get the docshell associated with the nsIDOMWindow passed to it, QueryInterface it to nsILoadContext and then hold on to that in an nsCOMPtr member variable.  Once that is done, nsFilePicker::IsPrivacyModeEnabled can be modified to call GetUsePrivateBrowsing on the member nsILoadContext and return the result, instead of querying nsIPrivateBrowsingEnabled.

Hessam, are you willing to work on this?
Comment 5 Josh Matthews [:jdm] 2012-03-22 20:17:31 PDT
Actually Diana has been looking into this bug already, I'm pretty sure.
Comment 6 :Ehsan Akhgari 2012-03-22 21:01:14 PDT
(In reply to Josh Matthews [:jdm] from comment #5)
> Actually Diana has been looking into this bug already, I'm pretty sure.

Oh, sorry for stepping on your toes here, Diana!  :-)

I'll look for another bug for you, Hessam!
Comment 7 Diana Koenraadt 2012-03-22 23:02:49 PDT
My fault really, should have taken the bug.
Comment 8 Diana Koenraadt 2012-04-09 10:30:56 PDT
Created attachment 613337 [details] [diff] [review]
use reference to docShell's loadContext to determine privacy mode
Comment 9 :Ehsan Akhgari 2012-04-09 10:41:01 PDT
Comment on attachment 613337 [details] [diff] [review]
use reference to docShell's loadContext to determine privacy mode

Review of attachment 613337 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like a great start, Diana!

I have a few comments below on the patch.  Also, can you please test this patch using the steps mentioned in bug 651093 comment 0 to make sure that this doesn't break anything?  Thanks!

::: widget/windows/nsFilePicker.cpp
@@ +1287,5 @@
>  
>  bool
>  nsFilePicker::IsPrivacyModeEnabled()
>  {
> +  bool usingPB;

Please initialize this to false, so that it would not have a random value inside it if mLoadContext is null.

@@ +1294,2 @@
>    }
> +  return usingPB;

With the usage of nsIPrivateBrowsingService removed, you can also remove this line at the beginning of this file:

#include "nsIPrivateBrowsingService.h"

::: widget/windows/nsFilePicker.h
@@ +137,5 @@
>    static void PickerCallbackTimerFunc(nsITimer *aTimer, void *aPicker);
>    static UINT_PTR CALLBACK MultiFilePickerHook(HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam);
>    static UINT_PTR CALLBACK FilePickerHook(HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam);
>  
> +  nsCOMPtr<nsILoadContext>  loadContext;

Nit: please use mLoadContext as the name.
Comment 10 Diana Koenraadt 2012-04-09 10:46:41 PDT
Thanks for the quick reply, will do!
Comment 11 Josh Matthews [:jdm] 2012-04-09 10:48:38 PDT
Comment on attachment 613337 [details] [diff] [review]
use reference to docShell's loadContext to determine privacy mode

Review of attachment 613337 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking good. I'll pass this on to someone who actually owns this code once the changes below are made :) Have you tried opening a random html file in normal mode, seeing if it shows up in Recent Documents, then entering PB mode, loading another file and making sure it's not present?

::: widget/windows/nsFilePicker.cpp
@@ +224,5 @@
>  
> +NS_IMETHODIMP nsFilePicker::Init(nsIDOMWindow *aParent, const nsAString& aTitle, PRInt16 aMode)
> +{
> +  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aParent);
> +  nsIDocShell* docShell = window->GetDocShell();

This should probably be |window ? window->GetDocShell() : NULL|. do_QueryInterface is null-safe, so we shouldn't need any other null checks.

@@ +1287,5 @@
>  
>  bool
>  nsFilePicker::IsPrivacyModeEnabled()
>  {
> +  bool usingPB;

This method can become |return mLoadContext && mLoadContext->UsePrivateBrowsing();| since there's a simplified getter at http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsILoadContext.idl#96.

::: widget/windows/nsFilePicker.h
@@ +54,5 @@
>  #undef _WIN32_IE
>  #define _WIN32_IE _WIN32_IE_IE70
>  #endif
>  
> +#include "nsILoadContext.h"

We can remove this include and use a forward declaration instead: |class nsILoadContext;|.

@@ +137,5 @@
>    static void PickerCallbackTimerFunc(nsITimer *aTimer, void *aPicker);
>    static UINT_PTR CALLBACK MultiFilePickerHook(HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam);
>    static UINT_PTR CALLBACK FilePickerHook(HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam);
>  
> +  nsCOMPtr<nsILoadContext>  loadContext;

mLoadContext
Comment 12 Diana Koenraadt 2012-04-09 11:26:33 PDT
Forward declaration yields error 'use of undefined type nsILoadContext' (both before and after #include block) so unless I'm mistaken including nsILoadContext.h is necessary.
Comment 13 Josh Matthews [:jdm] 2012-04-09 11:35:34 PDT
Including nsILoadContext.h in the CPP file is necessary, but forward declarations in headers are always preferable since they reduce build dependencies.
Comment 14 Diana Koenraadt 2012-04-09 11:40:14 PDT
Created attachment 613350 [details] [diff] [review]
Improvements from review have been processed

Tested and correct:
- printf of boolean on Ctrl+s
- menu > history item
- Windows 7 'recent items' behavior
Comment 15 Josh Matthews [:jdm] 2012-04-09 11:47:34 PDT
Comment on attachment 613350 [details] [diff] [review]
Improvements from review have been processed

Looks good; you'll want to upload another version with the patch metadata (author, commit message, etc.), so please make the forward declaration change at the same time when you do. Let's see what Jim says!
Comment 16 Jim Mathies [:jimm] 2012-04-09 13:16:43 PDT
Comment on attachment 613350 [details] [diff] [review]
Improvements from review have been processed

Does this means we are moving toward private browsing per tab? (Which would be awesome!)
Comment 17 Diana Koenraadt 2012-04-09 13:17:43 PDT
Created attachment 613368 [details] [diff] [review]
Proposed patch: use reference to docShell's loadContext to determine privacy mode
Comment 18 Josh Matthews [:jdm] 2012-04-09 13:19:04 PDT
Comment on attachment 613368 [details] [diff] [review]
Proposed patch: use reference to docShell's loadContext to determine privacy mode

This got an r+ from jimm, so I'll check it in.
Comment 19 Josh Matthews [:jdm] 2012-04-09 13:26:17 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/490b6732369e

And yes, jimm, per-window PB mode is under way!
Comment 20 :Ehsan Akhgari 2012-04-10 08:36:19 PDT
Thanks a lot for your patch, Diana!

https://hg.mozilla.org/mozilla-central/rev/490b6732369e

Let me know if you're interested to work on another bug about per-window private browsing.  :-)
Comment 21 Diana Koenraadt 2012-04-10 23:59:42 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #20)
> Thanks a lot for your patch, Diana!
> 
> https://hg.mozilla.org/mozilla-central/rev/490b6732369e
> 
> Let me know if you're interested to work on another bug about per-window
> private browsing.  :-)

Yes! :) Josh has already sent me an e-mail with some suggestions for bugs, this was one of them. Will look at that message again, but any other suggestions are welcome. Feel free to e-mail me.

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