355555 - Provide visual feedback and override support for content blocked by parental controls

Status: NEW

(Firefox :: General, defect)

Description

[Scott MacGregor]

Part of the integration work with Vista's new parental control features.

When some (but not all) of the content on a web page gets blocked by vista's parental controls, we should have a UI (like our popup blocking UI) which notifies the user that content has been blocked. It should have a more options link that fires up Vista's parental control URL override dialog.

Comment 1

[Doug Turner (:dougt)]

the behavior right now is that that when loading a web page, the content is blocked by Vista's parental controls will not be displayed and there is no indication to the user (child) why this content wasn't displayed.

Comment 2

[Doug Turner (:dougt)]

Attachment: patch v.1

This is the backend functionality needed to support Windows Vista parental controls.  I have kept it pretty modular so that if a OS X or linux parental control feature appeared, we could plugin it in without too much trouble.

This patch needs ws cleanup, idl comments, etc.  The changes to browser/base/content/browser.js are for illustration only.

I would like to get some feedback on this patch, but I do not intended to check this version in until it is more cleaned up. :-)

Comment 3

[Doug Turner (:dougt)]

Attachment: patch v.2 (only mozilla/toolkit/components/parental-control)

slightly more cleaned up patch.

Comment 4

[Doug Turner (:dougt)]

Attachment: patch v.3

Comment 5

[Doug Turner (:dougt)]

in nsParentalControls::IsDownloadingBlocked, we should return PR_FALSE outside the WIN32 part.  In the patch, it is the other way around because I was testing.

Comment 6

[Kathleen :Brade]

Doug -- Can you attach a screenshot that shows what the user will see (with your patch) when a page, or part of a page, is blocked?

Comment 7

[Doug Turner (:dougt)]

nothing.  this is only backend stuff.  The UI will be in another patch.  I still need to talk to the UI folks, but I think what will happen is this:

1) if the entire page is blocked, we will use the Vista HTML that is returned.  This will look exactly like IE.

2) if only part of the the page is blocked, we will put up an info bar.  This will be very similar to the IE experience.

Both of these will allow the child account to request permission to view the content.

Doug

Comment 8

[Benjamin Smedberg]

Comment on attachment 242420 [details] [diff] [review]
patch v.2 (only mozilla/toolkit/components/parental-control)

>Index: Makefile.in

>+LIBXUL_LIBRARY = 1

Since this is LIBXUL_LIBRARY, you'll need to add it to toolkit/library/libxul-config.mk and toolkit/library/nsStaticXULComponents.cpp

>Index: nsIParentalControls.idl

>+[scriptable, uuid(b0495fcb-b55c-4cd9-82c0-0096e29de1d1)]
>+interface nsIParentalControls : nsISupports
>+{
>+  /**
>+   * Attaches this object to a nsIDOMWindow so that
>+   * we can monitor status and state changes during
>+   * web content loading.
>+   *
>+   * There is a one-to-one relationship between a
>+   * nsIDOMWindow object and a nsIParentalControls
>+   * object.
>+   *
>+   */
>+  void init(in nsIDOMWindow window);

This needs more documentation: in particular, should the client (browser) code only attach the parental controls to the toplevel window? Or to all subframe nsIDOMWindows as well? Does this object stay attached across navigation from one page to the next? If so, we should document that we want the "inner" nsIDOMWindow, not the "outer".

It seems to me that in an ideal trunk world this interface would be on the docshell itself, or attached to it pretty tightly. But since you're targeting this for 2.0.0.1 that's probably not possible. Boris, do you have an opinion about this?

>+  /**
>+   * @returns true if the content that is currently
>+   * displayed in the attached nsIDOMWindow has been
>+   * filtered.
>+   */
>+  readonly attribute boolean wasContentFiltered;

Does this flag get reset when the user navigates to a different page?

>+  /**
>+   * Check if downloading for the current user blocked.
>+   *
>+   * @returns true if downloading has been blocked for the
>+   * current user.
>+   */
>+  boolean isDownloadingBlocked();

Is this setting really per-window? It seems to me that this is more a global setting that should be gotten off a separate service object.

>Index: nsParentalControls.cpp

>+static const CLSID CLSID_WindowsParentalControl = {0xE77CC89B,0x7401,0x4C04,{0x8C,0xED,0x14,0x9D,0xB3,0x5A,0xDD,0x04}};
>+static const IID   IID_IWindowsParentalControl  = {0x28b4d88b,0xe072,0x49e6,{0x80,0x4d,0x26,0xed,0xbe,0x21,0xa7,0xb9}};
>+
>+// IWPCSettings (parental controls) is not in the SDK we
>+// currently use.
>+#if !defined(IWPCSettings)
>+#include "ole2.h"

What SDK is it in? Copying interfaces into our source tree is probably not kosher licensing, unless we have a pretty liberal redistribution license.

>+nsParentalControls::~nsParentalControls()
>+{
>+#ifdef WIN32
>+  if (mWindowsWebSettings)
>+  {
>+    mWindowsWebSettings->Release();
>+    mWindowsWebSettings = NULL;
>+  }
>+#endif
>+}

Why are we even building this code for non-win32? Let's not overplan for a potential mac/linux solution until they come along.

Also, mWindowsWebSettings should be a nsRefPtr<IWPCWebSettings> which removes the need for the manual refcounting.

>+  IWindowsParentalControls* spiWPC = NULL; 

Use nsRefPtr and getter_AddRefs for reference-counting sanity here.

>+nsParentalControls::RequestAllowance(PRBool *aResult)

>+    // Free up the memory
>+    for (i=0; i<count; i++)
>+    {
>+      nsMemory::Free((void*)urls[i]);
>+      free((void*)urls);
>+    }
>+  }

