202906 - [FIX]"Load images from originating server only" does not show alt text

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, enhancement, P1)

Description

[Boris Zbarsky [:bzbarsky]]

This is just broken -- we fail to replace the image frame and fail to show the
alt text.

Comment 1

[Tim]

This should have nothing to do with ContentPolicy (the dependency on bug
201885).  I put forth that any failure to load the image, caused by anything
(OOM, ConPol rejection, or anything else) should result in the alt text being
displayed.  I'm not going to remove the dependency, but I strongly suggest it be
removed.

Comment 2

[Tim]

http://lxr.mozilla.org/seamonkey/search?string=nsIContentPolicy::IMAGE
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsImageLoadingContent.cpp#545

The above point to where the fix to this could easily go. Just separate the
!shouldLoad case.

Comment 3

[Tim]

Alternately, if you really want to make content policy's decision about an image
to affect alt-text-showing, I suggest you use the |extra| parameter I am
providing to accomplish that (how you do that is not my concern).

Comment 4

[Boris Zbarsky [:bzbarsky]]

This has everything to do with content policy, because there are two reasons
that an image load may be blocked:

1)  All image loads are blocked
2)  This particular site is not allowed to load images

The alt text behavior should be different for those cases. Hence the dependecy
on  bug 201885.  Note that the core gecko image loading code cannot encode ANY
knowledge of any content policy implementations -- if it's not in the content
policy API, it's not usable from this code.  As in, the "extra" parameter is
completely useless here.

Comment 5

[Tim]

I disagree that those two cases should be different, but I'll see if there is
some way to facilitate you here.  What other cases could/does anything else care
about why a request was rejected? Perhaps this discussion should continue in bug
201885, since that's closer to my work.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Let's just keep it all in bug 191839

Comment 7

[Aaron Leventhal]

"Do not load any images" also seems to have this problem.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Rough patch

Not really tested yet...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to trunk

I tested this, and it works and all.. mvl, does the setup (only show alt text
for images blocked by type) seem reasonable?

Comment 10

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

I can think of two reasons to block images: faster loading or not wanting ads.
The first user will disable image loading in prefs, but does want alt text, to
at least know something about the images. (Alt is in html, so loaded anyway.
Showing it doesn't block the goal). The other will block images based on host,
and doesn't want the alternative text, which is likely an ad also. So the setup
sounds right to me.

Comment 11

[Boris Zbarsky [:bzbarsky]]

OK.  In that case, wanna review?  ;)

Comment 12

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

Comment on attachment 174769 [details] [diff] [review]
Merged to trunk

>Index: content/base/public/nsContentUtils.h
>+   * @param aImageBlockingStatus the nsIContentPolicy blocking status for this
>+   *        image.  This will be set even if a security check fails for the
>+   *        image, to some reasonable REJECT_* value.  This out param will only
>+   *        be set if it's non-null.
>    * @return PR_TRUE if the load can proceed, or PR_FALSE if it is blocked

It might be worth it noticing that the return value is a subset of
aImageBlockingStatus, so they are not independant.

>Index: content/base/src/nsImageLoadingContent.h
>+   * @param aNewImageStatus the nsIContentPolicy status of the new image

(I don't really know this code, just from looking at this file:) what is 'the
new image'? Does a new image get created if a load is cancelled?

>Index: layout/generic/nsImageFrame.h

You can remove mPrefAllImagesBlocked from this file, right? It isn't used
anymore. (at least, it is never set anymore, so i really hope nothing reads it
anymore :) )

r=mvl

Comment 13

[Boris Zbarsky [:bzbarsky]]

> what is 'the new image'?

The cancel method is called when the src of an image changes; the new image is
the image the new src points to.

> You can remove mPrefAllImagesBlocked from this file, right?

Yep.  Good catch.

jst, assume I added comments for the first two review comments from mvl and
fixed the third one, ok?

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 174769 [details] [diff] [review]
Merged to trunk

Yeah, and you probably want to change the IID in nsIImageLoadingContent.idl
since you're changing the signature of a method...

sr=jst

Comment 15

[Boris Zbarsky [:bzbarsky]]

Fixed.