Add HTMLPictureElement-based source selection to HTMLImageElement

Source/core/html/HTMLImageElement.cpp

Index: Source/core/html/HTMLImageElement.cpp
diff --git a/Source/core/html/HTMLImageElement.cpp b/Source/core/html/HTMLImageElement.cpp
index f236266a651d13830a5dd6e38c292f095bb1c0ae..8c23ea9173cea044d980ada14c409d5d1b4cff6b 100644
--- a/Source/core/html/HTMLImageElement.cpp
+++ b/Source/core/html/HTMLImageElement.cpp
@@ -25,6 +25,7 @@
 
 #include "CSSPropertyNames.h"
 #include "HTMLNames.h"
+#include "MediaTypeNames.h"
 #include "RuntimeEnabledFeatures.h"
 #include "bindings/v8/ScriptEventListener.h"
 #include "core/css/MediaValuesCached.h"
@@ -34,10 +35,12 @@
 #include "core/html/HTMLAnchorElement.h"
 #include "core/html/HTMLCanvasElement.h"
 #include "core/html/HTMLFormElement.h"
+#include "core/html/HTMLSourceElement.h"
 #include "core/html/canvas/CanvasRenderingContext.h"
 #include "core/html/parser/HTMLParserIdioms.h"
 #include "core/html/parser/HTMLSrcsetParser.h"
 #include "core/rendering/RenderImage.h"
+#include "platform/MIMETypeRegistry.h"
 
 using namespace std;
 
@@ -150,23 +153,28 @@ void HTMLImageElement::resetFormOwner()
     }
 }
 
