Extended error reporting for SVG attribute parsing

third_party/WebKit/Source/core/svg/SVGParsingError.cpp

Index: third_party/WebKit/Source/core/svg/SVGParsingError.cpp
diff --git a/third_party/WebKit/Source/core/svg/SVGParsingError.cpp b/third_party/WebKit/Source/core/svg/SVGParsingError.cpp
new file mode 100644
index 0000000000000000000000000000000000000000..cb072e1b2ebeff1d430dd8ed36b544e9e544c063
--- /dev/null
+++ b/third_party/WebKit/Source/core/svg/SVGParsingError.cpp
@@ -0,0 +1,103 @@
+// 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.
+
+#include "core/svg/SVGParsingError.h"
+
+#include "core/dom/QualifiedName.h"
+#include "platform/JSONValues.h"
+#include "wtf/text/CharacterNames.h"
+#include "wtf/text/StringBuilder.h"
+
+#include <utility>
+
+namespace blink {
+
+namespace {
+
+void appendErrorContextInfo(StringBuilder& builder, const String& tagName, const QualifiedName& name)
+{
+    builder.append('<');
+    builder.append(tagName);
+    builder.appendLiteral("> attribute ");
+    builder.append(name.toString());
+}
+
+std::pair<const char*, const char*> messageForStatus(SVGStatus status)
+{
+    switch (status) {
+    case SVGStatus::TrailingGarbage:
+        return std::make_pair("Trailing garbage, ", ".");
+    case SVGStatus::ExpectedBoolean:
+        return std::make_pair("Expected 'true' or 'false', ", ".");
+    case SVGStatus::ExpectedEnumeration:
+        return std::make_pair("Unrecognized enumerated value, ", ".");
+    case SVGStatus::NegativeValue:
+        return std::make_pair("A negative value is not valid. (", ")");
+    case SVGStatus::ParsingFailed:
+    default:
+        ASSERT_NOT_REACHED();
+        break;
+    }
+    return std::make_pair("", "");
+}
+
+bool disableLocus(SVGStatus status)
+{
+    // Disable locus for semantic errors and generic errors (see TODO below).
+    return status == SVGStatus::NegativeValue
+        || status == SVGStatus::ParsingFailed;
+}
+
+void appendValue(StringBuilder& builder, SVGParsingError error, const AtomicString& value)
+{
+    builder.append('"');
+    if (!error.hasLocus() || disableLocus(error.status())) {
+        escapeStringForJSON(value.string(), &builder);
+    } else {
+        // Emit a string on the form: '"[...]<before><after>[...]"'
+        unsigned locus = error.locus();
+        ASSERT(locus >= 0 && locus <= value.length());

[f(malita), 2016/01/15 20:03:38]

nit: first part is obvious for unsigned.

[fs, 2016/01/15 20:46:14]

Indeed, will fix.

[fs, 2016/01/18 13:56:51]

Done.

+
+        // Amount of context to show before/after the error.
+        const unsigned kContext = 16;
+
+        unsigned contextStart = std::max(locus, kContext) - kContext;
+        unsigned contextEnd = std::min(locus + kContext, value.length());

[f(malita), 2016/01/15 20:03:38]

nit: I think these are safe, but just for sanity, maybe

  ASSERT(contextStart <= contextEnd);
  ASSERT(contextEnd <= value.length());
?

[fs, 2016/01/15 20:46:14]

String::substring is pretty resilient to weird ranges (and these are unsigned,
so underflow won't hurt any kittens.) I'll throw in the asserts for good measure
though.

[fs, 2016/01/18 13:56:51]

Added.

+        if (contextStart != 0)
+            builder.append(horizontalEllipsisCharacter);
+        escapeStringForJSON(value.string().substring(contextStart, contextEnd - contextStart), &builder);

[f(malita), 2016/01/15 20:03:38]

So the parse error is somewhere within this (potentially) 32-char string. 
Ideally we'd have some indication of exactly which char the parser choked on,
but not sure what's the best way for that.  We can't exactly add another line
with a caret pointing up at it (^, a-la clang compile errors), and adding
arbitrary markers around it (say "foo>Z<bar") would just be... arbitrary.  So
yeah, no bright ideas here.  

[fs, 2016/01/15 20:46:14]

Yeah, initially I had this displaying a caret using a "pipe" ('|') and then
played around with some fancy unicode characters (like U+21e5 [1]), but sort of
concluded that that might just be confusing. I tried just adding ', at <index>
"..."' to the message but didn't quite like that either. My best idea (and plan)
so far is to add a "highlight range" to ConsoleMessage and then apply those in
the console rendering. Haven't looked into that yet though, and it's bound to
slip onto the backburner (I need to get off this side-track soonish =)).

[1] http://www.fileformat.info/info/unicode/char/21e5/index.htm

+        if (contextEnd != value.length())
+            builder.append(horizontalEllipsisCharacter);
+    }
+    builder.append('"');
+}
+
+}
+
+String SVGParsingError::format(const String& tagName, const QualifiedName& name, const AtomicString& value) const
+{
+    StringBuilder builder;
+
+    // TODO(fs): Remove this case once enough specific errors have been added.
+    if (status() == SVGStatus::ParsingFailed) {
+        builder.appendLiteral("Invalid value for ");
+        appendErrorContextInfo(builder, tagName, name);
+        builder.append('=');
+        appendValue(builder, *this, value);
+    } else {
+        appendErrorContextInfo(builder, tagName, name);
+        builder.appendLiteral(": ");
+
+        if (hasLocus() && locus() == value.length())
+            builder.appendLiteral("Unexpected end of attribute. ");
+
+        auto message = messageForStatus(status());
+        builder.append(message.first);
+        appendValue(builder, *this, value);
+        builder.append(message.second);
+    }
+    return builder.toString();
+}
+
+} // namespace blink