Post a microtask to load <img> elements.

Post a microtask to load <img> elements.

See the "stable state" (step 1) part of the algorithm at
http://www.whatwg.org/specs/web-apps/current-work/#the-img-element

This is a requirement for <picture>, because we need to wait with
loading until we know whether we're in a <picture> element.
It also makes it so that the order of setting .src and .crossorigin
does not matter.

Unfortunately, this means that some exceptions no longer have a
stack trace.

BUG=341047
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173943

[philipj_slow, 2014-03-19 03:42:12]

Posting a microtask is not equivalent to awaiting a stable state in the spec,
although making it almost true is a work in progress:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=24724

See also the discussion in https://codereview.chromium.org/153813002/

[cbiesinger, 2014-03-19 18:08:52]

On 2014/03/19 03:42:12, philipj wrote:
> Posting a microtask is not equivalent to awaiting a stable state in the spec,
> although making it almost true is a work in progress:
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=24724
> 
> See also the discussion in https://codereview.chromium.org/153813002/

Hmm, I just followed Rafael's suggestion in
https://codereview.chromium.org/153813002/#msg20

Do you think I shouldn't implement it this way?

[cbiesinger, 2014-03-19 22:22:59]

So there is a problem with tests...

LayoutTests/http/tests/security/isolatedWorld/bypass-main-world-csp.html and
others trigger some loads in an "isolated world", which has different security
aspects. But since we initiate the load later from a task, it doesn't see that
isolated world, causing the test to fail.

Not currently sure what to do about that...

[philipj_slow, 2014-03-20 03:07:21]

On 2014/03/19 18:08:52, cbiesinger wrote:
> On 2014/03/19 03:42:12, philipj wrote:
> > Posting a microtask is not equivalent to awaiting a stable state in the
spec,
> > although making it almost true is a work in progress:
> > https://www.w3.org/Bugs/Public/show_bug.cgi?id=24724
> > 
> > See also the discussion in https://codereview.chromium.org/153813002/
> 
> Hmm, I just followed Rafael's suggestion in
> https://codereview.chromium.org/153813002/#msg20
> 
> Do you think I shouldn't implement it this way?

I just wanted to make sure that you knew about the status of things here.
Posting a microtask likely isn't going to exactly match what the spec ends up
saying, but if rafaelw is OK with doing this while waiting for the spec fix I
won't stand in the way. For HTMLMediaElement I'm just going to wait for the
spec, to avoid writing complicated test cases that then have to be thrown away.
Here's a bunch of tests for testing "await a stable state" for HTMLMediaElement
that should give you some idea of what to test:

https://github.com/foolip/web-platform-tests/commit/dae508d45f20388a32926a426...

[rafaelw, 2014-03-24 19:37:23]

Sorry guys, I'm not sure I have much guidance here.

I'm trying to push for not having more than one notion of "script is done
mucking with stuff", and it makes sense to me that microtask work is the way we
do it.

However, I'm pretty far from the specific requirements here. I leave it to your
judgement, but I don't see anything obviously wrong with using a microtask here.

[cbiesinger, 2014-04-01 22:57:14]

So the final test failure is
http/tests/w3c/webperf/submission/Google/resource-timing/html/test_resource_reparenting.html

Now that we're loading the <img> asynchronously, the initiating document for the
img is different when you move the <img> around after creating it. This makes a
difference for the resource timing spec (and presumably also for things like
referrer headers, etc). Going to read some specs now to see what the right
behavior is...

[cbiesinger, 2014-04-01 23:35:27]

On 2014/04/01 22:57:14, cbiesinger wrote:
> So the final test failure is
>
http/tests/w3c/webperf/submission/Google/resource-timing/html/test_resource_reparenting.html
> 
> Now that we're loading the <img> asynchronously, the initiating document for
the
> img is different when you move the <img> around after creating it. This makes
a
> difference for the resource timing spec (and presumably also for things like
> referrer headers, etc). Going to read some specs now to see what the right
> behavior is...

http://lists.w3.org/Archives/Public/public-web-perf/2014Apr/0000.html

[cbiesinger, 2014-04-01 23:38:13]

Adam, Eric, this should be ready for review!

[abarth-chromium, 2014-04-01 23:57:14]

not lgtm

Please note that stable states and microtask checkpoints are not the same thing.
 We currently have no way of computing stable states in Blink because they keep
