Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2283013002:
Adding chrome flag to disable implicit capture (Closed)
|
Created:
4 years, 3 months ago by Navid Zolghadr Modified:
4 years, 3 months ago CC:
asvitkine+watch_chromium.org, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, dtapuska+blinkwatch_chromium.org, jam, kinuko+watch, nzolghadr+blinkwatch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding chrome flag to disable implicit capture
This change adds a flag to disable implicit
touch pointer events. So that when the touch
goes down it does not gets captured to that
element. Note that it doesn't change the touch
events and it only affects the pointer events.
BUG=640700
Committed: https://crrev.com/8d8e3857dc1ad98b96493297bf3a95fa838eeb17
Cr-Commit-Position: refs/heads/master@{#415950}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Rename the flag #
Total comments: 6
Patch Set 3 : Addressing comments about comments :) #
Messages
Total messages: 41 (21 generated)
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
nzolghadr@chromium.org changed reviewers: + mustaq@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
dtapuska@chromium.org changed reviewers: + dtapuska@chromium.org
https://codereview.chromium.org/2283013002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2283013002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:5938: + <message name="IDS_FLAGS_EXPERIMENTAL_POINTER_EVENT_NO_IMPLICIT_CAPTURE_NAME" desc="Name for the flag to enable no implicit capture for the touch pointer events."> I don't think we need to add it to about:flags. Can't we just explicitly use the command line option to launch this? ie; we don't really want it discoverable by end users do we?
On 2016/08/29 14:50:48, dtapuska wrote: > https://codereview.chromium.org/2283013002/diff/1/chrome/app/generated_resour... > File chrome/app/generated_resources.grd (right): > > https://codereview.chromium.org/2283013002/diff/1/chrome/app/generated_resour... > chrome/app/generated_resources.grd:5938: + <message > name="IDS_FLAGS_EXPERIMENTAL_POINTER_EVENT_NO_IMPLICIT_CAPTURE_NAME" desc="Name > for the flag to enable no implicit capture for the touch pointer events."> > I don't think we need to add it to about:flags. Can't we just explicitly use the > command line option to launch this? > > ie; we don't really want it discoverable by end users do we? I believe we agreed on doing it to make it easier for us to flip the flag on and off on Android and Chrome OS.
lgtm
lgtm mod s/enable-no/disable/ Have we decided if we want two separate flags or one. I have no preference either way, but we we choose a single flag, the flag name should cover both cases. https://codereview.chromium.org/2283013002/diff/1/tools/metrics/actions/actio... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2283013002/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:207: Touch pointer events will not be captured implicitly "Disables implicit capturing of touch pointer events." https://codereview.chromium.org/2283013002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2283013002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:83771: + label="enable-no-implicit-touch-pointer-event-capture"/> Isn't "disable-implicit-..." better than "enable-no-implicit-..."? Double negatives are confusing in general.
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
Some drive-bys https://codereview.chromium.org/2283013002/diff/1/content/public/common/conte... File content/public/common/content_features.cc (right): https://codereview.chromium.org/2283013002/diff/1/content/public/common/conte... content/public/common/content_features.cc:111: // Pointer events no implicit capture Full sentence please. https://codereview.chromium.org/2283013002/diff/1/content/public/common/conte... content/public/common/content_features.cc:113: "PointerEventNoImplicitCapture", base::FEATURE_DISABLED_BY_DEFAULT}; Would it make more sense to have the feature be "ImplicitCapture" and be enabled by default? https://codereview.chromium.org/2283013002/diff/1/tools/metrics/actions/actio... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2283013002/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:204: <action name="AboutFlags_enable-no-implicit-touch-pointer-event-capture"> Don't add this - these are being replaced by a histogram. See: https://codereview.chromium.org/2257533005/ https://codereview.chromium.org/2283013002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2283013002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:83771: + label="enable-no-implicit-touch-pointer-event-capture"/> On 2016/08/30 18:38:56, mustaq wrote: > Isn't "disable-implicit-..." better than "enable-no-implicit-..."? Double > negatives are confusing in general. Since this is adding a base::Feature, I don't think you need an entry here.
https://codereview.chromium.org/2283013002/diff/1/content/public/common/conte... File content/public/common/content_features.cc (right): https://codereview.chromium.org/2283013002/diff/1/content/public/common/conte... content/public/common/content_features.cc:113: "PointerEventNoImplicitCapture", base::FEATURE_DISABLED_BY_DEFAULT}; On 2016/08/30 19:24:58, Alexei Svitkine (slow) wrote: > Would it make more sense to have the feature be "ImplicitCapture" and be enabled > by default? I'm going to change the name to kPointerEventV1SpecCapturing to avoid the confusion. https://codereview.chromium.org/2283013002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2283013002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:83771: + label="enable-no-implicit-touch-pointer-event-capture"/> On 2016/08/30 19:24:58, Alexei Svitkine (slow) wrote: > On 2016/08/30 18:38:56, mustaq wrote: > > Isn't "disable-implicit-..." better than "enable-no-implicit-..."? Double > > negatives are confusing in general. > > Since this is adding a base::Feature, I don't think you need an entry here. There is a comment at the end in about_flags.cc that I need to add it here as well. Doesn't that comment apply anymore?
https://codereview.chromium.org/2283013002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2283013002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:83771: + label="enable-no-implicit-touch-pointer-event-capture"/> On 2016/08/30 19:44:50, Navid Zolghadr wrote: > On 2016/08/30 19:24:58, Alexei Svitkine (slow) wrote: > > On 2016/08/30 18:38:56, mustaq wrote: > > > Isn't "disable-implicit-..." better than "enable-no-implicit-..."? Double > > > negatives are confusing in general. > > > > Since this is adding a base::Feature, I don't think you need an entry here. > > There is a comment at the end in about_flags.cc that I need to add it here as > well. Doesn't that comment apply anymore? It actually doesn't apply to base::Feature entries in about_flags.cc. It should probably be clarified.
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2283013002/diff/1/content/public/common/conte... File content/public/common/content_features.cc (right): https://codereview.chromium.org/2283013002/diff/1/content/public/common/conte... content/public/common/content_features.cc:111: // Pointer events no implicit capture On 2016/08/30 19:24:58, Alexei Svitkine (slow) wrote: > Full sentence please. Done. https://codereview.chromium.org/2283013002/diff/1/tools/metrics/actions/actio... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/2283013002/diff/1/tools/metrics/actions/actio... tools/metrics/actions/actions.xml:204: <action name="AboutFlags_enable-no-implicit-touch-pointer-event-capture"> On 2016/08/30 19:24:58, Alexei Svitkine (slow) wrote: > Don't add this - these are being replaced by a histogram. See: > https://codereview.chromium.org/2257533005/ Done. https://codereview.chromium.org/2283013002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2283013002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:83771: + label="enable-no-implicit-touch-pointer-event-capture"/> On 2016/08/30 20:04:07, Alexei Svitkine (slow) wrote: > On 2016/08/30 19:44:50, Navid Zolghadr wrote: > > On 2016/08/30 19:24:58, Alexei Svitkine (slow) wrote: > > > On 2016/08/30 18:38:56, mustaq wrote: > > > > Isn't "disable-implicit-..." better than "enable-no-implicit-..."? Double > > > > negatives are confusing in general. > > > > > > Since this is adding a base::Feature, I don't think you need an entry here. > > > > There is a comment at the end in about_flags.cc that I need to add it here as > > well. Doesn't that comment apply anymore? > > It actually doesn't apply to base::Feature entries in about_flags.cc. It should > probably be clarified. Done.
nzolghadr@chromium.org changed reviewers: + bokan@chromium.org, creis@chromium.org, rbyers@chromium.org, vollick@chromium.org
vollick@chromium.org: Please review changes in third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in bokan@chromium.org: Please review changes in third_party/WebKit/Source/core/input/PointerEventManager.cpp third_party/WebKit/Source/web/WebRuntimeFeatures.cpp rbyers@chromium.org: Please review changes in third_party/WebKit/public/web/WebRuntimeFeatures.h creis@chromium.org: Please review changes in content/*
LGTM % comment https://codereview.chromium.org/2283013002/diff/20001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2283013002/diff/20001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2079: {"enable-pointer-event-v1-spec-capturing-behavior", // FLAGS:RECORD_UMA Remove the FLAGS::RECORD_UMA comment. That's no longer a thing.
https://codereview.chromium.org/2283013002/diff/20001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2283013002/diff/20001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2079: {"enable-pointer-event-v1-spec-capturing-behavior", // FLAGS:RECORD_UMA On 2016/08/31 15:38:03, Alexei Svitkine (slow) wrote: > Remove the FLAGS::RECORD_UMA comment. That's no longer a thing. asvitkine@: Does this mean it's time to nuke the whole how-to block starting at Line 646 above? And all other FLAGS::RECORD_UMA's?
Source/platform LGTM with nit https://codereview.chromium.org/2283013002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2283013002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:164: PointerEventV1SpecCapturing nit: add a short comment such as: // For temporary compat testing of Edge-like model - crbug.com/640700
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2283013002/diff/20001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2283013002/diff/20001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2079: {"enable-pointer-event-v1-spec-capturing-behavior", // FLAGS:RECORD_UMA On 2016/08/31 15:50:47, mustaq wrote: > On 2016/08/31 15:38:03, Alexei Svitkine (slow) wrote: > > Remove the FLAGS::RECORD_UMA comment. That's no longer a thing. > > asvitkine@: Does this mean it's time to nuke the whole how-to block starting at > Line 646 above? And all other FLAGS::RECORD_UMA's? Yep, and it's already been done, see: https://codereview.chromium.org/2257533005/
https://codereview.chromium.org/2283013002/diff/20001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2283013002/diff/20001/chrome/browser/about_fl... chrome/browser/about_flags.cc:2079: {"enable-pointer-event-v1-spec-capturing-behavior", // FLAGS:RECORD_UMA On 2016/08/31 15:50:47, mustaq wrote: > On 2016/08/31 15:38:03, Alexei Svitkine (slow) wrote: > > Remove the FLAGS::RECORD_UMA comment. That's no longer a thing. > > asvitkine@: Does this mean it's time to nuke the whole how-to block starting at > Line 646 above? And all other FLAGS::RECORD_UMA's? Done. https://codereview.chromium.org/2283013002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2283013002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:164: PointerEventV1SpecCapturing On 2016/08/31 16:18:08, Rick Byers wrote: > nit: add a short comment such as: > // For temporary compat testing of Edge-like model - crbug.com/640700 Done.
RS LGTM for content/.
The CQ bit was unchecked by nzolghadr@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by nzolghadr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mustaq@chromium.org, dtapuska@chromium.org, rbyers@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2283013002/#ps40001 (title: "Addressing comments about comments :)")
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
third_party/WebKit/Source/core/input/PointerEventManager.cpp third_party/WebKit/Source/web/WebRuntimeFeatures.cpp lgtm
The CQ bit was checked by nzolghadr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Adding chrome flag to disable implicit capture This change adds a flag to disable implicit touch pointer events. So that when the touch goes down it does not gets captured to that element. Note that it doesn't change the touch events and it only affects the pointer events. BUG=640700 ========== to ========== Adding chrome flag to disable implicit capture This change adds a flag to disable implicit touch pointer events. So that when the touch goes down it does not gets captured to that element. Note that it doesn't change the touch events and it only affects the pointer events. BUG=640700 Committed: https://crrev.com/8d8e3857dc1ad98b96493297bf3a95fa838eeb17 Cr-Commit-Position: refs/heads/master@{#415950} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8d8e3857dc1ad98b96493297bf3a95fa838eeb17 Cr-Commit-Position: refs/heads/master@{#415950} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
