Bug 84252 – [Chromium] set layout fallback width width when FrameView is resized in AutoSizeMode

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Peter Kotwicz]

[Chromium] set layout fallback width width when FrameView is resized in AutoSizeMode

[Peter Kotwicz]

Created attachment 137721 [details]
Patch

[Fady Samuel]

Comment on attachment 137721 [details]
Patch

Please check the default device scale factor not the device scale factor.

[Peter Kotwicz]

Created attachment 137952 [details]
Patch

[Fady Samuel]

Comment on attachment 137952 [details]
Patch

The if statement is unnecessary. You can set the layoutFallbackWidth independent of fixed layout mode, it just won't have any effect.

[Peter Kotwicz]

Created attachment 137967 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 137967 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137967&action=review

> Source/WebCore/page/FrameView.cpp:2501
> +          page->settings()->setLayoutFallbackWidth(newSize.width() / page->settings()->defaultDeviceScaleFactor());

nit: indent by 4 spaces

[Darin Fisher (:fishd, Google)]

Comment on attachment 137967 [details]
Patch

Actually, R-

I have a few questions:

1-  Why are mutating Settings?  Normally, Settings are set by the embedder.  Perhaps the "effective layout fallback width" should be stored elsewhere?
2-  Wondering about Fady's comments.

[Fady Samuel]

(In reply to comment #7)
> (From update of attachment 137967 [details])
> Actually, R-
> 
> I have a few questions:
> 
> 1-  Why are mutating Settings?  Normally, Settings are set by the embedder.  Perhaps the "effective layout fallback width" should be stored elsewhere?
> 2-  Wondering about Fady's comments.

1. What's the right place to do this? The issue being solved here is we need to detect this resize, and update the layout fallback width so the viewport scaling is computed correctly

2. Peter had an additional if statement:

if (useFixedLayout()) that he later removed.

[David Levin]

Comment on attachment 137967 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137967&action=review

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=84252

What is this change fixing?

> Source/WebCore/ChangeLog:7
> +

Why is there no layout test? (Or any test at all?)

Is one possible? If not, why not?

> Source/WebCore/page/FrameView.cpp:2498
> +#if ENABLE(VIEWPORT)

Why is this behind ENABLE(VIEWPORT)?

> Source/WebCore/page/FrameView.cpp:2499
> +        Page* page = frame()->page();

Why is this done inside of the loop instead of after the loop in this method?

I wonder why it is done here at all as opposed to some place closer to "page->chrome()->client()->layoutUpdated(frame());".

[Fady Samuel]

Comment on attachment 137967 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137967&action=review

>> Source/WebCore/page/FrameView.cpp:2498
>> +#if ENABLE(VIEWPORT)
> 
> Why is this behind ENABLE(VIEWPORT)?

layoutFallbackWidth is logically associated with viewport computations and so it's behind ENABLE(VIEWPORT)

>> Source/WebCore/page/FrameView.cpp:2499
>> +        Page* page = frame()->page();
> 
> Why is this done inside of the loop instead of after the loop in this method?
> 
> I wonder why it is done here at all as opposed to some place closer to "page->chrome()->client()->layoutUpdated(frame());".

Good point. This doesn't need to be in WebCore at all I think. This can probably be moved to ChromeClientImpl.

[Peter Kotwicz]

Created attachment 138190 [details]
Patch

[David Levin]

Comment on attachment 138190 [details]
Patch

Looks so much better here. (Note that this method is called at the end of recalc regardless of autosize mode iirc.)

[Fady Samuel]

(In reply to comment #12)
> (From update of attachment 138190 [details])
> Looks so much better here. (Note that this method is called at the end of recalc regardless of autosize mode iirc.)

Yup this feels like the right place for this code. I think we might be able to remove the layoutFallbackWidth code from:

RenderViewImpl::OnResize in the chromium repo now. It only needs to be here I think. Peter could you please verify this?

[Stephen Chenney]

https://code.google.com/p/chromium/issues/detail?id=231320