Handle pointer-events: none in browser process hittesting.

Handle pointer-events: none in browser process hittesting.

This change adds one HittestData structure per surface and a HittestDelegate in RenderWidgetHostInputEventRouter that can reject surfaces based on the HittestData.

BUG=491334
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/e6119aac120294a5132703363801033bdad6b2f2
Cr-Commit-Position: refs/heads/master@{#371691}

[lfg, 2015-12-01 19:40:46]

Description was changed from

==========
WIP
==========

to

==========
WIP
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[lfg, 2016-01-13 18:43:09]

Description was changed from

==========
WIP
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Handle pointer-events: none in browser process hittesting.

This change adds a flag to SurfaceDrawQuad to signal that it should be ignored
by browser process hittesting.

BUG=491334

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[lfg, 2016-01-13 18:45:17]

lfg@chromium.org changed reviewers:
+ piman@chromium.org

[lfg, 2016-01-13 18:45:18]

Hi piman@, can you take a look?

[piman, 2016-01-13 19:23:59]

piman@chromium.org changed reviewers:
+ danakj@chromium.org, enne@chromium.org, vollick@chromium.org

[piman, 2016-01-13 19:24:00]

+others, maybe they feel differently.

As I mentioned separately, I'm really not excited about combining graphics data
with input data. Especially if it needs to flow through the compositor, where
we're actually trying to go the opposite direction (decouple input from the
compositor).

One way to do that is to use the RenderWidget::QueueMessage mechanism:
- from RenderWidget::WillBeginCompositorFrame go through the render frame
proxies
- for each RFP, get the relevant input data (e.g. here, the flag to ignore hit
testing)
- build a surface_id->input_data map (or render frame id -> input data,
whichever is convenient)
- send it through an input-specific message, using RenderWidget::QueueMessage
(which will guarantee that the message will go with the frame if we will be
sending a frame, or via regular IPC if not).

On the browser side, feed that information into the hit testing logic.

[danakj, 2016-01-13 19:29:40]

On Wed, Jan 13, 2016 at 11:23 AM, <piman@chromium.org> wrote:

> +others, maybe they feel differently.
>
> As I mentioned separately, I'm really not excited about combining graphics
> data
> with input data. Especially if it needs to flow through the compositor,
> where
> we're actually trying to go the opposite direction (decouple input from the
> compositor).
>
> One way to do that is to use the RenderWidget::QueueMessage mechanism:
> - from RenderWidget::WillBeginCompositorFrame go through the render frame
> proxies
> - for each RFP, get the relevant input data (e.g. here, the flag to ignore
> hit
> testing)
> - build a surface_id->input_data map (or render frame id -> input data,
> whichever is convenient)
> - send it through an input-specific message, using
> RenderWidget::QueueMessage
> (which will guarantee that the message will go with the frame if we will be
> sending a frame, or via regular IPC if not).
>
> On the browser side, feed that information into the hit testing logic.
>

Yeah, plumbing data through the compositor just to get it out the other end
and use it for something there raised red flags for me. I don't think we
want to create these sorts of patterns for non-presentation data, with the
exception of latencyinfos cuz we actually want to track them through the
gpu system.

I appreciate piman's thoughts on how to do this without adding plumbing
thru the compositor.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[lfg, 2016-01-13 21:21:24]

On 2016/01/13 19:24:00, piman (Slow to review) wrote:
> +others, maybe they feel differently.
> 
> As I mentioned separately, I'm really not excited about combining graphics
data
> with input data. Especially if it needs to flow through the compositor, where
> we're actually trying to go the opposite direction (decouple input from the
> compositor).
> 
> One way to do that is to use the RenderWidget::QueueMessage mechanism:
> - from RenderWidget::WillBeginCompositorFrame go through the render frame
> proxies
> - for each RFP, get the relevant input data (e.g. here, the flag to ignore hit
> testing)
> - build a surface_id->input_data map (or render frame id -> input data,
> whichever is convenient)
> - send it through an input-specific message, using RenderWidget::QueueMessage
> (which will guarantee that the message will go with the frame if we will be
> sending a frame, or via regular IPC if not).
> 
> On the browser side, feed that information into the hit testing logic.

I wouldn't mind implementing your proposed idea, but it would significantly
increase code complexity, as we would need to introduce and maintain maps
between surfaces<->data, plus it would increase the amount of data sent
from the renderer to the browser, as well as work done by the browser.
Also, this idea would not be able to solve the issue with transparent
layers, so all this extra complexity only works only for the one specific
case we need to ignore surfaces.

