Bug 87054 – ContainerNode::setActive should not sleep for 100ms on platforms that do not implement synchronous repaint(true) semantics

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[James Robinson]

ContainerNode::setActive should not sleep for 100ms on platforms that do not implement synchronous repaint(true) semantics

[James Robinson]

Created attachment 143117 [details]
Patch

[James Robinson]

I proposed removing this logic completely in https://bugs.webkit.org/show_bug.cgi?id=65003 but it seemed that the owners of the mac port still wanted this for WebKit1.  It's not fair to add the cost of this hack to other ports without synchronous repaint behavior, however.

It would be better if this block could be disabled in WebKit2 on all ports as well since it doesn't work there.  I'm not sure how to do that from within WebCore.

[Alexey Proskuryakov]

> it seemed that the owners of the mac port still wanted this for WebKit1

The behavior looks desirable for WebKit2 as well, although implementation likely needs to be entirely different.

> I'm not sure how to do that from within WebCore.

Such differences between WK1 and WK2 are generally implemented as PlatformStrategies.

[Darin Adler]

Comment on attachment 143117 [details]
Patch

It seems to me you are saying that nobody except Apple on the Mac wants this feature of blinking visibly when someone does a programmatic click of a button. What we want to do is to express two different things at either compile time or runtime:

1) Do we want this feature at all?
2) Do we have the repaint(true) code implemented needed to implement the feature?

If either (1) or (2) is false, then we don’t want to do any of the logic and we certainly don’t want a gratuitous sleep.

What simplifies the situation, though, is that as far as I can tell, we have no platforms where (1) and (2) are true. Specifically, the WebKit client on Mac that wanted this was Safari, and Safari has been using WebKit2 and so not getting this feature for multiple years.

Thus, I think we should delete the code entirely rather than wrapping it this #if/FIXME mess.

If we were keeping it, I think we should do a better job of teasing out (1) and (2) rather than conflating them.

Alexey was arguing to keep the code, but I think that currently it’s doing more harm than good.

By the way, I think that everyone should want this feature on all platforms. It’s great to see the button pushed!

review- but I will review+ a patch to just remove this entirely

[James Robinson]

(In reply to comment #4)
> review- but I will review+ a patch to just remove this entirely

I will do this.  If someone feels strongly about the effect I'm sure there are other ways to accomplish it, but given that nobody's ever implemented it for WebKit2 or for non-mac ports it seems that it's not in tremendously high demand.

[James Robinson]

(In reply to comment #4)
> review- but I will review+ a patch to just remove this entirely

The patch at https://bugs.webkit.org/show_bug.cgi?id=65003 does that (although it may need to be refreshed a touch since it's from mid-2011).  Would you like to revisit your review- on that patch, or is there additional work you would like to see?

[Darin Adler]

Let me check with Dan Bernstein and some others who may be more thoughtful about whether there is value in keeping this for WebKit1 on Mac than I am, and I’ll get back to you either in this bug or that one.

[James Robinson]

(In reply to comment #7)
> Let me check with Dan Bernstein and some others who may be more thoughtful about whether there is value in keeping this for WebKit1 on Mac than I am, and I’ll get back to you either in this bug or that one.

Any update?

[James Robinson]

(In reply to comment #8)
> (In reply to comment #7)
> > Let me check with Dan Bernstein and some others who may be more thoughtful about whether there is value in keeping this for WebKit1 on Mac than I am, and I’ll get back to you either in this bug or that one.
> 
> Any update?

Friendly ping.  I would really like to avoid having useless usleep()s in the Chromium port if not all ports.

[James Robinson]

Created attachment 191391 [details]
Patch

[Darin Adler]

Comment on attachment 191391 [details]
Patch

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

Looks fine.

> Source/WebCore/ChangeLog:16
> +        (ChromeClient):

Should drop this bogus line. Wish the change log script would do a better job.

> Source/WebKit/mac/ChangeLog:11
> +        (WebChromeClient):

Should drop this bogus line. Wish the change log script would do a better job.

> Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:506
> +
> +

Extra blank line here.

[James Robinson]

Created attachment 191522 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 191522 [details]
Patch for landing

Clearing flags on attachment: 191522

Committed r144795: <http://trac.webkit.org/changeset/144795>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.