[webkit-reviews] review granted: [Bug 22814] Add WML <img> element support : [Attachment 25969] Updated patch

[bugzilla-daemon at webkit.org]

Holger Freyther <zecke at selfish.org> has granted Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 22814: Add WML <img> element support
https://bugs.webkit.org/show_bug.cgi?id=22814

Attachment 25969: Updated patch
https://bugs.webkit.org/attachment.cgi?id=25969&action=review

------- Additional Comments from Holger Freyther <zecke at selfish.org>

> 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::hre
fAttr);
> +		|| 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.