Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(59)

Issue 600773002: Notify in console when an API fails because it needs to be invoked on user action. (Closed)

Created:
6 years, 3 months ago by Mayur Kankanwadi
Modified:
6 years, 2 months ago
CC:
blink-reviews, blink-reviews-dom_chromium.org, dglazkov+blink, sof, eae+blinkwatch, rwlbuis
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Project:
blink
Visibility:
Public.

Description

Notify in console when an API fails because it needs to be invoked on user action. This CL shows a console warning on requestFullScreen, requestPointerLock and Notification.requestPermission api's, when they are not invoked on user action. BUG=365779 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183069

Patch Set 1 #

Total comments: 8

Patch Set 2 : Review comments added with reset -expected.txt files. #

Total comments: 8

Patch Set 3 : Fixed console message and rebased expected.txt files. #

Total comments: 7

Patch Set 4 : Using ExceptionMessage for generating msgs. Updated expected.txt files. #

Patch Set 5 : Removed requestPointerLock related changes. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+12 lines, -1 line) Patch
M LayoutTests/fullscreen/full-screen-request-rejected-expected.txt View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/fullscreen/full-screen-request-removed-expected.txt View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/fullscreen/full-screen-restrictions-expected.txt View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/fullscreen/video-fail-to-enter-full-screen-expected.txt View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/media/video-enter-fullscreen-without-user-gesture-expected.txt View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/media/video-prefixed-fullscreen-expected.txt View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/dom/Fullscreen.cpp View 1 2 3 1 chunk +6 lines, -1 line 0 comments Download

Messages

