722864 - nsFilePicker uses global PB service to decide whether to add choices to recent documents

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Josh Matthews [:jdm]]

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

[(no longer active)]

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]]

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

[(no longer active)]

(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

[(no longer active)]

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]]

Actually Diana has been looking into this bug already, I'm pretty sure.

Comment 6

[(no longer active)]

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

My fault really, should have taken the bug.

Comment 8

[Diana Koenraadt]

Attachment: use reference to docShell's loadContext to determine privacy mode

Comment 9

[(no longer active)]

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]

Thanks for the quick reply, will do!

Comment 11

[Josh Matthews [:jdm]]

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]

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]]

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]

Attachment: 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]]

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]]

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]

Attachment: Proposed patch: use reference to docShell's loadContext to determine privacy mode

Comment 18

[Josh Matthews [:jdm]]

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]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/490b6732369e

And yes, jimm, per-window PB mode is under way!

Comment 20

[(no longer active)]

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]

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