Bug 130736 – Support form controls that may need incremental redraw

• Component: Forms
• Status: RESOLVED FIXED

[Dean Jackson]

Some system controls do not render completely in a single draw operation. Add support in RenderTheme such that a control can trigger a redraw of itself in the near future.

[Dean Jackson]

<rdar://problem/16175197>

[Dean Jackson]

Created attachment 227792 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 227792 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227792&action=review

> Source/WebCore/platform/Theme.h:39
> +typedef unsigned ThemeClient;

Seems odd for a "client" to be an ID and not something implementing a virtual interface.

> Source/WebCore/platform/mac/ThemeMac.h:68
> +    HashMap<ThemeClient, ControlStates> m_clientStates;
> +    HashMap<ThemeClient, NSButtonCell*> m_clientButtons;

Would be better to have just one hash pointing to a class/struct that has both the states and cells.

> Source/WebCore/platform/mac/ThemeMac.mm:104
> +- (void)addSubview:(NSView*)subview {

Brace on new line.

> Source/WebCore/platform/mac/ThemeMac.mm:105
> +    // By doing nothing in this method we explicitly forbid controls from adding subviews.

Not sure this is adding.

> Source/WebCore/platform/mac/ThemeMac.mm:294
> +static NSButtonCell *newCheckbox()

I'd call this createCheckboxCell()

> Source/WebCore/platform/mac/ThemeMac.mm:300
> +    NSButtonCell *checkboxCell = [[NSButtonCell alloc] init];
> +    [checkboxCell setButtonType:NSSwitchButton];
> +    [checkboxCell setTitle:nil];
> +    [checkboxCell setAllowsMixedState:YES];
> +    [checkboxCell setFocusRingType:NSFocusRingTypeExterior];

BEGIN_BLOCK_OBJC_EXCEPTIONS around this.

> Source/WebCore/platform/mac/ThemeMac.mm:365
> +        context->translate(inflatedRect.x(), inflatedRect.y());
> +        context->scale(FloatSize(1, -1));
> +        context->translate(0, -inflatedRect.height());

Do you need to save and restore the context state?

> Source/WebCore/rendering/RenderObject.cpp:127
> +    theme().clearThemeForRenderer(this);

Ew! Should avoid this if at all possible, at least have it on RenderElement. And we should know if a renderer used the theme at all, right?

> Source/WebCore/rendering/RenderTheme.cpp:1328
> +    if (!m_themeClients.contains(o))
> +        m_themeClients.add(o, ++s_nextThemeClient);
> +    
> +    return m_themeClients.get(o);

I think there's a way to do this with fewer hash lookups.

> Source/WebCore/rendering/RenderTheme.h:374
> +    void timerFired(Timer<RenderTheme>&);

Should be rendererRepaintTimerFired()

> Source/WebCore/rendering/RenderTheme.h:416
> +    Vector<const RenderObject*> m_repaintList;

This should be m_renderersRequiringRepaint or something.

> Source/WebCore/rendering/RenderTheme.h:417
> +    OwnPtr<Timer<RenderTheme>> m_timer;

We don't usually heap-allocate Timers.

[Beth Dakin]

Comment on attachment 227792 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227792&action=review

Here is co-review feedback from me and Sam. Might have slightly more coming in a few minutes.

>> Source/WebCore/platform/Theme.h:39
>> +typedef unsigned ThemeClient;
> 
> Seems odd for a "client" to be an ID and not something implementing a virtual interface.

Sam agrees.

> Source/WebCore/platform/mac/ThemeMac.mm:39
> +#import <AppKit/NSButtonCell_Private.h>

This won't work for open source contributors. You should instead check if this header exists, and only then include it. Then always forward declare what you need from NSButtonCell.

>> Source/WebCore/platform/mac/ThemeMac.mm:105
>> +    // By doing nothing in this method we explicitly forbid controls from adding subviews.
> 
> Not sure this is adding.

Sam likes the comment. He thinks otherwise it will be confusing why we have a random stub.

> Source/WebCore/rendering/RenderTheme.cpp:70
> +static ThemeClient s_nextThemeClient = 0;

Sam wants to know why we are using an int instead of a pointer to the RenderObject.

> Source/WebCore/rendering/RenderTheme.cpp:305
> +        if (m_theme->paint(themeClientForRenderer(o), part, controlStatesForRenderer(o), const_cast<GraphicsContext*>(paintInfo.context), r, o->style().effectiveZoom(), &o->view().frameView())) {

Sam doesn't care for this API contract. He doesn't like that you are finding out that this control is animating based on the return value from the paint() function. That is explained in you header comment, but it is not inherently obvious that that is what the return value would mean. Makes for bad code clarity.

> Source/WebCore/rendering/RenderTheme.h:415
> +    HashMap<const RenderObject*, ThemeClient> m_themeClients;

It's not clear why we need this indirection. Can we just use the RenderObject as a void* as the client id?

[Dean Jackson]

(In reply to comment #4)

> >> Source/WebCore/platform/Theme.h:39
> >> +typedef unsigned ThemeClient;
> > 
> > Seems odd for a "client" to be an ID and not something implementing a virtual interface.
> 
> Sam agrees.

I'll just use void*. That's what I wanted at first, but thought people would complain about ugliness.

> 
> > Source/WebCore/platform/mac/ThemeMac.mm:39
> > +#import <AppKit/NSButtonCell_Private.h>
> 
> This won't work for open source contributors. You should instead check if this header exists, and only then include it. Then always forward declare what you need from NSButtonCell.

I had planned to declare a private category for these and not include anything.

> > Source/WebCore/rendering/RenderTheme.cpp:70
> > +static ThemeClient s_nextThemeClient = 0;
> 
> Sam wants to know why we are using an int instead of a pointer to the RenderObject.

I prefer this.

> 
> > Source/WebCore/rendering/RenderTheme.cpp:305
> > +        if (m_theme->paint(themeClientForRenderer(o), part, controlStatesForRenderer(o), const_cast<GraphicsContext*>(paintInfo.context), r, o->style().effectiveZoom(), &o->view().frameView())) {
> 
> Sam doesn't care for this API contract. He doesn't like that you are finding out that this control is animating based on the return value from the paint() function. That is explained in you header comment, but it is not inherently obvious that that is what the return value would mean. Makes for bad code clarity.

The alternative would be to query the theme after the paint, to see if it should schedule another paint.

[Dean Jackson]

Comment on attachment 227792 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227792&action=review

I should have replied via review tool :(

>>> Source/WebCore/platform/Theme.h:39
>>> +typedef unsigned ThemeClient;
>> 
>> Seems odd for a "client" to be an ID and not something implementing a virtual interface.
> 
> Sam agrees.

We don't need this if we change to using RO*

>> Source/WebCore/platform/mac/ThemeMac.mm:39
>> +#import <AppKit/NSButtonCell_Private.h>
> 
> This won't work for open source contributors. You should instead check if this header exists, and only then include it. Then always forward declare what you need from NSButtonCell.

I was going to declare a private category for this, and avoid including the file at all.

>> Source/WebCore/platform/mac/ThemeMac.mm:300
>> +    [checkboxCell setFocusRingType:NSFocusRingTypeExterior];
> 
> BEGIN_BLOCK_OBJC_EXCEPTIONS around this.

It's already called from within something that has done this.

>> Source/WebCore/platform/mac/ThemeMac.mm:365
>> +        context->translate(0, -inflatedRect.height());
> 
> Do you need to save and restore the context state?

There is a StateSaver just out of view above.

>> Source/WebCore/rendering/RenderTheme.cpp:70
>> +static ThemeClient s_nextThemeClient = 0;
> 
> Sam wants to know why we are using an int instead of a pointer to the RenderObject.

I thought people would complain if I did something ugly like that, but it is what I wanted... so I'll change!

>> Source/WebCore/rendering/RenderTheme.cpp:305
>> +        if (m_theme->paint(themeClientForRenderer(o), part, controlStatesForRenderer(o), const_cast<GraphicsContext*>(paintInfo.context), r, o->style().effectiveZoom(), &o->view().frameView())) {
> 
> Sam doesn't care for this API contract. He doesn't like that you are finding out that this control is animating based on the return value from the paint() function. That is explained in you header comment, but it is not inherently obvious that that is what the return value would mean. Makes for bad code clarity.

OK. I think the other alternative would be to query Theme after painting to ask if it needs to schedule another paint.

[Dean Jackson]

Created attachment 227969 [details]
Patch

[Dean Jackson]

Created attachment 227996 [details]
Patch

[Dean Jackson]

Created attachment 228014 [details]
Patch

[Dean Jackson]

I won't be surprised if this isn't happy on non-Mac ports.

[Dean Jackson]

Bummer. Needs rebase.

[Dean Jackson]

Created attachment 228016 [details]
Patch

[Dean Jackson]

Created attachment 228038 [details]
Patch

[Dean Jackson]

Created attachment 228070 [details]
Patch

[Dean Jackson]

Created attachment 228071 [details]
Patch

[Dean Jackson]

Created attachment 228074 [details]
Patch

[Dean Jackson]

Committed r166422: <http://trac.webkit.org/changeset/166422>

[Antti Koivisto]

Comment on attachment 228074 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=228074&action=review

> Source/WebCore/rendering/RenderBox.h:729
> +
> +    Timer<RenderBox> m_repaintTimer;

It is not ok to bloat every RenderBox with a timer.

[Antti Koivisto]

Comment on attachment 228074 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=228074&action=review

> Source/WebCore/rendering/RenderBox.cpp:1243
> +    if (controlStates && controlStates->needsRepaint())
> +        m_repaintTimer.startOneShot(0);

Triggering paints from paint is not nice. Considering we have managed without such concept so far I suspect there are ways to implement this without introducing it now.

How do we know this does not create an infinite paint loop?