Use NS_Free in new code.

I'm pretty sure the free((void*)urls) needs to be outside the loop ;-)

Instead of creating the parentalcontrols object and then having to check whether we have a parental controls member inside, why not just fail to create the object if windows doesn't have any parental controls implementation?

Comment 9

[Doug Turner (:dougt)]

thanks for the review.  we are building this on non-windows platforms because we want to expose this object off of the browser widget.  I didn't think that testing for a property was as clean as simply just having a null impl.

I will better document to |init|.

wasContentFiltered does get reset.  I will better document that.

The last two APIs are more general, yet can be page specific.  I was thinking that isDownloadingBlocked might return true for some pages but not others.  if |init| is not called, the meaning would be is downloading blocked for any pages.  I will better document this.

However, |isWebFiltered| is much more general -- it really could be a pref, but i would have to poll to figure our if this preference ever changed.  Not sure if we really need to create a new interface for it, but maybe.

As far ask the SDK stuff goes... this interface is defined in the vista SDK in the file: wpcapi.h.  This is MIDL generated file which has no license that I can find associated with it.  The IDL is not distributed with the SDK.  This problem is not new and we have done similar things like this in the past.  The intent it is do it this way until we can all migrate to the latest Windows SDK.

New patch coming up.

Comment 10

[Doug Turner (:dougt)]

Attachment: patch v.3

does not include LIBXUL_LIBRARY pieces.  talking offline to benjamin about that piece.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 242776 [details] [diff] [review]
patch v.3

I assume this stuff will live in extensions/parental-controls, right?

Just looking at the API for now, and a little tiny bit at the code:

>Index: parental-controls/nsIParentalControls.idl

>+ * The Initial Developer of the Original Code is
>+ * Doug Turner <[e-mail]>.  All Rights Reserved.

I believe the Initial Developer is the Mozilla Corporation, with Doug as the Original Author listed under Contributors.  But IANAL.

>+   * Attaches this object to a nsIDOMWindow so that status
>+   * and state changes changes during web content loading
>+   * can be monitored. This attachment will persist until
>+   * |init| is called again passing |null|.

OK.  At what point does this need to happen for proper operation?  What's the ownership model?  Will this create a reference cycle until it's manually broken?  Is it allowable to call init() with a non-null window multiple times?

>+   * The |nsIDOMWindow| passed should be the topmost "inner"
>+   * nsIDOMWindow associated with the browser.

I'm really not sure what this means (esp. as the existence of "inner" windows is a Gecko implementation detail that should not be visible to Gecko clinets.

What does "associated with the browser" mean?

Can a chrome window be passed to init()?

>+   * wasContentFiltered
>+   *
>+   * if |init| was successfully called with a valid top
>+   * level window

What does "top level window" mean here?

>+   * This should only be called after the page has
>+   * completely loaded.

What if that never happens (progressive JPEG, document.written pages, server push, AJAX apps)?  The same applies to all other places where this occurs.

>  |wasContentFiltered| will be reset
>+   * during a new page load.

Meaning what?

>+   * @returns true if the content that is currently
>+   * displayed in the attached nsIDOMWindow has been
>+   * filtered.

In which part of the DOMWindow?  What if it has subframes?  What if the document in the attached DOMWindow is done loading but some of the subframes are loading right now?

>+  readonly attribute boolean wasContentFiltered;

I think "contentWasFiltered" would make more sense....

>+   * requestAllowance

Maybe requestLoadPermission?

>+   * Check if downloading for the current user blocked for
>+   * the top level window passed to |init|.

I can't figure out what that sentence means.  It seems to be missing a verb somewhere.

What does "downloading" mean in this context?

