Clamp Mac theme painting to reasonable large bounds.

Clamp Mac theme painting to reasonable large bounds.

Previously we aimed to clip to the interest rect of the graphics layer in LocalCurrentGraphicsContext. However, in some cases the rect we clip to is the clip inherited from an overflow clip ancestor, which can lead to complexity w.r.t. caching.

Rather than reworking interest rect and cache skipping logic across the code base for Mac theme specific painting purposes, we choose to clamp Mac theme painting to a reasonable bounding size. This means huge elements requiring native element painting will no longer render correctly, but in such cases we may already have been performing poorly.

BUG=592915
Committed: https://crrev.com/ec218fb8a6013ab21b83590acf77530cbbce1097
Cr-Commit-Position: refs/heads/master@{#392468}

[wkorman, 2016-05-05 21:57:43]

Description was changed from

==========
Clamp Mac theme painting to reasonable large bounds.

BUG=592915
==========

to

==========
Clamp Mac theme painting to reasonable large bounds.

Previously we aimed to clip to the interest rect of the graphics layer in
LocalCurrentGraphicsContext. However, in some cases the rect we clip to is the
clip inherited from an overflow clip ancestor, which can lead to complexity
w.r.t. caching.

Rather than reworking interest rect and cache skipping logic across the code
base for Mac theme specific painting purposes, we choose to clamp Mac theme
painting to a reasonable bounding size. This means huge elements requiring
native element painting will no longer render correctly, but in such cases we
may already have been performing poorly.

BUG=592915
==========

[wkorman, 2016-05-05 22:09:26]

wkorman@chromium.org changed reviewers:
+ chrishtr@chromium.org

[wkorman, 2016-05-05 22:09:27]

Minimal change, open to fancier business as you think warranted. Perhaps I've
overlooked complexity and should be starting with another initial bounds and
inflating.

Working on a test.

[chrishtr, 2016-05-05 23:50:58]

https://codereview.chromium.org/1956583002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/mac/LocalCurrentGraphicsContext.mm
(right):

https://codereview.chromium.org/1956583002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mac/LocalCurrentGraphicsContext.mm:47: if
(!clampRect.contains(m_inflatedDirtyRect)) {
I think you need to adjust clampRect by the paint offset. I'd suggest clamping
the size of the dirtyRect with setWidth(min(5000, width())) etc.

[wkorman, 2016-05-06 22:42:56]

Can't find a way to turn off the default layout mock theme. Do you know any of
the tests that have Mac-theme-specific pixel test baselines? I grepped around
but don't see a way to fiddle theme from a test.

https://codereview.chromium.org/1956583002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/mac/LocalCurrentGraphicsContext.mm
(right):

https://codereview.chromium.org/1956583002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mac/LocalCurrentGraphicsContext.mm:47: if
(!clampRect.contains(m_inflatedDirtyRect)) {
On 2016/05/05 23:50:58, chrishtr wrote:
> I think you need to adjust clampRect by the paint offset. I'd suggest clamping
> the size of the dirtyRect with setWidth(min(5000, width())) etc.

Reworked this and added a comment.

[chrishtr, 2016-05-06 23:02:53]

Maybe put a RELEASE_ASSERT(false) in ThemPainterMac and run the layout tests?

[wkorman, 2016-05-06 23:36:27]

Added tests. AFAICT Mac LayoutTests just run with Mac theme, at least locally.
Most of the fast/forms for example. So, I'll try it and see what happens.

[wkorman, 2016-05-06 23:54:41]

On 2016/05/06 23:36:27, wkorman wrote:
> Added tests. AFAICT Mac LayoutTests just run with Mac theme, at least locally.
> Most of the fast/forms for example. So, I'll try it and see what happens.

I think we should increase clamp size to 10K. 5K seems too possible with for
example a high resolution vertical monitor or tablet. I'll do that before
submit.

[wkorman, 2016-05-09 21:11:58]

PTAL, updated to 10K from 5K for max dirty pixel size.

[chrishtr, 2016-05-09 21:42:20]

lgtm

https://codereview.chromium.org/1956583002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/forms/huge-mac-input-clamped-height.html
(right):

https://codereview.chromium.org/1956583002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/huge-mac-input-clamped-height.html:2:
<p>Test passes if the input element is truncated beyond 5000px high on Mac.</p>
10000px

https://codereview.chromium.org/1956583002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/forms/huge-mac-input-clamped-width.html
(right):

https://codereview.chromium.org/1956583002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/huge-mac-input-clamped-width.html:2:
<p>Test passes if the input element is truncated beyond 5000px on Mac.</p>
10000px

https://codereview.chromium.org/1956583002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mac/LocalCurrentGraphicsContext.mm
(right):

https://codereview.chromium.org/1956583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mac/LocalCurrentGraphicsContext.mm:43: static
const int kMaxDirtyPixelSize = 10000;
kMaxDirtyRectPixelSize?

[wkorman, 2016-05-09 22:14:24]

https://codereview.chromium.org/1956583002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/forms/huge-mac-input-clamped-height.html
(right):

https://codereview.chromium.org/1956583002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/huge-mac-input-clamped-height.html:2:
<p>Test passes if the input element is truncated beyond 5000px high on Mac.</p>
On 2016/05/09 21:42:19, chrishtr wrote:
> 10000px

Done.

https://codereview.chromium.org/1956583002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/forms/huge-mac-input-clamped-width.html
(right):

https://codereview.chromium.org/1956583002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/forms/huge-mac-input-clamped-width.html:2:
<p>Test passes if the input element is truncated beyond 5000px on Mac.</p>
On 2016/05/09 21:42:20, chrishtr wrote:
> 10000px

Done.

https://codereview.chromium.org/1956583002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/mac/LocalCurrentGraphicsContext.mm
(right):

https://codereview.chromium.org/1956583002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/mac/LocalCurrentGraphicsContext.mm:43: static
const int kMaxDirtyPixelSize = 10000;
On 2016/05/09 21:42:20, chrishtr wrote:
> kMaxDirtyRectPixelSize?

Done.

[wkorman, 2016-05-09 22:14:48]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2016-05-09 22:14:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/1956583002/#ps80001
(title: "Integrate feedback.")

[commit-bot: I haz the power, 2016-05-09 22:15:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1956583002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1956583002/80001

[commit-bot: I haz the power, 2016-05-09 23:19:31]

Description was changed from

==========
Clamp Mac theme painting to reasonable large bounds.

Previously we aimed to clip to the interest rect of the graphics layer in
LocalCurrentGraphicsContext. However, in some cases the rect we clip to is the
clip inherited from an overflow clip ancestor, which can lead to complexity
w.r.t. caching.

Rather than reworking interest rect and cache skipping logic across the code
base for Mac theme specific painting purposes, we choose to clamp Mac theme
painting to a reasonable bounding size. This means huge elements requiring
native element painting will no longer render correctly, but in such cases we
may already have been performing poorly.

BUG=592915
==========

to

==========
Clamp Mac theme painting to reasonable large bounds.

Previously we aimed to clip to the interest rect of the graphics layer in
LocalCurrentGraphicsContext. However, in some cases the rect we clip to is the
clip inherited from an overflow clip ancestor, which can lead to complexity
w.r.t. caching.

Rather than reworking interest rect and cache skipping logic across the code
base for Mac theme specific painting purposes, we choose to clamp Mac theme
painting to a reasonable bounding size. This means huge elements requiring
native element painting will no longer render correctly, but in such cases we
may already have been performing poorly.

BUG=592915
==========

[commit-bot: I haz the power, 2016-05-09 23:19:33]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-09 23:22:25]

Description was changed from

==========
Clamp Mac theme painting to reasonable large bounds.

Previously we aimed to clip to the interest rect of the graphics layer in
LocalCurrentGraphicsContext. However, in some cases the rect we clip to is the
clip inherited from an overflow clip ancestor, which can lead to complexity
w.r.t. caching.

Rather than reworking interest rect and cache skipping logic across the code
base for Mac theme specific painting purposes, we choose to clamp Mac theme
painting to a reasonable bounding size. This means huge elements requiring
native element painting will no longer render correctly, but in such cases we
may already have been performing poorly.

BUG=592915
==========

to

==========
Clamp Mac theme painting to reasonable large bounds.

Previously we aimed to clip to the interest rect of the graphics layer in
LocalCurrentGraphicsContext. However, in some cases the rect we clip to is the
clip inherited from an overflow clip ancestor, which can lead to complexity
w.r.t. caching.

Rather than reworking interest rect and cache skipping logic across the code
base for Mac theme specific painting purposes, we choose to clamp Mac theme
painting to a reasonable bounding size. This means huge elements requiring
native element painting will no longer render correctly, but in such cases we
may already have been performing poorly.

BUG=592915

Committed: https://crrev.com/ec218fb8a6013ab21b83590acf77530cbbce1097
Cr-Commit-Position: refs/heads/master@{#392468}
==========

[commit-bot: I haz the power, 2016-05-09 23:22:26]

Patchset 5 (id:??) landed as
https://crrev.com/ec218fb8a6013ab21b83590acf77530cbbce1097
Cr-Commit-Position: refs/heads/master@{#392468}