[webkit-reviews] review denied: [Bug 45423] [Gtk] Port tiled backing store : [Attachment 69014] Updated version: Takes all the comments into account

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 45423: [Gtk] Port tiled backing store
https://bugs.webkit.org/show_bug.cgi?id=45423

Attachment 69014: Updated version: Takes all the comments into account
https://bugs.webkit.org/attachment.cgi?id=69014&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=69014&action=review

Looks good. I think we should avoid adding the option to GtkLauncher, as we try
to keep it very minimal and it isn't strictly necessary there. Since this is an
API change, it will need the approval of another GTK+ reviewer as well.

> WebCore/platform/graphics/gtk/TileGtk.cpp:27
> +   Copyright (C) 2010 codeaurora.org
> +   All rights reserved.
> +
> +   Redistribution and use in source and binary forms, with or without
> +   modification, are permitted provided that the following conditions
> +   are met:
> +   1. Redistributions of source code must retain the above copyright
> +   notice, this list of conditions and the following disclaimer.
> +   2. Redistributions in binary form must reproduce the above copyright
> +   notice, this list of conditions and the following disclaimer in the
> +   documentation and/or other materials provided with the distribution.
> +   3. The name of the author may not be used to endorse or promote products
> +   derived from this software without specific prior written permission.
> +
> +   THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
> +   IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES

> +   OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
> +   IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
> +   INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> +   NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,

> +   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> +   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> +   THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +*/
> +

Do you mind using the same license formatting as other files in the
WebCore/graphics/gtk/ directory? There's an example of a BSD license in
FontGtk.cpp. We should be as consistent as possible.

> WebCore/platform/graphics/gtk/TileGtk.cpp:45
> +static const unsigned checkerSize = 16;
> +static const unsigned checkerColor1 = 0xff555555;
> +static const unsigned checkerColor2 = 0xffaaaaaa;

If these are only used in checkeredPixmap, shouldn't they be defined there?

> WebCore/platform/graphics/gtk/TileGtk.cpp:50
> +    if (!pixmap) {

In WebKit, early returns are preferred, because it makes for shallower blocks.

> WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:381
> +#if ENABLE(TILED_BACKING_STORE)
> +    Frame* frame = core(m_webView)->mainFrame();
> +    if (frame && frame->tiledBackingStore())
> +	   frame->tiledBackingStore()->adjustVisibleRect();
> +#endif
> +

Can you confirm that this is needed here or if doing it in
::contentsSizeChanged is enough. I just took a guess on where this was
necessary, so I'm just surprised that I was right. :)

> WebKit/gtk/webkit/webkitwebview.cpp:519
> +static void renderFromTiledBackingStore(Frame *frame, GraphicsContext*
context, IntRect rect)

Frame *frame should be Frame* frame.