Bug 19357 – [GTK] Loading of resource images

• Component: WebKitGTK
• Status: RESOLVED WONTFIX

[Marco Barisione]

WebCore can request the loading of local images that are shown, for instance, as placeholders when images cannot be loaded (see loadResourceIntoArray in WebCore/platform/gtk/TemporaryLinkStubs.cpp).

Probably the best way to implement this is to add properties to WebKitWebSettings for images to use, but some questions remain open:
- What should we do by default? Do not show anything? Or provide default images?
- What should we store as a property? The uri of the image? A GFile? The image itself?

[Christian Dywan]

> - What should we do by default? Do not show anything? Or provide default
> images?

The default should probably gtk's stock 'missing' image.

> - What should we store as a property? The uri of the image? A GFile? The image
> itself?

I don't know what the use case for a custom image is. An icon name is nice, but depending on the task an actual pixbuf makes probably much more sense.

[Marco Barisione]

Created attachment 21841 [details]
Use the missing-image stock icon as a placeholder when an image or plugin is missing

Note that I'm using the same image for both missingImage and for nullPlugin as gtk doesn't have an icon for the latter.

[Christian Dywan]

Comment on attachment 21841 [details]
Use the missing-image stock icon as a placeholder when an image or plugin is missing

>+    if (strcmp(name,"missingImage") == 0 || strcmp(name, "nullPlugin") == 0) {
>+        GtkIconTheme* iconTheme = gtk_icon_theme_get_default();
>+        // The image must be a PNG or WebCore will not use it.
>+        GtkIconInfo* iconInfo = gtk_icon_theme_lookup_icon (iconTheme, GTK_STOCK_MISSING_IMAGE, 16, GTK_ICON_LOOKUP_NO_SVG);
>+        if (iconInfo) {

Why would WebCore not use an SVG image? This seems odd.

In any case, there are themes that contain only svg images. So we really want to support vector images as well.

[Holger Freyther]

substance:
   + I like the idea of using GtkIconTheme
   - You want to handle "missingImage", "nullPlugin", "urlIcon", "textAreaResizeCorner".
   - You certainly don't want to use missingImage for nullPlugin
   - The tricky part where I lack an answer is if we want to install the webkit icons for the above somewhere? Should we embed them in the code? Does the fdo icon theme spec have some icons we could reuse?

so I'm more tempted to say r=-.

style comments:
   - We tend to do early exits in error cases instead of deeply nesting the success cases

[Marco Barisione]

(In reply to comment #4)
>    - You want to handle "missingImage", "nullPlugin", "urlIcon",
> "textAreaResizeCorner".

For missingImage we want for sure to use the GTK icon. The other icons are probably less important as they are less common, so maybe we can add them in a second time.

Do you know when urlIcon and textAreaResizeCorner are used? I wasn't able to figure that out reading the code.

>    - You certainly don't want to use missingImage for nullPlugin

Isn't it still better than nothing?

>    - The tricky part where I lack an answer is if we want to install the webkit
> icons for the above somewhere? Should we embed them in the code? Does the fdo
> icon theme spec have some icons we could reuse?

Probably yes for the urlIcon but I don't know when it's used so I don't know what is the best icon for that.

The spec doesn't have anything for missingPlugin (and neither something for plugins in general). I already talked with some tango icon designers about this, probably the best thing is to get a plugin icon in the spec and use that. I will open a bug on fd.o but probably it will take time, so maybe it's better to ship an icon with WebKit for now.

textAreaResizeCorner doesn't seem like something that should be an icon, so I think it's better to ship it with WebKit.

> style comments:
>    - We tend to do early exits in error cases instead of deeply nesting the
> success cases

I will fix this.

[Marco Barisione]

(In reply to comment #5)
> The spec doesn't have anything for missingPlugin (and neither something for
> plugins in general). I already talked with some tango icon designers about
> this, probably the best thing is to get a plugin icon in the spec and use that.
> I will open a bug on fd.o but probably it will take time, so maybe it's better
> to ship an icon with WebKit for now.

See https://bugs.freedesktop.org/show_bug.cgi?id=16500 and http://bugzilla.gnome.org/show_bug.cgi?id=539994

[Christian Dywan]

(In reply to comment #5)
> (In reply to comment #4)
> >    - You want to handle "missingImage", "nullPlugin", "urlIcon",
> > "textAreaResizeCorner".
> 
> For missingImage we want for sure to use the GTK icon. The other icons are
> probably less important as they are less common, so maybe we can add them in a
> second time.
> 
> Do you know when urlIcon and textAreaResizeCorner are used? I wasn't able to
> figure that out reading the code.

I suspect textAreaResizeCorner is the resize grip on the bottom right of text areas that you typically see in applications as soon as scrollbars appear. In our case it's always visible if text areas can be resized.
Instead of using an icon WebKit needs to draw it with gtk_paint_resize_grip.

> >    - You certainly don't want to use missingImage for nullPlugin
> 
> Isn't it still better than nothing?

I tend to agree with 'anything would be good for now' but that always bares the danger of leaving the problem persist in trunk for even longer.

> >    - The tricky part where I lack an answer is if we want to install the webkit
> > icons for the above somewhere? Should we embed them in the code? Does the fdo
> > icon theme spec have some icons we could reuse?
> 
> Probably yes for the urlIcon but I don't know when it's used so I don't know
> what is the best icon for that.
> 
> The spec doesn't have anything for missingPlugin (and neither something for
> plugins in general). I already talked with some tango icon designers about
> this, probably the best thing is to get a plugin icon in the spec and use that.
> I will open a bug on fd.o but probably it will take time, so maybe it's better
> to ship an icon with WebKit for now.

There is no choice. If there is no standard icon, it needs to be shipped, be it in the spec or not. There is no way we can rely on one theme that gains the icon two days before the patch is committed.
We can safely try to get hold of someone who makes the missing icons, install them a private folder, such as $prefix/share/webkit and switch to something else later.

[Marco Barisione]

(In reply to comment #7)
> There is no choice. If there is no standard icon, it needs to be shipped, be it
> in the spec or not. There is no way we can rely on one theme that gains the
> icon two days before the patch is committed.
> We can safely try to get hold of someone who makes the missing icons, install
> them a private folder, such as $prefix/share/webkit and switch to something
> else later.

I already talked about this with Lapo Calamandrei (he works on gnome-icon-theme and tango), he already drew some plugin icons for other programs, so probably we can use one of those and switch to the icon in the spec later.

[Christian Dywan]

Comment on attachment 21841 [details]
Use the missing-image stock icon as a placeholder when an image or plugin is missing

Clearing the review flag until recent comments are addressed.

[Christian Dywan]

*** Bug 19826 has been marked as a duplicate of this bug. ***

[Holger Freyther]

Created attachment 30180 [details]
Another implementation of resource loading

I didn't see the earlier discussion until now. So the state is that we:
   - install every resource we use
   - provide the facility to map the WebCore resource to the fdo.org name, so we always prefer the icon theme icon if it exists...

What is not done:
   - RenderLayer is not changed and we use the textAreaResize... this actually means we can resize text areas right now instead of not be able to resize them. The RenderLayer change would probably include pushing things into RenderTheme... and have a different implementation for RenderThemeGtk.

[Holger Freyther]

Created attachment 30181 [details]
Another implementation of resource loading

actually compiling...

[Holger Freyther]

Created attachment 30182 [details]
Theme urlIcon

Load urlIcon through the Image class instead of having it compiled in.

[Gustavo Noronha (kov)]

Comment on attachment 30181 [details]
Another implementation of resource loading

> +        Install the four resources we are using. For the icons
> +        were an icon name is specified by freedeskop.org try to

were -> where

> +template <> void freeOwnedGPtr<GtkIconInfo>(GtkIconInfo*);
> +template <> void freeOwnedGPtr<GtkIconInfo>(GtkIconInfo* info)

Why do you need both? Any C++ craziness I'm not aware of?

> +{
> +    if (info)
> +	gtk_icon_info_free(info);
> +}

Indentation is off here, tab? There are many more throughout the patch, it seems.

> +    GOwnPtr<GtkIconInfo> info(gtk_icon_theme_lookup_icon(gtk_icon_theme_get_default(),
> +							 name, 16, GTK_ICON_LOOKUP_NO_SVG));

There's a comment by kalikiana regarding this already, from the earlier patches. Why NO_SVG? There are themes which use only SVG files, and there's no reason not to suport them, I guess. If we are worried about SVG being compiled out, we could just add a preprocessor guard?

> +    if (!info)
> +	return String::format("%s/webkit-1.0/images/%s.png", DATA_DIR, fallback).utf8();
> +
> +    return CString(gtk_icon_info_get_filename(info.get()));
> +}

More indentation issues.

I would say r=me if those issues (indentation and no SVG) are fixed, or if there's a rationale for not being able to support SVG in this specific instance.

[Gustavo Noronha (kov)]

Comment on attachment 30182 [details]
Theme urlIcon

r=me on this one, but for anyone looking for pending commits, we need to land this after the other patch is fixed/landed

[Holger Freyther]

(In reply to comment #14)

> More indentation issues.

ack

> 
> I would say r=me if those issues (indentation and no SVG) are fixed, or if
> there's a rationale for not being able to support SVG in this specific
> instance.

the no SVG has one practical issue. we are not able to load it. We will return a byte array here and we have no way to load that through any kind of svg renderer. So in the case of an all svg theme it is best to use our internal icon...

[Gustavo Noronha (kov)]

Comment on attachment 30181 [details]
Another implementation of resource loading

(In reply to comment #16)
> > I would say r=me if those issues (indentation and no SVG) are fixed, or if
> > there's a rationale for not being able to support SVG in this specific
> > instance.
> 
> the no SVG has one practical issue. we are not able to load it. We will return
> a byte array here and we have no way to load that through any kind of svg
> renderer. So in the case of an all svg theme it is best to use our internal
> icon...
 
r=me, then, with the other fixes; thanks for clarifying!

[Christian Dywan]

(In reply to comment #16)
> (In reply to comment #14)
> > I would say r=me if those issues (indentation and no SVG) are fixed, or if
> > there's a rationale for not being able to support SVG in this specific
> > instance.
> 
> the no SVG has one practical issue. we are not able to load it. We will return
> a byte array here and we have no way to load that through any kind of svg
> renderer. So in the case of an all svg theme it is best to use our internal
> icon...

Why can't we simply use GdkPixbuf or gtk_widget_render_icon for this? Is there a reason why WebCore has to have a filename that it can load?

Or does it boil down to not having support for GdkPixbuf loaders in general?

[Holger Freyther]

Comment on attachment 30182 [details]
Theme urlIcon

Clearing review. A follow up patch could use the GtkIconTheme to load a GdkPixbuf and we will convert that to a BitmapImage... this would allow us to work with pure svg themes too.

[Martin Robinson]

I think we can go on without this feature for the moment.