Implement accessibility support for CSS-transformed iframes.

Implement accessibility support for CSS-transformed iframes.

This patch adds a gfx::Transform to ui::AXNodeData so that any accessibility
node could potentially use a matrix transform to represent its offset relative
to its parent window, rather than a simple translation.

Currently, we only use this field to represent the transform between a frame's
local coordinates and the coordinates of its local frame root. In the future
we may use transformations elsewhere in the accessibility tree, this may
be a lot more efficient than storing frame-relative coordinates for everything.

In addition, this change also handles cross-process iframes by calling a
delegate function implemented by RenderFrameHostImpl to convert a point in
a remote frame's coordinate space to the global coordinate space.

BUG=551601
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/0aec386227a2a2ea357ccea031b8d91dc4a853b1
Cr-Commit-Position: refs/heads/master@{#383534}

[dmazzoni, 2016-02-18 21:06:18]

Description was changed from

==========
Implement accessibility support for CSS-transformed iframes.

This patch adds a gfx::Transform to ui::AXNodeData so that any accessibility
node could potentially use a matrix transform to represent its offset relative
to its parent window, rather than a simple translation.

Currently, we only use this field to represent the transform between a frame's
local coordinates and the coordinates of its local frame root. In the future
we may use transformations elsewhere in the accessibility tree, this may
be a lot more efficient than storing frame-relative coordinates for everything.

In addition, this change also handles cross-process iframes by calling a
delegate function implemented by RenderFrameHostImpl to convert a point in
a remote frame's coordinate space to the global coordinate space.

BUG=551601
==========

to

==========
Implement accessibility support for CSS-transformed iframes.

This patch adds a gfx::Transform to ui::AXNodeData so that any accessibility
node could potentially use a matrix transform to represent its offset relative
to its parent window, rather than a simple translation.

Currently, we only use this field to represent the transform between a frame's
local coordinates and the coordinates of its local frame root. In the future
we may use transformations elsewhere in the accessibility tree, this may
be a lot more efficient than storing frame-relative coordinates for everything.

In addition, this change also handles cross-process iframes by calling a
delegate function implemented by RenderFrameHostImpl to convert a point in
a remote frame's coordinate space to the global coordinate space.

BUG=551601
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[dmazzoni, 2016-02-18 21:12:13]

dmazzoni@chromium.org changed reviewers:
+ kenrb@chromium.org

[dmazzoni, 2016-02-18 21:12:14]

@kenrb: could you look at my use of TransformPointToRootCoordSpace? I'm not sure
why it's not working, it's getting called at the right place, and GetView() is a
RenderWidgetHostViewChildFrame(), but the transformed points are unchanged.

(Same-process frames are working correctly, that's what most of the code in this
change handles.)

[kenrb, 2016-02-18 21:39:18]

On 2016/02/18 21:12:14, dmazzoni wrote:
> @kenrb: could you look at my use of TransformPointToRootCoordSpace? I'm not
sure
> why it's not working, it's getting called at the right place, and GetView() is
a
> RenderWidgetHostViewChildFrame(), but the transformed points are unchanged.
> 
> (Same-process frames are working correctly, that's what most of the code in
this
> change handles.)

As discussed over chat, the test needs to compositor frames from all renderer
processes have been received before hit testing can work.

[dmazzoni, 2016-02-18 23:44:08]

On 2016/02/18 21:39:18, kenrb wrote:
> On 2016/02/18 21:12:14, dmazzoni wrote:
> > @kenrb: could you look at my use of TransformPointToRootCoordSpace? I'm not
> sure
> > why it's not working, it's getting called at the right place, and GetView()
is
> a
> > RenderWidgetHostViewChildFrame(), but the transformed points are unchanged.
> > 
> > (Same-process frames are working correctly, that's what most of the code in
> this
> > change handles.)
> 
> As discussed over chat, the test needs to compositor frames from all renderer
> processes have been received before hit testing can work.

I tried adding some code to wait until every frame had received one compositor
frame, but that didn't make a difference. However, if I add a delay, the
test does pass. Any other ideas?

I think it'd be good to move forward with this either way, I think the
change is good even if the test needs a hack.

[dmazzoni, 2016-02-18 23:46:52]

dmazzoni@chromium.org changed reviewers:
+ aboxhall@chromium.org, dcheng@chromium.org, jochen@chromium.org

[dmazzoni, 2016-02-18 23:46:53]

+aboxhall (primary reviewer)
+dcheng (messages files, WebKit/public)
+jochen (components/)

[kenrb, 2016-02-19 02:12:40]

On 2016/02/18 23:44:08, dmazzoni wrote:
> On 2016/02/18 21:39:18, kenrb wrote:
> > On 2016/02/18 21:12:14, dmazzoni wrote:
> > > @kenrb: could you look at my use of TransformPointToRootCoordSpace? I'm
not
> > sure
> > > why it's not working, it's getting called at the right place, and
GetView()
> is
> > a
> > > RenderWidgetHostViewChildFrame(), but the transformed points are
unchanged.
> > > 
> > > (Same-process frames are working correctly, that's what most of the code
in
> > this
> > > change handles.)
> > 
> > As discussed over chat, the test needs to compositor frames from all
renderer
> > processes have been received before hit testing can work.
> 
> I tried adding some code to wait until every frame had received one compositor
> frame, but that didn't make a difference. However, if I add a delay, the
> test does pass. Any other ideas?
> 
> I think it'd be good to move forward with this either way, I think the
> change is good even if the test needs a hack.

Okay, I think some of the other tests that rely on browser hit testing might
have this problem also, and it strongly suggests that we need a testing utility
added to handle this.

I forgot about this part earlier. Here is the problem:
- The parent frame renders first, and sends a compositor frame to the browser
process. This contains a blank rect for the out-of-process iframe.
- The child frame renders in the second process, and sends a compositor frame to
the browser process. The browser compositor creates a Surface for it.
- However, it isn't finished yet, because that Surface has nowhere to be
displayed. When the Surface is created, the SurfaceID is sent to the parent
frame renderer process.
- The parent frame then paints again, this time with a delegated layer
containing the child frame's SurfaceID. It then sends a new compositor frame to
the browser, at which point everything should display correctly.

The issue with the browser hit testing system is that it relies on all of the
above things to happen before it will work. This is fine in the browser because
users don't need to interact with elements that haven't been drawn yet, but in
tests it creates complicated races. It's not enough to wait until you have a
compositor frame received by all RenderWidgetHostViews, you need the top-level
frame to have received one *after* it got the SurfaceID from the child renderer.

I agree that your CL shouldn't be held up on this. Maybe mark a TODO, and I or
possibly lfg@ will try to fix it in the near future.

[aboxhall, 2016-02-24 18:46:07]

https://codereview.chromium.org/1713723002/diff/80001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility.cc (right):

https://codereview.chromium.org/1713723002/diff/80001/content/browser/accessi...
content/browser/accessibility/browser_accessibility.cc:885:
manager()->delegate()) {
When would this be false?

https://codereview.chromium.org/1713723002/diff/80001/content/test/data/acces...
File content/test/data/accessibility/html/frame/box.html (right):

https://codereview.chromium.org/1713723002/diff/80001/content/test/data/acces...
content/test/data/accessibility/html/frame/box.html:4: <div role="img"
style="width: 200px; height: 200px; background-color: #fef;"></div>
Can we somehow test that the scroll offsets are transformed correctly as well?

[dmazzoni, 2016-02-24 18:53:26]

https://codereview.chromium.org/1713723002/diff/80001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility.cc (right):

https://codereview.chromium.org/1713723002/diff/80001/content/browser/accessi...
content/browser/accessibility/browser_accessibility.cc:885:
manager()->delegate()) {
On 2016/02/24 18:46:07, aboxhall wrote:
> When would this be false?

Only in unit tests

https://codereview.chromium.org/1713723002/diff/80001/content/test/data/acces...
File content/test/data/accessibility/html/frame/box.html (right):

https://codereview.chromium.org/1713723002/diff/80001/content/test/data/acces...
content/test/data/accessibility/html/frame/box.html:4: <div role="img"
style="width: 200px; height: 200px; background-color: #fef;"></div>
On 2016/02/24 18:46:07, aboxhall wrote:
> Can we somehow test that the scroll offsets are transformed correctly as well?

Good idea. There's already a different test for a scrolled iframe, I can add one
for a scrolled iframe with css transformation too.

https://codereview.chromium.org/1713723002/diff/80001/ui/accessibility/ax_nod...
File ui/accessibility/ax_node_data.h (right):

https://codereview.chromium.org/1713723002/diff/80001/ui/accessibility/ax_nod...
ui/accessibility/ax_node_data.h:116: gfx::Transform transform;
Any thoughts on this? It's 16 floats, so 64 bytes of data per node, which is
annoying, and it also adds a dependency on skia to anyone who includes this
header file.

Making it scoped_ptr<gfx::Transform> would solve the space and header file
problem, but we'd have to write a copy constructor and assignment operator,
since scoped_ptr can't be copied.

It's too bad we don't have an optional<T> that's like a scoped_ptr but copyable
by default.

[danakj, 2016-02-24 19:31:44]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2016-02-24 19:31:45]

