Bug 38900 – Refactoring HTMLLinkElement::tokenizeRelAttribute so its signature doesn't change so much

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Leon Clarke]

This is a bit of refactoring as part of providing support for link prefetching
The prefetching bug is here https://bugs.webkit.org/show_bug.cgi?id=3652

tokienizeRelAttribute takes a long list of bools indicating if each of the possible values of rel are present. This means that if support for some attributes is implemented as optional features, the function will have a different signature for every combination of features. In addition, the call to it in the preloader needs to change whenever an extra rel type is supported. In android, we have 1 un-upstreamed rel type, and I was adding prefetch as another.

A more elegant approach is to have a simple structure containing all the possible things the rel attribute could be, and pass this in. The corresponding set of variables in the object also become an instance of the new struct.

[Leon Clarke]

Created attachment 55692 [details]
Proposed patch

[Steve Block]

Comment on attachment 55692 [details]
Proposed patch

You should mark this bug as blocking Bug 3652.


WebCore/html/HTMLLinkElement.h:45
 +              { };
This can all be on a single line, no semi-colon required. Or, if you'll soon be adding additional members behind ENABLE guards, use one initializer per line.

WebCore/html/HTMLLinkElement.h:93
 +  
Rogue whitespace

WebCore/html/HTMLLinkElement.h:97
 +      bool isAlternate() const { return m_disabledState && m_rel.m_isAlternate; }
Did you mean to invert the logic here?

Either way, we should probably use an enum. Testing what's effectively an enum with operator! is pretty confusing. Is it much extra effort to use an enum?


WebCore/html/HTMLLinkElement.h:107
 +      static void tokenizeRelAttribute(const AtomicString& value, RelAttribute& attribute);
No need to name the argument when it's obvious from the type.

WebCore/html/HTMLLinkElement.h:120
 +      RelAttribute m_rel;
Would m_relAttribute be more clear?

WebCore/html/HTMLLinkElement.cpp:138
 +  void HTMLLinkElement::tokenizeRelAttribute(const AtomicString& rel, RelAttribute& attribute)
Use relAttribute to match the name of the member?

[Leon Clarke]

Created attachment 55832 [details]
Following Steve's review comments

[Steve Block]

Comment on attachment 55832 [details]
Following Steve's review comments


WebCore/html/HTMLLinkElement.h:51
 +      };
This can be private and since we don't need the numeric values anywhere, there's no need to set them.

WebCore/html/HTMLLinkElement.cpp:130
 +          setAttributeEventListener(eventNames().loadEvent, createAttributeEventListener(this, attr));
Did you mean to add this?

WebCore/ChangeLog:5
 +          Refactor signature of tokenizeRelAttribute and the variables it refers to so that new features don't keep changing the signature.
Mention that you're adding the enum too.

[Leon Clarke]

Created attachment 55854 [details]
Following review comments

[Steve Block]

Comment on attachment 55854 [details]
Following review comments

WebCore/html/HTMLLinkElement.cpp:70
 +      int oldDisabledState = m_disabledState;
Just noticed this - it should use the new enum too. Particularly as we no longer explicitly assign values to the enum.

[Leon Clarke]

Created attachment 55855 [details]
Don't cast m_disabledState to int

[WebKit Commit Bot]

Comment on attachment 55855 [details]
Don't cast m_disabledState to int

Clearing flags on attachment: 55855

Committed r59511: <http://trac.webkit.org/changeset/59511>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.