Removed FrameView's windowToContents and contentsToWindow methods.

Removed FrameView's windowToContents and contentsToWindow methods.

This CL adds methods to FrameView for converting between the FrameView's
content space and Blink's viewport space and then replaces |window| in
all uses of contentsToWindow and windowToContents with either rootFrame
or Viewport. |Window| is equivalent to |RootFrame| so it has no behavioral
change. Changing |Window| to |Viewport| will change behavior under page
scale.

See http://www.chromium.org/developers/design-documents/blink-coordinate-spaces
for details.

BUG=371902, 455497, 455328
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192299

[bokan, 2015-03-04 23:00:37]

bokan@chromium.org changed reviewers:
+ rbyers@chromium.org

[bokan, 2015-03-04 23:00:54]

Rick, here's part 2, ptal.

[Rick Byers, 2015-03-05 17:56:21]

This seems reasonable, thanks for driving all the improvements here!

It seems our test coverage for behavior under page scale is pretty lacking. 
It's not practical to try to be exhaustive here, but perhaps it's worth adding
tests for the scenarios where you've found a bug that you're fixing (since those
may be most likely to regress without us easily noticing)?

Also I think this is starting to have a broad enough impact that it's worth a
PSA to blink-dev - especially the idea of replacing the concept of "window
co-ordinates" with "root-frame co-ordinates".

https://codereview.chromium.org/967213004/diff/80001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/967213004/diff/80001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:3666: IntRect frameRect =
convertFromContainingWindow(rectInRootFrame);
Are you planning on updating the terms in the Widget co-ordinate conversion APIs
as well?  Seems a little inconsistent to use 'RootFrame' here but 'Window' in
Widget.  I can't help but wonder if it's just simpler to just stick with the
term "window co-ordinate space" but clearly define it as the same space as the
root frame.  I don't have a strong opinion though - just would like things as
consistent and clear as possible.

https://codereview.chromium.org/967213004/diff/80001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.h (left):

https://codereview.chromium.org/967213004/diff/80001/Source/core/frame/FrameV...
Source/core/frame/FrameView.h:482: // of containing window is. (For example on
Mac it is the containing NSWindow.)
Does any of this comment still apply?  Eg. your CL description claims that
Window is equivalent to RootFrame, but this comment suggests that's not
necessarily the case on all platforms (eg. does the platform window ever add
some padding?).

https://codereview.chromium.org/967213004/diff/80001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/967213004/diff/80001/Source/core/frame/FrameV...
Source/core/frame/FrameView.h:475: IntPoint rootFrameToContents(const IntPoint&)
const;
Can you add a brief description of each of the co-ordinate system terms here?
e.g. make it clear that only viewport co-ordinates take pinch-zoom into account.

https://codereview.chromium.org/967213004/diff/80001/Source/core/frame/FrameV...
Source/core/frame/FrameView.h:487: // in viewport space, but sized in CSS
pixels. This is an artefact of the
nit: artifact

https://codereview.chromium.org/967213004/diff/80001/Source/core/frame/PinchV...
File Source/core/frame/PinchViewport.cpp (right):

https://codereview.chromium.org/967213004/diff/80001/Source/core/frame/PinchV...
Source/core/frame/PinchViewport.cpp:605: // FIXME: How to snap to pixels?
The only right answer here is probably to update all callers to expect float
return values, right?  How hard would that be?  If it's not too hard, it would
be nice to avoid introducing new int-based APIs for something that's inherently
float.  At least then you could easily apply different truncation policies at
the call site depending on the scenario (making it clear callers need to
consider how to correctly handle fractional values).

https://codereview.chromium.org/967213004/diff/80001/Source/web/tests/PinchVi...
File Source/web/tests/PinchViewportTest.cpp (right):

https://codereview.chromium.org/967213004/diff/80001/Source/web/tests/PinchVi...
Source/web/tests/PinchViewportTest.cpp:1468: // Tests that the maximum scroll
offset of the viewport can be fractional.
looks like you're not actually testing for fractional values.  That would be
valuable to add, right?

