Bug 89847

Summary:

Change the serialization format of form control state to make the code simple

Product:

WebKit

Reporter:

Kent Tamura <tkent>

Component:

Forms

Assignee:

Kent Tamura <tkent>

Status:

RESOLVED FIXED

Severity:

Minor

CC:

darin, eric, leviw, morrita, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

89628

Bug Blocks:

23346

Attachments:

Description	Flags
Patch	none
Patch 2	none
Patch 3	none
Patch 4	none
Patch	none

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.