Note also that hittesting is not only for input, it can also be used for
other things (for example widget positioning/placement). I would also argue
that hittesting is very tied to compositing, since it's pretty much
painting in reverse.

[piman, 2016-01-13 23:02:25]

On Wed, Jan 13, 2016 at 1:21 PM, <lfg@chromium.org> wrote:

> On 2016/01/13 19:24:00, piman (Slow to review) wrote:
>
>> +others, maybe they feel differently.
>>
>
> As I mentioned separately, I'm really not excited about combining graphics
>>
> data
>
>> with input data. Especially if it needs to flow through the compositor,
>> where
>> we're actually trying to go the opposite direction (decouple input from
>> the
>> compositor).
>>
>
> One way to do that is to use the RenderWidget::QueueMessage mechanism:
>> - from RenderWidget::WillBeginCompositorFrame go through the render frame
>> proxies
>> - for each RFP, get the relevant input data (e.g. here, the flag to
>> ignore hit
>> testing)
>> - build a surface_id->input_data map (or render frame id -> input data,
>> whichever is convenient)
>> - send it through an input-specific message, using
>> RenderWidget::QueueMessage
>> (which will guarantee that the message will go with the frame if we will
>> be
>> sending a frame, or via regular IPC if not).
>>
>
> On the browser side, feed that information into the hit testing logic.
>>
>
> I wouldn't mind implementing your proposed idea, but it would significantly
> increase code complexity, as we would need to introduce and maintain maps
> between surfaces<->data, plus it would increase the amount of data sent
> from the renderer to the browser, as well as work done by the browser.
> Also, this idea would not be able to solve the issue with transparent
> layers, so all this extra complexity only works only for the one specific
> case we need to ignore surfaces.
>
> Note also that hittesting is not only for input, it can also be used for
> other things (for example widget positioning/placement). I would also argue
> that hittesting is very tied to compositing, since it's pretty much
> painting in reverse.
>

Well, I disagree on that. Input is a web element-level concept, the shape
of which is completely lost once it gets transformed into graphics data.
Think e.g. rounded corners on an element (can't be represented as quads,
but affect hit testing). So what you have currently is an approximation,
and solving more and more edge cases will exponentially increase the
complexity of the graphics data until essentially we represent the original
DOM. This would not be acceptable for graphics data. So I think you need to
bite the bullet and separate the input data, the sooner the better.


>
>
> https://codereview.chromium.org/1489913003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[lfg, 2016-01-15 23:25:54]

On 2016/01/13 23:02:25, piman (Slow to review) wrote:
> Well, I disagree on that. Input is a web element-level concept, the shape
> of which is completely lost once it gets transformed into graphics data.
> Think e.g. rounded corners on an element (can't be represented as quads,
> but affect hit testing). So what you have currently is an approximation,
> and solving more and more edge cases will exponentially increase the
> complexity of the graphics data until essentially we represent the original
> DOM. This would not be acceptable for graphics data. So I think you need to
> bite the bullet and separate the input data, the sooner the better.

I'm still a bit worried about replicating a lot of the graphics data, just
because we need a few bits to be different, but hopefully this won't be
needed. I've implemented your suggestion, please take a look.

[piman, 2016-01-16 01:25:36]

This seems pretty reasonable. A few nits, and one fix needed.

https://codereview.chromium.org/1489913003/diff/240001/content/browser/render...
File content/browser/renderer_host/render_widget_host_input_event_router.cc
(right):

https://codereview.chromium.org/1489913003/diff/240001/content/browser/render...
content/browser/renderer_host/render_widget_host_input_event_router.cc:173: if
(it == hittest_data_.end())
nit: needs {} per style (both branches).

https://codereview.chromium.org/1489913003/diff/240001/content/browser/render...
content/browser/renderer_host/render_widget_host_input_event_router.cc:177:
it->second = data;
nit: hittest_data_[params.surface_id] = data; may just be as good as all of this
- single lookup, and very concise.

Other possibility, if you want to avoid the default constructor + copy, you can
insert unconditionally, and if returns (iterator, false), assign using the
iterator:
auto it_and_result = hittest_data_.insert(params.surface_id, data);
if (!it_and_result.second)
  *(it_and_result.first) = data;

https://codereview.chromium.org/1489913003/diff/240001/content/browser/render...
File content/browser/renderer_host/render_widget_host_input_event_router.h
(right):