https://codereview.chromium.org/1713723002/diff/80001/ui/accessibility/ax_nod...
File ui/accessibility/ax_node_data.h (right):

https://codereview.chromium.org/1713723002/diff/80001/ui/accessibility/ax_nod...
ui/accessibility/ax_node_data.h:116: gfx::Transform transform;
On 2016/02/24 18:53:26, dmazzoni wrote:
> Any thoughts on this? It's 16 floats, so 64 bytes of data per node, which is
> annoying, and it also adds a dependency on skia to anyone who includes this
> header file.
> 
> Making it scoped_ptr<gfx::Transform> would solve the space and header file
> problem, but we'd have to write a copy constructor and assignment operator,
> since scoped_ptr can't be copied.
> 
> It's too bad we don't have an optional<T> that's like a scoped_ptr but
copyable
> by default.

https://codereview.chromium.org/1245163002/ will add an Optional<T> class. But
Optional<T> holds a T, not a T*, so if gfx::Transform is too big,
Optional<gfx::Transform> will be too.

Blink has the "rare data" pattern for when there are 1000s of X but only 10s of
X::data_, where it puts that data into another object with a pointer to it on X.
Maybe that's what you want here, I'm not sure how common the AXNodeData class
is, nor how often you'll use the |transform| on it.

If you do need a rare data, a scoped_ptr is probably reasonable.

This class does seem really heavy to be copying. Why does it need to be
copyable? Is there some hopeful chance that with c++11 it can be moved instead
of copied and the need for copies can go away?

Last, adding a user-defined copy constructor will delete the default move
constructor. Because this class has STL containers in it, moving it is far far
cheaper than copying it, so I would super-recommend adding a =default move
constructor.

