Bug 142750 - WebKit1 Clients Are Not Reliably Repainted
Summary: WebKit1 Clients Are Not Reliably Repainted
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-03-16 14:52 PDT by Brent Fulgham
Modified: 2015-03-16 16:01 PDT (History)
2 users (show)

See Also:


Attachments
Patch (4.71 KB, patch)
2015-03-16 15:00 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (4.41 KB, patch)
2015-03-16 15:29 PDT, Brent Fulgham
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2015-03-16 14:52:54 PDT
In Bug 135514 I corrected a problem where paint operations were being performed during layout. An unintended side-effect of this was that WebKit clients that performed a programmatic scroll of web content (outside of WebKit's control) would no longer receive a "free" paint operation.

To avoid breaking clients that relied on this misfeature, we need to notify AppKit that the view needs to be repainted once layout completes.
Comment 1 Brent Fulgham 2015-03-16 15:00:58 PDT
Created attachment 248751 [details]
Patch
Comment 2 Brent Fulgham 2015-03-16 15:01:43 PDT
<rdar://problem/20042453>
Comment 3 Simon Fraser (smfr) 2015-03-16 15:05:02 PDT
Comment on attachment 248751 [details]
Patch

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

> Source/WebCore/page/FrameView.h:116
> +    WEBCORE_EXPORT bool isInViewSizeAdjust() { return m_layoutPhase == InViewSizeAdjust; }

I think this would be better called something like inPaintableState(), and it should test the inverse.

> Source/WebKit/mac/WebView/WebClipView.mm:118
> +    if (paintable)
> +        [[self window] _disableDelayedWindowDisplay];

Is this still required?

> Source/WebKit/mac/WebView/WebClipView.mm:123
> +    if (paintable)
> +        [[self window] _enableDelayedWindowDisplay];

And this?
Comment 4 Brent Fulgham 2015-03-16 15:29:52 PDT
Created attachment 248755 [details]
Patch
Comment 5 Simon Fraser (smfr) 2015-03-16 15:41:48 PDT
Comment on attachment 248755 [details]
Patch

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

> Source/WebCore/page/FrameView.h:116
> +    WEBCORE_EXPORT bool inPaintableState() { return m_layoutPhase != InViewSizeAdjust && !isInLayout(); }

I don't think this is tight enough. FrameView::layout() sets the phase to InPostLayout before we've done updateLayerPositionsAfterLayout(), and it's certainly bad to paint before we've updated layers. I would prohibit painting with InLayout, InViewSizeAdjust and InPostLayout.

> Source/WebKit/mac/WebView/WebClipView.mm:119
> +                    [self setNeedsDisplay: YES];

No space after the :
I think this deserves a comment describing the situation under which this can occur.
Comment 6 Brent Fulgham 2015-03-16 16:01:53 PDT
Committed r181587: <http://trac.webkit.org/changeset/181587>