https://codereview.chromium.org/1489913003/diff/240001/content/browser/render...
content/browser/renderer_host/render_widget_host_input_event_router.h:68: bool
ignored_for_hittest_;
nit: style for public struct members is without a trailing _.

https://codereview.chromium.org/1489913003/diff/240001/content/renderer/rende...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/1489913003/diff/240001/content/renderer/rende...
content/renderer/render_frame_proxy.cc:205:
MESSAGE_DELIVERY_POLICY_WITH_NEXT_SWAP);
You want MESSAGE_DELIVERY_POLICY_WITH_VISUAL_STATE to synchronize with the
commit.
MESSAGE_DELIVERY_POLICY_WITH_NEXT_SWAP skips the commit logic.

[lfg, 2016-01-18 21:35:20]

https://codereview.chromium.org/1489913003/diff/240001/content/browser/render...
File content/browser/renderer_host/render_widget_host_input_event_router.cc
(right):

https://codereview.chromium.org/1489913003/diff/240001/content/browser/render...
content/browser/renderer_host/render_widget_host_input_event_router.cc:173: if
(it == hittest_data_.end())
On 2016/01/16 01:25:36, piman (Slow to review) wrote:
> nit: needs {} per style (both branches).

I went with the suggestion below.

https://codereview.chromium.org/1489913003/diff/240001/content/browser/render...
content/browser/renderer_host/render_widget_host_input_event_router.cc:177:
it->second = data;
On 2016/01/16 01:25:36, piman (Slow to review) wrote:
> nit: hittest_data_[params.surface_id] = data; may just be as good as all of
this
> - single lookup, and very concise.
> 
> Other possibility, if you want to avoid the default constructor + copy, you
can
> insert unconditionally, and if returns (iterator, false), assign using the
> iterator:
> auto it_and_result = hittest_data_.insert(params.surface_id, data);
> if (!it_and_result.second)
>   *(it_and_result.first) = data;

Done.

https://codereview.chromium.org/1489913003/diff/240001/content/browser/render...
File content/browser/renderer_host/render_widget_host_input_event_router.h
(right):

https://codereview.chromium.org/1489913003/diff/240001/content/browser/render...
content/browser/renderer_host/render_widget_host_input_event_router.h:68: bool
ignored_for_hittest_;
On 2016/01/16 01:25:36, piman (Slow to review) wrote:
> nit: style for public struct members is without a trailing _.

Done.

https://codereview.chromium.org/1489913003/diff/240001/content/renderer/rende...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/1489913003/diff/240001/content/renderer/rende...
content/renderer/render_frame_proxy.cc:205:
MESSAGE_DELIVERY_POLICY_WITH_NEXT_SWAP);
On 2016/01/16 01:25:36, piman (Slow to review) wrote:
> You want MESSAGE_DELIVERY_POLICY_WITH_VISUAL_STATE to synchronize with the
> commit.
> MESSAGE_DELIVERY_POLICY_WITH_NEXT_SWAP skips the commit logic.

Done. It doesn't matter here since they are replaced, but wouldn't that cause
multiple updates to a single surface be submitted on a frame?

[piman, 2016-01-20 01:21:19]

lgtm

https://codereview.chromium.org/1489913003/diff/240001/content/renderer/rende...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/1489913003/diff/240001/content/renderer/rende...
content/renderer/render_frame_proxy.cc:205:
MESSAGE_DELIVERY_POLICY_WITH_NEXT_SWAP);
On 2016/01/18 21:35:20, lfg wrote:
> On 2016/01/16 01:25:36, piman (Slow to review) wrote:
> > You want MESSAGE_DELIVERY_POLICY_WITH_VISUAL_STATE to synchronize with the
> > commit.
> > MESSAGE_DELIVERY_POLICY_WITH_NEXT_SWAP skips the commit logic.
> 
> Done. It doesn't matter here since they are replaced, but wouldn't that cause
> multiple updates to a single surface be submitted on a frame?

I'm not fully sure I understand the question...

With MESSAGE_DELIVERY_POLICY_WITH_VISUAL_STATE it will send the message exactly
once - either with the commit, or, if there is no pending commit or the commit
or swap gets cancelled (e.g. no update), as a regular message.
With MESSAGE_DELIVERY_POLICY_WITH_NEXT_SWAP it would send the message exactly
once too, but it would skip the commit flow, i.e. if there's already a frame
pending before the commit, the message may be sent earlier (but "too early" in
your case).