getting delayed if more async work gets scheduled.

It's not clear to me that adding this complexity to HTMLImageElement is
worthwhile.  We might want to think about other ways of solving the underlying
problem.

https://codereview.chromium.org/200923002/diff/160001/Source/core/html/HTMLIm...
File Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/200923002/diff/160001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.cpp:90: cancelPendingUpdate();
This algorithm is O(N^2) when tearing down the document.  If there are N
HTMLImageElements, then each one will do O(N) work to remove itself from the
queue, which is O(N^2) total.

https://codereview.chromium.org/200923002/diff/160001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.cpp:503: }
The HTMLImageElement shouldn't have any knowledge of isolated worlds...

https://codereview.chromium.org/200923002/diff/160001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.cpp:505: if
(m_imageLoader.shouldLoadImmediately() || isIsolatedWorld) {
It's wrong to make image loading work differently in isolated worlds.

https://codereview.chromium.org/200923002/diff/160001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.cpp:539: /* static */ void
HTMLImageElement::processUpdateFromElementQueue()
We don't use /* static */ comments in Blink

https://codereview.chromium.org/200923002/diff/160001/Source/core/loader/Imag...
File Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/200923002/diff/160001/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:239: return true;
Why are data URLs special?

[abarth-chromium, 2014-04-02 00:11:46]

On 2014/04/01 23:57:14, abarth wrote:
> Please note that stable states and microtask checkpoints are not the same
thing.
>  We currently have no way of computing stable states in Blink because they
keep
> getting delayed if more async work gets scheduled.

I talked with hixie in #whatwg about this topic.  I had misunderstood how stable
states work.  They are different than microtask checkpoints, but they don't get
delayed if there's more async work, which means we can actually compute them.  I
think we need to do more work to compute them, however, because they're a subset
of the microtask checkpoints (basically, just the ones at the top-level event
loop).

[cbiesinger, 2014-04-02 01:27:08]

On 2014/04/02 00:11:46, abarth wrote:
> On 2014/04/01 23:57:14, abarth wrote:
> > Please note that stable states and microtask checkpoints are not the same
> thing.
> >  We currently have no way of computing stable states in Blink because they
> keep
> > getting delayed if more async work gets scheduled.
> 
> I talked with hixie in #whatwg about this topic.  I had misunderstood how
stable
> states work.  They are different than microtask checkpoints, but they don't
get
> delayed if there's more async work, which means we can actually compute them. 
I
> think we need to do more work to compute them, however, because they're a
subset
> of the microtask checkpoints (basically, just the ones at the top-level event
> loop).

OK, so, your point here is that I should implement stable state first, instead
of approximating it with a microtask? I understand there's a difference, it just
seemed like doing it with a microtask was much closer to the spec than doing it
synchronously.

> It's not clear to me that adding this complexity to HTMLImageElement is
> worthwhile.  We might want to think about other ways of solving the underlying
> problem.

Are you saying that we should find a simpler way to implement loading from a
stable state/microtask, OR are you saying that we should change the spec not to
require async loading?

Just trying to understand your objection so I can respond appropriately :)

(Re: isolated world, there are tests that set up an isolated world with a
different security context before they set .src. Using 
 that as a signal to trigger the load synchronously seemed like the most
straightforward way to implement that - the other option would require somehow
storing the security context and passing it on.

[abarth-chromium, 2014-04-02 05:43:52]

On 2014/04/02 01:27:08, cbiesinger wrote:
> OK, so, your point here is that I should implement stable state first, instead
> of approximating it with a microtask? I understand there's a difference, it
just
> seemed like doing it with a microtask was much closer to the spec than doing
it
> synchronously.

Yeah, but we've had the synchronous behavior for a while.  If we're going to
change behaviors, we should change to the spec-complaint behavior rather than go
through a series of intermediate states.

> > It's not clear to me that adding this complexity to HTMLImageElement is
> > worthwhile.  We might want to think about other ways of solving the
underlying
> > problem.
> 
> Are you saying that we should find a simpler way to implement loading from a
> stable state/microtask, OR are you saying that we should change the spec not
to
> require async loading?
> 
> Just trying to understand your objection so I can respond appropriately :)

The former.  I think the spec makes sense as written.  We should try to find a
simpler way of implementing it.

