Make AvoidPlugins respect iframes when calculating dimensions / locations (issue1070001)

[dbat...@google.com]

Reviewers: bmcquade,



Please review this at http://page-speed-codereview.appspot.com/1070001/

Affected files:
M pagespeed/rules/avoid_plugins.cc
M pagespeed/rules/avoid_plugins_test.cc

[bmcq...@google.com]

Looks good. I have a couple small suggestions for handling some clipping
cases but overall this looks right.


http://page-speed-codereview.appspot.com/1070001/diff/1/pagespeed/rules/avoid_plugins.cc
File pagespeed/rules/avoid_plugins.cc (right):

http://page-speed-codereview.appspot.com/1070001/diff/1/pagespeed/rules/avoid_plugins.cc#newcode383
pagespeed/rules/avoid_plugins.cc:383: const int x1 = frame_x1_ + x;
do you want to apply any clipping if x or y end up being negative?
That's slightly different than the clamping you do at the bottom.
Imagine this case
frame_x1_ is 10
x is -9
width is 8

In that case we have something that has positive coordinates in its
parent's document space but which we know will be clipped. So it seems
we should probably look at x and y to determine if they are negative and
process accordingly (clamp to 0 and reduce the width/height
accordingly?)

http://page-speed-codereview.appspot.com/1070001/diff/1/pagespeed/rules/avoid_plugins.cc#newcode390
pagespeed/rules/avoid_plugins.cc:390: if (frame_x2_ > 0 && frame_y2_ >
0) {
Technically you could have a frame that truly has width and height 0, in
which case you *would* want to apply clipping. So why don't we init
these values to -1 and then change these tests to:
if (frame_x2_ > -1 && frame_y2_ > -1) {
...
}

Also I'm not sure if you want to handle the case where frame_x2_ is -1
but frame_y2_ is not or vice versa... perhaps SetFrameBounds should just
blow up if any values are negative so that case is considered invalid.

http://page-speed-codereview.appspot.com/1070001/

[dbat...@google.com]

http://page-speed-codereview.appspot.com/1070001/diff/1/pagespeed/rules/avoid_plugins.cc
File pagespeed/rules/avoid_plugins.cc (right):

http://page-speed-codereview.appspot.com/1070001/diff/1/pagespeed/rules/avoid_plugins.cc#newcode383
pagespeed/rules/avoid_plugins.cc:383: const int x1 = frame_x1_ + x;

On 2013/11/15 17:55:21, bmcquade wrote:
> do you want to apply any clipping if x or y end up being negative?
That's
> slightly different than the clamping you do at the bottom. Imagine
this case
> frame_x1_ is 10
> x is -9
> width is 8

> In that case we have something that has positive coordinates in its
parent's
> document space but which we know will be clipped. So it seems we
should probably
> look at x and y to determine if they are negative and process
accordingly (clamp
> to 0 and reduce the width/height accordingly?)

Made SetFrameBounds set the visibility to false if there's a negative
value.

http://page-speed-codereview.appspot.com/1070001/diff/1/pagespeed/rules/avoid_plugins.cc#newcode390
pagespeed/rules/avoid_plugins.cc:390: if (frame_x2_ > 0 && frame_y2_ >
0) {

On 2013/11/15 17:55:21, bmcquade wrote:
> Technically you could have a frame that truly has width and height 0,
in which
> case you *would* want to apply clipping. So why don't we init these
values to -1
> and then change these tests to:
> if (frame_x2_ > -1 && frame_y2_ > -1) {
> ...
> }

> Also I'm not sure if you want to handle the case where frame_x2_ is -1
but
> frame_y2_ is not or vice versa... perhaps SetFrameBounds should just
blow up if
> any values are negative so that case is considered invalid.

I both changed those values to -1, and additionally made SetFrameBounds
set the visibility to false if the box has 0 width or height.

http://page-speed-codereview.appspot.com/1070001/

[bmcq...@google.com]

LGTM. Just one possible issue (see comment below) but once you've
addressed it please feel free to check in. Thanks!


http://page-speed-codereview.appspot.com/1070001/diff/50001/pagespeed/rules/avoid_plugins.cc
File pagespeed/rules/avoid_plugins.cc (right):

http://page-speed-codereview.appspot.com/1070001/diff/50001/pagespeed/rules/avoid_plugins.cc#newcode292
pagespeed/rules/avoid_plugins.cc:292: if (x1 < 0 || y1 < 0 || x2 < x1 ||
y2 < y1) {
x1 can be less than zero and as long as x2 is >0 the frame will be
partially visible
perhaps
if (x1 >= frame_width || y1 >= frame_height || x2 <= x1 || y2 <= y1)
not totally sure if that is right or if we have access to
frame_width/height.

if not, perhaps just
if (x2 <= x1 || y2 <= y1)
is enough

http://page-speed-codereview.appspot.com/1070001/

[bmcq...@google.com]

http://page-speed-codereview.appspot.com/1070001/diff/50001/pagespeed/rules/avoid_plugins.cc
File pagespeed/rules/avoid_plugins.cc (right):

http://page-speed-codereview.appspot.com/1070001/diff/50001/pagespeed/rules/avoid_plugins.cc#newcode292
pagespeed/rules/avoid_plugins.cc:292: if (x1 < 0 || y1 < 0 || x2 < x1 ||
y2 < y1) {

Great I see your fix. Thanks!

http://page-speed-codereview.appspot.com/1070001/diff/80001/pagespeed/rules/avoid_plugins.cc#newcode317
pagespeed/rules/avoid_plugins.cc:317: if (node.GetX(&x) ==
pagespeed::DomElement::SUCCESS &&
good catch

http://page-speed-codereview.appspot.com/1070001/