Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1808203005:
[OnionSoup] Moving VR service from content to Blink (Closed)
|
Created:
4 years, 9 months ago by r.kasibhatla Modified:
4 years, 7 months ago Reviewers:
Ken Rockot(use gerrit already), bajones, danakj, esprehn, jam, dcheng, Yuki, RaviKasibhatla, yzshen1, haraken CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dglazkov+blink, kinuko+watch, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[OnionSoup] Moving VR service from content to Blink
Migrate vr service from content/renderer to third_party/WebKit/Source/modules.
This CL removes the files
- content/renderer/vr/vr_dispatcher.*
- WebKit/public/platform/modules/vr/WebVRClient.h
since functionality is folded into WebKit/Source/modules/vr/VRController.h.*
This CL also moves the files
- content/renderer/vr/vr_type_converters.* to
WebKit/Source/modules/vr/VRTypeConverters.h.*
- content/common/vr_service.mojom to
WebKit/public/platform/modules/vr/vr_service.mojom
VRController has been modified to talk directly to the mojo service impl
in browser's render frame host.
* Plan to rewrite the mock service of vr/ for layout tests
based on JS-bindings of Mojo.
* Plan to fix unit tests present in chrome/browser using fake service.
* Plan to add unit tests in WebKit/Source/modules if required for
vr service using fake service.
BUG=593607
TESTS=NONE
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/9519542992a024e3c90c29b25c00d8bd87e71480
Cr-Commit-Position: refs/heads/master@{#392144}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Reworked as per comments! #
Total comments: 31
Patch Set 3 : More rework and build error on feature disabled platforms! #Patch Set 4 : Rebased to ToT! #Patch Set 5 : More build fixes! #Patch Set 6 : Reworked as per new comments. Fixed build errors! #
Total comments: 9
Patch Set 7 : More review comments and more build fixes! #Patch Set 8 : Rebased to ToT! #
Total comments: 15
Patch Set 9 : Rebased to ToT and fixed the crash in webkit_unit_tests! #
Total comments: 24
Patch Set 10 : Using std::unique_ptr and using [Sync] mojo call! #
Total comments: 4
Patch Set 11 : Rebased to ToT and addressed minor comments! #Patch Set 12 : Rebased to ToT and addressed minor comments! #Patch Set 13 : Rebased to ToT again! #
Total comments: 4
Patch Set 14 : Adddressing minor comments! #Patch Set 15 : Rebase to ToT! #Dependent Patchsets: Messages
Total messages: 127 (30 generated)
Description was changed from ========== [WIP] Migrate vr service from content/renderer/ to WebKit/platform/ Migrates vr service from content/renderer/ to WebKit/platform/ * Files under WebKit/Source/platform/vr/ are intentionally written in Chromium's coding style so that it's easy to follow the diff. BUG=593607 ========== to ========== [WIP] Migrate vr service from content/renderer/ to WebKit/platform/ Migrates vr service from content/renderer/ to WebKit/platform/ * Files under WebKit/Source/platform/vr/ are intentionally written in Chromium's coding style so that it's easy to follow the diff. BUG=593607 ==========
kphanee@chromium.org changed reviewers: + haraken@chromium.org, yukishiino@chromium.org
kphanee@chromium.org changed reviewers: + kphanee@chromium.org
@yuki/haraken,
Can you please check the first cut of the change? There are some rough edges in
the patch which I still have to sort it out like removing base::IDMap and using
WTF::HashMap(?). I will upload further tuned patch but I wanted your opinion on
the direction of the patch.
I also have couple of queries with regards to the changes.
1. Should I move content/common/vr_service.mojom to
WebKit/{public|Source}/platform? Right now I am including content/ in WebKit
which as I understand is against the deps rule.
2. I haven't noticed any unit tests or layout tests for the module as of now.
Should I add the tests now or they will be checked later?
> 1. Should I move content/common/vr_service.mojom to
> WebKit/{public|Source}/platform? Right now I am including content/ in WebKit
> which as I understand is against the deps rule.
You should put the mojom file in public/platform/. See PaymentRequest CL
(https://codereview.chromium.org/1753543002/).
> 2. I haven't noticed any unit tests or layout tests for the module as of now.
> Should I add the tests now or they will be checked later?
Yeah, it would be great if you could add tests for the feature. A follow-up CL
is fine.
https://codereview.chromium.org/1808203005/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/vr/VRController.h (right):
https://codereview.chromium.org/1808203005/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/vr/VRController.h:43:
RefPtrWillBeMember<WebVRClient> m_client;
This looks wrong.
- You cannot use RefPtr in non-oilpan because WebVRClient is not RefCounted. I
guess this will cause compile error in non-oilpan...
- You cannot use Member in oilpan because WebVRClient is not GarbageCollected.
https://codereview.chromium.org/1808203005/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/vr/vr_dispatcher.cc (right):
https://codereview.chromium.org/1808203005/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/vr/vr_dispatcher.cc:13: WebVRClient*
VRDispatcher::create()
VRDispatcher is not GarbageCollected, so you cannot return a raw pointer. I
think you need to make WebVRClient a GarbageCollected.
https://codereview.chromium.org/1808203005/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/vr/vr_dispatcher.h (right):
https://codereview.chromium.org/1808203005/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/vr/vr_dispatcher.h:44:
IDMap<WebVRGetDevicesCallback, IDMapOwnPointer> pending_requests_;
Hmm, IDMap looks nasty.
Can we introduce a new object created per request and avoid using IDMap/HashMap?
class VRGetDevicesRequest { // The instance is created per request.
...;
void OnGetDevices() {
m_callback->OnGetDevices();
}
WebVRGetDevicesCallback m_callback;
};
On 2016/03/19 00:45:18, haraken wrote:
> > 1. Should I move content/common/vr_service.mojom to
> > WebKit/{public|Source}/platform? Right now I am including content/ in WebKit
> > which as I understand is against the deps rule.
>
> You should put the mojom file in public/platform/. See PaymentRequest CL
> (https://codereview.chromium.org/1753543002/).
>
> > 2. I haven't noticed any unit tests or layout tests for the module as of
now.
> > Should I add the tests now or they will be checked later?
>
> Yeah, it would be great if you could add tests for the feature. A follow-up CL
> is fine.
>
>
https://codereview.chromium.org/1808203005/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/vr/VRController.h (right):
>
>
https://codereview.chromium.org/1808203005/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/vr/VRController.h:43:
> RefPtrWillBeMember<WebVRClient> m_client;
>
> This looks wrong.
>
> - You cannot use RefPtr in non-oilpan because WebVRClient is not RefCounted. I
> guess this will cause compile error in non-oilpan...
>
> - You cannot use Member in oilpan because WebVRClient is not GarbageCollected.
>
>
https://codereview.chromium.org/1808203005/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/vr/vr_dispatcher.cc (right):
>
>
https://codereview.chromium.org/1808203005/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/vr/vr_dispatcher.cc:13: WebVRClient*
> VRDispatcher::create()
>
> VRDispatcher is not GarbageCollected, so you cannot return a raw pointer. I
> think you need to make WebVRClient a GarbageCollected.
>
@haraken: A quick search in WebKit/public folder gave me 0 results for using oil
pan garbage collection usage. AFAIU, oil pan/GC terminology is not allowed in
the WebKit/public/ classes. Please correct me if my understanding is wrong.
Can I instead derive VRDispatcher class from GarbageCollected<T> and use instead
Member<VRDispatcher> in VRController? Would that be fine?
Also, if I don't derive WebVRClient from GC but just derive VRDispatcher as
"public GarbageCollected<VRDispatcher>", then would the statement
Member<WebVRClient> would be fine in VRController.h?
Sorry for asking naive questions on GC, as I am not well versed with the oil-pan
terminology as of today. :)
>
https://codereview.chromium.org/1808203005/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/vr/vr_dispatcher.h (right):
>
>
https://codereview.chromium.org/1808203005/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/vr/vr_dispatcher.h:44:
> IDMap<WebVRGetDevicesCallback, IDMapOwnPointer> pending_requests_;
>
> Hmm, IDMap looks nasty.
>
> Can we introduce a new object created per request and avoid using
IDMap/HashMap?
>
> class VRGetDevicesRequest { // The instance is created per request.
> ...;
> void OnGetDevices() {
> m_callback->OnGetDevices();
> }
> WebVRGetDevicesCallback m_callback;
> };
As you suggested, I have moved the mojom file to WebKit/public/platform/modules/vr (to be in sync with bluetooth or paymentrequest changes). I have addressed other comments as well. Will be adding the tests in a follow up CL. PTAL! https://codereview.chromium.org/1808203005/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/vr/VRController.h (right): https://codereview.chromium.org/1808203005/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/vr/VRController.h:43: RefPtrWillBeMember<WebVRClient> m_client; On 2016/03/19 00:45:18, haraken wrote: > > This looks wrong. > > - You cannot use RefPtr in non-oilpan because WebVRClient is not RefCounted. I > guess this will cause compile error in non-oilpan... > > - You cannot use Member in oilpan because WebVRClient is not GarbageCollected. I have added Member<VRDispatcher> in VRController. Also, made VRDispatcher a garbage collected class. PTAL current approach and let me know if I am doing anything wrong. https://codereview.chromium.org/1808203005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/vr/vr_dispatcher.cc (right): https://codereview.chromium.org/1808203005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/vr/vr_dispatcher.cc:13: WebVRClient* VRDispatcher::create() On 2016/03/19 00:45:18, haraken wrote: > > VRDispatcher is not GarbageCollected, so you cannot return a raw pointer. I > think you need to make WebVRClient a GarbageCollected. > PTAL at my current approach. Since WebVRClient is a public class, I think it is not correct to make it a GC class. As explained in previous comment, I have made VRDispatcher a GC class and keeping it as Member in VRController. https://codereview.chromium.org/1808203005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/vr/vr_dispatcher.h (right): https://codereview.chromium.org/1808203005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/vr/vr_dispatcher.h:44: IDMap<WebVRGetDevicesCallback, IDMapOwnPointer> pending_requests_; On 2016/03/19 00:45:18, haraken wrote: > > Hmm, IDMap looks nasty. > > Can we introduce a new object created per request and avoid using IDMap/HashMap? > > class VRGetDevicesRequest { // The instance is created per request. > ...; > void OnGetDevices() { > m_callback->OnGetDevices(); > } > WebVRGetDevicesCallback m_callback; > }; Done.
The CQ bit was checked by kphanee@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1808203005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1808203005/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
haraken@chromium.org changed reviewers: + yzshen@chromium.org
Mostly looks good. +yzshen for vr_type_converters.h. https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRController.cpp (right): https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRController.cpp:71: m_client = nullptr; Not related to this CL, can we add ASSERT(!m_client) to ~VRController? https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/vr/vr_dispatcher.cc (right): https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/vr/vr_dispatcher.cc:80: delete device_request; We really want to avoid calling manual delete. Can we make VRDispatcher hold HashSet<OwnPtr<VRGetDevicesRequest>> so that we don't need to call delete manually? https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/vr/vr_dispatcher.h (right): https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/vr/vr_dispatcher.h:16: WTF_MAKE_NONCOPYABLE(VRGetDevicesRequest); Add USING_FAST_MALLOC(). https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/vr/vr_dispatcher.h:23: WebVRGetDevicesCallback* get_devices_callback_; How is it guaranteed that the WebVRGetDevicesCallback is kept alive while some VRGetDevicesRequest is keeping a reference to it (i.e., the WebVRGetDevicesCallback)? https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/vr/vr_dispatcher.h:49: void OnGetDevices(VRGetDevicesRequest* device_request,//int* request_id, Remove the comment and add a TODO?
https://codereview.chromium.org/1808203005/diff/20001/content/browser/vr/andr... File content/browser/vr/android/cardboard/cardboard_vr_device.h (right): https://codereview.chromium.org/1808203005/diff/20001/content/browser/vr/andr... content/browser/vr/android/cardboard/cardboard_vr_device.h:14: using namespace mojom; In general, "using namespace" in headers is discouraged. All the files that include this header "cardboard_vr_device.h" will be affected by the "using namespace". https://codereview.chromium.org/1808203005/diff/20001/content/browser/vr/vr_d... File content/browser/vr/vr_device.h (right): https://codereview.chromium.org/1808203005/diff/20001/content/browser/vr/vr_d... content/browser/vr/vr_device.h:11: using namespace mojom; Ditto. https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/BUILD.gn (right): https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/BUILD.gn:342: "//content/common:mojo_bindings", As you already knew, Blink shouldn't depend on content/. Please remove this line. https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/blink_platform.gyp (right): https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/blink_platform.gyp:134: '<(DEPTH)/content/content_common_mojo_bindings.gyp:content_common_mojo_bindings', Ditto. https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/threading/BindForMojo.h (right): https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/threading/BindForMojo.h:31: return base::Bind(method, base::Unretained(instance), base::Unretained(bound)); In general, this is not a good idea because it's not always true that |bound| will be kept alive until the callback function gets called back. See the original use case, the caller of base::Bind specifies how to control the lifetime of |bound|, such as Unretained, Owned, etc. If possible, you'd better to avoid binding the arguments other than |this| until Blink gets to better support the callback binding. https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/vr/vr_type_converters.h (right): https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/vr/vr_type_converters.h:18: struct TypeConverter<blink::WebVRVector3, mojom::VRVector3Ptr> { IIUC, WebVRVector, etc. are no longer exposed to outside of Blink. They're only used inside Blink. Then, they shouldn't be prefixed with "Web". You'd better remove "Web" prefix from all the types that were once public but now that are not. You may want to rename them in a separate CL to not make this CL any bigger. https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1560: VRController::provideTo(*m_frame, VRDispatcher::create()); I think you no longer need this line. The clients of VR can directly call into the VR APIs because both of clients and the API implementation live in Blink. https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/platform/modules/vr/vr_service.mojom (right): https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/vr/vr_service.mojom:5: module mojom; I'd feel that "mojom" doesn't make much sense. There should be better names. You may want to ask naming ideas to reviewers. Just FYI, there is a discussion where we should put *.mojom files. One idea is to put *.mojom files in component/<feature>/*.mojom. So, "blink" shouldn't be a good name, too.
LG for vr_type_converters.h https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/vr/vr_type_converters.h (right): https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/vr/vr_type_converters.h:8: #include "mojo/common/common_type_converters.h" nit: #include "mojo/public/cpp/bindings/type_converter.h" instead?
Description was changed from ========== [WIP] Migrate vr service from content/renderer/ to WebKit/platform/ Migrates vr service from content/renderer/ to WebKit/platform/ * Files under WebKit/Source/platform/vr/ are intentionally written in Chromium's coding style so that it's easy to follow the diff. BUG=593607 ========== to ========== [WIP] Migrate vr service from content/renderer/ to WebKit/platform/ Migrates vr service from content/renderer/ to WebKit/platform/ * Files under WebKit/Source/platform/vr/ are intentionally written in Chromium's coding style so that it's easy to follow the diff. BUG=593607 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Modified as per all review comments. PTAL! https://codereview.chromium.org/1808203005/diff/20001/content/browser/vr/andr... File content/browser/vr/android/cardboard/cardboard_vr_device.h (right): https://codereview.chromium.org/1808203005/diff/20001/content/browser/vr/andr... content/browser/vr/android/cardboard/cardboard_vr_device.h:14: using namespace mojom; On 2016/03/22 07:15:03, Yuki wrote: > In general, "using namespace" in headers is discouraged. All the files that > include this header "cardboard_vr_device.h" will be affected by the "using > namespace". Done. https://codereview.chromium.org/1808203005/diff/20001/content/browser/vr/vr_d... File content/browser/vr/vr_device.h (right): https://codereview.chromium.org/1808203005/diff/20001/content/browser/vr/vr_d... content/browser/vr/vr_device.h:11: using namespace mojom; On 2016/03/22 07:15:03, Yuki wrote: > Ditto. Done. https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/vr/VRController.cpp (right): https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/vr/VRController.cpp:71: m_client = nullptr; On 2016/03/22 02:39:20, haraken wrote: > > Not related to this CL, can we add ASSERT(!m_client) to ~VRController? Done. https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/BUILD.gn (right): https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/BUILD.gn:342: "//content/common:mojo_bindings", On 2016/03/22 07:15:03, Yuki wrote: > As you already knew, Blink shouldn't depend on content/. Please remove this > line. Done. My bad. I had added when the new location of mojom file was not yet sure. After moving the file to WebKit/public/platform, I forgot to remove this change. https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/blink_platform.gyp (right): https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/blink_platform.gyp:134: '<(DEPTH)/content/content_common_mojo_bindings.gyp:content_common_mojo_bindings', On 2016/03/22 07:15:03, Yuki wrote: > Ditto. Done. https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/threading/BindForMojo.h (right): https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/threading/BindForMojo.h:31: return base::Bind(method, base::Unretained(instance), base::Unretained(bound)); On 2016/03/22 07:15:03, Yuki wrote: > In general, this is not a good idea because it's not always true that |bound| > will be kept alive until the callback function gets called back. See the > original use case, the caller of base::Bind specifies how to control the > lifetime of |bound|, such as Unretained, Owned, etc. > > If possible, you'd better to avoid binding the arguments other than |this| until > Blink gets to better support the callback binding. In GetDevices, the original Web callback is being stored in the VRGetDevicesRequest object created before binding the mojo::Callback. In GetSensorState, we receive the WebHMDSensorState variable which needs to be filled back in the mojo::Callback. So, for both the mojo::Callback binding calls in vr_dispatcher, we do need to bound objects other than 'this' pointer. Even in the original code, the state and request_id are binded in the callback. Regarding the lifetime of the bounded args, atleast in both the calling scenarios currently, the binded objects will be alive. I marked bind args as Unretained since the original code was binding them in same scope. But if needed, to safeguard to the lifetime of bound objects, I can mark them as "Owned", so that they are not deleted during the callback lifetime? https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/vr/vr_dispatcher.cc (right): https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/vr/vr_dispatcher.cc:80: delete device_request; On 2016/03/22 02:39:20, haraken wrote: > > We really want to avoid calling manual delete. Can we make VRDispatcher hold > HashSet<OwnPtr<VRGetDevicesRequest>> so that we don't need to call delete > manually? > > Done. https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/vr/vr_dispatcher.h (right): https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/vr/vr_dispatcher.h:16: WTF_MAKE_NONCOPYABLE(VRGetDevicesRequest); On 2016/03/22 02:39:20, haraken wrote: > > Add USING_FAST_MALLOC(). Done. https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/vr/vr_dispatcher.h:23: WebVRGetDevicesCallback* get_devices_callback_; On 2016/03/22 02:39:20, haraken wrote: > > How is it guaranteed that the WebVRGetDevicesCallback is kept alive while some > VRGetDevicesRequest is keeping a reference to it (i.e., the > WebVRGetDevicesCallback)? The original web caller actually transfers the responsibility of deletion of the callback to the callback runner (VRDispatcher)[1]. I was leaking the callback. I have fixed this now by storing the callback as OwnPtr which is deleted when the VRGetDevicesRequest object is released. [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/vr/vr_dispatcher.h:49: void OnGetDevices(VRGetDevicesRequest* device_request,//int* request_id, On 2016/03/22 02:39:20, haraken wrote: > > Remove the comment and add a TODO? Done. https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/vr/vr_type_converters.h (right): https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/vr/vr_type_converters.h:8: #include "mojo/common/common_type_converters.h" On 2016/03/22 16:13:49, yzshen1 wrote: > nit: #include "mojo/public/cpp/bindings/type_converter.h" instead? Done. https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/vr/vr_type_converters.h:18: struct TypeConverter<blink::WebVRVector3, mojom::VRVector3Ptr> { On 2016/03/22 07:15:03, Yuki wrote: > IIUC, WebVRVector, etc. are no longer exposed to outside of Blink. They're only > used inside Blink. Then, they shouldn't be prefixed with "Web". You'd better > remove "Web" prefix from all the types that were once public but now that are > not. > > You may want to rename them in a separate CL to not make this CL any bigger. Sure. Since, WebVR.h was a public header, I was not sure whether to modify the names or not. https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1560: VRController::provideTo(*m_frame, VRDispatcher::create()); On 2016/03/22 07:15:03, Yuki wrote: > I think you no longer need this line. The clients of VR can directly call into > the VR APIs because both of clients and the API implementation live in Blink. I am sorry but I didn't understood the concern here. We do need to set the frame and WebVR client and hence we need to call the client creation. Are you implying instead of calling from here, VRController directly calls for client creation, since VRController holds the reference to the WebVRClient (VRDispatcher)? All builds where feature was disabled were failing because of linker error at this statement. So, in new patch, I have added createWebVRClient API which returns 0 when the feature is disabled - checking using ENABLE(WEBVR). If there is a better way to handle the linker error, please suggest. https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/platform/modules/vr/vr_service.mojom (right): https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/vr/vr_service.mojom:5: module mojom; On 2016/03/22 07:15:03, Yuki wrote: > I'd feel that "mojom" doesn't make much sense. There should be better names. > You may want to ask naming ideas to reviewers. > > Just FYI, there is a discussion where we should put *.mojom files. One idea is > to put *.mojom files in component/<feature>/*.mojom. So, "blink" shouldn't be a > good name, too. I am happy to put any module name. :) Currently, following the web_bluetooth model, I am modified the module to blink.mojom. But I am open for any name as you suggest. :)
The CQ bit was checked by kphanee@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1808203005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1808203005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_blink_oilpan_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_blink_oil...)
https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/threading/BindForMojo.h (right): https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/threading/BindForMojo.h:31: return base::Bind(method, base::Unretained(instance), base::Unretained(bound)); On 2016/03/23 15:20:50, RaviKasibhatla wrote: > On 2016/03/22 07:15:03, Yuki wrote: > > In general, this is not a good idea because it's not always true that |bound| > > will be kept alive until the callback function gets called back. See the > > original use case, the caller of base::Bind specifies how to control the > > lifetime of |bound|, such as Unretained, Owned, etc. > > > > If possible, you'd better to avoid binding the arguments other than |this| > until > > Blink gets to better support the callback binding. > > In GetDevices, the original Web callback is being stored in the > VRGetDevicesRequest object created before binding the mojo::Callback. > In GetSensorState, we receive the WebHMDSensorState variable which needs to be > filled back in the mojo::Callback. So, for both the mojo::Callback binding calls > in vr_dispatcher, we do need to bound objects other than 'this' pointer. Even in > the original code, the state and request_id are binded in the callback. > > Regarding the lifetime of the bounded args, atleast in both the calling > scenarios currently, the binded objects will be alive. I marked bind args as > Unretained since the original code was binding them in same scope. > But if needed, to safeguard to the lifetime of bound objects, I can mark them as > "Owned", so that they are not deleted during the callback lifetime? I'm talking about not only VRGetDevicesRequest's case but also general cases. Other services will use this utility function. How can we guarantee that those services are okay with base::Unretained()? In general, it should be specified by the callers. Having said that, we wouldn't want to expose uses of base::Unretained outside this directory. Options in my mind are: 1) Rename the function to sameThreadBindForMojoWithUnretainedArgs so that it's clear that it's caller's duty to keep the args alive. 2) Do not bind the |state|. Supposing VRDispatcher does not support multi-thread accesses, you can always store the resulting |state| into a member variable. https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1560: VRController::provideTo(*m_frame, VRDispatcher::create()); On 2016/03/23 15:20:51, RaviKasibhatla wrote: > On 2016/03/22 07:15:03, Yuki wrote: > > I think you no longer need this line. The clients of VR can directly call > into > > the VR APIs because both of clients and the API implementation live in Blink. > > I am sorry but I didn't understood the concern here. We do need to set the frame > and WebVR client and hence we need to call the client creation. > > Are you implying instead of calling from here, VRController directly calls for > client creation, since VRController holds the reference to the WebVRClient > (VRDispatcher)? Yes. As you're moving the implementation from content/ to Blink's platform/, there should be no need to communicate with content/, no need to communicate through WebLocalFrame, I think. I've not yet dug into deep, I guess that we could get rid of WebLocalFrame. > All builds where feature was disabled were failing because of linker error at > this statement. So, in new patch, I have added createWebVRClient API which > returns 0 when the feature is disabled - checking using ENABLE(WEBVR). > If there is a better way to handle the linker error, please suggest. It seems compile errors happening. Please fix them first, and run trybots so that we can see linker errors.
haraken@chromium.org changed reviewers: + danakj@chromium.org, dcheng@chromium.org
https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/threading/BindForMojo.h (right): https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/threading/BindForMojo.h:31: return base::Bind(method, base::Unretained(instance), base::Unretained(bound)); On 2016/03/25 09:49:04, Yuki wrote: > On 2016/03/23 15:20:50, RaviKasibhatla wrote: > > On 2016/03/22 07:15:03, Yuki wrote: > > > In general, this is not a good idea because it's not always true that > |bound| > > > will be kept alive until the callback function gets called back. See the > > > original use case, the caller of base::Bind specifies how to control the > > > lifetime of |bound|, such as Unretained, Owned, etc. > > > > > > If possible, you'd better to avoid binding the arguments other than |this| > > until > > > Blink gets to better support the callback binding. > > > > In GetDevices, the original Web callback is being stored in the > > VRGetDevicesRequest object created before binding the mojo::Callback. > > In GetSensorState, we receive the WebHMDSensorState variable which needs to be > > filled back in the mojo::Callback. So, for both the mojo::Callback binding > calls > > in vr_dispatcher, we do need to bound objects other than 'this' pointer. Even > in > > the original code, the state and request_id are binded in the callback. > > > > Regarding the lifetime of the bounded args, atleast in both the calling > > scenarios currently, the binded objects will be alive. I marked bind args as > > Unretained since the original code was binding them in same scope. > > But if needed, to safeguard to the lifetime of bound objects, I can mark them > as > > "Owned", so that they are not deleted during the callback lifetime? > > I'm talking about not only VRGetDevicesRequest's case but also general cases. > Other services will use this utility function. How can we guarantee that those > services are okay with base::Unretained()? In general, it should be specified > by the callers. > > Having said that, we wouldn't want to expose uses of base::Unretained outside > this directory. > > Options in my mind are: > 1) Rename the function to sameThreadBindForMojoWithUnretainedArgs so that it's > clear that it's caller's duty to keep the args alive. > 2) Do not bind the |state|. Supposing VRDispatcher does not support > multi-thread accesses, you can always store the resulting |state| into a member > variable. Dcheng and Dana are working on it (https://bugs.chromium.org/p/chromium/issues/detail?id=597856).
On 2016/03/25 09:49:05, Yuki wrote: > https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/threading/BindForMojo.h (right): > > https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/threading/BindForMojo.h:31: return > base::Bind(method, base::Unretained(instance), base::Unretained(bound)); > On 2016/03/23 15:20:50, RaviKasibhatla wrote: > > On 2016/03/22 07:15:03, Yuki wrote: > > > In general, this is not a good idea because it's not always true that > |bound| > > > will be kept alive until the callback function gets called back. See the > > > original use case, the caller of base::Bind specifies how to control the > > > lifetime of |bound|, such as Unretained, Owned, etc. > > > > > > If possible, you'd better to avoid binding the arguments other than |this| > > until > > > Blink gets to better support the callback binding. > > > > In GetDevices, the original Web callback is being stored in the > > VRGetDevicesRequest object created before binding the mojo::Callback. > > In GetSensorState, we receive the WebHMDSensorState variable which needs to be > > filled back in the mojo::Callback. So, for both the mojo::Callback binding > calls > > in vr_dispatcher, we do need to bound objects other than 'this' pointer. Even > in > > the original code, the state and request_id are binded in the callback. > > > > Regarding the lifetime of the bounded args, atleast in both the calling > > scenarios currently, the binded objects will be alive. I marked bind args as > > Unretained since the original code was binding them in same scope. > > But if needed, to safeguard to the lifetime of bound objects, I can mark them > as > > "Owned", so that they are not deleted during the callback lifetime? > > I'm talking about not only VRGetDevicesRequest's case but also general cases. > Other services will use this utility function. How can we guarantee that those > services are okay with base::Unretained()? In general, it should be specified > by the callers. > > Having said that, we wouldn't want to expose uses of base::Unretained outside > this directory. > > Options in my mind are: > 1) Rename the function to sameThreadBindForMojoWithUnretainedArgs so that it's > clear that it's caller's duty to keep the args alive. > 2) Do not bind the |state|. Supposing VRDispatcher does not support > multi-thread accesses, you can always store the resulting |state| into a member > variable. > Sure. Option 2 sounds fine to me. I will make the change accordingly. > https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): > > https://codereview.chromium.org/1808203005/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1560: > VRController::provideTo(*m_frame, VRDispatcher::create()); > On 2016/03/23 15:20:51, RaviKasibhatla wrote: > > On 2016/03/22 07:15:03, Yuki wrote: > > > I think you no longer need this line. The clients of VR can directly call > > into > > > the VR APIs because both of clients and the API implementation live in > Blink. > > > > I am sorry but I didn't understood the concern here. We do need to set the > frame > > and WebVR client and hence we need to call the client creation. > > > > Are you implying instead of calling from here, VRController directly calls for > > client creation, since VRController holds the reference to the WebVRClient > > (VRDispatcher)? > > Yes. As you're moving the implementation from content/ to Blink's platform/, > there should be no need to communicate with content/, no need to communicate > through WebLocalFrame, I think. > > I've not yet dug into deep, I guess that we could get rid of WebLocalFrame. > > > All builds where feature was disabled were failing because of linker error at > > this statement. So, in new patch, I have added createWebVRClient API which > > returns 0 when the feature is disabled - checking using ENABLE(WEBVR). > > If there is a better way to handle the linker error, please suggest. > > It seems compile errors happening. Please fix them first, and run trybots so > that we can see linker errors. Yes, let me first fix all the compile errors on all bot configs. On my local system I was doing an android and desktop(gcc) gyp component build. Both builds are successful. However, other build configs are failing. :( I have also started the GN build and the oilpan build (with same config as seen in linux_blink_oilpan_rel bot) locally. I will also use trybots (commitbot dryrun) to iron out all the build errors. After build is fixed, we can start the patch review again. As regards to WebLocalFrame change, let me check how to remove that path completely and directly assign the client (VRDispatcher) to VRController.
I have some question about how layer separation is supposed to work when moving mojo interfaces into blink like this. I've been experimenting with some modifications to this CL (seen here: https://codereview.chromium.org/1906883002/) and there's a couple of things I'm not clear on. 1) Is it still necessary or beneficial to retain WebVRClient/VRDispatcher? Looking at the WakeLock migration (https://crrev.com/1878683002 and https://crrev.com/1883493003) it's having the WebLocalFrameImpl provide the ServiceRegistery directly to the module, and it seems like the same should work here by having the VRController manage the service connection. (I've done that in my version of the patch.) Doesn't seem like a layering violation, any issues there? 2) It's not clear to me where TypeConverters should live. I've currently got them in modules/vr/VRTypeConverters.h/cpp but the precommit hooks definitely don't like me including type_converters.h from there. 3) Then again, maybe the type converter isn't needed? I'm not seeing public/platform/modules/vr/WebVR.h serve much of a purpose any more and it seems cleaner to just let the rest of the module use the mojo structs directly. Any reason why that's a bad idea? 4) Finally, to avoid introducing a new sameThreadBindForMojoWithUnretainedArgs it seems like it's safe to, in the case of the DeviceInfo callbacks, just add the pending callbacks to an array and call all of them when the mojo call returns. This should be thread safe, right?
haraken@chromium.org changed reviewers: + esprehn@chromium.org, rockot@chromium.org
+esprehn +rockot On 2016/04/21 18:51:22, bajones wrote: > I have some question about how layer separation is supposed to work when moving > mojo interfaces into blink like this. I've been experimenting with some > modifications to this CL (seen here: > https://codereview.chromium.org/1906883002/) and there's a couple of things I'm > not clear on. > > 1) Is it still necessary or beneficial to retain WebVRClient/VRDispatcher? > Looking at the WakeLock migration (https://crrev.com/1878683002 and > https://crrev.com/1883493003) it's having the WebLocalFrameImpl provide the > ServiceRegistery directly to the module, and it seems like the same should work > here by having the VRController manage the service connection. (I've done that > in my version of the patch.) Doesn't seem like a layering violation, any issues > there? You're right. You can follow the pattern of WakeLock. > 2) It's not clear to me where TypeConverters should live. I've currently got > them in modules/vr/VRTypeConverters.h/cpp but the precommit hooks definitely > don't like me including type_converters.h from there. You should include platform/MojoHelper.h instead. Then modules/vr/VRTypeConverters can define the TypeConverter. > > 3) Then again, maybe the type converter isn't needed? I'm not seeing > public/platform/modules/vr/WebVR.h serve much of a purpose any more and it seems > cleaner to just let the rest of the module use the mojo structs directly. Any > reason why that's a bad idea? No, good idea :) See this CL (https://codereview.chromium.org/1753543002/), which implemented PaymentRequest-with-Mojo. modules are directly using mojom::wtf::ShippingAddressPtr. > > 4) Finally, to avoid introducing a new sameThreadBindForMojoWithUnretainedArgs > it seems like it's safe to, in the case of the DeviceInfo callbacks, just add > the pending callbacks to an array and call all of them when the mojo call > returns. This should be thread safe, right? That will work, but you won't need to introduce sameThreadBindForMojoWithBoundArgs either. You need to add the bound args because the callback method is on VRDispatcher. If you put the callback method on VRGetDevicesRequest, you can just put the bound args on the VRGetDevicesRequest object. Then the bound args won't be needed.
On 2016/04/21 23:18:47, haraken wrote: > > 4) Finally, to avoid introducing a new sameThreadBindForMojoWithUnretainedArgs > > it seems like it's safe to, in the case of the DeviceInfo callbacks, just add > > the pending callbacks to an array and call all of them when the mojo call > > returns. This should be thread safe, right? > > That will work, but you won't need to introduce > sameThreadBindForMojoWithBoundArgs either. You need to add the bound args > because the callback method is on VRDispatcher. If you put the callback method > on VRGetDevicesRequest, you can just put the bound args on the > VRGetDevicesRequest object. Then the bound args won't be needed. +1 to avoid introducing sameThreadBindForMojoWithBoundArgs. It's better to wait for the coming better migration of base::Bind and wtf::bind. Until then, we should avoid introducing sameThreadBindForMojoWithBoundArgs.
Description was changed from ========== [WIP] Migrate vr service from content/renderer/ to WebKit/platform/ Migrates vr service from content/renderer/ to WebKit/platform/ * Files under WebKit/Source/platform/vr/ are intentionally written in Chromium's coding style so that it's easy to follow the diff. BUG=593607 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== [OnionSoup] Moving VR service from content to Blink Migrate vr service from content/renderer to third_party/WebKit/Source/modules. This CL removes the files - content/renderer/vr/vr_dispatcher.* - WebKit/public/platform/modules/vr/WebVRClient.h since that functionality is folded into WebKit/Source/modules/vr/VRController.h.* This CL also moves the files - content/renderer/vr/vr_type_converters.* to WebKit/Source/modules/vr/VRTypeConverters.h.* VRController has been modified to talk directly to the mojo service impl in browser's render frame host. BUG=593607 TESTS=NONE ==========
Description was changed from ========== [OnionSoup] Moving VR service from content to Blink Migrate vr service from content/renderer to third_party/WebKit/Source/modules. This CL removes the files - content/renderer/vr/vr_dispatcher.* - WebKit/public/platform/modules/vr/WebVRClient.h since that functionality is folded into WebKit/Source/modules/vr/VRController.h.* This CL also moves the files - content/renderer/vr/vr_type_converters.* to WebKit/Source/modules/vr/VRTypeConverters.h.* VRController has been modified to talk directly to the mojo service impl in browser's render frame host. BUG=593607 TESTS=NONE ========== to ========== [OnionSoup] Moving VR service from content to Blink Migrate vr service from content/renderer to third_party/WebKit/Source/modules. This CL removes the files - content/renderer/vr/vr_dispatcher.* - WebKit/public/platform/modules/vr/WebVRClient.h since that functionality is folded into WebKit/Source/modules/vr/VRController.h.* This CL also moves the files - content/renderer/vr/vr_type_converters.* to WebKit/Source/modules/vr/VRTypeConverters.h.* - content/common/vr_service.mojom to WebKit/public/platform/modules/vr/vr_service.mojom VRController has been modified to talk directly to the mojo service impl in browser's render frame host. BUG=593607 TESTS=NONE ==========
Description was changed from ========== [OnionSoup] Moving VR service from content to Blink Migrate vr service from content/renderer to third_party/WebKit/Source/modules. This CL removes the files - content/renderer/vr/vr_dispatcher.* - WebKit/public/platform/modules/vr/WebVRClient.h since that functionality is folded into WebKit/Source/modules/vr/VRController.h.* This CL also moves the files - content/renderer/vr/vr_type_converters.* to WebKit/Source/modules/vr/VRTypeConverters.h.* - content/common/vr_service.mojom to WebKit/public/platform/modules/vr/vr_service.mojom VRController has been modified to talk directly to the mojo service impl in browser's render frame host. BUG=593607 TESTS=NONE ========== to ========== [OnionSoup] Moving VR service from content to Blink Migrate vr service from content/renderer to third_party/WebKit/Source/modules. This CL removes the files - content/renderer/vr/vr_dispatcher.* - WebKit/public/platform/modules/vr/WebVRClient.h since functionality is folded into WebKit/Source/modules/vr/VRController.h.* This CL also moves the files - content/renderer/vr/vr_type_converters.* to WebKit/Source/modules/vr/VRTypeConverters.h.* - content/common/vr_service.mojom to WebKit/public/platform/modules/vr/vr_service.mojom VRController has been modified to talk directly to the mojo service impl in browser's render frame host. * Plan to rewrite the mock service of vr/ for layout tests based on JS-bindings of Mojo. * Plan to fix unit tests present in chrome/browser using fake service. * Plan to add unit tests in WebKit/Source/modules if required for vr service using fake service. BUG=593607 TESTS=NONE ==========
kphanee@chromium.org changed reviewers: + bajones@chromium.org
@bajones,haraken,yuki: Sorry I didn't update the CL for so long as I was busy with some other important work. Thanks for the patience for waiting. I have updated the CL as per the recent discussion and the comments including removing unnecessary WebVRClient interface and directly integrating VRController with WebLocalFrameImpl. I have also folded vr_dispatcher class into VRController as suggested. There is still one important comment to be addressed - removing newly added API to MojoHelper.h and try to store the value within VRController for VR callbacks. I am working on the change and will update the patch by tomorrow with that change. I uploaded the patch to do a dry build on bots and see whether all bots are passing the build or not. I locally built 3-4 configs which were earlier failing promptly and those builds seems happy now.
The CQ bit was checked by kphanee@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1808203005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1808203005/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/1808203005/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/vr/vr_service.mojom (right): https://codereview.chromium.org/1808203005/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/vr/vr_service.mojom:5: module blink.mojom; Btw, let's make sure this file has noparent IPC OWNERS rules like the ones in //content/common. Thanks!
Blink-Mojo interaction looks good, but we need to improve pointer manipulations in this CL. https://codereview.chromium.org/1808203005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/DEPS (right): https://codereview.chromium.org/1808203005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/DEPS:10: "+mojo/public", I'm curious why we need to add this dependency now. We've already used Mojo in modules/ without adding the dependency. https://codereview.chromium.org/1808203005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRController.cpp (right): https://codereview.chromium.org/1808203005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.cpp:99: m_pendingRequests.remove(deviceRequest); BTW, why do you need m_pendingRequests? If it's used just to keep alive the VRGetDevicesCallback, it shouldn't be needed. Alternately you can pass the OwnPtr<VRGetDevicesCallback> to VRGetDevicesRequest and ask the VRGetDevicesRequest to keep alive the VRGetDevicesCallback. https://codereview.chromium.org/1808203005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.cpp:115: m_getDevicesCallback = adoptPtr(callback); It's nasty to call adoptPtr for a raw pointer... VRGetDevicesRequest should take PassOwnPtr<VRGetDevicesCallback>. https://codereview.chromium.org/1808203005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.cpp:120: m_getDevicesCallback.clear(); This won't be needed, since the destructor will automatically clear the m_getDevicesCallback. https://codereview.chromium.org/1808203005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/mojo/MojoHelper.h (right): https://codereview.chromium.org/1808203005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/mojo/MojoHelper.h:45: sameThreadBindForMojoWithBoundArgs(ReturnType (Class::*method)(BoundArgs*, Args...), Class* instance, BoundArgs* bound) I want to avoid introducing this method (because Unretained can lead to use-after-free if not used correctly). Can we avoid passing the parameters by adding the parameters to VRGetDevicesRequest instead?
On 2016/04/27 06:51:07, haraken wrote: > Blink-Mojo interaction looks good, but we need to improve pointer manipulations > in this CL. > > https://codereview.chromium.org/1808203005/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/DEPS (right): > > https://codereview.chromium.org/1808203005/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/DEPS:10: "+mojo/public", > > I'm curious why we need to add this dependency now. We've already used Mojo in > modules/ without adding the dependency. > > https://codereview.chromium.org/1808203005/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/vr/VRController.cpp (right): > > https://codereview.chromium.org/1808203005/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/vr/VRController.cpp:99: > m_pendingRequests.remove(deviceRequest); > > BTW, why do you need m_pendingRequests? If it's used just to keep alive the > VRGetDevicesCallback, it shouldn't be needed. Alternately you can pass the > OwnPtr<VRGetDevicesCallback> to VRGetDevicesRequest and ask the > VRGetDevicesRequest to keep alive the VRGetDevicesCallback. > > https://codereview.chromium.org/1808203005/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/vr/VRController.cpp:115: m_getDevicesCallback > = adoptPtr(callback); > > It's nasty to call adoptPtr for a raw pointer... VRGetDevicesRequest should take > PassOwnPtr<VRGetDevicesCallback>. > > https://codereview.chromium.org/1808203005/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/vr/VRController.cpp:120: > m_getDevicesCallback.clear(); > > This won't be needed, since the destructor will automatically clear the > m_getDevicesCallback. > > https://codereview.chromium.org/1808203005/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/mojo/MojoHelper.h (right): > > https://codereview.chromium.org/1808203005/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/mojo/MojoHelper.h:45: > sameThreadBindForMojoWithBoundArgs(ReturnType (Class::*method)(BoundArgs*, > Args...), Class* instance, BoundArgs* bound) > > I want to avoid introducing this method (because Unretained can lead to > use-after-free if not used correctly). Can we avoid passing the parameters by > adding the parameters to VRGetDevicesRequest instead? As I pointed in yesterday comment, following the suggestion in the comments, I am working on removing this method altogether. I wanted to check how the patch fares on all the bots - there were compile errors in many bots earlier. Unfortunately couple of bots still failed on compilation with latest patch and I am checking the cause of the build failure. I will upload the new patch today removing added mojo API as well as the changes suggested by you now.
Description was changed from ========== [OnionSoup] Moving VR service from content to Blink Migrate vr service from content/renderer to third_party/WebKit/Source/modules. This CL removes the files - content/renderer/vr/vr_dispatcher.* - WebKit/public/platform/modules/vr/WebVRClient.h since functionality is folded into WebKit/Source/modules/vr/VRController.h.* This CL also moves the files - content/renderer/vr/vr_type_converters.* to WebKit/Source/modules/vr/VRTypeConverters.h.* - content/common/vr_service.mojom to WebKit/public/platform/modules/vr/vr_service.mojom VRController has been modified to talk directly to the mojo service impl in browser's render frame host. * Plan to rewrite the mock service of vr/ for layout tests based on JS-bindings of Mojo. * Plan to fix unit tests present in chrome/browser using fake service. * Plan to add unit tests in WebKit/Source/modules if required for vr service using fake service. BUG=593607 TESTS=NONE ========== to ========== [OnionSoup] Moving VR service from content to Blink Migrate vr service from content/renderer to third_party/WebKit/Source/modules. This CL removes the files - content/renderer/vr/vr_dispatcher.* - WebKit/public/platform/modules/vr/WebVRClient.h since functionality is folded into WebKit/Source/modules/vr/VRController.h.* This CL also moves the files - content/renderer/vr/vr_type_converters.* to WebKit/Source/modules/vr/VRTypeConverters.h.* - content/common/vr_service.mojom to WebKit/public/platform/modules/vr/vr_service.mojom VRController has been modified to talk directly to the mojo service impl in browser's render frame host. * Plan to rewrite the mock service of vr/ for layout tests based on JS-bindings of Mojo. * Plan to fix unit tests present in chrome/browser using fake service. * Plan to add unit tests in WebKit/Source/modules if required for vr service using fake service. BUG=593607 TESTS=NONE CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Fixed more review comments. Want to try one more dry run to ensure all the bots are compile happy or not. :) https://codereview.chromium.org/1808203005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/DEPS (right): https://codereview.chromium.org/1808203005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/DEPS:10: "+mojo/public", On 2016/04/27 06:51:06, haraken wrote: > > I'm curious why we need to add this dependency now. We've already used Mojo in > modules/ without adding the dependency. > The other place that I noticed mojo/public was included was in modules/payments. A separate DEPS was added in modules/payments to filter the newly included headers. I have now done the same and added a new DEPS in modules/vr to filter the mojo/public inclusion. https://codereview.chromium.org/1808203005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRController.cpp (right): https://codereview.chromium.org/1808203005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.cpp:99: m_pendingRequests.remove(deviceRequest); On 2016/04/27 06:51:07, haraken wrote: > > BTW, why do you need m_pendingRequests? If it's used just to keep alive the > VRGetDevicesCallback, it shouldn't be needed. Alternately you can pass the > OwnPtr<VRGetDevicesCallback> to VRGetDevicesRequest and ask the > VRGetDevicesRequest to keep alive the VRGetDevicesCallback. > I have removed this entire logic. I am now simply storing the callback in WTF::Deque and processing the callback in the same order. Please let me know if you feel there is a concern in this approach. https://codereview.chromium.org/1808203005/diff/100001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/vr/vr_service.mojom (right): https://codereview.chromium.org/1808203005/diff/100001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/vr/vr_service.mojom:5: module blink.mojom; On 2016/04/27 00:09:29, dcheng wrote: > Btw, let's make sure this file has noparent IPC OWNERS rules like the ones in > //content/common. Thanks! Sorry, but I am not clear. content/common specified per-file owner for mojom files removing the parent owners list. Did you meant that we need to have same model here? i.e. I should maintain the same per-file owners list in WebKit/public/platform/modules/vr/OWNERS - as present in content/common.
The CQ bit was checked by kphanee@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1808203005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1808203005/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/1808203005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRController.cpp (right): https://codereview.chromium.org/1808203005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.cpp:45: void VRController::getDevices(VRGetDevicesCallback* callback) getDevices() should take PassOwnPtr. See my below comment. https://codereview.chromium.org/1808203005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.cpp:95: delete callback; We should avoid calling manual delete. Can we make m_pendingGetDevicesCallbacks Deque<OwnPtr<VRGetDevicesCallback>>? Then you won't need to call manual delete. https://codereview.chromium.org/1808203005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRTypeConverters.cc (right): https://codereview.chromium.org/1808203005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRTypeConverters.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/1808203005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRTypeConverters.cc:8: Remove the empty line. https://codereview.chromium.org/1808203005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRTypeConverters.h (right): https://codereview.chromium.org/1808203005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRTypeConverters.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016
https://codereview.chromium.org/1808203005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRController.cpp (right): https://codereview.chromium.org/1808203005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.cpp:54: m_service->GetDevices(sameThreadBindForMojo(&VRController::OnGetDevices, this)); If multiple getDevices requests are made while one is still outstanding we should be able to just queue up the callbacks and fire them all off when the outstanding request returns. I don't see any benefit to making a separate IPC call for each request. https://codereview.chromium.org/1808203005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.cpp:92: } Non-critical idea: Seems like this could benefit from using a converter similar to the one at https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
Finally all the bots are compile happy. :) I will check the cause of webkit_unit_tests failure in some bots along with the current set of comments. Hopefully next patch should keep all bots happy. :) https://codereview.chromium.org/1808203005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRController.cpp (right): https://codereview.chromium.org/1808203005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.cpp:54: m_service->GetDevices(sameThreadBindForMojo(&VRController::OnGetDevices, this)); On 2016/04/27 18:09:28, bajones wrote: > If multiple getDevices requests are made while one is still outstanding we > should be able to just queue up the callbacks and fire them all off when the > outstanding request returns. I don't see any benefit to making a separate IPC > call for each request. Isn't the current code doing the same? We are queuing callback received in each request. Once the request comes back, we fire the callback from the queue. Are you suggesting that we don't call m_service->GetDevices for each VRController call and rather make a single call to GetDevices and in response fire all the queued callbacks together? If yes, when exactly should we be calling m_service->GetDevices? How do we know when to trigger the service call knowing we have queued all incoming requests? https://codereview.chromium.org/1808203005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.cpp:92: } On 2016/04/27 18:09:28, bajones wrote: > Non-critical idea: Seems like this could benefit from using a converter similar > to the one at > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Yes, currently it uses the converter added in VRTypeConverter.h. In next iteration, probably we can remove VRTypeConverter and add the relevant converter in MojoHelper.h itself. https://codereview.chromium.org/1808203005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.cpp:95: delete callback; On 2016/04/27 15:52:12, haraken wrote: > > We should avoid calling manual delete. > > Can we make m_pendingGetDevicesCallbacks Deque<OwnPtr<VRGetDevicesCallback>>? > Then you won't need to call manual delete. Since we were already calling manual delete in VRController::getDevices(), I went in the same path. I will try moving all logic to using OwnPtr.
Addressed all the comments given till now. Also, changed the WebKit/Source/modules/OWNERS file as @dcheng suggested - retain the content/common/ model of per-file ownership. Fixed the crash observed on trybots in webkit_unit_tests. PTAL! https://codereview.chromium.org/1808203005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRController.cpp (right): https://codereview.chromium.org/1808203005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.cpp:45: void VRController::getDevices(VRGetDevicesCallback* callback) On 2016/04/27 15:52:12, haraken wrote: > > getDevices() should take PassOwnPtr. See my below comment. > Done. https://codereview.chromium.org/1808203005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.cpp:95: delete callback; On 2016/04/27 15:52:12, haraken wrote: > > We should avoid calling manual delete. > > Can we make m_pendingGetDevicesCallbacks Deque<OwnPtr<VRGetDevicesCallback>>? > Then you won't need to call manual delete. Done. https://codereview.chromium.org/1808203005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRTypeConverters.cc (right): https://codereview.chromium.org/1808203005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRTypeConverters.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/04/27 15:52:13, haraken wrote: > > 2016 Done. https://codereview.chromium.org/1808203005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRTypeConverters.cc:8: On 2016/04/27 15:52:13, haraken wrote: > > Remove the empty line. Done. https://codereview.chromium.org/1808203005/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRTypeConverters.h (right): https://codereview.chromium.org/1808203005/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRTypeConverters.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/04/27 15:52:13, haraken wrote: > > 2016 Done.
The CQ bit was checked by kphanee@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1808203005/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1808203005/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/OWNERS (right): https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/OWNERS:11: # introducing new sandbox escapes. Can we make this change in a separate CL? https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRController.cpp (right): https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.cpp:82: VRGetDevicesCallback* callback = m_pendingGetDevicesCallbacks.takeFirst().get(); OwnPtr<VRGetDevicesCallback> callback = m_pendingGetDevicesCallbacks.takeFirst(); https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRController.h (right): https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.h:53: WebHMDSensorState* m_pendingSensorStateRequest; Why does this need to be a pointer?
https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/OWNERS (right): https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/OWNERS:11: # introducing new sandbox escapes. On 2016/04/28 at 14:50:03, haraken wrote: > Can we make this change in a separate CL? This change probably wants to be higher up too, probably in the top level owners.
https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/OWNERS (right): https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/OWNERS:11: # introducing new sandbox escapes. On 2016/04/28 at 15:19:28, esprehn wrote: > On 2016/04/28 at 14:50:03, haraken wrote: > > Can we make this change in a separate CL? > > This change probably wants to be higher up too, probably in the top level owners. This should just go in platform/modules/vr for now. I'm working on getting these rules to work recursively, but there's a problem that we need something like CSS's !important as well (since we want to prevent child rules from applying).
FYI, r.kasibhatla@: Once this lands I've got a fairly large change ready to go (https://codereview.chromium.org/1918143007/) that refactors much of the Blink interface. I agree that we need layout tests for this module, and I intend to create some myself, but it's probably best to hold off on that until the API has been updated so that the tests don't have to be immediately re-written.
https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/OWNERS (right): https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/OWNERS:11: # introducing new sandbox escapes. On 2016/04/28 17:27:19, dcheng wrote: > On 2016/04/28 at 15:19:28, esprehn wrote: > > On 2016/04/28 at 14:50:03, haraken wrote: > > > Can we make this change in a separate CL? > > > > This change probably wants to be higher up too, probably in the top level > owners. > > This should just go in platform/modules/vr for now. > > I'm working on getting these rules to work recursively, but there's a problem > that we need something like CSS's !important as well (since we want to prevent > child rules from applying). So, should I add this change to WebKit/public/platform/modules/vr/OWNERS or defer any change to OWNERS to a separate CL? https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRController.cpp (right): https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.cpp:82: VRGetDevicesCallback* callback = m_pendingGetDevicesCallbacks.takeFirst().get(); On 2016/04/28 14:50:03, haraken wrote: > > OwnPtr<VRGetDevicesCallback> callback = > m_pendingGetDevicesCallbacks.takeFirst(); Done. Will make the change. https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRController.h (right): https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.h:53: WebHMDSensorState* m_pendingSensorStateRequest; On 2016/04/28 14:50:03, haraken wrote: > > Why does this need to be a pointer? getSensorState() passes a reference to WebHMDSensorState variable which is expected to be populated. Taking a pointer to populate the same address variable, as passed by the callee of getSensorState() API.
https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/OWNERS (right): https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/OWNERS:11: # introducing new sandbox escapes. On 2016/04/28 18:08:33, RaviKasibhatla wrote: > On 2016/04/28 17:27:19, dcheng wrote: > > On 2016/04/28 at 15:19:28, esprehn wrote: > > > On 2016/04/28 at 14:50:03, haraken wrote: > > > > Can we make this change in a separate CL? > > > > > > This change probably wants to be higher up too, probably in the top level > > owners. > > > > This should just go in platform/modules/vr for now. > > > > I'm working on getting these rules to work recursively, but there's a problem > > that we need something like CSS's !important as well (since we want to prevent > > child rules from applying). > > So, should I add this change to WebKit/public/platform/modules/vr/OWNERS or > defer any change to OWNERS to a separate CL? Let's follow dcheng's comment. https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRController.h (right): https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.h:53: WebHMDSensorState* m_pendingSensorStateRequest; On 2016/04/28 18:08:33, RaviKasibhatla wrote: > On 2016/04/28 14:50:03, haraken wrote: > > > > Why does this need to be a pointer? > > getSensorState() passes a reference to WebHMDSensorState variable which is > expected to be populated. Taking a pointer to populate the same address > variable, as passed by the callee of getSensorState() API. How can the caller of getSensorState() know the timing when the pointer value is updated?
On 2016/04/28 19:16:15, haraken wrote: > How can the caller of getSensorState() know the timing when the pointer value is > updated? The call is synchronous. WaitForIncomingResponse() won't return until OnGetSensorState() has been called and returned.
On 2016/04/28 19:46:35, bajones wrote: > On 2016/04/28 19:16:15, haraken wrote: > > How can the caller of getSensorState() know the timing when the pointer value > is > > updated? > > The call is synchronous. WaitForIncomingResponse() won't return until > OnGetSensorState() has been called and returned. Makes sense.
https://codereview.chromium.org/1808203005/diff/160001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1808203005/diff/160001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:1945: GetServiceRegistry()->AddService<blink::mojom::VRService>( Why does this template argument need to be specified? https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRController.cpp (right): https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.cpp:75: resetSensor(0); Why is this always 0? https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRController.h (right): https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.h:29: void getDevices(PassOwnPtr<VRGetDevicesCallback>); Use std::unique_ptr in new code, here and elsewhere. https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRTypeConverters.h (right): https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRTypeConverters.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. Let's not add new type converters. Can we add StructTraits for this instead? Also, StructTraits and type converters should be covered by IPC OWNERS rules as well.
jam@chromium.org changed reviewers: + jam@chromium.org
drive-by: no need to add type convertor, it'll be simpler to use typemapping. see https://www.chromium.org/developers/design-documents/mojo/type-mapping. This allows you to use the blink types directly when using the generated bindings.
In regards to converters/struct traits/typemapping, I don't actually see much benefit in converting to the WebVR types at all, since they're almost immediately re-converted into members of the various interfaces. Is there a reason that the mojo types shouldn't be passed to, say, VRDevice directly? https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRController.cpp (right): https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.cpp:75: resetSensor(0); On 2016/04/28 19:59:44, dcheng wrote: > Why is this always 0? I don't see a reason for the call to be there at all. Instead we should probably be detaching m_service in this call.
On 2016/04/28 at 20:21:51, bajones wrote: > In regards to converters/struct traits/typemapping, I don't actually see much benefit in converting to the WebVR types at all, since they're almost immediately re-converted into members of the various interfaces. Is there a reason that the mojo types shouldn't be passed to, say, VRDevice directly? +1 as long as Blink folks are OK with using the mojom types instead of the Web types in Blink. I would rather avoid any type mapping for such simple types like these. > > https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/vr/VRController.cpp (right): > > https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/vr/VRController.cpp:75: resetSensor(0); > On 2016/04/28 19:59:44, dcheng wrote: > > Why is this always 0? > > I don't see a reason for the call to be there at all. Instead we should probably be detaching m_service in this call.
On 2016/04/28 20:21:51, bajones wrote: > In regards to converters/struct traits/typemapping, I don't actually see much > benefit in converting to the WebVR types at all, since they're almost > immediately re-converted into members of the various interfaces. Is there a > reason that the mojo types shouldn't be passed to, say, VRDevice directly? Excellent point; yes if there are no types that you need to use elsewhere (say in code that for some strong reason couldn't depend on mojo), then that's even better :)
Did I read that right that this is using sync ipcs? I don't think we want that in blink. You're hanging the main thread whenever you do it. Only worker threads should be able to make sync mojo ipcs (and possibly very old web apis like alert, but certainly nothing new like VR) -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Perhaps VR could use shared memory + polling for low latency input. Probably out of scope for this CL though :) On Thu, Apr 28, 2016 at 2:24 PM, Elliott Sprehn <esprehn@chromium.org> wrote: > Did I read that right that this is using sync ipcs? I don't think we want > that in blink. You're hanging the main thread whenever you do it. Only > worker threads should be able to make sync mojo ipcs (and possibly very old > web apis like alert, but certainly nothing new like VR) > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Perhaps VR could use shared memory + polling for low latency input. Probably out of scope for this CL though :) On Thu, Apr 28, 2016 at 2:24 PM, Elliott Sprehn <esprehn@chromium.org> wrote: > Did I read that right that this is using sync ipcs? I don't think we want > that in blink. You're hanging the main thread whenever you do it. Only > worker threads should be able to make sync mojo ipcs (and possibly very old > web apis like alert, but certainly nothing new like VR) > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Did I read that right that this is using sync ipcs? I don't think we want that in blink. You're hanging the main thread whenever you do it. Only worker threads should be able to make sync mojo ipcs (and possibly very old web apis like alert, but certainly nothing new like VR) -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
We should block the Mojo WaitForIncomingMessage call inside blink though with a presubmit. We can whitelist this usage I guess since it already exists. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/04/28 21:24:53, esprehn wrote: > Did I read that right that this is using sync ipcs? I don't think we want > that in blink. You're hanging the main thread whenever you do it. Only > worker threads should be able to make sync mojo ipcs (and possibly very old > web apis like alert, but certainly nothing new like VR) we've very recently revamped the mojo docs, see https://www.chromium.org/developers/design-documents/mojo sync is specified by [Sync], which this interface isn't using. it just has callbacks for method calls, but those are async.
On Thu, Apr 28, 2016 at 2:31 PM, Elliott Sprehn <esprehn@chromium.org> wrote: > We should block the Mojo WaitForIncomingMessage call inside blink though > with a presubmit. We can whitelist this usage I guess since it already > exists. > Personally, I'd like to just delete WaitForIncomingMessage - or at least rename it to WaitForIncomingMessageForTesting. Nobody should ever use it. -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Thu, Apr 28, 2016 at 2:31 PM, Elliott Sprehn <esprehn@chromium.org> wrote: > We should block the Mojo WaitForIncomingMessage call inside blink though > with a presubmit. We can whitelist this usage I guess since it already > exists. > Personally, I'd like to just delete WaitForIncomingMessage - or at least rename it to WaitForIncomingMessageForTesting. Nobody should ever use it. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
We should block the Mojo WaitForIncomingMessage call inside blink though with a presubmit. We can whitelist this usage I guess since it already exists. -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
This was a concern that was brought up when the method was originally implemented. The argument for allowing it is that the entire purpose of this API is to poll information from the hardware with as little latency as possible to enable smooth VR rendering. Event-driven updates have already proven a poor fit for this sort of usage due to difficulties synchronizing input updates with rAF-driven rendering. It's very easy to get two input updates in the span of a single frame, followed by no input the next frame, leading to jitter that's unacceptable in the context of VR. A Gamepad-style shared memory pool suffers similar problems while introducing more latency. This is the only sync call we plan to have in this API, and it's a pretty lightweight one. All other updates can reasonably be done asynchronously.
Quick follow up: I don't see any reason why [Sync] can't be used here, it simply wasn't available when the method was first implemented. I have no issue with WaitForIncomingMessage disappearing as long as there's a well-supported synchronous call mechanism.
On 2016/04/28 21:45:29, bajones wrote: > Quick follow up: I don't see any reason why [Sync] can't be used here, it simply > wasn't available when the method was first implemented. I have no issue with > WaitForIncomingMessage disappearing as long as there's a well-supported > synchronous call mechanism. ah, I got thrown off because I searched the patch for WaitForIncomingMessage but the code was actually calling WaitForIncomingResponse. +1 to not having WaitForIncomingResponse and using [Sync] instead if a sync IPC has to be used. The current usage appears buggy, since WaitForIncomingResponse is not meant to be used on interfaces with multiple methods with return values unless it's used for all methods. Otherwise it can return when another method's response came back (i.e. GetDevices). Agreed in general that we should avoid sync ipc where possible. battre is adding https://github.com/google/flatbuffers support soon; perhaps this vr code can use it?
There will be no supported sync mechanism on the main thread, that's not how the web works. The browser process might be busy at that time, hanging the main thread on an IPC is never acceptable. You should invest in making sure your data is sent through the BeginFrame source IPC if you need it at exactly that time. Note that whole engine is working towards vsync aligned input. You shouldn't ever get unaligned input in the future. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Apr 28, 2016 5:54 PM, <jam@chromium.org> wrote: > > ... > > Agreed in general that we should avoid sync ipc where possible. battre is adding > https://github.com/google/flatbuffers support soon; perhaps this vr code can use > it? +1 using flatbuffers seems like a great idea if you need polling of real time data. :) -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
There will be no supported sync mechanism on the main thread, that's not how the web works. The browser process might be busy at that time, hanging the main thread on an IPC is never acceptable. You should invest in making sure your data is sent through the BeginFrame source IPC if you need it at exactly that time. Note that whole engine is working towards vsync aligned input. You shouldn't ever get unaligned input in the future. -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Apr 28, 2016 5:54 PM, <jam@chromium.org> wrote: > > ... > > Agreed in general that we should avoid sync ipc where possible. battre is adding > https://github.com/google/flatbuffers support soon; perhaps this vr code can use > it? +1 using flatbuffers seems like a great idea if you need polling of real time data. :) -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Forgive my unfamiliarity with flatbuffers. I've been under the impression that they're something akin to protobufs: an efficient serialization format. What advantage do they have for real time polling?
On 2016/04/28 22:00:23, esprehn wrote: > There will be no supported sync mechanism on the main thread, that's not > how the web works. The browser process might be busy at that time, hanging > the main thread on an IPC is never acceptable. That doesn't reflect how the renderer works today. While sync IPCs have many problems (performance, stability) and they should be avoided unless absolutely necessary, we have to use them to implement web platform APIS that weren't designed with multi-process browsers in mind. i.e. off the top of my head, things like cookies, plugins, workers, message ports, IDB, XHR, SQL, clipboard all need sync IPCs. I do absolutely agree that for new web platform apis, they should be designed such that we can implement them without the need for sync IPCs. > > You should invest in making sure your data is sent through the BeginFrame > source IPC if you need it at exactly that time. Note that whole engine is > working towards vsync aligned input. You shouldn't ever get unaligned input > in the future.
On 2016/04/28 22:15:16, bajones wrote: > Forgive my unfamiliarity with flatbuffers. I've been under the impression that > they're something akin to protobufs: an efficient serialization format. What > advantage do they have for real time polling? They allow arbitrary data structures to be written by browser and ready by any child process without having to do sync IPCs.
On Thu, Apr 28, 2016 at 2:37 PM, Ken Rockot <rockot@chromium.org> wrote: > > > On Thu, Apr 28, 2016 at 2:31 PM, Elliott Sprehn <esprehn@chromium.org> > wrote: > >> We should block the Mojo WaitForIncomingMessage call inside blink though >> with a presubmit. We can whitelist this usage I guess since it already >> exists. >> > Personally, I'd like to just delete WaitForIncomingMessage - or at least > rename it to WaitForIncomingMessageForTesting. Nobody should ever use it. > +1 -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Thu, Apr 28, 2016 at 2:37 PM, Ken Rockot <rockot@chromium.org> wrote: > > > On Thu, Apr 28, 2016 at 2:31 PM, Elliott Sprehn <esprehn@chromium.org> > wrote: > >> We should block the Mojo WaitForIncomingMessage call inside blink though >> with a presubmit. We can whitelist this usage I guess since it already >> exists. >> > Personally, I'd like to just delete WaitForIncomingMessage - or at least > rename it to WaitForIncomingMessageForTesting. Nobody should ever use it. > +1 -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Also, sorry for asking, but I got lost about what to do about the usage of sync mojo call in VRController::getSensorState. Should I keep using the WaitForIncomingResponse() for now and make another CL changing it to async or [Sync]? Or Should I try to make it async in this CL itself? Or Should I instead use [Sync] in this CL? I have no idea what it is though :) Or Should I wait for using flatbuffers (like battery)? I believe that support is yet to be added, so this doesn't seem to be an immediate option for this CL. https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRController.cpp (right): https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.cpp:75: resetSensor(0); On 2016/04/28 20:21:51, bajones wrote: > On 2016/04/28 19:59:44, dcheng wrote: > > Why is this always 0? > > I don't see a reason for the call to be there at all. Instead we should probably > be detaching m_service in this call. Agree. I have removed the resetSensor(0). However, for detaching the m_service, currently there is no call from ServiceRegistry.h. However, the content/common/ header does provide a RemoveService API. Should I add the RemoveService API to blink platform in this CL itself? Or Should I take it up in another CL? https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.cpp:82: VRGetDevicesCallback* callback = m_pendingGetDevicesCallbacks.takeFirst().get(); On 2016/04/28 14:50:03, haraken wrote: > > OwnPtr<VRGetDevicesCallback> callback = > m_pendingGetDevicesCallbacks.takeFirst(); Done. https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRController.h (right): https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.h:29: void getDevices(PassOwnPtr<VRGetDevicesCallback>); On 2016/04/28 19:59:44, dcheng wrote: > Use std::unique_ptr in new code, here and elsewhere. IIRC, @yutak recently sent a broadcast that the transition work for using std::unique_ptr in place of OwnPtr is ready to land. But he also mentioned that there is next step of work which will make OwnPtr behavior as close as possible to std::unique_ptr. Shouldn't we transition to unique_ptr after announcement from yutak that it is good to go? https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRTypeConverters.h (right): https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRTypeConverters.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/04/28 19:59:44, dcheng wrote: > Let's not add new type converters. Can we add StructTraits for this instead? > > Also, StructTraits and type converters should be covered by IPC OWNERS rules as > well. I am checking at the possibility of using mojom types all through in blink, as bajones suggested, if that's acceptable to all. Or for now should I try to use StructTraits instead and keep using Web types in blink?
On Fri, Apr 29, 2016 at 7:49 AM, <kphanee@chromium.org> wrote: > Also, sorry for asking, but I got lost about what to do about the usage of > sync > mojo call in VRController::getSensorState. > > Should I keep using the WaitForIncomingResponse() for now and make another > CL > changing it to async or [Sync]? > I would personally prefer that migration CLs like this change as little as necessary. To that end IMHO any change to the IPC calls should be done in a follow-up CL after more discussion. It's not like this is introducing *new* sync operations - that ship has already sailed. > > Or Should I try to make it async in this CL itself? > Or Should I instead use [Sync] in this CL? > I have no idea what it is though :) > Or Should I wait for using flatbuffers (like battery)? I believe that > support is > yet to be added, so this doesn't seem to be an immediate option for this > CL. > > > > https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/vr/VRController.cpp (right): > > > https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/vr/VRController.cpp:75: > resetSensor(0); > On 2016/04/28 20:21:51, bajones wrote: > > On 2016/04/28 19:59:44, dcheng wrote: > > > Why is this always 0? > > > > I don't see a reason for the call to be there at all. Instead we > should probably > > be detaching m_service in this call. > > Agree. I have removed the resetSensor(0). However, for detaching the > m_service, currently there is no call from ServiceRegistry.h. However, > the content/common/ header does provide a RemoveService API. Should I > add the RemoveService API to blink platform in this CL itself? > Or Should I take it up in another CL? > > > https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/vr/VRController.cpp:82: > VRGetDevicesCallback* callback = > m_pendingGetDevicesCallbacks.takeFirst().get(); > On 2016/04/28 14:50:03, haraken wrote: > > > > OwnPtr<VRGetDevicesCallback> callback = > > m_pendingGetDevicesCallbacks.takeFirst(); > > Done. > > > https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/vr/VRController.h (right): > > > https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/vr/VRController.h:29: void > getDevices(PassOwnPtr<VRGetDevicesCallback>); > On 2016/04/28 19:59:44, dcheng wrote: > > Use std::unique_ptr in new code, here and elsewhere. > > IIRC, @yutak recently sent a broadcast that the transition work for > using std::unique_ptr in place of OwnPtr is ready to land. But he also > mentioned that there is next step of work which will make OwnPtr > behavior as close as possible to std::unique_ptr. > Shouldn't we transition to unique_ptr after announcement from yutak that > it is good to go? > > > https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/vr/VRTypeConverters.h (right): > > > https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/vr/VRTypeConverters.h:1: // Copyright > 2016 The Chromium Authors. All rights reserved. > On 2016/04/28 19:59:44, dcheng wrote: > > Let's not add new type converters. Can we add StructTraits for this > instead? > > > > Also, StructTraits and type converters should be covered by IPC OWNERS > rules as > > well. > > I am checking at the possibility of using mojom types all through in > blink, as bajones suggested, if that's acceptable to all. > Or for now should I try to use StructTraits instead and keep using Web > types in blink? > > https://codereview.chromium.org/1808203005/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Fri, Apr 29, 2016 at 7:49 AM, <kphanee@chromium.org> wrote: > Also, sorry for asking, but I got lost about what to do about the usage of > sync > mojo call in VRController::getSensorState. > > Should I keep using the WaitForIncomingResponse() for now and make another > CL > changing it to async or [Sync]? > I would personally prefer that migration CLs like this change as little as necessary. To that end IMHO any change to the IPC calls should be done in a follow-up CL after more discussion. It's not like this is introducing *new* sync operations - that ship has already sailed. > > Or Should I try to make it async in this CL itself? > Or Should I instead use [Sync] in this CL? > I have no idea what it is though :) > Or Should I wait for using flatbuffers (like battery)? I believe that > support is > yet to be added, so this doesn't seem to be an immediate option for this > CL. > > > > https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/vr/VRController.cpp (right): > > > https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/vr/VRController.cpp:75: > resetSensor(0); > On 2016/04/28 20:21:51, bajones wrote: > > On 2016/04/28 19:59:44, dcheng wrote: > > > Why is this always 0? > > > > I don't see a reason for the call to be there at all. Instead we > should probably > > be detaching m_service in this call. > > Agree. I have removed the resetSensor(0). However, for detaching the > m_service, currently there is no call from ServiceRegistry.h. However, > the content/common/ header does provide a RemoveService API. Should I > add the RemoveService API to blink platform in this CL itself? > Or Should I take it up in another CL? > > > https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/vr/VRController.cpp:82: > VRGetDevicesCallback* callback = > m_pendingGetDevicesCallbacks.takeFirst().get(); > On 2016/04/28 14:50:03, haraken wrote: > > > > OwnPtr<VRGetDevicesCallback> callback = > > m_pendingGetDevicesCallbacks.takeFirst(); > > Done. > > > https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/vr/VRController.h (right): > > > https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/vr/VRController.h:29: void > getDevices(PassOwnPtr<VRGetDevicesCallback>); > On 2016/04/28 19:59:44, dcheng wrote: > > Use std::unique_ptr in new code, here and elsewhere. > > IIRC, @yutak recently sent a broadcast that the transition work for > using std::unique_ptr in place of OwnPtr is ready to land. But he also > mentioned that there is next step of work which will make OwnPtr > behavior as close as possible to std::unique_ptr. > Shouldn't we transition to unique_ptr after announcement from yutak that > it is good to go? > > > https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/vr/VRTypeConverters.h (right): > > > https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/vr/VRTypeConverters.h:1: // Copyright > 2016 The Chromium Authors. All rights reserved. > On 2016/04/28 19:59:44, dcheng wrote: > > Let's not add new type converters. Can we add StructTraits for this > instead? > > > > Also, StructTraits and type converters should be covered by IPC OWNERS > rules as > > well. > > I am checking at the possibility of using mojom types all through in > blink, as bajones suggested, if that's acceptable to all. > Or for now should I try to use StructTraits instead and keep using Web > types in blink? > > https://codereview.chromium.org/1808203005/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Apr 28, 2016 7:28 PM, <jam@chromium.org> wrote: > > On 2016/04/28 22:00:23, esprehn wrote: > > There will be no supported sync mechanism on the main thread, that's not > > how the web works. The browser process might be busy at that time, hanging > > the main thread on an IPC is never acceptable. > > That doesn't reflect how the renderer works today. While sync IPCs have many > problems (performance, stability) and they should be avoided unless absolutely > necessary, we have to use them to implement web platform APIS that weren't > designed with multi-process browsers in mind. i.e. off the top of my head, > things like cookies, plugins, workers, message ports, IDB, XHR, SQL, clipboard > all need sync IPCs. > IDB, Workers and MesaagePorts are all async, they shouldn't need sync ipcs. Sync XHR is deprecated and we plan to remove it eventually, same with web SQL. Clipboard and cookies are unfortunate mistakes. Plugins will probably go away over time, they're not really supported on mobile anyway. You're just repeating what I already said though. No new apis should use sync ipcs and no supported mechanism will be provided for it from the main thread *except* to support the old web apis. WebVR is brand new, it has no reason to use sync ipcs. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Apr 28, 2016 7:28 PM, <jam@chromium.org> wrote: > > On 2016/04/28 22:00:23, esprehn wrote: > > There will be no supported sync mechanism on the main thread, that's not > > how the web works. The browser process might be busy at that time, hanging > > the main thread on an IPC is never acceptable. > > That doesn't reflect how the renderer works today. While sync IPCs have many > problems (performance, stability) and they should be avoided unless absolutely > necessary, we have to use them to implement web platform APIS that weren't > designed with multi-process browsers in mind. i.e. off the top of my head, > things like cookies, plugins, workers, message ports, IDB, XHR, SQL, clipboard > all need sync IPCs. > IDB, Workers and MesaagePorts are all async, they shouldn't need sync ipcs. Sync XHR is deprecated and we plan to remove it eventually, same with web SQL. Clipboard and cookies are unfortunate mistakes. Plugins will probably go away over time, they're not really supported on mobile anyway. You're just repeating what I already said though. No new apis should use sync ipcs and no supported mechanism will be provided for it from the main thread *except* to support the old web apis. WebVR is brand new, it has no reason to use sync ipcs. -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2016/04/29 16:59:40, esprehn wrote: > IDB, Workers and MesaagePorts are all async, they shouldn't need sync ipcs. > Sync XHR is deprecated and we plan to remove it eventually, same with web > SQL. Clipboard and cookies are unfortunate mistakes. Plugins will probably > go away over time, they're not really supported on mobile anyway. > > You're just repeating what I already said though. No new apis should use > sync ipcs and no supported mechanism will be provided for it from the main > thread *except* to support the old web apis. WebVR is brand new, it has no > reason to use sync ipcs. I'm happy to continue this conversation and find an alternate way to provide low-latency polling for this API. For the sake of keeping this CL focused on the task at hand, however, can we agree that the right thing to do for the moment is to switch this call over to using a [Sync] call instead of the current reliance on WaitForIncomingResponse()?
Yeah totally sorry we got lost in the weeds here. This CL shouldn't change any more than is absolutely necessary. :) -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRController.h (right): https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.h:29: void getDevices(PassOwnPtr<VRGetDevicesCallback>); On 2016/04/29 at 14:49:05, RaviKasibhatla wrote: > On 2016/04/28 19:59:44, dcheng wrote: > > Use std::unique_ptr in new code, here and elsewhere. > > IIRC, @yutak recently sent a broadcast that the transition work for using std::unique_ptr in place of OwnPtr is ready to land. But he also mentioned that there is next step of work which will make OwnPtr behavior as close as possible to std::unique_ptr. > Shouldn't we transition to unique_ptr after announcement from yutak that it is good to go? No, new code should just use std::unique_ptr. It will save us migration work down the road.
Yeah totally sorry we got lost in the weeds here. This CL shouldn't change any more than is absolutely necessary. :) -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Replaced OwnPtr/PassOwnPtr with std::unique_ptr. Removed using WaitForIncomingResponse and instead made it a [Sync] mojo call. @dcheng: Can we take up defining StructTraits in a separate CL? I still lot very good usage of type_converter for many mojo calls. @bajones: Can we take up the detach from mojo service in willDetachFrameHost in a separate CL? As of now, there is no call plumbed to invoke RemoveService into blink code. https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/OWNERS (right): https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/OWNERS:11: # introducing new sandbox escapes. On 2016/04/28 19:16:15, haraken wrote: > On 2016/04/28 18:08:33, RaviKasibhatla wrote: > > On 2016/04/28 17:27:19, dcheng wrote: > > > On 2016/04/28 at 15:19:28, esprehn wrote: > > > > On 2016/04/28 at 14:50:03, haraken wrote: > > > > > Can we make this change in a separate CL? > > > > > > > > This change probably wants to be higher up too, probably in the top level > > > owners. > > > > > > This should just go in platform/modules/vr for now. > > > > > > I'm working on getting these rules to work recursively, but there's a > problem > > > that we need something like CSS's !important as well (since we want to > prevent > > > child rules from applying). > > > > So, should I add this change to WebKit/public/platform/modules/vr/OWNERS or > > defer any change to OWNERS to a separate CL? > > Let's follow dcheng's comment. Done. https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRController.cpp (right): https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.cpp:75: resetSensor(0); On 2016/04/29 14:49:05, RaviKasibhatla wrote: > On 2016/04/28 20:21:51, bajones wrote: > > On 2016/04/28 19:59:44, dcheng wrote: > > > Why is this always 0? > > > > I don't see a reason for the call to be there at all. Instead we should > probably > > be detaching m_service in this call. > > Agree. I have removed the resetSensor(0). However, for detaching the m_service, > currently there is no call from ServiceRegistry.h. However, the content/common/ > header does provide a RemoveService API. Should I add the RemoveService API to > blink platform in this CL itself? > Or Should I take it up in another CL? @bajones: Can we take the detaching of service in separate CL with a FIXME added for now? https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRController.h (right): https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.h:29: void getDevices(PassOwnPtr<VRGetDevicesCallback>); On 2016/04/29 17:28:11, dcheng wrote: > On 2016/04/29 at 14:49:05, RaviKasibhatla wrote: > > On 2016/04/28 19:59:44, dcheng wrote: > > > Use std::unique_ptr in new code, here and elsewhere. > > > > IIRC, @yutak recently sent a broadcast that the transition work for using > std::unique_ptr in place of OwnPtr is ready to land. But he also mentioned that > there is next step of work which will make OwnPtr behavior as close as possible > to std::unique_ptr. > > Shouldn't we transition to unique_ptr after announcement from yutak that it is > good to go? > > No, new code should just use std::unique_ptr. It will save us migration work > down the road. Done. https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRTypeConverters.h (right): https://codereview.chromium.org/1808203005/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRTypeConverters.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/04/28 19:59:44, dcheng wrote: > Let's not add new type converters. Can we add StructTraits for this instead? > > Also, StructTraits and type converters should be covered by IPC OWNERS rules as > well. @dcheng: By StructTraits, are you referring to the file like: https://code.google.com/p/chromium/codesearch#chromium/src/url/mojo/url_gurl_... I was checking in the current codebase and noticed StructTraits have been defined for very few files - apart from origin.mojom and url.mojom, few files in tests. Is this the preferred method over type_converters? I still see most of the implementations using type_converters. Can this be taken up in separate CL?
On 2016/05/03 14:34:22, RaviKasibhatla wrote: > @bajones: Can we take up the detach from mojo service in willDetachFrameHost in > a separate CL? As of now, there is no call plumbed to invoke RemoveService into > blink code. Yes, definitely! Let's get this landed and follow up with the requested improvements. (I'm eager to help with that!) content/browser/vr/ and modules/vr/ LGTM
modules/ LGTM. https://codereview.chromium.org/1808203005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRController.cpp (right): https://codereview.chromium.org/1808203005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.cpp:72: // FIXME:: Detach from the mojo service connection. TODO(your name)
Was there a conclusion to whether or not to even use StructTraits? It looks like it might be simpler to just use the mojom types directly.
On 2016/05/03 17:15:36, dcheng wrote: > Was there a conclusion to whether or not to even use StructTraits? It looks like > it might be simpler to just use the mojom types directly. I suggested using mojom types directly and it was well received. RaviKasibhatla asked to push that to another CL for the sake of keeping this one tightly scoped. I'm happy to make that change myself, since I have multiple other related CLs that are blocked on this landing.
lgtm https://codereview.chromium.org/1808203005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRController.h (right): https://codereview.chromium.org/1808203005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.h:13: #include "wtf/PassOwnPtr.h" Not needed anymore. But #include <memory> please.
The CQ bit was checked by kphanee@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1808203005/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1808203005/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by kphanee@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1808203005/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1808203005/240001
@esprrehn/@jam, PTAL! https://codereview.chromium.org/1808203005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRController.cpp (right): https://codereview.chromium.org/1808203005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.cpp:72: // FIXME:: Detach from the mojo service connection. On 2016/05/03 15:56:31, haraken wrote: > > TODO(your name) Done. https://codereview.chromium.org/1808203005/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRController.h (right): https://codereview.chromium.org/1808203005/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.h:13: #include "wtf/PassOwnPtr.h" On 2016/05/03 21:48:14, dcheng wrote: > Not needed anymore. But #include <memory> please. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
@esprrehn/@jam review ping on Ravi's behalf. Eager to push some follow-up CLs after this lands.
lgtm, we can just use the Mojo POD types for the data around the codebase if you want too. Now that we have WTF friendly mojo objects you don't need to convert to WebVRVector3 and such. https://codereview.chromium.org/1808203005/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/NavigatorVRDevice.cpp (right): https://codereview.chromium.org/1808203005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/NavigatorVRDevice.cpp:62: controller()->getDevices(std::move(callback)); You want to use WTF::wrapUnique here. https://codereview.chromium.org/1808203005/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRController.h (right): https://codereview.chromium.org/1808203005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.h:52: WTF::Deque<std::unique_ptr<VRGetDevicesCallback>> m_pendingGetDevicesCallbacks; Leave off the WTF::
On 2016/05/06 03:14:53, esprehn wrote: > lgtm, we can just use the Mojo POD types for the data around the codebase if you > want too. Now that we have WTF friendly mojo objects you don't need to convert > to WebVRVector3 and such. I've got the code for this transition ready to go (https://chromium.googlesource.com/experimental/chromium/src/+/e396dce998887d4...)
@jam: Gentle Ping for review :) https://codereview.chromium.org/1808203005/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/NavigatorVRDevice.cpp (right): https://codereview.chromium.org/1808203005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/NavigatorVRDevice.cpp:62: controller()->getDevices(std::move(callback)); On 2016/05/06 03:14:53, esprehn wrote: > You want to use WTF::wrapUnique here. Done. https://codereview.chromium.org/1808203005/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/vr/VRController.h (right): https://codereview.chromium.org/1808203005/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/vr/VRController.h:52: WTF::Deque<std::unique_ptr<VRGetDevicesCallback>> m_pendingGetDevicesCallbacks; On 2016/05/06 03:14:53, esprehn wrote: > Leave off the WTF:: Done.
On 2016/05/06 09:15:49, RaviKasibhatla wrote: > @jam: Gentle Ping for review :) sorry I missed this. Can you remove the type converters and just use the mojo type in this cl?
On 2016/05/06 16:05:21, jam wrote: > On 2016/05/06 09:15:49, RaviKasibhatla wrote: > > @jam: Gentle Ping for review :) > > sorry I missed this. > > Can you remove the type converters and just use the mojo type in this cl? I think the hope is to keep this CL more narrowly focused. As I mentioned to esprehn@, I've got the code that removes the type converters and uses Mojo types directly ready to go once this lands: https://chromium.googlesource.com/experimental/chromium/src/+/e396dce998887d4...
On 2016/05/06 16:10:20, bajones wrote: > On 2016/05/06 16:05:21, jam wrote: > > On 2016/05/06 09:15:49, RaviKasibhatla wrote: > > > @jam: Gentle Ping for review :) > > > > sorry I missed this. > > > > Can you remove the type converters and just use the mojo type in this cl? > > I think the hope is to keep this CL more narrowly focused. As I mentioned to > esprehn@, I've got the code that removes the type converters and uses Mojo types > directly ready to go once this lands: > https://chromium.googlesource.com/experimental/chromium/src/+/e396dce998887d4... sounds reasonable thanks, lgtm
The CQ bit was checked by bajones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bajones@chromium.org, dcheng@chromium.org, haraken@chromium.org, esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/1808203005/#ps280001 (title: "Rebase to ToT!")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1808203005/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1808203005/280001
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== [OnionSoup] Moving VR service from content to Blink Migrate vr service from content/renderer to third_party/WebKit/Source/modules. This CL removes the files - content/renderer/vr/vr_dispatcher.* - WebKit/public/platform/modules/vr/WebVRClient.h since functionality is folded into WebKit/Source/modules/vr/VRController.h.* This CL also moves the files - content/renderer/vr/vr_type_converters.* to WebKit/Source/modules/vr/VRTypeConverters.h.* - content/common/vr_service.mojom to WebKit/public/platform/modules/vr/vr_service.mojom VRController has been modified to talk directly to the mojo service impl in browser's render frame host. * Plan to rewrite the mock service of vr/ for layout tests based on JS-bindings of Mojo. * Plan to fix unit tests present in chrome/browser using fake service. * Plan to add unit tests in WebKit/Source/modules if required for vr service using fake service. BUG=593607 TESTS=NONE CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== [OnionSoup] Moving VR service from content to Blink Migrate vr service from content/renderer to third_party/WebKit/Source/modules. This CL removes the files - content/renderer/vr/vr_dispatcher.* - WebKit/public/platform/modules/vr/WebVRClient.h since functionality is folded into WebKit/Source/modules/vr/VRController.h.* This CL also moves the files - content/renderer/vr/vr_type_converters.* to WebKit/Source/modules/vr/VRTypeConverters.h.* - content/common/vr_service.mojom to WebKit/public/platform/modules/vr/vr_service.mojom VRController has been modified to talk directly to the mojo service impl in browser's render frame host. * Plan to rewrite the mock service of vr/ for layout tests based on JS-bindings of Mojo. * Plan to fix unit tests present in chrome/browser using fake service. * Plan to add unit tests in WebKit/Source/modules if required for vr service using fake service. BUG=593607 TESTS=NONE CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/9519542992a024e3c90c29b25c00d8bd87e71480 Cr-Commit-Position: refs/heads/master@{#392144} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/9519542992a024e3c90c29b25c00d8bd87e71480 Cr-Commit-Position: refs/heads/master@{#392144} |
