Have srcset respond to viewport changes

Have srcset respond to viewport changes

This CL gives srcset+sizes based images the ability to react to viewport changes, by selecting a more fitting resource, and by modifying their intrinsic sizing according to the viewport.

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

[Yoav Weiss, 2014-07-11 01:31:23]

The patch turned out to be bigger then I expected, so please let me know if
you'd prefer that I'd split it up.

It is comprised of 3 inter-dependent parts:
* Viewport change notifications
* Sizes +srcset awareness if their values are dependent on the viewport
* ImageLoader's ability to force intrinsic sizing update

[sof, 2014-07-11 05:46:41]

+oilpan-reviews.

[haraken, 2014-07-11 06:10:21]

I guess this CL won't compile in oilpan builds. Once it compiles, I'm happy to
take another close look.

https://codereview.chromium.org/369423002/diff/100001/Source/core/frame/Viewp...
File Source/core/frame/ViewportChangeNotifier.h (right):

https://codereview.chromium.org/369423002/diff/100001/Source/core/frame/Viewp...
Source/core/frame/ViewportChangeNotifier.h:15: virtual ~ViewportChangeListener()
{ }

You'll need to use
DECLARE_EMPTY_VIRTUAL_DESTRUCTOR_WILL_BE_REMOVED(ViewportChangeListener) in the
header file and use
DEFINE_EMPTY_DESTRUCTOR_WILL_BE_REMOVED(ViewportChangeListener).