> (Re: isolated world, there are tests that set up an isolated world with a
> different security context before they set .src. Using 
>  that as a signal to trigger the load synchronously seemed like the most
> straightforward way to implement that - the other option would require somehow
> storing the security context and passing it on.

Yes, that's one of the challenges with kicking off the load asynchronously:
there are other parts of the system that assume the load will be kicked off
synchronously.  Rather than having different loading behavior for all those
cases, we need to teach those parts of the system how to handle loads scheduled
in this way.

[cbiesinger, 2014-04-02 17:49:13]

Alright. I'm going to revive https://codereview.chromium.org/153813002/, figure
out a way to simplify the code here, and do something about the isolated world
issue.

[cbiesinger, 2014-04-03 00:26:52]

On 2014/04/02 17:49:13, cbiesinger wrote:
> Alright. I'm going to revive https://codereview.chromium.org/153813002/,
figure
> out a way to simplify the code here, and do something about the isolated world
> issue.

For my reference, the (only) test that needs the isolated world stuff is
http/tests/security/isolatedWorld/bypass-main-world-csp.html

[abarth-chromium, 2014-04-03 04:59:11]

On 2014/04/03 00:26:52, cbiesinger wrote:
> On 2014/04/02 17:49:13, cbiesinger wrote:
> > Alright. I'm going to revive https://codereview.chromium.org/153813002/,
> figure
> > out a way to simplify the code here, and do something about the isolated
world
> > issue.
> 
> For my reference, the (only) test that needs the isolated world stuff is
> http/tests/security/isolatedWorld/bypass-main-world-csp.html

One thing we could try is moving the code that captures the "bypass CSP" bit to
a place that runs synchronously.

If we're going to have a bunch of work run during stable states, maybe we want
to teach Timer how to run at stable states (or create another Timer-like class).
 That way we'll keep things in the right order.

[philipj_slow, 2014-04-03 19:36:36]

On 2014/04/03 04:59:11, abarth wrote:

> If we're going to have a bunch of work run during stable states, maybe we want
> to teach Timer how to run at stable states (or create another Timer-like
class).
>  That way we'll keep things in the right order.

https://codereview.chromium.org/153813002/ was pretty much that, but it's
blocked on a spec bug now.

[abarth-chromium, 2014-04-03 20:53:48]

On 2014/04/03 19:36:36, philipj wrote:
> https://codereview.chromium.org/153813002/ was pretty much that, but it's
> blocked on a spec bug now.

Ok, sounds like we are just blocked behind spec bugs then.

[rafaelw, 2014-04-04 17:58:49]

AFAICT, the need for stable state is exactly the same as the need for
microtasks: it's I want to wait until script is done doing it's thing before I
go do my thing.

As I noted on the bug, in discussion with Hixie about this, the only two reasons
he has for not using microtasks for the current uses of stable state are:

-Some vague idea that stable state work can be parallized (i.e. on multiple
threads)
-He wants setting a video src, then blocking script with an alert() to cause the
video to load while the alert is blocking.

[cbiesinger, 2014-04-22 23:46:52]

The spec issue is resolved (Hixie agreed to merge microtasks and stable states),
and this new patchset addresses most review comments, except for the test for
the shouldBypassMainWorldCSP. I will write that test Thursday (I'm OOO
tomorrow).

[cbiesinger, 2014-04-25 23:33:40]

OK, I added a test for the correct behavior of isolated world CSPs in
bypass-main-world-csp.html.

Adam, please take another look.

[abarth-chromium, 2014-04-25 23:59:43]

https://codereview.chromium.org/200923002/diff/240001/Source/core/loader/Imag...
File Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/200923002/diff/240001/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:69:
m_document->incrementLoadEventDelayCount();
Why is it the task's job to increment and decrement the delay load count?  That
seems more like the job of the loader.  I would think that the task's job is
just to call the loader back at the right time and to remember if we're supposed
to bypass CSP.  If we go that route, the Task doesn't need to hold onto the
document and we can remove the RefPtr<Document>.

https://codereview.chromium.org/200923002/diff/240001/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:73: frame &&
frame->script().shouldBypassMainWorldContentSecurityPolicy();
Can |frame| actually be zero here?

https://codereview.chromium.org/200923002/diff/240001/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:84: m_loader->m_pendingTask = 0;
This works seems like it should be done by the loader instead of the task.  That
way the loader doesn't need to be friends with the task.

https://codereview.chromium.org/200923002/diff/240001/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:85: m_loader->updateFromElement(true,
m_shouldBypassMainWorldContentSecurityPolicy);
Rather than passing a bool from the two different callers, can we refactor
updateFromElement into two different functions?  For example, one that schedules
the load (either immediately or delayed) and another that actually does the
load?

