Bug 34214 - HostWindow repaint and friends need a refactor
: HostWindow repaint and friends need a refactor
Status: RESOLVED FIXED
: WebKit
Platform
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-01-27 06:14 PST by
Modified: 2010-04-02 10:42 PST (History)


Attachments
Refactor of HostWindow repaint and friends (33.70 KB, patch)
2010-01-27 07:47 PST, Adam Treat
no flags Review Patch | Details | Formatted Diff | Diff
Second try adopting review comments (34.12 KB, patch)
2010-03-02 13:16 PST, Adam Treat
no flags Review Patch | 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 Review Patch | Details | Formatted Diff | Diff
Fourth try fixing chromium build (34.69 KB, patch)
2010-03-02 16:09 PST, Adam Treat
no flags Review Patch | 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+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2010-01-27 07:47:11 PST -------
Created an attachment (id=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 From 2010-01-27 08:09:27 PST -------
(From update of attachment 47534 [details])
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 From 2010-01-27 08:23:49 PST -------
Will review later today when I am at my computer.
------- Comment #4 From 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 From 2010-02-01 15:44:21 PST -------
Looks like the patch does not apply (at least on the EWS bots).
------- Comment #6 From 2010-02-02 09:48:59 PST -------
I'll work on getting one that will apply with latest.
------- Comment #7 From 2010-02-02 10:52:21 PST -------
(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.
------- Comment #8 From 2010-02-02 13:28:24 PST -------
(In reply to comment #7)
> (From update of attachment 47534 [details] [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 From 2010-03-02 13:16:49 PST -------
Created an attachment (id=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 From 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 From 2010-03-02 13:26:53 PST -------
Created an attachment (id=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 From 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 From 2010-03-02 16:09:39 PST -------
Created an attachment (id=49862) [details]
Fourth try fixing chromium build

Chromium has an extra FramelessScrollView class which needed to be fixed up.  Thanks trybot!
------- Comment #14 From 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 From 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 From 2010-03-03 04:53:50 PST -------
(In reply to comment #15)
> Attachment 49862 [details] [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 From 2010-03-03 05:14:24 PST -------
(From update of attachment 49862 [details])

> +    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 From 2010-03-03 06:48:19 PST -------
Created an attachment (id=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 From 2010-03-08 11:21:18 PST -------
(From update of attachment 49901 [details])
r=me
------- Comment #20 From 2010-03-08 11:44:10 PST -------
Committed with r55675.
------- Comment #21 From 2010-04-02 10:42:08 PST -------
This caused bug 37027