Bug 34214 - HostWindow repaint and friends need a refactor
: HostWindow repaint and friends need a refactor
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Platform
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-27 06:14 PST by Adam Treat
Modified: 2010-04-02 10:42 PDT (History)
13 users (show)

See Also:


Attachments
Refactor of HostWindow repaint and friends (33.70 KB, patch)
2010-01-27 07:47 PST, Adam Treat
no flags Details | Formatted Diff | Diff
Second try adopting review comments (34.12 KB, patch)
2010-03-02 13:16 PST, Adam Treat
no flags Details | Formatted Diff | Diff
Third try fixing misc style and build fixes for haiku/chromium ports (34.13 KB, patch)
2010-03-02 13:26 PST, Adam Treat
no flags Details | Formatted Diff | Diff
Fourth try fixing chromium build (34.69 KB, patch)
2010-03-02 16:09 PST, Adam Treat
no flags Details | Formatted Diff | Diff
Fifth effort fixing chromium again and gtk (37.61 KB, patch)
2010-03-03 06:48 PST, Adam Treat
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Treat 2010-01-27 06:14:30 PST
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.
Comment 1 Adam Treat 2010-01-27 07:47:11 PST
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 2 mitz@webkit.org 2010-01-27 08:09:27 PST
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.
Comment 3 Dave Hyatt 2010-01-27 08:23:49 PST
Will review later today when I am at my computer.
Comment 4 Adam Treat 2010-02-01 13:55:31 PST
(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?
Comment 5 Eric Seidel 2010-02-01 15:44:21 PST
Looks like the patch does not apply (at least on the EWS bots).
Comment 6 Adam Treat 2010-02-02 09:48:59 PST
I'll work on getting one that will apply with latest.
Comment 7 Dave Hyatt 2010-02-02 10:52:21 PST
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.
Comment 8 Adam Treat 2010-02-02 13:28:24 PST
(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.
Comment 9 Adam Treat 2010-03-02 13:16:49 PST
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.
Comment 10 WebKit Review Bot 2010-03-02 13:19:42 PST
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.
Comment 11 Adam Treat 2010-03-02 13:26:53 PST
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.
Comment 12 WebKit Review Bot 2010-03-02 14:05:47 PST
Attachment 49837 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/322041
Comment 13 Adam Treat 2010-03-02 16:09:39 PST
Created attachment 49862 [details]
Fourth try fixing chromium build

Chromium has an extra FramelessScrollView class which needed to be fixed up.  Thanks trybot!
Comment 14 WebKit Review Bot 2010-03-02 17:15:07 PST
Attachment 49862 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/322143
Comment 15 WebKit Review Bot 2010-03-02 21:30:48 PST
Attachment 49862 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/323250
Comment 16 Xan Lopez 2010-03-03 04:53:50 PST
(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.
Comment 17 Kenneth Rohde Christiansen 2010-03-03 05:14:24 PST
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?
Comment 18 Adam Treat 2010-03-03 06:48:19 PST
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.
Comment 19 Dave Hyatt 2010-03-08 11:21:18 PST
Comment on attachment 49901 [details]
Fifth effort fixing chromium again and gtk

r=me
Comment 20 Adam Treat 2010-03-08 11:44:10 PST
Committed with r55675.
Comment 21 Simon Fraser (smfr) 2010-04-02 10:42:08 PDT
This caused bug 37027