Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2520423004:
Don't log a console warning when touch-action is used. (Closed)
|
Created:
4 years ago by dtapuska Modified:
4 years ago Reviewers:
Rick Byers CC:
blink-reviews, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't log a console warning when touch-action is used.
To avoid log fatigue for the developer only log the "you are prevent
defaulting a passive event listener" if no touch-action is used. If
touch-action is being used then it is clear that the developer is doing
this to interop with other browsers that don't support touch-action.
BUG=667799
Committed: https://crrev.com/6ea5974532e7cfea654c59181f1819e6805bb368
Cr-Commit-Position: refs/heads/master@{#434536}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Move log into TouchEvent #Patch Set 3 : Add layout test #
Total comments: 1
Messages
Total messages: 25 (15 generated)
The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run
dtapuska@chromium.org changed reviewers: + rbyers@chromium.org
PTAL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Do you think it's worth adding a test? https://codereview.chromium.org/2520423004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/events/Event.cpp (right): https://codereview.chromium.org/2520423004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/Event.cpp:241: case PassiveMode::PassiveForcedDocumentLevel: I think this would get a little cleaner (and avoid the need to add another virtual on Event) if you moved this case into TouchEvent::preventDefault. I.e. keep the touch-intervention-specific stuff to TouchEvent as much as possible. There's already a bunch of related code in TouchEvent::preventDefault. WDYT? https://codereview.chromium.org/2520423004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/events/TouchEvent.cpp (right): https://codereview.chromium.org/2520423004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/TouchEvent.cpp:318: return m_currentTouchAction == TouchActionAuto; nit: add a comment explaining why we disable the warning in this case. It is a bit of a heuristic - it's possible the warning still applies.
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 dtapuska@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/22 20:40:33, Rick Byers wrote: > Do you think it's worth adding a test? > > https://codereview.chromium.org/2520423004/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/events/Event.cpp (right): > > https://codereview.chromium.org/2520423004/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/events/Event.cpp:241: case > PassiveMode::PassiveForcedDocumentLevel: > I think this would get a little cleaner (and avoid the need to add another > virtual on Event) if you moved this case into TouchEvent::preventDefault. I.e. > keep the touch-intervention-specific stuff to TouchEvent as much as possible. > There's already a bunch of related code in TouchEvent::preventDefault. WDYT? > > https://codereview.chromium.org/2520423004/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/events/TouchEvent.cpp (right): > > https://codereview.chromium.org/2520423004/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/events/TouchEvent.cpp:318: return > m_currentTouchAction == TouchActionAuto; > nit: add a comment explaining why we disable the warning in this case. It is a > bit of a heuristic - it's possible the warning still applies. Done; I tried to write a layout test. But didn't have success. I grepped for the other errors and it appears there are no tests for them either. I'm not sure if console warnings end up in the expected file; it didn't appear so to me.
On 2016/11/23 15:46:56, dtapuska wrote: > On 2016/11/22 20:40:33, Rick Byers wrote: > > Do you think it's worth adding a test? > > > > > https://codereview.chromium.org/2520423004/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/events/Event.cpp (right): > > > > > https://codereview.chromium.org/2520423004/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/events/Event.cpp:241: case > > PassiveMode::PassiveForcedDocumentLevel: > > I think this would get a little cleaner (and avoid the need to add another > > virtual on Event) if you moved this case into TouchEvent::preventDefault. > I.e. > > keep the touch-intervention-specific stuff to TouchEvent as much as possible. > > There's already a bunch of related code in TouchEvent::preventDefault. WDYT? > > > > > https://codereview.chromium.org/2520423004/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/events/TouchEvent.cpp (right): > > > > > https://codereview.chromium.org/2520423004/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/events/TouchEvent.cpp:318: return > > m_currentTouchAction == TouchActionAuto; > > nit: add a comment explaining why we disable the warning in this case. It is > a > > bit of a heuristic - it's possible the warning still applies. > > Done; I tried to write a layout test. But didn't have success. I grepped for the > other errors and it appears there are no tests for them either. I'm not sure if > console warnings end up in the expected file; it didn't appear so to me. err; there appears to be touch/touch-event-cancelable-expected.txt let me try again
The CQ bit was checked by dtapuska@chromium.org to run a CQ dry run
On 2016/11/23 15:47:56, dtapuska wrote: > On 2016/11/23 15:46:56, dtapuska wrote: > > On 2016/11/22 20:40:33, Rick Byers wrote: > > > Do you think it's worth adding a test? > > > > > > > > > https://codereview.chromium.org/2520423004/diff/1/third_party/WebKit/Source/c... > > > File third_party/WebKit/Source/core/events/Event.cpp (right): > > > > > > > > > https://codereview.chromium.org/2520423004/diff/1/third_party/WebKit/Source/c... > > > third_party/WebKit/Source/core/events/Event.cpp:241: case > > > PassiveMode::PassiveForcedDocumentLevel: > > > I think this would get a little cleaner (and avoid the need to add another > > > virtual on Event) if you moved this case into TouchEvent::preventDefault. > > I.e. > > > keep the touch-intervention-specific stuff to TouchEvent as much as > possible. > > > There's already a bunch of related code in TouchEvent::preventDefault. > WDYT? > > > > > > > > > https://codereview.chromium.org/2520423004/diff/1/third_party/WebKit/Source/c... > > > File third_party/WebKit/Source/core/events/TouchEvent.cpp (right): > > > > > > > > > https://codereview.chromium.org/2520423004/diff/1/third_party/WebKit/Source/c... > > > third_party/WebKit/Source/core/events/TouchEvent.cpp:318: return > > > m_currentTouchAction == TouchActionAuto; > > > nit: add a comment explaining why we disable the warning in this case. It > is > > a > > > bit of a heuristic - it's possible the warning still applies. > > > > Done; I tried to write a layout test. But didn't have success. I grepped for > the > > other errors and it appears there are no tests for them either. I'm not sure > if > > console warnings end up in the expected file; it didn't appear so to me. > > err; there appears to be touch/touch-event-cancelable-expected.txt let me try > again Added layout test.
The CQ bit was checked by dtapuska@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.
https://codereview.chromium.org/2520423004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/TouchEvent.cpp (right): https://codereview.chromium.org/2520423004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/TouchEvent.cpp:265: case PassiveMode::NotPassive: Shouldn't NotPassiveDefault be this case too? Looks like we lost that warning coverage at some point (probably didn't notice when the 'Default' split was added). The touch-event-cancelable.html test only checks the explicit {passive:false} case - please either add a test case for the default case, or if you prefer just change it to ONLY test the default case (the explicit passive:false case is probably going to be quite rare).
On 2016/11/25 at 16:06:29, Rick Byers wrote: > https://codereview.chromium.org/2520423004/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/events/TouchEvent.cpp (right): > > https://codereview.chromium.org/2520423004/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/events/TouchEvent.cpp:265: case PassiveMode::NotPassive: > Shouldn't NotPassiveDefault be this case too? Looks like we lost that warning coverage at some point (probably didn't notice when the 'Default' split was added). The touch-event-cancelable.html test only checks the explicit {passive:false} case - please either add a test case for the default case, or if you prefer just change it to ONLY test the default case (the explicit passive:false case is probably going to be quite rare). BTW if you want to fix that in a separate CL that's fine with me (since it's pre-existing). LGTM.
The CQ bit was checked by dtapuska@chromium.org
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": 40001, "attempt_start_ts": 1480091876355600,
"parent_rev": "4128a1c81c70905d104db1adb592e369927f12fc", "commit_rev":
"b3195007eceaf6de937d5b27aa9d2cb8364122d7"}
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Don't log a console warning when touch-action is used. To avoid log fatigue for the developer only log the "you are prevent defaulting a passive event listener" if no touch-action is used. If touch-action is being used then it is clear that the developer is doing this to interop with other browsers that don't support touch-action. BUG=667799 ========== to ========== Don't log a console warning when touch-action is used. To avoid log fatigue for the developer only log the "you are prevent defaulting a passive event listener" if no touch-action is used. If touch-action is being used then it is clear that the developer is doing this to interop with other browsers that don't support touch-action. BUG=667799 Committed: https://crrev.com/6ea5974532e7cfea654c59181f1819e6805bb368 Cr-Commit-Position: refs/heads/master@{#434536} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/6ea5974532e7cfea654c59181f1819e6805bb368 Cr-Commit-Position: refs/heads/master@{#434536} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