[bokan, 2015-03-06 21:54:37]

https://codereview.chromium.org/967213004/diff/80001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/967213004/diff/80001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:3666: IntRect frameRect =
convertFromContainingWindow(rectInRootFrame);
On 2015/03/05 17:56:20, Rick Byers wrote:
> Are you planning on updating the terms in the Widget co-ordinate conversion
APIs
> as well?  Seems a little inconsistent to use 'RootFrame' here but 'Window' in
> Widget.  I can't help but wonder if it's just simpler to just stick with the
> term "window co-ordinate space" but clearly define it as the same space as the
> root frame.  I don't have a strong opinion though - just would like things as
> consistent and clear as possible.

Yes, I plan to do that in another patch though as this one is already quite
large. I think the goal should be to remove "window" from the vocabulary since
it's ambiguous and a misnomer on some platforms.

https://codereview.chromium.org/967213004/diff/80001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.h (left):

https://codereview.chromium.org/967213004/diff/80001/Source/core/frame/FrameV...
Source/core/frame/FrameView.h:482: // of containing window is. (For example on
Mac it is the containing NSWindow.)
On 2015/03/05 17:56:20, Rick Byers wrote:
> Does any of this comment still apply?  Eg. your CL description claims that
> Window is equivalent to RootFrame, but this comment suggests that's not
> necessarily the case on all platforms (eg. does the platform window ever add
> some padding?).

I don't think so, I think this comment is out of date and there seems to be no
platform specificity here anymore. That said, I came to that conclusion through
grepping code so I'll look more closely at how this code runs on other
platforms.

https://codereview.chromium.org/967213004/diff/80001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/967213004/diff/80001/Source/core/frame/FrameV...
Source/core/frame/FrameView.h:475: IntPoint rootFrameToContents(const IntPoint&)
const;
On 2015/03/05 17:56:20, Rick Byers wrote:
> Can you add a brief description of each of the co-ordinate system terms here?
> e.g. make it clear that only viewport co-ordinates take pinch-zoom into
account.

Done.

https://codereview.chromium.org/967213004/diff/80001/Source/core/frame/FrameV...
Source/core/frame/FrameView.h:487: // in viewport space, but sized in CSS
pixels. This is an artefact of the
On 2015/03/05 17:56:20, Rick Byers wrote:
> nit: artifact

Done. (Though either works:
http://english.stackexchange.com/questions/37903/difference-between-artifact-...)
:)

https://codereview.chromium.org/967213004/diff/80001/Source/core/frame/PinchV...
File Source/core/frame/PinchViewport.cpp (right):

https://codereview.chromium.org/967213004/diff/80001/Source/core/frame/PinchV...
Source/core/frame/PinchViewport.cpp:605: // FIXME: How to snap to pixels?
On 2015/03/05 17:56:20, Rick Byers wrote:
> The only right answer here is probably to update all callers to expect float
> return values, right?  How hard would that be?  If it's not too hard, it would
> be nice to avoid introducing new int-based APIs for something that's
inherently
> float.  At least then you could easily apply different truncation policies at
> the call site depending on the scenario (making it clear callers need to
> consider how to correctly handle fractional values).

I'm of the opposite viewpoint. I think we should centralize all the pixel
snapping to these kinds of methods. From what I've seen, having call sites
decide how to pixel snap usually leads to arbitrary flooring/ceiling with subtle
bugs. I'm not an expert in snapping to discrete pixel values, so maybe this is
impractical, but I think having the decision applied consistently, or at least
based on semantics (changing the vocabulary from floats and ints will force
callers to think about it, when a dev sees float but wants int they know how to
get an int) is valuable. In any case, can we agree to fix this in a follow up?

[bokan, 2015-03-06 21:55:39]

On 2015/03/05 17:56:21, Rick Byers wrote:
> This seems reasonable, thanks for driving all the improvements here!
> 
> It seems our test coverage for behavior under page scale is pretty lacking. 
> It's not practical to try to be exhaustive here, but perhaps it's worth adding
> tests for the scenarios where you've found a bug that you're fixing (since
those
> may be most likely to regress without us easily noticing)?
> 
> Also I think this is starting to have a broad enough impact that it's worth a
> PSA to blink-dev - especially the idea of replacing the concept of "window
> co-ordinates" with "root-frame co-ordinates".

