Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2467913002:
Touch event flag should control only DOM event firing. (Closed)
|
Created:
4 years, 1 month ago by sunyunjia Modified:
4 years ago Reviewers:
dtapuska, Avi (use Gerrit), rkaplow, tkent, bokan, dtu, dcheng, aiolos (Not reviewing), Rick Byers, nednguyen, sadrul, mustaq CC:
apavlov+blink_chromium.org, blink-layers+watch_chromium.org, blink-reviews, blink-reviews-api_chromium.org, blink-reviews-dom_chromium.org, blink-reviews-events_chromium.org, blink-reviews-layout_chromium.org, caseq+blink_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, devtools-reviews_chromium.org, dglazkov+blink, dtapuska+blinkwatch_chromium.org, eae+blinkwatch, jam, jchaffraix+rendering, kenneth.christiansen, kinuko+watch, kozyatinskiy+blink_chromium.org, leviw+renderwatch, lushnikov+blink_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, pdr+renderingwatchlist_chromium.org, pfeldman+blink_chromium.org, rwlbuis, sof, szager+layoutwatch_chromium.org, tfarina, zoltan1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTouch event flag should control only DOM event firing.
Currently there is no way to disable TouchEvent API support
without affecting either PointerEvents (for touches) or
touch support in browser UI. With PointerEvents shipping in
M55, it now makes the most sense to turn on touch support in
browser unconditionally and let the --touch-events flag
control only the firing of DOM events.
This CL also renames the flag to clarify its purpose.
BUG=644318
Committed: https://crrev.com/7a880dd75bf766d22cbe4ba79bb168f7ea38c524
Cr-Commit-Position: refs/heads/master@{#434399}
Patch Set 1 #Patch Set 2 : Finished #Patch Set 3 : Complete #Patch Set 4 : Change the string in "about://flags" #
Total comments: 5
Patch Set 5 : Change touch_enabled.cc #Patch Set 6 : Format #Patch Set 7 : windows side #Patch Set 8 : Disable the hackernews test and change device_supports_touch. #Patch Set 9 : Change the names and file a bug. #Patch Set 10 : Change more names. #Patch Set 11 : Merge branch 'master' of https://chromium.googlesource.com/chromium/src into patchtoflags #
Total comments: 10
Patch Set 12 : Bring the auto back. #
Total comments: 8
Patch Set 13 : nits #
Total comments: 3
Patch Set 14 : Change !(A && B) to !A || !B #
Total comments: 1
Patch Set 15 : Cleaner and clearer code. #
Total comments: 2
Patch Set 16 : more updates. #Patch Set 17 : More updates. #Patch Set 18 : Merge branch 'master' of https://chromium.googlesource.com/chromium/src into flags1 #
Total comments: 2
Patch Set 19 : Update the description in histograms.xml #Patch Set 20 : Deprecate the histogram. #
Total comments: 1
Messages
Total messages: 218 (167 generated)
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: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
sunyunjia@chromium.org changed reviewers: + mustaq@chromium.org
Hi Mustaq, Here is the patch I'm working. I'm trying to fix the broken tests for now.
Thanks Sandra for going through the setting-flow, this looks close. There are two kinds of failures now: plugins and some browser tests. I would start with content_browertests in Linux. Since the only non-Windows functional change is the one in TouchEventManager, try the different value I suggested there, it should work. Here is how to run one failing test only: ninja -C out/Default content_browsertests ./out/Default/content_browsertests --gtest_filter=MainThreadEventQueueBrowserTest.TouchMove --single-process --no-renderer-sandbox https://codereview.chromium.org/2467913002/diff/60001/chrome/app/generated_re... File chrome/app/generated_resources.grd (left): https://codereview.chromium.org/2467913002/diff/60001/chrome/app/generated_re... chrome/app/generated_resources.grd:5445: Force touchscreen support to always be enabled or disabled, or to be enabled when a touchscreen is detected on startup (Automatic, the default). Enables support for the Touch Events API. https://codereview.chromium.org/2467913002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/2467913002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_view_host_impl.cc:499: prefs.device_supports_touch = prefs.touch_api_enabled && I think the removing touch_api_enabled condition here makes more sense. Please check all uses of device_supports_touch to confirm it. https://codereview.chromium.org/2467913002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/2467913002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:492: return WebInputEventResult::HandledSystem; Should be NotHandled instead, to let the upstream event handlers know that the page didn't do anything with the event. Could be HandledSuppressed, depends on how the return value is used. https://codereview.chromium.org/2467913002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2467913002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:232: TouchAPI status=stable Let's rename it to "TouchEventAPI" instead. Looks like we don't to use the term "API" in most other cases here. But we will still use it to avoid confusion. https://codereview.chromium.org/2467913002/diff/60001/ui/base/touch/touch_ena... File ui/base/touch/touch_enabled.cc (left): https://codereview.chromium.org/2467913002/diff/60001/ui/base/touch/touch_ena... ui/base/touch/touch_enabled.cc:53: return GetTouchScreensAvailability() == TouchScreensAvailability::ENABLED; IsTouchAPIEnabled() shouldn't depend on touch screen availability. I guess considering AUTO being enabled should work.
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: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_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: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Patchset #7 (id:120001) has been deleted
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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...
Patchset #7 (id:140001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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 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.
Patchset #8 (id:180001) has been deleted
Patchset #7 (id:160001) has been deleted
Patchset #6 (id:100001) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Patchset #7 (id:220001) has been deleted
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Here are a few more comments. Please address all comments, also disable the hackernews telemetry tests after filing a bug as we discussed offline. https://codereview.chromium.org/2467913002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2467913002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:236: TouchAPI status=stable As commented before, please rename this to TouchEventAPI. https://codereview.chromium.org/2467913002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/DevToolsEmulator.cpp (right): https://codereview.chromium.org/2467913002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/web/DevToolsEmulator.cpp:449: m_originalTouchEnabled = RuntimeEnabledFeatures::touchAPIEnabled(); Also rename this state var to m_originalTouchAPIEnabled.
Description was changed from ========== Change the flag "Enable touch events" to "Enable touch api" and control its behavior from blink side. BUG=644318 ========== to ========== Touch event flag should control only DOM event firing. Currently there is no way to disable TouchEvent API support without affecting either PointerEvents (for touches) or touch support in browser UI. With PointerEvents shipping in M55, it now makes the most sense to turn on touch support in browser unconditionally and let the --touch-events flag control only the firing of DOM events. This CL also renames the flag to clarify its purpose. BUG=644318 ==========
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 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...
Patchset #8 (id:260001) has been deleted
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 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...
Hi Mustaq, Please take a look at this patch, and let me know if it's time to send it to other reviewers. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
Patchset #8 (id:280001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Patchset #10 (id:340001) has been deleted
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
Patchset #10 (id:360001) has been deleted
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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.
Here are my initial comments: https://codereview.chromium.org/2467913002/diff/400001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2467913002/diff/400001/chrome/app/generated_r... chrome/app/generated_resources.grd:5481: + Support for the Touch Events API. The names & descriptions appear in about://flags. Please make the name more concise, like "Touch Events API". https://codereview.chromium.org/2467913002/diff/400001/chrome/app/generated_r... chrome/app/generated_resources.grd:5484: + Support for the Touch Events API. "Enables support for Touch Events API." https://codereview.chromium.org/2467913002/diff/400001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2467913002/diff/400001/chrome/browser/about_f... chrome/browser/about_flags.cc:854: {"touch-event-api", IDS_FLAGS_TOUCH_EVENTS_NAME, Leave it as "touch-events". The |internal_name| should never change: never shown to the user but stored in the prefs file. Missed this before, sorry. In general, we want to make the code more readable by changing unexposed identifiers as you did in your CL. But changing any exposed string is a bit risky. https://codereview.chromium.org/2467913002/diff/400001/chrome/browser/about_f... chrome/browser/about_flags.cc:856: SINGLE_DISABLE_VALUE_TYPE(switches::kDisableTouchEventAPI)}, The same reason as above: don't changed the exposed flags yet. We can always cleanup later on but let's focus on the real functional change here instead of changing the exposed things. https://codereview.chromium.org/2467913002/diff/400001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/2467913002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/TouchEventManager.cpp:490: return WebInputEventResult::HandledSystem; Please return ::NotHandled.
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: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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...
Thanks, this is almost done. Marked a few nits, should be quick & easy, will l-g-t-m after that. The failure in linux_android_rel_ng looks like a flake. I will rerun the test now. https://codereview.chromium.org/2467913002/diff/420001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2467913002/diff/420001/chrome/browser/about_f... chrome/browser/about_flags.cc:136: {IDS_GENERIC_EXPERIMENT_CHOICE_ENABLED, "", ""}, The formatting here seems different from all other entries. Please fix---you may try "git cl format". https://codereview.chromium.org/2467913002/diff/420001/content/public/common/... File content/public/common/web_preferences.h (right): https://codereview.chromium.org/2467913002/diff/420001/content/public/common/... content/public/common/web_preferences.h:15: #include "ui/base/touch/touch_device.h" Please remove. https://codereview.chromium.org/2467913002/diff/420001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/DevToolsEmulator.cpp (right): https://codereview.chromium.org/2467913002/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/web/DevToolsEmulator.cpp:449: m_originalTouchEnabled = RuntimeEnabledFeatures::touchEventAPIEnabled(); Please rename DevToolsEmulator::m_originalTouchEnabled to m_originalTouchEventAPIEnabled. https://codereview.chromium.org/2467913002/diff/420001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2467913002/diff/420001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/browsing_stories.py:137: @decorators.Disabled('win' ,'linux') # crbug.com/657665 #crbug.com/665007 # crbug.com/657665 for win, crbug.com/665007 for linux
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 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...
PTAL, thanks! https://codereview.chromium.org/2467913002/diff/400001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2467913002/diff/400001/chrome/app/generated_r... chrome/app/generated_resources.grd:5481: + Support for the Touch Events API. On 2016/11/15 20:33:58, mustaq wrote: > The names & descriptions appear in about://flags. Please make the name more > concise, like "Touch Events API". Done. https://codereview.chromium.org/2467913002/diff/400001/chrome/app/generated_r... chrome/app/generated_resources.grd:5484: + Support for the Touch Events API. On 2016/11/15 20:33:58, mustaq wrote: > "Enables support for Touch Events API." Done. https://codereview.chromium.org/2467913002/diff/400001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2467913002/diff/400001/chrome/browser/about_f... chrome/browser/about_flags.cc:854: {"touch-event-api", IDS_FLAGS_TOUCH_EVENTS_NAME, On 2016/11/15 20:33:58, mustaq wrote: > Leave it as "touch-events". The |internal_name| should never change: never shown > to the user but stored in the prefs file. Missed this before, sorry. > > In general, we want to make the code more readable by changing unexposed > identifiers as you did in your CL. But changing any exposed string is a bit > risky. Done. https://codereview.chromium.org/2467913002/diff/400001/chrome/browser/about_f... chrome/browser/about_flags.cc:856: SINGLE_DISABLE_VALUE_TYPE(switches::kDisableTouchEventAPI)}, On 2016/11/15 20:33:58, mustaq wrote: > The same reason as above: don't changed the exposed flags yet. We can always > cleanup later on but let's focus on the real functional change here instead of > changing the exposed things. Done. https://codereview.chromium.org/2467913002/diff/400001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/2467913002/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/TouchEventManager.cpp:490: return WebInputEventResult::HandledSystem; On 2016/11/15 20:33:58, mustaq wrote: > Please return ::NotHandled. The touch event will still be triggered if we return ::NotHandled https://codereview.chromium.org/2467913002/diff/420001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2467913002/diff/420001/chrome/browser/about_f... chrome/browser/about_flags.cc:136: {IDS_GENERIC_EXPERIMENT_CHOICE_ENABLED, "", ""}, On 2016/11/16 14:54:16, mustaq wrote: > The formatting here seems different from all other entries. Please fix---you may > try "git cl format". Done. https://codereview.chromium.org/2467913002/diff/420001/content/public/common/... File content/public/common/web_preferences.h (right): https://codereview.chromium.org/2467913002/diff/420001/content/public/common/... content/public/common/web_preferences.h:15: #include "ui/base/touch/touch_device.h" On 2016/11/16 14:54:16, mustaq wrote: > Please remove. This is actually needed. What we want to delete is "touch_enabled.h" https://codereview.chromium.org/2467913002/diff/420001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/DevToolsEmulator.cpp (right): https://codereview.chromium.org/2467913002/diff/420001/third_party/WebKit/Sou... third_party/WebKit/Source/web/DevToolsEmulator.cpp:449: m_originalTouchEnabled = RuntimeEnabledFeatures::touchEventAPIEnabled(); On 2016/11/16 14:54:16, mustaq wrote: > Please rename DevToolsEmulator::m_originalTouchEnabled to > m_originalTouchEventAPIEnabled. Done. https://codereview.chromium.org/2467913002/diff/420001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2467913002/diff/420001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/browsing_stories.py:137: @decorators.Disabled('win' ,'linux') # crbug.com/657665 #crbug.com/665007 On 2016/11/16 14:54:16, mustaq wrote: > # crbug.com/657665 for win, crbug.com/665007 for linux Done.
LGTM, thanks.
On 2016/11/16 16:57:52, mustaq wrote: > LGTM, thanks. lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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: + avi@chromium.org, bokan@chromium.org, dcheng@chromium.org, dtu@chromium.org, tkent@chromium.org
PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
third_party/WebKit lgtm
https://codereview.chromium.org/2467913002/diff/440001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/2467913002/diff/440001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:459: switches::kTouchEventsDisabled); I'm a bit confused why the pref is called "enabled", but the RHS checks equality of the switch value with "disabled". https://codereview.chromium.org/2467913002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2467913002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:100: DCHECK(!m_page); (In the future, it'd be nice to split clean ups like this into a separate CL)
Still LGTM. https://codereview.chromium.org/2467913002/diff/440001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/2467913002/diff/440001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:459: switches::kTouchEventsDisabled); On 2016/11/17 11:25:11, dcheng wrote: > I'm a bit confused why the pref is called "enabled", but the RHS checks equality > of the switch value with "disabled". I also got a bit confused about it on my first reading, it was not obvious to me that the NOT is for the whole RHS. May be replacing !(A && B) with !A || !B makes it cleaner?
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: 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_...)
Thanks for clarifying: I definitely missed there grouping. https://codereview.chromium.org/2467913002/diff/460001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/2467913002/diff/460001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:459: switches::kTouchEventsDisabled); Perhaps just write this as !=
Also, lgtm
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: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
mustaq@chromium.org changed reviewers: + sadrul@chromium.org
sadrul@: Need approval in ui/*
ui lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
sunyunjia@chromium.org changed reviewers: + aiolos@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
aiolos@: Could you please review the change in tools/perf? Thanks!
https://codereview.chromium.org/2467913002/diff/480001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2467913002/diff/480001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/browsing_stories.py:139: # crbug.com/657665 for win, #crbug.com/665007 for linux The change is fine. But I'm confused as to why it's included in this cl since the rest of the changes seem completely unrelated to the bug in this comment. Could you break it out into a separate cl?
aiolos@chromium.org changed reviewers: + nednguyen@google.com
https://codereview.chromium.org/2467913002/diff/480001/tools/perf/page_sets/s... File tools/perf/page_sets/system_health/browsing_stories.py (right): https://codereview.chromium.org/2467913002/diff/480001/tools/perf/page_sets/s... tools/perf/page_sets/system_health/browsing_stories.py:139: # crbug.com/657665 for win, #crbug.com/665007 for linux Talked to Ned offline, we should understand why the page is failing on this test before either removing he page or fixing the issue. I'll defer to his opinion on this one.
On 2016/11/17 22:54:07, aiolos wrote: > https://codereview.chromium.org/2467913002/diff/480001/tools/perf/page_sets/s... > File tools/perf/page_sets/system_health/browsing_stories.py (right): > > https://codereview.chromium.org/2467913002/diff/480001/tools/perf/page_sets/s... > tools/perf/page_sets/system_health/browsing_stories.py:139: # crbug.com/657665 > for win, #crbug.com/665007 for linux > Talked to Ned offline, we should understand why the page is failing on this test > before either removing he page or fixing the issue. > > I'll defer to his opinion on this one. We should only disable the test if we are sure that the failure is not legit. Otherwise, this may mean the users will observe the crash once this change lands.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I think you should also update TouchFactoryX11 to make sure we follow the same behaviour on cros-x11/linux.
On 2016/11/17 22:59:43, nednguyen wrote: > On 2016/11/17 22:54:07, aiolos wrote: > > > https://codereview.chromium.org/2467913002/diff/480001/tools/perf/page_sets/s... > > File tools/perf/page_sets/system_health/browsing_stories.py (right): > > > > > https://codereview.chromium.org/2467913002/diff/480001/tools/perf/page_sets/s... > > tools/perf/page_sets/system_health/browsing_stories.py:139: # crbug.com/657665 > > for win, #crbug.com/665007 for linux > > Talked to Ned offline, we should understand why the page is failing on this > test > > before either removing he page or fixing the issue. > > > > I'll defer to his opinion on this one. > > We should only disable the test if we are sure that the failure is not legit. > Otherwise, this may mean the users will observe the crash once this change > lands. This CL simply exposed an existing test bug (crbug.com/657665) because we changed --touch-events=auto to mean "enabled" here. I inspected that stack trace in the test failure log, and it is obvious that the test is now (with this CL) failing for the exact same reason it has been failing in Windows (crbug.com/657665). I have added the details in crbug.com/665007#c4 and #c5. On our local testing, the test fails in Linux even on ToT when there is a touchscreen present. Sandra, please reconfirm this on ToT with & without the touchscreen. This is clearly test a bug: it implies that the touch cmdline parameter for telemetry is set to "auto" in Linux which is equivalent to "enabled" iff a touchscreen is attached. The test was already failing on Windows because I think the touch may be enabled by default there.
On 2016/11/18 15:50:31, sadrul wrote: > I think you should also update TouchFactoryX11 to make sure we follow the same > behaviour on cros-x11/linux. Sorry, sadrul@, my last reply crossed yours. I think TouchFactoryX11 correctly detects the presence of a touch-enabled device (that's why the telemetry test is failing with a touch screen, and Sandra will confirm it on ToT). This CL is confining the effect of the --touch-events flag to firing/exposing DOM TouchEvents. Everything else that originally depended on physical presence of a touch screen remains the same. In particular, see the change in render_view_host_impl.cc: pref.device_supports_touch now depends on hardware capabilities only and not on the flag. This looks cleaner and more logical to me. Any objection?
On 2016/11/18 16:05:32, mustaq wrote: > On 2016/11/18 15:50:31, sadrul wrote: > > I think you should also update TouchFactoryX11 to make sure we follow the same > > behaviour on cros-x11/linux. > > Sorry, sadrul@, my last reply crossed yours. I think TouchFactoryX11 correctly > detects the presence of a touch-enabled device (that's why the telemetry test > is failing with a touch screen, and Sandra will confirm it on ToT). > > This CL is confining the effect of the --touch-events flag to firing/exposing > DOM TouchEvents. Everything else that originally depended on physical presence > of a touch screen remains the same. In particular, see the change in > render_view_host_impl.cc: pref.device_supports_touch now depends on hardware > capabilities only and not on the flag. This looks cleaner and more logical to > me. Any objection? I may have misunderstood what this CL is doing. My understanding is: if --touch-events=disabled flag is used, then we want the touch events to still be processed normally in the browser UI. If that is the case, then I think we do need to remove the relevant code from TouchFactory that looks at the --touch-events flag (https://cs.chromium.org/chromium/src/ui/events/devices/x11/touch_factory_x11....
On 2016/11/18 16:13:54, sadrul wrote: > On 2016/11/18 16:05:32, mustaq wrote: > > On 2016/11/18 15:50:31, sadrul wrote: > > > I think you should also update TouchFactoryX11 to make sure we follow the > same > > > behaviour on cros-x11/linux. > > > > Sorry, sadrul@, my last reply crossed yours. I think TouchFactoryX11 correctly > > detects the presence of a touch-enabled device (that's why the telemetry test > > is failing with a touch screen, and Sandra will confirm it on ToT). > > > > This CL is confining the effect of the --touch-events flag to firing/exposing > > DOM TouchEvents. Everything else that originally depended on physical presence > > of a touch screen remains the same. In particular, see the change in > > render_view_host_impl.cc: pref.device_supports_touch now depends on hardware > > capabilities only and not on the flag. This looks cleaner and more logical to > > me. Any objection? > > I may have misunderstood what this CL is doing. My understanding is: if > --touch-events=disabled flag is used, then we want the touch events to still be > processed normally in the browser UI. If that is the case, then I think we do > need to remove the relevant code from TouchFactory that looks at the > --touch-events flag > (https://cs.chromium.org/chromium/src/ui/events/devices/x11/touch_factory_x11.... Yes, you are right that TouchFactory still incorrectly depends on the flag.
Also update chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc RecordTouchEventState() too.
In this hackernews test, chrome cannot detect whether there is a touch screen or not. So in original build, the flag is always disabled, and the test passes. And in my patch, the flag is always enabled, and the test fails. After some investigation, I figured that the following attributes enabled by the flag causes the test to fail. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Globa... // Touch Events // https://w3c.github.io/touch-events/#extensions-to-the-globaleventhandlers-int... [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchcancel; [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchend; [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchmove; [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchstart;
On 2016/11/21 15:53:16, sunyunjia wrote: > In this hackernews test, chrome cannot detect whether there is a touch screen or > not. So in original build, the flag is always disabled, and the test passes. And > in my patch, the flag is always enabled, and the test fails. > > After some investigation, I figured that the following attributes enabled by the > flag causes the test to fail. > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Globa... > > > > // Touch Events > // > https://w3c.github.io/touch-events/#extensions-to-the-globaleventhandlers-int... > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchcancel; > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchend; > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchmove; > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchstart; Thanks Sandra for confirming the exact failure conditions. The conditions I had in my mind were somewhat inverted but my main argument about the test is still valid: a. The test stalls at pages for which chrome runs fine. Both the hackernews page and the Nintendo page (https://developer.nintendo.com/ where the test stalls) runs w/o any issue on a build based on this patch. b. The existing test failure bug (crbug.com/657665) for Windows shows a failure with the exact same stack trace. I guess there could be some problem with the cached test page for Nintendo. nednguyen and aiolos, do you see any reason to believe that the test failure is specific to this CL?
Can you look into moving the kTouchEvents and relevant flags out of ui/events/ into content/, since that's where it is used? That doesn't need to happen in this CL though, can be done in a follow up CL. This CL lgtm
On 2016/11/21 17:04:04, mustaq wrote: > On 2016/11/21 15:53:16, sunyunjia wrote: > > In this hackernews test, chrome cannot detect whether there is a touch screen > or > > not. So in original build, the flag is always disabled, and the test passes. > And > > in my patch, the flag is always enabled, and the test fails. > > > > After some investigation, I figured that the following attributes enabled by > the > > flag causes the test to fail. > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Globa... > > > > > > > > // Touch Events > > // > > > https://w3c.github.io/touch-events/#extensions-to-the-globaleventhandlers-int... > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchcancel; > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchend; > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchmove; > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchstart; > > Thanks Sandra for confirming the exact failure conditions. The conditions I had > in my mind were somewhat inverted but my main argument about the test is still > valid: > > a. The test stalls at pages for which chrome runs fine. Both the hackernews > page and the Nintendo page (https://developer.nintendo.com/ where the test > stalls) runs w/o any issue on a build based on this patch. > > b. The existing test failure bug (crbug.com/657665) for Windows shows a failure > with the exact same stack trace. > > I guess there could be some problem with the cached test page for Nintendo. > > nednguyen and aiolos, do you see any reason to believe that the test failure > is specific to this CL? What is the Chrome stack trace produced by the failure in this CL? In crbug.com/657665, I don't see any chrome stack trace, just the telemetry python stack. If you believe the failure is unrelated & probably flake, you can undo you change to tools/perf/ folder & try landing this.
On 2016/11/21 19:17:35, nednguyen wrote: > On 2016/11/21 17:04:04, mustaq wrote: > > On 2016/11/21 15:53:16, sunyunjia wrote: > > > In this hackernews test, chrome cannot detect whether there is a touch > screen > > or > > > not. So in original build, the flag is always disabled, and the test passes. > > And > > > in my patch, the flag is always enabled, and the test fails. > > > > > > After some investigation, I figured that the following attributes enabled by > > the > > > flag causes the test to fail. > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Globa... > > > > > > > > > > > > // Touch Events > > > // > > > > > > https://w3c.github.io/touch-events/#extensions-to-the-globaleventhandlers-int... > > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchcancel; > > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchend; > > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchmove; > > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchstart; > > > > Thanks Sandra for confirming the exact failure conditions. The conditions I > had > > in my mind were somewhat inverted but my main argument about the test is still > > valid: > > > > a. The test stalls at pages for which chrome runs fine. Both the hackernews > > page and the Nintendo page (https://developer.nintendo.com/ where the test > > stalls) runs w/o any issue on a build based on this patch. > > > > b. The existing test failure bug (crbug.com/657665) for Windows shows a > failure > > with the exact same stack trace. > > > > I guess there could be some problem with the cached test page for Nintendo. > > > > nednguyen and aiolos, do you see any reason to believe that the test failure > > is specific to this CL? > > What is the Chrome stack trace produced by the failure in this CL? In > crbug.com/657665, I don't see any chrome stack trace, just the telemetry python > stack. > > If you believe the failure is unrelated & probably flake, you can undo you > change to tools/perf/ folder & try landing this. This doesn't look like a flake, and Chrome is not crashing either. The python test code times out because the JS in the *test* Nintendo page seems to get trapped in an infinite loop in "barebone.jsp". Is there a way to load the test (cached) htmls from command line?
On 2016/11/21 21:06:17, mustaq wrote: > On 2016/11/21 19:17:35, nednguyen wrote: > > On 2016/11/21 17:04:04, mustaq wrote: > > > On 2016/11/21 15:53:16, sunyunjia wrote: > > > > In this hackernews test, chrome cannot detect whether there is a touch > > screen > > > or > > > > not. So in original build, the flag is always disabled, and the test > passes. > > > And > > > > in my patch, the flag is always enabled, and the test fails. > > > > > > > > After some investigation, I figured that the following attributes enabled > by > > > the > > > > flag causes the test to fail. > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Globa... > > > > > > > > > > > > > > > > // Touch Events > > > > // > > > > > > > > > > https://w3c.github.io/touch-events/#extensions-to-the-globaleventhandlers-int... > > > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchcancel; > > > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchend; > > > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchmove; > > > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchstart; > > > > > > Thanks Sandra for confirming the exact failure conditions. The conditions I > > had > > > in my mind were somewhat inverted but my main argument about the test is > still > > > valid: > > > > > > a. The test stalls at pages for which chrome runs fine. Both the hackernews > > > page and the Nintendo page (https://developer.nintendo.com/ where the test > > > stalls) runs w/o any issue on a build based on this patch. > > > > > > b. The existing test failure bug (crbug.com/657665) for Windows shows a > > failure > > > with the exact same stack trace. > > > > > > I guess there could be some problem with the cached test page for Nintendo. > > > > > > nednguyen and aiolos, do you see any reason to believe that the test failure > > > is specific to this CL? > > > > What is the Chrome stack trace produced by the failure in this CL? In > > crbug.com/657665, I don't see any chrome stack trace, just the telemetry > python > > stack. > > > > If you believe the failure is unrelated & probably flake, you can undo you > > change to tools/perf/ folder & try landing this. > > This doesn't look like a flake, and Chrome is not crashing either. The python > test code times out because the JS in the *test* Nintendo page seems to get > trapped in an infinite loop in "barebone.jsp". > > Is there a way to load the test (cached) htmls from command line? Ran a build where all chrome://flags matches those in the stalled test browser. The page developer.nintendo.com loads smoothly in our manual run but not in the test.
On 2016/11/21 21:21:53, mustaq wrote: > On 2016/11/21 21:06:17, mustaq wrote: > > On 2016/11/21 19:17:35, nednguyen wrote: > > > On 2016/11/21 17:04:04, mustaq wrote: > > > > On 2016/11/21 15:53:16, sunyunjia wrote: > > > > > In this hackernews test, chrome cannot detect whether there is a touch > > > screen > > > > or > > > > > not. So in original build, the flag is always disabled, and the test > > passes. > > > > And > > > > > in my patch, the flag is always enabled, and the test fails. > > > > > > > > > > After some investigation, I figured that the following attributes > enabled > > by > > > > the > > > > > flag causes the test to fail. > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Globa... > > > > > > > > > > > > > > > > > > > > // Touch Events > > > > > // > > > > > > > > > > > > > > > https://w3c.github.io/touch-events/#extensions-to-the-globaleventhandlers-int... > > > > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchcancel; > > > > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchend; > > > > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchmove; > > > > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchstart; > > > > > > > > Thanks Sandra for confirming the exact failure conditions. The conditions > I > > > had > > > > in my mind were somewhat inverted but my main argument about the test is > > still > > > > valid: > > > > > > > > a. The test stalls at pages for which chrome runs fine. Both the > hackernews > > > > page and the Nintendo page (https://developer.nintendo.com/ where the test > > > > stalls) runs w/o any issue on a build based on this patch. > > > > > > > > b. The existing test failure bug (crbug.com/657665) for Windows shows a > > > failure > > > > with the exact same stack trace. > > > > > > > > I guess there could be some problem with the cached test page for > Nintendo. > > > > > > > > nednguyen and aiolos, do you see any reason to believe that the test > failure > > > > is specific to this CL? > > > > > > What is the Chrome stack trace produced by the failure in this CL? In > > > crbug.com/657665, I don't see any chrome stack trace, just the telemetry > > python > > > stack. > > > > > > If you believe the failure is unrelated & probably flake, you can undo you > > > change to tools/perf/ folder & try landing this. > > > > This doesn't look like a flake, and Chrome is not crashing either. The python > > test code times out because the JS in the *test* Nintendo page seems to get > > trapped in an infinite loop in "barebone.jsp". > > > > Is there a way to load the test (cached) htmls from command line? > > Ran a build where all chrome://flags matches those in the stalled test browser. > The page http://developer.nintendo.com loads smoothly in our manual run but not in the > test. btw, the nintendo test page is https://...
On 2016/11/21 21:37:24, mustaq wrote: > On 2016/11/21 21:21:53, mustaq wrote: > > On 2016/11/21 21:06:17, mustaq wrote: > > > On 2016/11/21 19:17:35, nednguyen wrote: > > > > On 2016/11/21 17:04:04, mustaq wrote: > > > > > On 2016/11/21 15:53:16, sunyunjia wrote: > > > > > > In this hackernews test, chrome cannot detect whether there is a touch > > > > screen > > > > > or > > > > > > not. So in original build, the flag is always disabled, and the test > > > passes. > > > > > And > > > > > > in my patch, the flag is always enabled, and the test fails. > > > > > > > > > > > > After some investigation, I figured that the following attributes > > enabled > > > by > > > > > the > > > > > > flag causes the test to fail. > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Globa... > > > > > > > > > > > > > > > > > > > > > > > > // Touch Events > > > > > > // > > > > > > > > > > > > > > > > > > > > > https://w3c.github.io/touch-events/#extensions-to-the-globaleventhandlers-int... > > > > > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchcancel; > > > > > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchend; > > > > > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchmove; > > > > > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchstart; > > > > > > > > > > Thanks Sandra for confirming the exact failure conditions. The > conditions > > I > > > > had > > > > > in my mind were somewhat inverted but my main argument about the test is > > > still > > > > > valid: > > > > > > > > > > a. The test stalls at pages for which chrome runs fine. Both the > > hackernews > > > > > page and the Nintendo page (https://developer.nintendo.com/ where the > test > > > > > stalls) runs w/o any issue on a build based on this patch. > > > > > > > > > > b. The existing test failure bug (crbug.com/657665) for Windows shows a > > > > failure > > > > > with the exact same stack trace. > > > > > > > > > > I guess there could be some problem with the cached test page for > > Nintendo. > > > > > > > > > > nednguyen and aiolos, do you see any reason to believe that the test > > failure > > > > > is specific to this CL? > > > > > > > > What is the Chrome stack trace produced by the failure in this CL? In > > > > crbug.com/657665, I don't see any chrome stack trace, just the telemetry > > > python > > > > stack. > > > > > > > > If you believe the failure is unrelated & probably flake, you can undo you > > > > change to tools/perf/ folder & try landing this. > > > > > > This doesn't look like a flake, and Chrome is not crashing either. The > python > > > test code times out because the JS in the *test* Nintendo page seems to get > > > trapped in an infinite loop in "barebone.jsp". > > > > > > Is there a way to load the test (cached) htmls from command line? > > > > Ran a build where all chrome://flags matches those in the stalled test > browser. > > The page http://developer.nintendo.com loads smoothly in our manual run but > not in the > > test. > > btw, the nintendo test page is https://... To debug this, I suggest adding more DLOG(WARNING) to figure out why it's failing telemetry test. (https://bugs.chromium.org/p/chromium/issues/detail?id=665007#c6) You can search browser log in telemetry output by ctrl+f "* BROWSER LOG *"
The CQ bit was checked by mustaq@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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
On 2016/11/22 13:49:24, nednguyen wrote: > On 2016/11/21 21:37:24, mustaq wrote: > > On 2016/11/21 21:21:53, mustaq wrote: > > > On 2016/11/21 21:06:17, mustaq wrote: > > > > On 2016/11/21 19:17:35, nednguyen wrote: > > > > > On 2016/11/21 17:04:04, mustaq wrote: > > > > > > On 2016/11/21 15:53:16, sunyunjia wrote: > > > > > > > In this hackernews test, chrome cannot detect whether there is a > touch > > > > > screen > > > > > > or > > > > > > > not. So in original build, the flag is always disabled, and the test > > > > passes. > > > > > > And > > > > > > > in my patch, the flag is always enabled, and the test fails. > > > > > > > > > > > > > > After some investigation, I figured that the following attributes > > > enabled > > > > by > > > > > > the > > > > > > > flag causes the test to fail. > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Globa... > > > > > > > > > > > > > > > > > > > > > > > > > > > > // Touch Events > > > > > > > // > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://w3c.github.io/touch-events/#extensions-to-the-globaleventhandlers-int... > > > > > > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchcancel; > > > > > > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchend; > > > > > > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchmove; > > > > > > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchstart; > > > > > > > > > > > > Thanks Sandra for confirming the exact failure conditions. The > > conditions > > > I > > > > > had > > > > > > in my mind were somewhat inverted but my main argument about the test > is > > > > still > > > > > > valid: > > > > > > > > > > > > a. The test stalls at pages for which chrome runs fine. Both the > > > hackernews > > > > > > page and the Nintendo page (https://developer.nintendo.com/ where the > > test > > > > > > stalls) runs w/o any issue on a build based on this patch. > > > > > > > > > > > > b. The existing test failure bug (crbug.com/657665) for Windows shows > a > > > > > failure > > > > > > with the exact same stack trace. > > > > > > > > > > > > I guess there could be some problem with the cached test page for > > > Nintendo. > > > > > > > > > > > > nednguyen and aiolos, do you see any reason to believe that the test > > > failure > > > > > > is specific to this CL? > > > > > > > > > > What is the Chrome stack trace produced by the failure in this CL? In > > > > > crbug.com/657665, I don't see any chrome stack trace, just the telemetry > > > > python > > > > > stack. > > > > > > > > > > If you believe the failure is unrelated & probably flake, you can undo > you > > > > > change to tools/perf/ folder & try landing this. > > > > > > > > This doesn't look like a flake, and Chrome is not crashing either. The > > python > > > > test code times out because the JS in the *test* Nintendo page seems to > get > > > > trapped in an infinite loop in "barebone.jsp". > > > > > > > > Is there a way to load the test (cached) htmls from command line? > > > > > > Ran a build where all chrome://flags matches those in the stalled test > > browser. > > > The page http://developer.nintendo.com loads smoothly in our manual run but > > not in the > > > test. > > > > btw, the nintendo test page is https://... > > To debug this, I suggest adding more DLOG(WARNING) to figure out why it's > failing telemetry test. > (https://bugs.chromium.org/p/chromium/issues/detail?id=665007#c6) > > You can search browser log in telemetry output by ctrl+f "* BROWSER LOG *" The certificate verification errors in your debug log are certainly unrelated to this CL, and seem to suggest a setup issue here. Is this what happens when a recorded page tries access an unrecorded URL during a test? Here is why I am asking this question: the Nintendo page is clearly taking a different codepath when TouchEvents are enabled (and the new codepath works fine outside the test). It is conceivable that the page needs additional network access in that new codepath which was not cached during the recording. The bottom line is that we need to land this CL & merge to the M56 branch ASAP. Could we please defer fixing the test because the test-page works fine outside 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...
On 2016/11/23 21:32:30, mustaq wrote: > On 2016/11/22 13:49:24, nednguyen wrote: > > On 2016/11/21 21:37:24, mustaq wrote: > > > On 2016/11/21 21:21:53, mustaq wrote: > > > > On 2016/11/21 21:06:17, mustaq wrote: > > > > > On 2016/11/21 19:17:35, nednguyen wrote: > > > > > > On 2016/11/21 17:04:04, mustaq wrote: > > > > > > > On 2016/11/21 15:53:16, sunyunjia wrote: > > > > > > > > In this hackernews test, chrome cannot detect whether there is a > > touch > > > > > > screen > > > > > > > or > > > > > > > > not. So in original build, the flag is always disabled, and the > test > > > > > passes. > > > > > > > And > > > > > > > > in my patch, the flag is always enabled, and the test fails. > > > > > > > > > > > > > > > > After some investigation, I figured that the following attributes > > > > enabled > > > > > by > > > > > > > the > > > > > > > > flag causes the test to fail. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Globa... > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > // Touch Events > > > > > > > > // > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://w3c.github.io/touch-events/#extensions-to-the-globaleventhandlers-int... > > > > > > > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler > ontouchcancel; > > > > > > > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchend; > > > > > > > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler ontouchmove; > > > > > > > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler > ontouchstart; > > > > > > > > > > > > > > Thanks Sandra for confirming the exact failure conditions. The > > > conditions > > > > I > > > > > > had > > > > > > > in my mind were somewhat inverted but my main argument about the > test > > is > > > > > still > > > > > > > valid: > > > > > > > > > > > > > > a. The test stalls at pages for which chrome runs fine. Both the > > > > hackernews > > > > > > > page and the Nintendo page (https://developer.nintendo.com/ where > the > > > test > > > > > > > stalls) runs w/o any issue on a build based on this patch. > > > > > > > > > > > > > > b. The existing test failure bug (crbug.com/657665) for Windows > shows > > a > > > > > > failure > > > > > > > with the exact same stack trace. > > > > > > > > > > > > > > I guess there could be some problem with the cached test page for > > > > Nintendo. > > > > > > > > > > > > > > nednguyen and aiolos, do you see any reason to believe that the test > > > > failure > > > > > > > is specific to this CL? > > > > > > > > > > > > What is the Chrome stack trace produced by the failure in this CL? In > > > > > > crbug.com/657665, I don't see any chrome stack trace, just the > telemetry > > > > > python > > > > > > stack. > > > > > > > > > > > > If you believe the failure is unrelated & probably flake, you can undo > > you > > > > > > change to tools/perf/ folder & try landing this. > > > > > > > > > > This doesn't look like a flake, and Chrome is not crashing either. The > > > python > > > > > test code times out because the JS in the *test* Nintendo page seems to > > get > > > > > trapped in an infinite loop in "barebone.jsp". > > > > > > > > > > Is there a way to load the test (cached) htmls from command line? > > > > > > > > Ran a build where all chrome://flags matches those in the stalled test > > > browser. > > > > The page http://developer.nintendo.com loads smoothly in our manual run > but > > > not in the > > > > test. > > > > > > btw, the nintendo test page is https://... > > > > To debug this, I suggest adding more DLOG(WARNING) to figure out why it's > > failing telemetry test. > > (https://bugs.chromium.org/p/chromium/issues/detail?id=665007#c6) > > > > You can search browser log in telemetry output by ctrl+f "* BROWSER LOG *" > > The certificate verification errors in your debug log are certainly > unrelated to this CL, and seem to suggest a setup issue here. Is this what > happens when a recorded page tries access an unrecorded URL during a test? > Here is why I am asking this question: the Nintendo page is clearly taking > a different codepath when TouchEvents are enabled (and the new codepath > works fine outside the test). It is conceivable that the page needs > additional network access in that new codepath which was not cached during > the recording. This reasoning makes sense. > > The bottom line is that we need to land this CL & merge to the M56 branch > ASAP. Could we please defer fixing the test because the test-page works > fine outside the test? Can you run the benchmark with chromium built from this patch as follows: "./tools/perf/run_benchmark system_health.memory_desktop --story-filter=browse:news:hackernews --use-live-sites" If this passes on windows, I am fine with you disabling the test for now. Our team will take care of rerecording it.
On 2016/11/23 22:20:01, nednguyen wrote: > On 2016/11/23 21:32:30, mustaq wrote: > > On 2016/11/22 13:49:24, nednguyen wrote: > > > On 2016/11/21 21:37:24, mustaq wrote: > > > > On 2016/11/21 21:21:53, mustaq wrote: > > > > > On 2016/11/21 21:06:17, mustaq wrote: > > > > > > On 2016/11/21 19:17:35, nednguyen wrote: > > > > > > > On 2016/11/21 17:04:04, mustaq wrote: > > > > > > > > On 2016/11/21 15:53:16, sunyunjia wrote: > > > > > > > > > In this hackernews test, chrome cannot detect whether there is a > > > touch > > > > > > > screen > > > > > > > > or > > > > > > > > > not. So in original build, the flag is always disabled, and the > > test > > > > > > passes. > > > > > > > > And > > > > > > > > > in my patch, the flag is always enabled, and the test fails. > > > > > > > > > > > > > > > > > > After some investigation, I figured that the following > attributes > > > > > enabled > > > > > > by > > > > > > > > the > > > > > > > > > flag causes the test to fail. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Globa... > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > // Touch Events > > > > > > > > > // > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://w3c.github.io/touch-events/#extensions-to-the-globaleventhandlers-int... > > > > > > > > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler > > ontouchcancel; > > > > > > > > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler > ontouchend; > > > > > > > > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler > ontouchmove; > > > > > > > > > [RuntimeEnabled=TouchEventAPI] attribute EventHandler > > ontouchstart; > > > > > > > > > > > > > > > > Thanks Sandra for confirming the exact failure conditions. The > > > > conditions > > > > > I > > > > > > > had > > > > > > > > in my mind were somewhat inverted but my main argument about the > > test > > > is > > > > > > still > > > > > > > > valid: > > > > > > > > > > > > > > > > a. The test stalls at pages for which chrome runs fine. Both the > > > > > hackernews > > > > > > > > page and the Nintendo page (https://developer.nintendo.com/ where > > the > > > > test > > > > > > > > stalls) runs w/o any issue on a build based on this patch. > > > > > > > > > > > > > > > > b. The existing test failure bug (crbug.com/657665) for Windows > > shows > > > a > > > > > > > failure > > > > > > > > with the exact same stack trace. > > > > > > > > > > > > > > > > I guess there could be some problem with the cached test page for > > > > > Nintendo. > > > > > > > > > > > > > > > > nednguyen and aiolos, do you see any reason to believe that the > test > > > > > failure > > > > > > > > is specific to this CL? > > > > > > > > > > > > > > What is the Chrome stack trace produced by the failure in this CL? > In > > > > > > > crbug.com/657665, I don't see any chrome stack trace, just the > > telemetry > > > > > > python > > > > > > > stack. > > > > > > > > > > > > > > If you believe the failure is unrelated & probably flake, you can > undo > > > you > > > > > > > change to tools/perf/ folder & try landing this. > > > > > > > > > > > > This doesn't look like a flake, and Chrome is not crashing either. The > > > > python > > > > > > test code times out because the JS in the *test* Nintendo page seems > to > > > get > > > > > > trapped in an infinite loop in "barebone.jsp". > > > > > > > > > > > > Is there a way to load the test (cached) htmls from command line? > > > > > > > > > > Ran a build where all chrome://flags matches those in the stalled test > > > > browser. > > > > > The page http://developer.nintendo.com loads smoothly in our manual run > > but > > > > not in the > > > > > test. > > > > > > > > btw, the nintendo test page is https://... > > > > > > To debug this, I suggest adding more DLOG(WARNING) to figure out why it's > > > failing telemetry test. > > > (https://bugs.chromium.org/p/chromium/issues/detail?id=665007#c6) > > > > > > You can search browser log in telemetry output by ctrl+f "* BROWSER LOG *" > > > > The certificate verification errors in your debug log are certainly > > unrelated to this CL, and seem to suggest a setup issue here. Is this what > > happens when a recorded page tries access an unrecorded URL during a test? > > Here is why I am asking this question: the Nintendo page is clearly taking > > a different codepath when TouchEvents are enabled (and the new codepath > > works fine outside the test). It is conceivable that the page needs > > additional network access in that new codepath which was not cached during > > the recording. > > This reasoning makes sense. > > > > The bottom line is that we need to land this CL & merge to the M56 branch > > ASAP. Could we please defer fixing the test because the test-page works > > fine outside the test? > > Can you run the benchmark with chromium built from this patch as follows: > "./tools/perf/run_benchmark system_health.memory_desktop > --story-filter=browse:news:hackernews --use-live-sites" > > If this passes on windows, I am fine with you disabling the test for now. Our > team will take care of rerecording it. Sorry, forgot to mention that you need to add "--browser=release" flag to the command if you build chrome release.
Thanks! The test passes locally with the flag "use-live-sites".
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_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: Try jobs failed on following builders: chromium_presubmit 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...
sunyunjia@chromium.org changed reviewers: + rkaplow@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
rkaplow@, Could you please review the code for approval in chrome/browser/metrics? Thanks!
https://codereview.chromium.org/2467913002/diff/540001/chrome/browser/metrics... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2467913002/diff/540001/chrome/browser/metrics... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:277: UMA_HISTOGRAM_ENUMERATION("Touchscreen.TouchEventsEnabled", state, since this is changing what Touchscreen.TouchEventsEnabled will output, can you update the description in the histograms.xml to denote this?
PTAL. Thanks! https://codereview.chromium.org/2467913002/diff/540001/chrome/browser/metrics... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/2467913002/diff/540001/chrome/browser/metrics... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:277: UMA_HISTOGRAM_ENUMERATION("Touchscreen.TouchEventsEnabled", state, On 2016/11/24 15:46:02, rkaplow (slow and travelling) wrote: > since this is changing what Touchscreen.TouchEventsEnabled will output, can you > update the description in the histograms.xml to denote this? Done.
Can you also mark any enum entries which are no longer used as deprecated (in the label name) - it seems your histogram is the only one that uses the enum
On 2016/11/24 18:51:21, rkaplow (slow and travelling) wrote: > Can you also mark any enum entries which are no longer used as deprecated (in > the label name) - it seems your histogram is the only one that uses the enum Done. 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...
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, tkent@chromium.org, dtapuska@chromium.org, dcheng@chromium.org, avi@chromium.org, sadrul@chromium.org, nednguyen@google.com Link to the patchset: https://codereview.chromium.org/2467913002/#ps580001 (title: "Deprecate the histogram.")
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": 580001, "attempt_start_ts": 1480024841756410,
"parent_rev": "cb44e91a1ac078bc0a15ac0b52d8e7a8eaf846f0", "commit_rev":
"e12052411c156738a5309f89fa2392c3effaa619"}
Message was sent while issue was closed.
Description was changed from ========== Touch event flag should control only DOM event firing. Currently there is no way to disable TouchEvent API support without affecting either PointerEvents (for touches) or touch support in browser UI. With PointerEvents shipping in M55, it now makes the most sense to turn on touch support in browser unconditionally and let the --touch-events flag control only the firing of DOM events. This CL also renames the flag to clarify its purpose. BUG=644318 ========== to ========== Touch event flag should control only DOM event firing. Currently there is no way to disable TouchEvent API support without affecting either PointerEvents (for touches) or touch support in browser UI. With PointerEvents shipping in M55, it now makes the most sense to turn on touch support in browser unconditionally and let the --touch-events flag control only the firing of DOM events. This CL also renames the flag to clarify its purpose. BUG=644318 ==========
Message was sent while issue was closed.
Committed patchset #20 (id:580001)
Message was sent while issue was closed.
Description was changed from ========== Touch event flag should control only DOM event firing. Currently there is no way to disable TouchEvent API support without affecting either PointerEvents (for touches) or touch support in browser UI. With PointerEvents shipping in M55, it now makes the most sense to turn on touch support in browser unconditionally and let the --touch-events flag control only the firing of DOM events. This CL also renames the flag to clarify its purpose. BUG=644318 ========== to ========== Touch event flag should control only DOM event firing. Currently there is no way to disable TouchEvent API support without affecting either PointerEvents (for touches) or touch support in browser UI. With PointerEvents shipping in M55, it now makes the most sense to turn on touch support in browser unconditionally and let the --touch-events flag control only the firing of DOM events. This CL also renames the flag to clarify its purpose. BUG=644318 Committed: https://crrev.com/7a880dd75bf766d22cbe4ba79bb168f7ea38c524 Cr-Commit-Position: refs/heads/master@{#434399} ==========
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/7a880dd75bf766d22cbe4ba79bb168f7ea38c524 Cr-Commit-Position: refs/heads/master@{#434399}
Message was sent while issue was closed.
rbyers@chromium.org changed reviewers: + dtapuska@chromium.org, rbyers@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2467913002/diff/580001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2467913002/diff/580001/chrome/browser/about_f... chrome/browser/about_flags.cc:138: { IDS_GENERIC_EXPERIMENT_CHOICE_ENABLED, "", "" }, Does this mean that "enabled" is now the default, instead of "auto" (determined based on the presence of a touchscreen)? That's going to be a problem for web compat 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 IMHO we need to retain this hacky "auto" behavior as the default for the touch event API status. The rest of the patch looks great though - thanks especially for the renames / clarifications!
Message was sent while issue was closed.
On 2016/11/24 23:53:07, Rick Byers wrote: > https://codereview.chromium.org/2467913002/diff/580001/chrome/browser/about_f... > File chrome/browser/about_flags.cc (right): > > https://codereview.chromium.org/2467913002/diff/580001/chrome/browser/about_f... > chrome/browser/about_flags.cc:138: { IDS_GENERIC_EXPERIMENT_CHOICE_ENABLED, "", > "" }, > Does this mean that "enabled" is now the default, instead of "auto" (determined > based on the presence of a touchscreen)? That's going to be a problem for web > compat 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 IMHO we need to retain this hacky "auto" behavior as the default for the > touch event API status. > > The rest of the patch looks great though - thanks especially for the renames / > clarifications! We will bring back the original AUTO behavior through a separate CL asap. |
