Handle BOMs in dart2js.

sdk/lib/_internal/compiler/implementation/scanner/utf8_bytes_scanner.dart

Index: sdk/lib/_internal/compiler/implementation/scanner/utf8_bytes_scanner.dart
diff --git a/sdk/lib/_internal/compiler/implementation/scanner/utf8_bytes_scanner.dart b/sdk/lib/_internal/compiler/implementation/scanner/utf8_bytes_scanner.dart
index 62722ea6f7d0357831bf497801eca8244806f53f..eb3e7cd5b1d299ea734e1a391acf31e1e3e25665 100644
--- a/sdk/lib/_internal/compiler/implementation/scanner/utf8_bytes_scanner.dart
+++ b/sdk/lib/_internal/compiler/implementation/scanner/utf8_bytes_scanner.dart
@@ -69,6 +69,8 @@ class Utf8BytesScanner extends ArrayBasedScanner {
       : bytes = file.slowUtf8Bytes(),
         super(file, includeComments) {
     ensureZeroTermination();
+    // Skip a leading BOM.
+    if (_containsBomAt(0)) byteOffset += 3;
   }
 
   /**
@@ -95,6 +97,23 @@ class Utf8BytesScanner extends ArrayBasedScanner {
     }
   }
 
+  bool _containsBomAt(int offset) {
+    const BOM_UTF8 = const [0xEF, 0xBB, 0xBF];
+
+    assert(() {

[ahe, 2014/01/15 13:15:38]

I would probably add a unit test for this instead of an assert.

If you want a fast scanner, it should be so fast that it is done before an
optimizing JIT notices what is going on. This means that you should keep the
code simple, for example, by keeping the number of tokens low.

We know that dart2js performance is affected by how large the snapshot is
(because there will be less heap space available and more GCs).  By being frugal
about asserts, you help keep the snapshot smaller.

[floitsch, 2014/01/15 13:44:46]

There is already a test.
Removing the assert.

+      // Verify that BOM_UTF8 corresponds to the UTF-8 BOM character.
+      // The UTF-8 decoder strips leading BOMs. Add another character in front.
+      List<int> prefixedBom = [0x20]..addAll(BOM_UTF8);
+      String decoded = UTF8.decode(prefixedBom);
+      return decoded.codeUnitAt(1) == UNICODE_BOM_CHARACTER_RUNE;
+    });
+
+    return offset + 3 < bytes.length &&
+        bytes[offset] == BOM_UTF8[0] &&
+        bytes[offset + 1] == BOM_UTF8[1] &&
+        bytes[offset + 2] == BOM_UTF8[2];
+  }
+
   int advance() => bytes[++byteOffset];
 
   int peek() => bytes[byteOffset + 1];
@@ -120,6 +139,13 @@ class Utf8BytesScanner extends ArrayBasedScanner {
     // TODO(lry): measurably slow, decode creates first a Utf8Decoder and a
     // _Utf8Decoder instance. Also the sublist is eagerly allocated.
     String codePoint = UTF8.decode(bytes.sublist(startOffset, end));
+    if (codePoint.length == 0) {
+      // The UTF-8 decoder discards leading BOM characters.
+      // TODO(floitsch): don't just assume that removed characters were the
+      // BOM.
+      assert(_containsBomAt(startOffset));
+      codePoint = "\u{FEFF}";

[ahe, 2014/01/14 14:23:59]

Can you use UNICODE_BOM_CHARACTER_RUNE somehow?

[floitsch, 2014/01/14 18:37:58]

I don't think so.
I don't want to recreate a string every time. I could add an assert, though:
assert(codePoint == new String.fromCharCode(UNICODE_BOM_CHARACTER_RUNE));

should I?

[ahe, 2014/01/15 13:15:38]

I would expect that it is extremely rare if a file contains more than zero BOM.
I would suggest writing a test instead of using asserts.

[floitsch, 2014/01/15 13:44:46]

I will allocate a new string.
I prefer keeping the assert here. It is a code-path that is executed very
rarely.
Test already exists.

[ahe, 2014/01/15 13:54:23]

Yes. Keep the assert on line 146. That assert tests an invariant about code that
is out of your control.

Alternatively, turn it into an internal error.
My suggestion is this:

const String UNICODE_BOM_CHARACTER = "\u{FEFF}";

And then this test:

main() {
  Expect.stringEquals(new String.fromCharCode(UNICODE_BOM_CHARACTER_RUNE),
UNICODE_BOM_CHARACTER);
}

[floitsch, 2014/01/15 14:51:39]

I see.
As you say: there shouldn't be many BOMs inside the code. So keeping the simpler
new String.fromCharCode.

+    }
     if (codePoint.length == 1) {
       if (advance) {
         utf8Slack += (numBytes - 1);