Don't request wheel or touch events when the view of pepper has no area.

Don't request wheel or touch events when the view of pepper has no area.

Some pages have a zero width/height embedded flash element. They should
not request touch/wheel event listeners if they don't have a size. This
is the easiest way to let blink know that pepper doesn't require the
events.

BUG=670103
Committed: https://crrev.com/c86587bb2e6e0f384bfc21de8dad151cd6d9d21e
Cr-Commit-Position: refs/heads/master@{#436076}

[dtapuska, 2016-12-02 16:29:18]

The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 16:29:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dtapuska, 2016-12-02 16:31:55]

dtapuska@chromium.org changed reviewers:
+ bbudge@chromium.org

[dtapuska, 2016-12-02 16:31:56]

bbudge@ Can you let me know if this is sane?

[dtapuska, 2016-12-02 16:33:49]

Description was changed from

==========
Don't request wheel or mouse events when the view of pepper has no area.

Some pages have a zero width/height embedded flash element. They should
not request touch/wheel event listeners if they don't have a size. This
is the easiest way to let blink know that pepper doesn't require the
events.

BUG=670103
==========

to

==========
Don't request wheel or touch events when the view of pepper has no area.

Some pages have a zero width/height embedded flash element. They should
not request touch/wheel event listeners if they don't have a size. This
is the easiest way to let blink know that pepper doesn't require the
events.

BUG=670103
==========

[commit-bot: I haz the power, 2016-12-02 17:13:45]

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

[commit-bot: I haz the power, 2016-12-02 17:13:46]

Dry run: This issue passed the CQ dry run.

[bbudge, 2016-12-02 18:38:01]

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

[bbudge, 2016-12-02 18:38:02]

I've added a few comments on your CL. As far as the safety of modifying input
evens for Flash, I'm not an expert, so adding piman for his opinion.

https://codereview.chromium.org/2551493002/diff/1/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/2551493002/diff/1/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1298:
container_->setWantsWheelEvents(IsAcceptingWheelEvents());
Would you consider refactoring IsAcceptingWheelEvents into
UpdateWheelEventRequest to match how touch events work? It seems to be doing the
same thing.

