Bug 36961 – Enable HTMLProgressElement for Safari on OSX

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Yael]

A patch is coming soon.

[Yael]

Created attachment 52297 [details]
Patch

This patch adds painting code in RenderThemeMac for the progress bar.
Some assumptions I made in the patch:
1. fps for the progress bar is hard-coded at 33, seems that software update's progress bar has the same fps.
2. The animation of progress bar has 256 frames
3. Using HITheme to draw directly into the page gives poor results, so it draws into an image buffer first, and copied from there. The same technique is used for scrollbars.

[Kent Tamura]

Comment on attachment 52297 [details]
Patch

> Index: WebCore/html/HTMLProgressElement.idl
> ===================================================================
> --- WebCore/html/HTMLProgressElement.idl	(revision 56912)
> +++ WebCore/html/HTMLProgressElement.idl	(working copy)
> @@ -21,10 +21,12 @@ module html {
>      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

Why did you disabled bindings for other languages?

[Yael]

(In reply to comment #2)
> (From update of attachment 52297 [details])
> > Index: WebCore/html/HTMLProgressElement.idl
> > ===================================================================
> > --- WebCore/html/HTMLProgressElement.idl	(revision 56912)
> > +++ WebCore/html/HTMLProgressElement.idl	(working copy)
> > @@ -21,10 +21,12 @@ module html {
> >      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
> 
> Why did you disabled bindings for other languages?

I have been told that only people from Apple can add bindings for OBJC, as that touches public API. Is there another flag I should add to enable bindings for V8? If so, what is the flag?

[Kent Tamura]

(In reply to comment #3)
> > Why did you disabled bindings for other languages?
> 
> I have been told that only people from Apple can add bindings for OBJC, as that
> touches public API. Is there another flag I should add to enable bindings for
> V8? If so, what is the flag?

I understand though I haven't heard it.
V8 is JavaScript. So you don't need to worry about it.

[Darin Adler]

Comment on attachment 52297 [details]
Patch

>      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.

[Darin Adler]

(In reply to comment #5)
> Please fix at least one of the things I mention above.

At least some, I mean.

[Yael]

Thank you for the review.
(In reply to comment #5)
> > +    trackInfo.enableState = o->document()->frame()->page()->focusController()->isActive() ?  kThemeTrackActive : kThemeTrackInactive;
> 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.
> 
I was very reluctant to do that, but could not find a better way. Perhaps anyone has a suggestion for me?

[Darin Adler]

(In reply to comment #7)
> I was very reluctant to do that, but could not find a better way. Perhaps
> anyone has a suggestion for me?

We should look at how this is done with scrollbars or other such controls.

[Darin Adler]

(In reply to comment #7)
> I was very reluctant to do that, but could not find a better way. Perhaps
> anyone has a suggestion for me?

Can you use the RenderTheme::isActive(RenderObject*) function?

[Darin Adler]

(In reply to comment #9)
> Can you use the RenderTheme::isActive(RenderObject*) function?

Yes, that's the way to do it.

[Yael]

(In reply to comment #10)
> (In reply to comment #9)
> > Can you use the RenderTheme::isActive(RenderObject*) function?
> 
> Yes, that's the way to do it.

thanks :-)

[Yael]

Created attachment 52412 [details]
Patch, address comment #5

(In reply to comment #5)
>>  # 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?

The animation causes the result of the pixel test to depend on the speed of executing the test, and that will cause it to be flaky.
I introduced a manual test instead at WebCore/manual-tests/dom/progressbar.html

[Darin Adler]

(In reply to comment #12)
> (In reply to comment #5)
> >>  # 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?
> 
> The animation causes the result of the pixel test to depend on the speed of
> executing the test, and that will cause it to be flaky.

Then that's what the comment should say, rather than "renderTheme is not ready".

[Darin Adler]

Comment on attachment 52412 [details]
Patch, address comment #5

>  #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.

[Yael]

Created attachment 52458 [details]
Patch to address comment #14

This patch addresses comment #14 but does not address the ColorSpace issue.
I suppose there are at least 2 ways to address that:

1. Do the same as ScrollbarThemeMac does and use DeviceColorSpace.
2. Do something like paintInfo.context->drawImage(imageBuffer->image(), imageBuffer->context()->fillColorSpace(), rect.location());

But that would work only if HIThemeDrawTrack would set the color. 

What do you think?

[Darin Adler]

(In reply to comment #15)
> 1. Do the same as ScrollbarThemeMac does and use DeviceColorSpace.

This is fine for now. But I think the code in ScrollbarThemeMac may be incorrect and so both these should be fixed. Ideally we could come up with a way to share code with ScrollbarThemeMac so that we can fix this in one place.

> 2. Do something like paintInfo.context->drawImage(imageBuffer->image(),
> imageBuffer->context()->fillColorSpace(), rect.location());

That change won't do any good.

> But that would work only if HIThemeDrawTrack would set the color. 

I'm not sure what you mean.

[Darin Adler]

(In reply to comment #15)
> This patch addresses comment #14 but does not address the ColorSpace issue.

I believe the way to address the color space issue is to pass some color space other than DeviceRGB when creating the image buffer. We should file another bug about that.

[WebKit Commit Bot]

Comment on attachment 52458 [details]
Patch to address comment #14

Rejecting patch 52458 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against webkit-patch.
Failed to run "['WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', '52458', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
it
Fetching: https://bugs.webkit.org/show_bug.cgi?id=36961&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Processing patch 52458 from bug 36961.
Eric Carlson and Eric Seidel found in /Users/eseidel/Projects/CommitQueue/LayoutTests/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /Users/eseidel/Projects/CommitQueue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[WebKit Commit Bot]

Comment on attachment 52458 [details]
Patch to address comment #14

Clearing flags on attachment: 52458

Committed r57051: <http://trac.webkit.org/changeset/57051>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Looks like this caused a delayed build-break of 5 tests on the bots.  They need new baselines now that this is enabled on mac:
http://trac.webkit.org/changeset/57051/trunk/WebCore/Configurations/FeatureDefines.xcconfig

[Adam Barth]

This broke 5 tests because they enumerate everything the global scope and can now see HTMLProgressElement-related objects.  Updated expectations landed in <http://trac.webkit.org/changeset/57087>.

[Eric Seidel (no email)]

Filed bug 37102 about the build system failure.

[Yael]

(In reply to comment #22)
> This broke 5 tests because they enumerate everything the global scope and can
> now see HTMLProgressElement-related objects.  Updated expectations landed in
> <http://trac.webkit.org/changeset/57087>.

Sorry about that, and thanks for fixing it.

[Eric Seidel (no email)]

Looks like the Gtk bot has HTMLProgressElement off:
http://build.webkit.org/results/GTK%20Linux%2032-bit%20Release/r57088%20(10818)/results.html
meaning there are still failures after adam's updates.

[Yael]

Updated GTK expected results in http://trac.webkit.org/changeset/57092.

[Yael]

Updated win expected results in http://trac.webkit.org/changeset/57097.