Bug 102764 – Use m_webView->size() for viewport update

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tien-Ren Chen]

Use m_webView->size() for viewport update

[Tien-Ren Chen]

Created attachment 175128 [details]
Patch

[Tien-Ren Chen]

Created attachment 175130 [details]
Patch

[Brent Fulgham]

Comment on attachment 175130 [details]
Patch

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

I think the general concept here is good, but I think there is relatively weak evidence that this won't have unexpected changes in behavior.

> Source/WebKit/chromium/ChangeLog:12
> +        rendering purpose.

While the intent of this change seems good, it seems scary to apply such a modification without at least identifying some test cases that cover this behavior. From the sound of the bug, it should be possible to construct a test case illustrating the case of a pending resize that should not be used for the current rendering.

[Tien-Ren Chen]

(In reply to comment #3)
> (From update of attachment 175130 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175130&action=review
> 
> I think the general concept here is good, but I think there is relatively weak evidence that this won't have unexpected changes in behavior.
> 
> > Source/WebKit/chromium/ChangeLog:12
> > +        rendering purpose.
> 
> While the intent of this change seems good, it seems scary to apply such a modification without at least identifying some test cases that cover this behavior. From the sound of the bug, it should be possible to construct a test case illustrating the case of a pending resize that should not be used for the current rendering.

Makes sense. The real life bug I have is this one: https://code.google.com/p/chromium/issues/detail?id=160619
That windowRect will be one frame behind when rotating an Android device.

I think I can probably do a WebFrameTest for that. I'll try tomorrow.

[Adam Barth]

"Your client does not have permission to get URL /p/chromium/issues/detail?id=160619 from this server."

[Adam Barth]

Comment on attachment 175130 [details]
Patch

Can you add a test?

[Tien-Ren Chen]

(In reply to comment #3)
> (From update of attachment 175130 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175130&action=review
> 
> I think the general concept here is good, but I think there is relatively weak evidence that this won't have unexpected changes in behavior.
> 
> > Source/WebKit/chromium/ChangeLog:12
> > +        rendering purpose.
> 
> While the intent of this change seems good, it seems scary to apply such a modification without at least identifying some test cases that cover this behavior. From the sound of the bug, it should be possible to construct a test case illustrating the case of a pending resize that should not be used for the current rendering.

I just added some debug output to RenderWidget::setWindowRect and RenderWidget::OnResize. I think it is very wrong to use windowRect in the first place.

The semantics of windowRect is the outer rect of the window, that is, including all decorations like the title bar, status bar, navigation buttons and such. I don't think it makes any sense to use it for layout purpose.

(In reply to comment #6)
> (From update of attachment 175130 [details])
> Can you add a test?

Instead of adding tests, I think we should modify all tests in WebFrameTest to remove the windowRect() override in WebViewClient implementations. The tests should still work without windowRect().

[Adam Barth]

> Instead of adding tests, I think we should modify all tests in WebFrameTest to remove the windowRect() override in WebViewClient implementations. The tests should still work without windowRect().

That sounds worth trying.

[Tien-Ren Chen]

Created attachment 175313 [details]
Patch

[Adam Barth]

Comment on attachment 175313 [details]
Patch

Thanks for updating the tests.

[WebKit Review Bot]

Comment on attachment 175313 [details]
Patch

Clearing flags on attachment: 175313

Committed r135442: <http://trac.webkit.org/changeset/135442>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.