1067345 - Speculative parser doesn't handle <picture>/srcset based resources

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Yoav Weiss]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/37.0.2062.120 Safari/537.36

Steps to reproduce:

Looking at the code that handles speculative loads at http://mxr.mozilla.org/mozilla-central/source/parser/html/nsHtml5TreeBuilderCppSupplement.h#103 picture based resources don't seem to be handled.

Furthermore, it seems that <img> based resources will just preload the 'src' attribute regardless of the presence of the srcset attribute or a <picture> parent.

So, it seems like the problem is two-fold: src resources should not be preloaded for such resources, and srcset/<picture> based resources should be.

Comment 1

[Henri Sivonen (:hsivonen)]

At the very least we should not pref <picture> on by default as long as we preload <img src> as a child of <picture> or with an srcset attribute without actually checking if the <img src> will end up being used.

But indeed, it would be even better to gather the relevant <picture>/srcset bits in full in the preloader and ship them to the main thread for evaluation so that the right URL can be preloaded.

Comment 2

[John Schoenick [:johns]]

So the quirk fix is to not take action on <img src> with a <picture> parent or <srcset> attr, one of which is necessary to trigger non-simple-src rules.

To extend early image opening off thread to the full <picture> implementation, the ResponsiveImageSelector class is already designed to be runnable off thread, but there are some scenarios where e.g. viewport width isn't available yet that would need to return a indeterminate response -- we would resume loading in that case at element creation on the dom thread.

Comment 3

[John Schoenick [:johns]]

Attachment: Part 1.1 - Move ResponsiveImageSelector selection into GetSelectedImage instead of GetBestCandidateIndex

Comment 4

[John Schoenick [:johns]]

Attachment: Part 1.2 - ResponsiveImageSelector: Rename best candidate -> selected candidate, add clear function

Comment 5

[John Schoenick [:johns]]

Attachment: Part 1.3 - ResponsiveImageSelector should resolve the URL at selection time, store unresolved strings for candidates

Comment 6

[John Schoenick [:johns]]

Attachment: Part 1.4 - Allow ResponsiveImageSelector to take a nsIDocument as well as a specific nsIContent

Comment 7

[John Schoenick [:johns]]

Attachment: Part 2 - Add HTMLImageElement::SelectSourceForTagWithAttrs to do <picture> source selection without a DOM

Comment 8

[John Schoenick [:johns]]

Attachment: Part 3 - Implement speculative load operations for <picture> and <img srcset>

Comment 9

[John Schoenick [:johns]]

Attachment: Part 1.1 - Move ResponsiveImageSelector selection into GetSelectedImage instead of GetBestCandidateIndex

Comment 10

[John Schoenick [:johns]]

Attachment: Part 1.2 - ResponsiveImageSelector: Rename best candidate -> selected candidate, add clear function

Comment 11

[John Schoenick [:johns]]

Attachment: Part 1.3 - ResponsiveImageSelector should resolve the URL at selection time, store unresolved strings for candidates

Comment 12

[John Schoenick [:johns]]

Attachment: Part 1.4 - Allow ResponsiveImageSelector to take a nsIDocument as well as a specific nsIContent

Comment 13

[John Schoenick [:johns]]

Attachment: Part 2 - Add HTMLImageElement::SelectSourceForTagWithAttrs to do <picture> source selection without a DOM

Comment 14

[John Schoenick [:johns]]

Attachment: Part 3 - Implement speculative load operations for <picture> and <img srcset>

Comment 15

[John Schoenick [:johns]]

Attachment: Add tests for img/picture/srcset preloading

Comment 16

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/afeff812b5dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cc6d580cda6
https://hg.mozilla.org/integration/mozilla-inbound/rev/7884f103f1e7
https://hg.mozilla.org/integration/mozilla-inbound/rev/406045c02f89
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1b3abf6bb98
https://hg.mozilla.org/integration/mozilla-inbound/rev/b64c085b06e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c39680606e8

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/afeff812b5dd
https://hg.mozilla.org/mozilla-central/rev/4cc6d580cda6
https://hg.mozilla.org/mozilla-central/rev/7884f103f1e7
https://hg.mozilla.org/mozilla-central/rev/406045c02f89
https://hg.mozilla.org/mozilla-central/rev/c1b3abf6bb98
https://hg.mozilla.org/mozilla-central/rev/b64c085b06e1
https://hg.mozilla.org/mozilla-central/rev/9c39680606e8

Comment 18

[Boris Zbarsky [:bzbarsky]]

Who reviewed these patches?  I don't see reviews from anyone who's a peer for the HTML parser here, or reviews from anyone at all.

These added a ton of bloat to the nsHtml5SpeculativeLoad struct...

Comment 19

[Josh Matthews [:jdm]]

Apparently nobody. I thought I had checked that, but I guess it got lost in the shuffle when I inherited the responsive image work.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Alright, thanks.  I'll see what Henri ends up saying in bug 1382020 about how this code should actually work...