[webkit-reviews] review denied: [Bug 36961] Enable HTMLProgressElement for Safari on OSX : [Attachment 52297] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Yael <yael.aharon at nokia.com>'s request
for review:
Bug 36961: Enable HTMLProgressElement for Safari on OSX
https://bugs.webkit.org/show_bug.cgi?id=36961

Attachment 52297: Patch
https://bugs.webkit.org/attachment.cgi?id=52297&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
>      interface [
>	   Conditional=PROGRESS_TAG
>      ] HTMLProgressElement : HTMLElement {
> +#if defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT
>		    attribute  double		     value;
>		    attribute  double		     max;
>	   readonly attribute  double		     position;
>	   readonly attribute  HTMLFormElement	     form;
> +#endif
>      };

This is wrong. We don't want to leave the bindings for this out of the
Objective-C DOM API.

We do want them to start as private, but that's automatic for anything not in
PublicDOMOperations.h.

> +#if ENABLE(PROGRESS_TAG)
> +class RenderProgress;
> +#endif

There's no need to put this inside an #if. Also, such forward declarations
should go below all #import and #include.

> +#if ENABLE(PROGRESS_TAG)
> +#import "RenderProgress.h"
> +#endif

Conditional includes go in a separate paragraph of their own, not sorted into
the main paragraph of includes.

> +#if ENABLE(PROGRESS_TAG)
> +const double progressAnimationRepeatInterval = 0.033;
> +const double progressAnimationDuration = 256 * 0.033;

These constants should go at the top of the file, not sorted in with the
functions. And there should be comments explaining where these values came
from, even if they just came from your guesswork or something like that.

> +bool RenderThemeMac::paintProgressBar(RenderObject* o, const
RenderObject::PaintInfo& pi, const IntRect& r)

I would prefer that you use words for argument names in new code, not letters.

> +    trackInfo.max = std::numeric_limits<int>::max();

You should not be using "std::" here. And as long as you are doing it this way,
I suggest using the actual type of trackInfo.max:

    trackInfo.max = numeric_limits<SInt32>::max();

> +    trackInfo.value = (SInt32)(renderProgress->position() *
std::numeric_limits<int>::max());

A typecast should not be needed. If it is needed, it should be a static_cast.

But do we really want truncation rather than rounding? I think what we want
here is lround. And multiplying a value in the range 0 to 1 by an integer is
not exactly the right way to scale it. I think it is more correct to write
this:

    trackInfo.value = lround(renderProgress->position() *
nextafter(trackInfo.max, 0));

But please don't take my word for it; I have not tested the code above so you
should.

> +    trackInfo.trackInfo.progress.phase =
(renderProgress->animationProgress() * progressAnimationDuration /
progressAnimationRepeatInterval);

Please leave out the parentheses.

I'm surprised you do not need a typecast here to narrow from a double to an
SInt8 given that you had one above.

> +    trackInfo.attributes = 0;
> +    trackInfo.attributes |= kThemeTrackHorizontal;

This should just be a single assignment statement, not two.

> +    trackInfo.bounds = r;

> +    trackInfo.bounds = IntRect(IntPoint(), r.size());

You should not set trackInfo.bounds twice.

There should also be these:

    trackInfo.reserved = 0;
    trackInfo.filler1 = 0;

> +    trackInfo.enableState =
o->document()->frame()->page()->focusController()->isActive() ? 
kThemeTrackActive : kThemeTrackInactive;

Extra space here after the "?".

What guarantees that page() won't be 0? I think you need a null check for that.


Is this really the right way to check for the active state? It borders on a
layering violation to dig into the document like this.

> +    CGContextSaveGState(pi.context->platformContext());
> +
> +    if (renderProgress->style()->direction() == RTL) {
> +	   CGContextTranslateCTM(pi.context->platformContext(), 2 * r.x() +
r.width(), 0);
> +	   CGContextScaleCTM(pi.context->platformContext(), -1, 1);
> +    }
> +    HIThemeDrawTrack(&trackInfo, 0,
imageBuffer->context()->platformContext(), kHIThemeOrientationNormal);
> +    pi.context->drawImage(imageBuffer->image(), DeviceColorSpace,
r.location());
> +
> +    CGContextRestoreGState(pi.context->platformContext());
> +    return false;

I suggest calling HIThemeDrawTrack first, before calling CGContextSaveGState or
anything else.

For the code to draw from the image buffer into the context from PaintInfo I
suggest we consider WebCore GraphicsContext functions instead of Mac-specific
CGContext functions.

>  # renderTheme is not ready to draw progress element
>  fast/dom/HTMLProgressElement/progress-element.html
> -fast/dom/HTMLProgressElement/set-progress-properties.html

Why just one test, not both?

Please fix at least one of the things I mention above.