Total messages: 36 (5 generated)
Mayur Kankanwadi
Hi All, This is first cut of the fix. This is breaking 17 layout tests, ...
6 years, 3 months ago (2014-09-24 12:05:15 UTC) #2
Peter Beverloo
https://codereview.chromium.org/600773002/diff/1/Source/modules/notifications/Notification.cpp File Source/modules/notifications/Notification.cpp (right): https://codereview.chromium.org/600773002/diff/1/Source/modules/notifications/Notification.cpp#newcode167 Source/modules/notifications/Notification.cpp:167: } We removed the user gesture requirement for requesting ...
6 years, 3 months ago (2014-09-24 14:59:44 UTC) #4
Mayur Kankanwadi
https://codereview.chromium.org/600773002/diff/1/Source/modules/notifications/Notification.cpp File Source/modules/notifications/Notification.cpp (right): https://codereview.chromium.org/600773002/diff/1/Source/modules/notifications/Notification.cpp#newcode167 Source/modules/notifications/Notification.cpp:167: } On 2014/09/24 14:59:44, Peter Beverloo wrote: > We ...
6 years, 3 months ago (2014-09-24 15:03:42 UTC) #5
Peter Beverloo
https://codereview.chromium.org/600773002/diff/1/Source/modules/notifications/Notification.cpp File Source/modules/notifications/Notification.cpp (right): https://codereview.chromium.org/600773002/diff/1/Source/modules/notifications/Notification.cpp#newcode167 Source/modules/notifications/Notification.cpp:167: } On 2014/09/24 15:03:42, Mayur Kankanwadi wrote: > On ...
6 years, 3 months ago (2014-09-24 17:14:20 UTC) #6
meacer
Thanks for taking over this bug! I added some comments. https://codereview.chromium.org/600773002/diff/1/Source/core/dom/Fullscreen.cpp File Source/core/dom/Fullscreen.cpp (right): https://codereview.chromium.org/600773002/diff/1/Source/core/dom/Fullscreen.cpp#newcode236 ...
6 years, 3 months ago (2014-09-24 17:59:21 UTC) #7
Mayur Kankanwadi
Thanks for the review. Uploaded new patch. PTAL. https://codereview.chromium.org/600773002/diff/1/Source/core/dom/Fullscreen.cpp File Source/core/dom/Fullscreen.cpp (right): https://codereview.chromium.org/600773002/diff/1/Source/core/dom/Fullscreen.cpp#newcode236 Source/core/dom/Fullscreen.cpp:236: "Permission ...
6 years, 2 months ago (2014-09-25 06:43:02 UTC) #8
felt
https://codereview.chromium.org/600773002/diff/20001/Source/core/dom/Fullscreen.cpp File Source/core/dom/Fullscreen.cpp (right): https://codereview.chromium.org/600773002/diff/20001/Source/core/dom/Fullscreen.cpp#newcode236 Source/core/dom/Fullscreen.cpp:236: "requestFullScreen() can only be intiated by a user gesture.")); ...
6 years, 2 months ago (2014-09-25 08:48:39 UTC) #9
Mayur Kankanwadi
https://codereview.chromium.org/600773002/diff/20001/Source/core/dom/Fullscreen.cpp File Source/core/dom/Fullscreen.cpp (right): https://codereview.chromium.org/600773002/diff/20001/Source/core/dom/Fullscreen.cpp#newcode236 Source/core/dom/Fullscreen.cpp:236: "requestFullScreen() can only be intiated by a user gesture.")); ...
6 years, 2 months ago (2014-09-25 08:59:07 UTC) #10
Mayur Kankanwadi
6 years, 2 months ago (2014-09-26 05:31:16 UTC) #11
felt
https://codereview.chromium.org/600773002/diff/20001/Source/core/dom/Fullscreen.cpp File Source/core/dom/Fullscreen.cpp (right): https://codereview.chromium.org/600773002/diff/20001/Source/core/dom/Fullscreen.cpp#newcode236 Source/core/dom/Fullscreen.cpp:236: "requestFullScreen() can only be intiated by a user gesture.")); ...
6 years, 2 months ago (2014-09-26 05:35:46 UTC) #12
Mayur Kankanwadi
On 2014/09/26 05:35:46, felt wrote: > https://codereview.chromium.org/600773002/diff/20001/Source/core/dom/Fullscreen.cpp > File Source/core/dom/Fullscreen.cpp (right): > > https://codereview.chromium.org/600773002/diff/20001/Source/core/dom/Fullscreen.cpp#newcode236 > ...
6 years, 2 months ago (2014-09-26 07:16:52 UTC) #13
Mayur Kankanwadi
On 2014/09/26 07:16:52, Mayur Kankanwadi wrote: > On 2014/09/26 05:35:46, felt wrote: > > > ...
6 years, 2 months ago (2014-09-26 11:10:15 UTC) #14
meacer
Lgtm with a few comments. https://codereview.chromium.org/600773002/diff/20001/LayoutTests/fullscreen/full-screen-request-rejected-expected.txt File LayoutTests/fullscreen/full-screen-request-rejected-expected.txt (right): https://codereview.chromium.org/600773002/diff/20001/LayoutTests/fullscreen/full-screen-request-rejected-expected.txt#newcode1 LayoutTests/fullscreen/full-screen-request-rejected-expected.txt:1: CONSOLE WARNING: requestFullScreen() can ...
6 years, 2 months ago (2014-09-26 17:47:38 UTC) #15
Mayur Kankanwadi
PTAL. Changed the console message as per the suggestions. Thanks. https://codereview.chromium.org/600773002/diff/20001/Source/core/dom/Fullscreen.cpp File Source/core/dom/Fullscreen.cpp (right): https://codereview.chromium.org/600773002/diff/20001/Source/core/dom/Fullscreen.cpp#newcode236 ...
6 years, 2 months ago (2014-09-29 11:22:14 UTC) #16
Mayur Kankanwadi
PTAL.
6 years, 2 months ago (2014-09-29 11:25:55 UTC) #18
Mike West
Two drive-by comments. https://codereview.chromium.org/600773002/diff/40001/Source/core/dom/Fullscreen.cpp File Source/core/dom/Fullscreen.cpp (right): https://codereview.chromium.org/600773002/diff/40001/Source/core/dom/Fullscreen.cpp#newcode236 Source/core/dom/Fullscreen.cpp:236: "requestFullScreen can only be initiated by ...
6 years, 2 months ago (2014-09-29 12:24:08 UTC) #20
Mayur Kankanwadi
Hi All, As per Mike West's suggestion, updated the code to use ExceptionMessages for generating ...
6 years, 2 months ago (2014-09-30 13:56:40 UTC) #21
Mike West
https://codereview.chromium.org/600773002/diff/40001/Source/core/page/PointerLockController.cpp File Source/core/page/PointerLockController.cpp (right): https://codereview.chromium.org/600773002/diff/40001/Source/core/page/PointerLockController.cpp#newcode55 Source/core/page/PointerLockController.cpp:55: } On 2014/09/30 13:56:40, Mayur Kankanwadi wrote: > On ...
6 years, 2 months ago (2014-09-30 14:18:12 UTC) #22
Mayur Kankanwadi
On 2014/09/30 14:18:12, Mike West wrote: > https://codereview.chromium.org/600773002/diff/40001/Source/core/page/PointerLockController.cpp > File Source/core/page/PointerLockController.cpp (right): > > https://codereview.chromium.org/600773002/diff/40001/Source/core/page/PointerLockController.cpp#newcode55 ...
6 years, 2 months ago (2014-09-30 14:28:29 UTC) #23
Mike West
On 2014/09/30 14:28:29, Mayur Kankanwadi wrote: > On 2014/09/30 14:18:12, Mike West wrote: > > ...
6 years, 2 months ago (2014-09-30 14:30:31 UTC) #24
Mayur Kankanwadi
On 2014/09/30 14:30:31, Mike West wrote: > On 2014/09/30 14:28:29, Mayur Kankanwadi wrote: > > ...
6 years, 2 months ago (2014-09-30 14:33:44 UTC) #25
Mike West
> UserGestureIndicator::processingUserGesture() provides us the information of > whether > the api was invoked due ...
6 years, 2 months ago (2014-09-30 14:56:41 UTC) #26
Mayur Kankanwadi
Let's try one more time. :) >Is there another > spot in the flow of ...
6 years, 2 months ago (2014-10-01 06:32:57 UTC) #27
Mike West
On 2014/10/01 06:32:57, Mayur Kankanwadi wrote: > Yes, an ipc message is then sent to ...
6 years, 2 months ago (2014-10-01 08:43:04 UTC) #28
Mayur Kankanwadi
On 2014/10/01 08:43:04, Mike West wrote: > I don't understand why we send the IPC ...
6 years, 2 months ago (2014-10-01 09:19:11 UTC) #29
Mayur Kankanwadi
On 2014/10/01 09:19:11, MayurK (OOO 4D starting..) wrote: > On 2014/10/01 08:43:04, Mike West wrote: ...
6 years, 2 months ago (2014-10-01 12:19:10 UTC) #30
Mike West
I'd suggest limiting this CL to the Fullscreen API, and creating another CL that fires ...
6 years, 2 months ago (2014-10-01 12:32:38 UTC) #31
Mike West
(The Fullscreen portions of this CL LGTM, by the way. :) )
6 years, 2 months ago (2014-10-01 12:32:58 UTC) #32
Mayur Kankanwadi
On 2014/10/01 12:32:58, Mike West wrote: > (The Fullscreen portions of this CL LGTM, by ...
6 years, 2 months ago (2014-10-01 14:29:07 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/600773002/80001
6 years, 2 months ago (2014-10-01 14:29:44 UTC) #35
commit-bot: I haz the power
6 years, 2 months ago (2014-10-01 15:36:34 UTC) #36
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as 183069

Powered by Google App Engine
This is Rietveld 408576698