Add preloader support for picture based source selection

Add preloader support for picture based source selection

This CL adds preloader support to the source selection algorithm used by HTMLImageElement when it has a picture parent.

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

[Yoav Weiss, 2014-05-09 07:23:51]

Since the normal image loading is waiting for
https://codereview.chromium.org/200923002 to land, I started out by implementing
the preloadScanner parts of picture source selection.

[eseidel, 2014-05-09 07:30:36]

https://codereview.chromium.org/265763010/diff/100001/Source/core/html/parser...
File Source/core/html/parser/HTMLPreloadScanner.h (right):

https://codereview.chromium.org/265763010/diff/100001/Source/core/html/parser...
Source/core/html/parser/HTMLPreloadScanner.h:80: ,
pictureSourceURLStack(pictureSourceURLStack)
Why do these need to be stored on the checkpoint?  I guess this is part of the
fake-tree-builder state?

[Yoav Weiss, 2014-05-09 07:37:45]

On 2014/05/09 07:30:36, eseidel wrote:
>
https://codereview.chromium.org/265763010/diff/100001/Source/core/html/parser...
> File Source/core/html/parser/HTMLPreloadScanner.h (right):
> 
>
https://codereview.chromium.org/265763010/diff/100001/Source/core/html/parser...
> Source/core/html/parser/HTMLPreloadScanner.h:80: ,
> pictureSourceURLStack(pictureSourceURLStack)
> Why do these need to be stored on the checkpoint?  I guess this is part of the
> fake-tree-builder state?

Yeah. I'm not really building a fake tree, just keeping around a stack of picked
source URLs, so that when an image is encountered, it can use the right source.

[eseidel, 2014-05-09 08:54:07]

For context: Checkpoints occur at token boundaries.  We only ever roll-back to
checkpoints which occurred at </script> boundaries.  I'm not sure you're going
to need to store picture source lists there, unless you need to support
rollback.

Rollback only occurs when document.write is called and our tokenized chunks need
to be discarded because document.write left us in a state other than the Data
state and thus our tokens since then may be different.

It may be sufficient to just keep the source list outside of the checkpoint list
and discard it if we ever rollback.  I doubt that checkpoints we roll back to
ever can contain a non-empty source list?  Do we have a test which shows they
can?

[Yoav Weiss, 2014-05-09 09:17:52]

On 2014/05/09 08:54:07, eseidel wrote:
> For context: Checkpoints occur at token boundaries.  We only ever roll-back to
> checkpoints which occurred at </script> boundaries.  I'm not sure you're going
> to need to store picture source lists there, unless you need to support
> rollback.
> 
> Rollback only occurs when document.write is called and our tokenized chunks
need
> to be discarded because document.write left us in a state other than the Data
> state and thus our tokens since then may be different.

OK, I misunderstood checkpoints then. If they're used only to support
document.write, we can probably ignore them (at the price of prediction misses
when document.write is used inside a picture element, which shouldn't be
something authors are doing, ever).
I'll drop that part from the patch.

> 
> It may be sufficient to just keep the source list outside of the checkpoint
list
> and discard it if we ever rollback.  I doubt that checkpoints we roll back to
> ever can contain a non-empty source list?  Do we have a test which shows they
> can?

[eseidel, 2014-05-09 09:28:27]

If you ignore checkpoints (which sounds fine to me), I would be sure to just add
ASSERTS to validate your expectations.  For example that <script> is never run
inside <picture> or that the source list is always empty when a checkpoint is
hit, etc.

For example, I could imagine you'd need to care about checkpoints if the
following worked:

<picture>
<source ...>
<script>
document.write("</picture><picture>");
</script>
<source ...>
</picture>

[eseidel, 2014-05-09 09:30:22]

Actually, that won't make you care about checkpoints (since you begin and end in
the same tokenizer state), but rather that case makes it clear that the
preloader will sometimes be wrong.

<picture>
<script>
document.write("<foo");
</script>
></picture>

is more a case where you need to know about checkpoins, since they change the
meaning of later characters in the source document, and thus change the
tokenization and cause us to roll-back to a checkpoint and re-tokenize.

[Yoav Weiss, 2014-05-10 07:12:16]

