Bug 23465 – Refactor some more HTMLOptionElement code

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Nikolas Zimmermann]

Refactor some more HTMLOptionElement code in the new OptionElement class, so it can be used by WMLOptionElement as well.

[Nikolas Zimmermann]

Created attachment 26910 [details]
Initial patch

[Eric Seidel (no email)]

Comment on attachment 26910 [details]
Initial patch

Seems like this one:
 982         if (stripLeadingWhiteSpace(static_cast<HTMLOptionElement*>(items[index])->textIndentedToRespectGroupLabel()).startsWith(prefix, false)) {


Doesn't want that call.  Also, that line could be broken into multiple lines to be more readable.

What is "aggregateOptionText"?  Maybe you mean aggregate as a verb?  Still not sure why it's used that way.

maybe m_optionData?  m_data is so bland, reminds me of the kde "d" pointers which I was never a fan of.

I think "aggregate*" I would just call "optionText" and "optionValue".  The fact that they have to do work under the covers is OK to hide in this case I think.

Otherwise this looks fine to me.  I'd like to see the final patch though.

[Nikolas Zimmermann]

(In reply to comment #2)
> (From update of attachment 26910 [details] [review])
> Seems like this one:
>  982         if
> (stripLeadingWhiteSpace(static_cast<HTMLOptionElement*>(items[index])->textIndentedToRespectGroupLabel()).startsWith(prefix,
> false)) {
> 
> 
> Doesn't want that call.  Also, that line could be broken into multiple lines to
> be more readable.
Right, as discussed on IRC, it's as inefficient as before, so we can just leave it. Otherwhise we'd need to add a new text() method to OptionElement.

> 
> What is "aggregateOptionText"?  Maybe you mean aggregate as a verb?  Still not
> sure why it's used that way.
Renamed to "collectOption...".

> 
> maybe m_optionData?  m_data is so bland, reminds me of the kde "d" pointers
> which I was never a fan of.
I'd really like to leave it as m_data, as all other classes use the "XXXElementData" classes in the same way: (HTML/SVG/WML)(Style/Script/Option/...)Element.
 
> I think "aggregate*" I would just call "optionText" and "optionValue".  The
> fact that they have to do work under the covers is OK to hide in this case I
> think.
> 
> Otherwise this looks fine to me.  I'd like to see the final patch though.
Okay, uploading.

[Nikolas Zimmermann]

Created attachment 26915 [details]
Updated patch

[Eric Seidel (no email)]

Comment on attachment 26915 [details]
Updated patch

Looks fine.  I would change it back to "collect" instead of "collected", since the whole point is to designate that those methods do work instead of just returning a member variable.  I'm not sure "collectedFoo" is any better than "foo" in this case.  "collectFoo" at least shows that work is being done in the method, if you wanted to express that.    So I suggest either "optionText" or "collectOptionText" when you land.

[Nikolas Zimmermann]

Thanks Eric! Landed in r40130.