>  If |init| was
>+   * not called, |isDownloadingBlocked| will check to see if
>+   * downloading is blocked, in general, for the user (on
>+   * some systems this may be the same).

I'm not that happy having an object which is partly usable if not initialized....  Maybe this functionality should live on a separate (service) object?

>+  boolean isWebFiltered();

Again, maybe a service.

>Index: parental-controls/nsParentalControls.cpp
>+  // Attach ourselves as a progress listener.
>+  nsCOMPtr<nsIScriptGlobalObject> scriptObj(do_QueryInterface(mWindow));
>+  if (!scriptObj)
>+    return NS_ERROR_FAILURE;

Why not just do_GetInterface an nsIWebNavigation from the window, then do_GetInterface an nsIWebProgress from that?  I'm really not happy with this code knowing about nsIScriptGlobalObject or nsIDocShell.

>+nsParentalControls::OnStateChange(nsIWebProgress* aWebProgress,

Shouldn't you use this (in particular STATE_STOP or something) to look at the HTTP channel status?  I'm not sure why you're using OnStatusChange....

>+    // The only state change we care about is the event that
>+    // will alert us that a new top level load is occuring.
>+    // When this happens, we will clear out our cached info
>+    // about the past load.

So when loads happen in subframes we don't care?

>+    // There must be an easier way! 
>+    if (aProgressStateFlags & STATE_START &&
>+        aProgressStateFlags & STATE_IS_REQUEST &&

You could look for STATE_IS_DOCUMENT/STATE_IS_WINDOW instead of looking for the channel flags.

Comment 12

[Doug Turner (:dougt)]

bz, thanks for the comments regarding my idl comments.  probably could spend alot more time better documenting what each call does, etc.  the interface mirrors what the nsISecureBrowserUI.idl does, which has absolutely no comments -- not to say that this new code should or shouldn't, but rather to point out what I am trying to do. :-)  Will split the IDL into something that attaches to a window and a service, doc it better, etc.

Why do you think this should be in extensions?

> Why not just do_GetInterface an nsIWebNavigation from the window, then
> do_GetInterface an nsIWebProgress from that?  I'm really not happy with this
> code knowing about nsIScriptGlobalObject or nsIDocShell.

coding by example.  :-/  I can clean that up.


> Shouldn't you use this (in particular STATE_STOP or something) to look at the
> HTTP channel status?  I'm not sure why you're using OnStatusChange....

When using OnStateChange, i would not get the failure 450 status of all (or even most) http channels.  Only once in a while, would I get a OnStateChange whose request was a httpchannel with the status of 450.  

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Why do you think this should be in extensions?

I don't, offhand.  Is it going in a new toplevel dir, then?

> Only once in a while, would I get a OnStateChange
> whose request was a httpchannel with the status of 450. 

Hmm...  How did that correspond with the number of actual HTTP channels around?  There should be a OnStateChange with STATE_STOP for every single HTTP channel, I would think.

Comment 14

[Doug Turner (:dougt)]

this was going to be checked in under toolkit/components/.  I was hoping to avoid any dependencies outside of toolkit or browser.

regarding the number of actual HTTP channels around, i did not check.  I assumed it was less as content was failing to load yet I did not see those status codes.  I switched over to OnStateChange, saw what I wanted to see, and ignored the problem. 

Comment 15

[Boris Zbarsky [:bzbarsky]]

> this was going to be checked in under toolkit/components/.

Ah, sure.  If we think this should be just a toolkit thing not a general Gecko thing.

I think it's worth figuring out the notification issue; OnStatusChange is just the wrong notification to use for this.

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

+      urls[i] = ToNewUnicode(subURI);

should be UTF8ToNewUnicode. and NS_ConvertUTF8toUCS2 won't work on trunk (use UTF16 instead of UCS2)

+  virtual ~nsParentalControls();

shouldn't this be a private nonvirtual destructor? that'd save some codesize (and the private would ensure that when people want to subclass this, they notice that they may need to make it virtual)

I agree that this shouldn't be in browser/.

is there a specific reason not to add yourself to the root docloader? if there is, why do the initialization with a window, instead of with a docshell or web progress?

content failing to load should still have OnStateChange called...

Comment 17

[Doug Turner (:dougt)]

I will fix up the string changes.

> is there a specific reason not to add yourself to the root docloader? if there
> is, why do the initialization with a window, instead of with a docshell or web
> progress?

Do you have any samples of what you are talking about?  I am doing something similar to what the secure browser (ssl icon) does.  Maybe that should change too?

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

well, I don't know how your is expected to be used. hm... I guess you need to store the list of URIs per window/tab, so what you do seems better than adding to the root docloader.

Comment 19

[Doug Turner (:dougt)]

Attachment: patch v.4

1) fixed up strings and moved the destructor to be non-virtual and private per Christian.

2) incorporated BZ's comments about the comments.  I can see why it was hard to understand what I was trying to do with the interface.  I split the interface into three:  a setup interface, and settings interface, and a control interface.  It should make more sense now.  I still do not know how to correctly define in english what the variable that I expect to be passed to |init| really is.  I mean, i know it is the browser's content window... but i do not know if that is the right lingo to use in the interface.  bz, can you suggest something here?  In the browser.xml i plan to do something like:

            this.parentalControl.init(this.contentWindow);