WillBeginCompositorFrame should be called exactly once per RFP per commit, so I
don't think you're sending redundant information per commit (though one could
argue that you could send a message only if web_frame_->ignoredForHittest()
changes, not sure if it matters).

[lfg, 2016-01-20 17:52:52]

On 2016/01/20 01:21:19, piman (Slow to review) wrote:
> I'm not fully sure I understand the question...
> 
> With MESSAGE_DELIVERY_POLICY_WITH_VISUAL_STATE it will send the message
exactly
> once - either with the commit, or, if there is no pending commit or the commit
> or swap gets cancelled (e.g. no update), as a regular message.
> With MESSAGE_DELIVERY_POLICY_WITH_NEXT_SWAP it would send the message exactly
> once too, but it would skip the commit flow, i.e. if there's already a frame
> pending before the commit, the message may be sent earlier (but "too early" in
> your case).
> 
> WillBeginCompositorFrame should be called exactly once per RFP per commit, so
I
> don't think you're sending redundant information per commit (though one could
> argue that you could send a message only if web_frame_->ignoredForHittest()
> changes, not sure if it matters).

What I'm wondering is the following scenario:

1. Frame begins, WillCommit gets called and the message is queued.
2. Frame is aborted. Message stays in the queue and is not sent.
3. Another frame begins, WillCommit gets called again and another message is
queued. We now have 2 messages in the queue.
4. Frame commits and both messages are sent.

Like I said, this isn't important here, but I'd like to understand it as it may
impact future changes.

[lfg, 2016-01-20 18:14:16]

lfg@chromium.org changed reviewers:
+ dcheng@chromium.org

[lfg, 2016-01-20 18:14:17]

Hi Daniel, can you take a look at the blink changes?

[piman, 2016-01-20 22:37:39]

On Wed, Jan 20, 2016 at 9:52 AM, <lfg@chromium.org> wrote:

> On 2016/01/20 01:21:19, piman (Slow to review) wrote:
>
>> I'm not fully sure I understand the question...
>>
>
> With MESSAGE_DELIVERY_POLICY_WITH_VISUAL_STATE it will send the message
>>
> exactly
>
>> once - either with the commit, or, if there is no pending commit or the
>> commit
>> or swap gets cancelled (e.g. no update), as a regular message.
>> With MESSAGE_DELIVERY_POLICY_WITH_NEXT_SWAP it would send the message
>> exactly
>> once too, but it would skip the commit flow, i.e. if there's already a
>> frame
>> pending before the commit, the message may be sent earlier (but "too
>> early" in
>> your case).
>>
>
> WillBeginCompositorFrame should be called exactly once per RFP per commit,
>> so
>>
> I
>
>> don't think you're sending redundant information per commit (though one
>> could
>> argue that you could send a message only if
>> web_frame_->ignoredForHittest()
>> changes, not sure if it matters).
>>
>
> What I'm wondering is the following scenario:
>
> 1. Frame begins, WillCommit gets called and the message is queued.
> 2. Frame is aborted. Message stays in the queue and is not sent.
>

