Bug 22814 - Add WML <img> element support
Summary: Add WML <img> element support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks: 20393
  Show dependency treegraph
 
Reported: 2008-12-11 16:23 PST by Nikolas Zimmermann
Modified: 2008-12-11 18:02 PST (History)
0 users

See Also:


Attachments
Initial patch (30.56 KB, patch)
2008-12-11 16:51 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
LayoutTests (14.92 KB, patch)
2008-12-11 16:51 PST, Nikolas Zimmermann
zecke: review+
Details | Formatted Diff | Diff
Updated patch (31.68 KB, patch)
2008-12-11 17:30 PST, Nikolas Zimmermann
zecke: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2008-12-11 16:23:07 PST
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 :-)
Comment 1 Nikolas Zimmermann 2008-12-11 16:51:08 PST
Created attachment 25967 [details]
Initial patch

Patch contains a sorted version of WebCore.xcodeproj/project.pbxproj, that enlarges the patch a bit.
Comment 2 Nikolas Zimmermann 2008-12-11 16:51:30 PST
Created attachment 25968 [details]
LayoutTests
Comment 3 Holger Freyther 2008-12-11 17:25:34 PST
Comment on attachment 25968 [details]
LayoutTests

This looks sane.
Comment 4 Nikolas Zimmermann 2008-12-11 17:30:12 PST
Created attachment 25969 [details]
Updated patch

Incorporated Holgers comments.
Comment 5 Holger Freyther 2008-12-11 17:51:00 PST
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.
Comment 6 Nikolas Zimmermann 2008-12-11 18:02:58 PST
Thanks, Holger. Fixed your comments.

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