Initial stub for keyboard.getLayoutMap [chromium/src : master]
Gary Kacmarcik (Gerrit)
foolip: AIUI, there is no way to set the keyboard layout during WPT, and we can't assume that WPTs will be run with any particular layout (like en-US) selected. Any thoughts on how we can test this API properly once I start landing real implementations?
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/w3c/web-platform-tests/pull/10495.
If this CL lands and Travis CI upstream is green, we will auto-merge the PR.
Note: Please check the Travis CI status (at the bottom of the PR) before landing this CL and only land this CL if the status is green. Otherwise a human needs to step in and resolve it manually. (This may be automated in the future, see https://crbug.com/711447)
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_platform_tests.md#Automatic-export-process
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "rebaseline": https://github.com/w3c/web-platform-tests/pull/10495
Gary Kacmarcik (Gerrit)
>Ping? Is anyone there?
>No, we're all at BlinkOn.
>Oh... OK...
Daniel Cheng (Gerrit)
7 comments:
File third_party/blink/renderer/modules/keyboard/keyboard_layout_map.h:
Patch Set #3, Line 21:
explicit KeyboardLayoutMap();Nit: no explicit here
Patch Set #3, Line 28:
String At(String key) { return layout_map_.at(key); }const String&. Ditto below.
Also, what calls these methods?
Patch Set #3, Line 34:
PairIterable<String, String>::IterationSource* StartIteration(Nit: add a comment that this is implementing Maplike
File third_party/blink/renderer/modules/keyboard/keyboard_layout_map.cc:
auto&
Nit: = 0; here
(I believe it's not actually initialized at all atm)
File third_party/blink/renderer/modules/keyboard/keyboard_layout_map_promise.h:
Patch Set #3, Line 31:
KeyboardLayoutMapPromise(ScriptState*);Nit: explicit
File third_party/blink/renderer/modules/keyboard/keyboard_layout_map_promise.cc:
Patch Set #3, Line 20:
kmap_promise->GetTaskRunner()->PostTask(Is it possible that this will be called on a detached context, i.e. an iframe removes itself then calls navigation.keyboard.getLayoutMap()? Then I think GetTaskRunner() might not work, since it would return null (as there would be no execution context at that point).
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Gary Kacmarcik (Gerrit)
6 comments:
File third_party/blink/renderer/modules/keyboard/keyboard_layout_map.h:
Patch Set #3, Line 21:
KeyboardLayoutMap();Nit: no explicit here
Done
Patch Set #3, Line 28:
void Trace(blink::Visitor* visitor) { ScriptWrappable::Trace(visitor); }const String&. Ditto below. […]
Nothing anymore. Removed.
File third_party/blink/renderer/modules/keyboard/keyboard_layout_map.cc:
auto&
Done
Nit: = 0; here […]
Done
File third_party/blink/renderer/modules/keyboard/keyboard_layout_map_promise.h:
Patch Set #3, Line 31:
explicit KeyboardLayoutMapPromise(ScriptState*);Nit: explicit
Done
File third_party/blink/renderer/modules/keyboard/keyboard_layout_map_promise.cc:
Patch Set #3, Line 20:
ExecutionContext* context = kmap_promise->GetExecutionContext();Is it possible that this will be called on a detached context, i.e. […]
I put checks for context and taskRunner, but I think the taskRunner check might be overkill.
IIUC, then we'll want to make a similar change to modules/clipboard/clipboard_promise.cc?
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Review comments": https://github.com/w3c/web-platform-tests/pull/10495
Daniel Cheng (Gerrit)
3 comments:
File third_party/blink/renderer/modules/keyboard/keyboard_layout_map.h:
Patch Set #4, Line 31:
maplikeNit: MapLike to make it easy to do an exact string search
File third_party/blink/renderer/modules/keyboard/keyboard_layout_map_promise.cc:
Patch Set #3, Line 20:
ExecutionContext* context = kmap_promise->GetExecutionContext();
I put checks for context and taskRunner, but I think the taskRunner check might be overkill. […]
Perhaps!
But can we write a test to verify that this crashes without checking for context and passes with the check?
File third_party/blink/renderer/modules/keyboard/keyboard_layout_map_promise.cc:
Patch Set #4, Line 26:
if (taskRunner) {Yeah no need to check taskRunner (also this should be task_runner =)
I would structure this as:
if (context) {
// stuff
} else {
resolver->Reject();
}
return resolver->Promise();
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Gary Kacmarcik (Gerrit)
2 comments:
Patch Set #4, Line 31:
maplikeNit: MapLike to make it easy to do an exact string search
Lowercase 'l' though
File third_party/blink/renderer/modules/keyboard/keyboard_layout_map_promise.cc:
Patch Set #4, Line 26:
if (taskRunner) {
Yeah no need to check taskRunner (also this should be task_runner =) […]
We discussed this a few times over the past week WRT why context is not null inside the detached iframe.
I've rewritten this code to make mojom calls so I no longer need to use a taskRunner.
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Gary Kacmarcik (Gerrit)
Gary Kacmarcik would like Scott Violet to review this change.
Initial stub for keyboard.getLayoutMap
A proper implementation for this API will be in follow up cls,
a separate cl per platform.
Bug: 832811
Change-Id: I17dee21095665739a3f1986e48a2182d6b6fab00
---
M content/browser/keyboard_lock/keyboard_lock_service_impl.cc
M content/browser/keyboard_lock/keyboard_lock_service_impl.h
M content/browser/renderer_host/render_widget_host_view_base.cc
M content/browser/renderer_host/render_widget_host_view_base.h
M content/public/browser/render_widget_host_view.h
A third_party/WebKit/LayoutTests/external/wpt/interfaces/keyboard-layout-map.idl
A third_party/WebKit/LayoutTests/external/wpt/keyboard-layout-map/OWNERS
A third_party/WebKit/LayoutTests/external/wpt/keyboard-layout-map/idlharness.https.html
A third_party/WebKit/LayoutTests/external/wpt/keyboard-layout-map/keyboard-layout-map.https.html
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt
M third_party/blink/public/platform/modules/keyboard_lock/keyboard_lock.mojom
M third_party/blink/public/platform/web_feature.mojom
M third_party/blink/renderer/modules/keyboard/BUILD.gn
M third_party/blink/renderer/modules/keyboard/keyboard.cc
M third_party/blink/renderer/modules/keyboard/keyboard.h
M third_party/blink/renderer/modules/keyboard/keyboard.idl
A third_party/blink/renderer/modules/keyboard/keyboard_layout.cc
A third_party/blink/renderer/modules/keyboard/keyboard_layout.h
A third_party/blink/renderer/modules/keyboard/keyboard_layout_map.cc
A third_party/blink/renderer/modules/keyboard/keyboard_layout_map.h
A third_party/blink/renderer/modules/keyboard/keyboard_layout_map.idl
M third_party/blink/renderer/modules/modules_idl_files.gni
M third_party/blink/renderer/platform/runtime_enabled_features.json5
M tools/metrics/histograms/enums.xml
24 files changed, 395 insertions(+), 1 deletion(-)
Gary Kacmarcik (Gerrit)
+sky@ for the content/browser side of things.
This uses the Keyboard Lock Mojo service and the renderer/browser code is patterned after the keyboard lock code.
If this approach is acceptable to everyone, then I'll have a followup CL that renames the "KeyboardLock" mojo service to be a "Keyboard" mojo service (I didn't want to clutter this cl).
Gary Kacmarcik (Gerrit)
Gary Kacmarcik uploaded patch set #6 to this change.
Initial stub for keyboard.getLayoutMap
Spec: https://wicg.github.io/keyboard-map/
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Edit commit message": https://github.com/w3c/web-platform-tests/pull/10495
Scott Violet (Gerrit)
3 comments:
File content/public/browser/render_widget_host_view.h:
// Return a mapping dictionary from |code| to |key| values for the highest
// priority ASCII-capable layout.Please update this comment. This returns a boolean. Also, I'm not sure what this means. What is the priority layout?
File third_party/blink/public/platform/modules/keyboard_lock/keyboard_lock.mojom:
Patch Set #6, Line 19:
structIs there a reason you're using a struct, and not a map? If you go with the struct, is there a reason not to include the status in the struct?
Patch Set #6, Line 36:
GetKeyboardLayoutMapPlease add a description.
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Gary Kacmarcik (Gerrit)
Gary Kacmarcik uploaded patch set #7 to this change.
Initial stub for keyboard.getLayoutMap
This includes the WebIDL defs, initial WebPlatformTests and mojom
over to stubs in the RenderWidgetHost.
Spec: https://wicg.github.io/keyboard-map/
Gary Kacmarcik (Gerrit)
Gary Kacmarcik uploaded patch set #9 to this change.
Initial stub for navigator.keyboard.getLayoutMap
This includes the WebIDL defs, initial WebPlatformTests and mojom
over to stubs in the RenderWidgetHost.
Spec: https://wicg.github.io/keyboard-map/
24 files changed, 404 insertions(+), 1 deletion(-)
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Gary Kacmarcik (Gerrit)
3 comments:
// Return a mapping dictionary fr
Please update this comment. This returns a boolean. Also, I'm not sure what this means. […]
There is a stack of keyboard layouts with the topmost (the "highest priority") one being the current layout. If the topmost layout is not ASCII-capable (e.g.: current layout is Arabic or Japanese), then stack of layouts is scanned from top to bottom until the first ASCII-capable one is found.
I expanded the comment a bit and updated the signature to return a "base::flat_map<std::string, std::string>*" (which will have the mapping when the stub is fleshed out).
File third_party/blink/public/platform/modules/keyboard_lock/keyboard_lock.mojom:
Patch Set #6, Line 19:
struct
Is there a reason you're using a struct, and not a map? If you go with the struct, is there a reason […]
I was following a pattern that I saw in a few other interfaces[1], where there was a separate Status value and a Struct with more info.
I've updated to move the status into the Result struct.
[1] eg: Authenticator: https://cs.chromium.org/chromium/src/third_party/blink/public/platform/modules/webauth/authenticator.mojom?l=229
WebBluetooth: https://cs.chromium.org/chromium/src/third_party/blink/public/platform/modules/bluetooth/web_bluetooth.mojom?l=151
Please add a description.
Done
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Edit commit message": https://github.com/w3c/web-platform-tests/pull/10495
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Scott Violet (Gerrit)
3 comments:
File content/browser/renderer_host/render_widget_host_view_base.cc:
Patch Set #9, Line 233:
map = nullptr;Is there a reason to have this line? It doesn't really do anything.
File content/public/browser/render_widget_host_view.h:
Patch Set #9, Line 183:
base::flat_map<std::string, std::string>Could you instead return the map?
File third_party/blink/public/platform/modules/keyboard_lock/keyboard_lock.mojom:
Patch Set #9, Line 37:
|code| to |key|Generally |s surround parameters. There is no key/value param, so using | here is confusing.
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Gary Kacmarcik (Gerrit)
3 comments:
File content/browser/renderer_host/render_widget_host_view_base.cc:
Patch Set #9, Line 233:
return nullptr
Is there a reason to have this line? It doesn't really do anything.
Could you instead return the map?
Done
File third_party/blink/public/platform/modules/keyboard_lock/keyboard_lock.mojom:
Patch Set #9, Line 37:
"code" to "key"Generally |s surround parameters. There is no key/value param, so using | here is confusing.
Done
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Return map from GetKeyboardLayoutMap()": https://github.com/w3c/web-platform-tests/pull/10495
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "sync/merge": https://github.com/w3c/web-platform-tests/pull/10495
Gary Kacmarcik (Gerrit)
Gary Kacmarcik uploaded patch set #12 to this change.
[KeyboardMap] Initial stub for navigator.keyboard.getLayoutMap
This includes the WebIDL defs, initial WebPlatformTests and mojom
over to stubs in the RenderWidgetHost.
Spec: https://wicg.github.io/keyboard-map/
24 files changed, 402 insertions(+), 1 deletion(-)
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Edit commit message": https://github.com/w3c/web-platform-tests/pull/10495
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Scott Violet (Gerrit)
1 comment:
File content/browser/renderer_host/render_widget_host_view_base.h:
Patch Set #12, Line 121:
base::flat_map<std::string, std::string>*Sorry for not being clear. I'm suggesting this return a value, not the pointer (otherwise ownership isn't clear).
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Scott Violet (Gerrit)
I have to ask, what is "Blink WPT Bot"?
Gary Kacmarcik (Gerrit)
Patch Set 12:
I have to ask, what is "Blink WPT Bot"?
WebPlatformTests (in third_party/WebKit/LayoutTests/external/wpt).
They are automatically sync'ed with the external WPT repo by the bot. These tests are external because they are shared by all browser vendors.
Gary Kacmarcik (Gerrit)
1 comment:
Patch Set #12, Line 121:
base::flat_map<std::string, std::string>
Sorry for not being clear. […]
Done
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Return map instead of map*": https://github.com/w3c/web-platform-tests/pull/10495
Daniel Cheng (Gerrit)
Patch Set 12:
I have to ask, what is "Blink WPT Bot"?
WPT = Web Platform Tests. They're a set of test suites for various web platform features, maintained upstream at https://github.com/w3c/web-platform-tests/. The Blink WPT bot helps push test updates to the upstream repository (and also sync changes back down). I haven o
7 comments:
File content/browser/keyboard_lock/keyboard_lock_service_impl.cc:
Patch Set #12, Line 137:
response->layout_map.insert(std::make_pair("KeyE", "e"));Nit: please use emplace instead of insert + std::make_pair
File third_party/blink/public/platform/modules/keyboard_lock/keyboard_lock.mojom:
Returns a mapping of KeyboardEvent "code" to "key" values, based on the
// highest-priority ASCII-capable layout in the list of currently installed
// keyboard layouts.I'd suggest documenting the map on line 21, and then maybe rewriting this to:
"Requests the keyboard layout map of the highest-priority ASCII-capable layout currently installed."
Patch Set #12, Line 40:
GetKeyboardLayoutMap() => (GetKeyboardLayoutMapResult result);I assume we've determined that we can't get keyboard layouts from the renderer?
File third_party/blink/renderer/modules/keyboard/keyboard_layout.h:
Patch Set #12, Line 25:
virtual ~KeyboardLayout() = default;This should be ~KeyboardLayout() override = default;
(I'm actually surprised that this isn't warning and will have to dig in a bit more). Perhaps it has to do with the fact that the parent classes are templates?
File third_party/blink/renderer/modules/keyboard/keyboard_layout_map.h:
Patch Set #12, Line 23:
void AddEntry(String, String);const String&, const String& please. Also, please name the arguments here: I'd probably try to give it a more descriptive name than key/value as well.
Though I'm wondering: can we just make the constructor explicit and take the map argument, so we don't build it up incrementally here? Then we can just make layout_map_ const.
File third_party/blink/renderer/modules/keyboard/keyboard_layout_map.cc:
Patch Set #12, Line 39:
unsigned current_index_ = 0;If we pass the map in the ctor and make it const, then we can just hold a Member<> to the KeyboardLayoutMap object. Then we can just hold an iterator to the HashMap here and iterate through without copying the entire map into parallel Vectors.
Patch Set #12, Line 53:
if (layout_map_.Contains(key)) {Nit: it's be slightly better to write this as:
auto it = layout_map_.find(key);
if (it == layout_map_.end())
return false;value = it->second;
return true;Since that only does one map lookup.
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Scott Violet (Gerrit)
Patch set 13:Code-Review +1
1 comment:
File content/public/browser/render_widget_host_view.h:
Patch Set #13, Line 179:
|code| to |key|Again, please remove |s here. In other words, 'from keyboard code to key values'.
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Update maplike implementation": https://github.com/w3c/web-platform-tests/pull/10495
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Have IterationSource keep Member to KeyboardLayoutMap": https://github.com/w3c/web-platform-tests/pull/10495
Gary Kacmarcik (Gerrit)
8 comments:
File content/browser/keyboard_lock/keyboard_lock_service_impl.cc:
Patch Set #12, Line 137:
response->layout_map.emplace("KeyE", "e");
Nit: please use emplace instead of insert + std::make_pair
Done. Thanks! I didn't know about that nicer form.
File content/public/browser/render_widget_host_view.h:
Patch Set #13, Line 179:
code to key val
Again, please remove |s here. In other words, 'from keyboard code to key values'.
Done. Sorry for missing this one.
File third_party/blink/public/platform/modules/keyboard_lock/keyboard_lock.mojom:
// Removes all reserved keys. This function is expected to never fail.
CancelKeyboardLock()I'd suggest documenting the map on line 21, and then maybe rewriting this to: […]
Done
I assume we've determined that we can't get keyboard layouts from the renderer?
The X keyboard layout apis need a display, which I couldn't find access to in the renderer.
File third_party/blink/renderer/modules/keyboard/keyboard_layout.h:
Patch Set #12, Line 25:
virtual ~KeyboardLayout() = default;
This should be ~KeyboardLayout() override = default; […]
There's nothing to override (and possibly because this is a 'final' class).
Using ~KeyboardLayout() override = default;keyboard_layout.h:25:21: error: only virtual member functions can be marked 'override'
~KeyboardLayout() override = default;
^~~~~~~~~Using virtual ~KeyboardLayout() override = default;
keyboard_layout.h:25:11: error: '~KeyboardLayout' marked 'override' but does not override any member functions
virtual ~KeyboardLayout() override = default;
^
keyboard_layout.h:25:3: error: [chromium-style] 'virtual' is redundant; 'override' implies 'virtual'.
virtual ~KeyboardLayout() override = default;
^~~~~~~~
File third_party/blink/renderer/modules/keyboard/keyboard_layout_map.h:
Patch Set #12, Line 23:
HashMap<String, String>::const_iterator begin();const String&, const String& please. […]
Changed constructor and removed this.
File third_party/blink/renderer/modules/keyboard/keyboard_layout_map.cc:
If we pass the map in the ctor and make it const, then we can just hold a Member<> to the KeyboardLa […]
Done
Patch Set #12, Line 53:
bool KeyboardLayoutMap::GetMapEntry(ScriptState*,Nit: it's be slightly better to write this as: […]
Done
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Cheng (Gerrit)
4 comments:
Patch Set #12, Line 25:
virtual ~KeyboardLayout() = default;
There's nothing to override (and possibly because this is a 'final' class). […]
Sorry I thought I discarded this draft, but I guess not!
File third_party/blink/renderer/modules/keyboard/keyboard_layout.cc:
Patch Set #15, Line 54:
keyboard_layout_map_ = new KeyboardLayoutMap(result->layout_map);Does this need to be a member? Maybe it should just be passed directly to the script promise resolver.
File third_party/blink/renderer/modules/keyboard/keyboard_layout_map.h:
Patch Set #15, Line 21:
KeyboardLayoutMap(HashMap<String, String> map);Pass by const reference please.
Patch Set #15, Line 24:
HashMap<String, String>::const_iterator end();Let's just add a Map() getter that returns a const ref.
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Philip Jägenstedt (Gerrit)
LGTM % test nits.
Patch set 15:Code-Review +1
7 comments:
File third_party/WebKit/LayoutTests/external/wpt/interfaces/keyboard-layout-map.idl:
Patch Set #12, Line 1:
interfaceCan you name this file keyboard-map.idl and the directory keyboard-map/, to match the spec name? (https://wicg.github.io/keyboard-map/)
File third_party/WebKit/LayoutTests/external/wpt/keyboard-layout-map/OWNERS:
Patch Set #12, Line 1:
garykacCan you also add team and component? And WPT-NOTIFY if you'd like that.
File third_party/WebKit/LayoutTests/external/wpt/keyboard-layout-map/idlharness.https.html:
Patch Set #12, Line 21:
Keyboard: ['navigator.keyboard'],Can you add a KeyboardLayoutMap instance as well?
Patch Set #12, Line 31:
returnWith just a single file being fetched things can be simplified a bit. compat/interfaces.any.js and wake-lock/interfaces.https.html are pretty good examples to copy that have less boilerplate.
File third_party/WebKit/LayoutTests/external/wpt/keyboard-layout-map/keyboard-layout-map.https.html:
Patch Set #12, Line 9:
assert_not_equalsThe idlharness.js test should cover existence. Not sure if it'll check the type or [SameObject], though. In any event, splitting the two remaining assertions into two tests is probably good, since the [SameObject] stuff is easy to get wrong and making the problem obvious would be good.
Patch Set #12, Line 18:
navigatorThis also tests that the promise resolves instead of rejects. Given that the following test covers everything except "returns a Promise", maybe just fold the instanceof assertion into the following test?
Is there nothing more that can be asserted about this object? Or are those tests you'd expect to add in coming CLs?
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Gary Kacmarcik (Gerrit)
3 comments:
File third_party/blink/renderer/modules/keyboard/keyboard_layout.cc:
Patch Set #15, Line 54:
script_promise_resolver_->Resolve(Does this need to be a member? Maybe it should just be passed directly to the script promise resolve […]
Nice. I didn't notice that the refactoring made that unnecessary.
Done.
File third_party/blink/renderer/modules/keyboard/keyboard_layout_map.h:
Patch Set #15, Line 21:
KeyboardLayoutMap(const HashMap<String, String>& map);
Pass by const reference please.
Done
Patch Set #15, Line 24:
size_t size() const { return layout_map_.size(); }
Let's just add a Map() getter that returns a const ref.
Removed since I changed the constructor to take a start/end iterator, so this is no longer necessary.
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Cleanup map iterator": https://github.com/w3c/web-platform-tests/pull/10495
Daniel Cheng (Gerrit)
1 comment:
File third_party/blink/renderer/modules/keyboard/keyboard_layout_map.cc:
Patch Set #16, Line 39:
const HashMap<String, String>::const_iterator end_;Conceptually I think it'd be nicer to have a smaller object when possible. So I'd rather just expose Map() rather than adding an additional field. You can friend the iteration source if you want to make the getter private.
This seems to match how other IterationSources are implemented.
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Using map_->get().end()": https://github.com/w3c/web-platform-tests/pull/10495
Gary Kacmarcik (Gerrit)
1 comment:
File third_party/blink/renderer/modules/keyboard/keyboard_layout_map.cc:
Patch Set #16, Line 39:
: layout_map_(map) {}Conceptually I think it'd be nicer to have a smaller object when possible. […]
Done
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Add missing & operator": https://github.com/w3c/web-platform-tests/pull/10495
Daniel Cheng (Gerrit)
LGTM with comments addressed
Patch set 18:Code-Review +1
2 comments:
File third_party/blink/public/platform/modules/keyboard_lock/keyboard_lock.mojom:
, based on
// the highest-priority ASCII capable layout in the list of currently
// installed keyboard layouts.Nit: this section probably isn't necessary (it's already documented in the method call itself)
// Returns a mapping of KeyboardEvent "code" to "key" values, based on the
// highest-priority ASCII-capable layout in the list of currently installed
// keyboard layouts.
Remove this?
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Update WPTs": https://github.com/w3c/web-platform-tests/pull/10495
Gary Kacmarcik (Gerrit)
7 comments:
Patch Set #12, Line 1:
interface
Can you name this file keyboard-map. […]
Done
File third_party/WebKit/LayoutTests/external/wpt/keyboard-layout-map/OWNERS:
Patch Set #12, Line 1:
garykacCan you also add team and component? And WPT-NOTIFY if you'd like that.
Patch Set #12, Line 21:
Keyboard: ['navigator.keyboard'],Can you add a KeyboardLayoutMap instance as well?
Done
Patch Set #12, Line 31:
return
With just a single file being fetched things can be simplified a bit. compat/interfaces.any. […]
Done
File third_party/WebKit/LayoutTests/external/wpt/keyboard-layout-map/keyboard-layout-map.https.html:
Patch Set #12, Line 9:
assert_not_equals
The idlharness.js test should cover existence. […]
Done
Patch Set #12, Line 18:
navigator
This also tests that the promise resolves instead of rejects. […]
Done
Is there nothing more that can be asserted about this object? Or are those tests you'd expect to add […]
Unless we assume that the current layout is US (or something else), we can't check the values that are returned.
I added a for-loop over the data and a sanity check that the right types are returned. I intend to add more sanity checks if possible. E.g., I want to add a test that verifies that all the keys are valid 'code' values, but I can't do the same for the dict values (since they can be any string).
Do you know of any way that we can set the current keyboard layout for a test? I was unable to find anything that would allow that.
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Gary Kacmarcik (Gerrit)
2 comments:
, based on
// the highest-priority ASCII capable layout in the list of currently
// installed keyboard layouts.Nit: this section probably isn't necessary (it's already documented in the method call itself)
^_^ You suggested adding it in Patchset #12
// Returns a mapping of KeyboardEvent "code" to "key" values, based on the
// highest-priority ASCII-capable layout in the list of currently installed
// keyboard layouts.Remove this?
Done
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Update comments in mojom": https://github.com/w3c/web-platform-tests/pull/10495
Daniel Cheng (Gerrit)
1 comment:
// The browser side service to p
^_^ You suggested adding it in Patchset #12
To be clear:
The method should document what it does "return a keyboard layout mapping for the highest priority ... "
The field should document what it is: A map of KeyboardEvent code to KeyboardEvent key values.
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Gary Kacmarcik (Gerrit)
1 comment:
map<string, string> layout_map;
};To be clear: […]
Ah. Thanks for the clarification. I misinterpreted your previous comment.
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Gary Kacmarcik (Gerrit)
Patch set 21:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"mojom comments" https://chromium-review.googlesource.com/c/1014244/21
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/1014244/21
Bot data: {"action": "start", "triggered_at": "2018-05-04T20:58:27.0Z", "cq_cfg_revision": "b105d6f6ac7730607488e1628cf1ddc50541c62c", "revision": "c96c0969262b2bc42e4d4a035eb25a02d5492c38"}
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "mojom comments": https://github.com/w3c/web-platform-tests/pull/10495
Commit Bot (Gerrit)
Try jobs failed on following builders:
mac_chromium_rel_ng on luci.chromium.try (JOB_FAILED, https://ci.chromium.org/p/chromium/builders/luci.chromium.try/mac_chromium_rel_ng/39590)
Gary Kacmarcik (Gerrit)
Patch set 21:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"mojom comments" https://chromium-review.googlesource.com/c/1014244/21
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/1014244/21
Bot data: {"action": "start", "triggered_at": "2018-05-04T23:52:46.0Z", "cq_cfg_revision": "b105d6f6ac7730607488e1628cf1ddc50541c62c", "revision": "c96c0969262b2bc42e4d4a035eb25a02d5492c38"}
Commit Bot (Gerrit)
Try jobs failed on following builders:
linux_chromium_rel_ng on luci.chromium.try (JOB_FAILED, https://ci.chromium.org/p/chromium/builders/luci.chromium.try/linux_chromium_rel_ng/86247)
Kentaro Hara (Gerrit)
drive-by...
Implementation-wise LGTM.
Patch set 21:Code-Review +1
1 comment:
File third_party/blink/renderer/modules/keyboard/keyboard_layout_map.idl:
Add a link to the spec.
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Add links to spec": https://github.com/w3c/web-platform-tests/pull/10495
Gary Kacmarcik (Gerrit)
1 comment:
Add a link to the spec.
Done
To view, visit change 1014244. To unsubscribe, or for help writing mail filters, visit settings.
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Add links to spec" https://chromium-review.googlesource.com/c/1014244/22
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/1014244/22
Bot data: {"action": "start", "triggered_at": "2018-05-07T23:10:00.0Z", "cq_cfg_revision": "43b70f7465b96b6bef5729ba9cf194f9f5ad4e0f", "revision": "2ba46a3bf9a60b56393559e9bf8bdd355477b9da"}
Gary Kacmarcik (Gerrit)
Patch set 22:Commit-Queue +2
Commit Bot (Gerrit)
Commit Bot merged this change.
[KeyboardMap] Initial stub for navigator.keyboard.getLayoutMap
This includes the WebIDL defs, initial WebPlatformTests and mojom
over to stubs in the RenderWidgetHost.
Spec: https://wicg.github.io/keyboard-map/
A proper implementation for this API will be in follow up cls,
a separate cl per platform.
Bug: 832811
Change-Id: I17dee21095665739a3f1986e48a2182d6b6fab00
Reviewed-on: https://chromium-review.googlesource.com/1014244
Commit-Queue: Gary Kacmarcik <gar...@chromium.org>
Reviewed-by: Kentaro Hara <har...@chromium.org>
Reviewed-by: Daniel Cheng <dch...@chromium.org>
Reviewed-by: Philip Jägenstedt <foo...@chromium.org>
Reviewed-by: Scott Violet <s...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556639}
---
M content/browser/keyboard_lock/keyboard_lock_service_impl.cc
M content/browser/keyboard_lock/keyboard_lock_service_impl.h
M content/browser/renderer_host/render_widget_host_view_base.cc
M content/browser/renderer_host/render_widget_host_view_base.h
M content/public/browser/render_widget_host_view.h
A third_party/WebKit/LayoutTests/external/wpt/interfaces/keyboard-map.idl
A third_party/WebKit/LayoutTests/external/wpt/keyboard-map/OWNERS
A third_party/WebKit/LayoutTests/external/wpt/keyboard-map/idlharness.https.html
A third_party/WebKit/LayoutTests/external/wpt/keyboard-map/keyboard-map-https-expected.txt
A third_party/WebKit/LayoutTests/external/wpt/keyboard-map/keyboard-map-https.html
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt
M third_party/blink/public/platform/modules/keyboard_lock/keyboard_lock.mojom
M third_party/blink/public/platform/web_feature.mojom
M third_party/blink/renderer/modules/keyboard/BUILD.gn
M third_party/blink/renderer/modules/keyboard/keyboard.cc
M third_party/blink/renderer/modules/keyboard/keyboard.h
M third_party/blink/renderer/modules/keyboard/keyboard.idl
A third_party/blink/renderer/modules/keyboard/keyboard_layout.cc
A third_party/blink/renderer/modules/keyboard/keyboard_layout.h
A third_party/blink/renderer/modules/keyboard/keyboard_layout_map.cc
A third_party/blink/renderer/modules/keyboard/keyboard_layout_map.h
A third_party/blink/renderer/modules/keyboard/keyboard_layout_map.idl
M third_party/blink/renderer/modules/modules_idl_files.gni
M third_party/blink/renderer/platform/runtime_enabled_features.json5
M tools/metrics/histograms/enums.xml
25 files changed, 401 insertions(+), 1 deletion(-)
Blink WPT Bot (Gerrit)
The WPT PR for this CL has been merged upstream! https://github.com/w3c/web-platform-tests/pull/10495