New issue
Advanced search Search tips

Issue 755160 link

Starred by 1 user
Status: Fixed
Owner:
mustaq@chromium.org
Closed: Sep 2017
Cc:
dtapu...@chromium.org
mlamouri@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 3
Type: Bug

Blocking:
issue 696617
issue 729694



Sign in to add a comment

Consolidate UserGestureIndicator/Token split w/o changing current behavior

Project Member Reported by mustaq@chromium.org, Aug 14 2017 
To implement a Frame-based interface for our simple user gesture model (Issue 696617), we need to clean up the existing state handling interface.

More precisely, the current interface is split between UserGestureIndicator vs UserGestureToken, with "user classes" differing in which state to store, how to consume etc.  This split makes it hard to switch between current and new implementation through a flag.

We need to consolidate the interface split.  In addition to aiding our end goal, this will also make the old interface simpler and possibly more logical.

Comment 1 by mustaq@chromium.org, Aug 14 2017

Blocking: 696617 729694
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 15 2017 

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

commit 5690a51a6939855defec82ea8373196ba8ec73c4
Author: Mustaq Ahmed <mustaq@google.com>
Date: Tue Aug 15 18:23:22 2017

Added a "parallel" UGI creation interface in LocalFrame.

This is a code-cleanup CL without any change in functionality.  The CL
adds the new interface in LocalFrame to ease switching between UGI and
the planned simple user gesture model based on Frame states.  Here we
replaced the UGT::Create() calls in EventHandler.cpp & WebViewImpl.cpp
only to keep the first CL small; the remaining (~90) calls will be
replaced later on.

The CL also cleans up Frame/LocalFrame/LocalFrameClient interface for
user gestures with the new model in mind.

Bug:  755160 
Change-Id: I8d019921f688fd7c80c12da6a94b3c62d11e0d24
Reviewed-on: https://chromium-review.googlesource.com/610827
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Commit-Queue: Mustaq Ahmed <mustaq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494452}
[modify] https://crrev.com/5690a51a6939855defec82ea8373196ba8ec73c4/third_party/WebKit/Source/core/dom/UserGestureIndicator.cpp
[modify] https://crrev.com/5690a51a6939855defec82ea8373196ba8ec73c4/third_party/WebKit/Source/core/dom/UserGestureIndicator.h
[modify] https://crrev.com/5690a51a6939855defec82ea8373196ba8ec73c4/third_party/WebKit/Source/core/dom/UserGestureIndicatorTest.cpp
[modify] https://crrev.com/5690a51a6939855defec82ea8373196ba8ec73c4/third_party/WebKit/Source/core/exported/WebFrame.cpp
[modify] https://crrev.com/5690a51a6939855defec82ea8373196ba8ec73c4/third_party/WebKit/Source/core/exported/WebRemoteFrameImpl.cpp
[modify] https://crrev.com/5690a51a6939855defec82ea8373196ba8ec73c4/third_party/WebKit/Source/core/exported/WebViewImpl.cpp
[modify] https://crrev.com/5690a51a6939855defec82ea8373196ba8ec73c4/third_party/WebKit/Source/core/exported/WorkerShadowPage.cpp
[modify] https://crrev.com/5690a51a6939855defec82ea8373196ba8ec73c4/third_party/WebKit/Source/core/frame/Frame.cpp
[modify] https://crrev.com/5690a51a6939855defec82ea8373196ba8ec73c4/third_party/WebKit/Source/core/frame/Frame.h
[modify] https://crrev.com/5690a51a6939855defec82ea8373196ba8ec73c4/third_party/WebKit/Source/core/frame/LocalFrame.cpp
[modify] https://crrev.com/5690a51a6939855defec82ea8373196ba8ec73c4/third_party/WebKit/Source/core/frame/LocalFrame.h
[modify] https://crrev.com/5690a51a6939855defec82ea8373196ba8ec73c4/third_party/WebKit/Source/core/frame/WebLocalFrameImpl.cpp
[modify] https://crrev.com/5690a51a6939855defec82ea8373196ba8ec73c4/third_party/WebKit/Source/core/input/EventHandler.cpp
[modify] https://crrev.com/5690a51a6939855defec82ea8373196ba8ec73c4/third_party/WebKit/Source/modules/csspaint/PaintWorkletGlobalScope.cpp
[modify] https://crrev.com/5690a51a6939855defec82ea8373196ba8ec73c4/third_party/WebKit/Source/modules/webaudio/BaseAudioContextTest.cpp
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 16 2017 

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

commit 8f2e8f94d90f918a3766ee8e7ba0336d27b47e64
Author: Mustaq Ahmed <mustaq@google.com>
Date: Wed Aug 16 20:16:02 2017

Disallow UserGestureToken creation without a UserGestureIndicator

This is a code-cleanup CL without any change in functionality, follows
chromium-review.googlesource.com/c/610827 to complete ref fixes.

This CL removes all refs to UGT::Create and hides the method from all
users except from UGI, to ease switching between current UGI model and
the planned Frame-based user gesture model.

