Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2212303003:
Implement app launch changes for app runtime extension proposal. (Closed)
|
Created:
4 years, 4 months ago by jdufault Modified:
4 years, 4 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@tool-screenshot Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement app launch changes for app runtime extension proposal.
This does not include the manifest changes. The proposal is at
https://groups.google.com/a/chromium.org/forum/#!topic/apps-dev/F-Z6yXhiz0s.
BUG=635995
Committed: https://crrev.com/9d3e955fe48e265bbb9f67a8acd69891e3a587ce
Cr-Commit-Position: refs/heads/master@{#412357}
Patch Set 1 : Initial upload #
Total comments: 14
Patch Set 2 : Address comments #
Total comments: 8
Patch Set 3 : Fix compile #
Total comments: 10
Patch Set 4 : Comments & compile #Patch Set 5 : Rebase #
Total comments: 30
Patch Set 6 : Address comments #Patch Set 7 : Rebase #
Total comments: 4
Patch Set 8 : Nits #
Dependent Patchsets: Messages
Total messages: 53 (37 generated)
The CQ bit was checked by jdufault@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Description was changed from ========== Implement app launch changes for app runtime extension proposal. This does not include the manifest changes. The proposal is at https://groups.google.com/a/chromium.org/forum/#!topic/apps-dev/F-Z6yXhiz0s. BUG=616112 ========== to ========== Implement app launch changes for app runtime extension proposal. This does not include the manifest changes. The proposal is at https://groups.google.com/a/chromium.org/forum/#!topic/apps-dev/F-Z6yXhiz0s. BUG=616112 ==========
jdufault@chromium.org changed reviewers: + derat@chromium.org
derat@, here is the CL with the extension API implementation. This should be good enough until we need to filter out apps which support specific action types.
The CQ bit was checked by jdufault@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2212303003/diff/20001/apps/launcher.h File apps/launcher.h (right): https://codereview.chromium.org/2212303003/diff/20001/apps/launcher.h#newcode47 apps/launcher.h:47: extensions::ActionData action_data); make this also take an optional const base::FilePath&? https://codereview.chromium.org/2212303003/diff/20001/extensions/browser/api/... File extensions/browser/api/app_runtime/app_runtime_api.cc (right): https://codereview.chromium.org/2212303003/diff/20001/extensions/browser/api/... extensions/browser/api/app_runtime/app_runtime_api.cc:249: launch_data.action_data.reset(BuildActionData(action_data)); does the action_data member end up being a unique_ptr or scoped_ptr? if so, please make BuildActionData return that type instead of a bare pointer. https://codereview.chromium.org/2212303003/diff/20001/extensions/common/api/a... File extensions/common/api/app_runtime.idl (right): https://codereview.chromium.org/2212303003/diff/20001/extensions/common/api/a... extensions/common/api/app_runtime.idl:39: action i left a high-level comment on the api proposal about this; it feels strange to me for this to be a LaunchSource rather than an additional field in app.runtime describing the action that the app should perform. https://codereview.chromium.org/2212303003/diff/20001/extensions/common/api/a... extensions/common/api/app_runtime.idl:43: // other names include 'Action' or 'Workflow'. The idea is to encapsulate all i don't understand the "possible other names" sentence. where will those other names be used? https://codereview.chromium.org/2212303003/diff/20001/extensions/common/api/a... extensions/common/api/app_runtime.idl:50: // Specifies the specific <code>ActionType</code> the app was launched with. nit: delete "specific" since "specifies" is already pretty specific :-) https://codereview.chromium.org/2212303003/diff/20001/extensions/common/api/a... extensions/common/api/app_runtime.idl:51: // This also contains any action-type specific data. nit: i'd delete this until such data has actually been added to it https://codereview.chromium.org/2212303003/diff/20001/extensions/common/api/a... extensions/common/api/app_runtime.idl:91: // with. This also contains any action specific data. This is only relevant nit: the second sentence seems extraneous since it's currently already documented above the ActionData dictionary
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Implement app launch changes for app runtime extension proposal. This does not include the manifest changes. The proposal is at https://groups.google.com/a/chromium.org/forum/#!topic/apps-dev/F-Z6yXhiz0s. BUG=616112 ========== to ========== Implement app launch changes for app runtime extension proposal. This does not include the manifest changes. The proposal is at https://groups.google.com/a/chromium.org/forum/#!topic/apps-dev/F-Z6yXhiz0s. BUG=635995 ==========
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by jdufault@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2212303003/diff/20001/apps/launcher.h File apps/launcher.h (right): https://codereview.chromium.org/2212303003/diff/20001/apps/launcher.h#newcode47 apps/launcher.h:47: extensions::ActionData action_data); On 2016/08/05 23:36:45, Daniel Erat wrote: > make this also take an optional const base::FilePath&? Done. https://codereview.chromium.org/2212303003/diff/20001/extensions/browser/api/... File extensions/browser/api/app_runtime/app_runtime_api.cc (right): https://codereview.chromium.org/2212303003/diff/20001/extensions/browser/api/... extensions/browser/api/app_runtime/app_runtime_api.cc:249: launch_data.action_data.reset(BuildActionData(action_data)); On 2016/08/05 23:36:45, Daniel Erat wrote: > does the action_data member end up being a unique_ptr or scoped_ptr? if so, > please make BuildActionData return that type instead of a bare pointer. Done. https://codereview.chromium.org/2212303003/diff/20001/extensions/common/api/a... File extensions/common/api/app_runtime.idl (right): https://codereview.chromium.org/2212303003/diff/20001/extensions/common/api/a... extensions/common/api/app_runtime.idl:39: action On 2016/08/05 23:36:46, Daniel Erat wrote: > i left a high-level comment on the api proposal about this; it feels strange to > me for this to be a LaunchSource rather than an additional field in app.runtime > describing the action that the app should perform. I've removed the launch source for the time being, but depending on rdcronin@'s thoughts I may bring it back. https://codereview.chromium.org/2212303003/diff/20001/extensions/common/api/a... extensions/common/api/app_runtime.idl:43: // other names include 'Action' or 'Workflow'. The idea is to encapsulate all On 2016/08/05 23:36:46, Daniel Erat wrote: > i don't understand the "possible other names" sentence. where will those other > names be used? Blah, this is an accidental change that shouldn't be here. I'll tidy up the comments. https://codereview.chromium.org/2212303003/diff/20001/extensions/common/api/a... extensions/common/api/app_runtime.idl:50: // Specifies the specific <code>ActionType</code> the app was launched with. On 2016/08/05 23:36:45, Daniel Erat wrote: > nit: delete "specific" since "specifies" is already pretty specific :-) Done. https://codereview.chromium.org/2212303003/diff/20001/extensions/common/api/a... extensions/common/api/app_runtime.idl:51: // This also contains any action-type specific data. On 2016/08/05 23:36:46, Daniel Erat wrote: > nit: i'd delete this until such data has actually been added to it Done. https://codereview.chromium.org/2212303003/diff/20001/extensions/common/api/a... extensions/common/api/app_runtime.idl:91: // with. This also contains any action specific data. This is only relevant On 2016/08/05 23:36:45, Daniel Erat wrote: > nit: the second sentence seems extraneous since it's currently already > documented above the ActionData dictionary Done.
The CQ bit was checked by jdufault@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
generally looks fine to me, and i don't have any comments besides these. (there are some duplicates since i don't seem to be able to delete the ones that i wrote on the earlier patch set.) https://codereview.chromium.org/2212303003/diff/60001/apps/launcher.cc File apps/launcher.cc (right): https://codereview.chromium.org/2212303003/diff/60001/apps/launcher.cc#newcod... apps/launcher.cc:102: action_data_(), nit: leave this out? (i'm guessing it has a default c'tor that does whatever you're doing here) https://codereview.chromium.org/2212303003/diff/60001/apps/launcher.cc#newcod... apps/launcher.cc:112: action_data_(), nit: ditto https://codereview.chromium.org/2212303003/diff/60001/apps/launcher.h File apps/launcher.h (right): https://codereview.chromium.org/2212303003/diff/60001/apps/launcher.h#newcode45 apps/launcher.h:45: // |file_path| is an optional argument and if present contains any files that nit: s/files/file/ (unless you meant to make this a vector of FilePaths) https://codereview.chromium.org/2212303003/diff/60001/apps/launcher.h#newcode49 apps/launcher.h:49: extensions::ActionData action_data, should this be a const reference instead? i'm not sure of the type of the code that's generated from the idl (is it a struct?) and am still struggling with the details of base::Optional and std::optional. https://codereview.chromium.org/2212303003/diff/80001/apps/launcher.cc File apps/launcher.cc (right): https://codereview.chromium.org/2212303003/diff/80001/apps/launcher.cc#newcod... apps/launcher.cc:102: action_data_(), nit: leave this out? (i'm guessing it has a default c'tor that does whatever you're doing here) https://codereview.chromium.org/2212303003/diff/80001/apps/launcher.cc#newcod... apps/launcher.cc:112: action_data_(), nit: ditto https://codereview.chromium.org/2212303003/diff/80001/apps/launcher.h File apps/launcher.h (right): https://codereview.chromium.org/2212303003/diff/80001/apps/launcher.h#newcode45 apps/launcher.h:45: // |file_path| is an optional argument and if present contains any files that nit: s/files/file/ (unless you meant to make this a vector of FilePaths) https://codereview.chromium.org/2212303003/diff/80001/apps/launcher.h#newcode49 apps/launcher.h:49: extensions::ActionData action_data, should this be a const reference instead? i'm not sure of the type of the code that's generated from the idl (is it a struct?) and am still struggling with the details of base::Optional and std::optional. https://codereview.chromium.org/2212303003/diff/80001/extensions/browser/api/... File extensions/browser/api/app_runtime/app_runtime_api.cc (right): https://codereview.chromium.org/2212303003/diff/80001/extensions/browser/api/... extensions/browser/api/app_runtime/app_runtime_api.cc:38: default: can you leave out the default block here to get a compilation error if there's an unhandled value? (may need to add a NOTREACHED() and return of _NONE outside of the switch statement) https://codereview.chromium.org/2212303003/diff/80001/extensions/common/const... File extensions/common/constants.h (right): https://codereview.chromium.org/2212303003/diff/80001/extensions/common/const... extensions/common/constants.h:139: // SOURCE_ACTION. nit: delete obsolete second sentence
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jdufault@chromium.org to run a CQ dry run
https://codereview.chromium.org/2212303003/diff/60001/apps/launcher.cc File apps/launcher.cc (right): https://codereview.chromium.org/2212303003/diff/60001/apps/launcher.cc#newcod... apps/launcher.cc:102: action_data_(), On 2016/08/10 22:19:24, Daniel Erat wrote: > nit: leave this out? (i'm guessing it has a default c'tor that does whatever > you're doing here) Done. https://codereview.chromium.org/2212303003/diff/60001/apps/launcher.cc#newcod... apps/launcher.cc:112: action_data_(), On 2016/08/10 22:19:24, Daniel Erat wrote: > nit: ditto Done. https://codereview.chromium.org/2212303003/diff/60001/apps/launcher.h File apps/launcher.h (right): https://codereview.chromium.org/2212303003/diff/60001/apps/launcher.h#newcode45 apps/launcher.h:45: // |file_path| is an optional argument and if present contains any files that On 2016/08/10 22:19:24, Daniel Erat wrote: > nit: s/files/file/ (unless you meant to make this a vector of FilePaths) Done. https://codereview.chromium.org/2212303003/diff/60001/apps/launcher.h#newcode49 apps/launcher.h:49: extensions::ActionData action_data, On 2016/08/10 22:19:24, Daniel Erat wrote: > should this be a const reference instead? i'm not sure of the type of the code > that's generated from the idl (is it a struct?) and am still struggling with the > details of base::Optional and std::optional. Done. https://codereview.chromium.org/2212303003/diff/80001/apps/launcher.h File apps/launcher.h (right): https://codereview.chromium.org/2212303003/diff/80001/apps/launcher.h#newcode45 apps/launcher.h:45: // |file_path| is an optional argument and if present contains any files that On 2016/08/10 22:19:24, Daniel Erat wrote: > nit: s/files/file/ (unless you meant to make this a vector of FilePaths) Done. https://codereview.chromium.org/2212303003/diff/80001/apps/launcher.h#newcode49 apps/launcher.h:49: extensions::ActionData action_data, On 2016/08/10 22:19:24, Daniel Erat wrote: > should this be a const reference instead? i'm not sure of the type of the code > that's generated from the idl (is it a struct?) and am still struggling with the > details of base::Optional and std::optional. Done. https://codereview.chromium.org/2212303003/diff/80001/extensions/browser/api/... File extensions/browser/api/app_runtime/app_runtime_api.cc (right): https://codereview.chromium.org/2212303003/diff/80001/extensions/browser/api/... extensions/browser/api/app_runtime/app_runtime_api.cc:38: default: On 2016/08/10 22:19:24, Daniel Erat wrote: > can you leave out the default block here to get a compilation error if there's > an unhandled value? (may need to add a NOTREACHED() and return of _NONE outside > of the switch statement) Done. https://codereview.chromium.org/2212303003/diff/80001/extensions/common/const... File extensions/common/constants.h (right): https://codereview.chromium.org/2212303003/diff/80001/extensions/common/const... extensions/common/constants.h:139: // SOURCE_ACTION. On 2016/08/10 22:19:24, Daniel Erat wrote: > nit: delete obsolete second sentence Done.
Dry run: 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
Dry run: 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...)
jdufault@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
rdevlin.cronin@, PTAL. I've dropped the SOURCE_ACTION launch source and used SOURCE_UNTRACKED instead. If you strongly prefer SOURCE_ACTION I can add it back.
jdufault@chromium.org changed reviewers: + benwells@chromium.org
On 2016/08/11 19:08:29, jdufault wrote: > rdevlin.cronin@, PTAL. I've dropped the SOURCE_ACTION launch source and used > SOURCE_UNTRACKED instead. > > If you strongly prefer SOURCE_ACTION I can add it back. benwells@ PTAL as well (specifically for apps/* changes).
lgtm for extensions/, for whatever that's worth, which probably shouldn't be much. :-P https://codereview.chromium.org/2212303003/diff/120001/extensions/browser/api... File extensions/browser/api/app_runtime/app_runtime_api.cc (right): https://codereview.chromium.org/2212303003/diff/120001/extensions/browser/api... extensions/browser/api/app_runtime/app_runtime_api.cc:40: NOTREACHED(); nit: also log the bad type, e.g. NOTREACHED() << "Unhandled ActionType " << action_type; https://codereview.chromium.org/2212303003/diff/120001/extensions/browser/api... extensions/browser/api/app_runtime/app_runtime_api.cc:41: return app_runtime::ActionType::ACTION_TYPE_NONE; random question: does this _NONE enum value get automatically created? https://codereview.chromium.org/2212303003/diff/120001/extensions/browser/api... extensions/browser/api/app_runtime/app_runtime_api.cc:205: if (extensions::FeatureSwitch::trace_app_source()->IsEnabled()) { nit: move this up just below the line that initializes source_enum?
https://codereview.chromium.org/2212303003/diff/120001/apps/launcher.cc File apps/launcher.cc (right): https://codereview.chromium.org/2212303003/diff/120001/apps/launcher.cc#newco... apps/launcher.cc:337: base::Optional<extensions::ActionData> action_data_; Why an optional instead of a unique_ptr here? https://codereview.chromium.org/2212303003/diff/120001/apps/launcher.h File apps/launcher.h (right): https://codereview.chromium.org/2212303003/diff/120001/apps/launcher.h#newcode48 apps/launcher.h:48: const extensions::Extension* extension, let's use |app| instead of |extension| (I know the other places call it extension, but that's just awkward). https://codereview.chromium.org/2212303003/diff/120001/extensions/browser/api... File extensions/browser/api/app_runtime/app_runtime_api.cc (right): https://codereview.chromium.org/2212303003/diff/120001/extensions/browser/api... extensions/browser/api/app_runtime/app_runtime_api.cc:41: return app_runtime::ActionType::ACTION_TYPE_NONE; On 2016/08/11 20:37:06, Daniel Erat wrote: > random question: does this _NONE enum value get automatically created? Yep. https://codereview.chromium.org/2212303003/diff/120001/extensions/browser/api... extensions/browser/api/app_runtime/app_runtime_api.cc:198: app_runtime::LaunchSource source_enum = getLaunchSourceEnum(source); getLaunchSourceEnum -> GetLaunchSourceEnum I know it's not from this CL, but now's a good time to fix it. :) https://codereview.chromium.org/2212303003/diff/120001/extensions/browser/api... extensions/browser/api/app_runtime/app_runtime_api.cc:210: launch_data->Set("action_data", this should be actionData. If we used app_runtime::LaunchData instead and then used ToValue(), this would be a lot more stable. https://codereview.chromium.org/2212303003/diff/120001/extensions/common/api/... File extensions/common/api/app_runtime.idl (right): https://codereview.chromium.org/2212303003/diff/120001/extensions/common/api/... extensions/common/api/app_runtime.idl:42: // create a new note. If action data is present, a special UI should be shown Do we need to dictate that a "special UI should be shown"? https://codereview.chromium.org/2212303003/diff/120001/extensions/common/api/... extensions/common/api/app_runtime.idl:44: // with is available inside of the |action_data| field from the LaunchData actionData https://codereview.chromium.org/2212303003/diff/120001/extensions/common/api/... extensions/common/api/app_runtime.idl:51: // Specifies the <code>ActionType</code> the app was launched with. This should go over actionType, rather than here. https://codereview.chromium.org/2212303003/diff/120001/extensions/common/api/... extensions/common/api/app_runtime.idl:53: ActionType action_type; actionType https://codereview.chromium.org/2212303003/diff/120001/extensions/common/api/... extensions/common/api/app_runtime.idl:93: ActionData? action_data; actionData https://codereview.chromium.org/2212303003/diff/120001/extensions/common/cons... File extensions/common/constants.h (right): https://codereview.chromium.org/2212303003/diff/120001/extensions/common/cons... extensions/common/constants.h:143: ActionType action_type; Is there a reason to not use app_runtime::ActionData? https://codereview.chromium.org/2212303003/diff/120001/extensions/shell/brows... File extensions/shell/browser/shell_extension_system.cc (right): https://codereview.chromium.org/2212303003/diff/120001/extensions/shell/brows... extensions/shell/browser/shell_extension_system.cc:99: extensions::SOURCE_UNTRACKED, no need for extensions::
I'll defer to Devlin on all this, so if it lgthim it lgtm
The CQ bit was checked by jdufault@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2212303003/diff/120001/apps/launcher.cc File apps/launcher.cc (right): https://codereview.chromium.org/2212303003/diff/120001/apps/launcher.cc#newco... apps/launcher.cc:337: base::Optional<extensions::ActionData> action_data_; On 2016/08/11 20:51:20, Devlin wrote: > Why an optional instead of a unique_ptr here? It conveyed intent more clearly. With the removal of the wrapper type, I've moved to unique_ptr because of the DISALLOW_COPY_AND_ASSIGN. https://codereview.chromium.org/2212303003/diff/120001/apps/launcher.h File apps/launcher.h (right): https://codereview.chromium.org/2212303003/diff/120001/apps/launcher.h#newcode48 apps/launcher.h:48: const extensions::Extension* extension, On 2016/08/11 20:51:20, Devlin wrote: > let's use |app| instead of |extension| (I know the other places call it > extension, but that's just awkward). Done. https://codereview.chromium.org/2212303003/diff/120001/extensions/browser/api... File extensions/browser/api/app_runtime/app_runtime_api.cc (right): https://codereview.chromium.org/2212303003/diff/120001/extensions/browser/api... extensions/browser/api/app_runtime/app_runtime_api.cc:40: NOTREACHED(); On 2016/08/11 20:37:06, Daniel Erat (OOO 8-12 to 8-22) wrote: > nit: also log the bad type, e.g. > > NOTREACHED() << "Unhandled ActionType " << action_type; Done, but I had to add a cast since action_type is an enum class. https://codereview.chromium.org/2212303003/diff/120001/extensions/browser/api... extensions/browser/api/app_runtime/app_runtime_api.cc:198: app_runtime::LaunchSource source_enum = getLaunchSourceEnum(source); On 2016/08/11 20:51:21, Devlin wrote: > getLaunchSourceEnum -> GetLaunchSourceEnum > > I know it's not from this CL, but now's a good time to fix it. :) Done. https://codereview.chromium.org/2212303003/diff/120001/extensions/browser/api... extensions/browser/api/app_runtime/app_runtime_api.cc:205: if (extensions::FeatureSwitch::trace_app_source()->IsEnabled()) { On 2016/08/11 20:37:06, Daniel Erat (OOO 8-12 to 8-22) wrote: > nit: move this up just below the line that initializes source_enum? source_enum is used below as well (DispatchOnLaunchedEventImpl). https://codereview.chromium.org/2212303003/diff/120001/extensions/browser/api... extensions/browser/api/app_runtime/app_runtime_api.cc:210: launch_data->Set("action_data", On 2016/08/11 20:51:21, Devlin wrote: > this should be actionData. Done. > If we used app_runtime::LaunchData instead and then used ToValue(), this would > be a lot more stable. Yes, I agree. I converted it but the LaunchItem enum does not contain the necessary typings. We could go with a hybrid approach, where everything except the launch items are typed. Alternatively, we could add the typings to launch item. https://codereview.chromium.org/2212303003/diff/120001/extensions/common/api/... File extensions/common/api/app_runtime.idl (right): https://codereview.chromium.org/2212303003/diff/120001/extensions/common/api/... extensions/common/api/app_runtime.idl:42: // create a new note. If action data is present, a special UI should be shown On 2016/08/11 20:51:21, Devlin (ooo aug12) wrote: > Do we need to dictate that a "special UI should be shown"? Done. https://codereview.chromium.org/2212303003/diff/120001/extensions/common/api/... extensions/common/api/app_runtime.idl:44: // with is available inside of the |action_data| field from the LaunchData On 2016/08/11 20:51:21, Devlin (ooo aug12) wrote: > actionData Done. https://codereview.chromium.org/2212303003/diff/120001/extensions/common/api/... extensions/common/api/app_runtime.idl:51: // Specifies the <code>ActionType</code> the app was launched with. On 2016/08/11 20:51:21, Devlin (ooo aug12) wrote: > This should go over actionType, rather than here. Done. https://codereview.chromium.org/2212303003/diff/120001/extensions/common/api/... extensions/common/api/app_runtime.idl:53: ActionType action_type; On 2016/08/11 20:51:21, Devlin (ooo aug12) wrote: > actionType Done. https://codereview.chromium.org/2212303003/diff/120001/extensions/common/api/... extensions/common/api/app_runtime.idl:93: ActionData? action_data; On 2016/08/11 20:51:21, Devlin (ooo aug12) wrote: > actionData Done. https://codereview.chromium.org/2212303003/diff/120001/extensions/common/cons... File extensions/common/constants.h (right): https://codereview.chromium.org/2212303003/diff/120001/extensions/common/cons... extensions/common/constants.h:143: ActionType action_type; On 2016/08/11 20:51:21, Devlin (ooo aug12) wrote: > Is there a reason to not use app_runtime::ActionData? Done. https://codereview.chromium.org/2212303003/diff/120001/extensions/shell/brows... File extensions/shell/browser/shell_extension_system.cc (right): https://codereview.chromium.org/2212303003/diff/120001/extensions/shell/brows... extensions/shell/browser/shell_extension_system.cc:99: extensions::SOURCE_UNTRACKED, On 2016/08/11 20:51:21, Devlin (ooo aug12) wrote: > no need for extensions:: Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jdufault@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2212303003/diff/120001/extensions/browser/api... File extensions/browser/api/app_runtime/app_runtime_api.cc (right): https://codereview.chromium.org/2212303003/diff/120001/extensions/browser/api... extensions/browser/api/app_runtime/app_runtime_api.cc:210: launch_data->Set("action_data", On 2016/08/12 18:56:55, jdufault wrote: > On 2016/08/11 20:51:21, Devlin wrote: > > this should be actionData. > Done. > > > If we used app_runtime::LaunchData instead and then used ToValue(), this would > > be a lot more stable. > > Yes, I agree. I converted it but the LaunchItem enum does not contain the > necessary typings. We could go with a hybrid approach, where everything except > the launch items are typed. Alternatively, we could add the typings to launch > item. We should document the fields of the launch item, which would address this. Let's add a TODO for that for now. https://codereview.chromium.org/2212303003/diff/160001/extensions/common/api/... File extensions/common/api/app_runtime.idl (right): https://codereview.chromium.org/2212303003/diff/160001/extensions/common/api/... extensions/common/api/app_runtime.idl:89: // Contains data that specifies what the <code>ActionType</code> this app remove "what" https://codereview.chromium.org/2212303003/diff/160001/extensions/common/api/... extensions/common/api/app_runtime.idl:91: // specific action in mind. "in mind" is kind of weird phrasing here. Maybe something more like "with a specific action intent"?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! https://codereview.chromium.org/2212303003/diff/120001/extensions/browser/api... File extensions/browser/api/app_runtime/app_runtime_api.cc (right): https://codereview.chromium.org/2212303003/diff/120001/extensions/browser/api... extensions/browser/api/app_runtime/app_runtime_api.cc:210: launch_data->Set("action_data", On 2016/08/16 00:20:09, Devlin wrote: > On 2016/08/12 18:56:55, jdufault wrote: > > On 2016/08/11 20:51:21, Devlin wrote: > > > this should be actionData. > > Done. > > > > > If we used app_runtime::LaunchData instead and then used ToValue(), this > would > > > be a lot more stable. > > > > Yes, I agree. I converted it but the LaunchItem enum does not contain the > > necessary typings. We could go with a hybrid approach, where everything except > > the launch items are typed. Alternatively, we could add the typings to launch > > item. > > We should document the fields of the launch item, which would address this. > Let's add a TODO for that for now. Done. https://codereview.chromium.org/2212303003/diff/160001/extensions/common/api/... File extensions/common/api/app_runtime.idl (right): https://codereview.chromium.org/2212303003/diff/160001/extensions/common/api/... extensions/common/api/app_runtime.idl:89: // Contains data that specifies what the <code>ActionType</code> this app On 2016/08/16 00:20:09, Devlin wrote: > remove "what" Done. https://codereview.chromium.org/2212303003/diff/160001/extensions/common/api/... extensions/common/api/app_runtime.idl:91: // specific action in mind. On 2016/08/16 00:20:09, Devlin wrote: > "in mind" is kind of weird phrasing here. Maybe something more like "with a > specific action intent"? Done.
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from benwells@chromium.org, derat@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2212303003/#ps180001 (title: "Nits")
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 ========== Implement app launch changes for app runtime extension proposal. This does not include the manifest changes. The proposal is at https://groups.google.com/a/chromium.org/forum/#!topic/apps-dev/F-Z6yXhiz0s. BUG=635995 ========== to ========== Implement app launch changes for app runtime extension proposal. This does not include the manifest changes. The proposal is at https://groups.google.com/a/chromium.org/forum/#!topic/apps-dev/F-Z6yXhiz0s. BUG=635995 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Implement app launch changes for app runtime extension proposal. This does not include the manifest changes. The proposal is at https://groups.google.com/a/chromium.org/forum/#!topic/apps-dev/F-Z6yXhiz0s. BUG=635995 ========== to ========== Implement app launch changes for app runtime extension proposal. This does not include the manifest changes. The proposal is at https://groups.google.com/a/chromium.org/forum/#!topic/apps-dev/F-Z6yXhiz0s. BUG=635995 Committed: https://crrev.com/9d3e955fe48e265bbb9f67a8acd69891e3a587ce Cr-Commit-Position: refs/heads/master@{#412357} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/9d3e955fe48e265bbb9f67a8acd69891e3a587ce Cr-Commit-Position: refs/heads/master@{#412357} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
