Bug 41737

Created attachment 60681 [details] a repro

Created attachment 60690 [details] patch v0

Comment on attachment 60690 [details] patch v0 The general form of this patch looks good. I like the test, and I like compositing over inheritance. I just don't quite understand enough of how things work in the rendering folder to R+ this patch. Hopefully this will be an easy review for some of our rendering friends.

Comment on attachment 60690 [details] patch v0 WebCore/rendering/RenderThemeChromiumWin.cpp:91 + PassOwnPtr<TransparencyWin> fallback = adoptPtr(new TransparencyWin()); Style nit: Use OwnPtr. PassOwnPtr should only be used for (return and input) arguments. WebCore/rendering/RenderThemeChromiumWin.cpp:89 + // Because TransparencyWin doesn't allocate temporally buffer for NoLayer, s/temporally/temporary/ WebCore/ChangeLog:9 + allocate a temporal buffer to composition. This change add s/add/adds a/ WebCore/ChangeLog:10 + fallback path to ThemePainter to handle the bufer allocation s/bufer/buffer/ WebCore/ChangeLog:13 + ThemePainter is no lonnger a subclass of TransparencyWin. It has s/lonnger/longer/ WebCore/ChangeLog:9 + allocate a temporal buffer to composition. This change add s/to/for/ WebCore/ChangeLog:19 + (WebCore::TransparencyWin::destContext): Added to expose an internal state. Exposed state for use in the creation of the fallback window. WebCore/ChangeLog:22 + (WebCore::TransparencyWin::transformMode): Added to expose an internal state. s/Added to expose an internal state./Ditto./ WebCore/ChangeLog:24 + (WebCore::ThemePainter): Added a fallabck path. s/fallabck/fallback/ WebCore/rendering/RenderThemeChromiumWin.cpp:92 + fallback->init(m_helper.destContext(), TransparencyWin::NoLayer, m_helper.transformMode(), m_helper.sourceRect()); ASSERT(fallback->context()); ? WebCore/rendering/RenderThemeChromiumWin.cpp:88 + ASSERT(!m_fallbackHelper); This assert seems odd since this method doesn't even touch m_fallbackHelper. WebCore/rendering/RenderThemeChromiumWin.cpp:90 + // it can be used for fallback when buffer allocation failed. Why is it ok to use NoLayer? WebCore/platform/graphics/chromium/TransparencyWin.h:148 + GraphicsContext* destContext() const { return m_destContext; } Avoid abbreviations: s/dest/destination/ (for the method name). WebCore/rendering/RenderThemeChromiumWin.cpp:65 + m_helper.init(context, getLayerMode(context, transformMode), transformMode, r); Why not just do this? if (!m_helper.context()) m_helper(context, TransparencyWin::NoLayer, transformMode, r); Then, get rid of a lot of stuff like helper(), etc.

Created attachment 61477 [details] patch v1; took feedbacks

This Looks Good To Me.

> This Looks Good To Me. Thanks! I'll land this.

Created attachment 61595 [details] patch to land

Comment on attachment 61595 [details] patch to land Rejecting patch 61595 from commit-queue. Unexpected failure when processing patch! Please file a bug against webkit-patch. Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 61595, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1 Last 500 characters of output: 1595&action=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=41737&ctype=xml Processing 1 patch from 1 bug. Cleaning working directory Processing patch 61595 from bug 41737. NOBODY (OOPS!) found in /Users/eseidel/Projects/CommitQueue/LayoutTests/ChangeLog does not appear to be a valid reviewer according to committers.py. ERROR: /Users/eseidel/Projects/CommitQueue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Created attachment 61606 [details] patch to land (retry)

Comment on attachment 61606 [details] patch to land (retry) Rejecting patch 61606 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1 Running build-dumprendertree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 20689 test cases. fast/forms/large-parts.html -> failed Exiting early after 1 failures. 7632 tests run. 162.44s total testing time 7631 test cases (99%) succeeded 1 test case (<1%) had incorrect layout Full output: http://webkit-commit-queue.appspot.com/results/3409408

Created attachment 61761 [details] last patch was broken during a patch juggling. fixed and retrying

Comment on attachment 61761 [details] last patch was broken during a patch juggling. fixed and retrying Clearing flags on attachment: 61761 Committed r63511: <http://trac.webkit.org/changeset/63511>

All reviewed patches have been landed. Closing bug.