Last last, having a user-defined destructor also deletes the default move
constructor, so you'd benefit greatly from adding an =default move constructor
for this class in cases where the copy is from an rvalue (function return for
example) for the compiler to do a move instead.

[dmazzoni, 2016-02-24 19:37:41]

On 2016/02/24 19:31:45, danakj wrote:
> > It's too bad we don't have an optional<T> that's like a scoped_ptr but
> copyable
> > by default.
> 
> https://codereview.chromium.org/1245163002/ will add an Optional<T> class. But
> Optional<T> holds a T, not a T*, so if gfx::Transform is too big,
> Optional<gfx::Transform> will be too.

Got it, so it doesn't doesn't actually help with large data. I didn't realize
that.

> Blink has the "rare data" pattern for when there are 1000s of X but only 10s
of
> X::data_, where it puts that data into another object with a pointer to it on
X.
> Maybe that's what you want here, I'm not sure how common the AXNodeData class
> is, nor how often you'll use the |transform| on it.
> 
> If you do need a rare data, a scoped_ptr is probably reasonable.
> 
> This class does seem really heavy to be copying. Why does it need to be
> copyable? Is there some hopeful chance that with c++11 it can be moved instead
> of copied and the need for copies can go away?
> 
> Last, adding a user-defined copy constructor will delete the default move
> constructor. Because this class has STL containers in it, moving it is far far
> cheaper than copying it, so I would super-recommend adding a =default move
> constructor.
> 
> Last last, having a user-defined destructor also deletes the default move
> constructor, so you'd benefit greatly from adding an =default move constructor
> for this class in cases where the copy is from an rvalue (function return for
> example) for the compiler to do a move instead.

Thanks, this is great advice. Up until now this was copyable mainly to be
foolproof and because while it can be wasteful to copy it, it wasn't a
bottleneck.

I think that using a scoped_ptr for rare data and allowing it to be moved
or swapped but never copied would be a great solution. I'll look into that.

[aboxhall, 2016-02-24 19:38:21]

Leaving the memory discussion in Dana's much more capable hands - just one more
question as I get my head around this.

https://codereview.chromium.org/1713723002/diff/80001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility.cc (right):

https://codereview.chromium.org/1713723002/diff/80001/content/browser/accessi...
content/browser/accessibility/browser_accessibility.cc:881: // If this element
has a transform, apply it and we're done.
Why is this true in the case where, e.g., the parent also has a transform?

Also, this comment doesn't seem to match up to the block that immediately
follows it.

[danakj, 2016-02-24 19:48:10]

On 2016/02/24 19:37:41, dmazzoni wrote:
> On 2016/02/24 19:31:45, danakj wrote:
> > > It's too bad we don't have an optional<T> that's like a scoped_ptr but
> > copyable
> > > by default.
> > 
> > https://codereview.chromium.org/1245163002/ will add an Optional<T> class.
But
> > Optional<T> holds a T, not a T*, so if gfx::Transform is too big,
> > Optional<gfx::Transform> will be too.
> 
> Got it, so it doesn't doesn't actually help with large data. I didn't realize
> that.
> 
> > Blink has the "rare data" pattern for when there are 1000s of X but only 10s
> of
> > X::data_, where it puts that data into another object with a pointer to it
on
> X.
> > Maybe that's what you want here, I'm not sure how common the AXNodeData
class
> > is, nor how often you'll use the |transform| on it.
> > 
> > If you do need a rare data, a scoped_ptr is probably reasonable.
> > 
> > This class does seem really heavy to be copying. Why does it need to be
> > copyable? Is there some hopeful chance that with c++11 it can be moved
instead
> > of copied and the need for copies can go away?
> > 
> > Last, adding a user-defined copy constructor will delete the default move
> > constructor. Because this class has STL containers in it, moving it is far
far
> > cheaper than copying it, so I would super-recommend adding a =default move
> > constructor.
> > 
> > Last last, having a user-defined destructor also deletes the default move
> > constructor, so you'd benefit greatly from adding an =default move
constructor
> > for this class in cases where the copy is from an rvalue (function return
for
> > example) for the compiler to do a move instead.
> 
> Thanks, this is great advice. Up until now this was copyable mainly to be
> foolproof and because while it can be wasteful to copy it, it wasn't a
> bottleneck.
> 
> I think that using a scoped_ptr for rare data and allowing it to be moved
> or swapped but never copied would be a great solution. I'll look into that.

One more thought. As this class is quite large, writing a copy constructor
directly on it would also be rather problematic. If I had to add copyable rare
data rather than directly using a scoped_ptr, I'd maybe do something like this:

struct AXNode {
  ...
  AXNodeRareData rare_data_;
};

struct AXNodeRareData {
  AXNodeRareData(const AXNodeRareData& o) { ... copy ... }
  AXNodeRareData(AXNodeRareData&&) = default;

  scoped_ptr<gfx::Transform> transform;
};

That way your larger AXNode class doesn't need a giant hand-written copy
constructor, just your tiny rare data struct does.

[kenrb, 2016-02-29 14:54:46]

FYI: I recently added a helper class to content_browser_test_utils_internal.h to
avoid the need for timeouts when waiting for all the cross-process surface
compositing to resolve before relying on browser process hit testing in tests.
Unfortunately I don't know if there is an easy way to use that with RunHTMLTest.

Usage example:
https://codereview.chromium.org/1734403003/diff/60001/content/browser/site_pe...

