Skip to content

Commit

Permalink
[WebNFC] Rename NDEFReader onerror to onreadingerror
Browse files Browse the repository at this point in the history
The NDEFReader onerror event handler is renamed to onreadingerror to
make it clear to web developers that only NFC read errors will be fired
in this event as NDEFReader and NDEFWriter will soon been merged. Note
that an error message is now displayed in the devtools JS console to
help web developers to diagnose issues while it was part of the error
event before.

This CL also removes onreadingerror event fired on MojoConnectionError
as this is called if either the NFC service is unavailable in which case
NDEFReader.scan() will return a rejected promise, or when NFC permission
is revoked in which case the Permission API can be used to detect
changes.

Spec: w3c/web-nfc#601
Bug: 520391
Change-Id: I451083fb5382149e9a1e45da31e575709f190448
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2546011
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: François Beaufort <beaufort.francois@gmail.com>
Cr-Commit-Position: refs/heads/master@{#829626}
GitOrigin-RevId: 5d80d77e62168984affeee6ac9e335a788a997df
  • Loading branch information
beaufortfrancois authored and copybara-github committed Nov 20, 2020
1 parent 83e6f09 commit 33ca5fa
Show file tree
Hide file tree
Showing 15 changed files with 41 additions and 58 deletions.
1 change: 1 addition & 0 deletions blink/renderer/core/events/event_type_names.json5
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,7 @@
"quotachange",
"ratechange",
"reading",
"readingerror",
"readystatechange",
"reflectionchange",
"rejectionhandled",
Expand Down
15 changes: 6 additions & 9 deletions blink/renderer/modules/nfc/ndef_reader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,12 @@

#include "services/device/public/mojom/nfc.mojom-blink.h"
#include "third_party/blink/renderer/bindings/core/v8/script_promise_resolver.h"
#include "third_party/blink/renderer/bindings/core/v8/source_location.h"
#include "third_party/blink/renderer/bindings/modules/v8/v8_ndef_scan_options.h"
#include "third_party/blink/renderer/core/dom/abort_signal.h"
#include "third_party/blink/renderer/core/dom/dom_exception.h"
#include "third_party/blink/renderer/core/events/error_event.h"
#include "third_party/blink/renderer/core/frame/local_dom_window.h"
#include "third_party/blink/renderer/core/frame/local_frame.h"
#include "third_party/blink/renderer/core/inspector/console_message.h"
#include "third_party/blink/renderer/modules/event_target_modules.h"
#include "third_party/blink/renderer/modules/nfc/ndef_message.h"
#include "third_party/blink/renderer/modules/nfc/ndef_reading_event.h"
Expand Down Expand Up @@ -198,10 +197,11 @@ void NDEFReader::OnReading(const String& serial_number,
MakeGarbageCollected<NDEFMessage>(message)));
}

void NDEFReader::OnError(const String& message) {
ErrorEvent* event = ErrorEvent::Create(
message, SourceLocation::Capture(GetExecutionContext()), nullptr);
DispatchEvent(*event);
void NDEFReader::OnReadingError(const String& message) {
DispatchEvent(*Event::Create(event_type_names::kReadingerror));
GetExecutionContext()->AddConsoleMessage(MakeGarbageCollected<ConsoleMessage>(
mojom::blink::ConsoleMessageSource::kJavaScript,
mojom::blink::ConsoleMessageLevel::kInfo, message));
}

void NDEFReader::OnMojoConnectionError() {
Expand All @@ -212,9 +212,6 @@ void NDEFReader::OnMojoConnectionError() {
kNotSupportedOrPermissionDenied));
resolver_.Clear();
}

// Dispatches an error event.
OnError(kNotSupportedOrPermissionDenied);
}

