[Webkit-unassigned] [Bug 35937] Support for HTMLProgressElement

Tue Mar 9 14:42:21 PST 2010

https://bugs.webkit.org/show_bug.cgi?id=35937

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #50346|review?                     |review-
               Flag|                            |

--- Comment #5 from Darin Adler <darin at apple.com>  2010-03-09 14:42:21 PST ---
(From update of attachment 50346)
Great to see you tackling this form element.

> +#if PLATFORM(QT)
> +#if !defined(ENABLE_PROGRESS_TAG)
> +#define ENABLE_PROGRESS_TAG 1
> +#endif
> +#else
> +#if !defined(ENABLE_PROGRESS_TAG)
> +#define ENABLE_PROGRESS_TAG 0
> +#endif
> +#endif

We've handled different ENABLE defaults per platform earlier in the file in the
past. See the blocks above with things like #if PLATFORM(WX) around them.

In other cases we handled those defaults outside this file entirely in the
build system.

Lets keep to one of those if possible.

> +        was modified to actualy draw the progress element.

Typo: "actualy"

Seems like there's a lot of work here to make the progress element code
conditional. Can we do this in some simpler way with less logic in all the
build systems? It seems to me that it's even OK to compile the code as long as
we don't instantiate HTMLProgressElement on other platforms. I presume the plan
is to get this working on the other platforms as soon as possible, and taking a
little extra code size in the meantime seems OK to me.

> +/* progress */
> +
> +progress {
> +    -webkit-appearance: progress-bar;
> +    padding: initial;
> +    border: initial;
> +    margin: 2px;
> +}
> +
> +progress::-webkit-progress-bar {
> +    -webkit-appearance: progress-bar;
> +}

What effect does this have on the platforms with no progress support compiled
in?

> +HTMLProgressElement::HTMLProgressElement(const QualifiedName& tagName, Document* doc, HTMLFormElement* form)

Please use "document" instead of "doc".

> +    m_max = 1;
> +    m_value = 1;
> +    m_isDeteminate = false;

Please use member initialization instead of assignment.

> +HTMLProgressElement::~HTMLProgressElement()
> +{
> +}

I suggest not explicitly declaring or defining the destructor if it doesn't
need any custom code in it.

> +void HTMLProgressElement::parseMappedAttribute(MappedAttribute* attr)
> +{
> +    if (attr->name() == valueAttr)
> +        setValue(attr->value().toFloat());
> +    else if (attr->name() == maxAttr)
> +        setMax(attr->value().toFloat());

This is wrong. DOM bindings for attributes such as value and max need to change
the attribute values. Those in turn change the behavior of the class. It's
never right for parseMappedAttrbute to call the actual DOM binding functin.

> +void HTMLProgressElement::defaultEventHandler(Event* evt)
> +{
> +    HTMLFormControlElement::defaultEventHandler(evt);
> +}

This should not be overridden if we don't need to change what it does. Also
please use "event" not "evt".

> +float HTMLProgressElement::value() const
> +{
> +    if (m_value > m_max)
> +        return m_max;
> +    return m_value;
> +}

This is probably wrong for the DOM value attribute. It should just return the
result of valueAttr. The best way to do that is to use the Reflect keyword in
the DOM binding.

> +void HTMLProgressElement::setValue(float value)
> +{
> +    if (isnan(value) || value < 0)
> +        m_value = 0;
> +    else
> +        m_value = value;
> +    m_isDeteminate = true;
> +}

Same issue here. This function should just set valueAttr.

Typo: m_isDeteminate should instead be m_isDeterminate.

The code to handle nan is unnecessarily complex. It could be:

    m_value = value >= 0 ? value : 0;

But I don't think this should exist in this form. I don't think we should store
m_value.

> +void HTMLProgressElement::setMax(float max)
> +{
> +    if (!isnan(max) && max > 0)
> +        m_max = max;
> +    else
> +        m_max = 1;
> +}

Same thing here. The nan aspect of this function is handled correctly if you
write it like this:

    m_max = max > 0 ? max : 1;

> +public:
> +    HTMLProgressElement(const QualifiedName&, Document*, HTMLFormElement* = 0);

For new classes it's best to make the constructor private and provide a public
create function. Eventually all classes will work this way.

> +    virtual ~HTMLProgressElement();

You should just leave this out.

> +    virtual const AtomicString& formControlType() const;
> +
> +    virtual RenderObject* createRenderer(RenderArena*, RenderStyle*);
> +
> +    virtual void parseMappedAttribute(MappedAttribute*);

These should all be private.

> +    virtual void defaultEventHandler(Event*);

You should just leave this out.

> +    float value() const;
> +    void setValue(float);
> +
> +    float max() const;
> +    void setMax(float);

I don't think you need these, since [Reflect] in the IDL file should take care
of it.

> +    float position() const;

We may want to use double here instead of float. All numbers in JavaScript are
doubles, so choosing float incurs a performance penalty. Not sure if there's
some reason float is better.

> +    // NodeList* labels;

Please don't include any commented-out code. It's almost certainly not correct
to have NodeList.

> +progress constructorNeedsFormElement, conditional=PROGRESS_TAG, createWithNew

You should not use createWithNew. Also, we should make a test case to show that
the constructor really does need the form element. I'm not sure that's needed
for this.

If it is needed, we need a test to show the incorrect behavior you'd otherwise
get. And to try in other browsers. And read HTML5 to be sure it's helpful.

> +RenderProgress::~RenderProgress()
> +{
> +}

Same comment about unneeded destructors.

> +float RenderProgress::value() const
> +{
> +    HTMLProgressElement* element = static_cast<HTMLProgressElement*>(node());
> +    if (element)
> +        return element->value();
> +    return 1;
> +}

Do we really need this function? I don't think it's legal to have a
RenderProgress with a node() of 0. Is this really done often enough that we
need this helper function?

> +float RenderProgress::maxProgress() const
> +{
> +    HTMLProgressElement* element = static_cast<HTMLProgressElement*>(node());
> +    if (element)
> +        return element->max();
> +    return 1;
> +}

Same comment.

review- because I think you'll want to make at least some of the changes I
mentioned above.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.