Agreed on both points, will do.

[bokan, 2015-03-06 22:02:59]

Patchset #7 (id:120001) has been deleted

[bokan, 2015-03-06 22:07:20]

https://codereview.chromium.org/967213004/diff/80001/Source/web/tests/PinchVi...
File Source/web/tests/PinchViewportTest.cpp (right):

https://codereview.chromium.org/967213004/diff/80001/Source/web/tests/PinchVi...
Source/web/tests/PinchViewportTest.cpp:1468: // Tests that the maximum scroll
offset of the viewport can be fractional.
On 2015/03/05 17:56:20, Rick Byers wrote:
> looks like you're not actually testing for fractional values.  That would be
> valuable to add, right?

Done.

[Rick Byers, 2015-03-07 15:44:09]

Changes look good, thanks.

https://codereview.chromium.org/967213004/diff/80001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.h (left):

https://codereview.chromium.org/967213004/diff/80001/Source/core/frame/FrameV...
Source/core/frame/FrameView.h:482: // of containing window is. (For example on
Mac it is the containing NSWindow.)
On 2015/03/06 21:54:37, bokan wrote:
> On 2015/03/05 17:56:20, Rick Byers wrote:
> > Does any of this comment still apply?  Eg. your CL description claims that
> > Window is equivalent to RootFrame, but this comment suggests that's not
> > necessarily the case on all platforms (eg. does the platform window ever add
> > some padding?).
> 
> I don't think so, I think this comment is out of date and there seems to be no
> platform specificity here anymore. That said, I came to that conclusion
through
> grepping code so I'll look more closely at how this code runs on other
> platforms.

Nice, that would be an improvement if it's true (as I also suspect it is - I was
surprised to see this comment, I thought window was always root frame).  If this
comment dates back to WebKit then "platform" obviously had a much larger
meaning.

https://codereview.chromium.org/967213004/diff/80001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/967213004/diff/80001/Source/core/frame/FrameV...
Source/core/frame/FrameView.h:487: // in viewport space, but sized in CSS
pixels. This is an artefact of the
On 2015/03/06 21:54:37, bokan wrote:
> On 2015/03/05 17:56:20, Rick Byers wrote:
> > nit: artifact
> 
> Done. (Though either works:
>
http://english.stackexchange.com/questions/37903/difference-between-artifact-...)
> :)

Oh sorry!  I thought this was just a typo - I'm not one to insist on American
spelling :-)

https://codereview.chromium.org/967213004/diff/80001/Source/core/frame/PinchV...
File Source/core/frame/PinchViewport.cpp (right):

https://codereview.chromium.org/967213004/diff/80001/Source/core/frame/PinchV...
Source/core/frame/PinchViewport.cpp:605: // FIXME: How to snap to pixels?
On 2015/03/06 21:54:37, bokan wrote:
> On 2015/03/05 17:56:20, Rick Byers wrote:
> > The only right answer here is probably to update all callers to expect float
> > return values, right?  How hard would that be?  If it's not too hard, it
would
> > be nice to avoid introducing new int-based APIs for something that's
> inherently
> > float.  At least then you could easily apply different truncation policies
at
> > the call site depending on the scenario (making it clear callers need to
> > consider how to correctly handle fractional values).
> 
> I'm of the opposite viewpoint. I think we should centralize all the pixel
> snapping to these kinds of methods. From what I've seen, having call sites
> decide how to pixel snap usually leads to arbitrary flooring/ceiling with
subtle
> bugs. I'm not an expert in snapping to discrete pixel values, so maybe this is
> impractical, but I think having the decision applied consistently, or at least
> based on semantics (changing the vocabulary from floats and ints will force
> callers to think about it, when a dev sees float but wants int they know how
to
> get an int) is valuable. In any case, can we agree to fix this in a follow up?

