ContainerNode::setActive should not sleep for 100ms on platforms that do not implement synchronous repaint(true) semantics
Created attachment 143117 [details] Patch
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.
> 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.
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
(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.
(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?
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.
(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?
(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.
Created attachment 191391 [details] Patch
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.
Created attachment 191522 [details] Patch for landing
Comment on attachment 191522 [details] Patch for landing Clearing flags on attachment: 191522 Committed r144795: <http://trac.webkit.org/changeset/144795>
All reviewed patches have been landed. Closing bug.