Bug:  755160 
Change-Id: I1bd7b71bd9d922f678b5908791f14e6e7d4ff2c0
TBR: mlamouri@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/615471
Commit-Queue: Mustaq Ahmed <mustaq@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494919}
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/core/dom/UserGestureIndicator.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/core/dom/UserGestureIndicator.h
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/core/dom/UserGestureIndicatorTest.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/core/exported/WebFrameTest.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/core/exported/WebInputMethodControllerImpl.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/core/exported/WebPluginContainerImpl.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/core/exported/WebScopedUserGesture.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/core/exported/WebUserGestureTokenTest.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/core/exported/WebViewTest.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/core/frame/FrameTest.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/core/frame/SuspendableScriptExecutor.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/core/frame/WebFrameWidgetBase.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/core/frame/WebFrameWidgetImpl.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/core/frame/WebLocalFrameImpl.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/core/html/HTMLMediaElementEventListenersTest.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/core/html/HTMLVideoElementPersistentTest.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/core/html/media/MediaDocument.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/core/input/GestureManager.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/core/input/KeyboardEventManager.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/core/input/PointerEventManager.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/core/inspector/DevToolsHost.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/core/inspector/ThreadDebugger.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/core/page/DragController.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/modules/accessibility/AXNodeObject.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/modules/accessibility/AXObject.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/modules/media_controls/MediaControlsOrientationLockDelegateTest.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegate.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/modules/media_controls/MediaControlsRotateToFullscreenDelegateTest.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/modules/mediasession/MediaSession.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/modules/notifications/Notification.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/modules/remoteplayback/RemotePlaybackTest.cpp
[modify] https://crrev.com/8f2e8f94d90f918a3766ee8e7ba0336d27b47e64/third_party/WebKit/Source/modules/webaudio/BaseAudioContextTest.cpp

Comment 4 by mustaq@chromium.org, Aug 16 2017

Blockedon: 756150
It seems that all public methods in UGT can be nuked w/o any change in existing functionalities, with one exception: UGT::HasGestures().  This method is used in Pepper, and it's not clear why a token even needs saving there (crbug.com/756150).  We plan to fix that through a slight change in functionality (in the rare case we have two overlapping UGIs).
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 17 2017 

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

commit a89466ce1d4f5aac646b16c0c66a50cdeb8f228d
Author: Mustaq Ahmed <mustaq@google.com>
Date: Thu Aug 17 18:18:56 2017

Hide most of the public interface of UserGestureToken.

This is a code-cleanup CL without any change in functionality, follows
the following two cleanup CLs:
  chromium-review.googlesource.com/c/610827
  chromium-review.googlesource.com/c/615471

This CL makes almost all public functions of UserGestureToken privte to
futher unify token usage calls. The only remaining public method is
UGT::HasGestures, will be handled separately (crbug.com/756150).

There could be a slight performance overhead during JS debugging:
MainThreadDebugger::runMessageLoopOnPause will now check IsMainThread()
redundantly. This is not a concern because V8Debugger overhead should be
way bigger.

Bug:  755160 
Change-Id: I3f03f0ed73bb99b62f43e1ffba88021a164a8003
TBR: rbyers@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/617941
Commit-Queue: Mustaq Ahmed <mustaq@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Bill Budge <bbudge@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495229}
[modify] https://crrev.com/a89466ce1d4f5aac646b16c0c66a50cdeb8f228d/content/renderer/pepper/pepper_plugin_instance_impl.cc
[modify] https://crrev.com/a89466ce1d4f5aac646b16c0c66a50cdeb8f228d/third_party/WebKit/Source/core/dom/UserGestureIndicator.cpp
[modify] https://crrev.com/a89466ce1d4f5aac646b16c0c66a50cdeb8f228d/third_party/WebKit/Source/core/dom/UserGestureIndicator.h
[modify] https://crrev.com/a89466ce1d4f5aac646b16c0c66a50cdeb8f228d/third_party/WebKit/Source/core/exported/WebUserGestureIndicator.cpp
[modify] https://crrev.com/a89466ce1d4f5aac646b16c0c66a50cdeb8f228d/third_party/WebKit/Source/core/exported/WebUserGestureToken.cpp
[modify] https://crrev.com/a89466ce1d4f5aac646b16c0c66a50cdeb8f228d/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp
[modify] https://crrev.com/a89466ce1d4f5aac646b16c0c66a50cdeb8f228d/third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp
[modify] https://crrev.com/a89466ce1d4f5aac646b16c0c66a50cdeb8f228d/third_party/WebKit/Source/core/loader/NavigationScheduler.cpp
[modify] https://crrev.com/a89466ce1d4f5aac646b16c0c66a50cdeb8f228d/third_party/WebKit/Source/core/page/ChromeClientImpl.cpp
[modify] https://crrev.com/a89466ce1d4f5aac646b16c0c66a50cdeb8f228d/third_party/WebKit/public/web/WebUserGestureIndicator.h
[modify] https://crrev.com/a89466ce1d4f5aac646b16c0c66a50cdeb8f228d/third_party/WebKit/public/web/WebUserGestureToken.h

Comment 6 by mustaq@chromium.org, Sep 12 2017

Blockedon: -756150

Comment 7 by mustaq@chromium.org, Sep 12 2017

Status: Fixed (was: Assigned)
Other changes I planned here would affect UGTs around multiple simultaneous gestures.  While the diff would be hardly visible, it's better to address them separately to keep this code-health bug isolated.

Comment 8 by mustaq@chromium.org, Nov 1 2017

Labels: UserActivation
►

Sign in to add a comment