Bug 38900

Summary: Refactoring HTMLLinkElement::tokenizeRelAttribute so its signature doesn't change so much
Product: WebKit Reporter: Leon Clarke <leonclarke>
Component: DOMAssignee: Nobody <webkit-unassigned>
Severity: Normal CC: android-webkit-unforking, commit-queue, steveblock
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 3652    
Description Flags
Proposed patch
Following Steve's review comments
Following review comments
Don't cast m_disabledState to int none

Description Leon Clarke 2010-05-11 05:51:07 PDT
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.
Comment 1 Leon Clarke 2010-05-11 06:01:42 PDT
Created attachment 55692 [details]
Proposed patch
Comment 2 Steve Block 2010-05-12 02:30:27 PDT
Comment on attachment 55692 [details]
Proposed patch

You should mark this bug as blocking Bug 3652.

 +              { };
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.

Rogue whitespace

 +      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?

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

 +      RelAttribute m_rel;
Would m_relAttribute be more clear?

 +  void HTMLLinkElement::tokenizeRelAttribute(const AtomicString& rel, RelAttribute& attribute)
Use relAttribute to match the name of the member?
Comment 3 Leon Clarke 2010-05-12 04:29:14 PDT
Created attachment 55832 [details]
Following Steve's review comments
Comment 4 Steve Block 2010-05-12 05:12:57 PDT
Comment on attachment 55832 [details]
Following Steve's review comments

 +      };
This can be private and since we don't need the numeric values anywhere, there's no need to set them.

 +          setAttributeEventListener(eventNames().loadEvent, createAttributeEventListener(this, attr));
Did you mean to add this?

 +          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.
Comment 5 Leon Clarke 2010-05-12 09:00:45 PDT
Created attachment 55854 [details]
Following review comments
Comment 6 Steve Block 2010-05-12 09:09:31 PDT
Comment on attachment 55854 [details]
Following review comments

 +      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.
Comment 7 Leon Clarke 2010-05-12 09:20:22 PDT
Created attachment 55855 [details]
Don't cast m_disabledState to int
Comment 8 WebKit Commit Bot 2010-05-14 18:58:05 PDT
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>
Comment 9 WebKit Commit Bot 2010-05-14 18:58:10 PDT
All reviewed patches have been landed.  Closing bug.