Bug 107770

Created attachment 184371 [details] Patch

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?

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

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

Created attachment 184375 [details] Patch 2

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 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 on attachment 184375 [details] Patch 2 Makes sense to me

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.

Created attachment 184394 [details] Patch 3 Add Document* version of observe()

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.

Created attachment 184398 [details] Patch for landign Add a comment

Comment on attachment 184398 [details] Patch for landign Clearing flags on attachment: 184398 Committed r140655: <http://trac.webkit.org/changeset/140655>

All reviewed patches have been landed. Closing bug.