https://codereview.chromium.org/200923002/diff/240001/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:93: m_document.clear();
Once it's the loaders job to increment/decrement the LoadEventDelayCount, we can
just use WeakPtr<ImageLoader> and we don't have to re-implement the weak pointer
pattern manually.

[cbiesinger, 2014-05-01 00:09:07]

https://codereview.chromium.org/200923002/diff/240001/Source/core/loader/Imag...
File Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/200923002/diff/240001/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:69:
m_document->incrementLoadEventDelayCount();
On 2014/04/25 23:59:44, abarth wrote:
> Why is it the task's job to increment and decrement the delay load count? 
That
> seems more like the job of the loader.  I would think that the task's job is
> just to call the loader back at the right time and to remember if we're
supposed
> to bypass CSP.  If we go that route, the Task doesn't need to hold onto the
> document and we can remove the RefPtr<Document>.

Well the entire reason was your request to have a refptr-like object to keep
track of the delay load count. If I go back to manually calling
increment/decrement, then sure, I can move these calls back to ImageLoader.

https://codereview.chromium.org/200923002/diff/240001/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:73: frame &&
frame->script().shouldBypassMainWorldContentSecurityPolicy();
On 2014/04/25 23:59:44, abarth wrote:
> Can |frame| actually be zero here?

This was copied from
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...,
which this function will eventually call. So, I assumed that it can be null, but
I'm happy to remove it if you think that's safe.

https://codereview.chromium.org/200923002/diff/240001/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:84: m_loader->m_pendingTask = 0;
On 2014/04/25 23:59:44, abarth wrote:
> This works seems like it should be done by the loader instead of the task. 
That
> way the loader doesn't need to be friends with the task.

Done. (Note that the task is only friends implicitly, by virtue of being an
inner class)

https://codereview.chromium.org/200923002/diff/240001/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:85: m_loader->updateFromElement(true,
m_shouldBypassMainWorldContentSecurityPolicy);
On 2014/04/25 23:59:44, abarth wrote:
> Rather than passing a bool from the two different callers, can we refactor
> updateFromElement into two different functions?  For example, one that
schedules
> the load (either immediately or delayed) and another that actually does the
> load?

Done.

https://codereview.chromium.org/200923002/diff/240001/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:93: m_document.clear();
On 2014/04/25 23:59:44, abarth wrote:
> Once it's the loaders job to increment/decrement the LoadEventDelayCount, we
can
> just use WeakPtr<ImageLoader> and we don't have to re-implement the weak
pointer
> pattern manually.

Hm... WeakPtr requires refcounted objects?

[abarth-chromium, 2014-05-01 00:58:16]

On 2014/05/01 00:09:07, cbiesinger wrote:
>
https://codereview.chromium.org/200923002/diff/240001/Source/core/loader/Imag...
> File Source/core/loader/ImageLoader.cpp (right):
> 
>
https://codereview.chromium.org/200923002/diff/240001/Source/core/loader/Imag...
> Source/core/loader/ImageLoader.cpp:69:
> m_document->incrementLoadEventDelayCount();
> On 2014/04/25 23:59:44, abarth wrote:
> > Why is it the task's job to increment and decrement the delay load count? 
> That
> > seems more like the job of the loader.  I would think that the task's job is
> > just to call the loader back at the right time and to remember if we're
> supposed
> > to bypass CSP.  If we go that route, the Task doesn't need to hold onto the
> > document and we can remove the RefPtr<Document>.
> 
> Well the entire reason was your request to have a refptr-like object to keep
> track of the delay load count. If I go back to manually calling
> increment/decrement, then sure, I can move these calls back to ImageLoader.

Sorry for going back and forth about this topic.  We don't need to solve
everything in this CL, but IMHO the ideal outcome is for the ImageLoader to own
some sort of state object that increments and decrements the counter.  The goal
is to make the C++ type system enforce that the counter is balanced.  I'll stop
harping on this point though and we can do with something less robust if needed.

