Improve warnings for non-standard meta tag separators

third_party/WebKit/Source/core/html/HTMLMetaElement-in.cpp

Index: third_party/WebKit/Source/core/html/HTMLMetaElement-in.cpp
diff --git a/third_party/WebKit/Source/core/html/HTMLMetaElement-in.cpp b/third_party/WebKit/Source/core/html/HTMLMetaElement-in.cpp
index 2a1ff8af2e04771130bbe9b77584e745b36f5a55..b65fe1e85d9e24cd3fe49cd63ff828698fd97b12 100644
--- a/third_party/WebKit/Source/core/html/HTMLMetaElement-in.cpp
+++ b/third_party/WebKit/Source/core/html/HTMLMetaElement-in.cpp
@@ -73,7 +73,7 @@ static bool isSeparator(UChar c)
 
 void HTMLMetaElement::parseContentAttribute(const String& content, void* data, Document* document, bool viewportMetaZeroValuesQuirk)
 {
-    bool error = false;
+    bool hasInvalidSeparator = false;
 
     // Tread lightly in this code -- it was specifically designed to mimic Win IE's parsing behavior.
     unsigned keyBegin, keyEnd;
@@ -92,7 +92,7 @@ void HTMLMetaElement::parseContentAttribute(const String& content, void* data, D
 
         // skip to first separator
         while (!isSeparator(buffer[i])) {
-            error |= isInvalidSeparator(buffer[i]);
+            hasInvalidSeparator |= isInvalidSeparator(buffer[i]);
             if (i >= length)
                 break;
             i++;
@@ -101,7 +101,7 @@ void HTMLMetaElement::parseContentAttribute(const String& content, void* data, D
 
         // skip to first '=', but don't skip past a ',' or the end of the string
         while (buffer[i] != '=') {
-            error |= isInvalidSeparator(buffer[i]);
+            hasInvalidSeparator |= isInvalidSeparator(buffer[i]);
             if (buffer[i] == ',' || i >= length)
                 break;
             i++;
@@ -117,7 +117,7 @@ void HTMLMetaElement::parseContentAttribute(const String& content, void* data, D
 
         // skip to first separator
         while (!isSeparator(buffer[i])) {
-            error |= isInvalidSeparator(buffer[i]);
+            hasInvalidSeparator |= isInvalidSeparator(buffer[i]);
             if (i >= length)
                 break;
             i++;
@@ -128,9 +128,9 @@ void HTMLMetaElement::parseContentAttribute(const String& content, void* data, D
 
         String keyString = buffer.substring(keyBegin, keyEnd - keyBegin);
         String valueString = buffer.substring(valueBegin, valueEnd - valueBegin);
-        processViewportKeyValuePair(document, keyString, valueString, viewportMetaZeroValuesQuirk, data);
+        processViewportKeyValuePair(document, hasInvalidSeparator, keyString, valueString, viewportMetaZeroValuesQuirk, data);
     }
-    if (error && document) {
+    if (hasInvalidSeparator && document) {
         String message = "Error parsing a meta element's content: ';' is not a valid key-value pair separator. Please use ',' instead.";
         document->addConsoleMessage(ConsoleMessage::create(RenderingMessageSource, WarningMessageLevel, message));
     }
@@ -152,7 +152,7 @@ static inline float clampScaleValue(float value)
     return value;
 }
 
-float HTMLMetaElement::parsePositiveNumber(Document* document, const String& keyString, const String& valueString, bool* ok)
+float HTMLMetaElement::parsePositiveNumber(Document* document, bool& hasInvalidSeparator, const String& keyString, const String& valueString, bool* ok)

[bokan, 2016/09/07 15:13:06]

In this method and others, lets call the parameter `reportWarnings`, rather than
`hasInvalidSeparator`, and make its meaning opposite.

The variable name in the above method is fine though, that is, from
parseContentAttribute we keep track of hasInvalidSeparator and call
processViewportKeyValuePair which takes `reportWarnings` for which we pass
!hasInvalidSeparator.

 {
     size_t parsedLength;
     float value;
@@ -161,19 +161,19 @@ float HTMLMetaElement::parsePositiveNumber(Document* document, const String& key
     else
         value = charactersToFloat(valueString.characters16(), valueString.length(), parsedLength);
     if (!parsedLength) {
-        reportViewportWarning(document, UnrecognizedViewportArgumentValueError, valueString, keyString);
+        reportViewportWarning(document, hasInvalidSeparator, UnrecognizedViewportArgumentValueError, valueString, keyString);
         if (ok)
             *ok = false;
         return 0;
     }
     if (parsedLength < valueString.length())
-        reportViewportWarning(document, TruncatedViewportArgumentValueError, valueString, keyString);
+        reportViewportWarning(document, hasInvalidSeparator, TruncatedViewportArgumentValueError, valueString, keyString);
     if (ok)
         *ok = true;
     return value;
 }
 
-Length HTMLMetaElement::parseViewportValueAsLength(Document* document, const String& keyString, const String& valueString)
+Length HTMLMetaElement::parseViewportValueAsLength(Document* document, bool& hasInvalidSeparator, const String& keyString, const String& valueString)
 {
     // 1) Non-negative number values are translated to px lengths.
     // 2) Negative number values are translated to auto.
@@ -191,7 +191,7 @@ Length HTMLMetaElement::parseViewportValueAsLength(Document* document, const Str
         }
     }
 
-    float value = parsePositiveNumber(document, keyString, valueString);
+    float value = parsePositiveNumber(document, hasInvalidSeparator, keyString, valueString);
 
     if (value < 0)
         return Length(); // auto
@@ -199,7 +199,7 @@ Length HTMLMetaElement::parseViewportValueAsLength(Document* document, const Str
     return Length(clampLengthValue(value), Fixed);
 }
 
-float HTMLMetaElement::parseViewportValueAsZoom(Document* document, const String& keyString, const String& valueString, bool& computedValueMatchesParsedValue, bool viewportMetaZeroValuesQuirk)
+float HTMLMetaElement::parseViewportValueAsZoom(Document* document, bool& hasInvalidSeparator, const String& keyString, const String& valueString, bool& computedValueMatchesParsedValue, bool viewportMetaZeroValuesQuirk)
 {
     // 1) Non-negative number values are translated to <number> values.
     // 2) Negative number values are translated to auto.
@@ -225,13 +225,13 @@ float HTMLMetaElement::parseViewportValueAsZoom(Document* document, const String
         }
     }
 
-    float value = parsePositiveNumber(document, keyString, valueString);
+    float value = parsePositiveNumber(document, hasInvalidSeparator, keyString, valueString);
 
     if (value < 0)
         return ViewportDescription::ValueAuto;
 
     if (value > 10.0)
-        reportViewportWarning(document, MaximumScaleTooLargeError, String(), String());
+        reportViewportWarning(document, hasInvalidSeparator, MaximumScaleTooLargeError, String(), String());
 
     if (!value && viewportMetaZeroValuesQuirk)
         return ViewportDescription::ValueAuto;
@@ -243,7 +243,7 @@ float HTMLMetaElement::parseViewportValueAsZoom(Document* document, const String
     return clampedValue;
 }
 
-bool HTMLMetaElement::parseViewportValueAsUserZoom(Document* document, const String& keyString, const String& valueString, bool& computedValueMatchesParsedValue)
+bool HTMLMetaElement::parseViewportValueAsUserZoom(Document* document, bool& hasInvalidSeparator, const String& keyString, const String& valueString, bool& computedValueMatchesParsedValue)
 {
     // yes and no are used as keywords.
     // Numbers >= 1, numbers <= -1, device-width and device-height are mapped to yes.
@@ -269,14 +269,14 @@ bool HTMLMetaElement::parseViewportValueAsUserZoom(Document* document, const Str
         }
     }
 
-    float value = parsePositiveNumber(document, keyString, valueString);
+    float value = parsePositiveNumber(document, hasInvalidSeparator, keyString, valueString);
     if (fabs(value) < 1)
         return false;
 
     return true;
 }
 
-float HTMLMetaElement::parseViewportValueAsDPI(Document* document, const String& keyString, const String& valueString)
+float HTMLMetaElement::parseViewportValueAsDPI(Document* document, bool& hasInvalidSeparator, const String& keyString, const String& valueString)
 {
     unsigned length = valueString.length();
     DEFINE_ARRAY_FOR_MATCHING(characters, valueString, 10);
@@ -296,14 +296,14 @@ float HTMLMetaElement::parseViewportValueAsDPI(Document* document, const String&
     }
 
     bool ok;
-    float value = parsePositiveNumber(document, keyString, valueString, &ok);
+    float value = parsePositiveNumber(document, hasInvalidSeparator, keyString, valueString, &ok);
     if (!ok || value < 70 || value > 400)
         return ViewportDescription::ValueAuto;
 
     return value;
 }
 
-void HTMLMetaElement::processViewportKeyValuePair(Document* document, const String& keyString, const String& valueString, bool viewportMetaZeroValuesQuirk, void* data)
+void HTMLMetaElement::processViewportKeyValuePair(Document* document, bool& hasInvalidSeparator, const String& keyString, const String& valueString, bool viewportMetaZeroValuesQuirk, void* data)
 {
     ViewportDescription* description = static_cast<ViewportDescription*>(data);
 
@@ -312,7 +312,7 @@ void HTMLMetaElement::processViewportKeyValuePair(Document* document, const Stri
     DEFINE_ARRAY_FOR_MATCHING(characters, keyString, 17);
     SWITCH(characters, length) {
         CASE("width") {
-            const Length& width = parseViewportValueAsLength(document, keyString, valueString);
+            const Length& width = parseViewportValueAsLength(document, hasInvalidSeparator, keyString, valueString);
             if (width.isAuto())
                 return;
             description->minWidth = Length(ExtendToZoom);
@@ -320,7 +320,7 @@ void HTMLMetaElement::processViewportKeyValuePair(Document* document, const Stri
             return;
         }
         CASE("height") {
-            const Length& height = parseViewportValueAsLength(document, keyString, valueString);
+            const Length& height = parseViewportValueAsLength(document, hasInvalidSeparator, keyString, valueString);
             if (height.isAuto())
                 return;
             description->minHeight = Length(ExtendToZoom);
@@ -328,24 +328,24 @@ void HTMLMetaElement::processViewportKeyValuePair(Document* document, const Stri
             return;
         }
         CASE("initial-scale") {
-            description->zoom = parseViewportValueAsZoom(document, keyString, valueString, description->zoomIsExplicit, viewportMetaZeroValuesQuirk);
+            description->zoom = parseViewportValueAsZoom(document, hasInvalidSeparator, keyString, valueString, description->zoomIsExplicit, viewportMetaZeroValuesQuirk);
             return;
         }
         CASE("minimum-scale") {
-            description->minZoom = parseViewportValueAsZoom(document, keyString, valueString, description->minZoomIsExplicit, viewportMetaZeroValuesQuirk);
+            description->minZoom = parseViewportValueAsZoom(document, hasInvalidSeparator, keyString, valueString, description->minZoomIsExplicit, viewportMetaZeroValuesQuirk);
             return;
         }
         CASE("maximum-scale") {
-            description->maxZoom = parseViewportValueAsZoom(document, keyString, valueString, description->maxZoomIsExplicit, viewportMetaZeroValuesQuirk);
+            description->maxZoom = parseViewportValueAsZoom(document, hasInvalidSeparator, keyString, valueString, description->maxZoomIsExplicit, viewportMetaZeroValuesQuirk);
             return;
         }
         CASE("user-scalable") {
-            description->userZoom = parseViewportValueAsUserZoom(document, keyString, valueString, description->userZoomIsExplicit);
+            description->userZoom = parseViewportValueAsUserZoom(document, hasInvalidSeparator, keyString, valueString, description->userZoomIsExplicit);
             return;
         }
         CASE("target-densitydpi") {
-            description->deprecatedTargetDensityDPI = parseViewportValueAsDPI(document, keyString, valueString);
-            reportViewportWarning(document, TargetDensityDpiUnsupported, String(), String());
+            description->deprecatedTargetDensityDPI = parseViewportValueAsDPI(document, hasInvalidSeparator, keyString, valueString);
+            reportViewportWarning(document, hasInvalidSeparator, TargetDensityDpiUnsupported, String(), String());
             return;
         }
         CASE("minimal-ui") {
@@ -357,7 +357,7 @@ void HTMLMetaElement::processViewportKeyValuePair(Document* document, const Stri
             return;
         }
     }
-    reportViewportWarning(document, UnrecognizedViewportArgumentKeyError, keyString, String());
+    reportViewportWarning(document, hasInvalidSeparator, UnrecognizedViewportArgumentKeyError, keyString, String());
 }
 
 static const char* viewportErrorMessageTemplate(ViewportErrorCode errorCode)
@@ -388,9 +388,9 @@ static MessageLevel viewportErrorMessageLevel(ViewportErrorCode errorCode)
     return ErrorMessageLevel;
 }
 
-void HTMLMetaElement::reportViewportWarning(Document* document, ViewportErrorCode errorCode, const String& replacement1, const String& replacement2)
+void HTMLMetaElement::reportViewportWarning(Document* document, bool& hasInvalidSeparator, ViewportErrorCode errorCode, const String& replacement1, const String& replacement2)

[bokan, 2016/09/07 15:13:06]

This method shouldn't have this param. Instead, the caller should avoid calling
it based on `reportWarnings`

 {
-    if (!document || !document->frame())
+    if (!document || !document->frame() || hasInvalidSeparator)
         return;
 
     String message = viewportErrorMessageTemplate(errorCode);