New issue
Advanced search Search tips

Issue 680924 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Task

Blocked on:
issue 689501
issue 706407

Blocking:
issue 612328



Sign in to add a comment

Content Modularization Project: NFC

Project Member Reported by blundell@chromium.org, Jan 13 2017

Issue description

Tracking bug for the NFC part of the Content Modularization Project. The end goal should be to have a self-contained service. This means that the nfc.mojom interface should be hosted by the Device Service rather than by //content/browser.
 
There is a design doc for this effort: https://docs.google.com/document/d/1AzEZ9unVnbs7mMCEKcqH3xOzHi6hQm7UXqxA_McHQqw/edit#

If anyone other than me works on this, they should read that design doc before doing anything else, as it explains the challenges posed by this project and presents an initial design for solving them.
Labels: DeviceService
Blockedon: 689501
Blockedon: 706407
Status: Started (was: Assigned)
Project Member

Comment 6 by bugdroid1@chromium.org, May 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b167a1b1183710ff042fd72c8ca53aae3bb81bd3

commit b167a1b1183710ff042fd72c8ca53aae3bb81bd3
Author: blundell <blundell@chromium.org>
Date: Mon May 15 09:08:28 2017

[NFC] Only connect to NFC impl if ExecutionContext supports it

NFC has the spec property that it can only be accessed from the main
frame. Currently, NFC.cpp unconditionally connects to the NFC impl in
its constructor and then implements this spec by disallowing requests
made in the context of nested frames in the implementations of the IDL
invocations (e.g., push).

An upcoming refactor of NFC for servicification would be eased by the
simplifying property that the browser receives NFC requests only from
the main frame. This CL adds this property by having NFC.cpp connect
to the NFC implementation only if its ExecutionContext supports it.
This change will have no user-visible effect (all of the IDL invocations
will fail in the same way as they did before, as the CL does not change
the checks that they are making), but makes the servicification task
significantly easier.

BUG= 680924 