On 2014/05/09 09:28:27, eseidel wrote:
> If you ignore checkpoints (which sounds fine to me), I would be sure to just
add
> ASSERTS to validate your expectations.  For example that <script> is never run
> inside <picture> or that the source list is always empty when a checkpoint is
> hit, etc.
> 
> For example, I could imagine you'd need to care about checkpoints if the
> following worked:
> 
> <picture>
> <source ...>
> <script>
> document.write("</picture><picture>");
> </script>
> <source ...>
> </picture>


On 2014/05/09 09:30:22, eseidel wrote:
> Actually, that won't make you care about checkpoints (since you begin and end
in
> the same tokenizer state), but rather that case makes it clear that the
> preloader will sometimes be wrong.
> 
> <picture>
> <script>
> document.write("<foo");
> </script>
> ></picture>

Are ASSERTs the best way to keep this in check? Authors can have such code, even
if they have no good reason to do so.
Maybe just add tests that make sure that the preloader is wrong in these cases
in the way that we think it should be wrong? (with comments stating why)

> 
> is more a case where you need to know about checkpoins, since they change the
> meaning of later characters in the source document, and thus change the
> tokenization and cause us to roll-back to a checkpoint and re-tokenize.

Your example made me realize that my current stack based tracking is too naive.
e.g. the following would break it (or at least preload the wrong resources).
<picture>
<source>
<div>
</picture>
</div>
<img>
</picture>

I'll fix that and add proper tests.

[Yoav Weiss, 2014-05-12 13:49:25]

On 2014/05/10 07:12:16, Yoav Weiss wrote:
> On 2014/05/09 09:28:27, eseidel wrote:
> > If you ignore checkpoints (which sounds fine to me), I would be sure to just
> add
> > ASSERTS to validate your expectations.  For example that <script> is never
run
> > inside <picture> or that the source list is always empty when a checkpoint
is
> > hit, etc.
> > 
> > For example, I could imagine you'd need to care about checkpoints if the
> > following worked:
> > 
> > <picture>
> > <source ...>
> > <script>
> > document.write("</picture><picture>");
> > </script>
> > <source ...>
> > </picture>
> 
> 
> On 2014/05/09 09:30:22, eseidel wrote:
> > Actually, that won't make you care about checkpoints (since you begin and
end
> in
> > the same tokenizer state), but rather that case makes it clear that the
> > preloader will sometimes be wrong.
> > 
> > <picture>
> > <script>
> > document.write("<foo");
> > </script>
> > ></picture>
> 
> Are ASSERTs the best way to keep this in check? Authors can have such code,
even
> if they have no good reason to do so.
> Maybe just add tests that make sure that the preloader is wrong in these cases
> in the way that we think it should be wrong? (with comments stating why)

Added such tests.

> 
> > 
> > is more a case where you need to know about checkpoins, since they change
the
> > meaning of later characters in the source document, and thus change the
> > tokenization and cause us to roll-back to a checkpoint and re-tokenize.
> 
> Your example made me realize that my current stack based tracking is too
naive.
> e.g. the following would break it (or at least preload the wrong resources).
> <picture>
> <source>
> <div>
> </picture>
> </div>
> <img>
> </picture>
> 
> I'll fix that and add proper tests.

Fixed on the train over to BlinkOn (as planned :) ).

[eseidel, 2014-05-15 06:29:13]

https://codereview.chromium.org/265763010/diff/120001/Source/core/html/parser...
File Source/core/html/parser/HTMLPreloadScanner.h (right):

https://codereview.chromium.org/265763010/diff/120001/Source/core/html/parser...
Source/core/html/parser/HTMLPreloadScanner.h:91: const StringImpl* tagImpl;
Why not just String?

[Yoav Weiss, 2014-05-15 07:40:51]

On 2014/05/15 06:29:13, eseidel wrote:
>
https://codereview.chromium.org/265763010/diff/120001/Source/core/html/parser...
> File Source/core/html/parser/HTMLPreloadScanner.h (right):
> 
>
https://codereview.chromium.org/265763010/diff/120001/Source/core/html/parser...
> Source/core/html/parser/HTMLPreloadScanner.h:91: const StringImpl* tagImpl;
> Why not just String?

