Bug 89847 - Change the serialization format of form control state to make the code simple
Summary: Change the serialization format of form control state to make the code simple
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P3 Minor
Assignee: Kent Tamura
URL:
Keywords:
Depends on: 89628
Blocks: 23346
  Show dependency treegraph
 
Reported: 2012-06-24 20:20 PDT by Kent Tamura
Modified: 2012-08-10 15:39 PDT (History)
5 users (show)

See Also:


Attachments
Patch (13.56 KB, patch)
2012-06-24 20:37 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (13.59 KB, patch)
2012-06-24 20:39 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 3 (15.98 KB, patch)
2012-06-24 23:13 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 4 (16.53 KB, patch)
2012-06-24 23:23 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch (30.75 KB, text/plain)
2012-08-10 15:22 PDT, Levi Weintraub
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2012-06-24 20:20:35 PDT
https://bugs.webkit.org/show_bug.cgi?id=89409#c6
> The state (de)serialization flow looks becoming a bit cryptic. 
> My feeling is that it is because the state is stored as a string vector instead of string.
> If we control the format, the code will become much cleaner.
Comment 1 Kent Tamura 2012-06-24 20:37:52 PDT
Created attachment 149229 [details]
Patch
Comment 2 Kent Tamura 2012-06-24 20:39:08 PDT
Created attachment 149230 [details]
Patch 2

Remove TAB
Comment 3 Hajime Morrita 2012-06-24 22:03:02 PDT
Comment on attachment 149230 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=149230&action=review

> Source/WebCore/html/FormController.cpp:61
> +        }

Don't we have any escape function? If so, let's use it. Or let's define one.

> Source/WebCore/html/FormController.cpp:95
> +    }

Ditto.
Comment 4 Kent Tamura 2012-06-24 22:21:07 PDT
Comment on attachment 149230 [details]
Patch 2

View in context: https://bugs.webkit.org/attachment.cgi?id=149230&action=review

>> Source/WebCore/html/FormController.cpp:61
>> +        }
> 
> Don't we have any escape function? If so, let's use it. Or let's define one.

I couldn't find a generic escaping function. It's nice to have one because we have a similar code in CalendarPickerElement.cpp.

>> Source/WebCore/html/FormController.cpp:95
>> +    }
> 
> Ditto.

I have no good idea of unescaping function interface because we can't find delimiters of escaped strings without parsing.
Comment 5 Kent Tamura 2012-06-24 23:13:21 PDT
Created attachment 149246 [details]
Patch 3

Add an escape function
Comment 6 Kent Tamura 2012-06-24 23:23:34 PDT
Created attachment 149248 [details]
Patch 4
Comment 7 WebKit Review Bot 2012-06-25 02:43:25 PDT
Comment on attachment 149248 [details]
Patch 4

Clearing flags on attachment: 149248

Committed r121145: <http://trac.webkit.org/changeset/121145>
Comment 8 WebKit Review Bot 2012-06-25 02:43:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Darin Adler 2012-06-25 17:36:00 PDT
This seems like a step backwards to me. Serializing and then decoding the object is extra work. There is more code here than before.
Comment 10 Kent Tamura 2012-06-25 18:13:52 PDT
(In reply to comment #9)
> This seems like a step backwards to me. Serializing and then decoding the object is extra work. There is more code here than before.

ok, I'll roll it out.  I don't have strong preference for this.
Comment 11 Kent Tamura 2012-06-25 18:20:21 PDT
Reverted r121145 for reason:

Had an objection for the change.

Committed r121207: <http://trac.webkit.org/changeset/121207>
Comment 12 Darin Adler 2012-06-25 18:27:37 PDT
Maybe I came on too strong.

I liked the new helper function for appending an escaped string.

It’s also good to have all the state wrapped up in a single object rather than a confusing combination of a vector and a size.

But I do want us to keep serializing and then re-parsing to a minimum.
Comment 13 Levi Weintraub 2012-08-10 15:22:23 PDT
Reopening to attach new patch.
Comment 14 Levi Weintraub 2012-08-10 15:22:28 PDT
Created attachment 157818 [details]
Patch
Comment 15 Levi Weintraub 2012-08-10 15:29:52 PDT
Comment on attachment 157818 [details]
Patch

Whoops!! Wrong bug number! Sorry for the noise.
Comment 16 Levi Weintraub 2012-08-10 15:38:00 PDT
Reopening to attach new patch.