void NDEFReader::ContextDestroyed() {
Expand Down
4 changes: 2 additions & 2 deletions blink/renderer/modules/nfc/ndef_reader.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,16 +41,16 @@ class MODULES_EXPORT NDEFReader : public EventTargetWithInlineData,
// ActiveScriptWrappable overrides.
bool HasPendingActivity() const override;

DEFINE_ATTRIBUTE_EVENT_LISTENER(error, kError)
DEFINE_ATTRIBUTE_EVENT_LISTENER(reading, kReading)
DEFINE_ATTRIBUTE_EVENT_LISTENER(readingerror, kReadingerror)
ScriptPromise scan(ScriptState*, const NDEFScanOptions*, ExceptionState&);

void Trace(Visitor*) const override;

// Called by NFCProxy for dispatching events.
virtual void OnReading(const String& serial_number,
const device::mojom::blink::NDEFMessage&);
virtual void OnError(const String& message);
virtual void OnReadingError(const String& message);

// Called by NFCProxy for notification about connection error.
void OnMojoConnectionError();
Expand Down
2 changes: 1 addition & 1 deletion blink/renderer/modules/nfc/ndef_reader.idl
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
] interface NDEFReader : EventTarget {
[CallWith=ExecutionContext] constructor();
attribute EventHandler onreading;
attribute EventHandler onerror;
attribute EventHandler onreadingerror;

[CallWith=ScriptState, RaisesException] Promise<void> scan(
optional NDEFScanOptions options={});
Expand Down
6 changes: 3 additions & 3 deletions blink/renderer/modules/nfc/nfc_proxy.cc
Original file line number Diff line number Diff line change
Expand Up @@ -115,11 +115,11 @@ void NFCProxy::OnWatch(const Vector<uint32_t>& watch_ids,

void NFCProxy::OnError(device::mojom::blink::NDEFErrorPtr error) {
// Dispatch the event to all readers. We iterate on a copy of |readers_|
// because a reader's onerror event handler may remove itself from |readers_|
// just during the iteration process.
// because a reader's onreadingerror event handler may remove itself from
// |readers_| just during the iteration process.
ReaderMap copy = readers_;
for (auto& pair : copy) {
pair.key->OnError(error->error_message);
pair.key->OnReadingError(error->error_message);
}
}

Expand Down
2 changes: 2 additions & 0 deletions blink/web_tests/NeverFixTests
Original file line number Diff line number Diff line change
Expand Up @@ -1855,6 +1855,8 @@ external/wpt/visual-viewport/viewport-scale-manual.html [ Skip ]
external/wpt/visual-viewport/viewport-scroll-event-manual.html [ Skip ]
external/wpt/visual-viewport/viewport-url-bar-changes-height-manual.html [ Skip ]
external/wpt/screen-wake-lock/wakelock-document-hidden-manual.https.html [ Skip ]
external/wpt/web-nfc/NDEFReader-document-hidden-manual.https.html [ Skip ]
external/wpt/web-nfc/NDEFWriter-document-hidden-manual.https.html [ Skip ]
external/wpt/web-share/share-cancel-manual.https.html [ Skip ]
external/wpt/web-share/share-extra-argument-manual.https.html [ Skip ]
external/wpt/web-share/share-extra-field-manual.https.html [ Skip ]
Expand Down
2 changes: 1 addition & 1 deletion blink/web_tests/external/wpt/interfaces/web-nfc.idl
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ interface NDEFWriter {
interface NDEFReader : EventTarget {
constructor();

attribute EventHandler onerror;
attribute EventHandler onreading;
attribute EventHandler onreadingerror;

Promise<undefined> scan(optional NDEFScanOptions options={});
};
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,13 @@
<script>

nfc_test(async (t, mockNFC) => {
mockNFC.simulateClosedPipe();
const reader = new NDEFReader();
const readerWatcher = new EventWatcher(t, reader, ["reading", "error"]);
const promise = readerWatcher.wait_for("reading").then(event => {
if (document.hidden) reject();
resolve();
const readerWatcher = new EventWatcher(t, reader, ["reading", "readingerror"]);
const promise = new Promise((resolve, reject) => {
readerWatcher.wait_for("reading").then(event => {
if (document.hidden) reject();
else resolve();
});
});
await reader.scan();
await promise;
Expand Down
36 changes: 13 additions & 23 deletions blink/web_tests/external/wpt/web-nfc/NDEFReader_scan.https.html
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,8 @@
nfc_test(async (t, mockNFC) => {
mockNFC.simulateClosedPipe();
const reader = new NDEFReader();
const readerWatcher = new EventWatcher(t, reader, ["reading", "error"]);
const promise = readerWatcher.wait_for("error").then(event => {
assert_true(event instanceof ErrorEvent);
});
await promise_rejects_dom(t, 'NotSupportedError', reader.scan());
await promise;
}, "Test that an error event happens if no implementation for NFC Mojo interface \
is available.");
}, "NDEFReader.scan should faild if no implementation for NFC Mojo interface.");

nfc_test(async (t, mockNFC) => {
mockNFC.setHWStatus(NFCHWStatus.DISABLED);
Expand All @@ -70,7 +64,7 @@
nfc_test(async (t, mockNFC) => {
const reader = new NDEFReader();
const controller = new AbortController();
const readerWatcher = new EventWatcher(t, reader, ["reading", "error"]);
const readerWatcher = new EventWatcher(t, reader, ["reading", "readingerror"]);
const promise = readerWatcher.wait_for("reading").then(event => {
assert_true(event instanceof NDEFReadingEvent);
controller.abort();
Expand All @@ -84,7 +78,7 @@
nfc_test(async (t, mockNFC) => {
const reader = new NDEFReader();
const controller = new AbortController();
const readerWatcher = new EventWatcher(t, reader, ["reading", "error"]);
const readerWatcher = new EventWatcher(t, reader, ["reading", "readingerror"]);
const promise = readerWatcher.wait_for("reading").then(event => {
assert_true(event instanceof NDEFReadingEvent);
controller.abort();
Expand Down Expand Up @@ -114,7 +108,7 @@
nfc_test(async (t, mockNFC) => {
const reader = new NDEFReader();
const controller = new AbortController();
const readerWatcher = new EventWatcher(t, reader, ["reading", "error"]);
const readerWatcher = new EventWatcher(t, reader, ["reading", "readingerror"]);
const message = createMessage([createTextRecord(test_text_data)]);
const promise = readerWatcher.wait_for("reading").then(event => {
assert_true(event instanceof NDEFReadingEvent);
Expand All @@ -135,7 +129,7 @@
nfc_test(async (t, mockNFC) => {
const reader = new NDEFReader();
const controller = new AbortController();
const readerWatcher = new EventWatcher(t, reader, ["reading", "error"]);
const readerWatcher = new EventWatcher(t, reader, ["reading", "readingerror"]);
const promise = readerWatcher.wait_for("reading").then(event => {
controller.abort();
assert_true(event instanceof NDEFReadingEvent);
Expand Down Expand Up @@ -172,7 +166,7 @@
nfc_test(async (t, mockNFC) => {
const reader = new NDEFReader();
const controller = new AbortController();
const readerWatcher = new EventWatcher(t, reader, ["reading", "error"]);
const readerWatcher = new EventWatcher(t, reader, ["reading", "readingerror"]);
const promise = readerWatcher.wait_for("reading").then(event => {
controller.abort();
assert_true(event instanceof NDEFReadingEvent);
Expand Down Expand Up @@ -225,30 +219,26 @@
const promises = [];

const reader1 = new NDEFReader();
const readerWatcher1 = new EventWatcher(t, reader1, ["reading", "error"]);
const promise1 = readerWatcher1.wait_for("error").then(event => {
assert_true(event instanceof ErrorEvent);
});
const readerWatcher1 = new EventWatcher(t, reader1, ["reading", "readingerror"]);
const promise1 = readerWatcher1.wait_for("readingerror");
promises.push(promise1);
await reader1.scan();

const reader2 = new NDEFReader();
const readerWatcher2 = new EventWatcher(t, reader2, ["reading", "error"]);
const promise2 = readerWatcher2.wait_for("error").then(event => {
assert_true(event instanceof ErrorEvent);
});
const readerWatcher2 = new EventWatcher(t, reader2, ["reading", "readingerror"]);
const promise2 = readerWatcher2.wait_for("readingerror");
promises.push(promise2);
await reader2.scan();

mockNFC.simulateNonNDEFTagDiscovered();
await Promise.all(promises);
}, "Test that NDEFReader.onerror should be fired if the NFC tag does not \
}, "Test that NDEFReader.onreadingerror should be fired if the NFC tag does not \
expose NDEF technology.");

nfc_test(async (t, mockNFC) => {
const reader = new NDEFReader();
const controller = new AbortController();
const readerWatcher = new EventWatcher(t, reader, ["reading", "error"]);
const readerWatcher = new EventWatcher(t, reader, ["reading", "readingerror"]);
const promise = readerWatcher.wait_for("reading").then(event => {
assert_equals(event.serialNumber, fake_tag_serial_number);
assert_equals(event.message.records.length, 0);
Expand All @@ -272,7 +262,7 @@
createUrlRecord(test_url_data, true),
createRecord('w3.org:xyz', test_buffer_data)],
test_message_origin);
const readerWatcher = new EventWatcher(t, reader, ["reading", "error"]);
const readerWatcher = new EventWatcher(t, reader, ["reading", "readingerror"]);
const promise = readerWatcher.wait_for("reading").then(event => {
assert_equals(event.serialNumber, fake_tag_serial_number);
assertWebNDEFMessagesEqual(event.message, new NDEFMessage(message));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
nfc_test(async (t, mockNFC) => {
const reader = new NDEFReader();
const controller = new AbortController();
const readerWatcher = new EventWatcher(t, reader, ["reading", "error"]);
const readerWatcher = new EventWatcher(t, reader, ["reading", "readingerror"]);

const promise = readerWatcher.wait_for("reading").then(event => {
assert_true(event instanceof NDEFReadingEvent);
Expand Down

This file was deleted.

4 changes: 2 additions & 2 deletions blink/web_tests/external/wpt/web-nfc/resources/nfc-helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ function testMultiScanOptions(message, scanOptions, unmatchedScanOptions, desc)
unmatchedScanOptions.signal = controller.signal;
await reader1.scan(unmatchedScanOptions);

const readerWatcher = new EventWatcher(t, reader2, ["reading", "error"]);
const readerWatcher = new EventWatcher(t, reader2, ["reading", "readingerror"]);
const promise = readerWatcher.wait_for("reading").then(event => {
controller.abort();
assertWebNDEFMessagesEqual(event.message, new NDEFMessage(message));
Expand All @@ -221,7 +221,7 @@ function testMultiMessages(message, scanOptions, unmatchedMessage, desc) {
nfc_test(async (t, mockNFC) => {
const reader = new NDEFReader();
const controller = new AbortController();
const readerWatcher = new EventWatcher(t, reader, ["reading", "error"]);
const readerWatcher = new EventWatcher(t, reader, ["reading", "readingerror"]);
const promise = readerWatcher.wait_for("reading").then(event => {
controller.abort();
assertWebNDEFMessagesEqual(event.message, new NDEFMessage(message));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
OriginTrialsHelper.check_properties_exist(this,
{
'NDEFMessage': ['records'],
'NDEFReader': ['scan', 'onreading', 'onerror'],
'NDEFReader': ['scan', 'onreading', 'onreadingerror'],
'NDEFReadingEvent': ['serialNumber', 'message'],
'NDEFRecord': ['recordType', 'mediaType', 'id', 'encoding', 'lang', 'data', 'toRecords'],
'NDEFWriter': ['write'],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5377,12 +5377,12 @@ interface NDEFMessage
method constructor
interface NDEFReader : EventTarget
attribute @@toStringTag
getter onerror
getter onreading
getter onreadingerror
method constructor
method scan
setter onerror
setter onreading
setter onreadingerror
interface NDEFReadingEvent : Event
attribute @@toStringTag
getter message
Expand Down

0 comments on commit 33ca5fa

Please sign in to comment.