[dmazzoni, 2016-02-29 23:19:59]

On 2016/02/29 14:54:46, kenrb wrote:
> FYI: I recently added a helper class to content_browser_test_utils_internal.h
to
> avoid the need for timeouts when waiting for all the cross-process surface
> compositing to resolve before relying on browser process hit testing in tests.
> Unfortunately I don't know if there is an easy way to use that with
RunHTMLTest.
> 
> Usage example:
>
https://codereview.chromium.org/1734403003/diff/60001/content/browser/site_pe...

Thanks, that worked great! No more timeout needed in the test. PTAL.

[dmazzoni, 2016-02-29 23:20:07]

https://codereview.chromium.org/1713723002/diff/80001/content/test/data/acces...
File content/test/data/accessibility/html/frame/box.html (right):

https://codereview.chromium.org/1713723002/diff/80001/content/test/data/acces...
content/test/data/accessibility/html/frame/box.html:4: <div role="img"
style="width: 200px; height: 200px; background-color: #fef;"></div>
On 2016/02/24 18:53:26, dmazzoni wrote:
> On 2016/02/24 18:46:07, aboxhall wrote:
> > Can we somehow test that the scroll offsets are transformed correctly as
well?
> 
> Good idea. There's already a different test for a scrolled iframe, I can add
one
> for a scrolled iframe with css transformation too.

This is done now, take a look.

[dmazzoni, 2016-03-01 23:28:11]

dmazzoni@chromium.org changed reviewers:
+ chrishtr@chromium.org
- danakj@chromium.org

[dmazzoni, 2016-03-01 23:28:12]

+chrishtr for core/layout/

[dmazzoni, 2016-03-04 07:04:30]

Friendly ping

[jochen (gone - plz use gerrit), 2016-03-04 12:57:46]

you added me for components/ but there are no files touched in components/
anymore :)

[dmazzoni, 2016-03-04 20:22:40]

Description was changed from

==========
Implement accessibility support for CSS-transformed iframes.

This patch adds a gfx::Transform to ui::AXNodeData so that any accessibility
node could potentially use a matrix transform to represent its offset relative
to its parent window, rather than a simple translation.

Currently, we only use this field to represent the transform between a frame's
local coordinates and the coordinates of its local frame root. In the future
we may use transformations elsewhere in the accessibility tree, this may
be a lot more efficient than storing frame-relative coordinates for everything.

In addition, this change also handles cross-process iframes by calling a
delegate function implemented by RenderFrameHostImpl to convert a point in
a remote frame's coordinate space to the global coordinate space.

BUG=551601
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implement accessibility support for CSS-transformed iframes.

This patch adds a gfx::Transform to ui::AXNodeData so that any accessibility
node could potentially use a matrix transform to represent its offset relative
to its parent window, rather than a simple translation.

Currently, we only use this field to represent the transform between a frame's
local coordinates and the coordinates of its local frame root. In the future
we may use transformations elsewhere in the accessibility tree, this may
be a lot more efficient than storing frame-relative coordinates for everything.

In addition, this change also handles cross-process iframes by calling a
delegate function implemented by RenderFrameHostImpl to convert a point in
a remote frame's coordinate space to the global coordinate space.

BUG=551601
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[dmazzoni, 2016-03-04 20:22:40]

dmazzoni@chromium.org changed reviewers:
- jochen@chromium.org

[dmazzoni, 2016-03-04 20:23:02]

On 2016/03/04 12:57:46, jochen wrote:
> you added me for components/ but there are no files touched in components/
> anymore :)

Removed. Sorry!  :)

[dmazzoni, 2016-03-08 00:39:19]

From discussion with dcheng: I need to add a test case for a remote frame that
has a local frame child.

[dcheng, 2016-03-08 00:39:42]