Interesting.  You should have a chat with eae@ - he's the expert.  My
understanding, especially for rects, is that there's no one right answer (which
is why our generic APIs often have terms like 'floored', 'enclosing', etc. so
clients can pick the right one for their scenario).  Regardless I think we
should be continuing to move to replacing ints with floats / layoutunits as
appropriate - there are still a lot of examples of using ints for things that
really aren't integers.

But yes, absolutely leaving a FIXME and covering this in a follow-up CL is fine.

https://codereview.chromium.org/967213004/diff/140001/Source/core/frame/Frame...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/967213004/diff/140001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:475: // Coordinate space transform methods.
These comments are great, thanks!  Make sure to link to this in your blink-dev
PSA.

Can you also mention the impact of browser zoom?  Of the top of my head I
thought these technically weren't CSS pixels because true CSS pixels have
browser zoom applied, but I always forget the details and have to look it up
(but no longer!).

Also maybe mention the relationship to the types exposed to javascript.  Eg.
"page" (MouseEvent.pageX), "client" (MouseEvent.clientX, getBoundingClientRect),
"screen" (MouseEvent.screenX).

https://codereview.chromium.org/967213004/diff/140001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:491: // Viewport: The coordinate space of the
viewport as seen by the user, in
nit: "visual viewport" perhaps since you use that term in all your docs /
presentations?

https://codereview.chromium.org/967213004/diff/140001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:503: //         than physical pixels.
I don't think this is odd actually.  High-dpi support is intended to be totally
transparent - only things that really want to know about physical pixels should
get them.

[bokan, 2015-03-09 13:44:33]

https://codereview.chromium.org/967213004/diff/140001/Source/core/frame/Frame...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/967213004/diff/140001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:475: // Coordinate space transform methods.
On 2015/03/07 15:44:08, Rick Byers wrote:

> Can you also mention the impact of browser zoom?  Of the top of my head I
> thought these technically weren't CSS pixels because true CSS pixels have
> browser zoom applied, but I always forget the details and have to look it up
> (but no longer!).

You're right, I thought it didn't matter since converting between
Frame<->Contents in CSS pixels or post-scale DIPs will keep it in CSS pixels or
DIPs, but it's wrong if you try to go from CSS pixels to Viewport or Screen
using these so I've changed the comments to make this clear.

> 
> Also maybe mention the relationship to the types exposed to javascript.  Eg.
> "page" (MouseEvent.pageX), "client" (MouseEvent.clientX,
getBoundingClientRect),
> "screen" (MouseEvent.screenX). 

Done

https://codereview.chromium.org/967213004/diff/140001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:491: // Viewport: The coordinate space of the
viewport as seen by the user, in
On 2015/03/07 15:44:08, Rick Byers wrote:
> nit: "visual viewport" perhaps since you use that term in all your docs /
> presentations?

Done.

https://codereview.chromium.org/967213004/diff/140001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:503: //         than physical pixels.
On 2015/03/07 15:44:08, Rick Byers wrote:
> I don't think this is odd actually.  High-dpi support is intended to be
totally
> transparent - only things that really want to know about physical pixels
should
> get them.

Yah, you're right, I've removed the "oddly enough".

[Rick Byers, 2015-03-12 17:06:49]

https://codereview.chromium.org/967213004/diff/180001/Source/core/frame/Frame...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/967213004/diff/180001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:475: // Coordinate space transform methods.
Are you OK with having to maintain this in two places (in the code and in your
doc), or do you want to consolidate somehow?

https://codereview.chromium.org/967213004/diff/180001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:477: // to this frame. Note that Frame and
Contents spaces are not in CSS pixels;
nit: say what they are, not what they're not. Maybe even define a new term, eg:
"Frame and contents use pre-zoom CSS pixels - they're like CSS px units except
before browser zoom is applied".

https://codereview.chromium.org/967213004/diff/180001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:481: // Contents: The coordinate space of *this*
FrameView's document, in scaled
"scaled DIPs" is an ambiguous term.  Either re-use a term you defined above (eg.
"pre-zoom CSS pixels") or say exactly what you mean here.