https://codereview.chromium.org/2551493002/diff/1/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1644: if
(view_data_.rect.size.width == 0 || view_data_.rect.size.height == 0) {
Could you use unobscured_rect_.IsEmpty() here instead? I think that might be
better, if I understand the purpose of this change, which is to not request
events that we can never receive. I'm assuming it's impossible for Blink to
return touch events when the plugin is obscured.

https://cs.chromium.org/chromium/src/content/renderer/pepper/pepper_plugin_in...

[piman, 2016-12-02 19:37:14]

On Fri, Dec 2, 2016 at 10:38 AM, <bbudge@chromium.org> wrote:

> I've added a few comments on your CL. As far as the safety of modifying
> input
> evens for Flash, I'm not an expert, so adding piman for his opinion.
>

I /think/ that's ok.


>
>
> https://codereview.chromium.org/2551493002/diff/1/content/
> renderer/pepper/pepper_plugin_instance_impl.cc
> File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):
>
> https://codereview.chromium.org/2551493002/diff/1/content/
> renderer/pepper/pepper_plugin_instance_impl.cc#newcode1298
> content/renderer/pepper/pepper_plugin_instance_impl.cc:1298:
> container_->setWantsWheelEvents(IsAcceptingWheelEvents());
> Would you consider refactoring IsAcceptingWheelEvents into
> UpdateWheelEventRequest to match how touch events work? It seems to be
> doing the same thing.
>
> https://codereview.chromium.org/2551493002/diff/1/content/
> renderer/pepper/pepper_plugin_instance_impl.cc#newcode1644
> content/renderer/pepper/pepper_plugin_instance_impl.cc:1644: if
> (view_data_.rect.size.width == 0 || view_data_.rect.size.height == 0) {
> Could you use unobscured_rect_.IsEmpty() here instead? I think that
> might be better, if I understand the purpose of this change, which is to
> not request events that we can never receive. I'm assuming it's
> impossible for Blink to return touch events when the plugin is obscured.
>
> https://cs.chromium.org/chromium/src/content/renderer/
> pepper/pepper_plugin_instance_impl.cc?rcl=1480681004&l=1273
>
> https://codereview.chromium.org/2551493002/
>

-- 
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.

[dtapuska, 2016-12-02 19:42:08]

https://codereview.chromium.org/2551493002/diff/1/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/2551493002/diff/1/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1298:
container_->setWantsWheelEvents(IsAcceptingWheelEvents());
On 2016/12/02 18:38:01, bbudge wrote:
> Would you consider refactoring IsAcceptingWheelEvents into
> UpdateWheelEventRequest to match how touch events work? It seems to be doing
the
> same thing.

Done.

https://codereview.chromium.org/2551493002/diff/1/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1644: if
(view_data_.rect.size.width == 0 || view_data_.rect.size.height == 0) {
On 2016/12/02 18:38:01, bbudge wrote:
> Could you use unobscured_rect_.IsEmpty() here instead? I think that might be
> better, if I understand the purpose of this change, which is to not request
> events that we can never receive. I'm assuming it's impossible for Blink to
> return touch events when the plugin is obscured.
> 
>
https://cs.chromium.org/chromium/src/content/renderer/pepper/pepper_plugin_in...
> 

Although that may be optimal I'd worry that it would cause more work. Since if
we adjust when setWantsWheelEvents(...) it ends up telling the scroll
co-ordinator to update geometry. see;
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebPluginC...

The minimal thing I am after is fixing 0,0 width/height flash plugins asking to
listen to input which is un-necessary

[bbudge, 2016-12-02 22:10:29]

lgtm

https://codereview.chromium.org/2551493002/diff/1/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/2551493002/diff/1/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1644: if
(view_data_.rect.size.width == 0 || view_data_.rect.size.height == 0) {
On 2016/12/02 19:42:07, dtapuska wrote:
> On 2016/12/02 18:38:01, bbudge wrote:
> > Could you use unobscured_rect_.IsEmpty() here instead? I think that might be
> > better, if I understand the purpose of this change, which is to not request
> > events that we can never receive. I'm assuming it's impossible for Blink to
> > return touch events when the plugin is obscured.
> > 
> >
>
https://cs.chromium.org/chromium/src/content/renderer/pepper/pepper_plugin_in...
> > 
> 
> Although that may be optimal I'd worry that it would cause more work. Since if
> we adjust when setWantsWheelEvents(...) it ends up telling the scroll
> co-ordinator to update geometry. see;
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebPluginC...
> 
> The minimal thing I am after is fixing 0,0 width/height flash plugins asking
to
> listen to input which is un-necessary

OK

[dtapuska, 2016-12-02 22:13:58]

The CQ bit was checked by dtapuska@chromium.org

[commit-bot: I haz the power, 2016-12-02 22:14:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 23:21:37]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480716838201480,
"parent_rev": "d829a166a0a32d9fa75900d0bb269dc818ac4500", "commit_rev":
"c25a813b13ee2f8bd917a858bab2db67a5be66a1"}

[commit-bot: I haz the power, 2016-12-02 23:22:04]

Description was changed from

==========
Don't request wheel or touch events when the view of pepper has no area.

Some pages have a zero width/height embedded flash element. They should
not request touch/wheel event listeners if they don't have a size. This
is the easiest way to let blink know that pepper doesn't require the
events.

BUG=670103
==========

to

==========
Don't request wheel or touch events when the view of pepper has no area.

Some pages have a zero width/height embedded flash element. They should
not request touch/wheel event listeners if they don't have a size. This
is the easiest way to let blink know that pepper doesn't require the
events.

BUG=670103
==========

[commit-bot: I haz the power, 2016-12-02 23:22:04]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-02 23:24:54]

Description was changed from

==========
Don't request wheel or touch events when the view of pepper has no area.

Some pages have a zero width/height embedded flash element. They should
not request touch/wheel event listeners if they don't have a size. This
is the easiest way to let blink know that pepper doesn't require the
events.

BUG=670103
==========

to

==========
Don't request wheel or touch events when the view of pepper has no area.

Some pages have a zero width/height embedded flash element. They should
not request touch/wheel event listeners if they don't have a size. This
is the easiest way to let blink know that pepper doesn't require the
events.

BUG=670103

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

[commit-bot: I haz the power, 2016-12-02 23:24:55]

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