REGRESSION (r42334): Unnecessary scroll bars remain after the document shrinks

• Component: Frames
• Status: RESOLVED FIXED

In the attached test case, the document initially requires both scroll bars, but then it shrinks down to a size that will allow it to fit in a window without scroll bars. The scroll bars should be removed, but they are not, because the document’s size is compared to the viewable size with the scrollbars still present, which is smaller.

This is a regression from <http://trac.webkit.org/changeset/42334>, which removed some of the logic that account for present scroll bars.

<rdar://problem/6916276>

Created attachment 30603 [details]
Test case

[Dave Hyatt]

Created attachment 30670 [details]
Patch

[Darin Adler]

Comment on attachment 30670 [details]
Patch

> +        Added two tests in fast/dynamic.

It's better to give the names of the tests.

> -- (void)_boundsChanged
> +- (void)_boundsChangedToNewSize:(NSSize)size

I think _boundsChangedTo: would be fine here rather than _boundsChangedToNewSize:.

> -    if (!NSEqualSizes(_private->lastLayoutSize, [self bounds].size)) {
> +     if (!NSEqualSizes(_private->lastLayoutSize, size)) {

Indented one extra space here.

> -            frame->view()->resize([self bounds].size.width, [self bounds].size.height);
> +            frame->view()->resize(size.width, size.height);

How about just resize(IntSize(size)) here instead? Has to be an explicit conversion because it's a conversion from floating point to integer, but nice to leave it as an object instead of breaking up into width/height.

> +- (void)setFrameSize:(NSSize)size
> +{
> +    [self _boundsChangedToNewSize:size];
> +    [super setFrameSize:size];
> +}

I'm not sure we need a separate _boundsChangedToNewSize: method. I'd just put that code into the setFrameSize: override and not have a separate method.

r=me

[Dave Hyatt]

Comment on attachment 30670 [details]
Patch

Breaks mail. clearing flag.

[Dave Hyatt]

Comment on attachment 30670 [details]
Patch

Putting back darin's r+, since the bug in Mail is there in TOT and is not caused by this patch.

[Dave Hyatt]

Fixed in r44152.