Bug 22814 – Add WML <img> element support

• Status: RESOLVED
• Resolution: FIXED
• Component: XML

[Nikolas Zimmermann]

Adding <img> support is trivial, as there already exists a common ImageLoader class, and SVG/HTMLImage{Element/Loader} already share as much code as possible.

Only difference for WML is that there exists a second source attribute called 'localsrc' - that takes precedence over the 'src' attribute. If loading 'localsrc' fails, 'src' should be tried instead - crazy :-)

[Nikolas Zimmermann]

Created attachment 25967 [details]
Initial patch

Patch contains a sorted version of WebCore.xcodeproj/project.pbxproj, that enlarges the patch a bit.

[Nikolas Zimmermann]

Created attachment 25968 [details]
LayoutTests

[Holger Freyther]

Comment on attachment 25968 [details]
LayoutTests

This looks sane.

[Nikolas Zimmermann]

Created attachment 25969 [details]
Updated patch

Incorporated Holgers comments.

[Holger Freyther]

Comment on attachment 25969 [details]
Updated patch


> Index: WebCore.xcodeproj/project.pbxproj
> ===================================================================
> --- WebCore.xcodeproj/project.pbxproj	(revision 39224)
> +++ WebCore.xcodeproj/project.pbxproj	(working copy)

changing the order is okay according to Niko.


> Index: rendering/HitTestResult.cpp
> ===================================================================
> --- rendering/HitTestResult.cpp	(revision 39224)
> +++ rendering/HitTestResult.cpp	(working copy)

>      if (!m_innerNonSharedNode)
>          return String();
> -    
> +
>      if (m_innerNonSharedNode->hasTagName(imgTag)) {
>          HTMLImageElement* image = static_cast<HTMLImageElement*>(m_innerNonSharedNode.get());
>          return displayString(image->alt(), m_innerNonSharedNode.get());
>      }
> -    
> +
>      if (m_innerNonSharedNode->hasTagName(inputTag)) {
>          HTMLInputElement* input = static_cast<HTMLInputElement*>(m_innerNonSharedNode.get());
>          return displayString(input->alt(), m_innerNonSharedNode.get());
>      }
> -    
> +

please land without these whitespace changes. rs=me if you want to fix the bogus space.



> @@ -232,11 +240,14 @@ KURL HitTestResult::absoluteImageURL() c
>      AtomicString urlString;
>      if (m_innerNonSharedNode->hasTagName(imgTag) || m_innerNonSharedNode->hasTagName(inputTag))
>          urlString = static_cast<Element*>(m_innerNonSharedNode.get())->getAttribute(srcAttr);
> +    else if (m_innerNonSharedNode->hasTagName(embedTag)
>  #if ENABLE(SVG)
> -    else if (m_innerNonSharedNode->hasTagName(SVGNames::imageTag))
> -        urlString = static_cast<Element*>(m_innerNonSharedNode.get())->getAttribute(XLinkNames::hrefAttr);
> +             || m_innerNonSharedNode->hasTagName(SVGNames::imageTag)
> +#endif
> +#if ENABLE(WML)
> +             || m_innerNonSharedNode->hasTagName(WMLNames::imgTag)
>  #endif
> -    else if (m_innerNonSharedNode->hasTagName(embedTag) || m_innerNonSharedNode->hasTagName(objectTag)) {
> +             || m_innerNonSharedNode->hasTagName(objectTag)) {


yes, I checked, SVGImageElement returns XLinkNames::hrefAttr for imageSourceAttributeName...so it could be correct.


> +        imageObj->setCachedImage(m_imageLoader.image());
> +
> +        // If we have no image at all because we have no src attribute, set
> +        // image height and width for the alt text instead.
> +        if (!m_imageLoader.image() && !imageObj->cachedImage())
> +            imageObj->setImageSizeForAltText();


I see this is from HTMLImageElement. But after the above setCachedImage the following assert should be true. ASSERT(imageObj->cachedImage() == m_imageLoader.image())? If that is the case checking for m_imageLoader.image() or imageObj->cachedImage would be enough... but this can be cleaned up later.



> +    // WML doesn't fire any events.
> +    if (haveFiredLoadEvent())
> +        return;
> +
> +    setHaveFiredLoadEvent(true);

hehe...


> +void WMLImageLoader::notifyFinished(CachedResource* image)
> +{

yes, this should fallback to src when the loading failed and there is a HTMLNames::srcAttr on the WMLImageElement. Looks good.

[Nikolas Zimmermann]

Thanks, Holger. Fixed your comments.

Except the one affecting HTMLImageLoader, it should be fixed alltogether in a seperated patch.
Landed in r39225.