> For those of us following along at home, what's the purpose of this content attribute? How does it affect the request (which is presumably still a "simple" one)?
The attribute has two effects:
1) The request becomes a CORS request, so it gets an outgoing Origin header. I'll have to read the spec, but the image load might be blocked if CORS says no.
2) The attribute lets you control whether to make an anonymous request (as in CORS), so you can strip cookies / HTTP auth from the request if you like.
Apparently we can't just always use non-anonymous CORS for image loads because some servers get sad when you send them unexpected Origin headers.

I forgot the mention that the point of the attribute is to let you draw images onto 2D and 3D canvases and read back the results. In the case of 3D canvas, my understanding is that a thumbs up for CORS is going to be required for drawing the image at all.

Comment on attachment 94880[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=94880&action=review> Source/WebCore/html/HTMLCanvasElement.cpp:409
> +SecurityOrigin* HTMLCanvasElement::securityOrigin() const
Are we changing this because SecurityOrigin* can sometimes be NULL?
> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1584
> + bool originClean = false;
> + if (cachedImage->image()->hasSingleSecurityOrigin()) {
> + originClean = cachedImage->passesAccessControlCheck(canvas()->securityOrigin())
> + || !canvas()->securityOrigin()->taintsCanvas(cachedImage->response().url());
> + }
I would have just made this a helper. isOriginClean(canvas(), cachedImage); ten you can use early return, etc.
> Source/WebCore/loader/ImageLoader.cpp:168
> + updateRequestForAccessControl(request, document->securityOrigin(), allowCredentials);
Where is this free-function defined? (Why is it free?) And should we just be passing the crossOriginMode instead? I wonder if this whole block shouldn't be a separate function in ImageLoader, and then this call is just:
updateAccessControlsForCrossOriginMode(crossOriginMode)
> Source/WebCore/loader/cache/CachedResource.cpp:179
> + return WebCore::passesAccessControlCheck(m_response, resourceRequest().allowCookies(), securityOrigin, errorDescription);
Do you need/want the WebCore:: here? Should this function take a String* or StringImpl* since this is an optional return?

(In reply to comment #13)
> (From update of attachment 94880[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94880&action=review
>
> > Source/WebCore/html/HTMLCanvasElement.cpp:409
> > +SecurityOrigin* HTMLCanvasElement::securityOrigin() const
>
> Are we changing this because SecurityOrigin* can sometimes be NULL?
No, but SecurityOrigin is a pointer-type not a reference-type everywhere else in the code. This code is just randomly different.
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1584
> > + bool originClean = false;
> > + if (cachedImage->image()->hasSingleSecurityOrigin()) {
> > + originClean = cachedImage->passesAccessControlCheck(canvas()->securityOrigin())
> > + || !canvas()->securityOrigin()->taintsCanvas(cachedImage->response().url());
> > + }
>
> I would have just made this a helper. isOriginClean(canvas(), cachedImage); ten you can use early return, etc.
Ok.
> > Source/WebCore/loader/ImageLoader.cpp:168
> > + updateRequestForAccessControl(request, document->securityOrigin(), allowCredentials);
>
> Where is this free-function defined?
CrossOriginAccessControl.h
> (Why is it free?)
Because it has no associated state.
> And should we just be passing the crossOriginMode instead?
Nope. updateRequestForAccessControl is a lower-level primitive shared by all CORS APIs. crossOriginMode is specific to this API.
> I wonder if this whole block shouldn't be a separate function in ImageLoader, and then this call is just:
>
> updateAccessControlsForCrossOriginMode(crossOriginMode)
It's eventually going to be shared by Images, Audio, and Video. My plan was to move it somewhere else when implementing those APIs since the correct place will be more obvious then.
> > Source/WebCore/loader/cache/CachedResource.cpp:179
> > + return WebCore::passesAccessControlCheck(m_response, resourceRequest().allowCookies(), securityOrigin, errorDescription);
>
> Do you need/want the WebCore:: here?
It's needed to call the correct function because the free function and the member function have the same name.
> Should this function take a String* or StringImpl* since this is an optional return?
Dunno. I'm not sure it matters that much. I guess we could pass 0, which might be slightly better. Seems like an issue for another patch.

Created attachment 95017[details]
Archive of layout-test-results from ec2-cr-linux-02
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 95011[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=95011&action=review>> Source/WebCore/loader/ImageLoader.cpp:168
>> + updateRequestForAccessControl(request, document->securityOrigin(), allowCredentials);
>
> I really dislike these being free functions. We should consider namspacing them in a class at some point.
I guess beauty is in the eye of the beholder. I like them as free functions.

(In reply to comment #19)
> (From update of attachment 95011[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95011&action=review
>
> >> Source/WebCore/loader/ImageLoader.cpp:168
> >> + updateRequestForAccessControl(request, document->securityOrigin(), allowCredentials);
> >
> > I really dislike these being free functions. We should consider namspacing them in a class at some point.
>
> I guess beauty is in the eye of the beholder. I like them as free functions.
So true. :) I find it confusing that he has to WebCore:: namespace them in one place. We have very few free functions in WebCore. But I admit to not having looked at these in great detail.

> I didn't review the tests particularly closely. If you need me to, I'm happy to take another look.
The tests are based on the existing image-canvas tainting tests, but with the expected results tweaked to match the CORS status of the request/image.

Comment on attachment 95011[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=95011&action=review> Source/WebCore/html/HTMLCanvasElement.h:118
> - const SecurityOrigin& securityOrigin() const;
> + SecurityOrigin* securityOrigin() const;
I kind of like the old signature. It’s kind of weak to offer a security origin object that you can mutate for a canvas. We don’t want a caller to get it and then call something like setDomainFromDOM on it, do we? What’s the rationale here?
> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:95
> + if (cachedImage->image()->hasSingleSecurityOrigin())
> + return false;
Is there a ! missing here?
> Source/WebCore/html/canvas/CanvasRenderingContext.cpp:73
> - checkOrigin(KURL(KURL(), video->currentSrc()));
> + // FIXME: HTMLVideoElement::currentSrc() should return a KURL.
> + checkOrigin(KURL(ParsedURLString, video->currentSrc()));
Why change to this intermediate state? I am hoping we can get rid of ParsedURLString soon. Can’t we wait and not touch this call site until we fixed currentSrc to return a URL?
> Source/WebCore/loader/ImageLoader.cpp:164
> + String crossOriginMode = m_element->getAttribute(HTMLNames::crossoriginAttr);
Could use fastGetAttribute. That’s always allowed for any attribute that is guaranteed to not be an SVG animated attribute or the style attribute. And it’s faster.
> Source/WebCore/loader/ImageLoader.cpp:167
> + DEFINE_STATIC_LOCAL(String, useCredentials, ("use-credentials"));
> + bool allowCredentials = equalIgnoringCase(crossOriginMode, useCredentials);
I’m not sure that having a static local string is a more efficient way to do the equality check. You’d think that an equality check with a const char* could be super fast.

(In reply to comment #24)
> (From update of attachment 95011[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95011&action=review
>
> > Source/WebCore/html/HTMLCanvasElement.h:118
> > - const SecurityOrigin& securityOrigin() const;
> > + SecurityOrigin* securityOrigin() const;
>
> I kind of like the old signature. It’s kind of weak to offer a security origin object that you can mutate for a canvas. We don’t want a caller to get it and then call something like setDomainFromDOM on it, do we? What’s the rationale here?
The rationale is just consistency with everywhere else where we handle SecurityOrigin objects. In this case, we want to pass the object to other functions. There was already code that took the address of the reference, which seems somewhat goofy.
Maybe "SecurityOrigin const*" would be the best type?
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:95
> > + if (cachedImage->image()->hasSingleSecurityOrigin())
> > + return false;
>
> Is there a ! missing here?
Yep. I added it back in the final patch. (It causes a massive number of test failures, thankfully.)
> > Source/WebCore/html/canvas/CanvasRenderingContext.cpp:73
> > - checkOrigin(KURL(KURL(), video->currentSrc()));
> > + // FIXME: HTMLVideoElement::currentSrc() should return a KURL.
> > + checkOrigin(KURL(ParsedURLString, video->currentSrc()));
>
> Why change to this intermediate state? I am hoping we can get rid of ParsedURLString soon. Can’t we wait and not touch this call site until we fixed currentSrc to return a URL?
I started down that path, but the scope was bigger than I wanted to include in this patch. I'll do that in Bug 61578.
> > Source/WebCore/loader/ImageLoader.cpp:164
> > + String crossOriginMode = m_element->getAttribute(HTMLNames::crossoriginAttr);
>
> Could use fastGetAttribute. That’s always allowed for any attribute that is guaranteed to not be an SVG animated attribute or the style attribute. And it’s faster.
Thanks. I wasn't familiar with the details of when you can use fastGetAttribute. I'll fix that.
> > Source/WebCore/loader/ImageLoader.cpp:167
> > + DEFINE_STATIC_LOCAL(String, useCredentials, ("use-credentials"));
> > + bool allowCredentials = equalIgnoringCase(crossOriginMode, useCredentials);
>
> I’m not sure that having a static local string is a more efficient way to do the equality check. You’d think that an equality check with a const char* could be super fast.
Will do.