Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(43)

Issue 2805763004: [System-Keyboard-Lock] Forward navigator functions to RenderFrameHost (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 months, 2 weeks ago by Hzj_jie
Modified:
5 months, 3 weeks ago
CC:
Aaron Boodman, abarth-chromium, blink-reviews, blink-reviews-api_chromium.org, blink-reviews-frames_chromium.org, chromium-reviews, creis+watch_chromium.org, darin (slow to review), darin-cc_chromium.org, dglazkov+blink, jam, kinuko+watch, Łukasz Anforowicz, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, Jamie, garykac
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[System-Keyboard-Lock] Forward navigator functions to RenderFrameHost This change forwards navigator.requestKeyLock() and navigator.cancelKeyLock() functions to the RenderFrameHostImpl. System-Keyboard-Lock is a feature to detect the key presses which usually cannot be received by the browser, and send them to the web page. It contains various components to achieve the goal. This change is one of them, which receives JavaScript (or Web Platform API in the design doc) function calls and forwards them into RenderFrameHost. For detail, please refer to the design doc at, https://docs.google.com/document/d/1T9gJHYdA1VGZ6QHQeOu0hOacWWWJ7yNEt1VDbLju4bs/edit#heading=h.cgwemqs2j4ta W3C Working Draft: https://garykac.github.io/system-keyboard-lock/ Intent to implement: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/9pauQUAvrcw/lfbG7eunCAAJ BUG=680809 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2805763004 Cr-Commit-Position: refs/heads/master@{#468158} Committed: https://chromium.googlesource.com/chromium/src/+/6016778c99f93e8f91eec1d1f20fc44642d3bfde

Patch Set 1 #

Total comments: 2

Patch Set 2 : Resolve review comments #

Total comments: 22

Patch Set 3 : Resolve review comments - Without removing deprecated logic #

Patch Set 4 : Remove useless files #

Patch Set 5 : Add external/wpt tests #

Total comments: 34

Patch Set 6 : Resolve review comments #

Total comments: 2

Patch Set 7 : Resolve review comments #

Total comments: 10

Patch Set 8 : Resolve review comments #

Patch Set 9 : Synchronize latest changes #

Total comments: 75

Patch Set 10 : Resolve review comments #

Total comments: 2

Patch Set 11 : Sync latest changes #

Total comments: 7

Patch Set 12 : Resolve review comments #

Total comments: 15

Patch Set 13 : Resolve review comments #

Patch Set 14 : Missing comments #

Patch Set 15 : Resolve review comments #

Patch Set 16 : Fix media-capabilities idlharness failure as well #

Patch Set 17 : Sync latest changes to resolve broken merge #

Patch Set 18 : Resolve review comments #

Total comments: 2

Patch Set 19 : Resolve review comments #

Total comments: 32

Patch Set 20 : Resolve review comments and update the location of the spec #

Patch Set 21 : Sync latest changes #

Patch Set 22 : Add manifest #

Patch Set 23 : Resolve more review comments #

Patch Set 24 : Update manifest #

Total comments: 4

Patch Set 25 : Resolve review comments #

Total comments: 2

Patch Set 26 : Resolve review comments #

Patch Set 27 : Sync latest changes #

Unified diffs Side-by-side diffs Delta from patch set Stats (+444 lines, -0 lines) Patch
M content/browser/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +2 lines, -0 lines 0 comments Download
M content/browser/DEPS View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/frame_host/render_frame_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 2 chunks +4 lines, -0 lines 0 comments Download
A content/browser/keyboard_lock/keyboard_lock_service_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +29 lines, -0 lines 0 comments Download
A content/browser/keyboard_lock/keyboard_lock_service_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +38 lines, -0 lines 0 comments Download
M content/public/app/mojo/content_browser_manifest.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/W3CImportExpectations View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +5 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +41 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idlharness.https-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +12 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/navigator-cancelKeyLock.https.html View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +12 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/navigator-requestKeyLock-two-parallel-requests.https.html View 1 2 3 4 5 6 7 8 9 1 chunk +15 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/navigator-requestKeyLock-two-sequential-requests.https.html View 1 2 3 4 5 6 7 8 9 1 chunk +14 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/navigator-requestKeyLock.https.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +13 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/virtual/service-worker-navigation-preload-disabled/webexposed/global-interface-listing-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 2 chunks +2 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 2 chunks +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +1 line, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/keyboard_lock/BUILD.gn View 1 1 chunk +12 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +73 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 1 chunk +113 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.idl View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +23 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/modules_idl_files.gni View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5 View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/public/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +1 line, -0 lines 0 comments Download
A third_party/WebKit/public/platform/modules/keyboard_lock/OWNERS View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +4 lines, -0 lines 0 comments Download
A third_party/WebKit/public/platform/modules/keyboard_lock/keyboard_lock.mojom View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +21 lines, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 270 (214 generated)
whywhat
https://codereview.chromium.org/2805763004/diff/240001/third_party/WebKit/Source/core/frame/Navigator.idl File third_party/WebKit/Source/core/frame/Navigator.idl (right): https://codereview.chromium.org/2805763004/diff/240001/third_party/WebKit/Source/core/frame/Navigator.idl#newcode35 third_party/WebKit/Source/core/frame/Navigator.idl:35: void cancelKeyLock(); Drive-by review: I don't think we often ...
6 months, 2 weeks ago (2017-04-10 04:32:45 UTC) #70
Hzj_jie
On 2017/04/10 04:32:45, whywhat wrote: > https://codereview.chromium.org/2805763004/diff/240001/third_party/WebKit/Source/core/frame/Navigator.idl > File third_party/WebKit/Source/core/frame/Navigator.idl (right): > > https://codereview.chromium.org/2805763004/diff/240001/third_party/WebKit/Source/core/frame/Navigator.idl#newcode35 > ...
6 months, 2 weeks ago (2017-04-10 21:08:44 UTC) #71
whywhat
On 2017/04/10 at 21:08:44, zijiehe wrote: > On 2017/04/10 04:32:45, whywhat wrote: > > https://codereview.chromium.org/2805763004/diff/240001/third_party/WebKit/Source/core/frame/Navigator.idl ...
6 months, 2 weeks ago (2017-04-10 21:22:14 UTC) #72
whywhat
6 months, 2 weeks ago (2017-04-10 21:22:26 UTC) #74
Hzj_jie
On 2017/04/10 21:22:14, whywhat wrote: > On 2017/04/10 at 21:08:44, zijiehe wrote: > > On ...
6 months, 2 weeks ago (2017-04-10 21:34:38 UTC) #75
whywhat
On 2017/04/10 at 21:34:38, zijiehe wrote: > On 2017/04/10 21:22:14, whywhat wrote: > > On ...
6 months, 2 weeks ago (2017-04-10 21:51:48 UTC) #76
Hzj_jie
https://codereview.chromium.org/2805763004/diff/240001/third_party/WebKit/Source/core/frame/Navigator.idl File third_party/WebKit/Source/core/frame/Navigator.idl (right): https://codereview.chromium.org/2805763004/diff/240001/third_party/WebKit/Source/core/frame/Navigator.idl#newcode35 third_party/WebKit/Source/core/frame/Navigator.idl:35: void cancelKeyLock(); On 2017/04/10 04:32:45, whywhat wrote: > Drive-by ...
6 months, 2 weeks ago (2017-04-11 03:46:14 UTC) #81
dglazkov
Could the tests be layout tests? https://chromium.googlesource.com/chromium/src/+/master/docs/testing/layout_tests.md If so, this is how we would typically ...
6 months, 2 weeks ago (2017-04-11 04:02:21 UTC) #82
whywhat
I think in general you can avoid lots of ChromeClient plumbing by registering a separate ...
6 months, 2 weeks ago (2017-04-11 14:52:33 UTC) #85
Hzj_jie
https://codereview.chromium.org/2805763004/diff/260001/content/browser/keyboard_lock/keyboard_lock_browsertest.cc File content/browser/keyboard_lock/keyboard_lock_browsertest.cc (right): https://codereview.chromium.org/2805763004/diff/260001/content/browser/keyboard_lock/keyboard_lock_browsertest.cc#newcode50 content/browser/keyboard_lock/keyboard_lock_browsertest.cc:50: // TODO: This test should receive "Succeeded" once the ...
6 months, 2 weeks ago (2017-04-12 02:51:06 UTC) #102
whywhat
Mostly nits and missing comments atm. Based on the failing bots results, I think you ...
6 months, 1 week ago (2017-04-12 16:02:13 UTC) #105
whywhat
https://codereview.chromium.org/2805763004/diff/360001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/360001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode50 third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:50: GetSupplementable()->GetFrame()->GetInterfaceProvider()->GetInterface( You need to check if GetFrame() returns null ...
6 months, 1 week ago (2017-04-12 21:46:11 UTC) #111
Hzj_jie
https://codereview.chromium.org/2805763004/diff/320001/content/browser/frame_host/render_frame_host_impl.cc File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2805763004/diff/320001/content/browser/frame_host/render_frame_host_impl.cc#newcode18 content/browser/frame_host/render_frame_host_impl.cc:18: #include "base/strings/utf_string_conversions.h" On 2017/04/12 16:02:12, whywhat wrote: > nit: ...
6 months, 1 week ago (2017-04-13 00:38:54 UTC) #114
Hzj_jie
I may be wrong, but it seems like the test failures in chromeos do not ...
6 months, 1 week ago (2017-04-13 01:17:29 UTC) #117
whywhat
On 2017/04/13 at 01:17:29, zijiehe wrote: > I may be wrong, but it seems like ...
6 months, 1 week ago (2017-04-13 02:26:37 UTC) #118
whywhat
So I patched your CL locally and tried to break the layout tests as I ...
6 months, 1 week ago (2017-04-13 02:34:37 UTC) #120
Hzj_jie
Thank you Anton for the review and comments. Add Philips and Avi to cover both ...
6 months, 1 week ago (2017-04-13 22:24:25 UTC) #128
whywhat
non-owners lgtm https://codereview.chromium.org/2805763004/diff/380001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html File third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html (right): https://codereview.chromium.org/2805763004/diff/380001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html#newcode20 third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/idl-KeyboardLock.html:20: promise_rejects(t, null, p2, On 2017/04/13 at 22:24:25, ...
6 months, 1 week ago (2017-04-13 22:40:24 UTC) #129
Avi (ping after 24h)
content lgtm
6 months, 1 week ago (2017-04-13 22:55:18 UTC) #131
foolip
Which files should I look at? Too many reviewers to guess :)
6 months, 1 week ago (2017-04-14 09:50:25 UTC) #134
whywhat
Philip, I think you could review everything in third_party/WebKit (esp. the layout tests). Daniel's LGTM ...
6 months, 1 week ago (2017-04-14 16:15:53 UTC) #136
Hzj_jie
Sorry for that Philip. Anton has reviewed and LGTMed all the changes in this CL. ...
6 months, 1 week ago (2017-04-14 18:12:36 UTC) #137
foolip
I've looked at all of third_party/WebKit/, but it would be good if another owner was ...
6 months, 1 week ago (2017-04-17 08:46:05 UTC) #138
haraken
I just scanned third_party/WebKit/. Yeah, foolip's comments need to be addressed. https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/OWNERS File third_party/WebKit/Source/modules/keyboard_lock/OWNERS (right): ...
6 months, 1 week ago (2017-04-17 14:15:57 UTC) #140
dcheng
https://codereview.chromium.org/2805763004/diff/420001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc File content/browser/keyboard_lock/keyboard_lock_service_impl.cc (right): https://codereview.chromium.org/2805763004/diff/420001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc#newcode19 content/browser/keyboard_lock/keyboard_lock_service_impl.cc:19: impl->binding_.reset(new mojo::Binding<blink::mojom::KeyboardLockService>( Then here, you can call Bind(std::move(request)) https://codereview.chromium.org/2805763004/diff/420001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc#newcode26 ...
6 months, 1 week ago (2017-04-17 23:07:52 UTC) #145
Hzj_jie
https://codereview.chromium.org/2805763004/diff/420001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc File content/browser/keyboard_lock/keyboard_lock_service_impl.cc (right): https://codereview.chromium.org/2805763004/diff/420001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc#newcode19 content/browser/keyboard_lock/keyboard_lock_service_impl.cc:19: impl->binding_.reset(new mojo::Binding<blink::mojom::KeyboardLockService>( On 2017/04/17 23:07:52, dcheng wrote: > Then ...
6 months, 1 week ago (2017-04-18 02:26:09 UTC) #153
foolip
Would you like to land these changes before the spec changes are made? It would ...
6 months, 1 week ago (2017-04-18 05:10:58 UTC) #156
haraken
https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/Source/modules/keyboard_lock/OWNERS File third_party/WebKit/Source/modules/keyboard_lock/OWNERS (right): https://codereview.chromium.org/2805763004/diff/500001/third_party/WebKit/Source/modules/keyboard_lock/OWNERS#newcode1 third_party/WebKit/Source/modules/keyboard_lock/OWNERS:1: zijiehe@chromium.org On 2017/04/18 05:10:57, foolip OOO on Wednesday wrote: ...
6 months, 1 week ago (2017-04-18 07:38:36 UTC) #157
Hzj_jie
I have talked offline with Jamie, most of the issues should be addressed in the ...
6 months, 1 week ago (2017-04-19 00:45:55 UTC) #165
Hzj_jie
Philip, do you happen to know why the idlharness test may fail? I cannot reproduce ...
6 months ago (2017-04-20 21:23:58 UTC) #168
foolip
https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode60 third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:60: state, V8String(state->GetIsolate(), "Browser is not connecting.")); On 2017/04/19 00:45:54, ...
6 months ago (2017-04-21 06:08:32 UTC) #171
foolip
6 months ago (2017-04-21 06:08:43 UTC) #172
foolip
On 2017/04/20 21:23:58, Hzj_jie wrote: > Philip, do you happen to know why the idlharness ...
6 months ago (2017-04-21 06:12:10 UTC) #173
Hzj_jie
https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/420001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode60 third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:60: state, V8String(state->GetIsolate(), "Browser is not connecting.")); On 2017/04/21 06:08:32, ...
6 months ago (2017-04-22 01:20:26 UTC) #184
Hzj_jie
Hi, Philip, is the fix in idlharness.js reasonable?
6 months ago (2017-04-24 18:45:14 UTC) #187
foolip
On 2017/04/24 18:45:14, Hzj_jie wrote: > Hi, Philip, is the fix in idlharness.js reasonable? It ...
6 months ago (2017-04-24 19:05:44 UTC) #188
Hzj_jie
On 2017/04/24 19:05:44, foolip_UTC7 wrote: > On 2017/04/24 18:45:14, Hzj_jie wrote: > > Hi, Philip, ...
6 months ago (2017-04-24 19:08:34 UTC) #189
foolip
On 2017/04/24 19:08:34, Hzj_jie wrote: > On 2017/04/24 19:05:44, foolip_UTC7 wrote: > > On 2017/04/24 ...
6 months ago (2017-04-24 19:18:54 UTC) #190
Hzj_jie
On 2017/04/24 19:18:54, foolip_UTC7 wrote: > On 2017/04/24 19:08:34, Hzj_jie wrote: > > On 2017/04/24 ...
6 months ago (2017-04-24 19:55:37 UTC) #193
foolip
third_party/WebKit/LayoutTests/ LGTM % nit. Please also add keyboard-lock to W3CImportExpectations in this CL. That's how ...
6 months ago (2017-04-25 06:34:15 UTC) #196
Hzj_jie
Since Daniel is OOO, adding Elliott to this change. https://codereview.chromium.org/2805763004/diff/640001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/navigator-requestKeyLock.https.html File third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/navigator-requestKeyLock.https.html (right): https://codereview.chromium.org/2805763004/diff/640001/third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/navigator-requestKeyLock.https.html#newcode9 third_party/WebKit/LayoutTests/external/wpt/keyboard-lock/navigator-requestKeyLock.https.html:9: ...
6 months ago (2017-04-25 19:27:32 UTC) #200
haraken
From the implementation perspective, WebKit LGTM. https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode79 third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:79: bool NavigatorKeyboardLock::EnsureServiceConnected(String* error_message) ...
6 months ago (2017-04-26 06:56:03 UTC) #203
dcheng
https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc File content/browser/keyboard_lock/keyboard_lock_service_impl.cc (right): https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc#newcode13 content/browser/keyboard_lock/keyboard_lock_service_impl.cc:13: : binding_(this, std::move(request)) {} Nit: #include <utility> https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc#newcode21 content/browser/keyboard_lock/keyboard_lock_service_impl.cc:21: ...
5 months, 4 weeks ago (2017-04-26 13:27:18 UTC) #204
Hzj_jie
https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc File content/browser/keyboard_lock/keyboard_lock_service_impl.cc (right): https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc#newcode13 content/browser/keyboard_lock/keyboard_lock_service_impl.cc:13: : binding_(this, std::move(request)) {} On 2017/04/26 13:27:17, dcheng (OOO ...
5 months, 4 weeks ago (2017-04-26 22:05:57 UTC) #209
dcheng
https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc File content/browser/keyboard_lock/keyboard_lock_service_impl.cc (right): https://codereview.chromium.org/2805763004/diff/660001/content/browser/keyboard_lock/keyboard_lock_service_impl.cc#newcode21 content/browser/keyboard_lock/keyboard_lock_service_impl.cc:21: new KeyboardLockServiceImpl(std::move(request)); On 2017/04/26 22:05:56, Hzj_jie wrote: > On ...
5 months, 4 weeks ago (2017-04-27 02:53:11 UTC) #219
Hzj_jie
Though (some of ) the trybots are down, the latest change passes all the tests. ...
5 months, 4 weeks ago (2017-04-28 00:10:31 UTC) #237
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2805763004/760001
5 months, 4 weeks ago (2017-04-28 02:24:39 UTC) #242
dcheng
ipc lgtm with comments addressed https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode52 third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:52: if (!EnsureServiceConnected(&error_message)) { On ...
5 months, 4 weeks ago (2017-04-28 02:24:49 UTC) #243
Hzj_jie
https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/660001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode52 third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:52: if (!EnsureServiceConnected(&error_message)) { On 2017/04/28 02:24:49, dcheng (OOO through ...
5 months, 4 weeks ago (2017-04-28 04:53:01 UTC) #247
dcheng
https://codereview.chromium.org/2805763004/diff/780001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/780001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode55 third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:55: // not connected. This isn't a recoverable error -- ...
5 months, 3 weeks ago (2017-04-28 15:33:45 UTC) #250
Hzj_jie
https://codereview.chromium.org/2805763004/diff/780001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp (right): https://codereview.chromium.org/2805763004/diff/780001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode55 third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:55: // not connected. On 2017/04/28 15:33:45, dcheng (OOO through ...
5 months, 3 weeks ago (2017-04-28 19:16:02 UTC) #253
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2805763004/820001
5 months, 3 weeks ago (2017-04-28 21:55:57 UTC) #262
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2805763004/820001
5 months, 3 weeks ago (2017-04-28 22:11:49 UTC) #265
commit-bot: I haz the power
Committed patchset #27 (id:820001) as https://chromium.googlesource.com/chromium/src/+/6016778c99f93e8f91eec1d1f20fc44642d3bfde
5 months, 3 weeks ago (2017-04-28 22:31:30 UTC) #268
dcheng
On 2017/04/28 19:16:02, Hzj_jie wrote: > https://codereview.chromium.org/2805763004/diff/780001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp > File third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp > (right): > > https://codereview.chromium.org/2805763004/diff/780001/third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp#newcode55 ...
5 months, 3 weeks ago (2017-04-29 01:58:30 UTC) #269
Hzj_jie
5 months, 3 weeks ago (2017-04-29 02:06:10 UTC) #270
Message was sent while issue was closed.
On 2017/04/29 01:58:30, dcheng (OOO through May 2) wrote:
> On 2017/04/28 19:16:02, Hzj_jie wrote:
> >
>
https://codereview.chromium.org/2805763004/diff/780001/third_party/WebKit/Sou...
> > File
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/2805763004/diff/780001/third_party/WebKit/Sou...
> >
third_party/WebKit/Source/modules/keyboard_lock/NavigatorKeyboardLock.cpp:55:
> //
> > not connected.
> > On 2017/04/28 15:33:45, dcheng (OOO through May 2) wrote:
> > > This isn't a recoverable error -- once frame is null, it will always be
> null.
> > > 
> > > IMO we should keep the:
> > > 
> > > if (!frame)
> > >   return false;
> > > frame->GetInterfaceProvider()->GetInterface(...);
> > > return true;
> > > 
> > > part in a helper. Then requestKeyLock() will look like:
> > > 
> > > if (!EnsureServiceConnected()) {
> > >   return ScriptPromise::Reject(state, ...);
> > > }
> > > 
> > > and cancelKeyLock() will look like:
> > > 
> > > if (!EnsureServiceConnected())
> > >   return;
> > 
> > I do not really understand the insight: why would frame be null? I used to
> > believe it happens once the tab itself is detaching, but it looks like my
> > understanding is totally wrong.
> 
> Basically, frame will be null if the Document is not the active Document in a
> browsing context. A browsing context can be top-level (like the main frame in
a
> tab) or it can be in a subframe (which can be detached by removing the iframe
> element from the DOM). Once a Document is detached, it can never be reattached
> to the DOM. For example, if you have an iframe and remove it from the DOM, and
> then re-insert it, you get a new Frame / Document / Navigator / etc. I'm happy
> to explain more when I'm back in the office, as frame/document lifetimes are
> pretty complex.

Got you. We may talk about the detail later: I am curious whether the lifetime
of an iframe is also trackable, if it becomes complex, we may simply disable
this feature in iframe. (For security concern, I believe it's also a reasonable
choice.)
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 81bcdb8aa