https://codereview.chromium.org/1713723002/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1713723002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:665: *bounds =
gfx::Rect(new_bounds.x(), new_bounds.y(),
Unless there's something that requires us to do this, just return gfx::Rect()
directly: copy elision applies here.

https://codereview.chromium.org/1713723002/diff/180001/ui/accessibility/ax_no...
File ui/accessibility/ax_node_data.cc (right):

https://codereview.chromium.org/1713723002/diff/180001/ui/accessibility/ax_no...
ui/accessibility/ax_node_data.cc:81: if (other.transform.get())
No .get() when boolean testing scoped_ptr's (here and elsewhere)

https://codereview.chromium.org/1713723002/diff/180001/ui/accessibility/ax_no...
ui/accessibility/ax_node_data.cc:82: transform.reset(new
gfx::Transform(*other.transform.get()));
Similarly just use *other.transform.

https://codereview.chromium.org/1713723002/diff/180001/ui/gfx/ipc/gfx_param_t...
File ui/gfx/ipc/gfx_param_traits.cc (right):

https://codereview.chromium.org/1713723002/diff/180001/ui/gfx/ipc/gfx_param_t...
ui/gfx/ipc/gfx_param_traits.cc:375: m->WriteBytes(&column_major_data,
sizeof(SkMScalar) * 16);
I know you're just moving this, but mind adding a comment that we do this in one
Write() call for perf reasons
(https://chromium.googlesource.com/chromium/src/+/cc2141401d3ec9865eda78913a01...

Otherwise, everytime I see this and forget the context, I will wonder why we do
this instead of calling, say WriteParam() on the individual elements.

[dmazzoni, 2016-03-16 21:38:12]

dmazzoni@chromium.org changed reviewers:
- chrishtr@chromium.org

[dmazzoni, 2016-03-16 21:38:14]

Ready for another look.

https://codereview.chromium.org/1713723002/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1713723002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:665: *bounds =
gfx::Rect(new_bounds.x(), new_bounds.y(),
On 2016/03/08 00:39:41, dcheng wrote:
> Unless there's something that requires us to do this, just return gfx::Rect()
> directly: copy elision applies here.

Sorry, I don't follow what you want me to do. I need to convert
gfx::RectF to gfx::Rect.

https://codereview.chromium.org/1713723002/diff/180001/ui/accessibility/ax_no...
File ui/accessibility/ax_node_data.cc (right):

https://codereview.chromium.org/1713723002/diff/180001/ui/accessibility/ax_no...
ui/accessibility/ax_node_data.cc:81: if (other.transform.get())
On 2016/03/08 00:39:42, dcheng wrote:
> No .get() when boolean testing scoped_ptr's (here and elsewhere)

Done.

https://codereview.chromium.org/1713723002/diff/180001/ui/accessibility/ax_no...
ui/accessibility/ax_node_data.cc:82: transform.reset(new
gfx::Transform(*other.transform.get()));
On 2016/03/08 00:39:42, dcheng wrote:
> Similarly just use *other.transform.

Done.

https://codereview.chromium.org/1713723002/diff/180001/ui/gfx/ipc/gfx_param_t...
File ui/gfx/ipc/gfx_param_traits.cc (right):

https://codereview.chromium.org/1713723002/diff/180001/ui/gfx/ipc/gfx_param_t...
ui/gfx/ipc/gfx_param_traits.cc:375: m->WriteBytes(&column_major_data,
sizeof(SkMScalar) * 16);
On 2016/03/08 00:39:42, dcheng wrote:
> I know you're just moving this, but mind adding a comment that we do this in
one
> Write() call for perf reasons
>
(https://chromium.googlesource.com/chromium/src/+/cc2141401d3ec9865eda78913a01...)
> 
> Otherwise, everytime I see this and forget the context, I will wonder why we
do
> this instead of calling, say WriteParam() on the individual elements.

Done.

[dcheng, 2016-03-16 21:48:31]

https://codereview.chromium.org/1713723002/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1713723002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:665: *bounds =
gfx::Rect(new_bounds.x(), new_bounds.y(),
On 2016/03/16 at 21:38:13, dmazzoni wrote:
> On 2016/03/08 00:39:41, dcheng wrote:
> > Unless there's something that requires us to do this, just return
gfx::Rect()
> > directly: copy elision applies here.
> 
> Sorry, I don't follow what you want me to do. I need to convert
> gfx::RectF to gfx::Rect.

Just have the function return a gfx::Rect rather than using an out param.

[dmazzoni, 2016-03-16 21:53:34]

https://codereview.chromium.org/1713723002/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/1713723002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:665: *bounds =
gfx::Rect(new_bounds.x(), new_bounds.y(),
On 2016/03/16 21:48:31, dcheng wrote:
> On 2016/03/16 at 21:38:13, dmazzoni wrote:
> > On 2016/03/08 00:39:41, dcheng wrote:
> > > Unless there's something that requires us to do this, just return
> gfx::Rect()
> > > directly: copy elision applies here.
> > 
> > Sorry, I don't follow what you want me to do. I need to convert
> > gfx::RectF to gfx::Rect.
> 
> Just have the function return a gfx::Rect rather than using an out param.

Done.

[dcheng, 2016-03-17 19:53:19]

https://codereview.chromium.org/1713723002/diff/220001/content/browser/access...
File content/browser/accessibility/accessibility_tree_formatter_blink.cc
(right):

https://codereview.chromium.org/1713723002/diff/220001/content/browser/access...
content/browser/accessibility/accessibility_tree_formatter_blink.cc:58:
node.GetData().transform.get() &&
Nit: no .get()

https://codereview.chromium.org/1713723002/diff/220001/content/test/data/acce...
File content/test/data/accessibility/html/frame/nested-frame.html (right):

https://codereview.chromium.org/1713723002/diff/220001/content/test/data/acce...
content/test/data/accessibility/html/frame/nested-frame.html:4: <iframe
width=250 height=250 src="box.html" style="border: 0; position: absolute; top:
0px; left: 100px; transform: scale(2.0); transform-origin: left top;"></iframe>
Can we make the nested frame cross-process as well? I'm interested in seeing how
the:

a.com
  b.com
     c.com

case works. I haven't managed to convince myself it works, because we always
return immediately after consulting a cross-site parent (if necessary) for the
transforms in BrowserAccessibility::ElementBoundsToLocalBounds.

https://codereview.chromium.org/1713723002/diff/220001/ui/accessibility/ax_no...
File ui/accessibility/ax_node_data.cc (right):

https://codereview.chromium.org/1713723002/diff/220001/ui/accessibility/ax_no...
ui/accessibility/ax_node_data.cc:98: transform.reset(new
gfx::Transform(*other.transform.get()));
Nit: *other.transform should be sufficient

[dcheng, 2016-03-17 19:54:39]

https://codereview.chromium.org/1713723002/diff/220001/content/test/data/acce...
File content/test/data/accessibility/html/frame/nested-frame.html (right):

https://codereview.chromium.org/1713723002/diff/220001/content/test/data/acce...
content/test/data/accessibility/html/frame/nested-frame.html:4: <iframe
width=250 height=250 src="box.html" style="border: 0; position: absolute; top:
0px; left: 100px; transform: scale(2.0); transform-origin: left top;"></iframe>
On 2016/03/17 at 19:53:18, dcheng wrote:
> Can we make the nested frame cross-process as well? I'm interested in seeing
how the:
> 
> a.com
>   b.com
>      c.com
> 
> case works. I haven't managed to convince myself it works, because we always
return immediately after consulting a cross-site parent (if necessary) for the
transforms in BrowserAccessibility::ElementBoundsToLocalBounds.


Reformatting for the sake of email:
a.com
`-b.com
  `-c.com

[dcheng, 2016-03-17 19:55:41]

On 2016/03/17 at 19:54:39, dcheng wrote:
>
https://codereview.chromium.org/1713723002/diff/220001/content/test/data/acce...
> File content/test/data/accessibility/html/frame/nested-frame.html (right):
> 
>
https://codereview.chromium.org/1713723002/diff/220001/content/test/data/acce...
> content/test/data/accessibility/html/frame/nested-frame.html:4: <iframe
width=250 height=250 src="box.html" style="border: 0; position: absolute; top:
0px; left: 100px; transform: scale(2.0); transform-origin: left top;"></iframe>
> On 2016/03/17 at 19:53:18, dcheng wrote:
> > Can we make the nested frame cross-process as well? I'm interested in seeing
how the:
> > 
> > a.com
> >   b.com
> >      c.com
> > 
> > case works. I haven't managed to convince myself it works, because we always
return immediately after consulting a cross-site parent (if necessary) for the
transforms in BrowserAccessibility::ElementBoundsToLocalBounds.
> 
> 
> Reformatting for the sake of email:
> a.com
> `-b.com
>   `-c.com

Argh I give up but you get the idea.

[dmazzoni, 2016-03-18 17:10:59]

Added test for nested cross-process frames too!

https://codereview.chromium.org/1713723002/diff/220001/content/browser/access...
File content/browser/accessibility/accessibility_tree_formatter_blink.cc
(right):

https://codereview.chromium.org/1713723002/diff/220001/content/browser/access...
content/browser/accessibility/accessibility_tree_formatter_blink.cc:58:
node.GetData().transform.get() &&
On 2016/03/17 19:53:18, dcheng wrote:
> Nit: no .get()

Done.

https://codereview.chromium.org/1713723002/diff/220001/content/test/data/acce...
File content/test/data/accessibility/html/frame/nested-frame.html (right):

https://codereview.chromium.org/1713723002/diff/220001/content/test/data/acce...
content/test/data/accessibility/html/frame/nested-frame.html:4: <iframe
width=250 height=250 src="box.html" style="border: 0; position: absolute; top:
0px; left: 100px; transform: scale(2.0); transform-origin: left top;"></iframe>
On 2016/03/17 19:54:39, dcheng wrote:
> On 2016/03/17 at 19:53:18, dcheng wrote:
> > Can we make the nested frame cross-process as well? I'm interested in seeing
> how the:
> > 
> > a.com
> >   b.com
> >      c.com
> > 
> > case works. I haven't managed to convince myself it works, because we always
> return immediately after consulting a cross-site parent (if necessary) for the
> transforms in BrowserAccessibility::ElementBoundsToLocalBounds.
> 
> 
> Reformatting for the sake of email:
> a.com
> `-b.com
>   `-c.com

Yes, but I'll add a separate test for this. The remote -> local test was
actually broken, but remote -> remote already passed.

https://codereview.chromium.org/1713723002/diff/220001/content/test/data/acce...
content/test/data/accessibility/html/frame/nested-frame.html:4: <iframe
width=250 height=250 src="box.html" style="border: 0; position: absolute; top:
0px; left: 100px; transform: scale(2.0); transform-origin: left top;"></iframe>
On 2016/03/17 19:53:18, dcheng wrote:
> Can we make the nested frame cross-process as well? I'm interested in seeing
how
> the:
> 
> a.com
>   b.com
>      c.com
> 
> case works. I haven't managed to convince myself it works, because we always
> return immediately after consulting a cross-site parent (if necessary) for the
> transforms in BrowserAccessibility::ElementBoundsToLocalBounds.

The case of nested local frames was wrong, but fixed now.

Multiple remote frames returns immediately because the transformation done by
RFHI correctly transforms a remote frame's coordinate space all the way back to
the coordinate space of the WebContents, no matter how many nested frames there
are.

Either way, both are now tested.

https://codereview.chromium.org/1713723002/diff/220001/ui/accessibility/ax_no...
File ui/accessibility/ax_node_data.cc (right):

https://codereview.chromium.org/1713723002/diff/220001/ui/accessibility/ax_no...
ui/accessibility/ax_node_data.cc:98: transform.reset(new
gfx::Transform(*other.transform.get()));
On 2016/03/17 19:53:18, dcheng wrote:
> Nit: *other.transform should be sufficient

Done.

[dcheng, 2016-03-23 05:26:00]

Sorry for the delay on the review. Just a few small questions, but overall, the
patch LG.

https://codereview.chromium.org/1713723002/diff/260001/content/browser/access...
File content/browser/accessibility/browser_accessibility_manager.h (right):

https://codereview.chromium.org/1713723002/diff/260001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager.h:85: const
gfx::Rect& bounds) = 0;
Part of me wonders if we really want this interface: it seems kind of like we
could just use TestRenderFrameHost for unit tests, since this code lives in
//content.

(Not really something to tackle in this patch, just wondering aloud)

https://codereview.chromium.org/1713723002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp (right):

https://codereview.chromium.org/1713723002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:224: if
(!m_layoutObject || !documentFrameView() || !documentFrameView()->layoutView())
I'm a little surprised we need the latter two checks: shouldn't having a
LayoutObject imply the existence of the FrameView and LayoutView?

https://codereview.chromium.org/1713723002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:229: if
(!parentFrameView || !parentFrameView->layoutView())
Similar thought about the null check for parentFrameView->layoutView() here.

https://codereview.chromium.org/1713723002/diff/260001/ui/accessibility/ax_no...
File ui/accessibility/ax_node_data.cc (right):

https://codereview.chromium.org/1713723002/diff/260001/ui/accessibility/ax_no...
ui/accessibility/ax_node_data.cc:346: if (transform.get() &&
!transform->IsIdentity())
Nit: no .get()

[dmazzoni, 2016-03-24 04:26:53]

https://codereview.chromium.org/1713723002/diff/260001/content/browser/access...
File content/browser/accessibility/browser_accessibility_manager.h (right):

https://codereview.chromium.org/1713723002/diff/260001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager.h:85: const
gfx::Rect& bounds) = 0;
On 2016/03/23 05:26:00, dcheng wrote:
> Part of me wonders if we really want this interface: it seems kind of like we
> could just use TestRenderFrameHost for unit tests, since this code lives in
> //content.
> 
> (Not really something to tackle in this patch, just wondering aloud)

Yeah, the current design is that BrowserAccessibilityManager doesn't actually
depend on RFHI, it defines this delegate interface and RFHI depends on it.

It does seem like we could reverse the dependency and have this class depend on
RFHI instead. It's really only this way for historical reasons and I can see
pros and cons of both ways.

Maybe that's not exactly what you were asking.

For unit tests it does seem like we could use TestRenderFrameHost instead.

https://codereview.chromium.org/1713723002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp (right):

https://codereview.chromium.org/1713723002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:224: if
(!m_layoutObject || !documentFrameView() || !documentFrameView()->layoutView())
On 2016/03/23 05:26:00, dcheng wrote:
> I'm a little surprised we need the latter two checks: shouldn't having a
> LayoutObject imply the existence of the FrameView and LayoutView?

What about during tear-down? We've been bitten by bugs before when an
accessibility APi gets called during frame tear-down.

If you're sure it's still impossible I can remove these checks

[dcheng, 2016-03-24 04:33:14]

https://codereview.chromium.org/1713723002/diff/260001/content/browser/access...
File content/browser/accessibility/browser_accessibility_manager.h (right):

https://codereview.chromium.org/1713723002/diff/260001/content/browser/access...
content/browser/accessibility/browser_accessibility_manager.h:85: const
gfx::Rect& bounds) = 0;
On 2016/03/24 at 04:26:53, dmazzoni wrote:
> On 2016/03/23 05:26:00, dcheng wrote:
> > Part of me wonders if we really want this interface: it seems kind of like
we
> > could just use TestRenderFrameHost for unit tests, since this code lives in
> > //content.
> > 
> > (Not really something to tackle in this patch, just wondering aloud)
> 
> Yeah, the current design is that BrowserAccessibilityManager doesn't actually
depend on RFHI, it defines this delegate interface and RFHI depends on it.
> 
> It does seem like we could reverse the dependency and have this class depend
on RFHI instead. It's really only this way for historical reasons and I can see
pros and cons of both ways.
> 
> Maybe that's not exactly what you were asking.
> 
> For unit tests it does seem like we could use TestRenderFrameHost instead.

This is basically along the lines of what I was thinking, but it's definitely a
project for another patch (if we do it at all).

https://codereview.chromium.org/1713723002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp (right):

https://codereview.chromium.org/1713723002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:224: if
(!m_layoutObject || !documentFrameView() || !documentFrameView()->layoutView())
On 2016/03/24 at 04:26:53, dmazzoni wrote:
> On 2016/03/23 05:26:00, dcheng wrote:
> > I'm a little surprised we need the latter two checks: shouldn't having a
> > LayoutObject imply the existence of the FrameView and LayoutView?
> 
> What about during tear-down? We've been bitten by bugs before when an
accessibility APi gets called during frame tear-down.
> 
> If you're sure it's still impossible I can remove these checks

I guess I don't know enough about how AX hooks in to say this will be true for
sure.

Are there any preconditions around when AXObject::detach() is invoked? Is there
a guarantee that this will happen synchronously when a node is detached from the
DOM?

[dmazzoni, 2016-03-24 04:50:07]

Feedback addressed.

https://codereview.chromium.org/1713723002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp (right):

https://codereview.chromium.org/1713723002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:224: if
(!m_layoutObject || !documentFrameView() || !documentFrameView()->layoutView())
On 2016/03/24 04:33:14, dcheng wrote:
> On 2016/03/24 at 04:26:53, dmazzoni wrote:
> > On 2016/03/23 05:26:00, dcheng wrote:
> > > I'm a little surprised we need the latter two checks: shouldn't having a
> > > LayoutObject imply the existence of the FrameView and LayoutView?
> > 
> > What about during tear-down? We've been bitten by bugs before when an
> accessibility APi gets called during frame tear-down.
> > 
> > If you're sure it's still impossible I can remove these checks
> 
> I guess I don't know enough about how AX hooks in to say this will be true for
> sure.
> 
> Are there any preconditions around when AXObject::detach() is invoked? Is
there
> a guarantee that this will happen synchronously when a node is detached from
the
> DOM?

Yes, it happens synchronously, in Node::removedFrom.

Come to think of it, the past problems we had probably all involved weird corner
cases (usually found by clusterfuzz) involving multiple frames. Now that we know
that all of the AXObjects belong to the same document and frame, this ought to
be
a lot more safe.

After reading the code for documentFrameView() and layoutView() I'm comfortable
removing those checks.

https://codereview.chromium.org/1713723002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:229: if
(!parentFrameView || !parentFrameView->layoutView())
On 2016/03/23 05:26:00, dcheng wrote:
> Similar thought about the null check for parentFrameView->layoutView() here.

Done.

https://codereview.chromium.org/1713723002/diff/260001/ui/accessibility/ax_no...
File ui/accessibility/ax_node_data.cc (right):

https://codereview.chromium.org/1713723002/diff/260001/ui/accessibility/ax_no...
ui/accessibility/ax_node_data.cc:346: if (transform.get() &&
!transform->IsIdentity())
On 2016/03/23 05:26:00, dcheng wrote:
> Nit: no .get()

Done.

[dmazzoni, 2016-03-24 04:51:08]

dmazzoni@chromium.org changed reviewers:
+ jam@chromium.org

[dmazzoni, 2016-03-24 04:51:09]

+jam for content/browser/renderer_host

[jam, 2016-03-24 15:40:07]

On 2016/03/24 04:51:09, dmazzoni wrote:
> +jam for content/browser/renderer_host

lgtm

[dcheng, 2016-03-24 17:07:52]

lgtm

[dmazzoni, 2016-03-28 17:14:42]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-03-28 17:14:42]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1713723002/#ps320001
(title: "Fix gn build")

[commit-bot: I haz the power, 2016-03-28 17:15:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1713723002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1713723002/320001

[commit-bot: I haz the power, 2016-03-28 17:24:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-28 17:24:46]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dmazzoni, 2016-03-28 17:33:16]

dmazzoni@chromium.org changed reviewers:
+ tsepez@chromium.org

[Tom Sepez, 2016-03-28 17:37:02]

messages LGTM.

[dmazzoni, 2016-03-28 17:37:36]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2016-03-28 17:37:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1713723002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1713723002/320001

[commit-bot: I haz the power, 2016-03-28 19:07:29]

Description was changed from

==========
Implement accessibility support for CSS-transformed iframes.

This patch adds a gfx::Transform to ui::AXNodeData so that any accessibility
node could potentially use a matrix transform to represent its offset relative
to its parent window, rather than a simple translation.

Currently, we only use this field to represent the transform between a frame's
local coordinates and the coordinates of its local frame root. In the future
we may use transformations elsewhere in the accessibility tree, this may
be a lot more efficient than storing frame-relative coordinates for everything.

In addition, this change also handles cross-process iframes by calling a
delegate function implemented by RenderFrameHostImpl to convert a point in
a remote frame's coordinate space to the global coordinate space.

BUG=551601
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implement accessibility support for CSS-transformed iframes.

This patch adds a gfx::Transform to ui::AXNodeData so that any accessibility
node could potentially use a matrix transform to represent its offset relative
to its parent window, rather than a simple translation.

Currently, we only use this field to represent the transform between a frame's
local coordinates and the coordinates of its local frame root. In the future
we may use transformations elsewhere in the accessibility tree, this may
be a lot more efficient than storing frame-relative coordinates for everything.

In addition, this change also handles cross-process iframes by calling a
delegate function implemented by RenderFrameHostImpl to convert a point in
a remote frame's coordinate space to the global coordinate space.

BUG=551601
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-03-28 19:07:31]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2016-03-28 19:09:15]

