Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1475563002:
arc-bridge: Implement IPC message for app launcher (Closed)
|
Created:
5 years ago by khmel Modified:
5 years ago Reviewers:
elijahtaylor1, khmel1, jochen (gone - plz use gerrit), Luis Héctor Chávez, oshima, dcheng, Jorge Lucangeli Obes, hidehiko, yoshiki, satorux1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc-bridge: Implement IPC message for app launcher.
This CL comes along with:
https://codereview.chromium.org/1481523002 (split arc_bride_service)
https://codereview.chromium.org/1464353003 (fake_arc_bridge)
https://codereview.chromium.org/1413153007 (app_list)
This includes set of IPC messages to interact with Android
via ARC bridge service in chromeos.
BUG=558209
BUG=b/24545231
TEST=Build and deploy android, chrome, chromium CLs. Apps
appear in chromium app launcher. Once clicked, app is
started in Android window.
TEST=build Chrome with enable_arc=1 Debug/Release
TEST=unit_tests with enable_arc=1 Debug/Release
(Together with all required CLs)
Committed: https://crrev.com/6644c3726855d20038fc1a30d37afdc072f83632
Cr-Commit-Position: refs/heads/master@{#362895}
Patch Set 1 #Patch Set 2 : Reorder lines to reduce diff #Patch Set 3 : Add missing files #
Total comments: 2
Patch Set 4 : Use creator for ArcBridgeService #
Total comments: 26
Patch Set 5 : move spliting arc_bridge_service to another CL #
Total comments: 1
Patch Set 6 : few more nits addressed #Patch Set 7 : Rebased (change to use switch for arc) #
Total comments: 2
Patch Set 8 : Rebases, used IPC_STRUCT_ macros #Patch Set 9 : Add AppsObserver #
Total comments: 16
Patch Set 10 : Renaming and nits #
Total comments: 6
Patch Set 11 : Add arc::ScaleFactor enum #
Total comments: 9
Dependent Patchsets: Messages
Total messages: 73 (20 generated)
Description was changed from
==========
arc-bridge: Minimal support for ARC app launcher.
This CL comes along with:
This includes set of IPC messages to interact with android via ARC
bridge service in chromeos. This also refactores ArcBridgeService
in order to be able to implement FakeArcBridgeService.
BUG=558209
BUG=b/24545231
TEST=Build and deploy android, chrome, chromium CLs. Apps
appear in chromium app launcher. Once clicked, app is
started in Android window.
TEST=build Chrome with enable_arc=1 Debug/Release
TEST=unit_tests with enable_arc=1 Debug/Release
(Together with all required CLs)
==========
to
==========
arc-bridge: Minimal support for ARC app launcher.
This CL comes along with:
https://codereview.chromium.org/1464353003 (fake_arc_bridge)
https://codereview.chromium.org/1413153007 (app_list)
This includes set of IPC messages to interact with Android
via ARC bridge service in chromeos. This also refactores
ArcBridgeService in order to be able to implement
FakeArcBridgeService.
BUG=558209
BUG=b/24545231
TEST=Build and deploy android, chrome, chromium CLs. Apps
appear in chromium app launcher. Once clicked, app is
started in Android window.
TEST=build Chrome with enable_arc=1 Debug/Release
TEST=unit_tests with enable_arc=1 Debug/Release
(Together with all required CLs)
==========
khmel@google.com changed reviewers: + dcheng@chromium.org, elijahtaylor@chromium.org, jochen@chromium.org, lhchavez@chromium.org
khmel@google.com changed reviewers: + khmel@google.com
This is result of splitting off: https://codereview.chromium.org/1413153007 PTAL
khmel@google.com changed reviewers: + oshima@chromium.org
https://codereview.chromium.org/1475563002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/chrome_browser_main_chromeos.h (right): https://codereview.chromium.org/1475563002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/chrome_browser_main_chromeos.h:87: scoped_ptr<arc::ArcBridgeServiceImpl> arc_bridge_service_; why are you directly using the Impl instead of having a factory method or something in the arc component that gives you something that implements the interface?
Used creator to create ArcBridgeService. PTAL https://codereview.chromium.org/1475563002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/chrome_browser_main_chromeos.h (right): https://codereview.chromium.org/1475563002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/chrome_browser_main_chromeos.h:87: scoped_ptr<arc::ArcBridgeServiceImpl> arc_bridge_service_; On 2015/11/24 13:14:03, jochen wrote: > why are you directly using the Impl instead of having a factory method or > something in the arc component that gives you something that implements the > interface? Good point, thanks. Done
khmel@google.com changed reviewers: + hidehiko@chromium.org, satorux@chromium.org
Satoru-san, Could you please alternatively review chrome/browser/chromeos/chrome_browser_main_chromeos.cc? Hidehiko-san, Could you please take a look into components/arc changes once Elijah might be busy now?
dcheng@chromium.org changed reviewers: + jorgelo@chromium.org - hidehiko@chromium.org, satorux@chromium.org
Also, I think I've mentioned this in the past, but I don't feel like I understand enough about the security properties of arc++ to be able to do a meaningful security review. +jorgelo, in case he has any other thoughts https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... components/arc/arc_bridge_service.h:85: const std::vector<uint8_t>& icon_png_data) {} Where does icon_png_data originate from? Is this something controlled by the app in question? Will we end up decoding it in the Chrome browser process somewhere? It's not clear from the current code, since it's hidden behind an observer implementation.
satorux@chromium.org changed reviewers: + satorux@chromium.org
chrome/browser/chromeos/chrome_browser_main_chromeos.cc lgtm but one question: https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... File components/arc/arc_bridge_service_impl.cc (right): https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... components/arc/arc_bridge_service_impl.cc:308: ArcBridgeService* ArcBridgeService::Create( shouldn't this be defined in arc_bridge_service.cc? I wanted to tale a look at this function but got confused because it's not in arc_bridge_service.cc
I think you forgot to add hidehiko, but non-security stuff lgtm based on it being a copy of the bigger CL that I reviewed.
https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... components/arc/arc_bridge_service.h:85: const std::vector<uint8_t>& icon_png_data) {} On 2015/11/25 06:34:07, dcheng wrote: > Where does icon_png_data originate from? Is this something controlled by the app > in question? Will we end up decoding it in the Chrome browser process somewhere? > It's not clear from the current code, since it's hidden behind an observer > implementation. This png is encoded in Android: https://googleplex-android-review.git.corp.google.com/#/c/797530/7/arcapplaun... line 194 We decode it as an icon for arc app icon in App Launcher (Via ImageDecoder). https://codereview.chromium.org/1413153007/diff/320001/chrome/browser/ui/app_... https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... File components/arc/arc_bridge_service_impl.cc (right): https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... components/arc/arc_bridge_service_impl.cc:308: ArcBridgeService* ArcBridgeService::Create( On 2015/11/25 06:35:08, satorux1 wrote: > shouldn't this be defined in arc_bridge_service.cc? > > I wanted to tale a look at this function but got confused because it's not in > arc_bridge_service.cc It was not clear for me how to do better. But I found template in legacy code: https://code.google.com/p/chromium/codesearch#chromium/src/components/wifi/wi... If I move this function to arc_bridge_service.cc then I need to include arc_bridge_service_impl.h also, which my be not acceptable.
https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... File components/arc/arc_bridge_service_impl.cc (right): https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... components/arc/arc_bridge_service_impl.cc:308: ArcBridgeService* ArcBridgeService::Create( On 2015/11/25 06:51:08, khmel1 wrote: > On 2015/11/25 06:35:08, satorux1 wrote: > > shouldn't this be defined in arc_bridge_service.cc? > > > > I wanted to tale a look at this function but got confused because it's not in > > arc_bridge_service.cc > > It was not clear for me how to do better. But I found template in legacy code: > https://code.google.com/p/chromium/codesearch#chromium/src/components/wifi/wi... > If I move this function to arc_bridge_service.cc then I need to include > arc_bridge_service_impl.h also, which my be not acceptable. I think including arc_bridge_service_impl.h from arc_bridge_service.cc is alright.
https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... File components/arc/arc_bridge_service_impl.cc (right): https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... components/arc/arc_bridge_service_impl.cc:308: ArcBridgeService* ArcBridgeService::Create( FWIW, I played with a one liner to find foo.cc that includes Create() with corresponding foo_impl.cc % for i in chrome/browser/**/*_impl.cc; do grep -H '^[^ ].*::Create(' ${i:s/_impl.cc/.cc/}; done 2> /dev/null chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper.cc:scoped_ptr<EnterpriseEnrollmentHelper> EnterpriseEnrollmentHelper::Create( chrome/browser/local_discovery/gcd_api_flow.cc:scoped_ptr<GCDApiFlow> GCDApiFlow::Create( chrome/browser/ui/autofill/autofill_dialog_controller.cc:AutofillDialogController::Create( Returning scoped_ptr<> looks neat. Can we do this here?
hidehiko@chromium.org changed reviewers: + hidehiko@chromium.org
Can you split this into two? - Extract "impl" code. - Add your new IPC implementation. https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... File components/arc/arc_bridge_service.cc (right): https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... components/arc/arc_bridge_service.cc:61: // DCHECK on enum classes not supported. The comment looks unrelated to the code? https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... components/arc/arc_bridge_service.h:91: ArcBridgeService(); Please move them to protected section. https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... File components/arc/arc_bridge_service_impl.cc (right): https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... components/arc/arc_bridge_service_impl.cc:273: DCHECK(names.size() == packages.size() && names.size() == activities.size()); Can we have std::vector<AppInfo> where struct AppInfo { std::string name; std::string package; std::string activity; }; ? https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... components/arc/arc_bridge_service_impl.cc:308: ArcBridgeService* ArcBridgeService::Create( On 2015/11/25 07:18:56, satorux1 wrote: > FWIW, I played with a one liner to find foo.cc that includes Create() with > corresponding foo_impl.cc > > % for i in chrome/browser/**/*_impl.cc; do grep -H '^[^ ].*::Create(' > ${i:s/_impl.cc/.cc/}; done 2> /dev/null > chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper.cc:scoped_ptr<EnterpriseEnrollmentHelper> > EnterpriseEnrollmentHelper::Create( > chrome/browser/local_discovery/gcd_api_flow.cc:scoped_ptr<GCDApiFlow> > GCDApiFlow::Create( > chrome/browser/ui/autofill/autofill_dialog_controller.cc:AutofillDialogController::Create( > > > Returning scoped_ptr<> looks neat. Can we do this here? +1 for both scoped_ptr<> returning and moving this to arc_bridge_service.cc https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... File components/arc/arc_bridge_service_impl.h (right): https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... components/arc/arc_bridge_service_impl.h:26: Unnecessary space. https://codereview.chromium.org/1475563002/diff/60001/components/arc/common/a... File components/arc/common/arc_host_messages.h (right): https://codereview.chromium.org/1475563002/diff/60001/components/arc/common/a... components/arc/common/arc_host_messages.h:39: int, /* scale_factor */ nit: spacing.
Description was changed from ========== arc-bridge: Minimal support for ARC app launcher. This CL comes along with: https://codereview.chromium.org/1464353003 (fake_arc_bridge) https://codereview.chromium.org/1413153007 (app_list) This includes set of IPC messages to interact with Android via ARC bridge service in chromeos. This also refactores ArcBridgeService in order to be able to implement FakeArcBridgeService. BUG=558209 BUG=b/24545231 TEST=Build and deploy android, chrome, chromium CLs. Apps appear in chromium app launcher. Once clicked, app is started in Android window. TEST=build Chrome with enable_arc=1 Debug/Release TEST=unit_tests with enable_arc=1 Debug/Release (Together with all required CLs) ========== to ========== aarc-bridge: Minimal support for ARC app launcher. This CL comes along with: https://codereview.chromium.org/1481523002 (split arc_bride_service) https://codereview.chromium.org/1464353003 (fake_arc_bridge) https://codereview.chromium.org/1413153007 (app_list) This includes set of IPC messages to interact with Android via ARC bridge service in chromeos. BUG=558209 BUG=b/24545231 TEST=Build and deploy android, chrome, chromium CLs. Apps appear in chromium app launcher. Once clicked, app is started in Android window. TEST=build Chrome with enable_arc=1 Debug/Release TEST=unit_tests with enable_arc=1 Debug/Release (Together with all required CLs) ==========
Thanks for reviewing. 1. Moved splitting arc_bridge_servivce.h to another CL: https://codereview.chromium.org/1481523002 2. Introduced arc::AppInfo to simplify message definitions and passing data to ArcAppListPrefs and FakeArcBridgeService (next CLs) 3. nit comments addressed. PTAL https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... File components/arc/arc_bridge_service.cc (right): https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... components/arc/arc_bridge_service.cc:61: // DCHECK on enum classes not supported. On 2015/11/25 07:47:37, hidehiko wrote: > The comment looks unrelated to the code? Done. https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... components/arc/arc_bridge_service.h:91: ArcBridgeService(); On 2015/11/25 07:47:37, hidehiko wrote: > Please move them to protected section. scoped_ptr<ArcBridgeService> requires public access to it https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... File components/arc/arc_bridge_service_impl.cc (right): https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... components/arc/arc_bridge_service_impl.cc:273: DCHECK(names.size() == packages.size() && names.size() == activities.size()); On 2015/11/25 07:47:37, hidehiko wrote: > Can we have > > std::vector<AppInfo> > > where > > struct AppInfo { > std::string name; > std::string package; > std::string activity; > }; > > ? Yes, this simplifies code, however adds arc_message_traits.h/cc https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... components/arc/arc_bridge_service_impl.cc:308: ArcBridgeService* ArcBridgeService::Create( On 2015/11/25 07:47:37, hidehiko wrote: > On 2015/11/25 07:18:56, satorux1 wrote: > > FWIW, I played with a one liner to find foo.cc that includes Create() with > > corresponding foo_impl.cc > > > > % for i in chrome/browser/**/*_impl.cc; do grep -H '^[^ ].*::Create(' > > ${i:s/_impl.cc/.cc/}; done 2> /dev/null > > > chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper.cc:scoped_ptr<EnterpriseEnrollmentHelper> > > EnterpriseEnrollmentHelper::Create( > > chrome/browser/local_discovery/gcd_api_flow.cc:scoped_ptr<GCDApiFlow> > > GCDApiFlow::Create( > > > chrome/browser/ui/autofill/autofill_dialog_controller.cc:AutofillDialogController::Create( > > > > > > Returning scoped_ptr<> looks neat. Can we do this here? > > +1 for both scoped_ptr<> returning and moving this to arc_bridge_service.cc Thank for your help! Done https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... File components/arc/arc_bridge_service_impl.h (right): https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... components/arc/arc_bridge_service_impl.h:26: On 2015/11/25 07:47:37, hidehiko wrote: > Unnecessary space. Done. https://codereview.chromium.org/1475563002/diff/60001/components/arc/common/a... File components/arc/common/arc_host_messages.h (right): https://codereview.chromium.org/1475563002/diff/60001/components/arc/common/a... components/arc/common/arc_host_messages.h:39: int, /* scale_factor */ On 2015/11/25 07:47:37, hidehiko wrote: > nit: spacing. Done. https://codereview.chromium.org/1475563002/diff/80001/components/arc/common/a... File components/arc/common/arc_message_traits.h (left): https://codereview.chromium.org/1475563002/diff/80001/components/arc/common/a... components/arc/common/arc_message_traits.h:5: #ifndef IPC_MOJO_IPC_MOJO_PARAM_TRAITS_H_ Have no idea why it detected as A+ and this base. That is newly added file.
Description was changed from ========== aarc-bridge: Minimal support for ARC app launcher. This CL comes along with: https://codereview.chromium.org/1481523002 (split arc_bride_service) https://codereview.chromium.org/1464353003 (fake_arc_bridge) https://codereview.chromium.org/1413153007 (app_list) This includes set of IPC messages to interact with Android via ARC bridge service in chromeos. BUG=558209 BUG=b/24545231 TEST=Build and deploy android, chrome, chromium CLs. Apps appear in chromium app launcher. Once clicked, app is started in Android window. TEST=build Chrome with enable_arc=1 Debug/Release TEST=unit_tests with enable_arc=1 Debug/Release (Together with all required CLs) ========== to ========== arc-bridge: Minimal support for ARC app launcher. This CL comes along with: https://codereview.chromium.org/1481523002 (split arc_bride_service) https://codereview.chromium.org/1464353003 (fake_arc_bridge) https://codereview.chromium.org/1413153007 (app_list) This includes set of IPC messages to interact with Android via ARC bridge service in chromeos. BUG=558209 BUG=b/24545231 TEST=Build and deploy android, chrome, chromium CLs. Apps appear in chromium app launcher. Once clicked, app is started in Android window. TEST=build Chrome with enable_arc=1 Debug/Release TEST=unit_tests with enable_arc=1 Debug/Release (Together with all required CLs) ==========
https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... components/arc/arc_bridge_service.h:85: const std::vector<uint8_t>& icon_png_data) {} On 2015/11/25 at 06:51:08, khmel1 wrote: > On 2015/11/25 06:34:07, dcheng wrote: > > Where does icon_png_data originate from? Is this something controlled by the app > > in question? Will we end up decoding it in the Chrome browser process somewhere? > > It's not clear from the current code, since it's hidden behind an observer > > implementation. > > This png is encoded in Android: https://googleplex-android-review.git.corp.google.com/#/c/797530/7/arcapplaun... line 194 > > > We decode it as an icon for arc app icon in App Launcher (Via ImageDecoder). > https://codereview.chromium.org/1413153007/diff/320001/chrome/browser/ui/app_... Would it be feasible to decode here and just pass around the decoded bitmap? Then all the validation could happen here. Will there ever be multiple Observer implementations?
On 2015/11/25 19:34:55, dcheng wrote: > https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... > File components/arc/arc_bridge_service.h (right): > > https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... > components/arc/arc_bridge_service.h:85: const std::vector<uint8_t>& > icon_png_data) {} > On 2015/11/25 at 06:51:08, khmel1 wrote: > > On 2015/11/25 06:34:07, dcheng wrote: > > > Where does icon_png_data originate from? Is this something controlled by the > app > > > in question? Will we end up decoding it in the Chrome browser process > somewhere? > > > It's not clear from the current code, since it's hidden behind an observer > > > implementation. > > > > This png is encoded in Android: > https://googleplex-android-review.git.corp.google.com/#/c/797530/7/arcapplaun... > line 194 > > > > > > We decode it as an icon for arc app icon in App Launcher (Via ImageDecoder). > > > https://codereview.chromium.org/1413153007/diff/320001/chrome/browser/ui/app_... > > Would it be feasible to decode here and just pass around the decoded bitmap? > Then all the validation could happen here. > > Will there ever be multiple Observer implementations? We have more complicated logic. Passed icon from Android has to be stored (we use here <profile_folder>/arcapps/<appid> folder. This is required to cache icon in order to 1) Not to pass every time icon from Android. 2) Able to display icon while ArcBridgeService is not started on Android side.
lgtm from ARC side. https://codereview.chromium.org/1475563002/diff/120001/components/arc/arc_bri... File components/arc/arc_bridge_service_impl.cc (right): https://codereview.chromium.org/1475563002/diff/120001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.cc:279: const std::string& activity, nit: Misaligned
https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... components/arc/arc_bridge_service.h:91: ArcBridgeService(); On 2015/11/25 15:28:31, khmel1 wrote: > On 2015/11/25 07:47:37, hidehiko wrote: > > Please move them to protected section. > > scoped_ptr<ArcBridgeService> requires public access to it For dtor, yes. But it should not requires ctors, IIUC. https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... File components/arc/arc_bridge_service_impl.h (right): https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... components/arc/arc_bridge_service_impl.h:26: On 2015/11/25 15:28:31, khmel1 wrote: > On 2015/11/25 07:47:37, hidehiko wrote: > > Unnecessary space. > > Done. Not done yet. https://codereview.chromium.org/1475563002/diff/120001/components/arc/common/... File components/arc/common/arc_message_traits.h (right): https://codereview.chromium.org/1475563002/diff/120001/components/arc/common/... components/arc/common/arc_message_traits.h:15: struct IPC_EXPORT ParamTraits<arc::AppInfo> { IPC_STRUCT_TRAITS_* family is your friend. https://code.google.com/p/chromium/codesearch#chromium/src/ipc/param_traits_m...
Description was changed from ========== arc-bridge: Minimal support for ARC app launcher. This CL comes along with: https://codereview.chromium.org/1481523002 (split arc_bride_service) https://codereview.chromium.org/1464353003 (fake_arc_bridge) https://codereview.chromium.org/1413153007 (app_list) This includes set of IPC messages to interact with Android via ARC bridge service in chromeos. BUG=558209 BUG=b/24545231 TEST=Build and deploy android, chrome, chromium CLs. Apps appear in chromium app launcher. Once clicked, app is started in Android window. TEST=build Chrome with enable_arc=1 Debug/Release TEST=unit_tests with enable_arc=1 Debug/Release (Together with all required CLs) ========== to ========== arc-bridge: Implement IPC message for app launcher. This CL comes along with: https://codereview.chromium.org/1481523002 (split arc_bride_service) https://codereview.chromium.org/1464353003 (fake_arc_bridge) https://codereview.chromium.org/1413153007 (app_list) This includes set of IPC messages to interact with Android via ARC bridge service in chromeos. BUG=558209 BUG=b/24545231 TEST=Build and deploy android, chrome, chromium CLs. Apps appear in chromium app launcher. Once clicked, app is started in Android window. TEST=build Chrome with enable_arc=1 Debug/Release TEST=unit_tests with enable_arc=1 Debug/Release (Together with all required CLs) ==========
Base CL was submitted and I rebased. Also use IPC_ macros for arc::AppInfo params. PTAL https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... components/arc/arc_bridge_service.h:85: const std::vector<uint8_t>& icon_png_data) {} On 2015/11/25 19:34:55, dcheng wrote: > On 2015/11/25 at 06:51:08, khmel1 wrote: > > On 2015/11/25 06:34:07, dcheng wrote: > > > Where does icon_png_data originate from? Is this something controlled by the > app > > > in question? Will we end up decoding it in the Chrome browser process > somewhere? > > > It's not clear from the current code, since it's hidden behind an observer > > > implementation. > > > > This png is encoded in Android: > https://googleplex-android-review.git.corp.google.com/#/c/797530/7/arcapplaun... > line 194 > > > > > > We decode it as an icon for arc app icon in App Launcher (Via ImageDecoder). > > > https://codereview.chromium.org/1413153007/diff/320001/chrome/browser/ui/app_... > > Would it be feasible to decode here and just pass around the decoded bitmap? > Then all the validation could happen here. > > Will there ever be multiple Observer implementations? I already responded early, just for consistency: We have more complicated logic. Passed icon from Android has to be stored (we use here <profile_folder>/arcapps/<appid> folder). This is required to cache icon in order to 1) Not to pass every time icon from Android. 2) Able to display icon while ArcBridgeService is not started on Android side. https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... components/arc/arc_bridge_service.h:91: ArcBridgeService(); On 2015/11/26 03:47:22, hidehiko wrote: > On 2015/11/25 15:28:31, khmel1 wrote: > > On 2015/11/25 07:47:37, hidehiko wrote: > > > Please move them to protected section. > > > > scoped_ptr<ArcBridgeService> requires public access to it > > For dtor, yes. But it should not requires ctors, IIUC. Done, Was addressed in previous CL https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... File components/arc/arc_bridge_service_impl.h (right): https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... components/arc/arc_bridge_service_impl.h:26: On 2015/11/26 03:47:22, hidehiko wrote: > On 2015/11/25 15:28:31, khmel1 wrote: > > On 2015/11/25 07:47:37, hidehiko wrote: > > > Unnecessary space. > > > > Done. > > Not done yet. Was done in prev CL. updated here
https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... components/arc/arc_bridge_service.h:85: const std::vector<uint8_t>& icon_png_data) {} On 2015/11/27 at 05:24:01, khmel1 wrote: > On 2015/11/25 19:34:55, dcheng wrote: > > On 2015/11/25 at 06:51:08, khmel1 wrote: > > > On 2015/11/25 06:34:07, dcheng wrote: > > > > Where does icon_png_data originate from? Is this something controlled by the > > app > > > > in question? Will we end up decoding it in the Chrome browser process > > somewhere? > > > > It's not clear from the current code, since it's hidden behind an observer > > > > implementation. > > > > > > This png is encoded in Android: > > https://googleplex-android-review.git.corp.google.com/#/c/797530/7/arcapplaun... > > line 194 > > > > > > > > > We decode it as an icon for arc app icon in App Launcher (Via ImageDecoder). > > > > > https://codereview.chromium.org/1413153007/diff/320001/chrome/browser/ui/app_... > > > > Would it be feasible to decode here and just pass around the decoded bitmap? > > Then all the validation could happen here. > > > > Will there ever be multiple Observer implementations? > > I already responded early, just for consistency: > We have more complicated logic. Passed icon from Android has to be > stored (we use here <profile_folder>/arcapps/<appid> folder). This > is required to cache icon in order to 1) Not to pass every time icon > from Android. 2) Able to display icon while ArcBridgeService is not > started on Android side. That doesn't seem like it'd require multiple observers though? I'm just trying to understand how this is structured, and how the unsanitized PNG data will end up getting passed around.
https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... components/arc/arc_bridge_service.h:85: const std::vector<uint8_t>& icon_png_data) {} On 2015/11/27 05:39:56, dcheng wrote: > On 2015/11/27 at 05:24:01, khmel1 wrote: > > On 2015/11/25 19:34:55, dcheng wrote: > > > On 2015/11/25 at 06:51:08, khmel1 wrote: > > > > On 2015/11/25 06:34:07, dcheng wrote: > > > > > Where does icon_png_data originate from? Is this something controlled by > the > > > app > > > > > in question? Will we end up decoding it in the Chrome browser process > > > somewhere? > > > > > It's not clear from the current code, since it's hidden behind an > observer > > > > > implementation. > > > > > > > > This png is encoded in Android: > > > > https://googleplex-android-review.git.corp.google.com/#/c/797530/7/arcapplaun... > > > line 194 > > > > > > > > > > > > We decode it as an icon for arc app icon in App Launcher (Via > ImageDecoder). > > > > > > > > https://codereview.chromium.org/1413153007/diff/320001/chrome/browser/ui/app_... > > > > > > Would it be feasible to decode here and just pass around the decoded bitmap? > > > Then all the validation could happen here. > > > > > > Will there ever be multiple Observer implementations? > > > > I already responded early, just for consistency: > > We have more complicated logic. Passed icon from Android has to be > > stored (we use here <profile_folder>/arcapps/<appid> folder). This > > is required to cache icon in order to 1) Not to pass every time icon > > from Android. 2) Able to display icon while ArcBridgeService is not > > started on Android side. > > That doesn't seem like it'd require multiple observers though? I'm just trying > to understand how this is structured, and how the unsanitized PNG data will end > up getting passed around. Sorry, I don't clear understand your thoughts. What do you mean for 'multiple observer'. Is any your recommendation here?
On 2015/11/27 at 05:53:16, khmel wrote: > https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... > File components/arc/arc_bridge_service.h (right): > > https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... > components/arc/arc_bridge_service.h:85: const std::vector<uint8_t>& icon_png_data) {} > On 2015/11/27 05:39:56, dcheng wrote: > > On 2015/11/27 at 05:24:01, khmel1 wrote: > > > On 2015/11/25 19:34:55, dcheng wrote: > > > > On 2015/11/25 at 06:51:08, khmel1 wrote: > > > > > On 2015/11/25 06:34:07, dcheng wrote: > > > > > > Where does icon_png_data originate from? Is this something controlled by > > the > > > > app > > > > > > in question? Will we end up decoding it in the Chrome browser process > > > > somewhere? > > > > > > It's not clear from the current code, since it's hidden behind an > > observer > > > > > > implementation. > > > > > > > > > > This png is encoded in Android: > > > > > > https://googleplex-android-review.git.corp.google.com/#/c/797530/7/arcapplaun... > > > > line 194 > > > > > > > > > > > > > > > We decode it as an icon for arc app icon in App Launcher (Via > > ImageDecoder). > > > > > > > > > > > https://codereview.chromium.org/1413153007/diff/320001/chrome/browser/ui/app_... > > > > > > > > Would it be feasible to decode here and just pass around the decoded bitmap? > > > > Then all the validation could happen here. > > > > > > > > Will there ever be multiple Observer implementations? > > > > > > I already responded early, just for consistency: > > > We have more complicated logic. Passed icon from Android has to be > > > stored (we use here <profile_folder>/arcapps/<appid> folder). This > > > is required to cache icon in order to 1) Not to pass every time icon > > > from Android. 2) Able to display icon while ArcBridgeService is not > > > started on Android side. > > > > That doesn't seem like it'd require multiple observers though? I'm just trying > > to understand how this is structured, and how the unsanitized PNG data will end > > up getting passed around. > > Sorry, I don't clear understand your thoughts. What do you mean for 'multiple observer'. Is any your recommendation here? Will there ever be multiple implementations of ArcBridgeService::Observer?
On 2015/11/27 20:36:14, dcheng wrote: > On 2015/11/27 at 05:53:16, khmel wrote: > > > https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... > > File components/arc/arc_bridge_service.h (right): > > > > > https://codereview.chromium.org/1475563002/diff/60001/components/arc/arc_brid... > > components/arc/arc_bridge_service.h:85: const std::vector<uint8_t>& > icon_png_data) {} > > On 2015/11/27 05:39:56, dcheng wrote: > > > On 2015/11/27 at 05:24:01, khmel1 wrote: > > > > On 2015/11/25 19:34:55, dcheng wrote: > > > > > On 2015/11/25 at 06:51:08, khmel1 wrote: > > > > > > On 2015/11/25 06:34:07, dcheng wrote: > > > > > > > Where does icon_png_data originate from? Is this something > controlled by > > > the > > > > > app > > > > > > > in question? Will we end up decoding it in the Chrome browser > process > > > > > somewhere? > > > > > > > It's not clear from the current code, since it's hidden behind an > > > observer > > > > > > > implementation. > > > > > > > > > > > > This png is encoded in Android: > > > > > > > > > https://googleplex-android-review.git.corp.google.com/#/c/797530/7/arcapplaun... > > > > > line 194 > > > > > > > > > > > > > > > > > > We decode it as an icon for arc app icon in App Launcher (Via > > > ImageDecoder). > > > > > > > > > > > > > > > https://codereview.chromium.org/1413153007/diff/320001/chrome/browser/ui/app_... > > > > > > > > > > Would it be feasible to decode here and just pass around the decoded > bitmap? > > > > > Then all the validation could happen here. > > > > > > > > > > Will there ever be multiple Observer implementations? > > > > > > > > I already responded early, just for consistency: > > > > We have more complicated logic. Passed icon from Android has to be > > > > stored (we use here <profile_folder>/arcapps/<appid> folder). This > > > > is required to cache icon in order to 1) Not to pass every time icon > > > > from Android. 2) Able to display icon while ArcBridgeService is not > > > > started on Android side. > > > > > > That doesn't seem like it'd require multiple observers though? I'm just > trying > > > to understand how this is structured, and how the unsanitized PNG data will > end > > > up getting passed around. > > > > Sorry, I don't clear understand your thoughts. What do you mean for 'multiple > observer'. Is any your recommendation here? > > Will there ever be multiple implementations of ArcBridgeService::Observer? Thanks for explanation, Yes, we will have additional functionality there and therefore multiple observers and passing png data for each observer might have performance implication. I split observers into 2 parts. One generic ArcBridgeService events and second is ARC apps related events.
Split observers into 2 parts. One is generic ArcBridgeService events and second is ARC apps related events. PTAL
On 2015/11/30 at 01:58:28, khmel wrote: > Split observers into 2 parts. One is generic > ArcBridgeService events and second is ARC apps related events. > > PTAL I'm a bit confused. Now with the split... are there still going to be multiple AppsObservers?
On 2015/11/30 23:11:12, dcheng wrote: > On 2015/11/30 at 01:58:28, khmel wrote: > > Split observers into 2 parts. One is generic > > ArcBridgeService events and second is ARC apps related events. > > > > PTAL > > I'm a bit confused. Now with the split... are there still going to be multiple > AppsObservers? No, there is only one AppsObserver is expected: https://codereview.chromium.org/1413153007/diff/340001/chrome/browser/ui/app_...
On 2015/12/01 00:11:18, khmel1 wrote: > On 2015/11/30 23:11:12, dcheng wrote: > > On 2015/11/30 at 01:58:28, khmel wrote: > > > Split observers into 2 parts. One is generic > > > ArcBridgeService events and second is ARC apps related events. > > > > > > PTAL > > > > I'm a bit confused. Now with the split... are there still going to be multiple > > AppsObservers? > > No, there is only one AppsObserver is expected: > https://codereview.chromium.org/1413153007/diff/340001/chrome/browser/ui/app_... Sorry, please ignore last message. ArcAppListPrefs is per profile, once we might have several profiles, that means we may have several ArcAppListPrefs and therefore several observers in ArcBridgeService.
My main request is to update the comment in arc_host_messages.h. Other replies are style-/wording- related nitpicks or optional small proposal only so other files look good with them. https://codereview.chromium.org/1475563002/diff/160001/components/arc/arc_bri... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1475563002/diff/160001/components/arc/arc_bri... components/arc/arc_bridge_service.h:84: class AppsObserver { nit: I prefer AppObserver, as this also receives individual app icon update (OnAppIcon). WDYT? https://codereview.chromium.org/1475563002/diff/160001/components/arc/arc_bri... components/arc/arc_bridge_service.h:85: public: nit: indent. https://codereview.chromium.org/1475563002/diff/160001/components/arc/arc_bri... components/arc/arc_bridge_service.h:155: virtual bool RefreshApps() = 0; nit: Then, as you've written, how about name it RefereshAppList()? IMHO, it is slightly clear that this refreshes a list of currently installed applications, but does not refresh all applications running etc. WDYT? https://codereview.chromium.org/1475563002/diff/160001/components/arc/arc_bri... File components/arc/arc_bridge_service_impl.cc (right): https://codereview.chromium.org/1475563002/diff/160001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.cc:127: nit: no empty line please, for consistency. https://codereview.chromium.org/1475563002/diff/160001/components/arc/arc_bri... File components/arc/arc_bridge_service_impl.h (right): https://codereview.chromium.org/1475563002/diff/160001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.h:48: // Request to load icon of specific scale_factor. nit: Requests, load an icon? https://codereview.chromium.org/1475563002/diff/160001/components/arc/common/... File components/arc/common/arc_host_messages.h (right): https://codereview.chromium.org/1475563002/diff/160001/components/arc/common/... components/arc/common/arc_host_messages.h:22: // Sends a list of available ARC apps to Chrome. |labels|, |packages| and Could you update the comment here, too? Now, you only have "apps" argument. https://codereview.chromium.org/1475563002/diff/160001/components/arc/common/... File components/arc/common/arc_instance_messages.h (right): https://codereview.chromium.org/1475563002/diff/160001/components/arc/common/... components/arc/common/arc_instance_messages.h:30: // Sends a request to ARC to refresh list of ARC apps. nit: a list. https://codereview.chromium.org/1475563002/diff/160001/components/arc/common/... File components/arc/common/arc_message_types.h (right): https://codereview.chromium.org/1475563002/diff/160001/components/arc/common/... components/arc/common/arc_message_types.h:42: // Describe ARC app. nit: Describes, or Describing (for consistency with the above comment)?
Thanks Hidehiko-san for taking another round of review. I renamed some methods/fields and updated nits. There is additional information to question about number of AppObserver. Now I am working on Shelf integration and can confirm that I need one more AppObserver. https://codereview.chromium.org/1475563002/diff/160001/components/arc/arc_bri... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1475563002/diff/160001/components/arc/arc_bri... components/arc/arc_bridge_service.h:84: class AppsObserver { On 2015/12/01 08:18:25, hidehiko wrote: > nit: I prefer AppObserver, as this also receives individual app icon update > (OnAppIcon). WDYT? Sounds better, thanks https://codereview.chromium.org/1475563002/diff/160001/components/arc/arc_bri... components/arc/arc_bridge_service.h:85: public: On 2015/12/01 08:18:25, hidehiko wrote: > nit: indent. Done. https://codereview.chromium.org/1475563002/diff/160001/components/arc/arc_bri... components/arc/arc_bridge_service.h:155: virtual bool RefreshApps() = 0; On 2015/12/01 08:18:25, hidehiko wrote: > nit: Then, as you've written, how about name it RefereshAppList()? > IMHO, it is slightly clear that this refreshes a list of currently installed > applications, but does not refresh all applications running etc. WDYT? Agree https://codereview.chromium.org/1475563002/diff/160001/components/arc/arc_bri... File components/arc/arc_bridge_service_impl.cc (right): https://codereview.chromium.org/1475563002/diff/160001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.cc:127: On 2015/12/01 08:18:25, hidehiko wrote: > nit: no empty line please, for consistency. Done. https://codereview.chromium.org/1475563002/diff/160001/components/arc/arc_bri... File components/arc/arc_bridge_service_impl.h (right): https://codereview.chromium.org/1475563002/diff/160001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.h:48: // Request to load icon of specific scale_factor. On 2015/12/01 08:18:25, hidehiko wrote: > nit: Requests, load an icon? Done. https://codereview.chromium.org/1475563002/diff/160001/components/arc/common/... File components/arc/common/arc_host_messages.h (right): https://codereview.chromium.org/1475563002/diff/160001/components/arc/common/... components/arc/common/arc_host_messages.h:22: // Sends a list of available ARC apps to Chrome. |labels|, |packages| and On 2015/12/01 08:18:25, hidehiko wrote: > Could you update the comment here, too? > Now, you only have "apps" argument. Thanks for noticing this. https://codereview.chromium.org/1475563002/diff/160001/components/arc/common/... File components/arc/common/arc_instance_messages.h (right): https://codereview.chromium.org/1475563002/diff/160001/components/arc/common/... components/arc/common/arc_instance_messages.h:30: // Sends a request to ARC to refresh list of ARC apps. On 2015/12/01 08:18:25, hidehiko wrote: > nit: a list. Done. https://codereview.chromium.org/1475563002/diff/160001/components/arc/common/... File components/arc/common/arc_message_types.h (right): https://codereview.chromium.org/1475563002/diff/160001/components/arc/common/... components/arc/common/arc_message_types.h:42: // Describe ARC app. On 2015/12/01 08:18:25, hidehiko wrote: > nit: Describes, or Describing (for consistency with the above comment)? Done. https://codereview.chromium.org/1475563002/diff/180001/components/arc/arc_bri... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1475563002/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_service.h:87: virtual void OnAppListRefreshed(const std::vector<AppInfo>& apps) {} Additionally renamed for consistency. https://codereview.chromium.org/1475563002/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_service.h:162: virtual bool RequestAppIcon(const std::string& package, Additionally renamed for consistency.
PTAL
lgtm
https://codereview.chromium.org/1475563002/diff/180001/components/arc/common/... File components/arc/common/arc_host_messages.h (right): https://codereview.chromium.org/1475563002/diff/180001/components/arc/common/... components/arc/common/arc_host_messages.h:37: int, /* scale_factor */ Can scale_factor be negative? https://codereview.chromium.org/1475563002/diff/180001/components/arc/common/... File components/arc/common/arc_instance_messages.h (right): https://codereview.chromium.org/1475563002/diff/180001/components/arc/common/... components/arc/common/arc_instance_messages.h:48: int /* scale factor */) Same comment about scale_factor being negative or not.
https://codereview.chromium.org/1475563002/diff/180001/components/arc/common/... File components/arc/common/arc_host_messages.h (right): https://codereview.chromium.org/1475563002/diff/180001/components/arc/common/... components/arc/common/arc_host_messages.h:37: int, /* scale_factor */ On 2015/12/01 15:17:22, Jorge Lucangeli Obes wrote: > Can scale_factor be negative? It reflects enum here: https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/layout.h&q... De facto in range 1-9 https://codereview.chromium.org/1475563002/diff/180001/components/arc/common/... File components/arc/common/arc_instance_messages.h (right): https://codereview.chromium.org/1475563002/diff/180001/components/arc/common/... components/arc/common/arc_instance_messages.h:48: int /* scale factor */) On 2015/12/01 15:17:22, Jorge Lucangeli Obes wrote: > Same comment about scale_factor being negative or not. The same as in previous comment
On 2015/12/01 15:46:29, khmel1 wrote: > https://codereview.chromium.org/1475563002/diff/180001/components/arc/common/... > File components/arc/common/arc_host_messages.h (right): > > https://codereview.chromium.org/1475563002/diff/180001/components/arc/common/... > components/arc/common/arc_host_messages.h:37: int, /* scale_factor */ > On 2015/12/01 15:17:22, Jorge Lucangeli Obes wrote: > > Can scale_factor be negative? > > It reflects enum here: > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/layout.h&q... > > De facto in range 1-9 > Can you use the enum type directly? Are you checking that the int is in range before using it? > https://codereview.chromium.org/1475563002/diff/180001/components/arc/common/... > File components/arc/common/arc_instance_messages.h (right): > > https://codereview.chromium.org/1475563002/diff/180001/components/arc/common/... > components/arc/common/arc_instance_messages.h:48: int /* scale factor */) > On 2015/12/01 15:17:22, Jorge Lucangeli Obes wrote: > > Same comment about scale_factor being negative or not. > > The same as in previous comment
On 2015/12/01 15:52:00, Jorge Lucangeli Obes wrote: > On 2015/12/01 15:46:29, khmel1 wrote: > > > https://codereview.chromium.org/1475563002/diff/180001/components/arc/common/... > > File components/arc/common/arc_host_messages.h (right): > > > > > https://codereview.chromium.org/1475563002/diff/180001/components/arc/common/... > > components/arc/common/arc_host_messages.h:37: int, /* scale_factor */ > > On 2015/12/01 15:17:22, Jorge Lucangeli Obes wrote: > > > Can scale_factor be negative? > > > > It reflects enum here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/layout.h&q... > > > > De facto in range 1-9 > > > > Can you use the enum type directly? Are you checking that the int is in range > before using it? Yes. it is checked and error is reported if out of range. I cannot use this enum directly because this code is used on Android side too and chromium layout.h is not available there. I could duplicate enum here like arc::ScaleFactor but anyway I have to cast to ui::ScaleFactor and check afterword. Also this would make code heavier and redundant. > > > https://codereview.chromium.org/1475563002/diff/180001/components/arc/common/... > > File components/arc/common/arc_instance_messages.h (right): > > > > > https://codereview.chromium.org/1475563002/diff/180001/components/arc/common/... > > components/arc/common/arc_instance_messages.h:48: int /* scale factor */) > > On 2015/12/01 15:17:22, Jorge Lucangeli Obes wrote: > > > Same comment about scale_factor being negative or not. > > > > The same as in previous comment
On 2015/12/01 15:59:55, khmel1 wrote: > On 2015/12/01 15:52:00, Jorge Lucangeli Obes wrote: > > On 2015/12/01 15:46:29, khmel1 wrote: > > > > > > https://codereview.chromium.org/1475563002/diff/180001/components/arc/common/... > > > File components/arc/common/arc_host_messages.h (right): > > > > > > > > > https://codereview.chromium.org/1475563002/diff/180001/components/arc/common/... > > > components/arc/common/arc_host_messages.h:37: int, /* scale_factor */ > > > On 2015/12/01 15:17:22, Jorge Lucangeli Obes wrote: > > > > Can scale_factor be negative? > > > > > > It reflects enum here: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/layout.h&q... > > > > > > De facto in range 1-9 > > > > > > > Can you use the enum type directly? Are you checking that the int is in range > > before using it? > > Yes. it is checked and error is reported if out of range. I cannot use this enum > directly > because this code is used on Android side too and chromium layout.h is not > available > there. I could duplicate enum here like arc::ScaleFactor but anyway I have to > cast to ui::ScaleFactor and check afterword. Also this would make code heavier > and redundant. I think this is a bad idea. You're implicitly using an enum that can be changed at any given point. Nothing guarantees that the members won't be changed -- the whole point of the struct is being able to do changes like this without worrying about breaking things. > > > > > > > > https://codereview.chromium.org/1475563002/diff/180001/components/arc/common/... > > > File components/arc/common/arc_instance_messages.h (right): > > > > > > > > > https://codereview.chromium.org/1475563002/diff/180001/components/arc/common/... > > > components/arc/common/arc_instance_messages.h:48: int /* scale factor */) > > > On 2015/12/01 15:17:22, Jorge Lucangeli Obes wrote: > > > > Same comment about scale_factor being negative or not. > > > > > > The same as in previous comment
On 2015/12/01 16:51:26, Jorge Lucangeli Obes wrote: > On 2015/12/01 15:59:55, khmel1 wrote: > > On 2015/12/01 15:52:00, Jorge Lucangeli Obes wrote: > > > On 2015/12/01 15:46:29, khmel1 wrote: > > > > > > > > > > https://codereview.chromium.org/1475563002/diff/180001/components/arc/common/... > > > > File components/arc/common/arc_host_messages.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1475563002/diff/180001/components/arc/common/... > > > > components/arc/common/arc_host_messages.h:37: int, /* scale_factor */ > > > > On 2015/12/01 15:17:22, Jorge Lucangeli Obes wrote: > > > > > Can scale_factor be negative? > > > > > > > > It reflects enum here: > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/layout.h&q... > > > > > > > > De facto in range 1-9 > > > > > > > > > > Can you use the enum type directly? Are you checking that the int is in > range > > > before using it? > > > > Yes. it is checked and error is reported if out of range. I cannot use this > enum > > directly > > because this code is used on Android side too and chromium layout.h is not > > available > > there. I could duplicate enum here like arc::ScaleFactor but anyway I have to > > cast to ui::ScaleFactor and check afterword. Also this would make code heavier > > and redundant. > > I think this is a bad idea. You're implicitly using an enum that can be changed > at any given point. Nothing guarantees that the members won't be changed -- the > whole point of the struct is being able to do changes like this without worrying > about breaking things. > Could you please give a recommendation in this case? I am not sure what I can do now. > > > > > > > > > > > > > > https://codereview.chromium.org/1475563002/diff/180001/components/arc/common/... > > > > File components/arc/common/arc_instance_messages.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1475563002/diff/180001/components/arc/common/... > > > > components/arc/common/arc_instance_messages.h:48: int /* scale factor */) > > > > On 2015/12/01 15:17:22, Jorge Lucangeli Obes wrote: > > > > > Same comment about scale_factor being negative or not. > > > > > > > > The same as in previous comment
Added arc::ScaleFactor enum PTAL https://codereview.chromium.org/1475563002/diff/200001/components/arc/common/... File components/arc/common/arc_message_types.h (right): https://codereview.chromium.org/1475563002/diff/200001/components/arc/common/... components/arc/common/arc_message_types.h:43: enum ScaleFactor : int { I also will add a unit test at chrome/browser/ui/app_list/arc/arc_app_unittest.cc 'ScaleFactorUpToData' that validates its correctness. I am not doing it here to avoid additional dependencies from components/arc to ui/
lgtm from an ipc perspective For the future, it would make things easier if https://codereview.chromium.org/1413153007 were also in this patchset: after all, it's the actual code that's using the IPC data. https://codereview.chromium.org/1475563002/diff/200001/components/arc/arc_bri... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1475563002/diff/200001/components/arc/arc_bri... components/arc/arc_bridge_service.h:136: void RemoveAppObserver(AppObserver* observer); Combined with https://codereview.chromium.org/1475583002, there's a lot of types of observers now...
lgtm Hi Daniel, Thank you for your review. Yes, multiple CLs have some difficulties, however I when I had one CL I got request to split it. So there is a just trade off here. Thanks to everyone for your comments and ideas! Will try to land this now. https://codereview.chromium.org/1475563002/diff/200001/components/arc/arc_bri... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1475563002/diff/200001/components/arc/arc_bri... components/arc/arc_bridge_service.h:136: void RemoveAppObserver(AppObserver* observer); On 2015/12/02 18:44:29, dcheng wrote: > Combined with https://codereview.chromium.org/1475583002, there's a lot of types > of observers now... Yes, this bridge is quite complicate by nature. I think that having multiple observer for each functionality helps to relieve this situation a bit.
lgtm Hi Daniel, Thank you for your review. Yes, multiple CLs have some difficulties, however I when I had one CL I got request to split it. So there is a just trade off here. Thanks to everyone for your comments and ideas! Will try to land this now.
The CQ bit was checked by khmel@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from elijahtaylor@chromium.org, satorux@chromium.org, lhchavez@chromium.org, hidehiko@chromium.org Link to the patchset: https://codereview.chromium.org/1475563002/#ps200001 (title: "Add arc::ScaleFactor enum")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1475563002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1475563002/200001
lgtm
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by khmel@google.com
lgtm AddExtensionWithMenuOpen is flaky.
lgtm AddExtensionWithMenuOpen is flaky.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1475563002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1475563002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by khmel@google.com
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1475563002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1475563002/200001
yoshiki@chromium.org changed reviewers: + yoshiki@chromium.org
Sorry for jumping in, and let me add some nit comments. https://codereview.chromium.org/1475563002/diff/200001/components/arc/common/... File components/arc/common/arc_message_traits.h (right): https://codereview.chromium.org/1475563002/diff/200001/components/arc/common/... components/arc/common/arc_message_traits.h:10: arc::ScaleFactor::SCALE_FACTOR_100P, SCALE_FACTOR_NONE is never passed, right? SCALE_FACTOR_NONE is smaller than SCALE_FACTOR_100P. https://codereview.chromium.org/1475563002/diff/200001/components/arc/common/... components/arc/common/arc_message_traits.h:11: arc::ScaleFactor::NUM_SCALE_FACTORS); This is the max value, so you may specify "NUM_SCALE_FACTORS - 1"? https://codereview.chromium.org/1475563002/diff/200001/components/arc/common/... File components/arc/common/arc_message_types.h (right): https://codereview.chromium.org/1475563002/diff/200001/components/arc/common/... components/arc/common/arc_message_types.h:43: enum ScaleFactor : int { It might be better to use "enum class" instead.
Message was sent while issue was closed.
Description was changed from ========== arc-bridge: Implement IPC message for app launcher. This CL comes along with: https://codereview.chromium.org/1481523002 (split arc_bride_service) https://codereview.chromium.org/1464353003 (fake_arc_bridge) https://codereview.chromium.org/1413153007 (app_list) This includes set of IPC messages to interact with Android via ARC bridge service in chromeos. BUG=558209 BUG=b/24545231 TEST=Build and deploy android, chrome, chromium CLs. Apps appear in chromium app launcher. Once clicked, app is started in Android window. TEST=build Chrome with enable_arc=1 Debug/Release TEST=unit_tests with enable_arc=1 Debug/Release (Together with all required CLs) ========== to ========== arc-bridge: Implement IPC message for app launcher. This CL comes along with: https://codereview.chromium.org/1481523002 (split arc_bride_service) https://codereview.chromium.org/1464353003 (fake_arc_bridge) https://codereview.chromium.org/1413153007 (app_list) This includes set of IPC messages to interact with Android via ARC bridge service in chromeos. BUG=558209 BUG=b/24545231 TEST=Build and deploy android, chrome, chromium CLs. Apps appear in chromium app launcher. Once clicked, app is started in Android window. TEST=build Chrome with enable_arc=1 Debug/Release TEST=unit_tests with enable_arc=1 Debug/Release (Together with all required CLs) ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== arc-bridge: Implement IPC message for app launcher. This CL comes along with: https://codereview.chromium.org/1481523002 (split arc_bride_service) https://codereview.chromium.org/1464353003 (fake_arc_bridge) https://codereview.chromium.org/1413153007 (app_list) This includes set of IPC messages to interact with Android via ARC bridge service in chromeos. BUG=558209 BUG=b/24545231 TEST=Build and deploy android, chrome, chromium CLs. Apps appear in chromium app launcher. Once clicked, app is started in Android window. TEST=build Chrome with enable_arc=1 Debug/Release TEST=unit_tests with enable_arc=1 Debug/Release (Together with all required CLs) ========== to ========== arc-bridge: Implement IPC message for app launcher. This CL comes along with: https://codereview.chromium.org/1481523002 (split arc_bride_service) https://codereview.chromium.org/1464353003 (fake_arc_bridge) https://codereview.chromium.org/1413153007 (app_list) This includes set of IPC messages to interact with Android via ARC bridge service in chromeos. BUG=558209 BUG=b/24545231 TEST=Build and deploy android, chrome, chromium CLs. Apps appear in chromium app launcher. Once clicked, app is started in Android window. TEST=build Chrome with enable_arc=1 Debug/Release TEST=unit_tests with enable_arc=1 Debug/Release (Together with all required CLs) Committed: https://crrev.com/6644c3726855d20038fc1a30d37afdc072f83632 Cr-Commit-Position: refs/heads/master@{#362895} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/6644c3726855d20038fc1a30d37afdc072f83632 Cr-Commit-Position: refs/heads/master@{#362895}
Message was sent while issue was closed.
Hi Yoshiki-san, You are always welcome, please feel free to join any CL (at least my:)). https://codereview.chromium.org/1475563002/diff/200001/components/arc/common/... File components/arc/common/arc_message_traits.h (right): https://codereview.chromium.org/1475563002/diff/200001/components/arc/common/... components/arc/common/arc_message_traits.h:10: arc::ScaleFactor::SCALE_FACTOR_100P, On 2015/12/03 02:24:32, yoshiki wrote: > SCALE_FACTOR_NONE is never passed, right? SCALE_FACTOR_NONE is smaller than > SCALE_FACTOR_100P. Right, SCALE_FACTOR_NONE is invalid value for this IPC and it is yes, smaller than 100P. https://codereview.chromium.org/1475563002/diff/200001/components/arc/common/... components/arc/common/arc_message_traits.h:11: arc::ScaleFactor::NUM_SCALE_FACTORS); On 2015/12/03 02:24:32, yoshiki wrote: > This is the max value, so you may specify "NUM_SCALE_FACTORS - 1"? Yes, you are right, they are both inclusive and I had wrong understanding. Will fix in the next CL or at separate CL. 565065 is reported for this. Thanks for attention! https://codereview.chromium.org/1475563002/diff/200001/components/arc/common/... File components/arc/common/arc_message_types.h (right): https://codereview.chromium.org/1475563002/diff/200001/components/arc/common/... components/arc/common/arc_message_types.h:43: enum ScaleFactor : int { On 2015/12/03 02:24:32, yoshiki wrote: > It might be better to use "enum class" instead. I would keep this one-to-one as they are originally declared for consistency (if no rule to force to do differently). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