If the frame is aborted because of no changes (at commit time) or no damage
(at draw time), we will still send the queued message:
see QueueMessageSwapPromise::DidNotSwap and
FrameSwapMessageQueue::DidNotSwap. If the frame is aborted because we're
tearing down the compositor (because we're tearing down the RenderWidget),
we will not send the message, but we'll stop committing too.


> 3. Another frame begins, WillCommit gets called again and another message
> is
> queued. We now have 2 messages in the queue.
> 4. Frame commits and both messages are sent.
>
> Like I said, this isn't important here, but I'd like to understand it as
> it may
> impact future changes.
>
>
> https://codereview.chromium.org/1489913003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dcheng, 2016-01-21 23:47:20]

https://codereview.chromium.org/1489913003/diff/280001/content/renderer/rende...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/1489913003/diff/280001/content/renderer/rende...
content/renderer/render_frame_proxy.cc:199: if (ChildFrameCompositingHelper*
compositing_helper =
Just saying:
  if (compositing_helper_)

will be shorter.

https://codereview.chromium.org/1489913003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebRemoteFrameImpl.h (right):

https://codereview.chromium.org/1489913003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebRemoteFrameImpl.h:196: bool ignoredForHittest()
const override;
isIgnoredForHitTest or shouldIgnoreForHitTest

[lfg, 2016-01-25 18:45:01]

Patchset #20 (id:380001) has been deleted

[lfg, 2016-01-25 20:29:23]

dcheng@: please take a look.

danakj@: Can you take a look at surface_id.h? I had to change that after
piman@'s lgtm to fix compile on Linux. This change goes against the style guide
recommendation, but there's already a precedent to overriding the hash key of
SurfaceId on that file. I can change it if you prefer to specialize the hash
function in the map.

[lfg, 2016-01-25 20:51:38]

Description was changed from

==========
Handle pointer-events: none in browser process hittesting.

This change adds a flag to SurfaceDrawQuad to signal that it should be ignored
by browser process hittesting.

BUG=491334

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Handle pointer-events: none in browser process hittesting.

This change adds one HittestData structure per surface and a HittestDelegate in
RenderWidgetHostInputEventRouter that can reject surfaces based on the
HittestData.

BUG=491334

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[dcheng, 2016-01-26 00:26:25]

lgtm

[lfg, 2016-01-26 23:40:26]

danakj@: I've switched the hash implementation according to the new style.
Please take a look -- and sorry, I've rebased on the same patch :(

[danakj, 2016-01-27 00:24:54]

cc/ LGTM

[dcheng, 2016-01-27 00:25:46]

https://codereview.chromium.org/1489913003/diff/440001/cc/surfaces/surface_id.h
File cc/surfaces/surface_id.h (right):

https://codereview.chromium.org/1489913003/diff/440001/cc/surfaces/surface_id...
cc/surfaces/surface_id.h:38: return std::hash<uint64_t>()(key.id);
#include <functional>?

[lfg, 2016-01-27 00:41:02]

https://codereview.chromium.org/1489913003/diff/440001/cc/surfaces/surface_id.h
File cc/surfaces/surface_id.h (right):

https://codereview.chromium.org/1489913003/diff/440001/cc/surfaces/surface_id...
cc/surfaces/surface_id.h:38: return std::hash<uint64_t>()(key.id);
On 2016/01/27 00:25:46, dcheng wrote:
> #include <functional>?

Ugh, you're right. (For some reason I thought that wasn't necessary and removed
in the last patch, duh).

[lfg, 2016-01-27 00:44:42]

The CQ bit was checked by lfg@chromium.org

[lfg, 2016-01-27 00:44:43]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
dcheng@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1489913003/#ps460001
(title: "readding header")

[commit-bot: I haz the power, 2016-01-27 00:45:51]

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

[commit-bot: I haz the power, 2016-01-27 02:14:40]

Description was changed from

==========
Handle pointer-events: none in browser process hittesting.

This change adds one HittestData structure per surface and a HittestDelegate in
RenderWidgetHostInputEventRouter that can reject surfaces based on the
HittestData.

BUG=491334

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Handle pointer-events: none in browser process hittesting.

This change adds one HittestData structure per surface and a HittestDelegate in
RenderWidgetHostInputEventRouter that can reject surfaces based on the
HittestData.

BUG=491334

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-01-27 02:14:40]

Committed patchset #23 (id:460001)

[commit-bot: I haz the power, 2016-01-27 02:15:42]

Description was changed from

==========
Handle pointer-events: none in browser process hittesting.

This change adds one HittestData structure per surface and a HittestDelegate in
RenderWidgetHostInputEventRouter that can reject surfaces based on the
HittestData.

BUG=491334

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Handle pointer-events: none in browser process hittesting.

This change adds one HittestData structure per surface and a HittestDelegate in
RenderWidgetHostInputEventRouter that can reject surfaces based on the
HittestData.

BUG=491334

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/e6119aac120294a5132703363801033bdad6b2f2
Cr-Commit-Position: refs/heads/master@{#371691}
==========

[commit-bot: I haz the power, 2016-01-27 02:15:43]

Patchset 23 (id:??) landed as
https://crrev.com/e6119aac120294a5132703363801033bdad6b2f2
Cr-Commit-Position: refs/heads/master@{#371691}

[Charlie Reis, 2016-01-29 00:19:47]

A revert of this CL (patchset #23 id:460001) has been created in
https://codereview.chromium.org/1643063002/ by creis@chromium.org.

The reason for reverting is: Led to a large spike in renderer crashes in
https://crbug.com/582149.  There's a fix in review
(https://codereview.chromium.org/1648113002/) but it probably won't land in time
for the next canary..