81263 - nsIWebBrowserSetup needs a way to switch on/off images

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: APIs, defect, P1)

Description

[Gagan]

From bug 69453

Comment 1

[Judson Valeski]

I think it's going to be too expensive for the docshell (or some other
nsIContentPolicy object w/ a 1to1 relationship with the docshell) to respond to
global nsIContentPolicy callbacks. Also, to do this correctly, we need some
window context which would mean walking nsIContentPolicies DOM chain up to the
nsIDOMWindow, then iterating over all the docshell windows (sub-frames) doing
window compares to ensure that the given nsIDOMElement indeed belongs to the
given top-level window.

would it make sense to have nsIContentPolicy hand out a nsIDOMWindow or
nsIDOMDocument also? I'm still wondering if it would be better to have the
content policy deal w/ the window model. the idea of multiple windows fielding
callbacks that may, or may not, belong to them seems wrong. it seems that this
window level context would be helpful.

during nsIContentPolicy registration, you could specify an optional nsIDOMWindow
param that would indicate you only wants callbacks for the specified window.

Comment 2

[vidur (gone)]

I'm not sure if expense is an issue here, but I like the idea of a nsIDOMWindow
as a context parameter to NS_CheckContentPolicy. There are several cases I've
heard in which embedders want to control policy on a per-window basis.

Comment 3

[Judson Valeski]

After talking this over w/ shaver on irc, he wants to keep nsIContentPolicy more
global than my suggestion of maintianing a list of windows in it would make it.
I can live w/ that; it wasn't designed for window level callbacks, no need to
dirty it up. That being the case, we can go one step further and provide the
nsIDOMWindow param in the methods to provide more context, but we can also take
the existing nsIDOMElement and dig a nsIDOMWindow out of that on the callback
side, so maybe we should just do that and tell people wanting window context to
do the digging.

Comment 4

[Mike Shaver (:shaver emeritus)]

I think all I need is for nsIWebBrowserSetup to grow a matching getProperty.  Is
that doable?

Comment 5

[Mike Shaver (:shaver emeritus)]

Jud has ok'd adding getProperty to the nsIWebBrowserSetup interface, so I'm
going to do that.  This bug's mine too, then.

Comment 6

[Mike Shaver (:shaver emeritus)]

Attachment: sample JS component (non-functional) to illustrate strategy

Comment 7

[Mike Shaver (:shaver emeritus)]

The attached sample won't work, because it has to go through non-scriptable
interfaces and methods (nsIScriptGlobalObject::getDocShell, for one), but it
shows what we'll end up doing.

I'll hack away on nsIWebBrowserSetup::getProperty now.  (Say my name, docshell!)

Comment 8

[Mike Shaver (:shaver emeritus)]

Well, that's obviously not going to work; I have to ask the docshell for things,
because I can't get from it to the web browser setup thing.  No biggie, but I
just noticed that.

Comment 9

[Mike Shaver (:shaver emeritus)]

In fact, I don't even need to add that method to nsIWebBrowserSetup.  Do you
still want it, Jud?

Comment 10

[Mike Shaver (:shaver emeritus)]

Attachment: content policy enforcer for embedding

Comment 11

[Mike Shaver (:shaver emeritus)]

Latest patch builds for me.  I can't test it until we get the callsites fixed,
but I'll poke again then.

(30 lines of C++ in the meat of it.  I think that means that Jud and I came to a
good decision on the APIs.)

Comment 12

[Judson Valeski]

no need for the getproperty on nsIWebbrowsersetup then. coo'

Comment 13

[Judson Valeski]

- nsWebBrowserContentPolicy.cpp
* the constructor needs to call NS_INIT_ISUPPORTS() in order to be used as an
XPCOM component.

I'll be posting the allowImages attribute mods to the docshell and the
webbrowser setup mods to add the image enum shortly.

Comment 14

[Mike Shaver (:shaver emeritus)]

Blech.  That's just sloppy of me.  Sorry.  I need IMPL_ISUPPORTS, too.  What a
loser.

I'll post another patch in a little while.

Comment 15

[Judson Valeski]

Attachment: diffs to docshell, webbrowser and nsIWebBrowserSetup.

Comment 16

[Judson Valeski]

the attatched diffs:
- add the allowImages attribute to nsIDocShell and its impl
- add the ALLOW_IMAGES enum to nsIWebBrowserSetup (superceeds the
nsIWebBrowserSetup.idl mods in shaver's previous patch).
- add the ALLOW_IMAGES enum handling to nsWebBrowser.cpp

Comment 17

[Judson Valeski]

Attachment: grrr. the nsDocShell.cpp provided in my previous patch won't compile. this nsDocShell.cpp diff does compile. sorry.

Comment 18

[Mike Shaver (:shaver emeritus)]

Attachment: better patch for the content policy enforcer

Comment 19

[Mike Shaver (:shaver emeritus)]

New patch plays more nicely with XPCOM, and removes the nsIWebBrowserSetup.idl
changes in favour of Jud's.

Still haven't had a chance to test it with the call-site changes, etc.

Giving this to Jud, because I'm a loser, and his tree has most of the goodness.

Comment 20

[Judson Valeski]

Attachment: updated patch.

Comment 21

[Judson Valeski]

Summary of most recent patch:
- creates a nsWebBrowserContentPolicy class which registers for content policy
notifications. as more content policy callbacks come on-line, more than just
image blocking will be possible through this.
modifies nsIDocShell and nsIWebBrowserSetup to handle the image flag.

Comment 22

[Conrad Carlen (not reading bugmail)]

r=ccarlen

Comment 23

[rpotts (gone)]

sr=rpotts.

Just a couple of comments...
It looks like you have an extra 'return NS_OK' in UnregisterContentPolicy(...).

Also, it would be nice if DeleteCatagoryEntry(...) allowed a null out parameter
for the previous catagory entry - that way you wouldn't have to pass in that
XPIDLString that jsut gets thrown away :-(

Other than that, everything looks good.

-- rick

Comment 24

[Judson Valeski]

I removed the extra return NS_OK, and I filed
http://bugzilla.mozilla.org/show_bug.cgi?id=82000 on the bogus
DeleteCategoryEntry param in nsICategoryManager.

Comment 25

[Judson Valeski]

last patch is in, and I've added the new files.

Comment 26

[David Epstein]

SETUP_ALLOW_IMAGES is in nsIWebBrowserSetup.idl. Support for handling it is in 
nsWebBrowser.cpp. Image flags in nsDocShell.cpp. Content policy is enforced in 
nsWebBrowserContentPolicy.cpp (image tag case stmt).