Bug 34214 – HostWindow repaint and friends need a refactor

• Component: Platform
• Status: RESOLVED FIXED

[Adam Treat]

Currently HostWindow sports a repaint method with four parameters, three of which are bools.  This could stand a good refactoring.  As discussed with Hyatt on IRC I will attempt to clean this up by separating this one method out into a few other methods to eliminate the bool parameters.  Patch forthcoming.

[Adam Treat]

Created attachment 47534 [details]
Refactor of HostWindow repaint and friends

I've tried to be very careful to not introduce behavior changes to any of the ports.  But please check my work.

Comment on attachment 47534 [details]
Refactor of HostWindow repaint and friends

I suggest that you avoid using “blit” as the name of a method. A block transfers is a very well-defined low-level operation, and clearly the method you call blit() involves more than that.

[Dave Hyatt]

Will review later today when I am at my computer.

[Adam Treat]

(In reply to comment #3)
> Will review later today when I am at my computer.

Any chance you can find some time to review this one?  Or suggest someone else who might be able to?

[Eric Seidel (no email)]

Looks like the patch does not apply (at least on the EWS bots).

[Adam Treat]

I'll work on getting one that will apply with latest.

[Dave Hyatt]

Comment on attachment 47534 [details]
Refactor of HostWindow repaint and friends

I don't like some of the names chosen here.  I don't think they're very clear.

There are basically eight combinations for the repaint method.

The operations that are interesting in repaint are:
(1) Adding a dirty region to the backing store (contentChanged = true).
(2) Adding a dirty region to your native window (repaintContentOnly = false)
(3) Painting immediately (immediate = true) (which either means updating the backing store synchronously or updating the native window synchronously)

Thus the following operations are possible (both synchronous and asynchronous):

(1) Add a dirty region to the backing store and invalidate it (contentChanged = true, repaintContentOnly = true)
(2) Invalidate a portion of the window (contentChanged = false, repaintContentOnly = false)
(3) Add a dirty region to the backing store and invalidate both the backing store and the window (contentChanged = true, repaintContentOnly = false)
(4) Do nothing (contentChanged = false, repaintContentOnly = true).  Nonsense option.

So now we're down to six options. I think the way to break this up is to separate repainting of contents from just invalidating the window.

Here are my suggestions:

virtual void invalidateContents(const IntRect& updateRect, bool immediate = false);
    This method covers (1) above, both the synchronous and asynchronous case.

virtual void invalidateWindow(const IntRect& updateRect, bool immediate = false);
    This method covers (2) above, both the synchronous and asynchronous case.

virtual void invalidateContentsAndWindow(const IntRect& updateRect, bool immediate = false);
    This method covers (3) above, both the synchronous and asynchronous case.

[Adam Treat]

(In reply to comment #7)
> (From update of attachment 47534 [details])
> I don't like some of the names chosen here.  I don't think they're very clear.
> 
> There are basically eight combinations for the repaint method.
> 
> The operations that are interesting in repaint are:
> (1) Adding a dirty region to the backing store (contentChanged = true).
> (2) Adding a dirty region to your native window (repaintContentOnly = false)
> (3) Painting immediately (immediate = true) (which either means updating the
> backing store synchronously or updating the native window synchronously)
> 
> Thus the following operations are possible (both synchronous and asynchronous):
> 
> (1) Add a dirty region to the backing store and invalidate it (contentChanged =
> true, repaintContentOnly = true)
> (2) Invalidate a portion of the window (contentChanged = false,
> repaintContentOnly = false)
> (3) Add a dirty region to the backing store and invalidate both the backing
> store and the window (contentChanged = true, repaintContentOnly = false)
> (4) Do nothing (contentChanged = false, repaintContentOnly = true).  Nonsense
> option.
> 
> So now we're down to six options. I think the way to break this up is to
> separate repainting of contents from just invalidating the window.
> 
> Here are my suggestions:
> 
> virtual void invalidateContents(const IntRect& updateRect, bool immediate =
> false);
>     This method covers (1) above, both the synchronous and asynchronous case.
> 
> virtual void invalidateWindow(const IntRect& updateRect, bool immediate =
> false);
>     This method covers (2) above, both the synchronous and asynchronous case.
> 
> virtual void invalidateContentsAndWindow(const IntRect& updateRect, bool
> immediate = false);
>     This method covers (3) above, both the synchronous and asynchronous case.

So these three methods + the two scrolling methods? hostWindow()->scroll and hostWindow->repaintForSlowScroll ?

I think it is important to keep the scrolling methods separate as this provides contextual information to the port about *why* the invalidation is taking place.  Different ports have very different painting models and I know that it is useful to have this contextual information in some cases.

[Adam Treat]

Created attachment 49835 [details]
Second try adopting review comments

Once again, I've tried to ensure no behavior changes.  However, this is a bit confusing so please pick over.

[WebKit Review Bot]

Attachment 49835 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/ScrollView.cpp:935:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Treat]

Created attachment 49837 [details]
Third try fixing misc style and build fixes for haiku/chromium ports

Fixes a style problem that wasn't introduced by this patch, but whatever.  Also fixes compile failure on haiku and chromium ports.

[WebKit Review Bot]

Attachment 49837 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/322041

[Adam Treat]

Created attachment 49862 [details]
Fourth try fixing chromium build

Chromium has an extra FramelessScrollView class which needed to be fixed up.  Thanks trybot!

[WebKit Review Bot]

Attachment 49862 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/322143

[WebKit Review Bot]

Attachment 49862 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/323250

[Xan Lopez]

(In reply to comment #15)
> Attachment 49862 [details] did not build on gtk:
> Build output: http://webkit-commit-queue.appspot.com/results/323250

You are missing the WebCore:: namespace prefix in the header. The patch looks great other that, I was recently looking at this code and it makes it much easier to understand.

[Kenneth Rohde Christiansen]

Comment on attachment 49862 [details]
Fourth try fixing chromium build


> +    virtual void invalidateContents(const IntRect&, bool) { }
> +    virtual void invalidateWindow(const IntRect&, bool) { }
> +    virtual void invalidateContentsAndWindow(const IntRect&, bool) { }
> +    virtual void invalidateContentsForSlowScroll(const IntRect&, bool) {};

Looking at these methods, it almost seems like an idea to use a flag instead, like invalidate(Contents | Window, rect, false);

What is the reason for not doing this?

[Adam Treat]

Created attachment 49901 [details]
Fifth effort fixing chromium again and gtk

Seems chromium has a few more classes that depend on scrollview/hostwindow.  And gtk needs WebCore:: prefix in the header.

Kenneth, I thought it was cleaner to have different methods than a flag and because hyatt suggested it.  Also, because I want a different method for the slow scroll case altogether to give ports contextual information about why that rect is being invalidated.

[Dave Hyatt]

Comment on attachment 49901 [details]
Fifth effort fixing chromium again and gtk

r=me

[Adam Treat]

Committed with r55675.

[Simon Fraser (smfr)]

This caused bug 37027