201885 - ShouldLoad does not indicate why load should not occur

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We have some consumers of ShouldLoad (<img> elements to be exact) that need to
know _why_ the load should not occur.  More precisely, they need to
differentiate "no images should be loaded" and "this image should not be loaded"
-- the alt text behavior should be different in those two cases.

Right now, this is accomplished by layout "snooping" the "disable image loading"
pref that content policy uses to disable image loading.  This introduces a
dependency of the Gecko core on the particular nsIContentPolicy impl in use here
-- if someone writes a different image loading content policy using different
prefs, the Gecko back end will "break".

This doesn't seem so good to me.  ;)  So would it be possible to change the
boolean retval into an enum of some sort?  Possibly taking the values:

load
dontLoadThisItem
dontLoadThisTypeOfItem
prompt (???  We disabled this because it was implemented poorly; not sure how     
             the API should expose it...)

Thoughts?  (Apart from, "Boris, those enum value names are crap!")

Comment 1

[Michiel van Leeuwen (email: mvl+moz@)]

This return values would be quite image-specific, right? Ok, at the moment that
is the only thing that uses contentpolicy, but there might be more in the
future. (Blocking flash?)
Or do other types of blocking need this too?

But at least for images, i think this is ok. The display of alt-texts might
collide with whitelisting, where the pref that is read can be overruled by
cookperm.txt

My opinion on prompt is that we should not re-implement it. The people who want
it will be happy with whitelisting too. And that is much easier and usefull.
(but all ths should be in another bug :) )

Comment 2

[Boris Zbarsky [:bzbarsky]]

I would think all types of blocking could use this...  At the moment, images are
indeed the only place that needs to differentiate between "do not load any
objects of this type" and "do not load this one particular object".  But the
same could apply to plugins (eg "no flash allowed to load" vs "do not load flash
from doubleclick.net").

So no, the return value would not be image-specific at all, I would say.... 
Again, that's just my suggestion; perhaps there are better solutions to this
problem; if so, we should use them.  It's just that the current "solution" is
very broken.  ;)

Comment 3

[Tim]

You now get an |extra| parameter to do with as you please. Use it to pass out
extra data to the caller, to cook your pizza, or to make your whites
whiter--whatever. If you can think of general cases where distinction beyond
what I already provide is necessary, mention it here or in bug 191839.  Be sure
to defend why your suggestion is better done by altering ContentPolicy instead
of by using the extra parameter.

I don't know whether this should block or depend on bug 191839, but let's try
blocks first.

Comment 4

[Boris Zbarsky [:bzbarsky]]

I guess we may as well keep all the discussion in bug 191839 instead of
spreading it over 3 bugs...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Fixed by bug 191839.