Review-Url: https://codereview.chromium.org/2875283002
Cr-Commit-Position: refs/heads/master@{#471707}

[modify] https://crrev.com/b167a1b1183710ff042fd72c8ca53aae3bb81bd3/third_party/WebKit/Source/modules/nfc/NFC.cpp
[modify] https://crrev.com/b167a1b1183710ff042fd72c8ca53aae3bb81bd3/third_party/WebKit/Source/modules/nfc/NFC.h

Project Member

Comment 7 by bugdroid1@chromium.org, May 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f5316fccc7bac30a357a1254aef8dc4a605bc045

commit f5316fccc7bac30a357a1254aef8dc4a605bc045
Author: blundell <blundell@chromium.org>
Date: Mon May 15 11:49:03 2017

[Device Service] Decouple NFC implementation from //content

This CL ports the NFC implementation from being hosted by //content
to being hosted by the Device Service. This task is challenging because
the NFC implementation needs access to the Activity of the WebContents
that is associated with the requesting render frame. Moreore, that
Activity can change if the WebContents is reparented (e.g., moved from
being in a custom tab to being in Chrome by the user).

To handle these challenges, this CL takes a similar approach to that
taken for WakeLock: information is passed to the Device Service that
allows the NFC implementation to map a host ID to the Activity associated
with that ID. Specifically:

- The NFCProvider Mojo interface is introduced. It supports connecting to
  an NFC instance associated with a given host ID.
- The NFCDelegate.java interface is introduced. This interface supports
  mapping from host IDs to Activity, including tracking when the Activity
  associated with a host ID changes. This interface is passed into the
  Device Service by its embedder.
- NfcImpl.java uses the NFCDelegate to access the Activity that it needs.
- //content provides a ContentNFCDelegate implementation. This implementation
  maps host IDs to WebContents instances via an NfcHost internal helper
  class.
- //content/browser now handles requests to connect to NFC by forwarding
  them on to the Device Service (i.e., NFCProvider), passing the host
  ID associated with the WebContents in question in order to allow the
  NFC implementation to map back to that WebContents via NFCDelegate.

The net sum of the above is that //content's dependence on the NFC
implementation is removed.

Note that in addition to the below-described test, I also tested reparenting as
follows:
- Close Chromium on the device
- On the host, execute:
  adb shell "am start -a 'android.intent.action.VIEW' -n org.chromium.chrome/com.google.android.apps.chrome.Main -d https://colinblundell.github.io/nfc.html --es \"android.support.customtabs.extra.SESSION\" \"bla\""
- On the phone, you should see a custom tab pop up. Push a message.
- Reparent the tab by tapping "Open in Chromium." Verify that the tab was
  reparented, i.e., there was no reload operation.
- Bring the device close to the NFC tag and verify that you see "Message
  pushed" on the webpage.

BUG= 680924 
TEST=On Chrome on Android, go to about://flags and turn on "Experimental Web
Platform features" and "WebNFC Android". Restart Chrome. Obtain a physical
NFC tag.  Go to https://colinblundell.github.io/nfc.html and verify that the
page says "NFC API found". Enter a message on the webpage and press "Push
message to tag". Bring the phone near the tag and verifiy that the page outputs
"Message pushed." Press "Read data from tag", bring the phone near the tag,
and verify that the webpage outputs the message that you just pushed to the
tag.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2865653002
Cr-Commit-Position: refs/heads/master@{#471727}

[modify] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/content/browser/BUILD.gn
[modify] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/content/browser/android/content_feature_list.cc
[add] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/content/browser/android/nfc_host.cc
[add] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/content/browser/android/nfc_host.h
[modify] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/content/browser/frame_host/render_frame_host_delegate.cc
[modify] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/content/browser/frame_host/render_frame_host_delegate.h
[modify] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/content/browser/frame_host/render_frame_host_impl.h
[modify] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/content/browser/service_manager/service_manager_context.cc
[modify] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/content/public/android/BUILD.gn
[modify] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/content/public/android/java/src/org/chromium/content/browser/ContentFeatureList.java
[add] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/content/public/android/java/src/org/chromium/content/browser/ContentNfcDelegate.java
[modify] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/content/public/android/java/src/org/chromium/content/browser/InterfaceRegistrarImpl.java
[delete] https://crrev.com/d0458b1c5e117db5cb8b63688ef39af43fd4fc97/content/public/android/java/src/org/chromium/content/browser/NfcFactory.java
[add] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/content/public/android/java/src/org/chromium/content/browser/NfcHost.java
[modify] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/content/public/app/mojo/content_browser_manifest.json
[modify] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/device/BUILD.gn
[modify] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/device/nfc/BUILD.gn
[modify] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/device/nfc/android/BUILD.gn
[modify] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/device/nfc/android/java/src/org/chromium/device/nfc/NfcImpl.java
[add] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/device/nfc/android/java/src/org/chromium/device/nfc/NfcProviderImpl.java
[modify] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/device/nfc/android/java/src/org/chromium/device/nfc/NfcTagHandler.java
[modify] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/device/nfc/android/junit/src/org/chromium/device/nfc/NFCTest.java
[modify] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/device/nfc/nfc.mojom
[add] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/device/nfc/nfc_provider.mojom
[add] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/device/nfc/public/java/BUILD.gn
[add] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/device/nfc/public/java/src/org/chromium/device/nfc/NfcDelegate.java
[modify] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/services/device/BUILD.gn
[modify] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/services/device/android/java/src/org/chromium/services/device/InterfaceRegistrar.java
[modify] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/services/device/device_service.cc
[modify] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/services/device/device_service.h
[modify] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/services/device/device_service_test_base.cc
[modify] https://crrev.com/f5316fccc7bac30a357a1254aef8dc4a605bc045/services/device/manifest.json

Comment 8 by leon....@intel.com, May 17 2017

Cc: leon....@intel.com

Comment 9 by leon....@intel.com, Jun 19 2017

Status: Fixed (was: Started)
Components: Internals>Services>Device
Labels: Type-Task
Migrating S13N meta bugs to Type=Task, so that they can be distinguished from technical work.

Sign in to add a comment