+void HTMLImageElement::setBestFitURLAndDPRFromImageCandidate(const ImageCandidate& candidate)
+{
+    m_bestFitImageURL = candidate.toAtomicString();
+    float candidateScaleFactor = candidate.scaleFactor();
+    // FIXME: Make this ">0" part match the spec, once it settles.
+    if (candidateScaleFactor > 0)
+        m_imageDevicePixelRatio = 1 / candidateScaleFactor;
+    if (renderer() && renderer()->isImage())
+        toRenderImage(renderer())->setImageDevicePixelRatio(m_imageDevicePixelRatio);
+}
+
 void HTMLImageElement::parseAttribute(const QualifiedName& name, const AtomicString& value)
 {
     if (name == altAttr) {
         if (renderer() && renderer()->isImage())
             toRenderImage(renderer())->updateAltText();
     } else if (name == srcAttr || name == srcsetAttr || name == sizesAttr) {
-        int effectiveSize = 0;
+        unsigned effectiveSize = 0;
         if (RuntimeEnabledFeatures::pictureSizesEnabled())
             effectiveSize = SizesAttributeParser::findEffectiveSize(fastGetAttribute(sizesAttr), MediaValuesCached::create(document()));
         ImageCandidate candidate = bestFitSourceForImageAttributes(document().devicePixelRatio(), effectiveSize, fastGetAttribute(srcAttr), fastGetAttribute(srcsetAttr));
-        m_bestFitImageURL = candidate.toAtomicString();
-        float candidateScaleFactor = candidate.scaleFactor();
-        // FIXME: Make this ">0" part match the spec, once it settles.
-        if (candidateScaleFactor > 0)
-            m_imageDevicePixelRatio = 1 / candidateScaleFactor;
-        if (renderer() && renderer()->isImage())
-            toRenderImage(renderer())->setImageDevicePixelRatio(m_imageDevicePixelRatio);
+        setBestFitURLAndDPRFromImageCandidate(candidate);
         m_imageLoader.updateFromElementIgnoringPreviousError();
     } else if (name == usemapAttr) {
         setIsLink(!value.isNull());
@@ -192,6 +200,48 @@ const AtomicString& HTMLImageElement::altText() const
     return fastGetAttribute(titleAttr);
 }
 
+static bool supportedImageType(const String& type)
+{
+    return MIMETypeRegistry::isSupportedImageResourceMIMEType(type);
+}
+
+// http://picture.responsiveimages.org/#update-source-set
+bool HTMLImageElement::getBestFitImageFromPictureParent()

[esprehn, 2014/05/16 23:31:57]

This is pretty strange to start with "get", what does the boolean return mean
too?

+{
+    ASSERT(isMainThread());
+    Node* parent = parentNode();
+    if (!parent || !isHTMLPictureElement(*parent))
+        return false;
+    for (Node* child = parent->firstChild(); child; child = child->nextSibling()) {

[esprehn, 2014/05/16 23:31:57]

You want Traversal<HTMLSourceElement> here.

+        if (child == this)
+            return false;
+
+        if (!isHTMLSourceElement(*child))
+            continue;
+
+        HTMLSourceElement* source = toHTMLSourceElement(child);
+        if (!source->fastHasAttribute(srcsetAttr))
+            continue;
+        if (source->fastHasAttribute(typeAttr) && !supportedImageType(source->fastGetAttribute(typeAttr)))

[esprehn, 2014/05/16 23:31:57]

ditto. Just unconditionally call get and make supportedImageType take empty
strings.

+            continue;
+
+        if (source->fastHasAttribute(mediaAttr)) {

[esprehn, 2014/05/16 23:31:57]

I'd suggest just calling fastGetAttribute() and looking if it's empty. Doing
both has() and get() just makes this slower.

+            RefPtr<MediaQuerySet> mediaQueries = MediaQuerySet::create(source->fastGetAttribute(mediaAttr));
+            MediaQueryEvaluator mediaQueryEvaluator(MediaTypeNames::screen, document().frame());
+            if (!mediaQueryEvaluator.eval(mediaQueries.get()))

[esprehn, 2014/05/16 23:31:57]

Why are you not using document()->mediaQueryMatcher() ?

+                continue;
+        }
+
+        unsigned effectiveSize = SizesAttributeParser::findEffectiveSize(source->fastGetAttribute(sizesAttr), MediaValuesCached::create(document()));
+        ImageCandidate candidate = bestFitSourceForSrcsetAttribute(document().devicePixelRatio(), effectiveSize, source->fastGetAttribute(srcsetAttr));
+        if (candidate.isEmpty())
+            continue;
+        setBestFitURLAndDPRFromImageCandidate(candidate);

[esprehn, 2014/05/16 23:31:57]

Lets just return the candidate instead. This method should be called find*().

+        return true;
+    }
+    return false;

[esprehn, 2014/05/16 23:31:57]

return 0;

+}
+
 RenderObject* HTMLImageElement::createRenderer(RenderStyle* style)
 {
     if (style->hasContent())
@@ -236,9 +286,13 @@ Node::InsertionNotificationRequest HTMLImageElement::insertedInto(ContainerNode*
     if (!m_formWasSetByParser || insertionPoint->highestAncestorOrSelf() != m_form->highestAncestorOrSelf())
         resetFormOwner();
 
+    bool imageWasModified = false;
+    if (RuntimeEnabledFeatures::pictureEnabled())
+        imageWasModified = getBestFitImageFromPictureParent();

[esprehn, 2014/05/16 23:31:57]

getters don't start with "get", things that return through out params do.

This should be find*() and should return a candidate instead.

+
     // If we have been inserted from a renderer-less document,
     // our loader may have not fetched the image, so do it now.
-    if (insertionPoint->inDocument() && !m_imageLoader.image())
+    if ((insertionPoint->inDocument() && !m_imageLoader.image()) || imageWasModified)
         m_imageLoader.updateFromElement();
 
     return HTMLElement::insertedInto(insertionPoint);
@@ -315,6 +369,7 @@ int HTMLImageElement::naturalHeight() const
 
 const AtomicString& HTMLImageElement::currentSrc() const
 {
+    // FIXME - Need to absolutize the returned value.

[esprehn, 2014/05/16 23:31:57]

FIXME: not dash

     return m_bestFitImageURL;
 }