Bug 23465 - Refactor some more HTMLOptionElement code
Summary: Refactor some more HTMLOptionElement code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks: 20393
  Show dependency treegraph
 
Reported: 2009-01-21 15:58 PST by Nikolas Zimmermann
Modified: 2009-01-22 12:41 PST (History)
0 users

See Also:


Attachments
Initial patch (15.67 KB, patch)
2009-01-21 15:58 PST, Nikolas Zimmermann
eric: review-
Details | Formatted Diff | Diff
Updated patch (15.76 KB, patch)
2009-01-21 16:17 PST, Nikolas Zimmermann
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2009-01-21 15:58:18 PST
Refactor some more HTMLOptionElement code in the new OptionElement class, so it can be used by WMLOptionElement as well.
Comment 1 Nikolas Zimmermann 2009-01-21 15:58:57 PST
Created attachment 26910 [details]
Initial patch
Comment 2 Eric Seidel (no email) 2009-01-21 16:04:45 PST
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.
Comment 3 Nikolas Zimmermann 2009-01-21 16:16:00 PST
(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.
Comment 4 Nikolas Zimmermann 2009-01-21 16:17:12 PST
Created attachment 26915 [details]
Updated patch
Comment 5 Eric Seidel (no email) 2009-01-22 11:18:52 PST
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.
Comment 6 Nikolas Zimmermann 2009-01-22 12:41:29 PST
Thanks Eric! Landed in r40130.