Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2438103002:
Add postMessage APIs to the support lib (Closed)
|
Created:
4 years, 2 months ago by Yusuf Modified:
4 years, 1 month ago Reviewers:
Benoit L Target Ref:
refs/heads/master Project:
custom-tabs-client Visibility:
Public. |
DescriptionAdd postMessage APIs to the support lib
Adds CustomTabsSession#validateOriginForPostMessage,
CustomTabsSession#postMessage,
CustomTabsCallback#onMessageChannelCreated,
CustomTabsCallback#onPostMessage for sending an receiving postMessage
requests to the mainframe of the initial url.
BUG=657915
Committed: https://github.com/GoogleChrome/custom-tabs-client/commit/3cf33a835e4cd777cea7399ecd99e452ad292dae
Patch Set 1 #Patch Set 2 : compile fixes #Patch Set 3 : Overridden callback in CustomTabsSessionToken #
Total comments: 22
Patch Set 4 : lizeb@ comments #
Total comments: 28
Patch Set 5 : lizeb@ comments #
Total comments: 4
Patch Set 6 : Comments #Patch Set 7 : Made onMessageChannelReady synchronized as well #Patch Set 8 : removed unnecessary import #Messages
Total messages: 15 (4 generated)
yusufo@chromium.org changed reviewers: + lizeb@chromium.org
https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... File customtabs/src/android/support/customtabs/CustomTabsCallback.java (right): https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsCallback.java:57: /** Thinking about it, I wonder whether we should clarify and simplify threading across the board here. The main issue is that we never made explicit on which thread the callbacks are executed. This was probably not a huge issue until now, since we sent few callbacks, but this is becoming worse. Probably the simplest solution that's consistent for applications is either (a) to specify that there is no threading guarantees whatsoever, or (b) to guarantee that everything happens on the UI thread. Solution (a) is easier for us, but dangerous for applications. For CustomTabsCallback#onPostMessage(), if we want to keep the model that we have on the "web" side, then we should post the callback to the UI thread, symmetrically to what we do in Chrome's implementation. This also has to advantage of not blocking Chrome's UI thread if onPostMessage() is heavy. What about, in CustomTabsClient, when we create the CustomTabsCallback object, we actually make sure that everything is dispatched to the UI thread? https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... File customtabs/src/android/support/customtabs/CustomTabsClient.java (right): https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsClient.java:182: ICustomTabsCallback.Stub wrapper = new ICustomTabsCallback.Stub() { What about a proxy class: /** Posts the callback to the main thread. */ private static class CustomTabsCallbackProxy extends ICustomTabsCallback.Stub { private final Handler mHandler; private final CustomTabsCallback mCallback; public CustomTabsCallbackProxy(CustomTabsCallback callback) { mHandler = new Handler(Looper.getMainLooper()); mCallback = callback; } @Override public void onNavigationEvent(final int navigationEvent, final Bundle extras) { if (mCallback == null) return; mHandler.post(new Runnable() { @Override public void run() { mCallback.onNavigationEvent(navigationEvent, extras); } }); } @Override public void extraCallback(final String callbackName, final Bundle args) throws RemoteException { if (callback == null) return; mHandler.post(new Runnable() { @Override public void run() { mCallback.extraCallback(callbackName, args); } }); } } And then, here: CustomTabsCallbackProxy proxy = new CustomTabsCallbackProxy(callback); https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... File customtabs/src/android/support/customtabs/CustomTabsService.java (right): https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsService.java:53: public static final int RESULT_SUCCESS = 1; nit: These error codes are a bit weird. In the UNIX tradition, 0 means success, and the rest means that is an error. So it seems weird that success is 1, and the other ones are errors. https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsService.java:54: public static final int RESULT_FAILURE_DISALLOWED = 0; Can you use @IntDef here? https://developer.android.com/reference/android/support/annotation/IntDef.html https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsService.java:215: * Sends a request to the implementation for validate and assign a postMessage origin for the nit: s/for/to/ ? https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsService.java:222: * here doesn't mean an origin has already been assigned as the validation can be nit: s/can be/is/ https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsService.java:238: protected abstract int postMessage( Ditto, @IntDef and friends would make the return "type" clearer. https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... File customtabs/src/android/support/customtabs/CustomTabsSession.java (right): https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsSession.java:154: * Sends a request to the implementation for validate and assign a postMessage origin for the nit: s/for/to/ ? (not a native speaker, so I'm not 100% sure) https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsSession.java:173: * {@link CustomTabsCallback#onMessageChannelReady(Uri, Bundle)}. This method is syncronized nit: synchronized https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsSession.java:174: * to preserve the ordering of postMessage requests as they are going through the binder thread Also, I'm not sure that we need to explain why a given method is synchronized in the javadoc. What matters to app developers are the threading guarantees, not the actual implementation. Basically, the guarantee we provide is: "If postMessage() is called from a single thread, then the messages will be posted in the same order to the web page.". https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsSession.java:180: * @return An integer constant about the postMessage request result. ditto, @IntDef
https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... File customtabs/src/android/support/customtabs/CustomTabsCallback.java (right): https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsCallback.java:57: /** On 2016/11/07 17:50:38, Benoit L wrote: > Thinking about it, I wonder whether we should clarify and simplify threading > across the board here. > > The main issue is that we never made explicit on which thread the callbacks are > executed. This was probably not a huge issue until now, since we sent few > callbacks, but this is becoming worse. > > Probably the simplest solution that's consistent for applications is either > (a) to specify that there is no threading guarantees whatsoever, or > (b) to guarantee that everything happens on the UI thread. > > > Solution (a) is easier for us, but dangerous for applications. > For CustomTabsCallback#onPostMessage(), if we want to keep the model that we > have on the "web" side, then we should post the callback to the UI thread, > symmetrically to what we do in Chrome's implementation. This also has to > advantage of not blocking Chrome's UI thread if onPostMessage() is heavy. > > > What about, in CustomTabsClient, when we create the CustomTabsCallback object, > we actually make sure that everything is dispatched to the UI thread? Done. https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... File customtabs/src/android/support/customtabs/CustomTabsClient.java (right): https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsClient.java:182: ICustomTabsCallback.Stub wrapper = new ICustomTabsCallback.Stub() { On 2016/11/07 17:50:38, Benoit L wrote: > What about a proxy class: > > /** Posts the callback to the main thread. */ > private static class CustomTabsCallbackProxy extends > ICustomTabsCallback.Stub { > private final Handler mHandler; > private final CustomTabsCallback mCallback; > > public CustomTabsCallbackProxy(CustomTabsCallback callback) { > mHandler = new Handler(Looper.getMainLooper()); > mCallback = callback; > } > > @Override > public void onNavigationEvent(final int navigationEvent, final Bundle > extras) { > if (mCallback == null) return; > mHandler.post(new Runnable() { > @Override > public void run() { > mCallback.onNavigationEvent(navigationEvent, extras); > } > }); > } > > @Override > public void extraCallback(final String callbackName, final Bundle args) > throws RemoteException { > if (callback == null) return; > mHandler.post(new Runnable() { > @Override > public void run() { > mCallback.extraCallback(callbackName, args); > } > }); > } > } > > > And then, here: > CustomTabsCallbackProxy proxy = new CustomTabsCallbackProxy(callback); Done. https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... File customtabs/src/android/support/customtabs/CustomTabsService.java (right): https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsService.java:53: public static final int RESULT_SUCCESS = 1; On 2016/11/07 17:50:38, Benoit L wrote: > nit: These error codes are a bit weird. > > In the UNIX tradition, 0 means success, and the rest means that is an error. So > it seems weird that success is 1, and the other ones are errors. Done. https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsService.java:54: public static final int RESULT_FAILURE_DISALLOWED = 0; On 2016/11/07 17:50:38, Benoit L wrote: > Can you use @IntDef here? > https://developer.android.com/reference/android/support/annotation/IntDef.html > Done. https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsService.java:215: * Sends a request to the implementation for validate and assign a postMessage origin for the On 2016/11/07 17:50:38, Benoit L wrote: > nit: s/for/to/ ? Done. https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsService.java:222: * here doesn't mean an origin has already been assigned as the validation can be On 2016/11/07 17:50:38, Benoit L wrote: > nit: s/can be/is/ Done. https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsService.java:238: protected abstract int postMessage( On 2016/11/07 17:50:38, Benoit L wrote: > Ditto, @IntDef and friends would make the return "type" clearer. Done. https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... File customtabs/src/android/support/customtabs/CustomTabsSession.java (right): https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsSession.java:154: * Sends a request to the implementation for validate and assign a postMessage origin for the On 2016/11/07 17:50:38, Benoit L wrote: > nit: s/for/to/ ? > (not a native speaker, so I'm not 100% sure) Done. https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsSession.java:173: * {@link CustomTabsCallback#onMessageChannelReady(Uri, Bundle)}. This method is syncronized On 2016/11/07 17:50:38, Benoit L wrote: > nit: synchronized Done. https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsSession.java:174: * to preserve the ordering of postMessage requests as they are going through the binder thread On 2016/11/07 17:50:38, Benoit L wrote: > Also, I'm not sure that we need to explain why a given method is synchronized in > the javadoc. What matters to app developers are the threading guarantees, not > the actual implementation. > > Basically, the guarantee we provide is: > "If postMessage() is called from a single thread, then the messages will be > posted in the same order to the web page.". Done. https://codereview.chromium.org/2438103002/diff/40001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsSession.java:180: * @return An integer constant about the postMessage request result. On 2016/11/07 17:50:38, Benoit L wrote: > ditto, @IntDef Done.
Sorry, I missed things in the previous round. Mostly nits, and a comment about synchronization. https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... File customtabs/src/android/support/customtabs/CustomTabsCallback.java (right): https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsCallback.java:23: * A callback class for custom tabs client to get messages regarding events in their custom tabs. nit: Can you add that the callbacks are all dispatched to the main thread here as well? That's probably clearer. https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsCallback.java:77: * This method is syncronized to keep the ordering of messages sent across the binder thread nit: synchronized https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsCallback.java:78: * pool. When received on the client side, it is the client's responsibility to preserve the nit: Similar to postMessage(), should we actually only document the ordering guarantees? https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... File customtabs/src/android/support/customtabs/CustomTabsClient.java (right): https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsClient.java:183: public CustomTabsSession newSession(final CustomTabsCallback callback) { nit: Can be deferred to a subsequent CL, but we are now using the android support library annotations in several places, so it probably makes sense to put @NonNull and @Nullable annotations to params as we update the methods. Hopefully this is useful to Android Studio users... https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsClient.java:188: public void onNavigationEvent(int navigationEvent, Bundle extras) { Does it compile? I would expect that the parameters need to be final (here and below). https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... File customtabs/src/android/support/customtabs/CustomTabsService.java (right): https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsService.java:110: public boolean validatePostMessageOrigin(ICustomTabsCallback callback) { nit: This is likely for a future CL, but it would be nice to avoid having to create a new CustomTabsSessionToken for each message, and more generally for each service call. Since the Binder objects are comparable, we could perhaps cache the CustomTabsSessionToken objects. Or perhaps even just keep the most recent one, since we suspect that the majority of clients will have only one session. This is not important for correctness and only concerns the browser side, so I think we can safely defer that to a future CL. What do you think? https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsService.java:222: * <p>This also acts a trigger to setup a postMessage communication channel. nit: s/a trigger/as a trigger/ https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsService.java:233: * {@link CustomTabsCallback#onMessageChannelReady(Uri, Bundle)}. Returns false when called nit: This does not return a boolean, what is the error code in this case? https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... File customtabs/src/android/support/customtabs/CustomTabsSession.java (right): https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsSession.java:154: /** Actually, can the JavaDoc be removed, since it's a copy of the one from CustomTabsService? My knowledge of JavaDoc is not good enough to know the right tag to refer to the other one though... https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsSession.java:172: /** ditto https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... File customtabs/src/android/support/customtabs/CustomTabsSessionToken.java (right): https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsSessionToken.java:94: public void onMessageChannelReady(Uri origin, Bundle extras) { If we do need the extra synchronization for this method as well (see the comment below), then we should make it synchronized as well. https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... File customtabs/src/android/support/customtabs/ICustomTabsCallback.aidl (right): https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/ICustomTabsCallback.aidl:28: oneway void onMessageChannelReady(in Uri origin, in Bundle extras) = 3; Sorry, I missed that one in the previous review. If we want to make sure that the app gets a onMessageChannelReady() before receiving the first message, do we need to make this call not oneway as well? (and the method synchronized above)
https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... File customtabs/src/android/support/customtabs/CustomTabsCallback.java (right): https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsCallback.java:23: * A callback class for custom tabs client to get messages regarding events in their custom tabs. On 2016/11/08 16:27:06, Benoit L wrote: > nit: Can you add that the callbacks are all dispatched to the main thread here > as well? > That's probably clearer. Isn't that a feature of the Ibinder wrapper for this class inside CustomTabsClient, rather than the abstract class itself? https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsCallback.java:77: * This method is syncronized to keep the ordering of messages sent across the binder thread On 2016/11/08 16:27:06, Benoit L wrote: > nit: synchronized Done. https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsCallback.java:78: * pool. When received on the client side, it is the client's responsibility to preserve the On 2016/11/08 16:27:06, Benoit L wrote: > nit: Similar to postMessage(), should we actually only document the ordering > guarantees? Done. https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... File customtabs/src/android/support/customtabs/CustomTabsClient.java (right): https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsClient.java:183: public CustomTabsSession newSession(final CustomTabsCallback callback) { On 2016/11/08 16:27:06, Benoit L wrote: > nit: Can be deferred to a subsequent CL, but we are now using the android > support library annotations in several places, so it probably makes sense to put > @NonNull and @Nullable annotations to params as we update the methods. Hopefully > this is useful to Android Studio users... Acknowledged. https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsClient.java:188: public void onNavigationEvent(int navigationEvent, Bundle extras) { On 2016/11/08 16:27:06, Benoit L wrote: > Does it compile? > I would expect that the parameters need to be final (here and below). Caught me! No, it doesn't. https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... File customtabs/src/android/support/customtabs/CustomTabsService.java (right): https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsService.java:110: public boolean validatePostMessageOrigin(ICustomTabsCallback callback) { On 2016/11/08 16:27:07, Benoit L wrote: > nit: This is likely for a future CL, but it would be nice to avoid having to > create a new CustomTabsSessionToken for each message, and more generally for > each service call. > > Since the Binder objects are comparable, we could perhaps cache the > CustomTabsSessionToken objects. Or perhaps even just keep the most recent one, > since we suspect that the majority of clients will have only one session. > > This is not important for correctness and only concerns the browser side, so I > think we can safely defer that to a future CL. > > What do you think? Makes sense, and yeah since it is kind of gonna be a non-trivial change on the mechanism here, let's defer it. https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsService.java:222: * <p>This also acts a trigger to setup a postMessage communication channel. On 2016/11/08 16:27:07, Benoit L wrote: > nit: s/a trigger/as a trigger/ Done. https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsService.java:233: * {@link CustomTabsCallback#onMessageChannelReady(Uri, Bundle)}. Returns false when called On 2016/11/08 16:27:06, Benoit L wrote: > nit: This does not return a boolean, what is the error code in this case? CustomTabsService.Result (annotated below) is the return value. So it is the same error code that CustomTabsService returns. See line 117 up above. https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... File customtabs/src/android/support/customtabs/ICustomTabsCallback.aidl (right): https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/ICustomTabsCallback.aidl:28: oneway void onMessageChannelReady(in Uri origin, in Bundle extras) = 3; On 2016/11/08 16:27:07, Benoit L wrote: > Sorry, I missed that one in the previous review. > > If we want to make sure that the app gets a onMessageChannelReady() before > receiving the first message, do we need to make this call not oneway as well? > (and the method synchronized above) Even if we do that, I mean make both syncronized, that won't help with the ordering between the two methods right? That we basically have to make sure on the implementation side. I mean we can still fail to make them ordered even with these two guarantees?
https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... File customtabs/src/android/support/customtabs/CustomTabsCallback.java (right): https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsCallback.java:23: * A callback class for custom tabs client to get messages regarding events in their custom tabs. On 2016/11/08 18:59:10, Yusuf wrote: > On 2016/11/08 16:27:06, Benoit L wrote: > > nit: Can you add that the callbacks are all dispatched to the main thread here > > as well? > > That's probably clearer. > > Isn't that a feature of the Ibinder wrapper for this class inside > CustomTabsClient, rather than the abstract class itself? Yes, I was actually thinking about surfacing the guarantees in the documentation that application developers look at, which is there I guess https://developer.android.com/reference/android/support/customtabs/CustomTabs... So I think that it makes sense to point that out here as well. What do you think? https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... File customtabs/src/android/support/customtabs/CustomTabsService.java (right): https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsService.java:233: * {@link CustomTabsCallback#onMessageChannelReady(Uri, Bundle)}. Returns false when called On 2016/11/08 18:59:10, Yusuf wrote: > On 2016/11/08 16:27:06, Benoit L wrote: > > nit: This does not return a boolean, what is the error code in this case? > > CustomTabsService.Result (annotated below) is the return value. So it is the > same error code that CustomTabsService returns. See line 117 up above. The Javadoc is not correct then, "Returns false when". Sorry, the comment was not really clear. https://codereview.chromium.org/2438103002/diff/80001/Application/src/main/ja... File Application/src/main/java/org/chromium/customtabsclient/MainActivity.java (right): https://codereview.chromium.org/2438103002/diff/80001/Application/src/main/ja... Application/src/main/java/org/chromium/customtabsclient/MainActivity.java:173: PackageInfo info = getPackageManager().getPackageInfo("com.twitter.android", 0); You probably want to remove these lines. :-) https://codereview.chromium.org/2438103002/diff/80001/customtabs/src/android/... File customtabs/src/android/support/customtabs/CustomTabsClient.java (right): https://codereview.chromium.org/2438103002/diff/80001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsClient.java:185: private Handler mHandler = new Handler(Looper.getMainLooper()); nit: This can be made final as well.
https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... File customtabs/src/android/support/customtabs/CustomTabsCallback.java (right): https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsCallback.java:23: * A callback class for custom tabs client to get messages regarding events in their custom tabs. On 2016/11/09 10:11:00, Benoit L wrote: > On 2016/11/08 18:59:10, Yusuf wrote: > > On 2016/11/08 16:27:06, Benoit L wrote: > > > nit: Can you add that the callbacks are all dispatched to the main thread > here > > > as well? > > > That's probably clearer. > > > > Isn't that a feature of the Ibinder wrapper for this class inside > > CustomTabsClient, rather than the abstract class itself? > > Yes, I was actually thinking about surfacing the guarantees in the documentation > that application developers look at, which is there I guess > https://developer.android.com/reference/android/support/customtabs/CustomTabs... > > > So I think that it makes sense to point that out here as well. > What do you think? Done. https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... File customtabs/src/android/support/customtabs/CustomTabsService.java (right): https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsService.java:233: * {@link CustomTabsCallback#onMessageChannelReady(Uri, Bundle)}. Returns false when called On 2016/11/09 10:11:00, Benoit L wrote: > On 2016/11/08 18:59:10, Yusuf wrote: > > On 2016/11/08 16:27:06, Benoit L wrote: > > > nit: This does not return a boolean, what is the error code in this case? > > > > CustomTabsService.Result (annotated below) is the return value. So it is the > > same error code that CustomTabsService returns. See line 117 up above. > > The Javadoc is not correct then, "Returns false when". Sorry, the comment was > not really clear. Done. https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... File customtabs/src/android/support/customtabs/CustomTabsSession.java (right): https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsSession.java:154: /** On 2016/11/08 16:27:07, Benoit L wrote: > Actually, can the JavaDoc be removed, since it's a copy of the one from > CustomTabsService? > My knowledge of JavaDoc is not good enough to know the right tag to refer to the > other one though... Removed for now. Will ask Android reviewers. https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsSession.java:172: /** On 2016/11/08 16:27:07, Benoit L wrote: > ditto Done. https://codereview.chromium.org/2438103002/diff/80001/Application/src/main/ja... File Application/src/main/java/org/chromium/customtabsclient/MainActivity.java (right): https://codereview.chromium.org/2438103002/diff/80001/Application/src/main/ja... Application/src/main/java/org/chromium/customtabsclient/MainActivity.java:173: PackageInfo info = getPackageManager().getPackageInfo("com.twitter.android", 0); On 2016/11/09 10:11:00, Benoit L wrote: > You probably want to remove these lines. > :-) ha! https://codereview.chromium.org/2438103002/diff/80001/customtabs/src/android/... File customtabs/src/android/support/customtabs/CustomTabsClient.java (right): https://codereview.chromium.org/2438103002/diff/80001/customtabs/src/android/... customtabs/src/android/support/customtabs/CustomTabsClient.java:185: private Handler mHandler = new Handler(Looper.getMainLooper()); On 2016/11/09 10:11:00, Benoit L wrote: > nit: This can be made final as well. Done.
LGTM, thanks!
https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... File customtabs/src/android/support/customtabs/ICustomTabsCallback.aidl (right): https://codereview.chromium.org/2438103002/diff/60001/customtabs/src/android/... customtabs/src/android/support/customtabs/ICustomTabsCallback.aidl:28: oneway void onMessageChannelReady(in Uri origin, in Bundle extras) = 3; On 2016/11/08 18:59:10, Yusuf wrote: > On 2016/11/08 16:27:07, Benoit L wrote: > > Sorry, I missed that one in the previous review. > > > > If we want to make sure that the app gets a onMessageChannelReady() before > > receiving the first message, do we need to make this call not oneway as well? > > (and the method synchronized above) > > Even if we do that, I mean make both syncronized, that won't help with the > ordering between the two methods right? That we basically have to make sure on > the implementation side. I mean we can still fail to make them ordered even with > these two guarantees? Done.
The CQ bit was checked by yusufo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lizeb@chromium.org Link to the patchset: https://codereview.chromium.org/2438103002/#ps140001 (title: "removed unnecessary import")
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 postMessage APIs to the support lib Adds CustomTabsSession#validateOriginForPostMessage, CustomTabsSession#postMessage, CustomTabsCallback#onMessageChannelCreated, CustomTabsCallback#onPostMessage for sending an receiving postMessage requests to the mainframe of the initial url. BUG=657915 ========== to ========== Add postMessage APIs to the support lib Adds CustomTabsSession#validateOriginForPostMessage, CustomTabsSession#postMessage, CustomTabsCallback#onMessageChannelCreated, CustomTabsCallback#onPostMessage for sending an receiving postMessage requests to the mainframe of the initial url. BUG=657915 Committed: https://github.com/GoogleChrome/custom-tabs-client/commit/3cf33a835e4cd777cea... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://github.com/GoogleChrome/custom-tabs-client/commit/3cf33a835e4cd777cea... |