3) renamed wasContentFiltered to contentWasFiltered.

Comment 20

[Boris Zbarsky [:bzbarsky]]

That last patch doesn't address most of my concerns, for what it's worth...

As for how to describe the argument to be passed to init(), it sounds like you want a "toplevel content window".

It's still very unclear what will be done with this window from the existing comments.

Comment 21

[Doug Turner (:dougt)]

bz, read the last comment -- I am asking for your help :-)

Comment 22

[Boris Zbarsky [:bzbarsky]]

Er... I thought I tried to address that part in comment 20....

Comment 23

[Doug Turner (:dougt)]

thanks bz, misread your comment... i think.   I do indeed want the top level content window.

Outside of the API commenting, are there other things I should address before posting a new patch?

Comment 24

[Boris Zbarsky [:bzbarsky]]

Kinda hard to tell because I'm still trying to figure out what the code is trying to do...  

Comment 25

[Doug Turner (:dougt)]

it is just listening for 450 status response codes during a page load.  if we see one, we know that content has been blocked by parental controls iff the mWindowsWebSettings is non-null.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Yes, I got that much.  But what's the expected interaction with other code?  Do we not care about things that are not "during a page load" (e.g. AJAX stuff)?  What does "during a page load" mean for subframe navigation?  

These are the sort of things I was trying to get clarified in the API comments...  Once we get that sort of thing sorted out, we can look at whether the implementation is doing the right thing given the intent.

Comment 27

[Doug Turner (:dougt)]

I am pretty sure that we don't care about stuff that happens after the page load.  This behavior mirrors https mixed content warnings, right?  If we detect non-secure content loading when the main URI's was a secure load, we warn the user.  however, if the document subsequently uses an XMLHttpRequest to a non-secure site, we do not warn the user.  (probably related to bug 62178).  

That being said, XMLHttpRequest have the benefit domain checking thus allowing us to rarely see this problem.  

as for "subframes", i was imagining that |contentWasFiltered| and related methods would be called after all content was loaded (similar to what we do when we display the lock icon).

thoughts?

Comment 28

[Boris Zbarsky [:bzbarsky]]

> This behavior mirrors https mixed content warnings, right? 

I'm pretty sure there's a bug on the fact that we don't do HTTPs mixed content warnings for AJAXy stuff.  We really need to fix that.

> i was imagining that |contentWasFiltered| and related methods would be called
> after all content was loaded 

But the point is that _after_ this someone can click a link in a subframe.  Then what?

And again, the question of what happens for pages that never "finish" from comment 11 still stands.

Comment 29

[Doug Turner (:dougt)]

> We really need to fix that.

not in this bug. :-)

> But the point is that _after_ this someone can click a link in a subframe. 

isn't that a new load?

> What if that never happens (progressive JPEG, document.written pages, server
> push, AJAX apps)?  The same applies to all other places where this occurs.

Similar to the security lock icon which is not displayed until the page is completed loaded.  

Comment 30

[Doug Turner (:dougt)]

maybe it would be more clear if I pointed out that I plan to attach a parental control object to the browser widget.  See the first patch 241981 and look for toolkit/content/widgets/browser.xml

Comment 31

[Boris Zbarsky [:bzbarsky]]

> not in this bug. :-)

I'm not saying you need to fix the security UI.  I'm saying you should not put the same bogus assumptions that the _UI_ makes into the _interfaces_ here.

> isn't that a new load?

Not clear from your API comments.

> Similar to the security lock icon which is not displayed until the page is
> completed loaded.  

Again, I feel that's a bad design decision.

Comment 32

[Boris Zbarsky [:bzbarsky]]

> maybe it would be more clear if I pointed out that I plan to attach a parental
> control object to the browser widget

No, I understand that.  But you're building assumptions about how that object will work (or not work, as the case may be) into this interface, which seems suboptimal to me.  In particular, someone who wants a parental control object that works better wouldn't be able to do it.

Comment 33

[Doug Turner (:dougt)]

