Add H5vccSystem web interface, set window.h5vcc.h5vccSystem object #4763
Conversation
The readonly property advertisingId under H5vccSystem return a dummy value. b/377049113
zhongqiliang
requested review from
dahlstrom-g,
yell0wd0g,
hlwarriner and
andrewsavage1
There was a problem hiding this comment.
No major concerns, just a few drive-by comments. Will let @andrewsavage1 or @hlwarriner give the final RS.
| namespace blink { | ||
|
|
||
| // static | ||
| const char H5vccSystem::kSupplementName[] = "H5vccSystem"; |
|
|
||
| const String H5vccSystem::advertisingId() const { | ||
| // TODO populate the value here. | ||
| // return advertising_id_; |
There was a problem hiding this comment.
Maybe NOTIMPLEMENTED() << "Bla"; ?
| @@ -0,0 +1,57 @@ | |||
| // Copyright 2024 The Cobalt Authors. All Rights Reserved. | |||
There was a problem hiding this comment.
2024 here and elsewhere.
| Exposed=Window, | ||
| SecureContext | ||
| ] | ||
| interface H5vccSystem { |
There was a problem hiding this comment.
We need here some comment to link back to the spec this comes from, e.g. like in https://source.corp.google.com/h/lbshell-internal/cobalt_src/+/main:third_party/blink/renderer/modules/geolocation/geolocation.idl;l=26?q=renderer%2Fmodules%20geolocation%20idl&ss=h%2Flbshell-internal%2Fcobalt_src%2F%2B%2Frefs%2Fheads%2Fmain
| class ScriptPromiseResolver; | ||
| class ScriptState; | ||
|
|
||
| class MODULES_EXPORT H5vccSystem final |
There was a problem hiding this comment.
Can we add class comments here? Specially what it does : )
| @@ -18,4 +18,5 @@ | |||
| ] | |||
| interface H5vcc { | |||
| readonly attribute CrashAnnotator crashAnnotator; | |||
| readonly attribute H5vccSystem h5vccSystem; | |||
There was a problem hiding this comment.
Reading the name here it makes me think the H5vcc prefix is unnecessary, but if we were to just call this attribute system it would be almost meaningless. Can we think of a better name, or are we limited by backwards compatibility?
There was a problem hiding this comment.
We should call it system for now and adhere to c25 interfaces, but later evaluate renaming
| @@ -0,0 +1,7 @@ | |||
| import("//third_party/blink/renderer/modules/modules.gni") | |||
There was a problem hiding this comment.
Add a copyright header
There was a problem hiding this comment.
This file isn't properly formatted. Could you add a line in https://github.com/youtube/cobalt/blob/main/.pre-commit-config.yaml#L8 to add this directory to formatting rules please?
| namespace blink { | ||
|
|
||
| // static | ||
| const char H5vccSystem::kSupplementName[] = "H5vccSystem"; |
There was a problem hiding this comment.
This isn't needed—H5vccSystem will not be a Supplementable nor an ExecutionContextLifecycleObserver, it should only inherit from ScriptWrappable
| @@ -18,4 +18,5 @@ | |||
| ] | |||
| interface H5vcc { | |||
| readonly attribute CrashAnnotator crashAnnotator; | |||
| readonly attribute H5vccSystem h5vccSystem; | |||
There was a problem hiding this comment.
We should call it system for now and adhere to c25 interfaces, but later evaluate renaming
The readonly property advertisingId return a dummy value.
b/377049113