Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1413153007:
arc-app-launcher: Minimal support for ARC app launcher. (Closed)
|
Created:
5 years, 1 month ago by khmel Modified:
5 years ago Reviewers:
elijahtaylor1, khmel1, jochen (gone - plz use gerrit), Matt Giuca, xiyuan, Luis Héctor Chávez, elijahtaylor (use chromium), lpique, hidehiko, battre CC:
bengold Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc-app-launcher: Minimal support for ARC app launcher.
This provides extension to app_list model that manages arc
apps. It includes preference based service that interacts
with arc bridge and supports an arc app life cycle; app list
model builder for arc apps; arc list item. legacy
ExtensionAppModelBuilder and new ArcAppModelBuilder are
refactored in order to inherit common code.
BUG=558209
TEST=Build and deploy android, chrome, chromium CLs. Apps
appear in chromium app launcher. Once clicked, app
is started in Android window.
TEST=unit_tests with enable_arc=1 Debug/Release
Committed: https://crrev.com/d7c5a3215244c5aa2165b6284bc8d19d5792af8c
Cr-Commit-Position: refs/heads/master@{#363220}
Patch Set 1 #
Total comments: 19
Patch Set 2 : arc-app-launcher: Minimal support for ARC app launcher. #
Total comments: 3
Patch Set 3 : switch to new IPC #
Total comments: 118
Patch Set 4 : Process xiyuan #
Total comments: 3
Patch Set 5 : Discard accidental change in chrome/browser/prefs/browser_prefs.cc #
Total comments: 30
Patch Set 6 : Updated comments, calculate pos for items, rename arc_app_prefs, passing activities attrs from IPC … #Patch Set 7 : revert wrongly update chrome/chrome_browser_ui.gypi #
Total comments: 8
Patch Set 8 : Rebase, add unit_tests #Patch Set 9 : Fix WeakPtr issue. Change PostTask on PostTaskAndReplyWithResult #
Total comments: 22
Patch Set 10 : nit comments addressed #
Total comments: 26
Patch Set 11 : nit comments addressed. #Patch Set 12 : nit comment addressed #
Total comments: 19
Patch Set 13 : nit: fix typo in header, rename test lib to test_support #
Total comments: 10
Patch Set 14 : Nits, moved arc_app to arc subfolder, refactored AppModelBuilders #Patch Set 15 : Rebased #Patch Set 16 : Split off #
Total comments: 1
Patch Set 17 : Reflect changes in FakeArcBridgeService #Patch Set 18 : Rebased (change to use switch for arc), Reflected changes in base CLs #
Total comments: 2
Patch Set 19 : Rebased #
Total comments: 30
Patch Set 20 : Merged FakeArcBridgeService, comments addressed. #
Total comments: 29
Patch Set 21 : Removed inits from headers, nits addressed #Patch Set 22 : Restored non static class memeber initialization #Patch Set 23 : add missing method to fake_arc_bridge_service #Patch Set 24 : Added missing links to BUILD.gn #Messages
Total messages: 90 (27 generated)
Description was changed from ========== arc-app-launcher: Minimal support for ARC app launcher. This CL comes along with: https://googleplex-android-review.git.corp.google.com/#/c/797530 https://googleplex-android-review.git.corp.google.com/795572 https://chrome-internal-review.googlesource.com/#/c/235195 This includes set of IPC messages to interact with android via ARC bridge service in chromeos. This provides extension to app_list model that manages arc apps. It includes preference based service that interacts with arc bridge and supports an arc app life cycle; app list model builder for arc apps; arc list item. 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. ========== to ========== arc-app-launcher: Minimal support for ARC app launcher. This CL comes along with: https://googleplex-android-review.git.corp.google.com/#/c/797530 https://googleplex-android-review.git.corp.google.com/795572 https://chrome-internal-review.googlesource.com/#/c/235195 This includes set of IPC messages to interact with android via ARC bridge service in chromeos. This provides extension to app_list model that manages arc apps. It includes preference based service that interacts with arc bridge and supports an arc app life cycle; app list model builder for arc apps; arc list item. 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. ==========
khmel@google.com changed reviewers: + elijahtaylor@chromium.org, lhchavez@chromium.org
On 2015/10/27 11:23:32, khmel1 wrote: > mailto:khmel@google.com changed reviewers: > + mailto:elijahtaylor@chromium.org, mailto:lhchavez@chromium.org Arc bridge service is not committed yet so this one is based on DBUS. PTAL
khmel@google.com changed reviewers: + khmel@google.com
This is alternative app launcher implementation based on app_list model. Note, that final ArcBridgeService is not committed yet, so this CL is based on DBus impl.
On 2015/10/28 00:17:54, khmel1 wrote: > This is alternative app launcher implementation based on app_list model. Note, > that final ArcBridgeService is not committed yet, so this CL is based on DBus > impl. great, I'll take a look at this tonight (~4 hours)
Overall I'm comfortable with the direction of this CL. We should probably get someone familiar with the app list (jennyz@ has helped with other ARC things) and prefs (I don't recognize any of these owners, so you'll want to provide context) to take a look. A couple of high-level comments: 1) This support should only be active on Chrome OS. I'm not sure if that means all of this code should only be built on Chrome OS, but specifically the code interacting with the ARC Bridge Service should only be compiled on Chrome OS, and probably the Pref registration too. 2) If these preferences are synced via Chrome sync, it would be great to have a path to install ARC apps when an app is synced from prefs but isn't installed in Android. I think it's fine not to have for now, but please leave a TODO and probably file a bug to track this behavior. https://codereview.chromium.org/1413153007/diff/1/chrome/browser/prefs/browse... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/1413153007/diff/1/chrome/browser/prefs/browse... chrome/browser/prefs/browser_prefs.cc:523: ArcAppPrefs::RegisterProfilePrefs(registry); this should probably be in the OS_CHROMEOS section https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/app_list_syncable_service.cc (right): https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/app_list_syncable_service.cc:138: *type = sync_pb::AppListSpecifics::TYPE_APP; nit: alignment https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc_app_item.cc (right): https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc_app_item.cc:43: set_position(syncer::StringOrdinal("nq")); can you comment how this interacts with other apps ordering? https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc_app_item.cc:136: gfx::PNGCodec::Decode(data, file_contents.length(), &bitmap); I don't know if this is considered safe to do in the browser process. There is a utility process extensions use for decoding PNGs to isolate from the browser https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc_app_model_builder.cc (right): https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc_app_model_builder.cc:103: return; should this be a CHECK? (same Q for OnAppIconUpdated) https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc_app_prefs.cc (right): https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc_app_prefs.cc:53: switch (scale_factor) { it's not clear what all of these icon sizes are for. Is there a set of scale factors that are required in order to have a proper implementation? Are there incompatibilities between Chrome OS and Android? https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc_app_prefs.cc:104: return crx_file::id_util::GenerateId(input); does this need to be a CRX style ID? It would be more readable to just keep it as the package + activity IMO if that's possible https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc_app_prefs.cc:111: prefs_(prefs) { alignment https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc_app_prefs.h (right): https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc_app_prefs.h:41: bool enabled; maybe call this "ready" here and elsewhere to match the bridge method name? Enabled/Disabled already has a meaning in Apps and Extensions so it would be best to avoid it for this https://codereview.chromium.org/1413153007/diff/1/ui/app_list/resources/app_l... File ui/app_list/resources/app_list_resources.grd (right): https://codereview.chromium.org/1413153007/diff/1/ui/app_list/resources/app_l... ui/app_list/resources/app_list_resources.grd:18: <structure type="chrome_scaled_image" name="IDR_APP_LIST_ARC_APP_DEFAULT" file="common/app_list_arc_app_default.png" /> I can't comment on the binary files, but are we clear to add these to the chrome tree? are they license free and is it ok branding-wise? Please check with bengold (CC'd) on branding
I would refrain from putting new assets in. We should just use the normal placeholder png that's already in chrome, without the Android logo on it. On 2015/10/28 06:32:39, elijahtaylor1 wrote: > Overall I'm comfortable with the direction of this CL. We should probably get > someone familiar with the app list (jennyz@ has helped with other ARC things) > and prefs (I don't recognize any of these owners, so you'll want to provide > context) to take a look. > > A couple of high-level comments: > > 1) This support should only be active on Chrome OS. I'm not sure if that means > all of this code should only be built on Chrome OS, but specifically the code > interacting with the ARC Bridge Service should only be compiled on Chrome OS, > and probably the Pref registration too. > > 2) If these preferences are synced via Chrome sync, it would be great to have a > path to install ARC apps when an app is synced from prefs but isn't installed in > Android. I think it's fine not to have for now, but please leave a TODO and > probably file a bug to track this behavior. > > https://codereview.chromium.org/1413153007/diff/1/chrome/browser/prefs/browse... > File chrome/browser/prefs/browser_prefs.cc (right): > > https://codereview.chromium.org/1413153007/diff/1/chrome/browser/prefs/browse... > chrome/browser/prefs/browser_prefs.cc:523: > ArcAppPrefs::RegisterProfilePrefs(registry); > this should probably be in the OS_CHROMEOS section > > https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... > File chrome/browser/ui/app_list/app_list_syncable_service.cc (right): > > https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... > chrome/browser/ui/app_list/app_list_syncable_service.cc:138: *type = > sync_pb::AppListSpecifics::TYPE_APP; > nit: alignment > > https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... > File chrome/browser/ui/app_list/arc_app_item.cc (right): > > https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... > chrome/browser/ui/app_list/arc_app_item.cc:43: > set_position(syncer::StringOrdinal("nq")); > can you comment how this interacts with other apps ordering? > > https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... > chrome/browser/ui/app_list/arc_app_item.cc:136: gfx::PNGCodec::Decode(data, > file_contents.length(), &bitmap); > I don't know if this is considered safe to do in the browser process. There is > a utility process extensions use for decoding PNGs to isolate from the browser > > https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... > File chrome/browser/ui/app_list/arc_app_model_builder.cc (right): > > https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... > chrome/browser/ui/app_list/arc_app_model_builder.cc:103: return; > should this be a CHECK? (same Q for OnAppIconUpdated) > > https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... > File chrome/browser/ui/app_list/arc_app_prefs.cc (right): > > https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... > chrome/browser/ui/app_list/arc_app_prefs.cc:53: switch (scale_factor) { > it's not clear what all of these icon sizes are for. Is there a set of scale > factors that are required in order to have a proper implementation? Are there > incompatibilities between Chrome OS and Android? > > https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... > chrome/browser/ui/app_list/arc_app_prefs.cc:104: return > crx_file::id_util::GenerateId(input); > does this need to be a CRX style ID? It would be more readable to just keep it > as the package + activity IMO if that's possible > > https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... > chrome/browser/ui/app_list/arc_app_prefs.cc:111: prefs_(prefs) { > alignment > > https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... > File chrome/browser/ui/app_list/arc_app_prefs.h (right): > > https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... > chrome/browser/ui/app_list/arc_app_prefs.h:41: bool enabled; > maybe call this "ready" here and elsewhere to match the bridge method name? > Enabled/Disabled already has a meaning in Apps and Extensions so it would be > best to avoid it for this > > https://codereview.chromium.org/1413153007/diff/1/ui/app_list/resources/app_l... > File ui/app_list/resources/app_list_resources.grd (right): > > https://codereview.chromium.org/1413153007/diff/1/ui/app_list/resources/app_l... > ui/app_list/resources/app_list_resources.grd:18: <structure > type="chrome_scaled_image" name="IDR_APP_LIST_ARC_APP_DEFAULT" > file="common/app_list_arc_app_default.png" /> > I can't comment on the binary files, but are we clear to add these to the chrome > tree? are they license free and is it ok branding-wise? Please check with > bengold (CC'd) on branding
Moved ui code under OS_CHROMEOS Processed comments. https://codereview.chromium.org/1413153007/diff/1/chrome/browser/prefs/browse... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/1413153007/diff/1/chrome/browser/prefs/browse... chrome/browser/prefs/browser_prefs.cc:523: ArcAppPrefs::RegisterProfilePrefs(registry); On 2015/10/28 06:32:38, elijahtaylor1 wrote: > this should probably be in the OS_CHROMEOS section Done. https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/app_list_syncable_service.cc (right): https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/app_list_syncable_service.cc:138: *type = sync_pb::AppListSpecifics::TYPE_APP; On 2015/10/28 06:32:38, elijahtaylor1 wrote: > nit: alignment Done. https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc_app_item.cc (right): https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc_app_item.cc:136: gfx::PNGCodec::Decode(data, file_contents.length(), &bitmap); On 2015/10/28 06:32:38, elijahtaylor1 wrote: > I don't know if this is considered safe to do in the browser process. There is > a utility process extensions use for decoding PNGs to isolate from the browser I changed similar to extensions::ImageLoader by using PNGCodec::decode from browser blocking thread. This png is generated by Bitmal.compressor internally by our android side code. Also I tested fail case by passing broken content here. Icon is not loaded but nothing crashed. https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc_app_model_builder.cc (right): https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc_app_model_builder.cc:103: return; On 2015/10/28 06:32:38, elijahtaylor1 wrote: > should this be a CHECK? (same Q for OnAppIconUpdated) I took a look into ExtensionAppModelBuilder. Similar cases (many ::On...) are handled without any error logging. I think log warning here is good enough. https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc_app_prefs.cc (right): https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc_app_prefs.cc:53: switch (scale_factor) { On 2015/10/28 06:32:39, elijahtaylor1 wrote: > it's not clear what all of these icon sizes are for. Is there a set of scale > factors that are required in order to have a proper implementation? Are there > incompatibilities between Chrome OS and Android? Required icon is determined by scale factor of Display. For minnie we have it 1.0. You also may refer to ui::GetScaleFactorForNativeView for more details. Therefore, we may need to handle different resolutions. app list_view requires 48 dip icon. In case 100p this correlate with 48 × 48 android mdpi launcher icon. For 200p it correlates with xhdpi android icon. Please note, that there is initial implementation and for simple case I am sending here only one 48x48 icon (100p). I already created a task to handle different icon resolution. Ideally each icon should be installed on demand when resource is required by chrome. I am going to do it as separate CL in order not to have over-complicated CL (please consider complexity required for ArcBridge service side also). https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc_app_prefs.cc:104: return crx_file::id_util::GenerateId(input); On 2015/10/28 06:32:39, elijahtaylor1 wrote: > does this need to be a CRX style ID? It would be more readable to just keep it > as the package + activity IMO if that's possible My first impl was the same as you suggested. However, it contains '.' which is not compatible with preferences. Safest way is to use 16 bytes hash, which is calculated in crx_file::id_util::GenerateId. I agree that it looks a bit odd, but de-factor it is already used at many places. https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc_app_prefs.cc:111: prefs_(prefs) { On 2015/10/28 06:32:39, elijahtaylor1 wrote: > alignment Done. https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/arc_app_prefs.h (right): https://codereview.chromium.org/1413153007/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/arc_app_prefs.h:41: bool enabled; On 2015/10/28 06:32:39, elijahtaylor1 wrote: > maybe call this "ready" here and elsewhere to match the bridge method name? > Enabled/Disabled already has a meaning in Apps and Extensions so it would be > best to avoid it for this Good point. Done https://codereview.chromium.org/1413153007/diff/1/ui/app_list/resources/app_l... File ui/app_list/resources/app_list_resources.grd (right): https://codereview.chromium.org/1413153007/diff/1/ui/app_list/resources/app_l... ui/app_list/resources/app_list_resources.grd:18: <structure type="chrome_scaled_image" name="IDR_APP_LIST_ARC_APP_DEFAULT" file="common/app_list_arc_app_default.png" /> On 2015/10/28 06:32:39, elijahtaylor1 wrote: > I can't comment on the binary files, but are we clear to add these to the chrome > tree? are they license free and is it ok branding-wise? Please check with > bengold (CC'd) on branding Ok, Removed as bengold requested.
looks good except the png decoding I commented on again, but I will hold off with approving until the arc bridge service change lands and this has been converted. I would recommend in the meantime to get OWNERS looped in to review their respective code https://codereview.chromium.org/1413153007/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_item.cc (right): https://codereview.chromium.org/1413153007/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_item.cc:43: set_position(syncer::StringOrdinal("nq")); I had a comment on the earlier patch, can you comment here how this interacts with the ordering of other apps? https://codereview.chromium.org/1413153007/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_item.cc:134: gfx::PNGCodec::Decode(data, file_contents.length(), &bitmap); I'm still skeptical that this will pass security as is. I don't think the source of the png being internal android helps this completely, as bugs in that implementation and bugs in the png decoder could still create a problem in the browser process. The only reason extensions::ImageLoader allows this on the browser process is because the files were created by the browser and stored on disk.
khmel@google.com changed reviewers: + calamity@chromium.org, jochen@chromium.org, xiyuan@chromium.org
This update contains: 1. Migration to new IPC model. Please note, that there is pending CL https://codereview.chromium.org/1412863004/ has to be submitted first (this affects components/arc/arc_bridge_service.*). 2. DBus changes are discrded because they are depricated. 3. Introduced new icon workflow. Now icons are installed and loaded on demand for required scale factor only. PTAL https://codereview.chromium.org/1413153007/diff/40001/components/arc/arc_brid... File components/arc/arc_bridge_service.cc (right): https://codereview.chromium.org/1413153007/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_service.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. intial file is at 1412863004 https://codereview.chromium.org/1413153007/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_service.cc:149: bool ArcBridgeService::RefreshApps() { my change starts https://codereview.chromium.org/1413153007/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_service.cc:181: } my change ends https://codereview.chromium.org/1413153007/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_service.cc:276: void ArcBridgeService::OnAppsRefreshed( my change starts https://codereview.chromium.org/1413153007/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_service.cc:292: } my change ends https://codereview.chromium.org/1413153007/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_service.cc:307: IPC_MESSAGE_HANDLER(ArcInstanceHostMsg_AppsRefreshed, OnAppsRefreshed) my change is 2 lines https://codereview.chromium.org/1413153007/diff/40001/components/arc/arc_brid... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1413153007/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_service.h:75: // Called whenever ARC sends information about available apps. my change starts https://codereview.chromium.org/1413153007/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_service.h:83: const std::vector<uint8_t>& icon_png_data) {} my change ends https://codereview.chromium.org/1413153007/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_service.h:132: // Requests to refresh an app list. my change starts https://codereview.chromium.org/1413153007/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_service.h:142: my change ends https://codereview.chromium.org/1413153007/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_service.h:177: // Called whenever ARC sends information about available apps. my change starts https://codereview.chromium.org/1413153007/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_service.h:185: const std::vector<uint8_t>& icon_png_data); my change ends
https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_icon.cc (right): https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.cc:25: Source(ArcAppIcon* host, const gfx::Size& size_in_dip); |size_in_dip| seems not used? https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.cc:36: ArcAppIcon* host_; Do we want to use a WeakPtr<ArcAppIcon> here? This way, we don't need to manually call ResetHost. See https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p... https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.cc:49: host_ = NULL; nit: NULL -> nullptr https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.cc:93: host_ = NULL; nit: NULL -> nullptr https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.cc:120: host_->observer_->OnIconUpdated(); Would this be a use-after-free? |this| should be deleted after DiscardDecodeRequest above and its |host_| member should not be accessed thereafter. And think this should be part of host_'s Update() instead. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.cc:144: source_(NULL), nit: think in-class member initialization is preferred than this now. i.e. do this in the header file Source* source_ = nullptr; https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.cc:150: Update(ResourceBundle::GetSharedInstance().GetImageSkiaNamed( This Update() call has some side effects. ImageSkiaSource::GetImageForScale is only invoked when its hosting ImageSkia does not have ImageRep for a scale factor. It looks like we are relying on this behavior to trigger the ARC icon load. However, this Update() will fill in default icon for certain scale factors and we will only see the default icon for those scale factors as a result. To show the default icon before real icon, think we should return default icon in Source::GetImageForScale instead. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.cc:158: ImageDecoder::Cancel(decode_requests_[i]); nit: Cancel() is not necessary here since ImageDecoder::ImageRequest's dtor would call it anyway. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.cc:175: base::Unretained(this), Should this be weak_ptr_factory_.GetWeakPtr() instead of base::Unretained? https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.cc:178: content::BrowserThread::PostTask(content::BrowserThread::FILE, File thread is deprecated (because it is only one thread and easily got choked) and we should use blocking pool in new code. BrowserThread::GetBlockingPool()->PostTask(...) https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.cc:190: prefs->RequestIcon(app_id_, scale_factor); How would we find out after ArcAppPrefs gets the icon? https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.cc:200: base::Unretained(this), weak_ptr_factory_.GetWeakPtr()? https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.cc:217: base::Unretained(this), weak_ptr_factory_.GetWeakPtr()? https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.cc:231: scale_factor)); nit: align with previous line. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_icon.h (right): https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.h:82: base::WeakPtrFactory<ArcAppIcon> weak_ptr_factory_; [chromium-style] WeakPtrFactory members which refer to their outer class must be the last member in the outer class definition. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.h:93: void DiscardDecodeRequest(DecodeRequest* request); Methods should precede data members. See https://google.github.io/styleguide/cppguide.html#Declaration_Order https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_item.cc (right): https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_item.cc:15: const int kIconDimension = 48; Should we use app_list::kGridIconDimension instead? https://code.google.com/p/chromium/codesearch#chromium/src/ui/app_list/app_li... https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_item.cc:48: set_position(syncer::StringOrdinal("nq")); Can we leave position unset instead of using a hard-coded one? Default behavior should be add the new item to the last if the item does not have sync data. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_item.cc:64: DCHECK(app_info != nullptr); nit: DCHECK_NE(app_info, nullptr) https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_item.cc:103: void ArcAppItem::OnIconUpdated() { ArcAppItem should observe its |icon_|. Otherwise, this won't get called. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_item.h (right): https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_item.h:21: private ArcAppIcon::Observer { All inheritance should be public. Details here: https://google.github.io/styleguide/cppguide.html#Inheritance https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_item.h:30: ~ArcAppItem() override; nit: alignment https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_item.h:37: ArcAppIcon* icon() { return icon_.get(); } Can we give it a different name (e.g arc_app_icon()) because base class app_list::AppListItem has a icon() const accessor? https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_item.h:40: private: nit: insert an empty line before https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_item.h:43: scoped_ptr<ArcAppIcon> icon_; Move data members after the methods. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_model_builder.cc (right): https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_model_builder.cc:13: : profile_(NULL), nit: Use in-class member initialization and use nullptr in place of NULL. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_model_builder.cc:21: prefs_->RemoveObserver(this); Think we need to make AppListSyncableService depends on ArcAppPrefs to ensure |prefs_| is still valid at this point. Update AppListSyncableServiceFactory ctor with that dependency, somewhere around: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_model_builder.cc:52: std::vector<std::string> app_ids = prefs_->GetAppIds(); nit: #include <vector> https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_model_builder.h (right): https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_model_builder.h:49: app_list::AppListSyncableService* service_; Move data members down. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_prefs.cc (right): https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:62: kArcApps, kArcApps should be in chrome/common/pref_names.h https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:63: user_prefs::PrefRegistrySyncable::SYNCABLE_PREF); ArcAppPrefs looks like a local cache to store app info before ARC is up and running. Do we need to make it syncable? https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:75: return crx_file::id_util::GenerateId(input); nit: App list does not assume the format of the id as long as it is unique in the model. So we don't need to generate something similar to chrome app id here. I am slightly inclined to have something with a special prefix, e.g. arcapp-<package>#<activity> Just in case we need to figure out item type from the id in the future. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:81: : browser_context_(browser_context), |browser_context_| seems not used. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:92: if (bridge_service->state() == arc::ArcBridgeService::State::READY) { nit: How about doing "OnStateChanged(bridge_service->state());"? https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:100: LOG(ERROR) << "Bridge service is not available. Cannot release."; Do we know how ArcBridgeService starts and shuts down? Profile and its KeyedService is in pretty late stage. We might be always hitting this. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:220: for (it = ready_apps_.begin(); it != ready_apps_.end(); ++it) { nit: for (auto& app_id : ready_apps_) https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:282: std::set<std::string> old_ready_apps_; nit: old_ready_apps_ -> old_ready_apps https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:291: for (it = old_ready_apps_.begin(); it != old_ready_apps_.end(); ++it) { nit: for (auto& app_id : old_ready_apps) https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:323: base::Unretained(this), base::Unretained is dangerous. Add a WeakPtrFactory and use WeakPtr instead. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:327: content::BrowserThread::PostTask(content::BrowserThread::FILE, Use BrowserThrad::GetBlockingPool instead of file thread. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:346: DCHECK(wrote == static_cast<int>(content_png.size())); nit: DCHECK_EQ(wrote, static_cast<int>(content_png.size())); https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:349: base::Unretained(this), ditto https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_prefs.h (right): https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.h:29: class ArcAppPrefs : public KeyedService, nit: document the class? https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.h:30: private arc::ArcBridgeService::Observer { no private inheritance. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.h:45: virtual void OnAppReady(const std::string& id, bool ready) = 0; Can an app change from ready to not ready? If so, maybe rename this to OnAppReadyChanged. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.h:54: // Convenience function to get the ArcAppPrefs for a BrowserContext. nit: insert an empty line above to separate https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.h:56: // Constructs unique id based on package and activity information. This id nit: insert an empty line before https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.h:103: std::map<std::string, uint32> request_icon_deferred_; Move data members down. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.h:103: std::map<std::string, uint32> request_icon_deferred_; nit: #include <map>
calamity@chromium.org changed reviewers: + mgiuca@chromium.org - calamity@chromium.org
Hi Xiyuan, Thank you for your review and detailed comments. This helped me to understand some chromium concepts more clear. PTAL. https://codereview.chromium.org/1413153007/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_item.cc (right): https://codereview.chromium.org/1413153007/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_item.cc:134: gfx::PNGCodec::Decode(data, file_contents.length(), &bitmap); On 2015/11/03 22:38:19, elijahtaylor1 wrote: > I'm still skeptical that this will pass security as is. I don't think the > source of the png being internal android helps this completely, as bugs in that > implementation and bugs in the png decoder could still create a problem in the > browser process. > > The only reason extensions::ImageLoader allows this on the browser process is > because the files were created by the browser and stored on disk. Agree, I changed icon workflow and implemented utility process loading. Thanks! https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_icon.cc (right): https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.cc:25: Source(ArcAppIcon* host, const gfx::Size& size_in_dip); On 2015/11/11 22:15:49, xiyuan wrote: > |size_in_dip| seems not used? It is planned to use in future for new UI items but yes, at this moment we can remove this. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.cc:36: ArcAppIcon* host_; On 2015/11/11 22:15:49, xiyuan wrote: > Do we want to use a WeakPtr<ArcAppIcon> here? This way, we don't need to > manually call ResetHost. > > See > https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p... Thanks for good advice! It is more elegant (I used extensions::IconImage::Source for reference and it contains ResetHost) https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.cc:49: host_ = NULL; On 2015/11/11 22:15:49, xiyuan wrote: > nit: NULL -> nullptr Removed once WeakPtr is used. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.cc:93: host_ = NULL; On 2015/11/11 22:15:49, xiyuan wrote: > nit: NULL -> nullptr Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.cc:120: host_->observer_->OnIconUpdated(); On 2015/11/11 22:15:49, xiyuan wrote: > Would this be a use-after-free? |this| should be deleted after > DiscardDecodeRequest above and its |host_| member should not be accessed > thereafter. > > And think this should be part of host_'s Update() instead. Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.cc:144: source_(NULL), On 2015/11/11 22:15:49, xiyuan wrote: > nit: think in-class member initialization is preferred than this now. > > i.e. do this in the header file > Source* source_ = nullptr; Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.cc:150: Update(ResourceBundle::GetSharedInstance().GetImageSkiaNamed( On 2015/11/11 22:15:49, xiyuan wrote: > This Update() call has some side effects. > > ImageSkiaSource::GetImageForScale is only invoked when its hosting ImageSkia > does not have ImageRep for a scale factor. It looks like we are relying on this > behavior to trigger the ARC icon load. However, this Update() will fill in > default icon for certain scale factors and we will only see the default icon for > those scale factors as a result. > > To show the default icon before real icon, think we should return default icon > in Source::GetImageForScale instead. Yes, I also noticed this issue when was implementing. You suggestion is good fix. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.cc:158: ImageDecoder::Cancel(decode_requests_[i]); On 2015/11/11 22:15:49, xiyuan wrote: > nit: Cancel() is not necessary here since ImageDecoder::ImageRequest's dtor > would call it anyway. Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.cc:175: base::Unretained(this), On 2015/11/11 22:15:49, xiyuan wrote: > Should this be weak_ptr_factory_.GetWeakPtr() instead of base::Unretained? Yes, it should https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.cc:178: content::BrowserThread::PostTask(content::BrowserThread::FILE, On 2015/11/11 22:15:49, xiyuan wrote: > File thread is deprecated (because it is only one thread and easily got choked) > and we should use blocking pool in new code. > > BrowserThread::GetBlockingPool()->PostTask(...) Good to know! Done https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.cc:190: prefs->RequestIcon(app_id_, scale_factor); On 2015/11/11 22:15:49, xiyuan wrote: > How would we find out after ArcAppPrefs gets the icon? I added comment. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.cc:200: base::Unretained(this), On 2015/11/11 22:15:49, xiyuan wrote: > weak_ptr_factory_.GetWeakPtr()? Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.cc:217: base::Unretained(this), On 2015/11/11 22:15:49, xiyuan wrote: > weak_ptr_factory_.GetWeakPtr()? Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.cc:231: scale_factor)); On 2015/11/11 22:15:49, xiyuan wrote: > nit: align with previous line. Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_icon.h (right): https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.h:82: base::WeakPtrFactory<ArcAppIcon> weak_ptr_factory_; On 2015/11/11 22:15:50, xiyuan wrote: > [chromium-style] WeakPtrFactory members which refer to their outer class must be > the last member in the outer class definition. Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.h:93: void DiscardDecodeRequest(DecodeRequest* request); On 2015/11/11 22:15:50, xiyuan wrote: > Methods should precede data members. > See https://google.github.io/styleguide/cppguide.html#Declaration_Order Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_item.cc (right): https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_item.cc:15: const int kIconDimension = 48; On 2015/11/11 22:15:50, xiyuan wrote: > Should we use app_list::kGridIconDimension instead? > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/app_list/app_li... Yes. this is much better. I used extension icon as example and it also has separate constant for this. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_item.cc:48: set_position(syncer::StringOrdinal("nq")); On 2015/11/11 22:15:50, xiyuan wrote: > Can we leave position unset instead of using a hard-coded one? Default behavior > should be add the new item to the last if the item does not have sync data. Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_item.cc:64: DCHECK(app_info != nullptr); On 2015/11/11 22:15:50, xiyuan wrote: > nit: DCHECK_NE(app_info, nullptr) Done. Added LOG(ERROR) below https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_item.cc:103: void ArcAppItem::OnIconUpdated() { On 2015/11/11 22:15:50, xiyuan wrote: > ArcAppItem should observe its |icon_|. Otherwise, this won't get called. Sorry, I don't understand clear you. When I create icon_, I pass this class as observer and icon notifies us when it gets updated https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_item.h (right): https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_item.h:21: private ArcAppIcon::Observer { On 2015/11/11 22:15:50, xiyuan wrote: > All inheritance should be public. > > Details here: https://google.github.io/styleguide/cppguide.html#Inheritance Thanks for link. Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_item.h:30: ~ArcAppItem() override; On 2015/11/11 22:15:50, xiyuan wrote: > nit: alignment Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_item.h:37: ArcAppIcon* icon() { return icon_.get(); } On 2015/11/11 22:15:50, xiyuan wrote: > Can we give it a different name (e.g arc_app_icon()) because base class > app_list::AppListItem has a icon() const accessor? Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_item.h:40: private: On 2015/11/11 22:15:50, xiyuan wrote: > nit: insert an empty line before Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_item.h:43: scoped_ptr<ArcAppIcon> icon_; On 2015/11/11 22:15:50, xiyuan wrote: > Move data members after the methods. Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_model_builder.cc (right): https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_model_builder.cc:13: : profile_(NULL), On 2015/11/11 22:15:50, xiyuan wrote: > nit: Use in-class member initialization and use nullptr in place of NULL. Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_model_builder.cc:21: prefs_->RemoveObserver(this); On 2015/11/11 22:15:50, xiyuan wrote: > Think we need to make AppListSyncableService depends on ArcAppPrefs to ensure > |prefs_| is still valid at this point. > > Update AppListSyncableServiceFactory ctor with that dependency, somewhere > around: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_model_builder.cc:52: std::vector<std::string> app_ids = prefs_->GetAppIds(); On 2015/11/11 22:15:50, xiyuan wrote: > nit: #include <vector> Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_model_builder.h (right): https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_model_builder.h:49: app_list::AppListSyncableService* service_; On 2015/11/11 22:15:50, xiyuan wrote: > Move data members down. Done. Also made inheritance public https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_prefs.cc (right): https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:62: kArcApps, On 2015/11/11 22:15:51, xiyuan wrote: > kArcApps should be in chrome/common/pref_names.h Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:63: user_prefs::PrefRegistrySyncable::SYNCABLE_PREF); On 2015/11/11 22:15:50, xiyuan wrote: > ArcAppPrefs looks like a local cache to store app info before ARC is up and > running. Do we need to make it syncable? One of the next idea is to use sync-able feature to synchronize app across different devices. Elijah, could you please confirm that I understand correctly? That means we might want to install or uninstall some android app automatically. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:75: return crx_file::id_util::GenerateId(input); On 2015/11/11 22:15:50, xiyuan wrote: > nit: App list does not assume the format of the id as long as it is unique in > the model. So we don't need to generate something similar to chrome app id here. > > I am slightly inclined to have something with a special prefix, > e.g. > arcapp-<package>#<activity> > > Just in case we need to figure out item type from the id in the future. One of the reason I used this generator is '.' in package name that is not friendly when used as profile keys. So, this id is used in file paths and as key in prefs and it is safer to generate string that can be used everywhere. WDYT? https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:81: : browser_context_(browser_context), On 2015/11/11 22:15:50, xiyuan wrote: > |browser_context_| seems not used. Yes, we use here only root folder for profile. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:92: if (bridge_service->state() == arc::ArcBridgeService::State::READY) { On 2015/11/11 22:15:51, xiyuan wrote: > nit: How about doing "OnStateChanged(bridge_service->state());"? Personally I prefer to do but was not sure that is acceptable in chromium https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:100: LOG(ERROR) << "Bridge service is not available. Cannot release."; On 2015/11/11 22:15:51, xiyuan wrote: > Do we know how ArcBridgeService starts and shuts down? Profile and its > KeyedService is in pretty late stage. We might be always hitting this. This CL(1412863004) is still under review. But it is close to final stage and the lifetime of this service is determined by ChromeBrowserMainPartsChromeos class. But your comment makes sense and it is better to avoid error reporting. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:220: for (it = ready_apps_.begin(); it != ready_apps_.end(); ++it) { On 2015/11/11 22:15:51, xiyuan wrote: > nit: for (auto& app_id : ready_apps_) Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:282: std::set<std::string> old_ready_apps_; On 2015/11/11 22:15:51, xiyuan wrote: > nit: old_ready_apps_ -> old_ready_apps Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:291: for (it = old_ready_apps_.begin(); it != old_ready_apps_.end(); ++it) { On 2015/11/11 22:15:50, xiyuan wrote: > nit: for (auto& app_id : old_ready_apps) Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:323: base::Unretained(this), On 2015/11/11 22:15:50, xiyuan wrote: > base::Unretained is dangerous. Add a WeakPtrFactory and use WeakPtr instead. Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:327: content::BrowserThread::PostTask(content::BrowserThread::FILE, On 2015/11/11 22:15:50, xiyuan wrote: > Use BrowserThrad::GetBlockingPool instead of file thread. Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:346: DCHECK(wrote == static_cast<int>(content_png.size())); On 2015/11/11 22:15:51, xiyuan wrote: > nit: DCHECK_EQ(wrote, static_cast<int>(content_png.size())); As I understand potentially we may get situation when file is not written completely (end of space). In this case it is better to check actual written bytes and report error. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:349: base::Unretained(this), On 2015/11/11 22:15:51, xiyuan wrote: > ditto Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_prefs.h (right): https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.h:29: class ArcAppPrefs : public KeyedService, On 2015/11/11 22:15:51, xiyuan wrote: > nit: document the class? Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.h:30: private arc::ArcBridgeService::Observer { On 2015/11/11 22:15:51, xiyuan wrote: > no private inheritance. Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.h:45: virtual void OnAppReady(const std::string& id, bool ready) = 0; On 2015/11/11 22:15:51, xiyuan wrote: > Can an app change from ready to not ready? If so, maybe rename this to > OnAppReadyChanged. Yes, it may and this name sounds better https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.h:54: // Convenience function to get the ArcAppPrefs for a BrowserContext. On 2015/11/11 22:15:51, xiyuan wrote: > nit: insert an empty line above to separate Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.h:56: // Constructs unique id based on package and activity information. This id On 2015/11/11 22:15:51, xiyuan wrote: > nit: insert an empty line before Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.h:103: std::map<std::string, uint32> request_icon_deferred_; On 2015/11/11 22:15:51, xiyuan wrote: > nit: #include <map> Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.h:103: std::map<std::string, uint32> request_icon_deferred_; On 2015/11/11 22:15:51, xiyuan wrote: > Move data members down. Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.h:103: std::map<std::string, uint32> request_icon_deferred_; On 2015/11/11 22:15:51, xiyuan wrote: > Move data members down. Done. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.h:103: std::map<std::string, uint32> request_icon_deferred_; On 2015/11/11 22:15:51, xiyuan wrote: > nit: #include <map> Done.
PTAL. https://codereview.chromium.org/1413153007/diff/60001/chrome/browser/prefs/br... File chrome/browser/prefs/browser_prefs.cc (right): https://codereview.chromium.org/1413153007/diff/60001/chrome/browser/prefs/br... chrome/browser/prefs/browser_prefs.cc:198: #include "components/arc/arc_prefs.h" Sorry, this accidentally came after getting latest 1412863004. I remove it in this CL https://codereview.chromium.org/1413153007/diff/60001/chromeos/arc/bridge/com... File chromeos/arc/bridge/common/arc_host_messages.h (right): https://codereview.chromium.org/1413153007/diff/60001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_host_messages.h:8: #include "chromeos/chromeos_export.h" These changes(and below) come from recent 1412863004 https://codereview.chromium.org/1413153007/diff/60001/chromeos/arc/bridge/com... File chromeos/arc/bridge/common/arc_instance_messages.h (right): https://codereview.chromium.org/1413153007/diff/60001/chromeos/arc/bridge/com... chromeos/arc/bridge/common/arc_instance_messages.h:11: #include "chromeos/chromeos_export.h" These changes(and below) come from recent 1412863004
https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_item.cc (right): https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_item.cc:103: void ArcAppItem::OnIconUpdated() { On 2015/11/12 08:05:30, khmel1 wrote: > On 2015/11/11 22:15:50, xiyuan wrote: > > ArcAppItem should observe its |icon_|. Otherwise, this won't get called. > > Sorry, I don't understand clear you. When I create icon_, I pass this class as > observer and icon notifies us when it gets updated You are right. ArcAppItem is passed in |arc_app_icon_|'s ctor as observer. I kept thinking there should be AddObserver call somehow. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_prefs.cc (right): https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:63: user_prefs::PrefRegistrySyncable::SYNCABLE_PREF); On 2015/11/12 08:05:30, khmel1 wrote: > On 2015/11/11 22:15:50, xiyuan wrote: > > ArcAppPrefs looks like a local cache to store app info before ARC is up and > > running. Do we need to make it syncable? > > One of the next idea is to use sync-able feature to synchronize app across > different devices. Elijah, could you please confirm that I understand correctly? > That means we might want to install or uninstall some android app automatically. Acknowledged. Thanks for the explanation. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:75: return crx_file::id_util::GenerateId(input); On 2015/11/12 08:05:30, khmel1 wrote: > On 2015/11/11 22:15:50, xiyuan wrote: > > nit: App list does not assume the format of the id as long as it is unique in > > the model. So we don't need to generate something similar to chrome app id > here. > > > > I am slightly inclined to have something with a special prefix, > > e.g. > > arcapp-<package>#<activity> > > > > Just in case we need to figure out item type from the id in the future. > > One of the reason I used this generator is '.' in package name that is not > friendly when used as profile keys. So, this id is used in file paths and as key > in prefs and it is safer to generate string that can be used everywhere. WDYT? Acknowledged. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:100: LOG(ERROR) << "Bridge service is not available. Cannot release."; On 2015/11/12 08:05:31, khmel1 wrote: > On 2015/11/11 22:15:51, xiyuan wrote: > > Do we know how ArcBridgeService starts and shuts down? Profile and its > > KeyedService is in pretty late stage. We might be always hitting this. > > This CL(1412863004) is still under review. But it is close to final stage and > the lifetime of this service is determined by ChromeBrowserMainPartsChromeos > class. But your comment makes sense and it is better to avoid error reporting. Thanks for the CL reference. Yep, CL(1412863004) is stopping ArcBridgeService in PostMainMessageLoopRun, which happens before tearing down user profile. The RemoveObserver probably would not be called. It should be okay since ArcBridgeService's obsever list does not require all observers be removed. https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:346: DCHECK(wrote == static_cast<int>(content_png.size())); On 2015/11/12 08:05:30, khmel1 wrote: > On 2015/11/11 22:15:51, xiyuan wrote: > > nit: DCHECK_EQ(wrote, static_cast<int>(content_png.size())); > > As I understand potentially we may get situation when file is not written > completely (end of space). In this case it is better to check actual written > bytes and report error. Acknowledged. https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/app_list_syncable_service_factory.cc (right): https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/app_list_syncable_service_factory.cc:14: #include "chrome/browser/ui/app_list/arc_app_prefs_factory.h" Move this down into the #if defined(OS_CHROMEOS) block. https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/app_list_syncable_service_factory.cc:61: dependent_factories.insert(ArcAppPrefsFactory::GetInstance()); #if defined(OS_CHROMEOS) https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_prefs.h (right): https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.h:123: content::BrowserContext* browser_context_; remove? https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_prefs_factory.cc (right): https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs_factory.cc:36: Profile* profile = reinterpret_cast<Profile*>(context); reinterpret_cast<Profile*> -> static_cast<Profile*>
https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_icon.h (right): https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.h:25: // A class that provides an ImageSkia for UI code to use. It handles Arc app Use "ARC" consistently across this change. https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.h:52: // asynchronously read in context of browser file thread. On success read, nit: On successful read. https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.h:55: // active request is stored at decode_requests_ vector. When decoding is nit: |decode_requests_|. Same below. https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.h:61: // install required resource from Android side. ArcAppPrefs notifies UI items Try to say "ARC" instead of Android. https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.h:83: Source* source_ = nullptr; // Owned by ImageSkia storage. Can this be initialized in the constructor? https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_item.h (right): https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_item.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. "(c)" should not be used. `git cl format` _might_ fix this for you. Same in other files. https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_prefs.h (right): https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.h:35: class ArcAppPrefs : public KeyedService, Can this be called ArcAppListPrefs? https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.h:126: base::ObserverList<Observer> observer_list_; Comments for all these fields? Also separate them since it seems like they are owned by BrowserContext, but it's not true. https://codereview.chromium.org/1413153007/diff/80001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/1413153007/diff/80001/chrome/common/pref_name... chrome/common/pref_names.cc:16: // A preference to keep list of Arc(android) applications and its state. Let's just call these ARC applications. https://codereview.chromium.org/1413153007/diff/80001/chrome/common/pref_name... chrome/common/pref_names.cc:17: const char kArcApps[] = "arcapps"; const char kArcAppList[] = "arc.app_list"?
https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_icon.h (right): https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.h:83: Source* source_ = nullptr; // Owned by ImageSkia storage. On 2015/11/12 20:49:16, Luis Héctor Chávez wrote: > Can this be initialized in the constructor? This is the preferred style in chromium code. See https://chromium-cpp.appspot.com/, check out the "Non-Static Class Member Initializers" row.
https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_prefs.cc (right): https://codereview.chromium.org/1413153007/diff/40001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.cc:63: user_prefs::PrefRegistrySyncable::SYNCABLE_PREF); On 2015/11/12 17:32:44, xiyuan wrote: > On 2015/11/12 08:05:30, khmel1 wrote: > > On 2015/11/11 22:15:50, xiyuan wrote: > > > ArcAppPrefs looks like a local cache to store app info before ARC is up and > > > running. Do we need to make it syncable? > > > > One of the next idea is to use sync-able feature to synchronize app across > > different devices. Elijah, could you please confirm that I understand > correctly? > > That means we might want to install or uninstall some android app > automatically. > > Acknowledged. Thanks for the explanation. Just chiming in that yes we do want to sync this in the (near?) future.
I just skimmed this CL --- it is quite huge. Initial thoughts: - This CL is too long to review properly. I think it should be broken into smaller chunks and reviewed separately. (That would also help get the number of reviewers required for each CL down so we aren't talking across each other.) - It looks like a lot of the code in app_list has been copy-pasted from the existing extensions / apps code. (For example, ArcAppModelBuilder is a copy of ExtensionAppModelBuilder.) Although it may be a significant effort, I think it will be worth trying to reuse this existing code by refactoring / subclassing.
On 2015/11/13 07:39:54, Matt Giuca wrote: > I just skimmed this CL --- it is quite huge. Initial thoughts: Hi Matt, thank for taking a look into this. > - This CL is too long to review properly. I think it should be broken into > smaller chunks and reviewed separately. (That would also help get the number of > reviewers required for each CL down so we aren't talking across each other.) Yes, I agree with you that it looks big. I did not split into several CL because there is another ARC++ CL in chromium: 1412863004. In this CL it was request to merge CLs into one. My CL looks similar to it and I decided to keep everything in one. Asking reviewers, if you think that splitting this CL would help please let me know. > > - It looks like a lot of the code in app_list has been copy-pasted from the > existing extensions / apps code. (For example, ArcAppModelBuilder is a copy of > ExtensionAppModelBuilder.) Although it may be a significant effort, I think it > will be worth trying to reuse this existing code by refactoring / subclassing. ArcAppModelBuilder looks similar to ExtensionAppModelBuilder only by structure. I would mention that ::Initialize* are common. There is significant amount of code specific for extensions and for ARC.
1. Updated comments and renamed arc_app_prefs to arc_app_list_prefs 2. Adding activities parameter (before activities was reserved but not used. Now they are handled properly). 3. Found that leaving empty pos for app_item crashes Chrome. Added its calculation (Last item in first non-full page) PTAL https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/app_list_syncable_service_factory.cc (right): https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/app_list_syncable_service_factory.cc:14: #include "chrome/browser/ui/app_list/arc_app_prefs_factory.h" On 2015/11/12 17:32:44, xiyuan wrote: > Move this down into the #if defined(OS_CHROMEOS) block. Thanks, I missed this. https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/app_list_syncable_service_factory.cc:61: dependent_factories.insert(ArcAppPrefsFactory::GetInstance()); On 2015/11/12 17:32:44, xiyuan wrote: > #if defined(OS_CHROMEOS) Done. https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_icon.h (right): https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.h:25: // A class that provides an ImageSkia for UI code to use. It handles Arc app On 2015/11/12 20:49:16, Luis Héctor Chávez wrote: > Use "ARC" consistently across this change. Done. https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.h:52: // asynchronously read in context of browser file thread. On success read, On 2015/11/12 20:49:16, Luis Héctor Chávez wrote: > nit: On successful read. Done. https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.h:55: // active request is stored at decode_requests_ vector. When decoding is On 2015/11/12 20:49:16, Luis Héctor Chávez wrote: > nit: |decode_requests_|. Same below. Done. https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.h:61: // install required resource from Android side. ArcAppPrefs notifies UI items On 2015/11/12 20:49:16, Luis Héctor Chávez wrote: > Try to say "ARC" instead of Android. Done. https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.h:83: Source* source_ = nullptr; // Owned by ImageSkia storage. On 2015/11/12 20:49:16, Luis Héctor Chávez wrote: > Can this be initialized in the constructor? Acknowledged. https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_icon.h:83: Source* source_ = nullptr; // Owned by ImageSkia storage. On 2015/11/12 21:34:02, xiyuan wrote: > On 2015/11/12 20:49:16, Luis Héctor Chávez wrote: > > Can this be initialized in the constructor? > > This is the preferred style in chromium code. > > See https://chromium-cpp.appspot.com/, check out the "Non-Static Class Member > Initializers" row. Acknowledged. https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_item.h (right): https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_item.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/11/12 20:49:16, Luis Héctor Chávez wrote: > "(c)" should not be used. `git cl format` _might_ fix this for you. Same in > other files. Good catch. I was looking to extension_app_item where it was used. https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_prefs.h (right): https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.h:35: class ArcAppPrefs : public KeyedService, On 2015/11/12 20:49:16, Luis Héctor Chávez wrote: > Can this be called ArcAppListPrefs? Yes, we already have AppListPrefs so it might sounds better. https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.h:123: content::BrowserContext* browser_context_; On 2015/11/12 17:32:45, xiyuan wrote: > remove? Done. https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs.h:126: base::ObserverList<Observer> observer_list_; On 2015/11/12 20:49:16, Luis Héctor Chávez wrote: > Comments for all these fields? Also separate them since it seems like they are > owned by BrowserContext, but it's not true. Done. https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/arc_app_prefs_factory.cc (right): https://codereview.chromium.org/1413153007/diff/80001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/arc_app_prefs_factory.cc:36: Profile* profile = reinterpret_cast<Profile*>(context); On 2015/11/12 17:32:45, xiyuan wrote: > reinterpret_cast<Profile*> -> static_cast<Profile*> Done. https://codereview.chromium.org/1413153007/diff/80001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/1413153007/diff/80001/chrome/common/pref_name... chrome/common/pref_names.cc:16: // A preference to keep list of Arc(android) applications and its state. On 2015/11/12 20:49:16, Luis Héctor Chávez wrote: > Let's just call these ARC applications. Done. https://codereview.chromium.org/1413153007/diff/80001/chrome/common/pref_name... chrome/common/pref_names.cc:17: const char kArcApps[] = "arcapps"; On 2015/11/12 20:49:16, Luis Héctor Chávez wrote: > const char kArcAppList[] = "arc.app_list"? Done.
It seems that once an arc app is reported and the prefs would hold it forever and only flip its ready state. Is it intentional? How would we uninstall an arc app? And can we add some tests? If we can have a FakeArcBridgeService, it looks like that we can test most of the CL: - The prefs correctly stores the reported app from bridge; - The model builder correctly populate the arc apps to model; - The Item has correct name and icon; https://codereview.chromium.org/1413153007/diff/120001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc_app_list_prefs.cc (right): https://codereview.chromium.org/1413153007/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_list_prefs.cc:35: base::DictionaryValue* app = NULL; nit: NULL -> nullptr https://codereview.chromium.org/1413153007/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_list_prefs.cc:188: const base::DictionaryValue* app = NULL; nit: NULL -> nullptr https://codereview.chromium.org/1413153007/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_list_prefs.cc:205: const base::DictionaryValue* app = NULL; nit: NULL -> nullptr https://codereview.chromium.org/1413153007/diff/120001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc_app_list_prefs_factory.h (right): https://codereview.chromium.org/1413153007/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_list_prefs_factory.h:21: private: nit: insert an empty line before
1. 1412863004 was finally submitted and now I have a chance to rebase everything properly. This shows only real modifications. 2. Recent change at 1412863004 introduces 'enable_arc%': at common.gypi. I also updated my code to follow this scheme. 3. Arc code now uses #if defined(ENABLE_ARC) instead of #if defined(OS_CHROMEOS). This is done similar to 1412863004 4. Added unit tests for arc app functionality. 5. In order to provide FakeArcBridgeServiceForAppLauncher used in unit tests, I split ArcBridgeService to two parts. One part is ArcBridgeService includes common functionality and defines an interface. Second part ArcBridgeServiceImpl is the real bridge, based on IPC. FakeArcBridgeServiceForAppLauncher inherits from ArcBridgeService and used in unit tests. 6. In ArcAppIcon::LoadForScaleFactor and ArcAppListPrefs::InstallIcon changed weak_ptr_factory_.GetWeakPtr() to base::Unretained(this) because in Debug it throwed the error: FATAL:weak_ptr.cc(26) Check failed: sequence_checker_. CalledOnValidSequencedThread(). WeakPtrs must be checked on the same sequenced thread. Could someone suggest better solution? 7. Addressed nit comments. PTAL https://codereview.chromium.org/1413153007/diff/120001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc_app_list_prefs.cc (right): https://codereview.chromium.org/1413153007/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_list_prefs.cc:35: base::DictionaryValue* app = NULL; On 2015/11/13 17:14:22, xiyuan wrote: > nit: NULL -> nullptr Done. https://codereview.chromium.org/1413153007/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_list_prefs.cc:188: const base::DictionaryValue* app = NULL; On 2015/11/13 17:14:22, xiyuan wrote: > nit: NULL -> nullptr Done. https://codereview.chromium.org/1413153007/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_list_prefs.cc:205: const base::DictionaryValue* app = NULL; On 2015/11/13 17:14:22, xiyuan wrote: > nit: NULL -> nullptr Done. https://codereview.chromium.org/1413153007/diff/120001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc_app_list_prefs_factory.h (right): https://codereview.chromium.org/1413153007/diff/120001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_list_prefs_factory.h:21: private: On 2015/11/13 17:14:22, xiyuan wrote: > nit: insert an empty line before Done.
On 2015/11/13 17:14:23, xiyuan wrote: > It seems that once an arc app is reported and the prefs would hold it forever > and only flip its ready state. Is it intentional? How would we uninstall an arc > app? Thanks for asking this, I should mention this earlier. This CL is only minimal support required for our Demo. We have plan to support install/uninstall apps very soon. This CL + 3CLs on Android side is huge change and it is better to keep it smaller possible. > > And can we add some tests? If we can have a FakeArcBridgeService, it looks like > that we can test most of the CL: > - The prefs correctly stores the reported app from bridge; > - The model builder correctly populate the arc apps to model; > - The Item has correct name and icon; > > https://codereview.chromium.org/1413153007/diff/120001/chrome/browser/ui/app_... > File chrome/browser/ui/app_list/arc_app_list_prefs.cc (right): > > https://codereview.chromium.org/1413153007/diff/120001/chrome/browser/ui/app_... > chrome/browser/ui/app_list/arc_app_list_prefs.cc:35: base::DictionaryValue* app > = NULL; > nit: NULL -> nullptr > > https://codereview.chromium.org/1413153007/diff/120001/chrome/browser/ui/app_... > chrome/browser/ui/app_list/arc_app_list_prefs.cc:188: const > base::DictionaryValue* app = NULL; > nit: NULL -> nullptr > > https://codereview.chromium.org/1413153007/diff/120001/chrome/browser/ui/app_... > chrome/browser/ui/app_list/arc_app_list_prefs.cc:205: const > base::DictionaryValue* app = NULL; > nit: NULL -> nullptr > > https://codereview.chromium.org/1413153007/diff/120001/chrome/browser/ui/app_... > File chrome/browser/ui/app_list/arc_app_list_prefs_factory.h (right): > > https://codereview.chromium.org/1413153007/diff/120001/chrome/browser/ui/app_... > chrome/browser/ui/app_list/arc_app_list_prefs_factory.h:21: private: > nit: insert an empty line before
On 2015/11/17 13:18:02, khmel1 wrote:
...
> 6. In ArcAppIcon::LoadForScaleFactor and ArcAppListPrefs::InstallIcon
> changed weak_ptr_factory_.GetWeakPtr() to base::Unretained(this)
> because in Debug it throwed the error:
> FATAL:weak_ptr.cc(26) Check failed: sequence_checker_.
> CalledOnValidSequencedThread(). WeakPtrs must be checked
> on the same sequenced thread.
> Could someone suggest better solution?
...
Right. I missed that. WeakPtr is not thread safe and cannot be used on threads
other than where it is created.
In this case, we should make ReadOnFileThread take all needed info from its
argument (e.g. extract it into a static member func or move it out of the class
into a function in anonymous namespace) to avoid having to pass it |this|.
And we can use base::PostTaskAndReplyWithResult [1], e.g.
namespace {
// Returns the bytes read. Empty string if the file does not exist.
std::string ReadOnFileThread(base::FilePath& path) {
if (!base::PathExists(path)) {
return std::string();
}
std::string unsafe_icon_data;
base::ReadFileToString(path, &unsafe_icon_data);
return unsafe_icon_data;
}
} // namespace
void ArcAppIcon::OnFileRead(ui::ScaleFactor scale_factor,
const std::string& unsafe_icon_data) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
if (unsafe_icon_data.empty()) {
RequestIcon(scale_factor);
return;
}
OnIconRead(scale_factor, unsafe_icon_data);
}
And on the call site in ArcAppIcon::LoadForScaleFactor, we can do
base::PostTaskAndReplyWithResult(
FROM_HERE,
base::Bind(&ReadOnFileThread, path),
base::Bind(&ArcAppIcon::OnFileRead,
weak_ptr_factory_.GetWeakPtr(),
scale_factor));
[1]:
https://code.google.com/p/chromium/codesearch#chromium/src/base/task_runner_u...
On 2015/11/17 19:28:55, xiyuan wrote:
> On 2015/11/17 13:18:02, khmel1 wrote:
> ...
> > 6. In ArcAppIcon::LoadForScaleFactor and ArcAppListPrefs::InstallIcon
> > changed weak_ptr_factory_.GetWeakPtr() to base::Unretained(this)
> > because in Debug it throwed the error:
> > FATAL:weak_ptr.cc(26) Check failed: sequence_checker_.
> > CalledOnValidSequencedThread(). WeakPtrs must be checked
> > on the same sequenced thread.
> > Could someone suggest better solution?
> ...
>
> Right. I missed that. WeakPtr is not thread safe and cannot be used on threads
> other than where it is created.
>
> In this case, we should make ReadOnFileThread take all needed info from its
> argument (e.g. extract it into a static member func or move it out of the
class
> into a function in anonymous namespace) to avoid having to pass it |this|.
>
> And we can use base::PostTaskAndReplyWithResult [1], e.g.
>
> namespace {
>
> // Returns the bytes read. Empty string if the file does not exist.
> std::string ReadOnFileThread(base::FilePath& path) {
> if (!base::PathExists(path)) {
> return std::string();
> }
>
> std::string unsafe_icon_data;
> base::ReadFileToString(path, &unsafe_icon_data);
> return unsafe_icon_data;
> }
>
> } // namespace
>
> void ArcAppIcon::OnFileRead(ui::ScaleFactor scale_factor,
> const std::string& unsafe_icon_data) {
> DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
> if (unsafe_icon_data.empty()) {
> RequestIcon(scale_factor);
> return;
> }
>
> OnIconRead(scale_factor, unsafe_icon_data);
> }
>
> And on the call site in ArcAppIcon::LoadForScaleFactor, we can do
>
> base::PostTaskAndReplyWithResult(
> FROM_HERE,
> base::Bind(&ReadOnFileThread, path),
> base::Bind(&ArcAppIcon::OnFileRead,
> weak_ptr_factory_.GetWeakPtr(),
> scale_factor));
>
>
> [1]:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/task_runner_u...
Thanks for idea! It works well.
Fixed WeakPtr issue. PTAL https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc_app_icon.cc (right): https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_icon.cc:24: struct ArcAppIcon::ReadResult { I need to return more statuses than just ok/fail. So this struct is returned as result instead of simple std::string
Awesome. Thank you for refactoring and adding test. Mostly nits now. https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/chrome_browser_main_chromeos.h (right): https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/chrome_browser_main_chromeos.h:87: scoped_ptr<arc::ArcBridgeServiceImpl> arc_bridge_service_; The cc file needs to know ArcBridgeServiceImpl to create an instance. But here, think we should only need ARcBridgeService. https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc_app_icon.cc (right): https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_icon.cc:38: scoped_ptr<ArcAppIcon::ReadResult> ReadOnFileThread( nit: Having this in an anonymous namespace in middle of the class impl reads a bit strange. Maybe make this a private static member and make ReadResult private as well. https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc_app_model_builder.cc (right): https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_model_builder.cc:49: for (size_t i = 0; i < app_ids.size(); ++i) { nit: for (auto& app_id : app_ids) { https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc_app_unittest.cc (right): https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_unittest.cc:35: sizeof(FakeArcBridgeServiceForAppLauncher::AppInfo); nit: use arraysize in base/macros.h https://code.google.com/p/chromium/codesearch#chromium/src/base/macros.h&l=56 https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_unittest.cc:88: size_t GetModelItemCnt() const { nit: GetModelItemCnt -> GetArcItemCount to be more specific https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_unittest.cc:100: ArcAppItem* GetModelItem(size_t index) const { nit: GetModelItem -> GetArcItem https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_unittest.cc:117: ArcAppItem* FindModelItem(const std::string& id) const { nit: FindModelItem -> FindArcItem https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_unittest.cc:285: base::MessageLoop* current = base::MessageLoop::current(); New code should use base::RunLoop instead of using MessageLoop directly. We can just do base::RunLoop().RunUntilIdle(); at line 311. https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/test/fake_arc_bridge_service_for_app_launcher.cc (right): https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/test/fake_arc_bridge_service_for_app_launcher.cc:105: std::vector<unsigned char> result; |result| seems unused. https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/test/fake_arc_bridge_service_for_app_launcher.h (right): https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/test/fake_arc_bridge_service_for_app_launcher.h:15: class FakeArcBridgeServiceForAppLauncher : public arc::ArcBridgeService { This does not feel to be app_list specific. Think this file should be put in components/arc[/test], with ArcBridgeService and just call it FakeArcBridgeService. https://codereview.chromium.org/1413153007/diff/160001/components/arc/arc_bri... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1413153007/diff/160001/components/arc/arc_bri... components/arc/arc_bridge_service.h:155: class ArcBridgeServiceImpl : public ArcBridgeService, public IPC::Listener { Move this into its own file, arc_bridge_service_impl.h/cc
Hi Xiyuan, thank you for your reviews. I updated CL, no functional changes, only nits are addressed. PTAL https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc_app_icon.cc (right): https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_icon.cc:38: scoped_ptr<ArcAppIcon::ReadResult> ReadOnFileThread( On 2015/11/18 03:42:16, xiyuan wrote: > nit: Having this in an anonymous namespace in middle of the class impl reads a > bit strange. Maybe make this a private static member and make ReadResult private > as well. Yep, I also had filling that this was a little strange and was hesitating were to put it. https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc_app_model_builder.cc (right): https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_model_builder.cc:49: for (size_t i = 0; i < app_ids.size(); ++i) { On 2015/11/18 03:42:16, xiyuan wrote: > nit: for (auto& app_id : app_ids) { Done. https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc_app_unittest.cc (right): https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_unittest.cc:35: sizeof(FakeArcBridgeServiceForAppLauncher::AppInfo); On 2015/11/18 03:42:16, xiyuan wrote: > nit: use arraysize in base/macros.h > > https://code.google.com/p/chromium/codesearch#chromium/src/base/macros.h&l=56 Good to know, thanks! https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_unittest.cc:88: size_t GetModelItemCnt() const { On 2015/11/18 03:42:16, xiyuan wrote: > nit: GetModelItemCnt -> GetArcItemCount to be more specific Done. https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_unittest.cc:100: ArcAppItem* GetModelItem(size_t index) const { On 2015/11/18 03:42:16, xiyuan wrote: > nit: GetModelItem -> GetArcItem Done. https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_unittest.cc:117: ArcAppItem* FindModelItem(const std::string& id) const { On 2015/11/18 03:42:16, xiyuan wrote: > nit: FindModelItem -> FindArcItem Done. https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_unittest.cc:285: base::MessageLoop* current = base::MessageLoop::current(); On 2015/11/18 03:42:16, xiyuan wrote: > New code should use base::RunLoop instead of using MessageLoop directly. We can > just do > > base::RunLoop().RunUntilIdle(); > > at line 311. Done. https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/test/fake_arc_bridge_service_for_app_launcher.cc (right): https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/test/fake_arc_bridge_service_for_app_launcher.cc:105: std::vector<unsigned char> result; On 2015/11/18 03:42:16, xiyuan wrote: > |result| seems unused. Done. https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/test/fake_arc_bridge_service_for_app_launcher.h (right): https://codereview.chromium.org/1413153007/diff/160001/chrome/browser/ui/app_... chrome/browser/ui/app_list/test/fake_arc_bridge_service_for_app_launcher.h:15: class FakeArcBridgeServiceForAppLauncher : public arc::ArcBridgeService { On 2015/11/18 03:42:16, xiyuan wrote: > This does not feel to be app_list specific. Think this file should be put in > components/arc[/test], with ArcBridgeService and just call it > FakeArcBridgeService. Yes, makes sense. https://codereview.chromium.org/1413153007/diff/160001/components/arc/arc_bri... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/1413153007/diff/160001/components/arc/arc_bri... components/arc/arc_bridge_service.h:155: class ArcBridgeServiceImpl : public ArcBridgeService, public IPC::Listener { On 2015/11/18 03:42:16, xiyuan wrote: > Move this into its own file, arc_bridge_service_impl.h/cc Done.
LGTM Cool. Thank you for making all the changes. Think you still need a few more OWNER's stamps. "git cl presubmit" can tell you which files need them. https://codereview.chromium.org/1413153007/diff/180001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc_app_icon.cc (right): https://codereview.chromium.org/1413153007/diff/180001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_icon.cc:197: scoped_ptr<ArcAppIcon::ReadResult> ArcAppIcon::ReadOnFileThread( nit: chromium code usually adds a "// static" comment prior static method impl https://codereview.chromium.org/1413153007/diff/180001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc_app_icon.h (right): https://codereview.chromium.org/1413153007/diff/180001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_icon.h:30: struct ReadResult; nit: Think this can be private now. https://codereview.chromium.org/1413153007/diff/180001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc_app_unittest.cc (right): https://codereview.chromium.org/1413153007/diff/180001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_unittest.cc:307: base::RunLoop().RunUntilIdle(); nit: #include "base/run_loop.h" https://codereview.chromium.org/1413153007/diff/180001/chrome/chrome_tests_un... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/1413153007/diff/180001/chrome/chrome_tests_un... chrome/chrome_tests_unit.gypi:1640: '../components/arc/test/fake_arc_bridge_service.h', We should add a static_libary target in components/arc/BUILD.gn (and arc.gypi) then add that as a dependency around where you add the chrome_unit_tests_app_list_chromeos_arc_sources. At least, put a TODO here to clean it up later when FakeArcBridgeService needs to be used elsewhere. https://codereview.chromium.org/1413153007/diff/180001/components/arc/DEPS File components/arc/DEPS (right): https://codereview.chromium.org/1413153007/diff/180001/components/arc/DEPS#ne... components/arc/DEPS:2: "+chrome/browser/ui/app_list/arc_app_list_prefs.h", We should probably add a new DEPS file for test/ dir and add those rules there. https://codereview.chromium.org/1413153007/diff/180001/components/arc/arc_bri... File components/arc/arc_bridge_service_impl.h (right): https://codereview.chromium.org/1413153007/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.h:19: class ArcBridgeServiceImpl : public ArcBridgeService, public IPC::Listener { nit: move "public IPC::Listener" to its own line.
https://codereview.chromium.org/1413153007/diff/180001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc_app_unittest.cc (right): https://codereview.chromium.org/1413153007/diff/180001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_unittest.cc:87: size_t GetArcItemCnt() const { nit: GetArcItemCount. I don't see count being abbreviated to 'cnt' anywhere else, so also do not do that in any param name or in L34. https://codereview.chromium.org/1413153007/diff/180001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_unittest.cc:178: // Process the rest apps. nit: the rest of the https://codereview.chromium.org/1413153007/diff/180001/chrome/chrome_tests_un... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/1413153007/diff/180001/chrome/chrome_tests_un... chrome/chrome_tests_unit.gypi:1640: '../components/arc/test/fake_arc_bridge_service.h', On 2015/11/18 17:41:15, xiyuan wrote: > We should add a static_libary target in components/arc/BUILD.gn (and arc.gypi) > then add that as a dependency around where you add the > chrome_unit_tests_app_list_chromeos_arc_sources. > > At least, put a TODO here to clean it up later when FakeArcBridgeService needs > to be used elsewhere. I'd also prefer for the FakeArcBridgeService to be built there and its library referenced here. https://codereview.chromium.org/1413153007/diff/180001/components/arc/arc_bri... File components/arc/arc_bridge_service_impl.cc (right): https://codereview.chromium.org/1413153007/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.cc:109: DCHECK(ipc_task_runner_->RunsTasksOnCurrentThread()); Unfortunately, there was a bug in the original CL, so all the app-related methods you added need to DCHECK(origin_task_runner_->RunsTasksOnCurrentThread()); https://codereview.chromium.org/1413153007/diff/180001/components/arc/arc_bri... File components/arc/arc_bridge_service_impl.h (right): https://codereview.chromium.org/1413153007/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.h:39: // Requests to refresh an app list. nit: Be consistent with the spacing. Add a newline before L39. https://codereview.chromium.org/1413153007/diff/180001/components/arc/test/fa... File components/arc/test/fake_arc_bridge_service.h (right): https://codereview.chromium.org/1413153007/diff/180001/components/arc/test/fa... components/arc/test/fake_arc_bridge_service.h:5: #ifndef CHROME_BROWSER_UI_APP_LIST_TEST_FAKE_ARC_BRIDGE_SERVICE_FOR_APP_LAUNCHER_H_ COMPONENTS_ARC_TEST_FAKE_ARC_BRIDGE_SERVICE_H https://codereview.chromium.org/1413153007/diff/180001/components/arc/test/fa... components/arc/test/fake_arc_bridge_service.h:47: int refresh_apps_cnt() const { return refresh_apps_cnt_; } As before, don't abbreviate 'count'.
https://codereview.chromium.org/1413153007/diff/180001/components/arc/arc_bri... File components/arc/arc_bridge_service_impl.cc (left): https://codereview.chromium.org/1413153007/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.cc:181: bool ArcBridgeService::Connect(const IPC::ChannelHandle& handle, Also, is it possible to preserve the order of methods to reduce diff length?
khmel@google.com changed reviewers: + dcheng@chromium.org, sadrul@chromium.org
Nit comments were addressed. Elijah, could you please take a look? You are OWNER of components/arc/ Daniel, could you please take a look to components/arc/common/* and components/arc/test/DEPS? Sadrul, could you please take a look to components/arc/test/DEPS? PTAL https://codereview.chromium.org/1413153007/diff/180001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc_app_icon.cc (right): https://codereview.chromium.org/1413153007/diff/180001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_icon.cc:197: scoped_ptr<ArcAppIcon::ReadResult> ArcAppIcon::ReadOnFileThread( On 2015/11/18 17:41:15, xiyuan wrote: > nit: chromium code usually adds a "// static" comment prior static method impl Done. https://codereview.chromium.org/1413153007/diff/180001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc_app_icon.h (right): https://codereview.chromium.org/1413153007/diff/180001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_icon.h:30: struct ReadResult; On 2015/11/18 17:41:15, xiyuan wrote: > nit: Think this can be private now. Yes, is works update recent updates. https://codereview.chromium.org/1413153007/diff/180001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc_app_unittest.cc (right): https://codereview.chromium.org/1413153007/diff/180001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_unittest.cc:87: size_t GetArcItemCnt() const { On 2015/11/18 18:22:19, Luis Héctor Chávez wrote: > nit: GetArcItemCount. I don't see count being abbreviated to 'cnt' anywhere > else, so also do not do that in any param name or in L34. Done. https://codereview.chromium.org/1413153007/diff/180001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_unittest.cc:178: // Process the rest apps. On 2015/11/18 18:22:18, Luis Héctor Chávez wrote: > nit: the rest of the Done. https://codereview.chromium.org/1413153007/diff/180001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_unittest.cc:307: base::RunLoop().RunUntilIdle(); On 2015/11/18 17:41:15, xiyuan wrote: > nit: #include "base/run_loop.h" Done. https://codereview.chromium.org/1413153007/diff/180001/chrome/chrome_tests_un... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/1413153007/diff/180001/chrome/chrome_tests_un... chrome/chrome_tests_unit.gypi:1640: '../components/arc/test/fake_arc_bridge_service.h', On 2015/11/18 18:22:19, Luis Héctor Chávez wrote: > On 2015/11/18 17:41:15, xiyuan wrote: > > We should add a static_libary target in components/arc/BUILD.gn (and arc.gypi) > > then add that as a dependency around where you add the > > chrome_unit_tests_app_list_chromeos_arc_sources. > > > > At least, put a TODO here to clean it up later when FakeArcBridgeService needs > > to be used elsewhere. > > I'd also prefer for the FakeArcBridgeService to be built there and its library > referenced here. Thanks for advice, it is really better. https://codereview.chromium.org/1413153007/diff/180001/components/arc/DEPS File components/arc/DEPS (right): https://codereview.chromium.org/1413153007/diff/180001/components/arc/DEPS#ne... components/arc/DEPS:2: "+chrome/browser/ui/app_list/arc_app_list_prefs.h", On 2015/11/18 17:41:15, xiyuan wrote: > We should probably add a new DEPS file for test/ dir and add those rules there. Yes, this is more elegant https://codereview.chromium.org/1413153007/diff/180001/components/arc/arc_bri... File components/arc/arc_bridge_service_impl.cc (left): https://codereview.chromium.org/1413153007/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.cc:181: bool ArcBridgeService::Connect(const IPC::ChannelHandle& handle, On 2015/11/18 18:33:06, Luis Héctor Chávez wrote: > Also, is it possible to preserve the order of methods to reduce diff length? Done. https://codereview.chromium.org/1413153007/diff/180001/components/arc/arc_bri... File components/arc/arc_bridge_service_impl.cc (right): https://codereview.chromium.org/1413153007/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.cc:109: DCHECK(ipc_task_runner_->RunsTasksOnCurrentThread()); On 2015/11/18 18:22:19, Luis Héctor Chávez wrote: > Unfortunately, there was a bug in the original CL, so all the app-related > methods you added need to > > DCHECK(origin_task_runner_->RunsTasksOnCurrentThread()); Done. Should we do the same for RegisterInputDevice? I updated it so. Please let me know if I am wrong. https://codereview.chromium.org/1413153007/diff/180001/components/arc/arc_bri... File components/arc/arc_bridge_service_impl.h (right): https://codereview.chromium.org/1413153007/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.h:19: class ArcBridgeServiceImpl : public ArcBridgeService, public IPC::Listener { On 2015/11/18 17:41:15, xiyuan wrote: > nit: move "public IPC::Listener" to its own line. Done. https://codereview.chromium.org/1413153007/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.h:39: // Requests to refresh an app list. On 2015/11/18 18:22:19, Luis Héctor Chávez wrote: > nit: Be consistent with the spacing. Add a newline before L39. Done. https://codereview.chromium.org/1413153007/diff/180001/components/arc/test/fa... File components/arc/test/fake_arc_bridge_service.h (right): https://codereview.chromium.org/1413153007/diff/180001/components/arc/test/fa... components/arc/test/fake_arc_bridge_service.h:5: #ifndef CHROME_BROWSER_UI_APP_LIST_TEST_FAKE_ARC_BRIDGE_SERVICE_FOR_APP_LAUNCHER_H_ On 2015/11/18 18:22:19, Luis Héctor Chávez wrote: > COMPONENTS_ARC_TEST_FAKE_ARC_BRIDGE_SERVICE_H Done.
https://codereview.chromium.org/1413153007/diff/220001/components/arc/BUILD.gn File components/arc/BUILD.gn (right): https://codereview.chromium.org/1413153007/diff/220001/components/arc/BUILD.g... components/arc/BUILD.gn:21: static_library("fake_arc_bridge_service") { nit: I'd call this "test_support", just in case we will add more stuff there later and it is more consistent with other components. https://codereview.chromium.org/1413153007/diff/220001/components/arc/test/fa... File components/arc/test/fake_arc_bridge_service.h (right): https://codereview.chromium.org/1413153007/diff/220001/components/arc/test/fa... components/arc/test/fake_arc_bridge_service.h:5: #ifndef COMPONENTS_ARC_TEST_FAKE_ARC_BRIDGE_SERVICE_H nit: end it with "_", i.e. COMPONENTS_ARC_TEST_FAKE_ARC_BRIDGE_SERVICE_H_
elijahtaylor@google.com changed reviewers: + elijahtaylor@google.com
LGTM, mostly nits a couple high level comments: 1) chromium commits should reference chromium bugs. I'm working on getting some automatic mirroring going between buganizer issues with chrome dependencies, but for now let's just file them manually at crbug.com 2) I just want to make sure you ran unit_tests with enable_arc=1, it wasn't clear from the TEST= line 3) We talked about whether to open a new public issue to run tryjobs, did you figure out a plan there? https://codereview.chromium.org/1413153007/diff/220001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc_app_list_prefs.cc (right): https://codereview.chromium.org/1413153007/diff/220001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_list_prefs.cc:317: for (auto& app_id : old_ready_apps) { this is a little unexpected to me. Should apps get disabled if they existed before and now don't exist in the refresh? Do we expect that to happen? I would think this would signify an app removal more than anything else. The not-ready to ready state seems like it would be used for ARC not ready but apps are in prefs, and then ARC is ready. (or now I see the bridge stopping would disable apps too) https://codereview.chromium.org/1413153007/diff/220001/chrome/chrome_browser_... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/1413153007/diff/220001/chrome/chrome_browser_... chrome/chrome_browser_ui.gypi:2954: ['chromeos==1 and enable_arc == 1', { nit: looks like the convention is enable_arc==1 https://codereview.chromium.org/1413153007/diff/220001/components/arc/common/... File components/arc/common/arc_instance_messages.h (right): https://codereview.chromium.org/1413153007/diff/220001/components/arc/common/... components/arc/common/arc_instance_messages.h:40: // It sends a request to ARC for the ARC app icon of a required scale factor. nit: remove "It"? https://codereview.chromium.org/1413153007/diff/220001/components/arc/test/fa... File components/arc/test/fake_arc_bridge_service.h (right): https://codereview.chromium.org/1413153007/diff/220001/components/arc/test/fa... components/arc/test/fake_arc_bridge_service.h:17: struct AppInfo { I was a little confused by this at first because it shares a name with another similar struct in the app list code. Maybe a different name to make this clear it's just for testing purposes? https://codereview.chromium.org/1413153007/diff/220001/components/arc/test/fa... components/arc/test/fake_arc_bridge_service.h:41: bool LaunchApp(const std::string& package,CHROME_BROWSER_UI_APP_LIST_TEST_FAKE_ARC_BRIDGE_SERVICE_FOR_APP_LAUNCHER_H_ looks like an unintentional paste accident
On 2015/11/13 14:43:53, khmel1 wrote: > On 2015/11/13 07:39:54, Matt Giuca wrote: > > I just skimmed this CL --- it is quite huge. Initial thoughts: > > Hi Matt, thank for taking a look into this. Sorry it has been a few days since I got back to you. I still have concerns about both a) the overall size of this CL, and b) potential for code reuse versus duplication. > > > - This CL is too long to review properly. I think it should be broken into > > smaller chunks and reviewed separately. (That would also help get the number > of > > reviewers required for each CL down so we aren't talking across each other.) > > Yes, I agree with you that it looks big. I did not split into several CL because > there is another ARC++ CL in chromium: 1412863004. In this CL it was request to > merge CLs into one. My CL looks similar to it and I decided to keep everything > in > one. Asking reviewers, if you think that splitting this CL would help please > let me know. I do think that would help. I realise this is in the late stages and has already gotten reviews and LGs (so maybe this can just be a lesson for the future and not necessarily applicable to this CL). Splitting CLs into smaller chunks is usually a good thing -- if in doubt, split. If reviewers feel they need more context, you can link them to the other CLs. Large CLs tend to be quadratic in complexity, as you need to pull in more reviewers (this one has 8 reviewers) and they generally all need to review the whole thing. That means more comments, more revisions (this is up to 13 patch sets), and ironically it being generally more difficult for reviewers to see the big picture. Most importantly, I think while Xiyuan, Elijah and Luis have done a very detailed job, it is in general much easier for bugs to slip in in a CL of this size. If I had seen https://codereview.chromium.org/1412863004 I would have also said it was too big... but even there, that was only +835 lines. This is +2348 lines, which is about three times as big. My rule of thumb is >400 lines is "too big". > > > > > - It looks like a lot of the code in app_list has been copy-pasted from the > > existing extensions / apps code. (For example, ArcAppModelBuilder is a copy of > > ExtensionAppModelBuilder.) Although it may be a significant effort, I think it > > will be worth trying to reuse this existing code by refactoring / subclassing. > ArcAppModelBuilder looks similar to ExtensionAppModelBuilder only by structure. > I would mention that ::Initialize* are common. There is significant amount of > code specific for extensions and for ARC. I agree, but I think these classes have more in common than different. I took a more detailed look and I agree that the other classes are fairly unique. But AAMB and EAMB have too much in common --- I definitely think you can reuse some code. Make a common base class for them and put the common code there. Then put the specific code in the subclasses. I have made specific notes where I think code can be reused.
https://codereview.chromium.org/1413153007/diff/220001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc_app_model_builder.h (right): https://codereview.chromium.org/1413153007/diff/220001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_model_builder.h:25: class ArcAppModelBuilder : public ArcAppListPrefs::Observer { I wanted to take another look over this CL to see if there were any duplications of existing code. This class seems to be almost a complete duplicate of ExtensionAppModelBuilder. https://codereview.chromium.org/1413153007/diff/240001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc_app_icon.cc (right): https://codereview.chromium.org/1413153007/diff/240001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_icon.cc:5: #include "chrome/browser/ui/app_list/arc_app_icon.h" This is a significant number of files. I think it would be prudent to move them all into chrome/browser/ui/app_list/arc/, to keep them contained. The top-level app_list directory already has enough stuff in it. https://codereview.chromium.org/1413153007/diff/240001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc_app_model_builder.cc (right): https://codereview.chromium.org/1413153007/diff/240001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_model_builder.cc:20: void ArcAppModelBuilder::InitializeWithService( This is almost the same as ExtensionAppModelBuilder::InitializeWithService. https://codereview.chromium.org/1413153007/diff/240001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_model_builder.cc:32: app_list::AppListModel* model) { This is almost the same as ExtensionAppModelBuilder::InitializeWithProfile. But it is missing model_->top_level_item_list()->AddObserver(this);. (Is it meant to be missing that?) https://codereview.chromium.org/1413153007/diff/240001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_model_builder.cc:58: ArcAppItem* ArcAppModelBuilder::GetArcAppItem(const std::string& item_id) { This is almost the same as ExtensionAppModelBuilder::GetExtensionAppItem (even with the same error). But it has a different return type. Even if the signature has to be different, it could at least be abstracted into a GetAppItem protected method of a common base class, with a public wrapper that does the static_cast. https://codereview.chromium.org/1413153007/diff/240001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_model_builder.cc:80: void ArcAppModelBuilder::InsertApp(scoped_ptr<ArcAppItem> app) { This is exactly the same as ExtensionAppModelBuilder::InsertApp.
Description was changed from ========== arc-app-launcher: Minimal support for ARC app launcher. This CL comes along with: https://googleplex-android-review.git.corp.google.com/#/c/797530 https://googleplex-android-review.git.corp.google.com/795572 https://chrome-internal-review.googlesource.com/#/c/235195 This includes set of IPC messages to interact with android via ARC bridge service in chromeos. This provides extension to app_list model that manages arc apps. It includes preference based service that interacts with arc bridge and supports an arc app life cycle; app list model builder for arc apps; arc list item. 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. ========== to ========== arc-app-launcher: Minimal support for ARC app launcher. This CL comes along with: https://googleplex-android-review.git.corp.google.com/#/c/797530 https://googleplex-android-review.git.corp.google.com/795572 https://chrome-internal-review.googlesource.com/#/c/235195 This includes set of IPC messages to interact with android via ARC bridge service in chromeos. This provides extension to app_list model that manages arc apps. It includes preference based service that interacts with arc bridge and supports an arc app life cycle; app list model builder for arc apps; arc list item. 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=unit_tests with enable_arc=1 Debug/Release ==========
On 2015/11/19 05:52:01, elijahtaylor (use chromium) wrote: > LGTM, mostly nits > > a couple high level comments: > > 1) chromium commits should reference chromium bugs. I'm working on getting some > automatic mirroring going between buganizer issues with chrome dependencies, but > for now let's just file them manually at http://crbug.com Done > > 2) I just want to make sure you ran unit_tests with enable_arc=1, it wasn't > clear from the TEST= line Done > > 3) We talked about whether to open a new public issue to run tryjobs, did you > figure out a plan there? I tried at 1456933003 but with no success (I created new public CL with no reviewers), git cl try also did not work. It seems it is possible to uncheck protected for this issue to make it public. Right? > > https://codereview.chromium.org/1413153007/diff/220001/chrome/browser/ui/app_... > File chrome/browser/ui/app_list/arc_app_list_prefs.cc (right): > > https://codereview.chromium.org/1413153007/diff/220001/chrome/browser/ui/app_... > chrome/browser/ui/app_list/arc_app_list_prefs.cc:317: for (auto& app_id : > old_ready_apps) { > this is a little unexpected to me. Should apps get disabled if they existed > before and now don't exist in the refresh? Do we expect that to happen? I > would think this would signify an app removal more than anything else. > > The not-ready to ready state seems like it would be used for ARC not ready but > apps are in prefs, and then ARC is ready. (or now I see the bridge stopping > would disable apps too) > > https://codereview.chromium.org/1413153007/diff/220001/chrome/chrome_browser_... > File chrome/chrome_browser_ui.gypi (right): > > https://codereview.chromium.org/1413153007/diff/220001/chrome/chrome_browser_... > chrome/chrome_browser_ui.gypi:2954: ['chromeos==1 and enable_arc == 1', { > nit: looks like the convention is enable_arc==1 > > https://codereview.chromium.org/1413153007/diff/220001/components/arc/common/... > File components/arc/common/arc_instance_messages.h (right): > > https://codereview.chromium.org/1413153007/diff/220001/components/arc/common/... > components/arc/common/arc_instance_messages.h:40: // It sends a request to ARC > for the ARC app icon of a required scale factor. > nit: remove "It"? > > https://codereview.chromium.org/1413153007/diff/220001/components/arc/test/fa... > File components/arc/test/fake_arc_bridge_service.h (right): > > https://codereview.chromium.org/1413153007/diff/220001/components/arc/test/fa... > components/arc/test/fake_arc_bridge_service.h:17: struct AppInfo { > I was a little confused by this at first because it shares a name with another > similar struct in the app list code. Maybe a different name to make this clear > it's just for testing purposes? > > https://codereview.chromium.org/1413153007/diff/220001/components/arc/test/fa... > components/arc/test/fake_arc_bridge_service.h:41: bool LaunchApp(const > std::string& > package,CHROME_BROWSER_UI_APP_LIST_TEST_FAKE_ARC_BRIDGE_SERVICE_FOR_APP_LAUNCHER_H_ > looks like an unintentional paste accident
On 2015/11/19 09:12:16, Matt Giuca wrote: > On 2015/11/13 14:43:53, khmel1 wrote: > > On 2015/11/13 07:39:54, Matt Giuca wrote: > > > I just skimmed this CL --- it is quite huge. Initial thoughts: > > > > Hi Matt, thank for taking a look into this. > > Sorry it has been a few days since I got back to you. > > I still have concerns about both a) the overall size of this CL, and b) > potential for code reuse versus duplication. > > > > > > - This CL is too long to review properly. I think it should be broken into > > > smaller chunks and reviewed separately. (That would also help get the number > > of > > > reviewers required for each CL down so we aren't talking across each other.) > > > > Yes, I agree with you that it looks big. I did not split into several CL > because > > there is another ARC++ CL in chromium: 1412863004. In this CL it was request > to > > merge CLs into one. My CL looks similar to it and I decided to keep everything > > in > > one. Asking reviewers, if you think that splitting this CL would help please > > let me know. > > I do think that would help. I realise this is in the late stages and has already > gotten reviews and LGs (so maybe this can just be a lesson for the future and > not necessarily applicable to this CL). Splitting CLs into smaller chunks is > usually a good thing -- if in doubt, split. If reviewers feel they need more > context, you can link them to the other CLs. > > Large CLs tend to be quadratic in complexity, as you need to pull in more > reviewers (this one has 8 reviewers) and they generally all need to review the > whole thing. That means more comments, more revisions (this is up to 13 patch > sets), and ironically it being generally more difficult for reviewers to see the > big picture. Most importantly, I think while Xiyuan, Elijah and Luis have done a > very detailed job, it is in general much easier for bugs to slip in in a CL of > this size. > > If I had seen https://codereview.chromium.org/1412863004 I would have also said > it was too big... but even there, that was only +835 lines. This is +2348 lines, > which is about three times as big. My rule of thumb is >400 lines is "too big". > Hi Matt, I agree with you 100%. I explained why this happened. Anyway I always open to do it better for reviewing. If you see anything I can do please let me know. > > > > > > > > - It looks like a lot of the code in app_list has been copy-pasted from the > > > existing extensions / apps code. (For example, ArcAppModelBuilder is a copy > of > > > ExtensionAppModelBuilder.) Although it may be a significant effort, I think > it > > > will be worth trying to reuse this existing code by refactoring / > subclassing. > > ArcAppModelBuilder looks similar to ExtensionAppModelBuilder only by > structure. > > I would mention that ::Initialize* are common. There is significant amount of > > code specific for extensions and for ARC. > > I agree, but I think these classes have more in common than different. I took a > more detailed look and I agree that the other classes are fairly unique. But > AAMB and EAMB have too much in common --- I definitely think you can reuse some > code. Make a common base class for them and put the common code there. Then put > the specific code in the subclasses. I have made specific notes where I think > code can be reused.
1. Nits addressed. 2. Moved arc_app_ to arc/ subfolder. 3. Refactored ExtensionAppModelBuilder and ArcAppModelBuilder to use base class AppListModelBuilder 4. During running unit_tests found rare crash (in Debug) on ArcAppModelBuilderTest.InstallIcon happened in case when tests are executed all together. Reason is not in this CL but I added workaround. Please see ArcAppIcon::DisableDecodingForTesting for more details. PTAL https://codereview.chromium.org/1413153007/diff/220001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc_app_list_prefs.cc (right): https://codereview.chromium.org/1413153007/diff/220001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_list_prefs.cc:317: for (auto& app_id : old_ready_apps) { On 2015/11/19 05:52:00, elijahtaylor (use chromium) wrote: > this is a little unexpected to me. Should apps get disabled if they existed > before and now don't exist in the refresh? Do we expect that to happen? I > would think this would signify an app removal more than anything else. > > The not-ready to ready state seems like it would be used for ARC not ready but > apps are in prefs, and then ARC is ready. (or now I see the bridge stopping > would disable apps too) There is possible scenario when user has appA and appB on deviceA and appB on deviceB. Now it login to deviceB and its profile contains information about appA and appB. So after refreshing apps we might detect that appB missing and may try to install it. Item is grayed until it is installed. Strictly speaking this should not be possible that sequenced refreshes contains different list (assume we have handler for install/uninstall). But as we see from interface, bridge may stopped and resumed. Who can guaranty that nothing happens on Android side at this moment? Theoretically it possible that some app may get unstalled without our attention. There are many edge cases and it is hard to forecast everything now. I think current behavior is not bed for this moment. WDYT? https://codereview.chromium.org/1413153007/diff/220001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc_app_model_builder.h (right): https://codereview.chromium.org/1413153007/diff/220001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_model_builder.h:25: class ArcAppModelBuilder : public ArcAppListPrefs::Observer { On 2015/11/19 09:12:35, Matt Giuca wrote: > I wanted to take another look over this CL to see if there were any duplications > of existing code. > > This class seems to be almost a complete duplicate of ExtensionAppModelBuilder. Hi Matt, Thank you for taking second round for this CL. I refactored as you recommended. https://codereview.chromium.org/1413153007/diff/220001/chrome/chrome_browser_... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/1413153007/diff/220001/chrome/chrome_browser_... chrome/chrome_browser_ui.gypi:2954: ['chromeos==1 and enable_arc == 1', { On 2015/11/19 05:52:00, elijahtaylor (use chromium) wrote: > nit: looks like the convention is enable_arc==1 Actually there are different styles currently in the code: for example: chrome/chrome_browser_chromeos.gypi uses enable_arc chrome/browser/ui/BUILD.gn uses both. components/components_tests.gyp uses both. and so on. most uses both defines. IMHO it is better to unify everything in separate CL. https://codereview.chromium.org/1413153007/diff/220001/components/arc/BUILD.gn File components/arc/BUILD.gn (right): https://codereview.chromium.org/1413153007/diff/220001/components/arc/BUILD.g... components/arc/BUILD.gn:21: static_library("fake_arc_bridge_service") { On 2015/11/19 03:13:49, xiyuan wrote: > nit: I'd call this "test_support", just in case we will add more stuff there > later and it is more consistent with other components. Done. https://codereview.chromium.org/1413153007/diff/220001/components/arc/common/... File components/arc/common/arc_instance_messages.h (right): https://codereview.chromium.org/1413153007/diff/220001/components/arc/common/... components/arc/common/arc_instance_messages.h:40: // It sends a request to ARC for the ARC app icon of a required scale factor. On 2015/11/19 05:52:00, elijahtaylor (use chromium) wrote: > nit: remove "It"? Done. https://codereview.chromium.org/1413153007/diff/220001/components/arc/test/fa... File components/arc/test/fake_arc_bridge_service.h (right): https://codereview.chromium.org/1413153007/diff/220001/components/arc/test/fa... components/arc/test/fake_arc_bridge_service.h:5: #ifndef COMPONENTS_ARC_TEST_FAKE_ARC_BRIDGE_SERVICE_H On 2015/11/19 03:13:49, xiyuan wrote: > nit: end it with "_", > i.e. > COMPONENTS_ARC_TEST_FAKE_ARC_BRIDGE_SERVICE_H_ Done. https://codereview.chromium.org/1413153007/diff/220001/components/arc/test/fa... components/arc/test/fake_arc_bridge_service.h:17: struct AppInfo { On 2015/11/19 05:52:00, elijahtaylor (use chromium) wrote: > I was a little confused by this at first because it shares a name with another > similar struct in the app list code. Maybe a different name to make this clear > it's just for testing purposes? Good point TestAppInfo sounds better https://codereview.chromium.org/1413153007/diff/220001/components/arc/test/fa... components/arc/test/fake_arc_bridge_service.h:41: bool LaunchApp(const std::string& package,CHROME_BROWSER_UI_APP_LIST_TEST_FAKE_ARC_BRIDGE_SERVICE_FOR_APP_LAUNCHER_H_ On 2015/11/19 05:52:00, elijahtaylor (use chromium) wrote: > looks like an unintentional paste accident Done. https://codereview.chromium.org/1413153007/diff/240001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc_app_icon.cc (right): https://codereview.chromium.org/1413153007/diff/240001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_icon.cc:5: #include "chrome/browser/ui/app_list/arc_app_icon.h" On 2015/11/19 09:12:36, Matt Giuca wrote: > This is a significant number of files. I think it would be prudent to move them > all into chrome/browser/ui/app_list/arc/, to keep them contained. The top-level > app_list directory already has enough stuff in it. Done. https://codereview.chromium.org/1413153007/diff/240001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc_app_model_builder.cc (right): https://codereview.chromium.org/1413153007/diff/240001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_model_builder.cc:20: void ArcAppModelBuilder::InitializeWithService( On 2015/11/19 09:12:36, Matt Giuca wrote: > This is almost the same as ExtensionAppModelBuilder::InitializeWithService. Done. https://codereview.chromium.org/1413153007/diff/240001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_model_builder.cc:32: app_list::AppListModel* model) { On 2015/11/19 09:12:36, Matt Giuca wrote: > This is almost the same as ExtensionAppModelBuilder::InitializeWithProfile. > > But it is missing model_->top_level_item_list()->AddObserver(this);. (Is it > meant to be missing that?) Yes, need more explanation here. There are 2 possible initialization, one with AppListSyncableService second with Profile. In case of Profile observer is set in order to be able to update positions manually after item moving. No need to do this in case of Service. Ideally we have to support both method. But if you take a look to ExtensionAppModelBuilder implementation you might see that it is based on extensions::AppSorting. This is only one available way to update positions manually. It is designed to work with extensions, contains extensions terms and closely integrated with other extension services. That means it is unusable for other app item types (before arc it was only one extension item it this was kind of ok). From other side, ChromeOS always uses initialization with Service so we may not implement this observer. Ideally, this scheme need to be refactored, but complexity of this is too high and too risky. I think this might be good dedicated task to do. https://codereview.chromium.org/1413153007/diff/240001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_model_builder.cc:58: ArcAppItem* ArcAppModelBuilder::GetArcAppItem(const std::string& item_id) { On 2015/11/19 09:12:36, Matt Giuca wrote: > This is almost the same as ExtensionAppModelBuilder::GetExtensionAppItem (even > with the same error). But it has a different return type. Even if the signature > has to be different, it could at least be abstracted into a GetAppItem protected > method of a common base class, with a public wrapper that does the static_cast. Refactored. BTW. Which error do you refer to? https://codereview.chromium.org/1413153007/diff/240001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_model_builder.cc:80: void ArcAppModelBuilder::InsertApp(scoped_ptr<ArcAppItem> app) { On 2015/11/19 09:12:36, Matt Giuca wrote: > This is exactly the same as ExtensionAppModelBuilder::InsertApp. Done.
https://codereview.chromium.org/1413153007/diff/220001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc_app_list_prefs.cc (right): https://codereview.chromium.org/1413153007/diff/220001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc_app_list_prefs.cc:317: for (auto& app_id : old_ready_apps) { On 2015/11/19 17:11:41, khmel1 wrote: > On 2015/11/19 05:52:00, elijahtaylor (use chromium) wrote: > > this is a little unexpected to me. Should apps get disabled if they existed > > before and now don't exist in the refresh? Do we expect that to happen? I > > would think this would signify an app removal more than anything else. > > > > The not-ready to ready state seems like it would be used for ARC not ready but > > apps are in prefs, and then ARC is ready. (or now I see the bridge stopping > > would disable apps too) > > There is possible scenario when user has appA and appB on deviceA and appB on > deviceB. Now it login to deviceB and its profile contains information about appA > and appB. So after refreshing apps we might detect that appB missing and may try > to install it. Item is grayed until it is installed. > Strictly speaking this should not be possible that sequenced refreshes contains > different list (assume we have handler for install/uninstall). But as we see > from interface, bridge may stopped and resumed. Who can guaranty that nothing > happens on Android side at this moment? Theoretically it possible that some app > may get unstalled without our attention. There are many edge cases and it is > hard to forecast everything now. I think current behavior is not bed for this > moment. WDYT? This is very nuanced and I think what you have is acceptable for now. It may become more clear what the behavior needs to be when we implement uninstall and sync. I'll file a bug for bengold to take a look since there are UX things here in the meantime. https://codereview.chromium.org/1413153007/diff/220001/chrome/chrome_browser_... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/1413153007/diff/220001/chrome/chrome_browser_... chrome/chrome_browser_ui.gypi:2954: ['chromeos==1 and enable_arc == 1', { On 2015/11/19 17:11:41, khmel1 wrote: > On 2015/11/19 05:52:00, elijahtaylor (use chromium) wrote: > > nit: looks like the convention is enable_arc==1 > > Actually there are different styles currently in the code: > for example: > > chrome/chrome_browser_chromeos.gypi uses enable_arc > chrome/browser/ui/BUILD.gn uses both. > components/components_tests.gyp uses both. > and so on. > > > most uses both defines. IMHO it is better to unify everything > in separate CL. Sorry I was being super nitty and meant "enable_arc==1" (no spaces) instead of "enable_arc == 1" (with spaces). I think checking chromeos and enable_arc is fine and matches a lot of other checks I've seen.
Description was changed from ========== arc-app-launcher: Minimal support for ARC app launcher. This CL comes along with: https://googleplex-android-review.git.corp.google.com/#/c/797530 https://googleplex-android-review.git.corp.google.com/795572 https://chrome-internal-review.googlesource.com/#/c/235195 This includes set of IPC messages to interact with android via ARC bridge service in chromeos. This provides extension to app_list model that manages arc apps. It includes preference based service that interacts with arc bridge and supports an arc app life cycle; app list model builder for arc apps; arc list item. 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=unit_tests with enable_arc=1 Debug/Release ========== to ========== arc-app-launcher: Minimal support for ARC app launcher. This CL comes along with: https://codereview.chromium.org/1475563002 (arc_bridge) https://codereview.chromium.org/1464353003 (fake_arc_bridge) This provides extension to app_list model that manages arc apps. It includes preference based service that interacts with arc bridge and supports an arc app life cycle; app list model builder for arc apps; arc list item. legacy ExtensionAppModelBuilder and new ArcAppModelBuilder are refactored in order to inherit common code. 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=unit_tests with enable_arc=1 Debug/Release ==========
After a discussion decision to split off this CL was made. This CL includes app_list related changes + unit_tests in order to keep Xiyuan's review here. All other CLs are listed in the CL description. PTAL https://codereview.chromium.org/1413153007/diff/220001/chrome/chrome_browser_... File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/1413153007/diff/220001/chrome/chrome_browser_... chrome/chrome_browser_ui.gypi:2954: ['chromeos==1 and enable_arc == 1', { On 2015/11/19 17:45:49, elijahtaylor (use chromium) wrote: > On 2015/11/19 17:11:41, khmel1 wrote: > > On 2015/11/19 05:52:00, elijahtaylor (use chromium) wrote: > > > nit: looks like the convention is enable_arc==1 > > > > Actually there are different styles currently in the code: > > for example: > > > > chrome/chrome_browser_chromeos.gypi uses enable_arc > > chrome/browser/ui/BUILD.gn uses both. > > components/components_tests.gyp uses both. > > and so on. > > > > > > most uses both defines. IMHO it is better to unify everything > > in separate CL. > > Sorry I was being super nitty and meant "enable_arc==1" (no spaces) instead of > "enable_arc == 1" (with spaces). I think checking chromeos and enable_arc is > fine and matches a lot of other checks I've seen. :) Done https://codereview.chromium.org/1413153007/diff/300001/chrome/common/pref_nam... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/1413153007/diff/300001/chrome/common/pref_nam... chrome/common/pref_names.cc:15: #if defined(ENABLE_ARC) Fix (OS_CHROMEOS) -> (ENABLE_ARC)
khmel@google.com changed reviewers: - dcheng@chromium.org, jochen@chromium.org, sadrul@chromium.org
khmel@google.com changed reviewers: + battre@chromium.org
chrome/browser/prefs/browser_prefs.cc LGTM
lpique@chromium.org changed reviewers: + lpique@chromium.org
https://codereview.chromium.org/1413153007/diff/340001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/1413153007/diff/340001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:111: bridge_service->AddObserver(this); Note: You also need to call bridge_service->AddAppsObserver(this);
https://codereview.chromium.org/1413153007/diff/340001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/1413153007/diff/340001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:111: bridge_service->AddObserver(this); On 2015/12/01 23:34:54, lpique wrote: > Note: You also need to call bridge_service->AddAppsObserver(this); Thanks for noticing this, yes, required CL is currently having small nit updates. I will update this CL once it is stabilized. Sorry for inconvenience
jochen@chromium.org changed reviewers: + jochen@chromium.org
please remove the reference to the buganizer entry https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/app_list_model_builder.cc (right): https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/app_list_model_builder.cc:18: if (!service_) { no {} for single line body https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/app_list_model_builder.cc:59: LOG_IF(ERROR, item && item->GetItemType() != item_type_) VLOG() or DVLOG(). why are we returning the item if it's wrong? https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/app_list_model_builder.h (right): https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/app_list_model_builder.h:22: explicit AppListModelBuilder(AppListControllerDelegate* controller, no explicit, can you clarify whether it takes ownership of controller, why is item_type not a std::string? https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/app_list_model_builder.h:26: // Initialize to use app-list sync and sets |service_| to |service|. this comment should tell whether it takes ownership of service and model https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/app_list_model_builder.h:30: // Initialize to use extension sync and sets |service_| to NULL. Used in nullptr https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_icon.cc (right): https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_icon.cc:30: struct ArcAppIcon::ReadResult { please add a ctor that takes args for and initializes all fields https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_icon.cc:69: if (host_) { no {} for single line bodies https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:64: DCHECK(base::DeleteFile(icon_path, false)); DCHECK() must not have side effects https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:256: LOG(ERROR) << "Name, package and activity cannot be empty."; please don't use LOG(ERROR) all over the place if these errors can happen, the code needs to react in a reasonable way. Users won't see those errors, they'll just see a broken product. for debugging, use VLOG/DVLOG https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.h (right): https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:40: AppInfo(); the ctor should take args for all fields and initialize them https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:49: public: needs virtual dtor https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_model_builder.h (right): https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_model_builder.h:46: ArcAppListPrefs* prefs_ = nullptr; please initialize in ctor https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/extension_app_model_builder.cc (right): https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/extension_app_model_builder.cc:1: // Copyright 2012 The Chromium Authors. All rights reserved. don't modify copyright headers
Description was changed from ========== arc-app-launcher: Minimal support for ARC app launcher. This CL comes along with: https://codereview.chromium.org/1475563002 (arc_bridge) https://codereview.chromium.org/1464353003 (fake_arc_bridge) This provides extension to app_list model that manages arc apps. It includes preference based service that interacts with arc bridge and supports an arc app life cycle; app list model builder for arc apps; arc list item. legacy ExtensionAppModelBuilder and new ArcAppModelBuilder are refactored in order to inherit common code. 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=unit_tests with enable_arc=1 Debug/Release ========== to ========== arc-app-launcher: Minimal support for ARC app launcher. This CL comes along with: https://codereview.chromium.org/1481523002 (split arc_bride_service) https://codereview.chromium.org/1475563002 (arc_bridge) This provides extension to app_list model that manages arc apps. It includes preference based service that interacts with arc bridge and supports an arc app life cycle; app list model builder for arc apps; arc list item. legacy ExtensionAppModelBuilder and new ArcAppModelBuilder are refactored in order to inherit common code. 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=unit_tests with enable_arc=1 Debug/Release ==========
Description was changed from ========== arc-app-launcher: Minimal support for ARC app launcher. This CL comes along with: https://codereview.chromium.org/1481523002 (split arc_bride_service) https://codereview.chromium.org/1475563002 (arc_bridge) This provides extension to app_list model that manages arc apps. It includes preference based service that interacts with arc bridge and supports an arc app life cycle; app list model builder for arc apps; arc list item. legacy ExtensionAppModelBuilder and new ArcAppModelBuilder are refactored in order to inherit common code. 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=unit_tests with enable_arc=1 Debug/Release ========== to ========== arc-app-launcher: Minimal support for ARC app launcher. This CL comes along with: https://codereview.chromium.org/1481523002 (split arc_bride_service) https://codereview.chromium.org/1475563002 (arc_bridge) This provides extension to app_list model that manages arc apps. It includes preference based service that interacts with arc bridge and supports an arc app life cycle; app list model builder for arc apps; arc list item. legacy ExtensionAppModelBuilder and new ArcAppModelBuilder are refactored in order to inherit common code. BUG=558209 TEST=Build and deploy android, chrome, chromium CLs. Apps appear in chromium app launcher. Once clicked, app is started in Android window. TEST=unit_tests with enable_arc=1 Debug/Release ==========
khmel@google.com changed reviewers: + hidehiko@chromium.org
Hi, Thanks Jochen for reviewing. I addressed your comments. All required CLs for this CL were landed and FakeArcBridgeService CL was merged to it as requested. Last updates contain rebasing and updating comments and nits. Hidehiko-san or Elijah, could you please transfer your LGTM for FakeArcBridgeService. PTAL https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/app_list_model_builder.cc (right): https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/app_list_model_builder.cc:18: if (!service_) { On 2015/12/03 10:25:39, jochen wrote: > no {} for single line body Done. https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/app_list_model_builder.cc:59: LOG_IF(ERROR, item && item->GetItemType() != item_type_) On 2015/12/03 10:25:39, jochen wrote: > VLOG() or DVLOG(). > > why are we returning the item if it's wrong? Good point. Return nullptr in this case https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/app_list_model_builder.h (right): https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/app_list_model_builder.h:22: explicit AppListModelBuilder(AppListControllerDelegate* controller, On 2015/12/03 10:25:39, jochen wrote: > no explicit, can you clarify whether it takes ownership of controller, why is > item_type not a std::string? Removed explicit (left from legacy code with one param). AppListItem::GetItemType returns const char*. Type is is declared for example: in header: class ExtensionAppItem ... static const char kItemType[]; in cc const char ExtensionAppItem::kItemType[] = "ExtensionAppItem"; This constant is returned at GetItemType Legacy code checks in many places like this: if (item->GetItemType() == ExtensionAppItem::kItemType) So, I just followed legacy code design. https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/app_list_model_builder.h:26: // Initialize to use app-list sync and sets |service_| to |service|. On 2015/12/03 10:25:39, jochen wrote: > this comment should tell whether it takes ownership of service and model Done. https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/app_list_model_builder.h:30: // Initialize to use extension sync and sets |service_| to NULL. Used in On 2015/12/03 10:25:39, jochen wrote: > nullptr Done. https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_icon.cc (right): https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_icon.cc:30: struct ArcAppIcon::ReadResult { On 2015/12/03 10:25:39, jochen wrote: > please add a ctor that takes args for and initializes all fields Done. https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_icon.cc:69: if (host_) { On 2015/12/03 10:25:39, jochen wrote: > no {} for single line bodies Done. https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc (right): https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:64: DCHECK(base::DeleteFile(icon_path, false)); On 2015/12/03 10:25:39, jochen wrote: > DCHECK() must not have side effects Done. https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc:256: LOG(ERROR) << "Name, package and activity cannot be empty."; On 2015/12/03 10:25:39, jochen wrote: > please don't use LOG(ERROR) all over the place > > if these errors can happen, the code needs to react in a reasonable way. Users > won't see those errors, they'll just see a broken product. > > for debugging, use VLOG/DVLOG Done. https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_list_prefs.h (right): https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:40: AppInfo(); On 2015/12/03 10:25:39, jochen wrote: > the ctor should take args for all fields and initialize them Done. https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_list_prefs.h:49: public: On 2015/12/03 10:25:40, jochen wrote: > needs virtual dtor Done. https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_model_builder.h (right): https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_model_builder.h:46: ArcAppListPrefs* prefs_ = nullptr; On 2015/12/03 10:25:40, jochen wrote: > please initialize in ctor I was asked by Xiyuan to use this style. I followed it in all places. https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/extension_app_model_builder.cc (right): https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/extension_app_model_builder.cc:1: // Copyright 2012 The Chromium Authors. All rights reserved. On 2015/12/03 10:25:40, jochen wrote: > don't modify copyright headers Done.
https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_model_builder.h (right): https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_model_builder.h:46: ArcAppListPrefs* prefs_ = nullptr; On 2015/12/03 at 12:34:49, khmel1 wrote: > On 2015/12/03 10:25:40, jochen wrote: > > please initialize in ctor > > I was asked by Xiyuan to use this style. I followed it in all places. the style guide doesn't say anything on this, but it's uncommon in chromium, where we try to move as much out of the headers as possible: https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Minimi... https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/app_list_model_builder.h (right): https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... chrome/browser/ui/app_list/app_list_model_builder.h:64: AppListControllerDelegate* controller_; ^^^ you also don't use it consistently https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_icon.cc (right): https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_icon.cc:25: } } // namespace https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_icon.cc:120: return; no {} for single line bodies, here and everywhere https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_icon.cc:129: return; don't you need to discard the request here as well? also, won't this have a user-visible impact? will the host just hang there and wait for the icon forever? https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_icon.cc:179: VLOG(2) << "ARC preferences service is not available."; how can this ever happen? https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_item.cc (right): https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_item.cc:89: << ". Bridge service is not ready."; what will the user see here?
LGTM for components/arc. Just a walk through for all files in CL, and commented some style-/coding- related stuff (but not design's). https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_icon.cc (right): https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_icon.cc:31: enum Status { enum class ? https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_icon.cc:66: : host_(host) { 4 indent. https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_icon.cc:108: : host_(host), Ditto. https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_icon.cc:216: scoped_ptr<ArcAppIcon::ReadResult> result(new ArcAppIcon::ReadResult( I think it's better to inline to the return statement. return new ArcAppIcon::ReadResult(...::OK, scale_factor); etc. for each return statement below. https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_item.h (right): https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_item.h:17: } // namespace https://codereview.chromium.org/1413153007/diff/380001/components/arc/test/fa... File components/arc/test/fake_arc_bridge_service.cc (right): https://codereview.chromium.org/1413153007/diff/380001/components/arc/test/fa... components/arc/test/fake_arc_bridge_service.cc:117: base::FilePath icon_file_path = base_path.AppendASCII("components") nit: Maybe, line break just before '.' operator to align? https://codereview.chromium.org/1413153007/diff/380001/components/arc/test/fa... components/arc/test/fake_arc_bridge_service.cc:126: png_data_as_string->c_str(), nit: std::vector<uint8_t> png_data( png_data_as_string->begin(), png_data_as_string->end()); should work? https://codereview.chromium.org/1413153007/diff/380001/components/arc/test/fa... File components/arc/test/fake_arc_bridge_service.h (right): https://codereview.chromium.org/1413153007/diff/380001/components/arc/test/fa... components/arc/test/fake_arc_bridge_service.h:41: DISALLOW_COPY_AND_ASSIGN(Request); (optional) Just a note. IMHO, it's ok to make this copy-able, and use std::vector<Request> below. Though, it's up to you.
https://codereview.chromium.org/1413153007/diff/380001/components/arc/test/fa... File components/arc/test/fake_arc_bridge_service.h (right): https://codereview.chromium.org/1413153007/diff/380001/components/arc/test/fa... components/arc/test/fake_arc_bridge_service.h:41: DISALLOW_COPY_AND_ASSIGN(Request); On 2015/12/03 at 13:08:39, hidehiko wrote: > (optional) Just a note. > IMHO, it's ok to make this copy-able, and use std::vector<Request> below. Though, it's up to you. then it needs to be a struct
Removed inits from headers and few more nits https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_model_builder.h (right): https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_model_builder.h:46: ArcAppListPrefs* prefs_ = nullptr; On 2015/12/03 12:55:51, jochen wrote: > On 2015/12/03 at 12:34:49, khmel1 wrote: > > On 2015/12/03 10:25:40, jochen wrote: > > > please initialize in ctor > > > > I was asked by Xiyuan to use this style. I followed it in all places. > > the style guide doesn't say anything on this, but it's uncommon in chromium, > where we try to move as much out of the headers as possible: > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Minimi... Ok, thanks for pointing, I removed this style in my code. https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/app_list_model_builder.h (right): https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... chrome/browser/ui/app_list/app_list_model_builder.h:64: AppListControllerDelegate* controller_; On 2015/12/03 12:55:51, jochen wrote: > ^^^ you also don't use it consistently I removed initializing in header so this no longer relevant. https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_icon.cc (right): https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_icon.cc:25: } On 2015/12/03 12:55:52, jochen wrote: > } // namespace Done. https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_icon.cc:31: enum Status { On 2015/12/03 13:08:39, hidehiko wrote: > enum class ? Done. https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_icon.cc:66: : host_(host) { On 2015/12/03 13:08:39, hidehiko wrote: > 4 indent. Done. https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_icon.cc:108: : host_(host), On 2015/12/03 13:08:39, hidehiko wrote: > Ditto. Done. https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_icon.cc:120: return; On 2015/12/03 12:55:52, jochen wrote: > no {} for single line bodies, here and everywhere Done. https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_icon.cc:129: return; On 2015/12/03 12:55:51, jochen wrote: > don't you need to discard the request here as well? > > also, won't this have a user-visible impact? will the host just hang there and > wait for the icon forever? Yes, thanks for catching. In case something wrong with icon, user will see just default app icon. No hang in this case. In my tests this use case happened. https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_icon.cc:179: VLOG(2) << "ARC preferences service is not available."; On 2015/12/03 12:55:52, jochen wrote: > how can this ever happen? It is impossible. I removed check here for consistency (other code does not check) https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_icon.cc:216: scoped_ptr<ArcAppIcon::ReadResult> result(new ArcAppIcon::ReadResult( On 2015/12/03 13:08:39, hidehiko wrote: > I think it's better to inline to the return statement. > > return new ArcAppIcon::ReadResult(...::OK, scale_factor); > > etc. for each return statement below. Good point, thanks https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_item.cc (right): https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_item.cc:89: << ". Bridge service is not ready."; On 2015/12/03 12:55:52, jochen wrote: > what will the user see here? ARC app has not started and all ARC app icon at app launcher should be grayed. https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_item.h (right): https://codereview.chromium.org/1413153007/diff/380001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_item.h:17: } On 2015/12/03 13:08:39, hidehiko wrote: > // namespace Done. https://codereview.chromium.org/1413153007/diff/380001/components/arc/test/fa... File components/arc/test/fake_arc_bridge_service.cc (right): https://codereview.chromium.org/1413153007/diff/380001/components/arc/test/fa... components/arc/test/fake_arc_bridge_service.cc:117: base::FilePath icon_file_path = base_path.AppendASCII("components") On 2015/12/03 13:08:39, hidehiko wrote: > nit: Maybe, line break just before '.' operator to align? Done. https://codereview.chromium.org/1413153007/diff/380001/components/arc/test/fa... components/arc/test/fake_arc_bridge_service.cc:126: png_data_as_string->c_str(), On 2015/12/03 13:08:39, hidehiko wrote: > nit: > std::vector<uint8_t> png_data( > png_data_as_string->begin(), png_data_as_string->end()); > > should work? Yes, nicer https://codereview.chromium.org/1413153007/diff/380001/components/arc/test/fa... File components/arc/test/fake_arc_bridge_service.h (right): https://codereview.chromium.org/1413153007/diff/380001/components/arc/test/fa... components/arc/test/fake_arc_bridge_service.h:41: DISALLOW_COPY_AND_ASSIGN(Request); On 2015/12/03 13:12:46, jochen wrote: > On 2015/12/03 at 13:08:39, hidehiko wrote: > > (optional) Just a note. > > IMHO, it's ok to make this copy-able, and use std::vector<Request> below. > Though, it's up to you. > > then it needs to be a struct Thanks for explanation. My concern was that that Request was too heavy to be a struct and made it as class.
https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_model_builder.h (right): https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_model_builder.h:46: ArcAppListPrefs* prefs_ = nullptr; On 2015/12/03 14:12:41, khmel1 wrote: > On 2015/12/03 12:55:51, jochen wrote: > > On 2015/12/03 at 12:34:49, khmel1 wrote: > > > On 2015/12/03 10:25:40, jochen wrote: > > > > please initialize in ctor > > > > > > I was asked by Xiyuan to use this style. I followed it in all places. > > > > the style guide doesn't say anything on this, but it's uncommon in chromium, > > where we try to move as much out of the headers as possible: > > > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Minimi... > > Ok, thanks for pointing, I removed this style in my code. This is allowed in chromium. https://chromium-cpp.appspot.com/, check out the "Non-Static Class Member Initializers" row. My reviewer on the other CL prefers this style.
I have everything ready to try to submit this CL. If you don't have any comments I will proceed to commit soon. Thanks https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... File chrome/browser/ui/app_list/arc/arc_app_model_builder.h (right): https://codereview.chromium.org/1413153007/diff/360001/chrome/browser/ui/app_... chrome/browser/ui/app_list/arc/arc_app_model_builder.h:46: ArcAppListPrefs* prefs_ = nullptr; On 2015/12/03 17:58:43, xiyuan wrote: > On 2015/12/03 14:12:41, khmel1 wrote: > > On 2015/12/03 12:55:51, jochen wrote: > > > On 2015/12/03 at 12:34:49, khmel1 wrote: > > > > On 2015/12/03 10:25:40, jochen wrote: > > > > > please initialize in ctor > > > > > > > > I was asked by Xiyuan to use this style. I followed it in all places. > > > > > > the style guide doesn't say anything on this, but it's uncommon in chromium, > > > where we try to move as much out of the headers as possible: > > > > > > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Minimi... > > > > Ok, thanks for pointing, I removed this style in my code. > > This is allowed in chromium. > https://chromium-cpp.appspot.com/, check out the "Non-Static Class Member > Initializers" row. > > My reviewer on the other CL prefers this style. Thanks Xiyuan for the link, yes, they are officially allowed. Moreover, I searched over the chromium classes using regex expression '\_\s\=\snullptr\;\n' for example and found that this construction is widely used already. From this point of view I restored my code.
Hi, sorry I missed this review last week. Thanks for doing the refactoring I suggested. > Refactored ExtensionAppModelBuilder and ArcAppModelBuilder to use base class AppListModelBuilder This is good but really it is *precisely* the sort of thing that should be split out into a separate CL. In general, if you identify a refactor that supports some work you're doing, do the refactor in a CL by itself. This means that a) the CL you are trying to land won't be further bloated by the refactor, and b) you can land the refactor and make sure it didn't break any existing functionality, independent of the other code you're working on. If it is too late in this CL's work-flow to break it up, I would at least like you to observe this principle in the future. Big CLs = more complicated code reviews = less scrutiny and more surface for things to go wrong, harder to revert if they break stuff, etc. > This CL comes along with: > https://codereview.chromium.org/1481523002 (split arc_bride_service) > https://codereview.chromium.org/1475563002 (arc_bridge) CLs don't "come along with" other CLs. The other CLs have already landed, so you don't need to mention it in the description. (If you want to let reviewers know of dependent CLs before they land, just paste this sort of information in the review email.)
Description was changed from ========== arc-app-launcher: Minimal support for ARC app launcher. This CL comes along with: https://codereview.chromium.org/1481523002 (split arc_bride_service) https://codereview.chromium.org/1475563002 (arc_bridge) This provides extension to app_list model that manages arc apps. It includes preference based service that interacts with arc bridge and supports an arc app life cycle; app list model builder for arc apps; arc list item. legacy ExtensionAppModelBuilder and new ArcAppModelBuilder are refactored in order to inherit common code. BUG=558209 TEST=Build and deploy android, chrome, chromium CLs. Apps appear in chromium app launcher. Once clicked, app is started in Android window. TEST=unit_tests with enable_arc=1 Debug/Release ========== to ========== arc-app-launcher: Minimal support for ARC app launcher. This provides extension to app_list model that manages arc apps. It includes preference based service that interacts with arc bridge and supports an arc app life cycle; app list model builder for arc apps; arc list item. legacy ExtensionAppModelBuilder and new ArcAppModelBuilder are refactored in order to inherit common code. BUG=558209 TEST=Build and deploy android, chrome, chromium CLs. Apps appear in chromium app launcher. Once clicked, app is started in Android window. TEST=unit_tests with enable_arc=1 Debug/Release ==========
On 2015/12/04 03:03:26, Matt Giuca wrote: > Hi, sorry I missed this review last week. > > Thanks for doing the refactoring I suggested. > > > Refactored ExtensionAppModelBuilder and ArcAppModelBuilder to use base class > AppListModelBuilder > > This is good but really it is *precisely* the sort of thing that should be split > out into a separate CL. > > In general, if you identify a refactor that supports some work you're doing, do > the refactor in a CL by itself. This means that a) the CL you are trying to land > won't be further bloated by the refactor, and b) you can land the refactor and > make sure it didn't break any existing functionality, independent of the other > code you're working on. > > If it is too late in this CL's work-flow to break it up, I would at least like > you to observe this principle in the future. Big CLs = more complicated code > reviews = less scrutiny and more surface for things to go wrong, harder to > revert if they break stuff, etc. > > > This CL comes along with: > > https://codereview.chromium.org/1481523002 (split arc_bride_service) > > https://codereview.chromium.org/1475563002 (arc_bridge) > > CLs don't "come along with" other CLs. The other CLs have already landed, so you > don't need to mention it in the description. > > (If you want to let reviewers know of dependent CLs before they land, just paste > this sort of information in the review email.) Thanks Matt for you comments! I have WIP for further ARC integration into app_list and to Shelf and so far I see several places for refactoring and I will definitely follow your suggestions. Thanks
The CQ bit was checked by khmel@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, elijahtaylor@google.com, battre@chromium.org, khmel@google.com, hidehiko@chromium.org Link to the patchset: https://codereview.chromium.org/1413153007/#ps420001 (title: "Restored non static class memeber initialization")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413153007/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413153007/420001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
ArcBridgeService was recently updated and I added missing function impl to FakeArcBridgeService + reabased.
The CQ bit was checked by khmel@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from khmel@google.com, xiyuan@chromium.org, elijahtaylor@google.com, hidehiko@chromium.org, battre@chromium.org Link to the patchset: https://codereview.chromium.org/1413153007/#ps440001 (title: "add missing method to fake_arc_bridge_service")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413153007/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413153007/440001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I added missed links to BUILD.gn files, required for gn gen builds. Jochen, as I understand correctly, you are the owner of chrome/browser/ui/BUILD.gn and chrome/test/BUILD.gn. PTAL
build gn files lgtm
On 2015/12/04 14:09:49, jochen wrote: > build gn files l/g/t/m Thanks for quick approval!
The CQ bit was checked by khmel@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from khmel@google.com, xiyuan@chromium.org, elijahtaylor@google.com, hidehiko@chromium.org, battre@chromium.org Link to the patchset: https://codereview.chromium.org/1413153007/#ps460001 (title: "Added missing links to BUILD.gn")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413153007/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413153007/460001
Message was sent while issue was closed.
Description was changed from
==========
arc-app-launcher: Minimal support for ARC app launcher.
This provides extension to app_list model that manages arc
apps. It includes preference based service that interacts
with arc bridge and supports an arc app life cycle; app list
model builder for arc apps; arc list item. legacy
ExtensionAppModelBuilder and new ArcAppModelBuilder are
refactored in order to inherit common code.
BUG=558209
TEST=Build and deploy android, chrome, chromium CLs. Apps
appear in chromium app launcher. Once clicked, app
is started in Android window.
TEST=unit_tests with enable_arc=1 Debug/Release
==========
to
==========
arc-app-launcher: Minimal support for ARC app launcher.
This provides extension to app_list model that manages arc
apps. It includes preference based service that interacts
with arc bridge and supports an arc app life cycle; app list
model builder for arc apps; arc list item. legacy
ExtensionAppModelBuilder and new ArcAppModelBuilder are
refactored in order to inherit common code.
BUG=558209
TEST=Build and deploy android, chrome, chromium CLs. Apps
appear in chromium app launcher. Once clicked, app
is started in Android window.
TEST=unit_tests with enable_arc=1 Debug/Release
==========
Message was sent while issue was closed.
Committed patchset #24 (id:460001)
Message was sent while issue was closed.
Description was changed from
==========
arc-app-launcher: Minimal support for ARC app launcher.
This provides extension to app_list model that manages arc
apps. It includes preference based service that interacts
with arc bridge and supports an arc app life cycle; app list
model builder for arc apps; arc list item. legacy
ExtensionAppModelBuilder and new ArcAppModelBuilder are
refactored in order to inherit common code.
BUG=558209
TEST=Build and deploy android, chrome, chromium CLs. Apps
appear in chromium app launcher. Once clicked, app
is started in Android window.
TEST=unit_tests with enable_arc=1 Debug/Release
==========
to
==========
arc-app-launcher: Minimal support for ARC app launcher.
This provides extension to app_list model that manages arc
apps. It includes preference based service that interacts
with arc bridge and supports an arc app life cycle; app list
model builder for arc apps; arc list item. legacy
ExtensionAppModelBuilder and new ArcAppModelBuilder are
refactored in order to inherit common code.
BUG=558209
TEST=Build and deploy android, chrome, chromium CLs. Apps
appear in chromium app launcher. Once clicked, app
is started in Android window.
TEST=unit_tests with enable_arc=1 Debug/Release
Committed: https://crrev.com/d7c5a3215244c5aa2165b6284bc8d19d5792af8c
Cr-Commit-Position: refs/heads/master@{#363220}
==========
Message was sent while issue was closed.
Patchset 24 (id:??) landed as https://crrev.com/d7c5a3215244c5aa2165b6284bc8d19d5792af8c Cr-Commit-Position: refs/heads/master@{#363220} |
