Ticapix/fix srcset discovery

[ticapix]

What this PR changes

changed the default discovery function to properly detect srcset url according to the specs

Rationale

srcset discovery function wasn't parsing correctly url with zero or more than 2 args.
Added a test as an exemple.

[ticapix]

Any update if this PR will be merged ?

[fredrikekelund]

Hi @ticapix! Sorry about the slow response here. I've never encountered a srcset attribute with mixed width and pixel densite descriptors, and looking at MDN, it appears to be invalid syntax. I don't think simplecrawler should be too picky about standards if this is something used a lot in the wild, but we have also made the discovery functionality intentionally pluggable, meaning it's easy to swap out for your own needs. Do you know that this is a syntax that appears on a lot of different sites?

[ticapix]

Thanks for looking into it.
From the draft spec http://w3c.github.io/html/semantics-embedded-content.html#element-attrdef-img-srcset, it's a valid syntax. Maybe MDN is describing Mozilla implementation.
But anyway, I would prefer simplecrawler to get better link discovery than carry out to the letter the MDN or other browser documentation.
I hope this PR will be useful to others.

[fredrikekelund]

Hmm well, the way I read that spec, we can only have "zero or one" of a width descriptor or a pixel density descriptor. I absolutely agree that simplecrawler shouldn't get too bogged down in HTML standards if we see a different syntax being used in the wild, but it's also unnecessary to support an invalid syntax if it's not being widely used. Do you know some places where it is being used?

[ticapix]

https://www.ovh.com/fr/
https://www.runabove.com

[fredrikekelund]

Could we remove 800w towards the end of this line? As we discussed previously, I don't think the spec permits two descriptors there, so it would be better to not make that part of the test suite (even if the new implementation would unofficially support that syntax)

[fredrikekelund]

I had another look at the diff here, and I don't see any harm in the changes you propose to crawler.js. The current test suite has a minor fault which you got rid of as well, so that's good. I left a single line comment in the diff, but after we've addressed that, I think we're good to merge :)

[ticapix]

should be good now.

[fredrikekelund]

Thanks, @ticapix!