New issue
Advanced search Search tips

Issue 827175 link

Starred by 3 users
Status: Fixed
Owner:
ekaramad@chromium.org
Closed: May 2018
Cc:
kenrb@chromium.org
dtapu...@chromium.org
dcheng@chromium.org
wfh@chromium.org
creis@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

EventHandlerRegistry should be per-LocalFrameRoot and not per-Page

Project Member Reported by wjmaclean@chromium.org, Mar 29 2018 
The code in EventHandlerRegistry is currently a per-Page, but as event handlers often need to be tracked by compositors, it would make more sense for this to be owned by LocalFrameRoots, which have a 1:1 correspondence with LayerTreeViews.

We should refactor the code to reflect this.

Updating the references to |page_| in the class itself should be straightforward, so the main work appears to be in converting call sites for Page::GetEventHandlerRegistry(), most of which are associated with a Document or Frame reference.

Comment 1 by ekaramad@chromium.org, May 15 2018

Owner: ekaramad@chromium.org
Assigning to myself to take a stab at this refactor.

Comment 2 by wjmaclean@chromium.org, May 18 2018 

Issue 817792 has been merged into this issue.
Project Member

Comment 3 by bugdroid1@chromium.org, May 23 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7f1d43dbfb0a9b4267697da2ea8401c45a1dc745

commit 7f1d43dbfb0a9b4267697da2ea8401c45a1dc745
Author: Ehsan Karamad <ekaramad@chromium.org>
Date: Wed May 23 13:30:50 2018

[refactor] Move EventHandlerRegistry from Page to LocalFrame

Currently, all the event targets in a page are clumped together in the
single EventHandlerRegistry instance which is owned by the page. In a
world with OOPIFs and multiple local roots per process (i.e., multiple
LocalRoots sharing the same Page) the idea of putting all the handlers
together is not as interesting.

Essentially, all the events targeted to a LocalRoot should only deal
with event targets associated with the LocalRoot only. When a task
inside a frame requires probing event targets, it should ideally only
loop through those associated with the frame or at most those of the
local root which frame belongs to.

This CL will move the ownership of EHR to LocalFrame's which are local
roots. Therefore, with the CL each LocalFrameRoot will own an instance
of EHR which only deals with the event targets inside that root.

Bug:  827175 
Change-Id: Ic0be17ea611355ac739a9ce7867c3c4d3fbc2d4b
Reviewed-on: https://chromium-review.googlesource.com/1060519
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Ehsan Karamad <ekaramad@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Commit-Queue: Ehsan Karamad <ekaramad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561055}
[modify] https://crrev.com/7f1d43dbfb0a9b4267697da2ea8401c45a1dc745/third_party/blink/renderer/core/dom/document.cc
[modify] https://crrev.com/7f1d43dbfb0a9b4267697da2ea8401c45a1dc745/third_party/blink/renderer/core/dom/node.cc
[modify] https://crrev.com/7f1d43dbfb0a9b4267697da2ea8401c45a1dc745/third_party/blink/renderer/core/exported/web_frame_test.cc
[modify] https://crrev.com/7f1d43dbfb0a9b4267697da2ea8401c45a1dc745/third_party/blink/renderer/core/exported/web_plugin_container_impl.cc
[modify] https://crrev.com/7f1d43dbfb0a9b4267697da2ea8401c45a1dc745/third_party/blink/renderer/core/exported/web_plugin_container_test.cc
[modify] https://crrev.com/7f1d43dbfb0a9b4267697da2ea8401c45a1dc745/third_party/blink/renderer/core/exported/web_view_test.cc
[modify] https://crrev.com/7f1d43dbfb0a9b4267697da2ea8401c45a1dc745/third_party/blink/renderer/core/frame/event_handler_registry.cc
[modify] https://crrev.com/7f1d43dbfb0a9b4267697da2ea8401c45a1dc745/third_party/blink/renderer/core/frame/event_handler_registry.h
[modify] https://crrev.com/7f1d43dbfb0a9b4267697da2ea8401c45a1dc745/third_party/blink/renderer/core/frame/local_dom_window.cc
[modify] https://crrev.com/7f1d43dbfb0a9b4267697da2ea8401c45a1dc745/third_party/blink/renderer/core/frame/local_frame.cc
[modify] https://crrev.com/7f1d43dbfb0a9b4267697da2ea8401c45a1dc745/third_party/blink/renderer/core/frame/local_frame.h
[modify] https://crrev.com/7f1d43dbfb0a9b4267697da2ea8401c45a1dc745/third_party/blink/renderer/core/frame/local_frame_view.cc
[modify] https://crrev.com/7f1d43dbfb0a9b4267697da2ea8401c45a1dc745/third_party/blink/renderer/core/html/forms/slider_thumb_element.cc
[modify] https://crrev.com/7f1d43dbfb0a9b4267697da2ea8401c45a1dc745/third_party/blink/renderer/core/input/event_handler.cc
[modify] https://crrev.com/7f1d43dbfb0a9b4267697da2ea8401c45a1dc745/third_party/blink/renderer/core/input/event_handler.h
[modify] https://crrev.com/7f1d43dbfb0a9b4267697da2ea8401c45a1dc745/third_party/blink/renderer/core/input/pointer_event_manager.cc
[modify] https://crrev.com/7f1d43dbfb0a9b4267697da2ea8401c45a1dc745/third_party/blink/renderer/core/input/touch_event_manager.cc
[modify] https://crrev.com/7f1d43dbfb0a9b4267697da2ea8401c45a1dc745/third_party/blink/renderer/core/layout/layout_object.cc
[modify] https://crrev.com/7f1d43dbfb0a9b4267697da2ea8401c45a1dc745/third_party/blink/renderer/core/page/page.cc
[modify] https://crrev.com/7f1d43dbfb0a9b4267697da2ea8401c45a1dc745/third_party/blink/renderer/core/page/page.h
[modify] https://crrev.com/7f1d43dbfb0a9b4267697da2ea8401c45a1dc745/third_party/blink/renderer/core/page/scrolling/scrolling_coordinator.cc
[modify] https://crrev.com/7f1d43dbfb0a9b4267697da2ea8401c45a1dc745/third_party/blink/renderer/core/testing/internals.cc
Project Member

Comment 4 by bugdroid1@chromium.org, May 23 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8d4fc7b5059ccc21292fe30689c9fa3ad4505b5f

commit 8d4fc7b5059ccc21292fe30689c9fa3ad4505b5f
Author: Ehsan Karamad <ekaramad@chromium.org>
Date: Wed May 23 21:57:53 2018

Followup to CL 1060519 to address a missed comment

We do not need a separate check for GetPage(), since GetPage() and
GetFrame() start returning null (with the same timing) once
LocalDOMWindow/Document are no longer attached to a frame.

Bug:  827175 
Change-Id: I963d22da35e731a4b249abfb1fb77ecd37c8a750
Reviewed-on: https://chromium-review.googlesource.com/1070501
Commit-Queue: Ehsan Karamad <ekaramad@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561267}
[modify] https://crrev.com/8d4fc7b5059ccc21292fe30689c9fa3ad4505b5f/third_party/blink/renderer/core/frame/local_dom_window.cc

Comment 5 by ekaramad@chromium.org, May 23 2018

Status: Fixed (was: Assigned)
Marking as fixed since as of comment #3 the EHR is in EeventHandler of the local root.
►

Sign in to add a comment