[Webkit-unassigned] [Bug 45423] [Gtk] Port tiled backing store

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=45423





--- Comment #8 from Martin Robinson <mrobinson at webkit.org>  2010-09-22 07:51:52 PST ---
(From update of attachment 68365)
View in context: https://bugs.webkit.org/attachment.cgi?id=68365&action=review

Looks better. Eventually we need to do something about regions to get us out of this #ifdef jungle. That doesn't have to happen in this patch though.

> WebCore/platform/graphics/gtk/TileGtk.cpp:16
> +   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

What license is this? It look like BSD, plus another clause. I feel uncomfortable r+ing a patch with a unique license. I believe you need to use one of the licenses found in the LICENSE* files in the WebCore directory. Also, the block is not formatted like blocks in other files.

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

Please use an early return here.

> WebCore/platform/graphics/gtk/TileGtk.cpp:69
> +        cairo_t* cr = cairo_create(surface.get());
> +        Color color1(checkerColor1);
> +        Color color2(checkerColor2);
> +        float red1, green1, blue1, alpha1;
> +        float red2, green2, blue2, alpha2;
> +        color1.getRGBA(red1, green1, blue1, alpha1);
> +        color2.getRGBA(red2, green2, blue2, alpha2);
> +        for (unsigned y = 0; y < checkerSize; y += checkerSize / 2) {
> +            bool alternate = y % checkerSize;
> +            for (unsigned x = 0; x < checkerSize; x += checkerSize / 2) {
> +                cairo_rectangle(cr, x, y, checkerSize / 2, checkerSize /2);
> +                cairo_set_source_rgb(cr, alternate ? red1 : red2, alternate ? green1 : green2, alternate ? blue1 : blue2);
> +                cairo_fill(cr);
> +                alternate = !alternate;
> +            }
> +        }
> +        pattern = cairo_pattern_create_for_surface (surface.get());
> +    }

Isn't it possible to switch to the stippling approach from the link I gave above?

> WebCore/platform/graphics/gtk/TileGtk.cpp:136
> +    gdk_region_get_rectangles (m_dirtyRegion, &rects.outPtr(), &rectCount);

Watch the spacing here.

> WebCore/platform/graphics/gtk/TileGtk.cpp:179
> +    cairo_save(cr);
> +    cairo_set_source_surface(cr, m_buffer.get(), target.x() - source.x(), target.y() - source.y());
> +    cairo_rectangle(cr, target.x(), target.y(), target.width(), target.height());
> +    cairo_fill(cr);
> +    cairo_restore(cr);

save/restore are pretty expensive, is it possible to just save the previous source and restore it after the operation?

> WebKit/gtk/webkit/webkitwebview.cpp:535
> +    context->save();
> +    context->translate(-scrollX, -scrollY);
> +    rect.move(scrollX, scrollY);
> +    frame->tiledBackingStore()->paint(context, rect);
> +    context->restore();

Save and restore are actually pretty expensive. Is it possible to translate the context back after the operation?

> WebKit/gtk/webkit/webkitwebview.cpp:572
> +            if (frame->tiledBackingStore()){ 

Watch the spacing around your parenthesis.

> WebKit/gtk/webkit/webkitwebview.cpp:573
> +                // FIXME:  We should set the backing store viewport earlier than in paint

Is it possible to fix this now?

> WebKit/gtk/webkit/webkitwebview.cpp:588
> +                if (frame->tiledBackingStore()){ 

Ditto.

> WebKit/gtk/WebCoreSupport/ChromeClientGtk.h:145
> +#if ENABLE(TILED_BACKING_STORE)
> +        virtual WebCore::IntRect visibleRectForTiledBackingStore() const;
> +#endif
> +

It seems this could just be a helper method in webkitwebview.cpp. The only member your using in ChromeClientGtk is m_webView, which is a local variable in the context you use it.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.