Bug 107770 - Add form-related instrumentations, and support 33+ features in FeatureObserver
Summary: Add form-related instrumentations, and support 33+ features in FeatureObserver
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kent Tamura
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-23 18:00 PST by Kent Tamura
Modified: 2013-01-23 22:09 PST (History)
5 users (show)

See Also:


Attachments
Patch (24.72 KB, patch)
2013-01-23 18:07 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (24.64 KB, patch)
2013-01-23 18:36 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 3 (24.89 KB, patch)
2013-01-23 21:12 PST, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch for landign (24.97 KB, patch)
2013-01-23 21:39 PST, Kent Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2013-01-23 18:00:36 PST
Add form-related instrumentations, and support 33+ features in FeatureObserver
Comment 1 Kent Tamura 2013-01-23 18:07:03 PST
Created attachment 184371 [details]
Patch
Comment 2 Kentaro Hara 2013-01-23 18:15:52 PST
Comment on attachment 184371 [details]
Patch

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

> Source/WebCore/html/TelephoneInputType.cpp:43
> +    FeatureObserver::observe(element->document()->domWindow(), FeatureObserver::InputTypeTel);

InputTypeTel => InputTypeTelephone ?

> Source/WebCore/page/FeatureObserver.cpp:48
> +    if (!m_hasNonZeroFeatureBits)

'if (!m_featureBits.size())' is slower than '!m_hasNonZeroFeatureBits' ?

If you need to use m_hasNonZeroFeatureBits for performance, let's rename it to m_hasFeatureBits (and flip true/false).

> Source/WebCore/page/FeatureObserver.h:94
> +        AutoFocusAttribute,
> +        AutoSaveAttribute,
> +        FormAttribute,
> +        IncrementalAttribute,
> +        ListAttribute,
> +        MinAttribute,
> +        MaxAttribute,
> +        PatternAttribute,
> +        PlaceholderAttribute,
> +        PrecisionAttribute,
> +        PrefixedDirectoryAttribute,
> +        PrefixedSpeechAttribute,
> +        RequiredAttribute,
> +        ResultsAttribute,
> +        StepAttribute,
> +        DataListElement,
> +        InputTypeColor,
> +        InputTypeDate,
> +        InputTypeDateTime,
> +        InputTypeDateTimeFallback,
> +        InputTypeDateTimeLocal,
> +        InputTypeEmail,
> +        InputTypeMonth,
> +        InputTypeNumber,
> +        InputTypeRange,
> +        InputTypeSearch,
> +        InputTypeTel,
> +        InputTypeTime,
> +        InputTypeURL,
> +        InputTypeWeek,
> +        InputTypeWeekFallback,

Shall we sort them in the alphabetical order?
Comment 3 Kentaro Hara 2013-01-23 18:17:22 PST
(In reply to comment #2)
> 'if (!m_featureBits.size())' is slower than '!m_hasNonZeroFeatureBits' ?
> 
> If you need to use m_hasNonZeroFeatureBits for performance, let's rename it to m_hasFeatureBits (and flip true/false).

Ignore this part: "(and flip true/false)".
Comment 4 Kent Tamura 2013-01-23 18:20:15 PST
Comment on attachment 184371 [details]
Patch

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

>> Source/WebCore/html/TelephoneInputType.cpp:43
>> +    FeatureObserver::observe(element->document()->domWindow(), FeatureObserver::InputTypeTel);
> 
> InputTypeTel => InputTypeTelephone ?

I think either is ok.  The class name is "TelephoneInputTyp", but it represents <input type=tel>.

>> Source/WebCore/page/FeatureObserver.cpp:48
>> +    if (!m_hasNonZeroFeatureBits)
> 
> 'if (!m_featureBits.size())' is slower than '!m_hasNonZeroFeatureBits' ?
> 
> If you need to use m_hasNonZeroFeatureBits for performance, let's rename it to m_hasFeatureBits (and flip true/false).

m_featureBits has a fixed size.  m_featureBits.size() is always true.

>> Source/WebCore/page/FeatureObserver.h:94
>> +        InputTypeWeekFallback,
> 
> Shall we sort them in the alphabetical order?

will do.
Comment 5 Kentaro Hara 2013-01-23 18:26:19 PST
(In reply to comment #4)
> > If you need to use m_hasNonZeroFeatureBits for performance, let's rename it to m_hasFeatureBits (and flip true/false).
> 
> m_featureBits has a fixed size.  m_featureBits.size() is always true.

At first I thought it would be better to implement BitVector::isEmpty(), but it might be tricky to implement it without adding one extra bool to BitVector. So adding one bool to FeatureObserver makes sense to me.
Comment 6 Kent Tamura 2013-01-23 18:36:36 PST
Created attachment 184375 [details]
Patch 2
Comment 7 Kentaro Hara 2013-01-23 18:42:18 PST
Comment on attachment 184375 [details]
Patch 2

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

> Source/WebCore/page/FeatureObserver.h:68
> +        AutoFocusAttribute,
> +        AutoSaveAttribute,
> +        DataListElement,

Maybe we want to sort all items in the Feature enum? (If you want to keep all form-related features nearby each other, you can prefix them with "Form".)

> Source/WebCore/page/FeatureObserver.h:115
> +    OwnPtr<BitVector> m_featureBits;

Even better!
Comment 8 Kent Tamura 2013-01-23 19:09:31 PST
Comment on attachment 184375 [details]
Patch 2

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

>> Source/WebCore/page/FeatureObserver.h:68
>> +        DataListElement,
> 
> Maybe we want to sort all items in the Feature enum? (If you want to keep all form-related features nearby each other, you can prefix them with "Form".)

No. We shouldn't change integer values for existing features because the integer values are already being recorded.
Comment 9 Kentaro Hara 2013-01-23 19:27:35 PST
Comment on attachment 184375 [details]
Patch 2

Makes sense to me
Comment 10 Kent Tamura 2013-01-23 20:36:24 PST
Comment on attachment 184375 [details]
Patch 2

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

> Source/WebCore/html/ColorInputType.cpp:75
> +    FeatureObserver::observe(element->document()->domWindow(), FeatureObserver::InputTypeColor);

element->document()->domWindow() is inefficient because FeatureObserver::observe obtains the Document from the DOMWindow.
Comment 11 Kent Tamura 2013-01-23 21:12:45 PST
Created attachment 184394 [details]
Patch 3

Add Document* version of observe()
Comment 12 Kentaro Hara 2013-01-23 21:21:10 PST
Comment on attachment 184394 [details]
Patch 3

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

> Source/WebCore/page/FeatureObserver.h:98
>          // Add new features above this line.

Also it might be better to mention that a new feature must be added at the last line so that we won't break already recorded results.
Comment 13 Kent Tamura 2013-01-23 21:39:50 PST
Created attachment 184398 [details]
Patch for landign

Add a comment
Comment 14 WebKit Review Bot 2013-01-23 22:09:33 PST
Comment on attachment 184398 [details]
Patch for landign

Clearing flags on attachment: 184398

Committed r140655: <http://trac.webkit.org/changeset/140655>
Comment 15 WebKit Review Bot 2013-01-23 22:09:38 PST
All reviewed patches have been landed.  Closing bug.