>
https://codereview.chromium.org/200923002/diff/240001/Source/core/loader/Imag...
> Source/core/loader/ImageLoader.cpp:73: frame &&
> frame->script().shouldBypassMainWorldContentSecurityPolicy();
> On 2014/04/25 23:59:44, abarth wrote:
> > Can |frame| actually be zero here?
> 
> This was copied from
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...,
> which this function will eventually call. So, I assumed that it can be null,
but
> I'm happy to remove it if you think that's safe.

If we don't have a test demonstrating that its needed, let's remove it.  If it
crashes, then we can add a test.

>
https://codereview.chromium.org/200923002/diff/240001/Source/core/loader/Imag...
> Source/core/loader/ImageLoader.cpp:93: m_document.clear();
> On 2014/04/25 23:59:44, abarth wrote:
> > Once it's the loaders job to increment/decrement the LoadEventDelayCount, we
> can
> > just use WeakPtr<ImageLoader> and we don't have to re-implement the weak
> pointer
> > pattern manually.
> 
> Hm... WeakPtr requires refcounted objects?

Nope.  You just need a weak pointer factory member in the object to produce one.

[cbiesinger, 2014-05-01 01:27:33]

On 2014/05/01 00:58:16, abarth wrote:
> On 2014/05/01 00:09:07, cbiesinger wrote:
> >
>
https://codereview.chromium.org/200923002/diff/240001/Source/core/loader/Imag...
> > File Source/core/loader/ImageLoader.cpp (right):
> > 
> >
>
https://codereview.chromium.org/200923002/diff/240001/Source/core/loader/Imag...
> > Source/core/loader/ImageLoader.cpp:69:
> > m_document->incrementLoadEventDelayCount();
> > On 2014/04/25 23:59:44, abarth wrote:
> > > Why is it the task's job to increment and decrement the delay load count? 
> > That
> > > seems more like the job of the loader.  I would think that the task's job
is
> > > just to call the loader back at the right time and to remember if we're
> > supposed
> > > to bypass CSP.  If we go that route, the Task doesn't need to hold onto
the
> > > document and we can remove the RefPtr<Document>.
> > 
> > Well the entire reason was your request to have a refptr-like object to keep
> > track of the delay load count. If I go back to manually calling
> > increment/decrement, then sure, I can move these calls back to ImageLoader.
> 
> Sorry for going back and forth about this topic.  We don't need to solve
> everything in this CL, but IMHO the ideal outcome is for the ImageLoader to
own
> some sort of state object that increments and decrements the counter.  The
goal
> is to make the C++ type system enforce that the counter is balanced.  I'll
stop
> harping on this point though and we can do with something less robust if
needed.

OK, so I could make a generic helper class IncrementDocumentDelayCount, to which
ImageLoader will store an OwnPtr addition to m_pendingTask? I guess it bothers
me a little to have two members which will always be set/unset in conjunction,
but that's ok.

I will investigate the other two things tomorrow.

BTW, I really appreciate your superfast review turnaround times, even if I often
upload a patch just before I go home.

[cbiesinger, 2014-05-01 23:14:50]

Comments fixed, including removing the nullcheck for frame.

[abarth-chromium, 2014-05-02 17:04:58]

https://codereview.chromium.org/200923002/diff/290036/Source/core/loader/Imag...
File Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/200923002/diff/290036/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:274: return;
Do we need to do this check both here and in doUpdateFromElement?

https://codereview.chromium.org/200923002/diff/290036/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:277: if (!attr.isNull() && !url.isNull()) {
Won't imageURL() always return a null |url| if |attr| is null (care of line
323)?

https://codereview.chromium.org/200923002/diff/290036/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:282: m_pendingTask = WeakPtr<Task>();
We should add a clear() method to WeakPtr

https://codereview.chromium.org/200923002/diff/290036/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:288: Task* task = new Task(this);
OwnPtr<Task> task = adoptPtr(new Task(this));

All calls to |new| should be wrapped immediately in either adoptPtr or adoptRef
so it's clear there's no leak.  To see that there's no leak in this code, you
need to look down a few lines.

https://codereview.chromium.org/200923002/diff/290036/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:290:
Microtask::enqueueMicrotask(adoptPtr(task));
s/adoptPtr(task)/task.release()/

