Consider making ActivityName.activity_name optional in intent_helper.mojom |
||||||
Issue description
The ActivityName struct in intent_helper.mojom currently looks like this:
// Describes an activity.
struct ActivityName {
string package_name;
string activity_name;
};
At least some of the code on the Android side seems to expect the activity name to sometimes be unset. See ArcIntentHelperService#handleUrlList, for instance:
if (activity.activityName == null) {
intent.setComponent(new ComponentName(activity.packageName, activity.activityName));
} else {
intent.setPackage(activity.packageName);
}
A null activity name isn't permitted by the .mojom file, I think, so this code will never run, but it seems like a reasonable thing to allow -- per jsharkey@, only setting the package allows an app to rename its activities more-or-less transparently.
I think we should make activity_name nullable in the .mojom file and update Android code that handles ActivityName structs to check "activityName != null && !activityName.isEmpty()". Sound reasonable?
If we want to stay away from optional fields apart from backward-compatibility scenarios, I'll instead change the Android side to check whether the string is empty rather than null.
I'll update the .mojom file to document what's expected in either case. :-P
,
Oct 26 2016
Oh, interesting. I'd assumed that making a field nullable was backwards-compatible -- at least, things seem to work when I mark it nullable on the Chrome side but not the Android side (as long as I'm still assigning an empty string to it in Chrome, since Android expects that). I didn't see a definitive answer in the Mojo docs, but maybe I missed it.
,
Oct 26 2016
If you send a null to a non-nullable element, it will throw a DeserializationException[1]. I guess making it empty is the only way to go :( 1: https://cs.chromium.org/chromium/src/mojo/public/java/bindings/src/org/chromium/mojo/bindings/Decoder.java?q=readString+file:src/mojo&sq=package:chromium&l=313
,
Oct 26 2016
We're talking about going the other way, I think: 1. Update sender mojom to be nullable, but continue setting the field. 2. Update receiver mojom to be nullable, and update code to handle null. 3. Update sender to not set the field.
,
Oct 26 2016
I once did a change from non-nullable to nullable https://codereview.chromium.org/1973233002/ and the answer from the expert in Mojo team (+yzshen) was that it basically works safe as expected, except the one obvious scenario: when a sender using the new nullable mojom sends null to a receiver using the old non-nullable mojom. Hence, if we secure a safe margin between the steps 2 and 3, I think it is doable.
,
Oct 26 2016
Great, that matches what I'm seeing.
,
Oct 26 2016
IIRC, the activity.activityName == null case was used in the past for supporting deprecated mojo call; at the beginning it only passed the package name and later changed to add the activity name as well, to disambiguate multiple handlers from the same package, on the Chromium side. (And thus now it is not used, since the current intent chooser on Chrome side wants to fully disambiguate them always.) If there's a need for sending an intent that does not want to set activity name, making it nullable sounds making sense.
,
Oct 26 2016
Great, thanks. Yeah, I would find being able to do this useful for issue 647802.
,
Oct 26 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6a13c43acff6124abf2f0816492d348bce8b16cf commit 6a13c43acff6124abf2f0816492d348bce8b16cf Author: derat <derat@chromium.org> Date: Wed Oct 26 23:36:48 2016 arc: Make ActivityName.activity_name nullable. Make the ActivityName struct's activity_name field nullable so that IntentHelperInstance.HandleIntent can be used to launch an arbitrary applicable activity within a given package. BUG= 659802 Review-Url: https://codereview.chromium.org/2448833006 Cr-Commit-Position: refs/heads/master@{#427879} [modify] https://crrev.com/6a13c43acff6124abf2f0816492d348bce8b16cf/components/arc/common/intent_helper.mojom
,
Nov 2 2016
,
Nov 23 2016
Closing bug as it is related to code change.
,
Dec 17 2016
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by yusukes@chromium.org
, Oct 26 2016