boris, this is a interface between the toolkit and the browser widget.  it is not public to anyone else.  If someone wants to improve it, they are free to go ahead and change the interface and implemenation.  I do not expect nor encourage people implement this interface anywhere else.

Comment 34

[Boris Zbarsky [:bzbarsky]]

This brings us back to the discussion of what "toolkit" is that we were having this morning....  If this code is not meant for use by anyone outside of mozilla/browser, it should live in mozilla/browser.  Again, in my opinion.

At which point, I won't have much to say about the interface, since it won't affect either me or anything I care about.  I'd be happy to look at the implementation, of course.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Or is the point that it needs to live in toolkit because tabbrowser does?

Comment 36

[Boris Zbarsky [:bzbarsky]]

Maybe I should clarify my point of view a little...

If we're creating an interface for general toolkit/XULRunner consumers, then the interface as currently attached to this bug is not nearly clear enough, in my opinion.

If we're not creating such an interface, then (1) Why are we placing it in toolkit? and (2) Why are we asking me about it?   Sorry to be so blunt, but this is kinda out of the way of the things I normally work on, and it's been taking up a _lot_ of my time this week...  If it's not a general embedding interface, then I'm not the best person to comment.

Comment 37

[Doug Turner (:dougt)]

There is only one interface that will be for general toolkit/XULRunner consumers and that is nsIParentalControlsSettings.  This is the interface that is going to tell a user if "download" is blocked or if "parental controls" is enabled.  

The rest of the interfaces are for the exclusive use of the browser widget and browser.

the interfaces are going to be used in a couple places:

it is going to be initialized in
   (via nsIParentalControlsSetup):
   toolkit/content/widgets/browser.xml

it is going to be called to check to see if anything was filtered in
   (via nsIParentalControls):
   browser/base/content/browser.js

it is going to be called to check if "download" has been blocked in
   (via nsIParentalControlsSettings):
   tbd, but i think it is going to be a few places mostly under toolkit.

Comment 38

[Doug Turner (:dougt)]

Comment on attachment 242893 [details] [diff] [review]
patch v.4

bsmedberg, can you review the above comments and suggest next steps?

Comment 39

[Benjamin Smedberg]

Most of my previous architectural/documentation concerns are still valid. I don't know from the documentation of the interfaces what is supposed to happen in the following situations:

1) Client calls init() with a domwindow that has already loaded, or is in the middle of loading. Does init() succeed or fail? Do we get valid data from the methods?

2) Client calls init() with a domwindow and then loads a URL. The user subsequently navigates to a new page. Is the information still valid? Is the client supposed to call init() again?

3) Client calls init() with a domwindow and then loads a URL. The user subsequently navigates within a subframe to a new page. Is the information updated? Is the client supposed to call init() again?

4) Does this code intercept and notify about blocked images or just blocked "content" (HTML) pages?

It seems to me that a framework that notified about blocked loads as they happen through a callback mechanism (nsIObserver or something more special-purpose) would be easier to use. Then all you have to do is break reference cycles.

Comment 40

[Doug Turner (:dougt)]

benjamin, who is the "client" in questions 1-3?  The only client should be the browser widget (toolkit/content/widgets/browser.xml).  Take a look the first patch to see how this hooks up to the widget.  Would you just like to see more documentation on the IDL?

question 4:  it is completely up to the underlying parental control system.  

Your assertion about being easier using an observer makes sense if you didn't have to collected blocked URIs and their associated window to implement |requestAllowance|

Comment 41

[Benjamin Smedberg]

