Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2272813003:
Add ARC++ specific fields to launch data for specific apps. (Closed)
|
Created:
4 years, 4 months ago by rkc Modified:
4 years, 3 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, extensions-reviews_chromium.org, yusukes+watch_chromium.org, tfarina, hidehiko+watch_chromium.org, dzhioev+watch_chromium.org, achuith+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd ARC++ specific fields to launch data for specific apps.
On devices that have ARC++ available or enabled, certain internal apps need
to be able to get this information. Currently these are limited to the
GetHelp and the demo app.
These are fields we'd like to eventually just expose to all apps but currently
security is not comfortable with that yet, so for now, we'll set these flags
only for the apps that currently need it.
Reviews requested,
xiyuan@ - General review.
asargent@ - //apps and extension parts
lhchavez@ - ARC++ parts
sky@ - //chrome/browser/ui
R=asargent@chromium.org, lhchavez@chromium.org, sky@chromium.org, xiyuan@chromium.org
BUG=640330
Committed: https://crrev.com/3d97de469a486debcb8956de8f7ed679fa862cbf
Committed: https://crrev.com/79bf63cbe7a00345f152d0dbb3dc20e24442713f
Cr-Original-Commit-Position: refs/heads/master@{#414348}
Cr-Commit-Position: refs/heads/master@{#414535}
Patch Set 1 #Patch Set 2 : . #
Total comments: 12
Patch Set 3 : . #
Total comments: 6
Patch Set 4 : . #Patch Set 5 : . #
Total comments: 4
Patch Set 6 : win build fix #Patch Set 7 : crash fix #Patch Set 8 : chrome build fix. #Messages
Total messages: 44 (13 generated)
I'm still thinking about how to test this. One place seemed like the demo app launcher browsertest, but that test will require quite a bit of work to be able to hook in the ability to test this. The current test does nothing but verify that the demo app was brought up. I'll work on a test in a separate CL. Since this needs to go into 54, I don't want to risk blocking this.
https://codereview.chromium.org/2272813003/diff/20001/components/arc/arc_brid... File components/arc/arc_bridge_service.cc (right): https://codereview.chromium.org/2272813003/diff/20001/components/arc/arc_brid... components/arc/arc_bridge_service.cc:58: // https://codereview.chromium.org/2266123002/ has landed. This has landed :)
https://codereview.chromium.org/2272813003/diff/20001/components/arc/arc_brid... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/2272813003/diff/20001/components/arc/arc_brid... components/arc/arc_bridge_service.h:71: static bool IsArcAvailable(); Also, for testing you might want to pass in the base::CommandLine* like for GetEnabled(). That way you can launch the test several times with different flags.
https://codereview.chromium.org/2272813003/diff/20001/chrome/browser/ui/exten... File chrome/browser/ui/extensions/app_launch_params.h (right): https://codereview.chromium.org/2272813003/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/app_launch_params.h:71: PlayStoreStatus play_store_status; Isn't arc chromeos specific? In other words, shouldn't all the arc structs be behind #if defined(OS_CHROMEOS)?
https://codereview.chromium.org/2272813003/diff/20001/chrome/browser/ui/exten... File chrome/browser/ui/extensions/app_launch_params.h (right): https://codereview.chromium.org/2272813003/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/app_launch_params.h:71: PlayStoreStatus play_store_status; On 2016/08/24 16:20:39, sky wrote: > Isn't arc chromeos specific? In other words, shouldn't all the arc structs be > behind #if defined(OS_CHROMEOS)? Whoops - yep. Put all of that code behind #ifdefs and compiled on multiple platforms to confirm everything works. I've left this field and other related code outside ifdefs since (a) we can't not have this field in the IDL (we can't have platform specific structures/enums) and (b) putting this behind an ifdef would introduce ifdefs all over the code, which would result in much ugliness. https://codereview.chromium.org/2272813003/diff/20001/components/arc/arc_brid... File components/arc/arc_bridge_service.cc (right): https://codereview.chromium.org/2272813003/diff/20001/components/arc/arc_brid... components/arc/arc_bridge_service.cc:58: // https://codereview.chromium.org/2266123002/ has landed. On 2016/08/24 15:06:24, Luis Héctor Chávez wrote: > This has landed :) Acknowledged. https://codereview.chromium.org/2272813003/diff/20001/components/arc/arc_brid... File components/arc/arc_bridge_service.h (right): https://codereview.chromium.org/2272813003/diff/20001/components/arc/arc_brid... components/arc/arc_bridge_service.h:71: static bool IsArcAvailable(); On 2016/08/24 15:10:31, Luis Héctor Chávez wrote: > Also, for testing you might want to pass in the base::CommandLine* like for > GetEnabled(). That way you can launch the test several times with different > flags. Done.
components/arc lgtm
https://codereview.chromium.org/2272813003/diff/20001/chrome/browser/ui/exten... File chrome/browser/ui/extensions/app_launch_params.h (right): https://codereview.chromium.org/2272813003/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/app_launch_params.h:33: bool set_playstore_status = false); Document what set_playstore_status means. https://codereview.chromium.org/2272813003/diff/20001/chrome/browser/ui/exten... chrome/browser/ui/extensions/app_launch_params.h:71: PlayStoreStatus play_store_status; On 2016/08/24 20:02:36, rkc wrote: > On 2016/08/24 16:20:39, sky wrote: > > Isn't arc chromeos specific? In other words, shouldn't all the arc structs be > > behind #if defined(OS_CHROMEOS)? > > Whoops - yep. Put all of that code behind #ifdefs and compiled on multiple > platforms to confirm everything works. > > I've left this field and other related code outside ifdefs since (a) we can't > not have this field in the IDL (we can't have platform specific > structures/enums) and (b) putting this behind an ifdef would introduce ifdefs > all over the code, which would result in much ugliness. As long as this doesn't result in pulling in any arc specific headers, ok. https://codereview.chromium.org/2272813003/diff/40001/chrome/browser/ui/exten... File chrome/browser/ui/extensions/app_launch_params.h (right): https://codereview.chromium.org/2272813003/diff/40001/chrome/browser/ui/exten... chrome/browser/ui/extensions/app_launch_params.h:19: using extensions::api::app_runtime::PlayStoreStatus; Style guide says no using like this in header (otherwise everyone picks it up, whether they want it or not).
https://codereview.chromium.org/2272813003/diff/40001/chrome/browser/ui/exten... File chrome/browser/ui/extensions/app_launch_params.h (right): https://codereview.chromium.org/2272813003/diff/40001/chrome/browser/ui/exten... chrome/browser/ui/extensions/app_launch_params.h:19: using extensions::api::app_runtime::PlayStoreStatus; On 2016/08/24 20:34:23, sky wrote: > Style guide says no using like this in header (otherwise everyone picks it up, > whether they want it or not). Done.
https://codereview.chromium.org/2272813003/diff/20001/apps/launcher.cc File apps/launcher.cc (right): https://codereview.chromium.org/2272813003/diff/20001/apps/launcher.cc#newcod... apps/launcher.cc:398: launch_data->play_store_status = play_store_status; If this line is skipped (i.e. play_store_status == UNKNOWN), what value would launch_data carry? Optional enum field is tricky. It probably does not have unique_ptr for it so must carry some value. https://codereview.chromium.org/2272813003/diff/20001/apps/launcher.h File apps/launcher.h (right): https://codereview.chromium.org/2272813003/diff/20001/apps/launcher.h#newcode15 apps/launcher.h:15: using extensions::api::app_runtime::PlayStoreStatus; Just checking, is "using" allowed in header file?
LGTM
https://codereview.chromium.org/2272813003/diff/20001/apps/launcher.cc File apps/launcher.cc (right): https://codereview.chromium.org/2272813003/diff/20001/apps/launcher.cc#newcod... apps/launcher.cc:398: launch_data->play_store_status = play_store_status; On 2016/08/24 20:50:31, xiyuan wrote: > If this line is skipped (i.e. play_store_status == UNKNOWN), what value would > launch_data carry? Optional enum field is tricky. It probably does not have > unique_ptr for it so must carry some value. It ends up being 'undefined' in the app. I tested to make sure. https://codereview.chromium.org/2272813003/diff/20001/apps/launcher.h File apps/launcher.h (right): https://codereview.chromium.org/2272813003/diff/20001/apps/launcher.h#newcode15 apps/launcher.h:15: using extensions::api::app_runtime::PlayStoreStatus; On 2016/08/24 20:50:31, xiyuan wrote: > Just checking, is "using" allowed in header file? Nope, fixed. Done.
lgtm
https://codereview.chromium.org/2272813003/diff/40001/apps/launcher.cc File apps/launcher.cc (right): https://codereview.chromium.org/2272813003/diff/40001/apps/launcher.cc#newcod... apps/launcher.cc:212: profile_, app, launch_source_, std::move(launch_data)); can you just use std::move(action_data_) here, and skip temporarily putting it into |launch_data| only to move it right back out again? https://codereview.chromium.org/2272813003/diff/40001/apps/launcher.cc#newcod... apps/launcher.cc:396: base::MakeUnique<app_runtime::LaunchData>(); Not that it matters much here for performance since this code isn't run much, but I think we really don't need to create an empty app_runtime::LaunchData object here - we can just leave it as null, since the LaunchData::ToValue just skips the action_data field if it is null. https://codereview.chromium.org/2272813003/diff/80001/extensions/common/api/a... File extensions/common/api/app_runtime.idl (right): https://codereview.chromium.org/2272813003/diff/80001/extensions/common/api/a... extensions/common/api/app_runtime.idl:105: [nodoc] PlayStoreStatus? playStoreStatus; Is this status expected to change often? It's a little unusual to have something that seems like a fairly static bit of device information that most apps don't care about passed on every single app launch, as opposed to say having a separate method for fetching that state for apps that care (and possibly an event for finding out when it changes).
https://codereview.chromium.org/2272813003/diff/40001/apps/launcher.cc File apps/launcher.cc (right): https://codereview.chromium.org/2272813003/diff/40001/apps/launcher.cc#newcod... apps/launcher.cc:212: profile_, app, launch_source_, std::move(launch_data)); On 2016/08/24 21:50:32, Antony Sargent wrote: > can you just use std::move(action_data_) here, and skip temporarily putting it > into |launch_data| only to move it right back out again? Nevermind - I was confusing launch_data with action_data
https://codereview.chromium.org/2272813003/diff/40001/apps/launcher.cc File apps/launcher.cc (right): https://codereview.chromium.org/2272813003/diff/40001/apps/launcher.cc#newcod... apps/launcher.cc:396: base::MakeUnique<app_runtime::LaunchData>(); On 2016/08/24 21:50:32, Antony Sargent wrote: > Not that it matters much here for performance since this code isn't run much, > but I think we really don't need to create an empty app_runtime::LaunchData > object here - we can just leave it as null, since the LaunchData::ToValue just > skips the action_data field if it is null. We're passing launch_data, which in the caller will be called with ->ToValue(). So passing in a nullptr would crash there. We could add in null checks, but then we'd have to add null checks in all the way down to the actual event dispatch, negating any performance benefit we could get. https://codereview.chromium.org/2272813003/diff/80001/extensions/common/api/a... File extensions/common/api/app_runtime.idl (right): https://codereview.chromium.org/2272813003/diff/80001/extensions/common/api/a... extensions/common/api/app_runtime.idl:105: [nodoc] PlayStoreStatus? playStoreStatus; On 2016/08/24 21:50:32, Antony Sargent wrote: > Is this status expected to change often? It's a little unusual to have something > that seems like a fairly static bit of device information that most apps don't > care about passed on every single app launch, as opposed to say having a > separate method for fetching that state for apps that care (and possibly an > event for finding out when it changes). This won't be set for all apps at the moment. Eventually knowing the kind of device you're running on is useful information for an app to know when it starts up. The same was as the launch source, is a public or kiosk session, etc. The primary argument against not plugging this into an API that an app that cares calls versus us providing it to an app we care about was whitelisting. If an app calls back into an API to get this info, considering that we cannot expose this to all apps, we have to whitelist that app for that API call. This puts into the same cycle of needing to whitelist apps everytime a new one comes along, the id changes, etc. This way, 'whatever' demo app we launch (which later could be a vendor provided one), this will always work and the app will always know whether playstore is available or not on that device.
https://codereview.chromium.org/2272813003/diff/80001/extensions/common/api/a... File extensions/common/api/app_runtime.idl (right): https://codereview.chromium.org/2272813003/diff/80001/extensions/common/api/a... extensions/common/api/app_runtime.idl:105: [nodoc] PlayStoreStatus? playStoreStatus; On 2016/08/24 22:05:57, rkc wrote: > On 2016/08/24 21:50:32, Antony Sargent wrote: > > Is this status expected to change often? It's a little unusual to have > something > > that seems like a fairly static bit of device information that most apps don't > > care about passed on every single app launch, as opposed to say having a > > separate method for fetching that state for apps that care (and possibly an > > event for finding out when it changes). > > This won't be set for all apps at the moment. Eventually knowing the kind of > device you're running on is useful information for an app to know when it starts > up. The same was as the launch source, is a public or kiosk session, etc. On a given device, an app can either be launched in a public session or not, or in a kiosk session or not. Can the PlayStoreStatus change similarly? > The primary argument against not plugging this into an API that an app that > cares calls versus us providing it to an app we care about was whitelisting. > > If an app calls back into an API to get this info, considering that we cannot > expose this to all apps, we have to whitelist that app for that API call. This > puts into the same cycle of needing to whitelist apps everytime a new one comes > along, the id changes, etc. > > > This way, 'whatever' demo app we launch (which later could be a vendor provided > one), this will always work and the app will always know whether playstore is > available or not on that device. I'm not sure I understand your argument about whitelisting- wouldn't we either need a whitelist or not regardless of *how* we provide this information? If we provide it as a property of LaunchData, we either provide the value to all apps or use some kind of whitelist to decide which apps see a non-null (or fake?) value.
https://codereview.chromium.org/2272813003/diff/80001/extensions/common/api/a... File extensions/common/api/app_runtime.idl (right): https://codereview.chromium.org/2272813003/diff/80001/extensions/common/api/a... extensions/common/api/app_runtime.idl:105: [nodoc] PlayStoreStatus? playStoreStatus; On 2016/08/24 22:37:19, Antony Sargent wrote: > On 2016/08/24 22:05:57, rkc wrote: > > On 2016/08/24 21:50:32, Antony Sargent wrote: > > > Is this status expected to change often? It's a little unusual to have > > something > > > that seems like a fairly static bit of device information that most apps > don't > > > care about passed on every single app launch, as opposed to say having a > > > separate method for fetching that state for apps that care (and possibly an > > > event for finding out when it changes). > > > > This won't be set for all apps at the moment. Eventually knowing the kind of > > device you're running on is useful information for an app to know when it > starts > > up. The same was as the launch source, is a public or kiosk session, etc. > > On a given device, an app can either be launched in a public session or not, or > in a kiosk session or not. Can the PlayStoreStatus change similarly? > > > The primary argument against not plugging this into an API that an app that > > cares calls versus us providing it to an app we care about was whitelisting. > > > > If an app calls back into an API to get this info, considering that we cannot > > expose this to all apps, we have to whitelist that app for that API call. This > > puts into the same cycle of needing to whitelist apps everytime a new one > comes > > along, the id changes, etc. > > > > > > This way, 'whatever' demo app we launch (which later could be a vendor > provided > > one), this will always work and the app will always know whether playstore is > > available or not on that device. > > I'm not sure I understand your argument about whitelisting- wouldn't we either > need a whitelist or not regardless of *how* we provide this information? If we > provide it as a property of LaunchData, we either provide the value to all apps > or use some kind of whitelist to decide which apps see a non-null (or fake?) > value. This value won't go to any apps at all except for the Demo App and the GetHelp app. The playStoreStatus parameter defaults to UNKNOWN for all other apps. Only when we're specifically launching the Demo app and the GetHelp app, do we specify that we want to send this parameter and only those apps get this. All other apps don't see this value at all (and since it is no-doc, they don't see it in any documentation either).
On 2016/08/24 22:41:38, rkc wrote: > https://codereview.chromium.org/2272813003/diff/80001/extensions/common/api/a... > File extensions/common/api/app_runtime.idl (right): > > https://codereview.chromium.org/2272813003/diff/80001/extensions/common/api/a... > extensions/common/api/app_runtime.idl:105: [nodoc] PlayStoreStatus? > playStoreStatus; > On 2016/08/24 22:37:19, Antony Sargent wrote: > > On 2016/08/24 22:05:57, rkc wrote: > > > On 2016/08/24 21:50:32, Antony Sargent wrote: > > > > Is this status expected to change often? It's a little unusual to have > > > something > > > > that seems like a fairly static bit of device information that most apps > > don't > > > > care about passed on every single app launch, as opposed to say having a > > > > separate method for fetching that state for apps that care (and possibly > an > > > > event for finding out when it changes). > > > > > > This won't be set for all apps at the moment. Eventually knowing the kind of > > > device you're running on is useful information for an app to know when it > > starts > > > up. The same was as the launch source, is a public or kiosk session, etc. > > > > On a given device, an app can either be launched in a public session or not, > or > > in a kiosk session or not. Can the PlayStoreStatus change similarly? > > > > > The primary argument against not plugging this into an API that an app that > > > cares calls versus us providing it to an app we care about was whitelisting. > > > > > > If an app calls back into an API to get this info, considering that we > cannot > > > expose this to all apps, we have to whitelist that app for that API call. > This > > > puts into the same cycle of needing to whitelist apps everytime a new one > > comes > > > along, the id changes, etc. > > > > > > > > > This way, 'whatever' demo app we launch (which later could be a vendor > > provided > > > one), this will always work and the app will always know whether playstore > is > > > available or not on that device. > > > > I'm not sure I understand your argument about whitelisting- wouldn't we either > > need a whitelist or not regardless of *how* we provide this information? If we > > provide it as a property of LaunchData, we either provide the value to all > apps > > or use some kind of whitelist to decide which apps see a non-null (or fake?) > > value. > > > This value won't go to any apps at all except for the Demo App and the GetHelp > app. The playStoreStatus parameter defaults to UNKNOWN for all other apps. Only > when we're specifically launching the Demo app and the GetHelp app, do we > specify that we want to send this parameter and only those apps get this. > > All other apps don't see this value at all (and since it is no-doc, they don't > see it in any documentation either). That seems suspiciously similar to a whitelist to me. =) It sounds like you've thought this through so I don't want to hold up progress on this point, as it really isn't that big a deal. So lgtm if you want to go forward with this approach. However, one final thing to consider is whether there are other times in an app's lifecycle where they'd want this information; for instance apps which do work in response to events other than onLaunched such as runtime.onStartup, runtime.onMessage, sockets.tcp.onReceive, usb.onDeviceAdded, etc. If that seems likely, then you'll end up with the choice of either plumbing the state through each of those on a case-by-case basis, or just adding a standalone API method. At that point, would you wish you had just added a standalone method for this use case?
On 2016/08/24 23:13:38, Antony Sargent wrote: > On 2016/08/24 22:41:38, rkc wrote: > > > https://codereview.chromium.org/2272813003/diff/80001/extensions/common/api/a... > > File extensions/common/api/app_runtime.idl (right): > > > > > https://codereview.chromium.org/2272813003/diff/80001/extensions/common/api/a... > > extensions/common/api/app_runtime.idl:105: [nodoc] PlayStoreStatus? > > playStoreStatus; > > On 2016/08/24 22:37:19, Antony Sargent wrote: > > > On 2016/08/24 22:05:57, rkc wrote: > > > > On 2016/08/24 21:50:32, Antony Sargent wrote: > > > > > Is this status expected to change often? It's a little unusual to have > > > > something > > > > > that seems like a fairly static bit of device information that most apps > > > don't > > > > > care about passed on every single app launch, as opposed to say having a > > > > > separate method for fetching that state for apps that care (and possibly > > an > > > > > event for finding out when it changes). > > > > > > > > This won't be set for all apps at the moment. Eventually knowing the kind > of > > > > device you're running on is useful information for an app to know when it > > > starts > > > > up. The same was as the launch source, is a public or kiosk session, etc. > > > > > > On a given device, an app can either be launched in a public session or not, > > or > > > in a kiosk session or not. Can the PlayStoreStatus change similarly? > > > > > > > The primary argument against not plugging this into an API that an app > that > > > > cares calls versus us providing it to an app we care about was > whitelisting. > > > > > > > > If an app calls back into an API to get this info, considering that we > > cannot > > > > expose this to all apps, we have to whitelist that app for that API call. > > This > > > > puts into the same cycle of needing to whitelist apps everytime a new one > > > comes > > > > along, the id changes, etc. > > > > > > > > > > > > This way, 'whatever' demo app we launch (which later could be a vendor > > > provided > > > > one), this will always work and the app will always know whether playstore > > is > > > > available or not on that device. > > > > > > I'm not sure I understand your argument about whitelisting- wouldn't we > either > > > need a whitelist or not regardless of *how* we provide this information? If > we > > > provide it as a property of LaunchData, we either provide the value to all > > apps > > > or use some kind of whitelist to decide which apps see a non-null (or fake?) > > > value. > > > > > > This value won't go to any apps at all except for the Demo App and the GetHelp > > app. The playStoreStatus parameter defaults to UNKNOWN for all other apps. > Only > > when we're specifically launching the Demo app and the GetHelp app, do we > > specify that we want to send this parameter and only those apps get this. > > > > All other apps don't see this value at all (and since it is no-doc, they don't > > see it in any documentation either). > > That seems suspiciously similar to a whitelist to me. =) > > It sounds like you've thought this through so I don't want to hold up progress > on this point, as it really isn't that big a deal. So lgtm if you want to go > forward with this approach. > > However, one final thing to consider is whether there are other times in an > app's lifecycle where they'd want this information; for instance apps which do > work in response to events other than onLaunched such as runtime.onStartup, > runtime.onMessage, sockets.tcp.onReceive, usb.onDeviceAdded, etc. If that seems > likely, then you'll end up with the choice of either plumbing the state through > each of those on a case-by-case basis, or just adding a standalone API method. > At that point, would you wish you had just added a standalone method for this > use case? An app that needs this information should pick it up at launch and keep it throughout its lifetime - since this is not information that is every going to change. We had a pretty long internal discussion about this. There are pros and cons for both approach, but this seemed to be cleaner. Whitelisting is icky :)
The CQ bit was checked by rkc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2272813003/#ps80001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by rkc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, lhchavez@chromium.org, asargent@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2272813003/#ps120001 (title: "crash fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by rkc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add ARC++ specific fields to launch data for specific apps. On devices that have ARC++ available or enabled, certain internal apps need to be able to get this information. Currently these are limited to the GetHelp and the demo app. These are fields we'd like to eventually just expose to all apps but currently security is not comfortable with that yet, so for now, we'll set these flags only for the apps that currently need it. Reviews requested, xiyuan@ - General review. asargent@ - //apps and extension parts lhchavez@ - ARC++ parts sky@ - //chrome/browser/ui R=asargent@chromium.org, lhchavez@chromium.org, sky@chromium.org, xiyuan@chromium.org BUG=640330 ========== to ========== Add ARC++ specific fields to launch data for specific apps. On devices that have ARC++ available or enabled, certain internal apps need to be able to get this information. Currently these are limited to the GetHelp and the demo app. These are fields we'd like to eventually just expose to all apps but currently security is not comfortable with that yet, so for now, we'll set these flags only for the apps that currently need it. Reviews requested, xiyuan@ - General review. asargent@ - //apps and extension parts lhchavez@ - ARC++ parts sky@ - //chrome/browser/ui R=asargent@chromium.org, lhchavez@chromium.org, sky@chromium.org, xiyuan@chromium.org BUG=640330 Committed: https://crrev.com/3d97de469a486debcb8956de8f7ed679fa862cbf Cr-Commit-Position: refs/heads/master@{#414348} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/3d97de469a486debcb8956de8f7ed679fa862cbf Cr-Commit-Position: refs/heads/master@{#414348}
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:120001) has been created in https://codereview.chromium.org/2276293002/ by haraken@chromium.org. The reason for reverting is: This broke compilation of ChromeOS: https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Chrom... .
Message was sent while issue was closed.
Description was changed from ========== Add ARC++ specific fields to launch data for specific apps. On devices that have ARC++ available or enabled, certain internal apps need to be able to get this information. Currently these are limited to the GetHelp and the demo app. These are fields we'd like to eventually just expose to all apps but currently security is not comfortable with that yet, so for now, we'll set these flags only for the apps that currently need it. Reviews requested, xiyuan@ - General review. asargent@ - //apps and extension parts lhchavez@ - ARC++ parts sky@ - //chrome/browser/ui R=asargent@chromium.org, lhchavez@chromium.org, sky@chromium.org, xiyuan@chromium.org BUG=640330 Committed: https://crrev.com/3d97de469a486debcb8956de8f7ed679fa862cbf Cr-Commit-Position: refs/heads/master@{#414348} ========== to ========== Add ARC++ specific fields to launch data for specific apps. On devices that have ARC++ available or enabled, certain internal apps need to be able to get this information. Currently these are limited to the GetHelp and the demo app. These are fields we'd like to eventually just expose to all apps but currently security is not comfortable with that yet, so for now, we'll set these flags only for the apps that currently need it. Reviews requested, xiyuan@ - General review. asargent@ - //apps and extension parts lhchavez@ - ARC++ parts sky@ - //chrome/browser/ui R=asargent@chromium.org, lhchavez@chromium.org, sky@chromium.org, xiyuan@chromium.org BUG=640330 Committed: https://crrev.com/3d97de469a486debcb8956de8f7ed679fa862cbf Cr-Commit-Position: refs/heads/master@{#414348} ==========
The CQ bit was checked by rkc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, lhchavez@chromium.org, asargent@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2272813003/#ps140001 (title: "chrome build fix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add ARC++ specific fields to launch data for specific apps. On devices that have ARC++ available or enabled, certain internal apps need to be able to get this information. Currently these are limited to the GetHelp and the demo app. These are fields we'd like to eventually just expose to all apps but currently security is not comfortable with that yet, so for now, we'll set these flags only for the apps that currently need it. Reviews requested, xiyuan@ - General review. asargent@ - //apps and extension parts lhchavez@ - ARC++ parts sky@ - //chrome/browser/ui R=asargent@chromium.org, lhchavez@chromium.org, sky@chromium.org, xiyuan@chromium.org BUG=640330 Committed: https://crrev.com/3d97de469a486debcb8956de8f7ed679fa862cbf Cr-Commit-Position: refs/heads/master@{#414348} ========== to ========== Add ARC++ specific fields to launch data for specific apps. On devices that have ARC++ available or enabled, certain internal apps need to be able to get this information. Currently these are limited to the GetHelp and the demo app. These are fields we'd like to eventually just expose to all apps but currently security is not comfortable with that yet, so for now, we'll set these flags only for the apps that currently need it. Reviews requested, xiyuan@ - General review. asargent@ - //apps and extension parts lhchavez@ - ARC++ parts sky@ - //chrome/browser/ui R=asargent@chromium.org, lhchavez@chromium.org, sky@chromium.org, xiyuan@chromium.org BUG=640330 Committed: https://crrev.com/3d97de469a486debcb8956de8f7ed679fa862cbf Cr-Commit-Position: refs/heads/master@{#414348} ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add ARC++ specific fields to launch data for specific apps. On devices that have ARC++ available or enabled, certain internal apps need to be able to get this information. Currently these are limited to the GetHelp and the demo app. These are fields we'd like to eventually just expose to all apps but currently security is not comfortable with that yet, so for now, we'll set these flags only for the apps that currently need it. Reviews requested, xiyuan@ - General review. asargent@ - //apps and extension parts lhchavez@ - ARC++ parts sky@ - //chrome/browser/ui R=asargent@chromium.org, lhchavez@chromium.org, sky@chromium.org, xiyuan@chromium.org BUG=640330 Committed: https://crrev.com/3d97de469a486debcb8956de8f7ed679fa862cbf Cr-Commit-Position: refs/heads/master@{#414348} ========== to ========== Add ARC++ specific fields to launch data for specific apps. On devices that have ARC++ available or enabled, certain internal apps need to be able to get this information. Currently these are limited to the GetHelp and the demo app. These are fields we'd like to eventually just expose to all apps but currently security is not comfortable with that yet, so for now, we'll set these flags only for the apps that currently need it. Reviews requested, xiyuan@ - General review. asargent@ - //apps and extension parts lhchavez@ - ARC++ parts sky@ - //chrome/browser/ui R=asargent@chromium.org, lhchavez@chromium.org, sky@chromium.org, xiyuan@chromium.org BUG=640330 Committed: https://crrev.com/3d97de469a486debcb8956de8f7ed679fa862cbf Committed: https://crrev.com/79bf63cbe7a00345f152d0dbb3dc20e24442713f Cr-Original-Commit-Position: refs/heads/master@{#414348} Cr-Commit-Position: refs/heads/master@{#414535} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/79bf63cbe7a00345f152d0dbb3dc20e24442713f Cr-Commit-Position: refs/heads/master@{#414535} |
