Bug 53715

Created attachment 81114 [details] v1 patch The corresponding changes to TestShell are here: http://codereview.chromium.org/6283019 (Those are not yet ready for review, but they give you an idea of how WebPaintSurface may be implemented.)

@brettw, it'd be great to get your input on the plugin related changes. This is my attempt to bring your plugin painting optimization into WebKit.

Attachment 81114 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7689755

Created attachment 81117 [details] v2 patch

Comment on attachment 81117 [details] v2 patch View in context: https://bugs.webkit.org/attachment.cgi?id=81117&action=review Seems pretty sane to me. We're supposed to use 2-clause BSD these days > Source/WebKit/chromium/src/painting/PaintController.cpp:201 > + context.setStrokeColor(colors[colorSelector++ & 3], ColorSpaceDeviceRGB); % 3, & 3 will allow the value '3' which will index out of bounds here

Attachment 81114 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1 Source/WebKit/chromium/src/WebPopupMenuImpl.h:131: Should have a space between // and comment [whitespace/comments] [4] Source/WebKit/chromium/src/WebPopupMenuImpl.h:132: Should have a space between // and comment [whitespace/comments] [4] Source/WebKit/chromium/src/painting/PaintController.cpp:36: Alphabetical sorting problem. [build/include_order] [4] Source/WebKit/chromium/src/painting/PaintController.cpp:207: One space before end of line comments [whitespace/comments] [5] Source/WebKit/chromium/src/WebViewImpl.h:209: Should have a space between // and comment [whitespace/comments] [4] Source/WebKit/chromium/src/WebViewImpl.h:210: Should have a space between // and comment [whitespace/comments] [4] Source/WebKit/chromium/src/painting/PaintAggregator.cpp:205: One space before end of line comments [whitespace/comments] [5] Source/WebKit/chromium/src/painting/PaintAggregator.cpp:248: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebKit/chromium/tests/PaintAggregatorTest.cpp:282: One space before end of line comments [whitespace/comments] [5] Source/WebKit/chromium/tests/PaintAggregatorTest.cpp:299: One space before end of line comments [whitespace/comments] [5] Source/WebKit/chromium/src/painting/PaintAggregator.h:83: The parameter name "rect" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 11 in 27 files If any of these errors are false positives, please file a bug against check-webkit-style.

Attachment 81117 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/WebKit.gyp', u'Sour..." exit_code: 1 Source/WebKit/chromium/src/WebPopupMenuImpl.h:131: Should have a space between // and comment [whitespace/comments] [4] Source/WebKit/chromium/src/WebPopupMenuImpl.h:132: Should have a space between // and comment [whitespace/comments] [4] Source/WebKit/chromium/src/painting/PaintController.cpp:36: Alphabetical sorting problem. [build/include_order] [4] Source/WebKit/chromium/src/painting/PaintController.cpp:207: One space before end of line comments [whitespace/comments] [5] Source/WebKit/chromium/src/WebViewImpl.h:209: Should have a space between // and comment [whitespace/comments] [4] Source/WebKit/chromium/src/WebViewImpl.h:210: Should have a space between // and comment [whitespace/comments] [4] Source/WebKit/chromium/src/painting/PaintAggregator.cpp:205: One space before end of line comments [whitespace/comments] [5] Source/WebKit/chromium/src/painting/PaintAggregator.cpp:248: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebKit/chromium/tests/PaintAggregatorTest.cpp:282: One space before end of line comments [whitespace/comments] [5] Source/WebKit/chromium/tests/PaintAggregatorTest.cpp:299: One space before end of line comments [whitespace/comments] [5] Source/WebKit/chromium/src/painting/PaintAggregator.h:83: The parameter name "rect" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 11 in 26 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #5) > (From update of attachment 81117 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81117&action=review > > Seems pretty sane to me. We're supposed to use 2-clause BSD these days ah, right. thanks! > > > Source/WebKit/chromium/src/painting/PaintController.cpp:201 > > + context.setStrokeColor(colors[colorSelector++ & 3], ColorSpaceDeviceRGB); > > % 3, & 3 will allow the value '3' which will index out of bounds here whoops... yeah, that's a typo in my translation of the chromium code :-/ thanks for catching it.

Comment on attachment 81114 [details] v1 patch View in context: https://bugs.webkit.org/attachment.cgi?id=81114&action=review > Source/WebKit/chromium/public/WebPaintSurface.h:61 > + // and the buffer may be deleted once flush() returns. May be deleted? What happens if I don't delete a WebPaintBuffer? Can i efficiently paint with it next frame? > Source/WebKit/chromium/public/WebPlugin.h:78 > + virtual WebPaintBuffer* cloneBackingBuffer() { return 0; } The name of this sort of confused me at first... I'm probably missing some critical detail, but can we reverse the impedence of this and say paintIntoSurface(WebPaintSurface*)? > Source/WebKit/chromium/src/WebViewImpl.h:360 > + void scheduleInvalidation(const WebCore::IntRect&); Can we remove WebViewImpl::invalidateRootLayerRect/scrollRootLayerRect completely and put their implementation behind scheduleInvalidation/etc? This might allow ChromeClient to be agnostic about compositing. > Source/WebKit/chromium/src/painting/PaintController.h:54 > +class PaintController : public WebPaintSurfaceClient { Hmm... isn't paintController ~= LayerChromium, plus an update loop? If we leave the updateTimer on WebView, we could fuse scheduleComposite with this update. That might then pave the way to another step which is fusing PaintController with LayerChromium. I'm trying to figure out the step after this where PaintController and LayerChromium fuse together a bit. > Source/WebKit/chromium/src/painting/Painter.h:49 > +public: If you hoist animate and layout of these (which are sort of global rather than RenderLayer-specific iirw) then you have something nearly identical to TilePaintInterface, which is Enne's abstraction for painting... maybe we can fuse those together.

