Change the serialization format of form control state to make the code simple
authortkent@chromium.org <tkent@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 25 Jun 2012 09:43:07 +0000 (09:43 +0000)
committertkent@chromium.org <tkent@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 25 Jun 2012 09:43:07 +0000 (09:43 +0000)
https://bugs.webkit.org/show_bug.cgi?id=89847

Reviewed by Hajime Morita.

Source/WebCore:

We used multiple strings to represent state of single form control. It
made the code complex. We change the serialization format so that one
CSV string represents state.

Examples in the old format:
    "0"
    "1", "value"
    "3", "value1", "value2,value2", "value3"

Examples in the new format:
    ""
    ",value"
    ",value1,value2\,value2,value3"

Test: fast/forms/state-restore-various-values.html

* html/FormController.cpp:
(WebCore::FormControlState::serialize):
Generate comma-separated string.
',' in a value is serialized as "\,".
We changed the signature because we don't need the out-argument.
(WebCore::FormControlState::deserialize):
Parses the input comma-separated string.
We changed the signature because we don't need multiple input strings.
(formStateSignature):
Bump up the version because of the representation change.
(WebCore::FormController::formElementsState):
The new serialized format occupies just one string for one control.
- Expected size is now 3n+1.
- Use FormControlState::serialize().
(WebCore::FormController::setStateForNewFormElements):
The new serialized format occupies just one string for one control.
So we can check the vector size before the iteration.
* html/FormController.h:
(FormControlState): Change the function signatures.

* html/shadow/CalendarPickerElement.cpp:
(WebCore::addJavaScriptString): Use StringBuilder::appendEscaped().

Source/WTF:

* wtf/text/StringBuilder.h:
(WTF::StringBuilder::appendEscaped): Added. This function adds the
escaped form of the input string. e.g. if stiring="foo,bar" escape='\'
special=',', the appended string is foo\,bar.

LayoutTests:

* fast/forms/state-restore-broken-state-expected.txt:
Apply the serialization format change.
* fast/forms/state-restore-various-values-expected.txt: Added.
* fast/forms/state-restore-various-values.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@121145 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/forms/state-restore-broken-state-expected.txt
LayoutTests/fast/forms/state-restore-various-values-expected.txt [new file with mode: 0644]
LayoutTests/fast/forms/state-restore-various-values.html [new file with mode: 0644]
Source/WTF/ChangeLog
Source/WTF/wtf/text/StringBuilder.h
Source/WebCore/ChangeLog
Source/WebCore/html/FormController.cpp
Source/WebCore/html/FormController.h
Source/WebCore/html/shadow/CalendarPickerElement.cpp

index e069d0b..eca8c97 100644 (file)
@@ -1,3 +1,15 @@
+2012-06-25  Kent Tamura  <tkent@chromium.org>
+
+        Change the serialization format of form control state to make the code simple
+        https://bugs.webkit.org/show_bug.cgi?id=89847
+
+        Reviewed by Hajime Morita.
+
+        * fast/forms/state-restore-broken-state-expected.txt:
+        Apply the serialization format change.
+        * fast/forms/state-restore-various-values-expected.txt: Added.
+        * fast/forms/state-restore-various-values.html: Added.
+
 2012-06-24  Krist√≥f Koszty√≥  <kkristof@inf.u-szeged.hu>
 
         [Qt] Unreviewed gardening, skip new failing tests.
