New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 823301 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Last visit > 30 days ago
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment

Annotate or use template classes for InvocationHandlers in the webview support library?

Project Member Reported by gsennton@chromium.org, Mar 19 2018

Issue description

When using InvocationHandlers representing many different objects it's very easy to make mistakes by creating an InvocationHandler for the wrong object, e.g. the method

class SupportLibServiceWorkerControllerAdapter:
   public InvocationHandler getServiceWorkerWebSettings()

should return an InvocationHandler representing ServiceWorkerWebSettingsBoundaryInterface, but we currently have no (compile-time) mechanism to ensure the returned InvocationHandler represents a ServiceWorkerWebSettingsBoundaryInterface.

You could for example implement getServiceWorkerWebSettings in the following way without compilation errors:


public InvocationHandler getServiceWorkerWebSettings() {
    return BoundaryInterfaceReflectionUtil.createInvocationHandlerFor(
            mAwServiceWorkerController.getAwServiceWorkerSettings()); // Wrong, this is an AwServiceWorkerSettings object, not a ServiceWorkerWebSettingsBoundaryInterface
}


One possibility here would be to have BoundaryInterfaceReflectionUtil.createInvocationHandlerFor return a new class implementing InvocationHandler:
TemplatedInvocationHandler<BoundaryInterface>
which declares a specific boundary interface as template, to ensure we type-check InvocationHandlers.
An alternative could be to use some kind of annotation for each invocationhandler.

Nate, WDYT?
 
I proposed handling this with a presubmit check ( issue 842022 ). I think that's the simplest solution.

One issue with TemplatedInvocationHandler is that it isn't a framework type, so we must declare it in the boundary interfaces. At runtime, this will be loaded under both ClassLoaders, so we'll need to convert across ClassLoaders. This would re-introduce the (currently redundant) runtime work I'm trying to remove in  issue 837820 .
Agree with that statement - we should keep the InvocationHandlers clean

Comment 3 by torne@chromium.org, May 11 2018

Yeah I think the presubmit check is the best approach rather than making the types more complex here.
Status: WontFix (was: Assigned)
Marking this as Won't fix given the presubmit check approach.

Sign in to add a comment