Bug 107770 – Add form-related instrumentations, and support 33+ features in FeatureObserver

• Component: Forms
• Status: RESOLVED FIXED

[Kent Tamura]

Add form-related instrumentations, and support 33+ features in FeatureObserver

[Kent Tamura]

Created attachment 184371 [details]
Patch

[Kentaro Hara]

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?

[Kentaro Hara]

(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)".

[Kent Tamura]

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.

[Kentaro Hara]

(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.

[Kent Tamura]

Created attachment 184375 [details]
Patch 2

[Kentaro Hara]

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!

[Kent Tamura]

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.

[Kentaro Hara]

Comment on attachment 184375 [details]
Patch 2

Makes sense to me

[Kent Tamura]

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.

[Kent Tamura]

Created attachment 184394 [details]
Patch 3

Add Document* version of observe()

[Kentaro Hara]

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.

[Kent Tamura]

Created attachment 184398 [details]
Patch for landign

Add a comment

[WebKit Review Bot]

Comment on attachment 184398 [details]
Patch for landign

Clearing flags on attachment: 184398

Committed r140655: <http://trac.webkit.org/changeset/140655>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.