(In reply to comment #9) > (From update of attachment 81114 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81114&action=review > > > Source/WebKit/chromium/public/WebPaintSurface.h:61 > > + // and the buffer may be deleted once flush() returns. > > May be deleted? What happens if I don't delete a WebPaintBuffer? Can i efficiently paint with it next frame? We have a choice. There is nothing that technically precludes it. We probably just want to strongly encourage people to not modify the WebPaintBuffer while it is being flushed :-) > > Source/WebKit/chromium/public/WebPlugin.h:78 > > + virtual WebPaintBuffer* cloneBackingBuffer() { return 0; } > > The name of this sort of confused me at first... Yeah, I had a hard time coming up with a good name. "clone" seemed better than "create" (sounds like it would return something that is new and uninitialized) or "get" (sounds like it would not be something the caller should need to free later). "clone" on the other hand sounds like it is a copy, which should be freed later. I used the term "backing buffer" to mimic the term already used to get the backing texture. > I'm probably missing some critical detail, but can we reverse the impedence of this and say paintIntoSurface(WebPaintSurface*)? Maybe. The problem with doing this is that we need to keep the buffer alive until flush is called. I could fix that by making it so that WebPaintBuffer is reference counted such that WebPaintSurface::paint would just take an owning reference. Yeah, this might be a lot nicer. It helps avoid the nasty problem of exposing the canvas of the underlying WebPaintBuffer of the plugin. I'll noodle on this some more... > > Source/WebKit/chromium/src/WebViewImpl.h:360 > > + void scheduleInvalidation(const WebCore::IntRect&); > > Can we remove WebViewImpl::invalidateRootLayerRect/scrollRootLayerRect completely and put their implementation behind scheduleInvalidation/etc? This might allow ChromeClient to be agnostic about compositing. Yes, we probably can. I'll give that a try. > > Source/WebKit/chromium/src/painting/PaintController.h:54 > > +class PaintController : public WebPaintSurfaceClient { > > Hmm... isn't paintController ~= LayerChromium, plus an update loop? Good question. I need to learn more about LayerChromium. > If we leave the updateTimer on WebView, we could fuse scheduleComposite with this update. That might then pave the way to another step which is fusing PaintController with LayerChromium. I definitely want to avoid adding complexity to WebView though... I had the idea of writing a PaintController unit test :) > I'm trying to figure out the step after this where PaintController and LayerChromium fuse together a bit. Yeah... this is something I haven't thought enough about yet. > > Source/WebKit/chromium/src/painting/Painter.h:49 > > +public: > > If you hoist animate and layout of these (which are sort of global rather than RenderLayer-specific iirw) then you have something nearly identical to TilePaintInterface, which is Enne's abstraction for painting... maybe we can fuse those together. Interesting. I'll take a look.

> I definitely want to avoid adding complexity to WebView though... > I had the idea of writing a PaintController unit test :) > Hmm! Like that bit, lots... Thinking out loud... WebViewPainter { -> recieves damage, knows how to paint webview onto websurface invalidate/scroll/needsAnimate; PaintController*; // forward invalidations here paint aggregator; WebView*; WebSurface*; } PaintController { -> schedules updates ("update controller"), now for just sw path, soon for hw rendering case too anim/layout/paint scheduling } WebView { PaintController; } ChromeClient { SoftwarePainter*; // forward invalidations directly here? idk... }

The API looks good overall. I'm also a bit confused about the "clone" part. So the caller needs to free the buffer? How can we avoid copying the backing store pixels in this case? Sorry if I missed something obvious, I'm a bit rushed at the moment.

(In reply to comment #12) > The API looks good overall. I'm also a bit confused about the "clone" part. So the caller needs to free the buffer? How can we avoid copying the backing store pixels in this case? Not exactly. The caller deletes the handle--the WebPaintBuffer. The implementation of WebPaintBuffer will most likely be reference counted. I used the term "clone" to indicate that the returned pointer should be freed, but that doesn't mean it has to be deep-copied :)

Comment on attachment 81117 [details] v2 patch View in context: https://bugs.webkit.org/attachment.cgi?id=81117&action=review I'm not sure whether you're still interested in this patch, but there are a number of style nits reported by the bot. I'm going to mark this R- for now because we'll at least need to clean up the style problems. If you're still interested in the patch, please feel encouraged to upload a new version for review. > Source/WebKit/chromium/src/WebPopupMenuImpl.cpp:320 > +#if WEBKIT_USING_PAINTSURFACE This doesn't seem like the right style of define. Wouldn't USE(PAINTSURFACE) or ENABLE(PAINTSURFACE) be more appropriate?

Created attachment 122319 [details] Patch

Will pull the surface stuff to a new patch. This imported the paint aggregator in a nice way. Lets get that landed. @fishd: r?

Comment on attachment 122319 [details] Patch OK... did you confirm that all/any downstream paint_aggregator.cc changes that may have occurred since the original patch were incorporated into this copy?

(In reply to comment #17) > (From update of attachment 122319 [details]) Yeah, I was amazed to find it hasn't been touched since January 2011 except for moves relating to content refactoring.

Created attachment 124768 [details] Patch for landing

Comment on attachment 124768 [details] Patch for landing Clearing flags on attachment: 124768 Committed r106399: <http://trac.webkit.org/changeset/106399>

All reviewed patches have been landed. Closing bug.