When <img crossorigin> fails the CORS check, we should fire the error event
Created attachment 133395 [details] Patch
http://code.google.com/p/chromium/issues/detail?id=112170
Is this the same as bug 80028?
Comment on attachment 133395 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133395&action=review > Source/WebCore/ChangeLog:11 > + otherwise be because we're firing the event asynchronously, but that > + seems like the right thing to do. Could you please explain why this is the right thing to do? Can CORS failure happen synchronously somehow?
Yes, I forgot we had it on file already. CORS can probably fail synchronously if we get a hit in the MemoryCache.
Hmm, I think that memory cache works asynchronously. Not as sure about CORS own cache though. I think that it's worth checking before adding code for asynchronous event dispatch, we usually try to avoid it for events triggered by network.
The memory cache can definitely respond synchronously on a cache hit.
Comment on attachment 133395 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133395&action=review > Source/WebCore/loader/ImageLoader.cpp:90 > , m_firedLoad(true) > + , m_firedError(true) > , m_imageComplete(true) Is there any hope of merging any of these booleans? In particular, I would have thought m_firedLoad and m_firedError would be mutually exclusive. Also, I had never noticed that these are initialized to true. That's counter-intuitive to me, do you know the history behind it?
> Also, I had never noticed that these are initialized to true. That's counter-intuitive to me, do you know the history behind it? I think they have slightly non-intuitive names. The "true" state is the quiescent state. The "false" state means "I wanted to fire this event, but I haven't gotten around to it yet," which has some consequences (like remembering to cancel the event if we don't want it to fire anymore). Perhaps we should rename m_firedLoad (and friends) to m_loadEventIsPending (and negate the value). Then the initialization should make sense since "false" would be the quiescent state. As for merging the bools, they're somewhat separate pieces of state. I almost was tempted to break them out into a little subobject, but that seemed a bit overkill.
(In reply to comment #9) > > Also, I had never noticed that these are initialized to true. That's counter-intuitive to me, do you know the history behind it? > > I think they have slightly non-intuitive names. The "true" state is the quiescent state. The "false" state means "I wanted to fire this event, but I haven't gotten around to it yet," which has some consequences (like remembering to cancel the event if we don't want it to fire anymore). > > Perhaps we should rename m_firedLoad (and friends) to m_loadEventIsPending (and negate the value). Then the initialization should make sense since "false" would be the quiescent state. > > As for merging the bools, they're somewhat separate pieces of state. I almost was tempted to break them out into a little subobject, but that seemed a bit overkill. Ok, things make sense now. I'd be in favor of the m_loadEventIsPending naming scheme. I'm not sure whether they're truly separate pieces of state, though, unless we can have both a pending load event and a pending error event at the same time.
Comment on attachment 133395 [details] Patch Would you mind if we changed the names in a follow-up patch. The issue is that the state is public: bool haveFiredBeforeLoadEvent() const { return m_firedBeforeLoad; } bool haveFiredLoadEvent() const { return m_firedLoad; } so the renaming is going to spider a bit.
Comment on attachment 133395 [details] Patch Yeah, I think it's ok. I don't see any reason to hold this patch up because of the existing weird name convention.
Comment on attachment 133395 [details] Patch Clearing flags on attachment: 133395 Committed r112190: <http://trac.webkit.org/changeset/112190>
All reviewed patches have been landed. Closing bug.
*** Bug 80028 has been marked as a duplicate of this bug. ***