https://codereview.chromium.org/967213004/diff/180001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:486: //           MouseEvent.pageX).
You need to take browser zoom into account somehow in this last sentence.

https://codereview.chromium.org/967213004/diff/180001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:492: //        coordinates" (e.g.
MouseEvent.clientX).
ditto two comments from above

[bokan, 2015-03-14 01:12:57]

I'm working on adding tests for what I can but, if you're ok with it, I'd like
to land those in a separate CL since they may take some time and this is
difficult to keep rebased. They'll also increase the likelihood of breakage and
this CL might be messy to revert.

https://codereview.chromium.org/967213004/diff/180001/Source/core/frame/Frame...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/967213004/diff/180001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:475: // Coordinate space transform methods.
On 2015/03/12 17:06:48, Rick Byers wrote:
> Are you OK with having to maintain this in two places (in the code and in your
> doc), or do you want to consolidate somehow?

Nope, I've added a link to
http://www.chromium.org/developers/design-documents/blink-coordinate-spaces in
the comments.

https://codereview.chromium.org/967213004/diff/180001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:477: // to this frame. Note that Frame and
Contents spaces are not in CSS pixels;
On 2015/03/12 17:06:48, Rick Byers wrote:
> nit: say what they are, not what they're not. Maybe even define a new term,
eg:
> "Frame and contents use pre-zoom CSS pixels - they're like CSS px units except
> before browser zoom is applied".

I added "unzoomed CSS pixel" and described it, see the doc.

https://codereview.chromium.org/967213004/diff/180001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:481: // Contents: The coordinate space of *this*
FrameView's document, in scaled
On 2015/03/12 17:06:48, Rick Byers wrote:
> "scaled DIPs" is an ambiguous term.  Either re-use a term you defined above
(eg.
> "pre-zoom CSS pixels") or say exactly what you mean here.

Done.

https://codereview.chromium.org/967213004/diff/180001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:486: //           MouseEvent.pageX).
On 2015/03/12 17:06:48, Rick Byers wrote:
> You need to take browser zoom into account somehow in this last sentence.

Done.

https://codereview.chromium.org/967213004/diff/180001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:492: //        coordinates" (e.g.
MouseEvent.clientX).
On 2015/03/12 17:06:48, Rick Byers wrote:
> ditto two comments from above

Done.

[Rick Byers, 2015-03-17 14:51:19]

On 2015/03/14 01:12:57, bokan wrote:
> I'm working on adding tests for what I can but, if you're ok with it, I'd like
> to land those in a separate CL since they may take some time and this is
> difficult to keep rebased. They'll also increase the likelihood of breakage
and
> this CL might be messy to revert.

Alright, that's OK with me.

Thanks for the extra work on the definitions - we will probably want to keep
tweaking them (eg. someone may ask how CSS 'zoom' and 'transform: scale' apply),
but they're a great place to start.

LGTM
 
