[Webkit-unassigned] [Bug 36961] Enable HTMLProgressElement for Safari on OSX

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

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




--- Comment #14 from Darin Adler <darin at apple.com>  2010-04-02 09:36:03 PST ---
(From update of attachment 52412)
>  #import "RenderTheme.h"
> +
>  #import <wtf/HashMap.h>
>  #import <wtf/RetainPtr.h>

This blank line should not be added.

> -
> +#if ENABLE(PROGRESS_TAG)
> +    // Returns the repeat interval of the animation for the progress bar.
> +    virtual double animationRepeatIntervalForProgressBar(RenderProgress*) const;
> +    // Returns the duration of the animation for the progress bar.
> +    virtual double animationDurationForProgressBar(RenderProgress*) const;
> +#endif
> +    

It would be better not to add the new whitespace here (in the line after
#endif). Especially since other places in the same file you are explicitly
removing trailing whitespace.

>  };
> -
> +    
>  } // namespace WebCore

Again, adding whitespace here and removing it elsewhere in the same file and
the same patch.

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

I think you could make this include unconditional.

> +// fps for the animation of the progress baris hard-coded at 33 fps, it seems to be the same as other applications
> +// that use a progress bar on OSX.
> +const double progressAnimationRepeatInterval = 0.033;

I would make the comment be this:

    // We estimate the animation rate of a Mac OS X progress bar is 33 fps.
    // Hard code the value here because we haven't found API for it.

The comment in the patch refers to "OSX", which is not correct, also has the
typo "baris", refers to "other applications" but WebKit is not an application.
And it doesn't mention the real issue, which is that we are guessing the value
and hard coding it here because we can't find API for it.

Also, I am not sure what a "repeat interval" is. It does not seem that this
animation "repeats" ever 33 frames per second. The constant should be named
progressAnimationFrameRate.

> +// It seems that the animation of the progress bar has 256 frames, thus the duration is estimated at 256 times the fps.
> +const double progressAnimationDuration = 256 * 0.033;

And this should be:

    // Mac OS X progress bar animation seems to have 256 frames.
    const int progressAnimationNumFrames = 256;

Then below you can replace:

    "progressAnimationDuration / progressAnimationRepeatInterval" with
"progressAnimationNumFrames"

And

    "progressAnimationDuration" with "progressAnimationNumFrames *
progressAnimationFrameRate"

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

I think the correct computation here would be:

    lround(renderProgress->animationProgress() *
nextafter(progressAnimationNumFrames, 0))

For the same reason the calculation for trackInfo.value above works like that.
But I am a little worried that the way this animation works is kind of
indirect.

I also think this won't compile because it is trackInfo.progress rather than
trackInfo.trackInfo.progress.

> +    IntRect bufferRect(trackInfo.bounds);
> +
> +    OwnPtr<ImageBuffer> imageBuffer = ImageBuffer::create(bufferRect.size());

I don't think we need bufferRect. bufferRect.size() is the same as rect.size(),
and there is no other use of bufferRect. Or you could write
IntRect(trackInfo.bounds).size().

> +    paintInfo.context->drawImage(imageBuffer->image(), DeviceColorSpace, rect.location());

I don't think the color space handling in this function is correct. The color
space passed should be renderObject->style()->colorSpace().

However to get color right we also have to set up the image buffer with the
correct color space. I think what we actually want is a way to create an image
buffer with sRGB color space when renderObject->style()->colorSpace() is
sRGBColorSpace, but ImageBuffer.h does not currently offer that option.

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

Comment is no longer correct and should be updated.

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