Description was changed from

==========
Implement accessibility support for CSS-transformed iframes.

This patch adds a gfx::Transform to ui::AXNodeData so that any accessibility
node could potentially use a matrix transform to represent its offset relative
to its parent window, rather than a simple translation.

Currently, we only use this field to represent the transform between a frame's
local coordinates and the coordinates of its local frame root. In the future
we may use transformations elsewhere in the accessibility tree, this may
be a lot more efficient than storing frame-relative coordinates for everything.

In addition, this change also handles cross-process iframes by calling a
delegate function implemented by RenderFrameHostImpl to convert a point in
a remote frame's coordinate space to the global coordinate space.

BUG=551601
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Implement accessibility support for CSS-transformed iframes.

This patch adds a gfx::Transform to ui::AXNodeData so that any accessibility
node could potentially use a matrix transform to represent its offset relative
to its parent window, rather than a simple translation.

Currently, we only use this field to represent the transform between a frame's
local coordinates and the coordinates of its local frame root. In the future
we may use transformations elsewhere in the accessibility tree, this may
be a lot more efficient than storing frame-relative coordinates for everything.

In addition, this change also handles cross-process iframes by calling a
delegate function implemented by RenderFrameHostImpl to convert a point in
a remote frame's coordinate space to the global coordinate space.

BUG=551601
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/0aec386227a2a2ea357ccea031b8d91dc4a853b1
Cr-Commit-Position: refs/heads/master@{#383534}
==========

[commit-bot: I haz the power, 2016-03-28 19:09:17]

Patchset 17 (id:??) landed as
https://crrev.com/0aec386227a2a2ea357ccea031b8d91dc4a853b1
Cr-Commit-Position: refs/heads/master@{#383534}