[Device Service] Decouple NFC implementation from //content

content/public/android/java/src/org/chromium/content/browser/NfcHost.java

Index: content/public/android/java/src/org/chromium/content/browser/NfcHost.java
diff --git a/content/public/android/java/src/org/chromium/content/browser/NfcHost.java b/content/public/android/java/src/org/chromium/content/browser/NfcHost.java
new file mode 100644
index 0000000000000000000000000000000000000000..fd6096bb90f4508b783b503f74659d5147fc7f7c
--- /dev/null
+++ b/content/public/android/java/src/org/chromium/content/browser/NfcHost.java
@@ -0,0 +1,104 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.content.browser;
+
+import android.app.Activity;
+import android.util.SparseArray;
+
+import org.chromium.base.Callback;
+import org.chromium.base.annotations.CalledByNative;
+import org.chromium.content_public.browser.WebContents;
+import org.chromium.content_public.browser.WebContentsObserver;
+import org.chromium.ui.base.WindowAndroid;
+
+/**
+ * Tracks the Activiy for a given WebContents on behalf of a NFC instance that
+ * cannot talk directly to WebContents.
+ */
+class NfcHost extends WebContentsObserver implements WindowAndroidChangedObserver {
+    // The WebContents with which this host is associated.
+    private WebContents mWebContents;
+    private ContentViewCore mContentViewCore;
+
+    // The callback that the NFC instance has registered for being notified
+    // when the Activity changes.
+    private Callback<Activity> mCallback;
+
+    // The context ID with which this host is associated.
+    private int mContextId;
+
+    private static final SparseArray<NfcHost> mContextHostsMap = new SparseArray<NfcHost>();

[Ted C, 2017/05/10 14:37:41]

non-constant static in java start with a lowercase s instead of m

I would put statics at the top of the file as well.

[blundell, 2017/05/11 15:13:52]

Done.

+
+    /**
+     * Provides access to NfcHost via context ID.
+     */
+    public static NfcHost fromContextId(int contextId) {
+        return mContextHostsMap.get(contextId);
+    }
+
+    @CalledByNative
+    private static NfcHost create(WebContents webContents, int contextId) {
+        return new NfcHost(webContents, contextId);
+    }
+
+    NfcHost(WebContents webContents, int contextId) {

[Ted C, 2017/05/10 14:37:41]

you'll need to do:

super(webContents)

Otherwise, the webcontentsobserver#destroy will never be called.

[blundell, 2017/05/11 15:13:52]

Done.

+        mCallback = null;

[Ted C, 2017/05/10 14:37:41]

no need for this, it will default to null in java

[blundell, 2017/05/11 15:13:52]

Done.

+        mWebContents = webContents;

[Ted C, 2017/05/10 14:37:41]

looks like mWebContents, mContentViewCore, and mContextId can all be final

[blundell, 2017/05/11 15:13:52]

Done.

+        mContentViewCore = ContentViewCore.fromWebContents(mWebContents);
+        mContextId = contextId;
+        mContextHostsMap.put(mContextId, this);
+    }
+
+    /** Called by the NFC implementation (via ContentNfcDelegate) to allow

[Ted C, 2017/05/10 14:37:41]

If you have a /** comment header, it should be on its own line

You can also wrap comment in java to 100 chars

[blundell, 2017/05/11 15:13:52]

Done.
 
Thanks, working on getting my vim configuration to wrap java at 100 chars and
then will rewrap the comments here.

+     * that implementation to track changes to the Activity associated with
+     * its context ID (i.e., the activity associated with |mWebContents|).
+     */
+    public void trackActivityChanges(Callback<Activity> callback) {
+        // Only the main frame is allowed to access NFC
+        // (https://w3c.github.io/web-nfc/#security-policies). The renderer
+        // enforces this by dropping connection requests from nested frames.
+        // Therefore, this class should never see a request to track activity
+        // changes while there is already such a request.
+        assert mCallback == null : "Unexpected request to track activity changes";
+        mCallback = callback;
+        Activity activity = null;
+
+        if (mContentViewCore != null) {

[Ted C, 2017/05/10 14:37:41]

What is the timeline of the NfcHost?  I'm more wondering if the constructor vs
trackActivityChanges will happen far enough apart that we could lazily build the
reference to CVC here.

If CVC is null, NFC really won't work, so I'm wondering if that should be more
explicit (i.e. should the Delegate have a "supportsNfc" or something that this
would essentially check for a null CVC.)  If null isn't valid and we expect that
never to happen for this, we should add an assert for not null in the
constructor for CVC

The problem I see is that if a CVC is ever constructed for the web contents
after the NfcHost is built, then this would never get notified.  I don't really
think that is drastically different than the existing behavior though.

But if we think trackActivityChanges happens much later than the constructor, we
could consider setting mContentViewCore here (and null in stop).

[blundell, 2017/05/11 15:13:52]

NfcHost is constructed in response to a request to connect to NFC from the
renderer. If the CVC isn't constructed at this point, I don't see a reason that
it would necessarily be created when the NfcHost subsequently receives the
request from the NFC impl to track Activity changes (that request is sent by
NfcImpl on its creation). 

Note that in the codebase as it currently stands, the NFC factory checks whether
the CVC is null on receiving a NFC connection request from the renderer and
drops the request if so. I didn't preserve this behavior, as you noted. What is
the relationship between CVC and WebContents on Android? i.e., in what cases can
there not be a CVC for a WebContents instance? It would be nice to not make this
code more complex than it has to be.

[Ted C, 2017/05/11 16:36:49]

CVC has historically been the glue between the webcontents and the android view
system (although slowly changing to be closer to other platforms).  Cases like
pre-render wouldn't have CVC.

Any time a webcontents is created by the renderer, a CVC would only be created
once the browser side is aware of it.

I know there are people investigating other background loading options on
Android,
and I'm not sure if they will have a CVC or not.  Is NFC gated by any user
interaction?  If so, then I think it would be safe to assume a CVC (in general,
I
think NFC could probably get away with assuming CVC regardless).

If we have the assert above that it is non-null, then let's remove the null
check
here and crash so we can address it if needed.

[blundell, 2017/05/12 11:01:14]

Makes sense to me. I added a comment explaining the current assumption and that
we would handle the case where there is no CVC by simply dropping the request if
that case ever comes to pass.

It looks like by the spec (and enforced by Blink) NFC is only allowed to be used
when the page is visible, which implies to me based on your explanation that
there *will* always be a CVC.

+            mContentViewCore.addWindowAndroidChangedObserver(this);
+            if (mContentViewCore.getWindowAndroid() != null) {
+                activity = mContentViewCore.getWindowAndroid().getActivity().get();
+            }
+        }
+
+        mCallback.onResult(activity);
+    }
+
+    public void stopTrackingActivityChanges() {

[Ted C, 2017/05/10 14:37:41]

javadoc non-private methods

Although here, I suspect you could so something as simple as:

/**
 * @see org.chromium.device.nfc.NfcDelegate#stopTrackingActivityForHost(int)
 */ 

[blundell, 2017/05/11 15:13:52]

Done.

+        mCallback = null;
+        if (mContentViewCore != null) {
+            mContentViewCore.removeWindowAndroidChangedObserver(this);
+        }
+    }
+
+    @Override
+    public void destroy() {
+        mContextHostsMap.remove(mContextId);
+        mCallback = null;

[Ted C, 2017/05/10 14:37:41]

maybe replace these three following lines with stopTrackingActivityChanges?

[blundell, 2017/05/11 15:13:52]

Done.

+        if (mContentViewCore != null) {
+            mContentViewCore.removeWindowAndroidChangedObserver(this);
+        }
+        super.destroy();
+    }
+
+    @Override
+    public void onWindowAndroidChanged(WindowAndroid newWindowAndroid) {
+        Activity activity = null;
+        if (newWindowAndroid != null) {
+            activity = newWindowAndroid.getActivity().get();
+        }
+        assert mCallback != null : "should have callback";
+        mCallback.onResult(activity);
+    }
+}