index d952e93..25f6dc7 100644 (file)
@@ -1,4 +1,4 @@
-CONSOLE MESSAGE: line 5: Generated state: [name1,text,1,modified]
+CONSOLE MESSAGE: line 5: Generated state: [name1,text,,modified]
 The value was modified in the first load of state-restore-broken-state-1.html, but it should not be restored because the state-restore-broken-state-2.html breaks the state.
 
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
diff --git a/LayoutTests/fast/forms/state-restore-various-values-expected.txt b/LayoutTests/fast/forms/state-restore-various-values-expected.txt
new file mode 100644 (file)
index 0000000..ad36482
--- /dev/null
@@ -0,0 +1,17 @@
+Test if special characters are correctly restored.
+
+PASS $("opt-01").selected is true
+PASS $("opt-02").selected is true
+PASS $("opt-03").selected is true
+PASS $("opt-04").selected is true
+PASS $("opt-05").selected is true
+PASS $("opt-06").selected is true
+PASS $("opt-07").selected is true
+PASS $("opt-08").selected is true
+PASS $("opt-09").selected is true
+PASS $("opt-10").selected is true
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
+
diff --git a/LayoutTests/fast/forms/state-restore-various-values.html b/LayoutTests/fast/forms/state-restore-various-values.html
new file mode 100644 (file)
index 0000000..5e137e5
--- /dev/null
@@ -0,0 +1,70 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="../js/resources/js-test-pre.js"></script>
+<script src="resources/common.js"></script>
+</head>
+<body>
+<p>Test if special characters are correctly restored.</p>
+<div id="console"></div>
+
+<input id="emptyOnFirstVisit">
+<div id="parent">
+<form action="data:text/html,&lt;script>history.back()&lt;/script>" id=form1>
+<select id="select1" multiple>
+  <option id="opt-01"></option>
+  <option id="opt-02">,</option>
+  <option id="opt-03">\</option>
+  <option id="opt-04">,a,</option>
+  <option id="opt-05">,\,</option>
+  <option id="opt-06">,\\,</option>
+  <option id="opt-07">\a\</option>
+  <option id="opt-08">\n\</option>
+  <option id="opt-09">\,,\</option>
+  <option id="opt-10">&amp;&#0;&#x0d;&#x0a;,</option>
+</select>
+</form>
+</div>
+
+<script>
+// Note that this test depends on the fact that select options are stored by
+// value strings, not indexes.
+
+jsTestIsAsync = true;
+
+function runTest()
+{
+    var state = document.getElementById('emptyOnFirstVisit');
+    if (!state.value) {
+        // First visit.
+        setTimeout(function() {
+            state.value = 'visited';
+            var options = $('select1').options;
+            for (var i = 0; i < options.length; ++i)
+                options[i].selected = true;
+            $('form1').submit();
+        }, 0);
+    } else {
+        // Went back to this page again, and form state should be restored.
+        shouldBeTrue('$("opt-01").selected');
+        shouldBeTrue('$("opt-02").selected');
+        shouldBeTrue('$("opt-03").selected');
+        shouldBeTrue('$("opt-04").selected');
+        shouldBeTrue('$("opt-05").selected');
+        shouldBeTrue('$("opt-06").selected');
+        shouldBeTrue('$("opt-07").selected');
+        shouldBeTrue('$("opt-08").selected');
+        shouldBeTrue('$("opt-09").selected');
+        shouldBeTrue('$("opt-10").selected');
+    
+        $('parent').innerHTML = '';
+        setTimeout(function() {
+            finishJSTest();
+        }, 0);
+    }
+}
+
+runTest();
+</script>
+<script src="../js/resources/js-test-post.js"></script>
+</body>
index be2aaa8..8a37467 100644 (file)
@@ -1,3 +1,15 @@
+2012-06-25  Kent Tamura  <tkent@chromium.org>
+
+        Change the serialization format of form control state to make the code simple
+        https://bugs.webkit.org/show_bug.cgi?id=89847
+
+        Reviewed by Hajime Morita.
+
+        * wtf/text/StringBuilder.h:
+        (WTF::StringBuilder::appendEscaped): Added. This function adds the
+        escaped form of the input string. e.g. if stiring="foo,bar" escape='\'
+        special=',', the appended string is foo\,bar.
+
 2012-06-24  Adam Barth  <abarth@webkit.org>
 
         Remove USE(CHROMIUM_NET) because it is unused
index d896d17..c73d463 100644 (file)
@@ -130,6 +130,21 @@ public:
         append(static_cast<LChar>(c));
     }
 