https://codereview.chromium.org/200923002/diff/290036/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:318: return url;
This is a third place this check appears.  Shouldn't we be able to just make
this check in one place?

[cbiesinger, 2014-05-02 23:24:03]

Comments fixed, but now I'm tracking down a strange SVG test failure...

https://codereview.chromium.org/200923002/diff/290036/Source/core/loader/Imag...
File Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/200923002/diff/290036/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:274: return;
On 2014/05/02 17:04:59, abarth wrote:
> Do we need to do this check both here and in doUpdateFromElement?

Oh, no, can be removed from doUpdate. Done.

https://codereview.chromium.org/200923002/diff/290036/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:277: if (!attr.isNull() && !url.isNull()) {
On 2014/05/02 17:04:59, abarth wrote:
> Won't imageURL() always return a null |url| if |attr| is null (care of line
> 323)?

Indeed, done.

https://codereview.chromium.org/200923002/diff/290036/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:282: m_pendingTask = WeakPtr<Task>();
On 2014/05/02 17:04:59, abarth wrote:
> We should add a clear() method to WeakPtr

Turns out that method exists and I just missed it :/

https://codereview.chromium.org/200923002/diff/290036/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:288: Task* task = new Task(this);
On 2014/05/02 17:04:59, abarth wrote:
> OwnPtr<Task> task = adoptPtr(new Task(this));
> 
> All calls to |new| should be wrapped immediately in either adoptPtr or
adoptRef
> so it's clear there's no leak.  To see that there's no leak in this code, you
> need to look down a few lines.

Done.

https://codereview.chromium.org/200923002/diff/290036/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:290:
Microtask::enqueueMicrotask(adoptPtr(task));
On 2014/05/02 17:04:59, abarth wrote:
> s/adoptPtr(task)/task.release()/

Done.

https://codereview.chromium.org/200923002/diff/290036/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:318: return url;
On 2014/05/02 17:04:59, abarth wrote:
> This is a third place this check appears.  Shouldn't we be able to just make
> this check in one place?

Yeah, we don't need it here, a leftover from an earlier version.

[abarth-chromium, 2014-05-03 01:22:31]

LGTM!

https://codereview.chromium.org/200923002/diff/340001/Source/core/dom/Increme...
File Source/core/dom/IncrementLoadEventDelayCount.h (right):

https://codereview.chromium.org/200923002/diff/340001/Source/core/dom/Increme...
Source/core/dom/IncrementLoadEventDelayCount.h:17: class
IncrementLoadEventDelayCount {
It would be nice to deploy this class in more places, but that's for another CL.
 :)

https://codereview.chromium.org/200923002/diff/340001/Source/core/loader/Imag...
File Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/200923002/diff/340001/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:76: clearLoader();
Why do we need to clear the loader from the destructor?  It's just setting a
private member to zero.

https://codereview.chromium.org/200923002/diff/340001/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:174: m_pendingTask.clear();
Do we need to call clearLoader() on the m_pendingTask?

[Erik Dahlström (inactive), 2014-05-05 08:41:00]

https://codereview.chromium.org/200923002/diff/340001/Source/core/loader/Imag...
File Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/200923002/diff/340001/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:316: if (!attr.isNull() &&
!stripLeadingAndTrailingHTMLSpaces(attr).isEmpty()) {
technically this may not always be a 'src' attribute (in svg it is usually
xlink:href), so applying html-specific handling here might possibly be wrong.
It's probably fine though, and svg and html should be the same here IMHO.

Feel free to tweak the comment to reflect this, but no need to change the code.

https://codereview.chromium.org/200923002/diff/340001/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:331: return false;
should we delay loads for <svg:image> elements?

[cbiesinger, 2014-05-05 22:39:29]

Thanks for the lgtm! Uploaded with minor changes.

I'll submit as soon as I figure out how my change caused
svg/wicd/test-rightsizing-{a,b}.xhtml to fail...

https://codereview.chromium.org/200923002/diff/340001/Source/core/dom/Increme...
File Source/core/dom/IncrementLoadEventDelayCount.h (right):

https://codereview.chromium.org/200923002/diff/340001/Source/core/dom/Increme...
Source/core/dom/IncrementLoadEventDelayCount.h:17: class
IncrementLoadEventDelayCount {
On 2014/05/03 01:22:31, abarth wrote:
> It would be nice to deploy this class in more places, but that's for another
CL.
>  :)

Yep :) The plugin element could easily use it.