No matter whether the browser widget is the only client we should have accurate and complete usage docs in the interface. We also need to find a way to unit-test this. Please talk to sayrer about what test framework makes the most sense in this context (especially since we'll need to run the test on a limited-privilege account).

Comment 42

[Doug Turner (:dougt)]

how does this sound for the setup interface:

interface nsIParentalControlsSetup : nsISupports
{
  /**
   * Initialize with a top level content window so that
   * status and state changes during a page load can be
   * monitored.
   *
   * |init| must be called before loading any content in
   * order to ensure accurate monitoring.
   *
   * The object will own a reference to the nsIDOMWindow,
   * and continue to monitor all page loads including
   * subframes, until |init| is called again passing null.
   */
  void init(in nsIDOMWindow window);
};

Comment 43

[Doug Turner (:dougt)]

apparently for images there is no OnStatusChange message sent to our WebProgressListener.  So, if you go to a page like www.meer.net/~dougt/porn and it has a bunch of images to porn, vista probably will block those image loads from happening, but we will not hear about it in our WebProgressListener.  The end result would be the page that is broken and there is no notification to the user that this has happened.

any thoughts on how to avoid this without a rewrite to the imgRequest.

Comment 44

[Doug Turner (:dougt)]

Attachment: patch v.5

incorporating bz/bsmedberg requests.  Making this win32 only.

Comment 45

[Doug Turner (:dougt)]

Attachment: patch v.6

After talking with stuart, this is the approach to expose the network status from the imgIRequest with is past to the WPL.

Comment 46

[Stuart Parmenter]

Comment on attachment 245148 [details] [diff] [review]
patch v.6

can you add some comments about the saving of the status?

Comment 47

[Doug Turner (:dougt)]

Comment on attachment 245148 [details] [diff] [review]
patch v.6

Checking in src/imgRequest.cpp;
/cvsroot/mozilla/modules/libpr0n/src/imgRequest.cpp,v  <--  imgRequest.cpp
new revision: 1.91; previous revision: 1.90
done
Checking in src/imgRequest.h;
/cvsroot/mozilla/modules/libpr0n/src/imgRequest.h,v  <--  imgRequest.h
new revision: 1.35; previous revision: 1.34
done
Checking in src/imgRequestProxy.cpp;
/cvsroot/mozilla/modules/libpr0n/src/imgRequestProxy.cpp,v  <--  imgRequestProxy.cpp
new revision: 1.57; previous revision: 1.56
done

Comment 48

[Doug Turner (:dougt)]

Comment on attachment 244919 [details] [diff] [review]
patch v.5

i am going to put up a new patch after I talk with belzner about the UI next week.

Comment 49

[Marcia Knous [:marcia]]

Doug: Did you ever connect with beltzner regarding the UI and do you have a new patch? Thanks.

Comment 50

[Doug Turner (:dougt)]

nope.  mike needs to comment, and I need to get him the data to comment on. 

Comment 51

[Doug Turner (:dougt)]

This will require string changes and therefore I didn't continue implementing it for FF2. 

I am going to need someone to pick up this patch and run with it.

Comment 52

[Jay Patel [:jay]]

Not blocking for 1.8.1.2, but we need marketing/products input on whether such
string changes might be worth doing for FF2 to support Vista parental controls.

schrep:  Is this something we would take in a future release if we coordinated
the string changes with all locales?

Comment 53

[Jim Mathies [:jimm]]

This patch seems stalled out - no comments in six months? Is there anything I can do to help finish this up for FF3?

Jim

Comment 54

[Doug Turner (:dougt)]

sure Jim.  If you have the time, we need to cleanup and land the parental control part of this patch, work with the UI team to figure out how to best show these types of notifications (right now we use the notification bar), work with intl to ensure that our strings can get in, and help QA build test cases for this stuff.

I think benjamin had some issues with toolkit verses browser or something, but they probably can be cleared up with some documentation.

Comment 55

[Doug Turner (:dougt)]

mass reassigning to nobody.

Comment 56

[Boris Zbarsky [:bzbarsky]]

The imgRequest changes here look wrong, since mRequest is not a channel.  If we actually need the channel status, we're not getting it.  Part of the problem is that the patch in this bug isn't what actually landed.  I've filed bug 455353 on this.

Comment 57

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

From bug 455353: 

We need to back this out until we figure out what the right fix is --
having bug 355555 still open and with a maybe-patch checked in is not a good
state.

Comment 58

[Jim Mathies [:jimm]]

Attachment: parental ctrls notifications v.1

First rev of this. Current patch takes care of notifications in the browser and overriding blocked content for inline frames and pages. I decided to use an observer event for propagating info up to the chrome.

I haven't found a way of solving blocked inline content yet. Status codes seem pretty buried for inline content loads of images, flash, sound, etc.., if I could find a central spot where I have the actual code and the a shell or window, I could send an event. Haven't found the right spot yet though.

Even if that isn't possible, this patch as it stands improves usability and gives us better parity with IE in this type of environment.

Comment 59

[Jim Mathies [:jimm]]

Attachment: parental ctrls notifications v.1

Benjamin, would mind looking this over when you get a chance and provide some feedback on the implementation?

Comment 60

[Benjamin Smedberg]

Comment on attachment 388923 [details] [diff] [review]
parental ctrls notifications v.1

It should be documented that the interfacerequestor which leads to GetHWND is a chrome (toplevel) window, not the content window (especially since in the future the content HWND may be unavailable).

I'm really not the right person to review the rest of this: bz for the docshell bits and some browser peer for the Firefox bits (it perhaps needs a UI-review as well).

Comment 61

[Jim Mathies [:jimm]]

(In reply to comment #60)
> (From update of attachment 388923 [details] [diff] [review])
> It should be documented that the interfacerequestor which leads to GetHWND is a
> chrome (toplevel) window, not the content window (especially since in the
> future the content HWND may be unavailable).
> 

Will do.

Comment 62

[Jim Mathies [:jimm]]

Comment on attachment 388923 [details] [diff] [review]
parental ctrls notifications v.1

bz -> docshell observer addition.

I'll get the added hwnd commenting in the next rev.

Comment 63

[Boris Zbarsky [:bzbarsky]]

I don't understand this patch.  Isn't the point to do something about non-toplevel requests that get blocked via parental controls?  If so, why are we only notifying about one URI per pageload?  Why are we hooking this into docshell state notifications (so that resources in resource documents, or images, or XHR loads won't trigger the notification)?

Or did I misunderstand either what the code is doing or what we're trying to do?

Comment 64

[Jim Mathies [:jimm]]

(In reply to comment #63)
> I don't understand this patch.  Isn't the point to do something about
> non-toplevel requests that get blocked via parental controls?  If so, why are
> we only notifying about one URI per pageload?  Why are we hooking this into
> docshell state notifications (so that resources in resource documents, or
> images, or XHR loads won't trigger the notification)?


The plan was to better handle both. Currently we don't notify for any blocked content except in the download manager. When I first started looking at this, I was hoping the doc shell state notifications would give me information about inline content, but it turns out that wasn't the case. That information doesn't propagate up far enough to where I can access it (AFAICT).

My plan was to address pages and sub frames (which is what this patch does) and then implement a second patch for inline content if I can figure out a way to reliable detect 450 status codes in a generic way for all content, not just a particular type like images.

With this patch, if a page or a sub frame gets blocked, we put up the notification bar offering the user the chance to override the blocked content and reload the page. That covers a majority of cases and improves overall user experience.

As far doc shell state notification integration goes, that was the reason I requested r? from Benjamin and then you, I'm more interested in getting feedback on the idea rather than a final review.

Comment 65

[Boris Zbarsky [:bzbarsky]]

It seems to me that if we're trying to do this for all requests, in general, it would be better to do it on the http level directly (that is, send the notifications from nsHttpChannel).

You'll still have problems with images, since image loads are not tied to a particular window, so given an image HTTP response you have no idea which window it was being loaded in (and it might be more than one).  But for everything else, that should work pretty well, I would think.

Comment 66

[Jim Mathies [:jimm]]

(In reply to comment #65)
> It seems to me that if we're trying to do this for all requests, in general, it
> would be better to do it on the http level directly (that is, send the
> notifications from nsHttpChannel).
> 
> You'll still have problems with images, since image loads are not tied to a
> particular window, so given an image HTTP response you have no idea which
> window it was being loaded in (and it might be more than one).  But for
> everything else, that should work pretty well, I would think.

Thanks for the feedback, I'll take a look at the image code.

Comment 67

[Mike Beltzner [:beltzner, not reading bugmail]]

IMO we should skip the notification bar if parenting to the window will be tricky. A better way would be:

 - get an icon to represent blocked content (for now use something temporary, file a follow up to replace)
 - if a user tries to visit a blocked page, we should show a DNS-error like page, but have a new message saying that the page is blocked by local settings or something
 - for inline content, just replace the DIV or IMG with the blocked content icon

Since we expect a lot of Windows users to migrate to Windows 7, I think we should consider this as part of Windows 7 readiness and get it for mozilla-1.9.2/Firefox 3.6.

Doable, Jim? We can talk UI next week.

Comment 68

[Jim Mathies [:jimm]]

(In reply to comment #67)
> IMO we should skip the notification bar if parenting to the window will be
> tricky. A better way would be:
> 
>  - get an icon to represent blocked content (for now use something temporary,
> file a follow up to replace)
>  - if a user tries to visit a blocked page, we should show a DNS-error like
> page, but have a new message saying that the page is blocked by local settings
> or something

The notification bar isn't so much for alerting the user that something was blocked, it's for providing the option of overriding the blocked content and reloading the page. The windows proxy already provides a blocked page filler similar to our dns page. We can override that if we want, although it's really not needed unless we feel the "brand" should match other Fx error pages. What do you think?

>  - for inline content, just replace the DIV or IMG with the blocked content
> icon
> 
> Since we expect a lot of Windows users to migrate to Windows 7, I think we
> should consider this as part of Windows 7 readiness and get it for
> mozilla-1.9.2/Firefox 3.6.
> 
> Doable, Jim? We can talk UI next week.

Yep. Once I finish implementing bz's suggestion of the http request level notification this'll pretty much be done. 

FYI - blew my back out a week ago so won't be at the work week, but should be around on irc. Been on pto all this week and last.

Comment 69

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #68)
> The notification bar isn't so much for alerting the user that something was
> blocked, it's for providing the option of overriding the blocked content and
> reloading the page. The windows proxy already provides a blocked page filler
> similar to our dns page. We can override that if we want, although it's really
> not needed unless we feel the "brand" should match other Fx error pages. What
> do you think?

I'm fine to let the "brand" be the Windows Parental Controls blocking things, as it helps users know where to go to override. Can we see a screenshot?

Comment 70

[Mike Beltzner [:beltzner, not reading bugmail]]

Jim: no additional strings would be needed here, if we just pass through the content from the proxy, right?

Comment 71

[Jim Mathies [:jimm]]

(In reply to comment #70)
> Jim: no additional strings would be needed here, if we just pass through the
> content from the proxy, right?

Additional strings as in, strings in addition to what's in the current patch? In that case, no additional strings are needed. The alert bar has some new strings associated with it though that will be needed if the functionality in the last patch lands.

I read the note about a shared code string freeze for 1.9.2, but didn't see anything about browser strings.. do we have one coming up? FYI this is next on my list once I get that dde installer work finished tomorrow.

Comment 72

[Jim Mathies [:jimm]]

Attachment: full page blocked screenshot

> I'm fine to let the "brand" be the Windows Parental Controls blocking things,
> as it helps users know where to go to override. Can we see a screenshot?

Comment 73

[Jim Mathies [:jimm]]

Attachment: netwerk-toolkit patch v.1

Bz, curious if you can offer some feedback on this. I've moved the notification code down into netwerk/http (and I think I'll need it in ftp as well as the pc proxies work on both) and I've replaced the doc shell in the report with the interface requestor for http callbacks. The idea would be to decipher what that interface supports up in browser - if it's a doc shell, I can tie the alerts bar to a specific window, if it's something else like an XHR request, I can display the alert in the foreground window.

Curious what your thoughts are? I'd really like to tie all blocked content to specific window so that I can reload it if the user overrides the blocked content, but it looks like that wont be possible in all cases.

Comment 74

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #71)
> Additional strings as in, strings in addition to what's in the current patch?
> In that case, no additional strings are needed. The alert bar has some new
> strings associated with it though that will be needed if the functionality in
> the last patch lands.

We've passed string freeze, so unless those strings were already landed on 1.9.2 we can't take them for this version. I thought that we weren't showing the alert bar anymore, though, and instead just passing the content straight through from the Windows proxy, as per attachment 399797 [details]

I think this needs to be in the beta if we're going to ship it, though, so I'm moving this to a P1 blocker.

Comment 75

[Jim Mathies [:jimm]]

(In reply to comment #74)
> (In reply to comment #71)
> > Additional strings as in, strings in addition to what's in the current patch?
> > In that case, no additional strings are needed. The alert bar has some new
> > strings associated with it though that will be needed if the functionality in
> > the last patch lands.
> 
> We've passed string freeze, so unless those strings were already landed on
> 1.9.2 we can't take them for this version. I thought that we weren't showing
> the alert bar anymore, though, and instead just passing the content straight
> through from the Windows proxy, as per attachment 399797 [details]
> 
> I think this needs to be in the beta if we're going to ship it, though, so I'm
> moving this to a P1 blocker.

The windows proxy takes care of displaying a page indicating the page was blocked and offers the chance to override.  That works now (and has since 3.0). This bug was all about giving people an alert bar and the ability to override inline content that gets blocked, which the proxy doesn't take care of. There's really no point in this blocking if we can't get the strings in, it should just land on trunk once the patch is complete and release with the next branch.

Comment 76

[Boris Zbarsky [:bzbarsky]]

> Curious what your thoughts are?

General thoughts:

1)  Is the reason you're putting this into HTTP instead of, say, using web
    progress listener notifications, that you want to be able to see
    "background" requests (e.g. various prefetch loads, offline cache updates,
    XBL resources, XHR, <video> seek requests, that sort of thing)?  I assume
    the UI will deal sanely with possibly hundreds to thousands of denied loads
    (see "video seek requests")?
2)  If it does need to be down in HTTP, do you really want to limit to the
    _channel_'s callbacks (which are usually null), or do you also want to use
    the _loadgroup's_ callbacks (which are usually something much more useful)?
3)  Does the consumer really need the full callbacks, or would just getting the
    nsILoadContext from the callbacks be good enough?

Note that for content loaded by untrusted stuff (e.g. web pages) we can _always_ tie it to a specific window.  Otherwise the stop button wouldn't work on it.  ;)

Comment 77

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 401880 [details] [diff] [review]
netwerk-toolkit patch v.1

r- pending answers to the questions in comment 76.

Comment 78

[Robert Strong (they/them - no direct email)]

Not a shell integration bug. I suspect this is a Firefox -> General bug but moving to General since this would likely touch multiple areas of Firefox.