+    void appendEscaped(const String& string, UChar escape, UChar special)
+    {
+        if (string.isEmpty())
+            return;
+        unsigned requiredSize = length() + string.length();
+        if (capacity() < requiredSize)
+            reserveCapacity(requiredSize);
+        for (unsigned i = 0; i < string.length(); ++i) {
+            UChar ch = string[i];
+            if (ch == escape || ch == special)
+                append(escape);
+            append(ch);
+        }
+    }
+
     String toString()
     {
         shrinkToFit();
index c3e694b..4a4f84d 100644 (file)
@@ -1,3 +1,49 @@
+2012-06-25  Kent Tamura  <tkent@chromium.org>
+
+        Change the serialization format of form control state to make the code simple
+        https://bugs.webkit.org/show_bug.cgi?id=89847
+
+        Reviewed by Hajime Morita.
+
+        We used multiple strings to represent state of single form control. It
+        made the code complex. We change the serialization format so that one
+        CSV string represents state.
+
+        Examples in the old format:
+            "0"
+            "1", "value"
+            "3", "value1", "value2,value2", "value3"
+
+        Examples in the new format:
+            ""
+            ",value"
+            ",value1,value2\,value2,value3"
+
+        Test: fast/forms/state-restore-various-values.html
+
+        * html/FormController.cpp:
+        (WebCore::FormControlState::serialize):
+        Generate comma-separated string.
+        ',' in a value is serialized as "\,".
+        We changed the signature because we don't need the out-argument.
+        (WebCore::FormControlState::deserialize):
+        Parses the input comma-separated string.
+        We changed the signature because we don't need multiple input strings.
+        (formStateSignature):
+        Bump up the version because of the representation change.
+        (WebCore::FormController::formElementsState):
+        The new serialized format occupies just one string for one control.
+        - Expected size is now 3n+1.
+        - Use FormControlState::serialize().
+        (WebCore::FormController::setStateForNewFormElements):
+        The new serialized format occupies just one string for one control.
+        So we can check the vector size before the iteration.
+        * html/FormController.h:
+        (FormControlState): Change the function signatures.
+
+        * html/shadow/CalendarPickerElement.cpp:
+        (WebCore::addJavaScriptString): Use StringBuilder::appendEscaped().
+
 2012-06-22  Yury Semikhatsky  <yurys@chromium.org>
 
         Web Inspector: add external resources size to the native memory diagram
index 31c7763..84f44ee 100644 (file)
@@ -22,6 +22,7 @@
 #include "FormController.h"
 
 #include "HTMLFormControlElementWithState.h"
+#include <wtf/text/StringBuilder.h>
 
 namespace WebCore {
 
@@ -30,38 +31,61 @@ using namespace HTMLNames;
 // ----------------------------------------------------------------------------
 
 // Serilized form of FormControlState:
-//  (',' means strings around it are separated in stateVector.)
 //
 // SerializedControlState ::= SkipState | RestoreState
-// SkipState ::= '0'
-// RestoreState ::= UnsignedNumber, ControlValue+
-// UnsignedNumber ::= [0-9]+
-// ControlValue ::= arbitrary string
-//
-// RestoreState has a sequence of ControlValues. The length of the
-// sequence is represented by UnsignedNumber.
+// SkipState ::= ''
+// RestoreState ::= (',' EscapedValue )+
+// EscapedValue ::= ('\\' | '\,' | [^\,])+
 
-void FormControlState::serializeTo(Vector<String>& stateVector) const
+String FormControlState::serialize() const
 {
     ASSERT(!isFailure());
-    stateVector.append(String::number(m_values.size()));
+    if (!m_values.size())
+        return emptyString();
+
+    size_t enoughSize = 0;
     for (size_t i = 0; i < m_values.size(); ++i)
-        stateVector.append(m_values[i].isNull() ? emptyString() : m_values[i]);
+        enoughSize += 1 + m_values[i].length() * 2;
+    StringBuilder builder;
+    builder.reserveCapacity(enoughSize);
+    for (size_t i = 0; i < m_values.size(); ++i) {
+        builder.append(',');
+        builder.appendEscaped(m_values[i], '\\', ',');
+    }
+    return builder.toString();
 }
 
-FormControlState FormControlState::deserialize(const Vector<String>& stateVector, size_t& index)
+FormControlState FormControlState::deserialize(const String& escaped)
 {
-    if (index >= stateVector.size())
-        return FormControlState(TypeFailure);
-    size_t valueSize = stateVector[index++].toUInt();
-    if (!valueSize)
+    if (!escaped.length())
         return FormControlState();
-    if (index + valueSize > stateVector.size())
+    if (escaped[0] != ',')
         return FormControlState(TypeFailure);
+
+    size_t valueSize = 1;
+    for (unsigned i = 1; i < escaped.length(); ++i) {
+        if (escaped[i] == '\\') {
+            if (++i >= escaped.length())
+                return FormControlState(TypeFailure);
+        } else if (escaped[i] == ',')
+            valueSize++;
+    }
+
     FormControlState state;
     state.m_values.reserveCapacity(valueSize);
-    for (size_t i = 0; i < valueSize; ++i)
-        state.append(stateVector[index++]);
+    StringBuilder builder;
+    for (unsigned i = 1; i < escaped.length(); ++i) {
+        if (escaped[i] == '\\') {
+            if (++i >= escaped.length())
+                return FormControlState(TypeFailure);
+            builder.append(escaped[i]);
+        } else if (escaped[i] == ',') {
+            state.append(builder.toString());
+            builder.clear();
+        } else
+            builder.append(escaped[i]);
+    }
+    state.append(builder.toString());
     return state;
 }
 
@@ -81,14 +105,14 @@ static String formStateSignature()
     // In the legacy version of serialized state, the first item was a name
     // attribute value of a form control. The following string literal should
     // contain some characters which are rarely used for name attribute values.
-    DEFINE_STATIC_LOCAL(String, signature, ("\n\r?% WebKit serialized form state version 3 \n\r=&"));
+    DEFINE_STATIC_LOCAL(String, signature, ("\n\r?% WebKit serialized form state version 4 \n\r=&"));
     return signature;
 }
 
 Vector<String> FormController::formElementsState() const
 {
     Vector<String> stateVector;
-    stateVector.reserveInitialCapacity(m_formElementsWithState.size() * 4 + 1);
+    stateVector.reserveInitialCapacity(m_formElementsWithState.size() * 3 + 1);
     stateVector.append(formStateSignature());
     typedef FormElementListHashSet::const_iterator Iterator;
     Iterator end = m_formElementsWithState.end();
@@ -98,7 +122,7 @@ Vector<String> FormController::formElementsState() const
             continue;
         stateVector.append(elementWithState->name().string());
         stateVector.append(elementWithState->formControlType().string());
-        elementWithState->saveFormControlState().serializeTo(stateVector);
+        stateVector.append(elementWithState->saveFormControlState().serialize());
     }
     return stateVector;
 }
