Bug 26062 – Refactor HTMLSelectElement code, to be reusable for WMLSelectElement

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

Refactor HTMLSelectElement code, just like it has been done for HTMLInput/Option/OptGroupElement before. This is the last big refactorization, needed for WML.

A follow-up patch will add WMLSelectElement and integrate it. <select> is the last missing piece for WML.

[Nikolas Zimmermann]

Created attachment 30736 [details]
Initial patch

No regressions.

[Nikolas Zimmermann]

Created attachment 30738 [details]
Updated patch

Oops, the initial patch wouldn't compile on non-mac platforms. Fixed.

[Darin Adler]

Comment on attachment 30738 [details]
Updated patch

It's unfortunate that we're losing the Subversion history for all these functions. Also makes the patch harder to review, since I can't see what you did and did not change when moving the functions.

> +static const DOMTimeStamp s_typeAheadTimeout = 1000;

The "s_" prefix seems a little strange here, since this is not a static data member. (Not that I like the s_ prefix even for static data members.) I liked the old name without a prefix better.

> +        cachedStateForActiveSelection.append(optionElement ? optionElement->selected() : false);

This is probably not new code, but I would write this with && instead of ?: as optionElement && optionElement->selected().

> +    for (unsigned int i = 0; i < items.size(); ++i) {

Should be unsigned or size_t, not unsigned int.

> +    Vector<char, 1024> characters(length);
> +    for (int i = 0; i < length; ++i) {
> +        OptionElement* optionElement = toOptionElement(items[i]);
> +        bool selected = optionElement ? optionElement->selected() : false;
> +        characters[i] = selected ? 'X' : '.';
> +    }
> +
> +    value = String(characters.data(), length);

Not related to refactoring, but we should change this to use String::createUninitialized.

> +    // ### this case should not happen. make sure that we select the first option
> +    // in any case. otherwise we have no consistency with the DOM interface. FIXME!
> +    // we return the first one if it was a combobox select

This old comment could be changed to use modern WebKit style, with FIXME prefix instead of ### and sentence format.

>  #include <wtf/Vector.h>
> +#include "Event.h"

We normally put "" includes before <> includes because that's ASCII sort order.

> +    ~SelectElementData();

Does this need to be explicitly declared and defined? Can't we just let the compiler generate it?

r=me

[Nikolas Zimmermann]

(In reply to comment #3)
> (From update of attachment 30738 [details] [review])
> It's unfortunate that we're losing the Subversion history for all these
> functions. Also makes the patch harder to review, since I can't see what you
> did and did not change when moving the functions.
Thanks, that you took the time despite that fact :-)
 
> > +static const DOMTimeStamp s_typeAheadTimeout = 1000;
> 
> The "s_" prefix seems a little strange here, since this is not a static data
> member. (Not that I like the s_ prefix even for static data members.) I liked
> the old name without a prefix better.
Restored the old name.
 
> > +        cachedStateForActiveSelection.append(optionElement ? optionElement->selected() : false);
> 
> This is probably not new code, but I would write this with && instead of ?: as
> optionElement && optionElement->selected().
Fixed. There were other places, that I fixed as well.
 
> > +    for (unsigned int i = 0; i < items.size(); ++i) {
> 
> Should be unsigned or size_t, not unsigned int.
Fixed. What about the other loops using "for (unsigned i = 0; i < items.size();..."?
Does that need to be fixed as well?
 
> > +    Vector<char, 1024> characters(length);
> > +    for (int i = 0; i < length; ++i) {
> > +        OptionElement* optionElement = toOptionElement(items[i]);
> > +        bool selected = optionElement ? optionElement->selected() : false;
> > +        characters[i] = selected ? 'X' : '.';
> > +    }
> > +
> > +    value = String(characters.data(), length);
> 
> Not related to refactoring, but we should change this to use
> String::createUninitialized.
Oh, interessting. I'll change the code.
 
> > +    // ### this case should not happen. make sure that we select the first option
> > +    // in any case. otherwise we have no consistency with the DOM interface. FIXME!
> > +    // we return the first one if it was a combobox select
> 
> This old comment could be changed to use modern WebKit style, with FIXME prefix
> instead of ### and sentence format.
Fixed.
 
> >  #include <wtf/Vector.h>
> > +#include "Event.h"
> 
> We normally put "" includes before <> includes because that's ASCII sort order.
Fixed.
 
> > +    ~SelectElementData();
> 
> Does this need to be explicitly declared and defined? Can't we just let the
> compiler generate it?
Ah I always forget about that. The rule is to only add empty dtors if they are virtual, to avoid bloat, right?
 
> r=me

Thanks, I'm going to test the changes first, then land later on.

[Darin Adler]

(In reply to comment #4)
> > > +    for (unsigned int i = 0; i < items.size(); ++i) {
> > 
> > Should be unsigned or size_t, not unsigned int.
> Fixed. What about the other loops using "for (unsigned i = 0; i <
> items.size();..."?
> Does that need to be fixed as well?

unsigned int is never correct. It's just a wordier synonym for unsigned that we don't use. So that half of the issue is clear cut.

As far as switching from unsigned to size_t is concerned, I don't know about that. In 64-bit configurations, size_t is 64-bit and unsigned is 32-bit, and I'm not sure what the best practice is throughout WebCore code. I'd love to hear Maciej's thoughts on that.

> > Does this need to be explicitly declared and defined? Can't we just let the
> > compiler generate it?
> Ah I always forget about that. The rule is to only add empty dtors if they are
> virtual, to avoid bloat, right?

There are two reasons I can think of to explicitly declare and define empty destructors.

    1) To put a destructor into a .cpp file to cut down on the includes needed in a header file. For example, if class A has a member of type RefPtr<B> and class B's definition is not included in the header, you might have a compile failure if you tried to destroy an A object, which could be avoided by having A.cpp include B.h and define the destructor.

    2) For some compilers it's important that the first virtual function explicitly defined in a class be explicitly defined in a .cpp file and not in a header file. But I don't think the destructor matters unless you explicitly define the empty destructor; leaving it out entirely does not have this issue.

If the destructor is virtual, there *might* be some value to putting it in the .cpp file but I'm not aware of any. I normally omit even virtual destructors unless reason (1) applies. And reason (2) only applies if you are defining an empty destructor in the header for some reason, perhaps making it protected or private while leaving it empty.

[Nikolas Zimmermann]

(In reply to comment #5)
> unsigned int is never correct. It's just a wordier synonym for unsigned that we
> don't use. So that half of the issue is clear cut.
> 
> As far as switching from unsigned to size_t is concerned, I don't know about
> that. In 64-bit configurations, size_t is 64-bit and unsigned is 32-bit, and
> I'm not sure what the best practice is throughout WebCore code. I'd love to
> hear Maciej's thoughts on that.
Okay, CC'ing Maciej, as I'm also interessted in that topic in general. What's good practice and what not.

> 
> > > Does this need to be explicitly declared and defined? Can't we just let the
> > > compiler generate it?
> > Ah I always forget about that. The rule is to only add empty dtors if they are
> > virtual, to avoid bloat, right?
> 
> There are two reasons I can think of to explicitly declare and define empty
> destructors.
> 
>     1) To put a destructor into a .cpp file to cut down on the includes needed
> in a header file. For example, if class A has a member of type RefPtr<B> and
> class B's definition is not included in the header, you might have a compile
> failure if you tried to destroy an A object, which could be avoided by having
> A.cpp include B.h and define the destructor.
> 
>     2) For some compilers it's important that the first virtual function
> explicitly defined in a class be explicitly defined in a .cpp file and not in a
> header file. But I don't think the destructor matters unless you explicitly
> define the empty destructor; leaving it out entirely does not have this issue.
> 
> If the destructor is virtual, there *might* be some value to putting it in the
> .cpp file but I'm not aware of any. I normally omit even virtual destructors
> unless reason (1) applies. And reason (2) only applies if you are defining an
> empty destructor in the header for some reason, perhaps making it protected or
> private while leaving it empty.

Thanks for that guide, that helps for the future. When I submitted a patch a while ago, omitting a virtual dtor, I thought Alexey complained about that ('code bloat with some compilers'), but I may misremember.

I've added a FIXME to cover changing that String code path to use createUninitialized(), as I'd like to seperate that from the current patch. There may be a reason for a follow-up patch as well if it turns out the unsigned -> size_t conversion should be done as well. I'd volunteer for doing this if necessary.

[Nikolas Zimmermann]

Landed in r44243.