https://codereview.chromium.org/369423002/diff/100001/Source/core/frame/Viewp...
Source/core/frame/ViewportChangeNotifier.h:18: class ViewportChangeNotifier {

This should be:

class ViewportChangeNotifier : public
NoBaseWillBeGarbageCollected<ViewportChangeNotifier> { ... };

https://codereview.chromium.org/369423002/diff/100001/Source/core/frame/Viewp...
Source/core/frame/ViewportChangeNotifier.h:30: ListenerContainer m_listeners;

You need to add a trace() method and trace m_listeners.

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

https://codereview.chromium.org/369423002/diff/100001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.cpp:86:
document().viewportChangeNotifier().removeListener(m_viewportListener);

In oilpan, destructors cannot touch other on-heap objects.

I think you can make ViewportChangeNotifier::m_listeners a hash set of weak
members, and ask oilpan's GC to automatically clear entries of dead elements.

https://codereview.chromium.org/369423002/diff/100001/Source/core/html/HTMLIm...
File Source/core/html/HTMLImageElement.h (right):

https://codereview.chromium.org/369423002/diff/100001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.h:105: class ImageViewportChangeListener:
public ViewportChangeListener {

Add FINAL.

https://codereview.chromium.org/369423002/diff/100001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.h:120:
ImageViewportChangeListener(HTMLImageElement* element)

Add explicit.

https://codereview.chromium.org/369423002/diff/100001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.h:127: HTMLImageElement* m_element;

I guess this should be RawPtrWillBeMember<HTMLImageElement>.

You need to add a trace() method and trace m_element.

[esprehn, 2014-07-14 08:39:50]

This is not correct, you shouldn't be adding more viewport size machinery, just
use the stuff Christian already built. You added synchronous callbacks... :(

https://codereview.chromium.org/369423002/diff/120001/Source/core/css/parser/...
File Source/core/css/parser/SizesAttributeParser.cpp (right):

https://codereview.chromium.org/369423002/diff/120001/Source/core/css/parser/...
Source/core/css/parser/SizesAttributeParser.cpp:26: viewportDependant =
parser.viewportDependant();
You should hide this logic inside an instance method.

SizesAttributeParser parser(attribute, mediaValues);
parser.parse();
if (parser.viewportDependant()) {
  ...
}

https://codereview.chromium.org/369423002/diff/120001/Source/core/css/parser/...
File Source/core/css/parser/SizesAttributeParser.h (right):

https://codereview.chromium.org/369423002/diff/120001/Source/core/css/parser/...
Source/core/css/parser/SizesAttributeParser.h:18: static unsigned
findEffectiveSize(const String& attribute, PassRefPtr<MediaValues>, bool&
viewportDependant);
Instead of this being static and trying to return through out params you should
have callers make a SizesAttributeParser on the stack and use the getter. You
should get rid of this static method entirely.

https://codereview.chromium.org/369423002/diff/120001/Source/core/css/parser/...
Source/core/css/parser/SizesAttributeParser.h:35: bool viewportDependant() const
{ return m_viewportDependant; }
This should be public, you should have callers put this on the stack.

https://codereview.chromium.org/369423002/diff/120001/Source/core/css/parser/...
File Source/core/css/parser/SizesCalcParser.cpp (right):

https://codereview.chromium.org/369423002/diff/120001/Source/core/css/parser/...
Source/core/css/parser/SizesCalcParser.cpp:18: viewportDependant =
parser.viewportDependant();
ditto, make a real API.

https://codereview.chromium.org/369423002/diff/120001/Source/core/css/parser/...
Source/core/css/parser/SizesCalcParser.cpp:97: if
(CSSPrimitiveValue::isViewportPercentageLength(it->unitType()))
Does this happen repeatedly? Should you be doing m_viewportDependant =
m_viewportDependant ||
CSSPrimitiveValue::isViewportPercentageLength(it->unitType()) instead?

It seems you're iterating here and doing duplicate work.

https://codereview.chromium.org/369423002/diff/120001/Source/core/css/parser/...
File Source/core/css/parser/SizesCalcParser.h (right):

https://codereview.chromium.org/369423002/diff/120001/Source/core/css/parser/...
Source/core/css/parser/SizesCalcParser.h:37: static bool
parse(MediaQueryTokenIterator start, MediaQueryTokenIterator end,
PassRefPtr<MediaValues>, unsigned& result, bool& viewportDependant);
Same thing, this is way too many out params now, you should make one of these on
the stack and use an API. Get rid of these static methods.

https://codereview.chromium.org/369423002/diff/120001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/369423002/diff/120001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:3397: m_viewportChangeNotifier->viewportChanged();
This is synchronous, which is specifically what I didn't want and why Christian
made the listeners fire at raf time.

https://codereview.chromium.org/369423002/diff/120001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/369423002/diff/120001/Source/core/dom/Documen...
Source/core/dom/Document.h:1037: ViewportChangeNotifier&
viewportChangeNotifier() { return *m_viewportChangeNotifier; }
I don't think we want more APIs here.

https://codereview.chromium.org/369423002/diff/120001/Source/core/frame/Viewp...
File Source/core/frame/ViewportChangeNotifier.h (right):

https://codereview.chromium.org/369423002/diff/120001/Source/core/frame/Viewp...
Source/core/frame/ViewportChangeNotifier.h:12: class ViewportChangeListener :
public RefCountedWillBeGarbageCollected<ViewportChangeListener> {
This is just a duplicate of what the mediaQuery thing already does.

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

https://codereview.chromium.org/369423002/diff/120001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.cpp:87:
document().viewportChangeNotifier().removeListener(m_viewportListener.get());
Or slower...

https://codereview.chromium.org/369423002/diff/120001/Source/core/html/HTMLIm...
File Source/core/html/HTMLImageElement.h (right):

https://codereview.chromium.org/369423002/diff/120001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.h:105: class ImageViewportChangeListener FINAL
: public ViewportChangeListener {
Why is the mediaQuery thing not handling this directly?

https://codereview.chromium.org/369423002/diff/120001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.h:179:
RefPtrWillBeMember<ImageViewportChangeListener> m_viewportListener;
I don't think we want to make every <img> bigger like this.

https://codereview.chromium.org/369423002/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderImage.h (right):

https://codereview.chromium.org/369423002/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderImage.h:70:
imageChanged(m_imageResource->imagePtr());
This shouldn't be inline.

[Yoav Weiss, 2014-07-14 14:51:37]

On 2014/07/14 08:39:50, esprehn wrote:
> This is not correct, you shouldn't be adding more viewport size machinery,
just
> use the stuff Christian already built. You added synchronous callbacks... :(
> 
>
https://codereview.chromium.org/369423002/diff/120001/Source/core/css/parser/...
> File Source/core/css/parser/SizesAttributeParser.cpp (right):
> 
>
https://codereview.chromium.org/369423002/diff/120001/Source/core/css/parser/...
> Source/core/css/parser/SizesAttributeParser.cpp:26: viewportDependant =
> parser.viewportDependant();
> You should hide this logic inside an instance method.
> 
> SizesAttributeParser parser(attribute, mediaValues);
> parser.parse();
> if (parser.viewportDependant()) {
>   ...
> }
> 
>
https://codereview.chromium.org/369423002/diff/120001/Source/core/css/parser/...
> File Source/core/css/parser/SizesAttributeParser.h (right):
> 
>
https://codereview.chromium.org/369423002/diff/120001/Source/core/css/parser/...
> Source/core/css/parser/SizesAttributeParser.h:18: static unsigned
> findEffectiveSize(const String& attribute, PassRefPtr<MediaValues>, bool&
> viewportDependant);
> Instead of this being static and trying to return through out params you
should
> have callers make a SizesAttributeParser on the stack and use the getter. You
> should get rid of this static method entirely.
> 
>
https://codereview.chromium.org/369423002/diff/120001/Source/core/css/parser/...
> Source/core/css/parser/SizesAttributeParser.h:35: bool viewportDependant()
const
> { return m_viewportDependant; }
> This should be public, you should have callers put this on the stack.
> 
>
https://codereview.chromium.org/369423002/diff/120001/Source/core/css/parser/...
> File Source/core/css/parser/SizesCalcParser.cpp (right):
> 
>
https://codereview.chromium.org/369423002/diff/120001/Source/core/css/parser/...
> Source/core/css/parser/SizesCalcParser.cpp:18: viewportDependant =
> parser.viewportDependant();
> ditto, make a real API.
> 
>
https://codereview.chromium.org/369423002/diff/120001/Source/core/css/parser/...
> Source/core/css/parser/SizesCalcParser.cpp:97: if
> (CSSPrimitiveValue::isViewportPercentageLength(it->unitType()))
> Does this happen repeatedly? Should you be doing m_viewportDependant =
> m_viewportDependant ||
> CSSPrimitiveValue::isViewportPercentageLength(it->unitType()) instead?
> 
> It seems you're iterating here and doing duplicate work.
> 
>
https://codereview.chromium.org/369423002/diff/120001/Source/core/css/parser/...
> File Source/core/css/parser/SizesCalcParser.h (right):
> 
>
https://codereview.chromium.org/369423002/diff/120001/Source/core/css/parser/...
> Source/core/css/parser/SizesCalcParser.h:37: static bool
> parse(MediaQueryTokenIterator start, MediaQueryTokenIterator end,
> PassRefPtr<MediaValues>, unsigned& result, bool& viewportDependant);
> Same thing, this is way too many out params now, you should make one of these
on
> the stack and use an API. Get rid of these static methods.
> 
>
https://codereview.chromium.org/369423002/diff/120001/Source/core/dom/Documen...
> File Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/369423002/diff/120001/Source/core/dom/Documen...
> Source/core/dom/Document.cpp:3397:
m_viewportChangeNotifier->viewportChanged();
> This is synchronous, which is specifically what I didn't want and why
Christian
> made the listeners fire at raf time.
> 
>
https://codereview.chromium.org/369423002/diff/120001/Source/core/dom/Document.h
> File Source/core/dom/Document.h (right):
> 
>
https://codereview.chromium.org/369423002/diff/120001/Source/core/dom/Documen...
> Source/core/dom/Document.h:1037: ViewportChangeNotifier&
> viewportChangeNotifier() { return *m_viewportChangeNotifier; }
> I don't think we want more APIs here.
> 
>
https://codereview.chromium.org/369423002/diff/120001/Source/core/frame/Viewp...
> File Source/core/frame/ViewportChangeNotifier.h (right):
> 
>
https://codereview.chromium.org/369423002/diff/120001/Source/core/frame/Viewp...
> Source/core/frame/ViewportChangeNotifier.h:12: class ViewportChangeListener :
> public RefCountedWillBeGarbageCollected<ViewportChangeListener> {
> This is just a duplicate of what the mediaQuery thing already does.
> 
>
https://codereview.chromium.org/369423002/diff/120001/Source/core/html/HTMLIm...
> File Source/core/html/HTMLImageElement.cpp (right):
> 
>
https://codereview.chromium.org/369423002/diff/120001/Source/core/html/HTMLIm...
> Source/core/html/HTMLImageElement.cpp:87:
> document().viewportChangeNotifier().removeListener(m_viewportListener.get());
> Or slower...
> 
>
https://codereview.chromium.org/369423002/diff/120001/Source/core/html/HTMLIm...
> File Source/core/html/HTMLImageElement.h (right):
> 
>
https://codereview.chromium.org/369423002/diff/120001/Source/core/html/HTMLIm...
> Source/core/html/HTMLImageElement.h:105: class ImageViewportChangeListener
FINAL
> : public ViewportChangeListener {
> Why is the mediaQuery thing not handling this directly?
> 
>
https://codereview.chromium.org/369423002/diff/120001/Source/core/html/HTMLIm...
> Source/core/html/HTMLImageElement.h:179:
> RefPtrWillBeMember<ImageViewportChangeListener> m_viewportListener;
> I don't think we want to make every <img> bigger like this.
> 
>
https://codereview.chromium.org/369423002/diff/120001/Source/core/rendering/R...
> File Source/core/rendering/RenderImage.h (right):
> 
>
https://codereview.chromium.org/369423002/diff/120001/Source/core/rendering/R...
> Source/core/rendering/RenderImage.h:70:
> imageChanged(m_imageResource->imagePtr());
> This shouldn't be inline.

The reason I went with a separate mechanism was that I didn't have a real MQ to
to define as a listener.
But you're right of course that this reintroduced sync calls, so that was a bad
idea.

So, I think I'll resolve it by introducing a "fake" viewport-based MQ and have
viewport dependent images use that to listen on MQMatcher, and drop the
ViewportChange* stuff.
I'll also address the rest of your review comments.

[sof, 2014-07-14 20:38:09]

Some Oilpan feedback that might be of help.

https://codereview.chromium.org/369423002/diff/120001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/369423002/diff/120001/Source/core/dom/Documen...
Source/core/dom/Document.h:1331: OwnPtrWillBeMember<ViewportChangeNotifier>
m_viewportChangeNotifier;
If you add a Member to a class, you also need to trace it in the class' trace().

https://codereview.chromium.org/369423002/diff/120001/Source/core/frame/Viewp...
File Source/core/frame/ViewportChangeNotifier.h (right):

https://codereview.chromium.org/369423002/diff/120001/Source/core/frame/Viewp...
Source/core/frame/ViewportChangeNotifier.h:14: virtual void viewportChanged() =
0;
All classes that directly derive from a garbage collected class should define a
trace() method, so add

  virtual void trace(Visitor*) { }

https://codereview.chromium.org/369423002/diff/120001/Source/core/frame/Viewp...
Source/core/frame/ViewportChangeNotifier.h:28: ViewportChangeNotifier() { };
nit: redundant semicolon.

https://codereview.chromium.org/369423002/diff/120001/Source/core/frame/Viewp...
Source/core/frame/ViewportChangeNotifier.h:29: void trace(Visitor*);
Make this a public method.

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

https://codereview.chromium.org/369423002/diff/120001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.cpp:95: visitor->trace(m_imageLoader);
You need to trace the added field,

  visitor->trace(m_viewportListener);

https://codereview.chromium.org/369423002/diff/120001/Source/core/html/HTMLIm...
File Source/core/html/HTMLImageElement.h (right):

https://codereview.chromium.org/369423002/diff/120001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.h:126: void trace(Visitor* visitor)
This is an OVERRIDE (of ViewportChangeListener::trace()). Move the definition of
it into the .cpp file and manually delegate the trace() --

void trace(Visitor* visitor)
{
    visitor->trace(m_element);
    ViewportChangeListener::trace(visitor);
}

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

https://codereview.chromium.org/369423002/diff/120001/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:75: static PassOwnPtrWillBeRawPtr<Task>
create(ImageLoader* loader, UpdateFromElementBehavior updateBehavior)
blink::WebThread::Task is not on the heap, so you can leave the return type as
PassOwnPtr<>.

[Yoav Weiss, 2014-07-15 09:46:31]

Thanks esprehn and sof for the review comments! :)

I've updated the CL accordingly. Some unaddressed concerns below:

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

https://codereview.chromium.org/369423002/diff/140001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.cpp:111: m_listener->clearElement();
Note that clearElement is only called on images that are viewport dependent.

https://codereview.chromium.org/369423002/diff/140001/Source/core/html/HTMLIm...
File Source/core/html/HTMLImageElement.h (left):

https://codereview.chromium.org/369423002/diff/140001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.h:144: };
If the extra memory on HTMLImageElement is an issue here (considering that
m_listener and m_mediaQueryList) are created only for images that need them, I'd
appreciate some advice on how to reduce that footprint.
Possible optimizations include:
* Instruct the compiler to pack the 4 booleans on HTMLImageElement.
* Create a single raw pointer that would hold both m_listener and
m_mediaQueryList. (Note that they are created only when they are actually
needed, so when the image is viewport dependent)
* Avoid storing m_mediaQueryList and create it each time the source is
recalculated.

[sof, 2014-07-15 10:27:30]

https://codereview.chromium.org/369423002/diff/140001/Source/core/html/HTMLIm...
File Source/core/html/HTMLImageElement.h (right):

https://codereview.chromium.org/369423002/diff/140001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.h:153: RefPtrWillBeMember<Listener>
m_listener;
Whenever a traceable Member is added to a class, you need to trace() it in the
class' trace() method. Both the Members here aren't traced.

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

https://codereview.chromium.org/369423002/diff/140001/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:75: static PassOwnPtrWillBeRawPtr<Task>
create(ImageLoader* loader, UpdateFromElementBehavior updateBehavior)
PassOwnPtr<>

[sof, 2014-07-15 10:57:01]

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

https://codereview.chromium.org/369423002/diff/160001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.h:153: RefPtrWillBeRawPtr<Listener>
m_listener;
I think you meant to keep this as RefPtrWillBeMember<> :)

[Yoav Weiss, 2014-07-15 14:01:45]

On 2014/07/15 10:57:01, sof wrote:
>
https://codereview.chromium.org/369423002/diff/160001/Source/core/html/HTMLIm...
> File Source/core/html/HTMLImageElement.h (right):
> 
>
https://codereview.chromium.org/369423002/diff/160001/Source/core/html/HTMLIm...
> Source/core/html/HTMLImageElement.h:153: RefPtrWillBeRawPtr<Listener>
> m_listener;
> I think you meant to keep this as RefPtrWillBeMember<> :)

Yeah. Fixed

[esprehn, 2014-07-16 22:00:04]

Why can't you create a real MQ? This alwaysNotify thing doesn't make sense, the
image doesn't need to know every single time the viewport changes, why can't it
create the correct MQ for the sizes attr and listen on those?

If we do need this you should add a separate listener list for viewport size
changes and notify in the same step, but separately. I feel like we're hacking
around here though, what are you trying to implement?

[Yoav Weiss, 2014-07-16 22:08:05]

On 2014/07/16 22:00:04, esprehn wrote:
> Why can't you create a real MQ? This alwaysNotify thing doesn't make sense,
the
> image doesn't need to know every single time the viewport changes, why can't
it
> create the correct MQ for the sizes attr and listen on those?

The main reason here is to respond to viewport changes even if they didn't go
beyond a 'sizes' breakpoint.
An image which intrinsic size is relative to the viewport (so e.g. an image
which sizes' length is defined with 'vw' and its resource is defined with a 'w'
descriptor) needs to be notified on each viewport change, so that it can update
its intrinsic size.

> 
> If we do need this you should add a separate listener list for viewport size
> changes and notify in the same step, but separately. I feel like we're hacking
> around here though, what are you trying to implement?

[Yoav Weiss, 2014-07-24 14:05:05]

I've updated the patch to do the following:
* MQMatcher now has another hashSet containing listeners to viewport changes
* These listeners are MQListListeners, which should probably be renamed once
they're no longer Web exposed
* The viewport listeners and the MQLists are queued separately on the same
mechanism of ScriptedAnimationController

Note that the functionality this adds is that viewport dependent images (so
images with a viewport based 'sizes' length *and* a 'w' descriptor) will get
their intrinsic size updated when the viewport changes, regardless of the sizes
MQs, and if those MQs need to be re-evaluated.
This change does not trigger re-download if the 'sizes' is not viewport
dependant. That is something I intend to do in a separate CL.

[haraken, 2014-07-25 01:01:00]

Just a drive-by.

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

https://codereview.chromium.org/369423002/diff/220001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.cpp:63: void clearElement() { m_element =
nullptr; }

You can add #if !ENABLE(OILPAN) to this method.

https://codereview.chromium.org/369423002/diff/220001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.cpp:70: Listener(HTMLImageElement* element) :
m_element(element) { }

Add explicit.

[esprehn, 2014-07-25 23:51:56]

https://codereview.chromium.org/369423002/diff/240001/LayoutTests/fast/dom/HT...
File
LayoutTests/fast/dom/HTMLImageElement/image-srcset-react-to-media-changes-when-image-not-changed.html
(right):

https://codereview.chromium.org/369423002/diff/240001/LayoutTests/fast/dom/HT...
LayoutTests/fast/dom/HTMLImageElement/image-srcset-react-to-media-changes-when-image-not-changed.html:23:
window.requestAnimationFrame(function() {
you don't need the window. prefixes on all these.

https://codereview.chromium.org/369423002/diff/240001/LayoutTests/fast/dom/HT...
LayoutTests/fast/dom/HTMLImageElement/image-srcset-react-to-media-changes-when-image-not-changed.html:25:
window.setTimeout(function() {
Why do you need the set timeout?

https://codereview.chromium.org/369423002/diff/240001/Source/core/css/parser/...
File Source/core/css/parser/SizesCalcParser.cpp (right):

https://codereview.chromium.org/369423002/diff/240001/Source/core/css/parser/...
Source/core/css/parser/SizesCalcParser.cpp:23: unsigned
SizesCalcParser::result()
const

https://codereview.chromium.org/369423002/diff/240001/Source/core/css/parser/...
File Source/core/css/parser/SizesCalcParser.h (right):

https://codereview.chromium.org/369423002/diff/240001/Source/core/css/parser/...
Source/core/css/parser/SizesCalcParser.h:38: bool viewportDependant() const {
return m_viewportDependant; }
new line between constructors and methods.

https://codereview.chromium.org/369423002/diff/240001/Source/core/css/parser/...
Source/core/css/parser/SizesCalcParser.h:39: unsigned result();
const ?

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

https://codereview.chromium.org/369423002/diff/240001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.cpp:57: virtual void call() OVERRIDE
We should really give this a better name I suspect, and then make this pure
virtual so the HTMLImageElement itself can be the listener. (future patches).

https://codereview.chromium.org/369423002/diff/240001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.cpp:628: m_listener = Listener::create(this);
Listener is so generic, can you qualify the name or give it a better name?

https://codereview.chromium.org/369423002/diff/240001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.cpp:629:
document().mediaQueryMatcher().addViewportListener(m_listener.get());
What prevents this from adding the same listener twice?

https://codereview.chromium.org/369423002/diff/240001/Source/core/html/HTMLIm...
File Source/core/html/HTMLImageElement.h (right):

https://codereview.chromium.org/369423002/diff/240001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.h:152: bool m_effectiveSizeViewportDependant;
Please pack these bools with the existing ones, use : 1 for each, make the type
of all of them unsigned.

https://codereview.chromium.org/369423002/diff/240001/Source/core/html/HTMLIm...
Source/core/html/HTMLImageElement.h:153: RefPtrWillBeMember<Listener>
m_listener;
This should be with the other ptr m_imageLoader.

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

https://codereview.chromium.org/369423002/diff/240001/Source/core/html/parser...
Source/core/html/parser/HTMLPreloadScanner.cpp:121: m_sourceSize =
parser.length();
This can still be one line.

SizesAttributeParser(m_mediaValues, String()).length()

https://codereview.chromium.org/369423002/diff/240001/Source/core/html/parser...
Source/core/html/parser/HTMLPreloadScanner.cpp:246: m_sourceSize =
parser.length();
ditto

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

https://codereview.chromium.org/369423002/diff/240001/Source/core/loader/Imag...
Source/core/loader/ImageLoader.cpp:294: if (m_element->renderer() &&
m_element->renderer()->isImage())
combine with the other ifs?

if (updateBehavior == UpdateForce && m_element->renderer() &&
m_element->renderer()->isImage()) {
  ...
}

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

https://codereview.chromium.org/369423002/diff/240001/Source/core/loader/Imag...
Source/core/loader/ImageLoader.h:80: UpdateForce
Can we give this a better name like UpdateSizeChanged ?

[Yoav Weiss, 2014-07-26 05:32:01]

On 2014/07/25 23:51:56, esprehn wrote:
>
https://codereview.chromium.org/369423002/diff/240001/LayoutTests/fast/dom/HT...
> File
>
LayoutTests/fast/dom/HTMLImageElement/image-srcset-react-to-media-changes-when-image-not-changed.html
> (right):
> 
>
https://codereview.chromium.org/369423002/diff/240001/LayoutTests/fast/dom/HT...
>
LayoutTests/fast/dom/HTMLImageElement/image-srcset-react-to-media-changes-when-image-not-changed.html:23:
> window.requestAnimationFrame(function() {
> you don't need the window. prefixes on all these.

removed
> 
>
https://codereview.chromium.org/369423002/diff/240001/LayoutTests/fast/dom/HT...
>
LayoutTests/fast/dom/HTMLImageElement/image-srcset-react-to-media-changes-when-image-not-changed.html:25:
> window.setTimeout(function() {
> Why do you need the set timeout?

Waiting till the end of RAF. The test was flaky without it.

> 
>
https://codereview.chromium.org/369423002/diff/240001/Source/core/css/parser/...
> File Source/core/css/parser/SizesCalcParser.cpp (right):
> 
>
https://codereview.chromium.org/369423002/diff/240001/Source/core/css/parser/...
> Source/core/css/parser/SizesCalcParser.cpp:23: unsigned
> SizesCalcParser::result()
> const

Added
> 
>
https://codereview.chromium.org/369423002/diff/240001/Source/core/css/parser/...
> File Source/core/css/parser/SizesCalcParser.h (right):
> 
>
https://codereview.chromium.org/369423002/diff/240001/Source/core/css/parser/...
> Source/core/css/parser/SizesCalcParser.h:38: bool viewportDependant() const {
> return m_viewportDependant; }
> new line between constructors and methods.

Added
> 
>
https://codereview.chromium.org/369423002/diff/240001/Source/core/css/parser/...
> Source/core/css/parser/SizesCalcParser.h:39: unsigned result();
> const ?

Added
> 
>
https://codereview.chromium.org/369423002/diff/240001/Source/core/html/HTMLIm...
> File Source/core/html/HTMLImageElement.cpp (right):
> 
>
https://codereview.chromium.org/369423002/diff/240001/Source/core/html/HTMLIm...
> Source/core/html/HTMLImageElement.cpp:57: virtual void call() OVERRIDE
> We should really give this a better name I suspect, and then make this pure
> virtual so the HTMLImageElement itself can be the listener. (future patches).

I agree
> 
>
https://codereview.chromium.org/369423002/diff/240001/Source/core/html/HTMLIm...
> Source/core/html/HTMLImageElement.cpp:628: m_listener =
Listener::create(this);
> Listener is so generic, can you qualify the name or give it a better name?

Changed to ViewportChangeListener

> 
>
https://codereview.chromium.org/369423002/diff/240001/Source/core/html/HTMLIm...
> Source/core/html/HTMLImageElement.cpp:629:
> document().mediaQueryMatcher().addViewportListener(m_listener.get());
> What prevents this from adding the same listener twice?

I've added protection against that, even though it's added to a HashSet, so
AFAIU, there's no harm in adding it twice.

> 
>
https://codereview.chromium.org/369423002/diff/240001/Source/core/html/HTMLIm...
> File Source/core/html/HTMLImageElement.h (right):
> 
>
https://codereview.chromium.org/369423002/diff/240001/Source/core/html/HTMLIm...
> Source/core/html/HTMLImageElement.h:152: bool
m_effectiveSizeViewportDependant;
> Please pack these bools with the existing ones, use : 1 for each, make the
type
> of all of them unsigned.

Done
> 
>
https://codereview.chromium.org/369423002/diff/240001/Source/core/html/HTMLIm...
> Source/core/html/HTMLImageElement.h:153: RefPtrWillBeMember<Listener>
> m_listener;
> This should be with the other ptr m_imageLoader.

Moved
> 
>
https://codereview.chromium.org/369423002/diff/240001/Source/core/html/parser...
> File Source/core/html/parser/HTMLPreloadScanner.cpp (right):
> 
>
https://codereview.chromium.org/369423002/diff/240001/Source/core/html/parser...
> Source/core/html/parser/HTMLPreloadScanner.cpp:121: m_sourceSize =
> parser.length();
> This can still be one line.
> 
> SizesAttributeParser(m_mediaValues, String()).length()
> 
>
https://codereview.chromium.org/369423002/diff/240001/Source/core/html/parser...
> Source/core/html/parser/HTMLPreloadScanner.cpp:246: m_sourceSize =
> parser.length();
> ditto
> 
>
https://codereview.chromium.org/369423002/diff/240001/Source/core/loader/Imag...
> File Source/core/loader/ImageLoader.cpp (right):
> 
>
https://codereview.chromium.org/369423002/diff/240001/Source/core/loader/Imag...
> Source/core/loader/ImageLoader.cpp:294: if (m_element->renderer() &&
> m_element->renderer()->isImage())
> combine with the other ifs?

combined
> 
> if (updateBehavior == UpdateForce && m_element->renderer() &&
> m_element->renderer()->isImage()) {
>   ...
> }
> 
>
https://codereview.chromium.org/369423002/diff/240001/Source/core/loader/Imag...
> File Source/core/loader/ImageLoader.h (right):
> 
>
https://codereview.chromium.org/369423002/diff/240001/Source/core/loader/Imag...
> Source/core/loader/ImageLoader.h:80: UpdateForce
> Can we give this a better name like UpdateSizeChanged ?

Changed

[esprehn, 2014-07-27 16:53:06]

This assert doesn't make sense.

https://codereview.chromium.org/369423002/diff/260001/Source/core/dom/Scripte...
File Source/core/dom/ScriptedAnimationController.cpp (right):

https://codereview.chromium.org/369423002/diff/260001/Source/core/dom/Scripte...
Source/core/dom/ScriptedAnimationController.cpp:176:
ASSERT(m_mediaQueryListListeners.size());
Why does there need to be a listener? We unconditionally call this method, it
doesn't make sense to assert this.

[Yoav Weiss, 2014-07-27 20:48:45]

On 2014/07/27 16:53:06, esprehn wrote:
> This assert doesn't make sense.
> 
>
https://codereview.chromium.org/369423002/diff/260001/Source/core/dom/Scripte...
> File Source/core/dom/ScriptedAnimationController.cpp (right):
> 
>
https://codereview.chromium.org/369423002/diff/260001/Source/core/dom/Scripte...
> Source/core/dom/ScriptedAnimationController.cpp:176:
> ASSERT(m_mediaQueryListListeners.size());
> Why does there need to be a listener? We unconditionally call this method, it
> doesn't make sense to assert this.

You're right, it makes no sense. It was a left over from an intermediate change.
Thanks for catching that!

[esprehn, 2014-08-01 14:06:44]

This lgtm, but you have a typo which looks like a bug. In the future if we can
do these patches in smaller parts it'll be easier and faster to review too since
it's quicker to catch bugs like this.

https://codereview.chromium.org/369423002/diff/280001/Source/core/css/parser/...
File Source/core/css/parser/SizesAttributeParser.cpp (right):

https://codereview.chromium.org/369423002/diff/280001/Source/core/css/parser/...
Source/core/css/parser/SizesAttributeParser.cpp:41: m_viewportDependant =
CSSPrimitiveValue::isViewportPercentageLength(startToken->unitType());
Does this need to be m_viewportDependant ||
CSSPrimitiveValue::isViewportPercentageLength(startToken->unitType()); ?

https://codereview.chromium.org/369423002/diff/280001/Source/core/css/parser/...
File Source/core/css/parser/SizesCalcParser.cpp (right):

https://codereview.chromium.org/369423002/diff/280001/Source/core/css/parser/...
Source/core/css/parser/SizesCalcParser.cpp:20: m_isValid = calculate();
m_isValid = calcToReversePolishNotation(start, end) && calculate(); ?

[Yoav Weiss, 2014-08-01 20:59:24]

On 2014/08/01 14:06:44, esprehn wrote:
> This lgtm, but you have a typo which looks like a bug. In the future if we can
> do these patches in smaller parts it'll be easier and faster to review too
since
> it's quicker to catch bugs like this.

Thanks. Will do.
> 
>
https://codereview.chromium.org/369423002/diff/280001/Source/core/css/parser/...
> File Source/core/css/parser/SizesAttributeParser.cpp (right):
> 
>
https://codereview.chromium.org/369423002/diff/280001/Source/core/css/parser/...
> Source/core/css/parser/SizesAttributeParser.cpp:41: m_viewportDependant =
> CSSPrimitiveValue::isViewportPercentageLength(startToken->unitType());
> Does this need to be m_viewportDependant ||
> CSSPrimitiveValue::isViewportPercentageLength(startToken->unitType()); ?

Good catch, but the bug is actually the other way around. m_viewportDependent
shouldn't rely on its past value for neither dimension tokens nor function
tokens.
I've fixed it, and added unit tests that cover that for both token types.

> 
>
https://codereview.chromium.org/369423002/diff/280001/Source/core/css/parser/...
> File Source/core/css/parser/SizesCalcParser.cpp (right):
> 
>
https://codereview.chromium.org/369423002/diff/280001/Source/core/css/parser/...
> Source/core/css/parser/SizesCalcParser.cpp:20: m_isValid = calculate();
> m_isValid = calcToReversePolishNotation(start, end) && calculate(); ?
Changed.

[Yoav Weiss, 2014-08-01 21:34:52]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-08-01 21:35:16]

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

[commit-bot: I haz the power, 2014-08-01 22:21:39]

Change committed as 179422