https://codereview.chromium.org/200923002/diff/340001/Source/core/loader/Imag...
File Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/200923002/diff/340001/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:76: clearLoader();
On 2014/05/03 01:22:31, abarth wrote:
> Why do we need to clear the loader from the destructor?  It's just setting a
> private member to zero.

Oh, yeah, a leftover. Let me remove that.

https://codereview.chromium.org/200923002/diff/340001/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:174: m_pendingTask.clear();
On 2014/05/03 01:22:31, abarth wrote:
> Do we need to call clearLoader() on the m_pendingTask?

No - let me add a comment to this effect:
    // We don't need to call clearLoader here: Either we were called from the
    // task, or our caller updateFromElement cleared the task's loader (and set
    // m_pendingTask to null).

https://codereview.chromium.org/200923002/diff/340001/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:316: if (!attr.isNull() &&
!stripLeadingAndTrailingHTMLSpaces(attr).isEmpty()) {
On 2014/05/05 08:41:01, Erik Dahlström wrote:
> technically this may not always be a 'src' attribute (in svg it is usually
> xlink:href), so applying html-specific handling here might possibly be wrong.
> It's probably fine though, and svg and html should be the same here IMHO.
> 
> Feel free to tweak the comment to reflect this, but no need to change the
code.

I'm not changing this code (and comment) - it's identical to what used to be on
line 142.

https://codereview.chromium.org/200923002/diff/340001/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:331: return false;
On 2014/05/05 08:41:01, Erik Dahlström wrote:
> should we delay loads for <svg:image> elements?

I think so. Do you disagree?

[cbiesinger, 2014-05-12 01:59:23]

Adam - I would like to fix the remaining test failure
(svg/wicd/test-rightsizing-{a,b}.xhtml) by adding an
isObjectElement/isEmbedElement check to shouldLoadImmediately as per the patch I
just uploaded. Does that sound OK to you? <object>/<embed> don't need the delay
logic because their loading works differently anyway. See also
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-elemen...,
which doesn't require it.

[abarth-chromium, 2014-05-12 04:33:25]

lgtm

[Erik Dahlström (inactive), 2014-05-12 07:44:29]

On 2014/05/12 01:59:23, cbiesinger wrote:
> Adam - I would like to fix the remaining test failure
> (svg/wicd/test-rightsizing-{a,b}.xhtml) by adding an
> isObjectElement/isEmbedElement check to shouldLoadImmediately as per the patch
I
> just uploaded. Does that sound OK to you? <object>/<embed> don't need the
delay
> logic because their loading works differently anyway. See also
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-elemen...,
> which doesn't require it.

Why not make it check for <img> element instead, and do shouldLoadImmediately
for everything else?

For svg it's very common to use <iframe> elements for referencing, and they
should really behave the same as <object> and <embed> in this regard.

[cbiesinger, 2014-05-13 07:42:59]

On 2014/05/12 07:44:29, Erik Dahlström wrote:
> On 2014/05/12 01:59:23, cbiesinger wrote:
> > Adam - I would like to fix the remaining test failure
> > (svg/wicd/test-rightsizing-{a,b}.xhtml) by adding an
> > isObjectElement/isEmbedElement check to shouldLoadImmediately as per the
patch
> I
> > just uploaded. Does that sound OK to you? <object>/<embed> don't need the
> delay
> > logic because their loading works differently anyway. See also
> >
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-elemen...,
> > which doesn't require it.
> 
> Why not make it check for <img> element instead, and do shouldLoadImmediately
> for everything else?
> 
> For svg it's very common to use <iframe> elements for referencing, and they
> should really behave the same as <object> and <embed> in this regard.

Eric - <iframe> does not use ImageLoader. In fact, loading of SVG files should
never go through this codepath (maybe except for <img src=foo.svg>). But an
<svg:image> should behave the same as an <html:img> and go through this
codepath.

[cbiesinger, 2014-05-13 07:43:07]

The CQ bit was checked by cbiesinger@chromium.org

[commit-bot: I haz the power, 2014-05-13 07:43:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/cbiesinger@chromium.org/200923002/380001

[commit-bot: I haz the power, 2014-05-13 08:58:24]

Change committed as 173943