Added preloader support for sizes based RW values

Added preloader support for sizes based RW values

This CL adds support for RW hints that are based on the element's sizes attribute.
It currently adds this only to the PreloadScanner.
The same would be added later to the regular parser.

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

[Yoav Weiss, 2015-04-09 11:17:19]

yoav@yoav.ws changed reviewers:
+ mkwst@chromium.org

[Yoav Weiss, 2015-04-09 11:17:19]

Hey Mike, can you take a look?

This adds "real" RW values to some image requests triggered by the
preloadScanner. The rest of image requests will follow.

[Mike West, 2015-04-09 11:29:12]

Looks like some CH tests are failing. Mind taking a look before I dive into the
code?

[Mike West, 2015-04-09 11:42:48]

LGTM, assuming you fix the tests (which I suspect are just caused by now
expecting an `rw` GET parameter.

It would be ideal if we could change RW to unsigned rather than int, but if we
can't, please add a comment about the magicness of -1.

https://codereview.chromium.org/1078633003/diff/1/Source/core/loader/FrameFet...
File Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/1078633003/diff/1/Source/core/loader/FrameFet...
Source/core/loader/FrameFetchContext.cpp:671: if (resourceWidth < 0)
Can resourceWidth reasonably be 0? It would be nice if we could make this
unsigned.

[Yoav Weiss, 2015-04-09 11:44:59]

On 2015/04/09 11:42:48, Mike West wrote:
> LGTM, assuming you fix the tests (which I suspect are just caused by now
> expecting an `rw` GET parameter.

Yeah, fixed that.

> 
> It would be ideal if we could change RW to unsigned rather than int, but if we
> can't, please add a comment about the magicness of -1.
> 
>
https://codereview.chromium.org/1078633003/diff/1/Source/core/loader/FrameFet...
> File Source/core/loader/FrameFetchContext.cpp (right):
> 
>
https://codereview.chromium.org/1078633003/diff/1/Source/core/loader/FrameFet...
> Source/core/loader/FrameFetchContext.cpp:671: if (resourceWidth < 0)
> Can resourceWidth reasonably be 0? It would be nice if we could make this
> unsigned.

I can turn it to unsigned, but then we'd also need to communicate an
"isResourceWidthSet" boolean along with it.
If you think it's cleaner, I'll do that

[Yoav Weiss, 2015-04-09 11:45:48]

On 2015/04/09 11:44:59, Yoav Weiss wrote:
> On 2015/04/09 11:42:48, Mike West wrote:
> > LGTM, assuming you fix the tests (which I suspect are just caused by now
> > expecting an `rw` GET parameter.
> 
> Yeah, fixed that.
> 
> > 
> > It would be ideal if we could change RW to unsigned rather than int, but if
we
> > can't, please add a comment about the magicness of -1.
> > 
> >
>
https://codereview.chromium.org/1078633003/diff/1/Source/core/loader/FrameFet...
> > File Source/core/loader/FrameFetchContext.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1078633003/diff/1/Source/core/loader/FrameFet...
> > Source/core/loader/FrameFetchContext.cpp:671: if (resourceWidth < 0)
> > Can resourceWidth reasonably be 0? It would be nice if we could make this
> > unsigned.
> 
> I can turn it to unsigned, but then we'd also need to communicate an
> "isResourceWidthSet" boolean along with it.
> If you think it's cleaner, I'll do that

And to answer the original question, yeah, it can be zero per spec (although it
makes very little sense to set it to zero).

[Mike West, 2015-04-09 11:52:50]

On 2015/04/09 at 11:45:48, yoav wrote:
> > I can turn it to unsigned, but then we'd also need to communicate an
> > "isResourceWidthSet" boolean along with it.
> > If you think it's cleaner, I'll do that
> 
> And to answer the original question, yeah, it can be zero per spec (although
it makes very little sense to set it to zero).

I guess I'm asking whether we can change that behavior if there's no sane reason
to do it. That is, it would be nice if `0` had the effect that `-1` has in this
patch.

You can land it like this (assuming the bots are happy) if you add a note about
`-1`'s magical behavior. I'd appreciate it if you'd follow up in spec-land to
see if the defined behavior can be changed. :)

[Yoav Weiss, 2015-04-09 12:34:36]

On 2015/04/09 11:52:50, Mike West wrote:
> On 2015/04/09 at 11:45:48, yoav wrote:
> > > I can turn it to unsigned, but then we'd also need to communicate an
> > > "isResourceWidthSet" boolean along with it.
> > > If you think it's cleaner, I'll do that
> > 
> > And to answer the original question, yeah, it can be zero per spec (although
> it makes very little sense to set it to zero).
> 
> I guess I'm asking whether we can change that behavior if there's no sane
reason
> to do it. That is, it would be nice if `0` had the effect that `-1` has in
this
> patch.
> 
> You can land it like this (assuming the bots are happy) if you add a note
about
> `-1`'s magical behavior. I'd appreciate it if you'd follow up in spec-land to
> see if the defined behavior can be changed. :)

talking to zcorpan I realized that sourceSize (and hence ResourceWidth) should
be a float. Filed https://codereview.chromium.org/1073783002
I'll change this CL accordingly.

[Yoav Weiss, 2015-04-09 19:01:19]

On 2015/04/09 12:34:36, Yoav Weiss wrote:
> On 2015/04/09 11:52:50, Mike West wrote:
> > On 2015/04/09 at 11:45:48, yoav wrote:
> > > > I can turn it to unsigned, but then we'd also need to communicate an
> > > > "isResourceWidthSet" boolean along with it.
> > > > If you think it's cleaner, I'll do that
> > > 
> > > And to answer the original question, yeah, it can be zero per spec
(although
> > it makes very little sense to set it to zero).
> > 
> > I guess I'm asking whether we can change that behavior if there's no sane
> reason
> > to do it. That is, it would be nice if `0` had the effect that `-1` has in
> this
> > patch.
> > 
> > You can land it like this (assuming the bots are happy) if you add a note
> about
> > `-1`'s magical behavior. I'd appreciate it if you'd follow up in spec-land
to
> > see if the defined behavior can be changed. :)
> 
> talking to zcorpan I realized that sourceSize (and hence ResourceWidth) should
> be a float. Filed https://codereview.chromium.org/1073783002
> I'll change this CL accordingly.

Since resource width is a float, it makes no sense to forbid a 0 value.
I've now changed it to be a struct with an boolean that indicates if it was set
or not.

[Mike West, 2015-04-09 19:02:29]

Sounds good.

[Yoav Weiss, 2015-04-09 21:02:27]

The CQ bit was checked by yoav@yoav.ws

[Yoav Weiss, 2015-04-09 21:02:28]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1078633003/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2015-04-09 21:02:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1078633003/60001

[commit-bot: I haz the power, 2015-04-09 21:07:48]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193479