Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2531413002:
Reset the default value of touch event flag back to "Auto". (Closed)
DescriptionReset the default value of touch event flag back to "Auto".
This is a partial revert of issue 2467913002.
https://codereview.chromium.org/2467913002
In the previous patch, we set the default value of touch-event-api as enabled.
However, this may cause troubles due to http://crbug.com/392584. Basically
there are still a bunch of websites on which mouse support is broken when the
TouchEvents API is present. So we need to retain the "auto" behavior as the
default. We also keep firing DOM events as before even when the flag says
disabled.
BUG=644318, 669386, 669132
Committed: https://crrev.com/78171bf829d1ae47b7d7237c08878783a2cdadb5
Cr-Commit-Position: refs/heads/master@{#435821}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Pass the tests. #Patch Set 3 : Typo #
Total comments: 4
Patch Set 4 : Nit #
Total comments: 1
Patch Set 5 : Merge with origin/master #Patch Set 6 : Keep firing dom events. #
Total comments: 2
Patch Set 7 : Wording. #
Messages
Total messages: 68 (49 generated)
Description was changed from ========== Reset the default value of touch event flag to "Auto" BUG=644318 ========== to ========== Reset the default value of touch event flag to "Auto". This is a partial revert of issue 2467913002. https://codereview.chromium.org/2467913002 In the previous patch, we set the default value of touche-event-api as enabled. However, this may cause troubles due to http://crbug.com/392584. Basically there are still a bunch of websites on which mouse support is broken when the TouchEvents API is present. So we need to retain the "auto" behavior as the default. BUG=644318 ==========
sunyunjia@chromium.org changed reviewers: + mustaq@chromium.org
PTAL, thanks!
The CQ bit was checked by sunyunjia@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...
A few nits: CL description: s/touche/touch/. https://codereview.chromium.org/2531413002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2531413002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:5497: + Force touchscreen support to always be enabled or disabled, or to be enabled when a touchscreen is detected on startup (Automatic, the default). Force Touch Events API to always be enabled or disabled... https://codereview.chromium.org/2531413002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2531413002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:66683: Tracks whether touch events are enabled. This is based on the touch events Tracks whether TouchEvents API is enabled. This is based on ...
Description was changed from ========== Reset the default value of touch event flag to "Auto". This is a partial revert of issue 2467913002. https://codereview.chromium.org/2467913002 In the previous patch, we set the default value of touche-event-api as enabled. However, this may cause troubles due to http://crbug.com/392584. Basically there are still a bunch of websites on which mouse support is broken when the TouchEvents API is present. So we need to retain the "auto" behavior as the default. BUG=644318 ========== to ========== Reset the default value of touch event flag back to "Auto". This is a partial revert of issue 2467913002. https://codereview.chromium.org/2467913002 In the previous patch, we set the default value of touche-event-api as enabled. However, this may cause troubles due to http://crbug.com/392584. Basically there are still a bunch of websites on which mouse support is broken when the TouchEvents API is present. So we need to retain the "auto" behavior as the default. BUG=644318 ==========
Description was changed from ========== Reset the default value of touch event flag back to "Auto". This is a partial revert of issue 2467913002. https://codereview.chromium.org/2467913002 In the previous patch, we set the default value of touche-event-api as enabled. However, this may cause troubles due to http://crbug.com/392584. Basically there are still a bunch of websites on which mouse support is broken when the TouchEvents API is present. So we need to retain the "auto" behavior as the default. BUG=644318 ========== to ========== Reset the default value of touch event flag back to "Auto". This is a partial revert of issue 2467913002. https://codereview.chromium.org/2467913002 In the previous patch, we set the default value of touch-event-api as enabled. However, this may cause troubles due to http://crbug.com/392584. Basically there are still a bunch of websites on which mouse support is broken when the TouchEvents API is present. So we need to retain the "auto" behavior as the default. BUG=644318 ==========
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
PTAL, thanks! https://codereview.chromium.org/2531413002/diff/1/chrome/app/generated_resour... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2531413002/diff/1/chrome/app/generated_resour... chrome/app/generated_resources.grd:5497: + Force touchscreen support to always be enabled or disabled, or to be enabled when a touchscreen is detected on startup (Automatic, the default). On 2016/11/28 18:47:47, mustaq wrote: > Force Touch Events API to always be enabled or disabled... Done. https://codereview.chromium.org/2531413002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2531413002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:66683: Tracks whether touch events are enabled. This is based on the touch events On 2016/11/28 18:47:47, mustaq wrote: > Tracks whether TouchEvents API is enabled. This is based on ... Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sunyunjia@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
sunyunjia@chromium.org changed reviewers: + avi@chromium.org, sadrul@chromium.org
Description was changed from ========== Reset the default value of touch event flag back to "Auto". This is a partial revert of issue 2467913002. https://codereview.chromium.org/2467913002 In the previous patch, we set the default value of touch-event-api as enabled. However, this may cause troubles due to http://crbug.com/392584. Basically there are still a bunch of websites on which mouse support is broken when the TouchEvents API is present. So we need to retain the "auto" behavior as the default. BUG=644318 ========== to ========== Reset the default value of touch event flag back to "Auto". This is a partial revert of issue 2467913002. https://codereview.chromium.org/2467913002 In the previous patch, we set the default value of touch-event-api as enabled. However, this may cause troubles due to http://crbug.com/392584. Basically there are still a bunch of websites on which mouse support is broken when the TouchEvents API is present. So we need to retain the "auto" behavior as the default. BUG=644318, 669472, 669386, 669132 ==========
sunyunjia@chromium.org changed reviewers: + raymes@chromium.org, rkaplow@chromium.org
PTAL, thanks!
https://codereview.chromium.org/2531413002/diff/50001/chrome/test/ppapi/ppapi... File chrome/test/ppapi/ppapi_test.cc (right): https://codereview.chromium.org/2531413002/diff/50001/chrome/test/ppapi/ppapi... chrome/test/ppapi/ppapi_test.cc:148: // Enable touche events api. nit: touche->touch https://codereview.chromium.org/2531413002/diff/50001/chrome/test/ppapi/ppapi... chrome/test/ppapi/ppapi_test.cc:149: command_line->AppendSwitch(switches::kTouchEvents); Hmm, how come this is needed?
https://codereview.chromium.org/2531413002/diff/50001/chrome/test/ppapi/ppapi... File chrome/test/ppapi/ppapi_test.cc (right): https://codereview.chromium.org/2531413002/diff/50001/chrome/test/ppapi/ppapi... chrome/test/ppapi/ppapi_test.cc:148: // Enable touche events api. On 2016/11/30 02:14:41, raymes wrote: > nit: touche->touch Done. https://codereview.chromium.org/2531413002/diff/50001/chrome/test/ppapi/ppapi... chrome/test/ppapi/ppapi_test.cc:149: command_line->AppendSwitch(switches::kTouchEvents); On 2016/11/30 02:14:41, raymes wrote: > Hmm, how come this is needed? In the previous patch(https://codereview.chromium.org/2467913002), we prevent the touch-actions in TouchEventManager based on the touch-events flag https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/input/Tou.... In that patch, we set the default value of the flag as "enabled" so there's no problem. In this patch, we reset the default value to "auto" depending on whether there's a touchscreen, which is false in the test, and the test fails because of that. So we want to manually set it as "enabled" to pass the test.
The CQ bit was checked by sunyunjia@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...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by sunyunjia@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Reset the default value of touch event flag back to "Auto". This is a partial revert of issue 2467913002. https://codereview.chromium.org/2467913002 In the previous patch, we set the default value of touch-event-api as enabled. However, this may cause troubles due to http://crbug.com/392584. Basically there are still a bunch of websites on which mouse support is broken when the TouchEvents API is present. So we need to retain the "auto" behavior as the default. BUG=644318, 669472, 669386, 669132 ========== to ========== Reset the default value of touch event flag back to "Auto". This is a partial revert of issue 2467913002. https://codereview.chromium.org/2467913002 In the previous patch, we set the default value of touch-event-api as enabled. However, this may cause troubles due to http://crbug.com/392584. Basically there are still a bunch of websites on which mouse support is broken when the TouchEvents API is present. So we need to retain the "auto" behavior as the default. BUG=644318, 669386, 669132 ==========
lgtm
\ https://codereview.chromium.org/2531413002/diff/70001/content/public/test/con... File content/public/test/content_browser_test.cc (right): https://codereview.chromium.org/2531413002/diff/70001/content/public/test/con... content/public/test/content_browser_test.cc:61: command_line->AppendSwitch(switches::kTouchEvents); This affects all tests. Are you sure you want to set it here? Should you instead set it for the tests that depend on it? (see the various overrides of SetUpCommandLine() that add custom switches)
lgtm
Patchset #5 (id:90001) has been deleted
After an offline discussion today with Rick, we decided to keep firing DOM events as before even when the flag says disabled. In that case, we shouldn't need any test related changes here. Please leave a comment in TouchEventManager: 'We ideally want to suppress TouchEvent firing when touch detection is disabled but we need to support dev-tools emulation and "some" dynamic cases.'
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
The CQ bit was unchecked by sunyunjia@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by sunyunjia@chromium.org
Patchset #6 (id:130001) has been deleted
The CQ bit was checked by sunyunjia@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...
Description was changed from ========== Reset the default value of touch event flag back to "Auto". This is a partial revert of issue 2467913002. https://codereview.chromium.org/2467913002 In the previous patch, we set the default value of touch-event-api as enabled. However, this may cause troubles due to http://crbug.com/392584. Basically there are still a bunch of websites on which mouse support is broken when the TouchEvents API is present. So we need to retain the "auto" behavior as the default. BUG=644318, 669386, 669132 ========== to ========== Reset the default value of touch event flag back to "Auto". This is a partial revert of issue 2467913002. https://codereview.chromium.org/2467913002 In the previous patch, we set the default value of touch-event-api as enabled. However, this may cause troubles due to http://crbug.com/392584. Basically there are still a bunch of websites on which mouse support is broken when the TouchEvents API is present. So we need to retain the "auto" behavior as the default. We also keep firing DOM events as before even when the flag says disabled. BUG=644318, 669386, 669132 ==========
sunyunjia@chromium.org changed reviewers: + rbyers@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with minor wording nits https://codereview.chromium.org/2531413002/diff/150001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2531413002/diff/150001/chrome/app/generated_r... chrome/app/generated_resources.grd:5497: + Force Touch Events API to always be enabled or disabled, or to be enabled when a touchscreen is detected on startup (Automatic, the default). Nit: for clarity how about: "Force Touch Events API feature detection to always be ..."? https://codereview.chromium.org/2531413002/diff/150001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/2531413002/diff/150001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/TouchEventManager.cpp:492: // We ideally want to suppress TouchEvent firing when touch detection is FWIW I don't agree with this comment. We'd "ideally" send touch events all the time AND enable TouchEvent feature detection all the time. But the latter has stupid web compat problems, so we can't do it. But IMHO there's no good reason not to do the former. Perhaps we should rename (again, sorry) the feature from "touchEventAPIEnabled" to "touchEventFeatureDetectionEnabled"? Or for now just add a comment to RuntimeEnabledFeatures.in saying that's really what it means (that "touch events" themselves are always enabled since they're a feature always supported by Chrome).
The CQ bit was checked by sunyunjia@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...
sunyunjia@chromium.org changed reviewers: + tkent@chromium.org
Hi tkent@: could you please review the change in blink? Thanks!
On 2016/12/01 at 22:43:39, sunyunjia wrote: > Hi tkent@: could you please review the change in blink? Thanks! lgtm
lgtm
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 sunyunjia@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mustaq@chromium.org, avi@chromium.org, rkaplow@chromium.org, raymes@chromium.org, rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/2531413002/#ps170001 (title: "Wording.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 170001, "attempt_start_ts": 1480643951597750,
"parent_rev": "cba9124348fa282716c4736754e5fd586eaad371", "commit_rev":
"bb642a631687a42b7cc3368bc2e8e4a597fdd120"}
Message was sent while issue was closed.
Description was changed from ========== Reset the default value of touch event flag back to "Auto". This is a partial revert of issue 2467913002. https://codereview.chromium.org/2467913002 In the previous patch, we set the default value of touch-event-api as enabled. However, this may cause troubles due to http://crbug.com/392584. Basically there are still a bunch of websites on which mouse support is broken when the TouchEvents API is present. So we need to retain the "auto" behavior as the default. We also keep firing DOM events as before even when the flag says disabled. BUG=644318, 669386, 669132 ========== to ========== Reset the default value of touch event flag back to "Auto". This is a partial revert of issue 2467913002. https://codereview.chromium.org/2467913002 In the previous patch, we set the default value of touch-event-api as enabled. However, this may cause troubles due to http://crbug.com/392584. Basically there are still a bunch of websites on which mouse support is broken when the TouchEvents API is present. So we need to retain the "auto" behavior as the default. We also keep firing DOM events as before even when the flag says disabled. BUG=644318, 669386, 669132 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:170001)
Message was sent while issue was closed.
Description was changed from ========== Reset the default value of touch event flag back to "Auto". This is a partial revert of issue 2467913002. https://codereview.chromium.org/2467913002 In the previous patch, we set the default value of touch-event-api as enabled. However, this may cause troubles due to http://crbug.com/392584. Basically there are still a bunch of websites on which mouse support is broken when the TouchEvents API is present. So we need to retain the "auto" behavior as the default. We also keep firing DOM events as before even when the flag says disabled. BUG=644318, 669386, 669132 ========== to ========== Reset the default value of touch event flag back to "Auto". This is a partial revert of issue 2467913002. https://codereview.chromium.org/2467913002 In the previous patch, we set the default value of touch-event-api as enabled. However, this may cause troubles due to http://crbug.com/392584. Basically there are still a bunch of websites on which mouse support is broken when the TouchEvents API is present. So we need to retain the "auto" behavior as the default. We also keep firing DOM events as before even when the flag says disabled. BUG=644318, 669386, 669132 Committed: https://crrev.com/78171bf829d1ae47b7d7237c08878783a2cdadb5 Cr-Commit-Position: refs/heads/master@{#435821} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/78171bf829d1ae47b7d7237c08878783a2cdadb5 Cr-Commit-Position: refs/heads/master@{#435821} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