@@ -113,14 +137,15 @@ void FormController::setStateForNewFormElements(const Vector<String>& stateVecto
     typedef FormElementStateMap::iterator Iterator;
     m_formElementsWithState.clear();
 
-    size_t i = 0;
-    if (stateVector.size() < 1 || stateVector[i++] != formStateSignature())
+    if (stateVector.size() < 1 || stateVector[0] != formStateSignature())
+        return;
+    if ((stateVector.size() - 1) % 3)
         return;
 
-    while (i + 2 < stateVector.size()) {
-        AtomicString name = stateVector[i++];
-        AtomicString type = stateVector[i++];
-        FormControlState state = FormControlState::deserialize(stateVector, i);
+    for (size_t i = 1; i < stateVector.size(); i += 3) {
+        AtomicString name = stateVector[i];
+        AtomicString type = stateVector[i + 1];
+        FormControlState state = FormControlState::deserialize(stateVector[i + 2]);
         if (type.isEmpty() || type.impl()->find(isNotFormControlTypeCharacter) != notFound || state.isFailure())
             break;
 
@@ -134,8 +159,6 @@ void FormController::setStateForNewFormElements(const Vector<String>& stateVecto
             m_stateForNewFormElements.set(key, stateList);
         }
     }
-    if (i != stateVector.size())
-        m_stateForNewFormElements.clear();
 }
 
 bool FormController::hasStateForNewFormElements() const
index fa5463b..4de51f5 100644 (file)
@@ -78,7 +78,7 @@ class FormControlState {
 public:
     FormControlState() : m_type(TypeSkip) { }
     explicit FormControlState(const String& value) : m_type(TypeRestore) { m_values.append(value); }
-    static FormControlState deserialize(const Vector<String>& stateVector, size_t& index);
+    static FormControlState deserialize(const String&);
     FormControlState(const FormControlState& another) : m_type(another.m_type), m_values(another.m_values) { }
     FormControlState& operator=(const FormControlState&);
 
@@ -86,7 +86,7 @@ public:
     size_t valueSize() const { return m_values.size(); }
     const String& operator[](size_t i) const { return m_values[i]; }
     void append(const String&);
-    void serializeTo(Vector<String>& stateVector) const;
+    String serialize() const;
 
 private:
     enum Type { TypeSkip, TypeRestore, TypeFailure };
index 5358cca..fe1f64e 100644 (file)
@@ -151,12 +151,7 @@ static void addJavaScriptString(const String& str, DocumentWriter& writer)
 {
     addLiteral("\"", writer);
     StringBuilder builder;
-    builder.reserveCapacity(str.length());
-    for (unsigned i = 0; i < str.length(); ++i) {
-        if (str[i] == '\\' || str[i] == '"')
-            builder.append('\\');
-        builder.append(str[i]);
-    }
+    builder.appendEscaped(str, '\\', '"');
     addString(builder.toString(), writer);
     addLiteral("\"", writer);
 }