>
https://codereview.chromium.org/967213004/diff/180001/Source/core/frame/Frame...
> File Source/core/frame/FrameView.h (right):
> 
>
https://codereview.chromium.org/967213004/diff/180001/Source/core/frame/Frame...
> Source/core/frame/FrameView.h:475: // Coordinate space transform methods.
> On 2015/03/12 17:06:48, Rick Byers wrote:
> > Are you OK with having to maintain this in two places (in the code and in
your
> > doc), or do you want to consolidate somehow?
> 
> Nope, I've added a link to
> http://www.chromium.org/developers/design-documents/blink-coordinate-spaces in
> the comments.
> 
>
https://codereview.chromium.org/967213004/diff/180001/Source/core/frame/Frame...
> Source/core/frame/FrameView.h:477: // to this frame. Note that Frame and
> Contents spaces are not in CSS pixels;
> On 2015/03/12 17:06:48, Rick Byers wrote:
> > nit: say what they are, not what they're not. Maybe even define a new term,
> eg:
> > "Frame and contents use pre-zoom CSS pixels - they're like CSS px units
except
> > before browser zoom is applied".
> 
> I added "unzoomed CSS pixel" and described it, see the doc.
> 
>
https://codereview.chromium.org/967213004/diff/180001/Source/core/frame/Frame...
> Source/core/frame/FrameView.h:481: // Contents: The coordinate space of *this*
> FrameView's document, in scaled
> On 2015/03/12 17:06:48, Rick Byers wrote:
> > "scaled DIPs" is an ambiguous term.  Either re-use a term you defined above
> (eg.
> > "pre-zoom CSS pixels") or say exactly what you mean here.
> 
> Done.
> 
>
https://codereview.chromium.org/967213004/diff/180001/Source/core/frame/Frame...
> Source/core/frame/FrameView.h:486: //           MouseEvent.pageX).
> On 2015/03/12 17:06:48, Rick Byers wrote:
> > You need to take browser zoom into account somehow in this last sentence.
> 
> Done.
> 
>
https://codereview.chromium.org/967213004/diff/180001/Source/core/frame/Frame...
> Source/core/frame/FrameView.h:492: //        coordinates" (e.g.
> MouseEvent.clientX).
> On 2015/03/12 17:06:48, Rick Byers wrote:
> > ditto two comments from above
> 
> Done.

[bokan, 2015-03-17 14:54:15]

On 2015/03/17 14:51:19, Rick Byers wrote:
> On 2015/03/14 01:12:57, bokan wrote:
> > I'm working on adding tests for what I can but, if you're ok with it, I'd
like
> > to land those in a separate CL since they may take some time and this is
> > difficult to keep rebased. They'll also increase the likelihood of breakage
> and
> > this CL might be messy to revert.
> 
> Alright, that's OK with me.
> 
> Thanks for the extra work on the definitions - we will probably want to keep
> tweaking them (eg. someone may ask how CSS 'zoom' and 'transform: scale'
apply),
> but they're a great place to start.
> 
> LGTM

Someone already did :). I agree we should keep tweaking it (it's now up on the
engineering docs section of chromium.org:
http://dev.chromium.org/developers/design-documents/blink-coordinate-spaces) and
I'll add the more content-centric coordinate spaces once I dig in and figure out
how they actually work myself.

[bokan, 2015-03-17 16:12:38]

bokan@chromium.org changed reviewers:
+ aelias@chromium.org

[bokan, 2015-03-17 16:12:39]

+Alexandre for Source/web

I also went ahead and fixed a few of the spots I left FIXMEs  in:
WebViewImpl::saveImageAt/CopyImageAt/performMediaPlayerAction

I also fixed up EventHandler::sendContextMenuEventForKey which was actually in
rootFrame coordinates all along (though the global position was wrong). I've
tested all the cases there and it works well under pinch now (I'll add a test to
my test CL shortly).

Speaking of tests: I have a WIP CL up:
https://codereview.chromium.org/1007503003

I have tests for about half the fixes so far

[bokan, 2015-03-20 17:20:22]

ping

[aelias_OOO_until_Jul13, 2015-03-20 18:54:54]

lgtm.  I'd like to see later followup on FIXMEs in
WebFrameWidgetImpl::selectionBounds and WebViewImpl::applyPluginAction though.

[bokan, 2015-03-20 18:57:00]

On 2015/03/20 18:54:54, aelias wrote:
> lgtm.  I'd like to see later followup on FIXMEs in
> WebFrameWidgetImpl::selectionBounds and WebViewImpl::applyPluginAction though.

Will do, I'm not done with the coordinate cleanups here so I'm not just going to
abandon these. I'll confirm these and fix shortly.

[Rick Byers, 2015-03-20 19:01:00]

Still LGTM with latest changes - land-away

[bokan, 2015-03-20 20:07:33]

The CQ bit was checked by bokan@chromium.org

[bokan, 2015-03-20 20:07:34]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/967213004/#ps280001
(title: "Rebase")

[commit-bot: I haz the power, 2015-03-20 20:08:08]

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

[commit-bot: I haz the power, 2015-03-21 00:31:58]

Committed patchset #14 (id:280001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192299