I saw tagImpl used to run match() (e.g. in scanCommon) and copied that pattern.
I'll switch it to String and run match with .impl(), if that's preferred.

[Yoav Weiss, 2014-05-20 07:34:49]

On 2014/05/15 07:40:51, Yoav Weiss wrote:
> On 2014/05/15 06:29:13, eseidel wrote:
> >
>
https://codereview.chromium.org/265763010/diff/120001/Source/core/html/parser...
> > File Source/core/html/parser/HTMLPreloadScanner.h (right):
> > 
> >
>
https://codereview.chromium.org/265763010/diff/120001/Source/core/html/parser...
> > Source/core/html/parser/HTMLPreloadScanner.h:91: const StringImpl* tagImpl;
> > Why not just String?
> 
> I saw tagImpl used to run match() (e.g. in scanCommon) and copied that
pattern.
> I'll switch it to String and run match with .impl(), if that's preferred.

Actually changing it to string is not trivial (at least I didn't find a trivial
way to do that) since the tagImpl Parent is taking as input is a const.

[Yoav Weiss, 2014-05-20 15:41:52]

Fixed an algorithm issue with self closing tokens pointed out by Simon Pieters.

[eseidel, 2014-05-21 22:44:18]

I'm still not quite sure what the stack is for, perhaps you can explain to me
over irc.

https://codereview.chromium.org/265763010/diff/140001/Source/core/html/parser...
File Source/core/html/parser/HTMLPreloadScanner.cpp (right):

https://codereview.chromium.org/265763010/diff/140001/Source/core/html/parser...
Source/core/html/parser/HTMLPreloadScanner.cpp:184: // FIXME - What should
happen if we have multiple crossOrigin attributes with different values?
I figured last one, but it looks like first does:
http://jsbin.com/giwemivo/1/edit

Internally for all these things we use getAttribute.

Obviously if someone sets these twice from javascript the last one wins.

https://codereview.chromium.org/265763010/diff/140001/Source/core/html/parser...
Source/core/html/parser/HTMLPreloadScanner.cpp:400: static bool
startNesting(const StringImpl* tagImpl)
Can you give this a more descriptive name?

[eseidel, 2014-05-21 23:15:05]

Discussed with Yoav via #blink and recommended that he ditch the stack
complexity and just track the last seen sourcURL from a <picture> start tag
which he clears when he sees a </picture> end tag.  that will get the common
case of no-ntesting correct, and even the accidental nesting case correct:
<picture source=foo><img><picture><picture source=bar><img></picture> (note the
missing / in the first picture) at the cost of getting the crazy intentional
case of nesting wrong: <picture source=foo><picture
source=bar><img></picture><img></picture> (the second <img> won't know its
source).

Hopefully that will be much simpler code and he won't need to know then which
elements are self-closing in the treebuidler in order to keep a valid element
stack.  (Then again, maybe the TreeBuidlerSimulator already tracks this or
should?)

[Yoav Weiss, 2014-05-22 12:24:35]

On 2014/05/21 23:15:05, eseidel wrote:
> Discussed with Yoav via #blink and recommended that he ditch the stack
> complexity and just track the last seen sourcURL from a <picture> start tag
> which he clears when he sees a </picture> end tag.  that will get the common
> case of no-ntesting correct, and even the accidental nesting case correct:
> <picture source=foo><img><picture><picture source=bar><img></picture> (note
the
> missing / in the first picture) at the cost of getting the crazy intentional
> case of nesting wrong: <picture source=foo><picture
> source=bar><img></picture><img></picture> (the second <img> won't know its
> source).
> 
> Hopefully that will be much simpler code and he won't need to know then which
> elements are self-closing in the treebuidler in order to keep a valid element
> stack.  (Then again, maybe the TreeBuidlerSimulator already tracks this or
> should?)

Modified the code and the tests accordingly.

[eseidel, 2014-05-23 20:39:06]

lgtm

[Yoav Weiss, 2014-05-23 20:43:57]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-05-23 20:44:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yoav@yoav.ws/265763010/180001

[commit-bot: I haz the power, 2014-05-23 21:11:41]

Change committed as 174711