Remove RenderIFrame::updateLogicalHeight and RenderIFrame::updateLogicalWidth

[Tony Chang]

Replace RenderIFrame::updateLogicalHeight with RenderIFrame::computeLogicalHeight

[Tony Chang]

Created attachment 164629 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 164629 [details]
Patch

LGTM.  Do we (I?) need to add a test for changed behavior here?  I'm sure I may have gotten this function wrong in my initial implementation.

[Tony Chang]

Created attachment 164632 [details]
Patch

[Tony Chang]

(In reply to comment #2)
> (From update of attachment 164629 [details])
> LGTM.  Do we (I?) need to add a test for changed behavior here?  I'm sure I may have gotten this function wrong in my initial implementation.

I tried, but it involves enabling frame flattening in Chromium DRT.  That's easy enough through internal.settings, but it looks like someone already tried this https://bugs.webkit.org/show_bug.cgi?id=87149, which lead me to https://bugs.webkit.org/show_bug.cgi?id=87993 at which point I cried and gave up.

[Eric Seidel (no email)]

I see, these are frame-flattening only paths?  Should we add a mac-only test?

[Tony Chang]

Comment on attachment 164632 [details]
Patch

Oh yeah, I could write a test and verify it using Qt or GTK+.  I'll add a test and re-upload.

[Eric Seidel (no email)]

I really appreciate your thoroughness.  Thanks.

[Tony Chang]

Created attachment 164773 [details]
Patch

[Eric Seidel (no email)]

We should CC whoever wrote frame flattening.  I don't have enough knowledge to know if this is correct or not.

[Eric Seidel (no email)]

I think Kenneth did.

[Eric Seidel (no email)]

Comment on attachment 164773 [details]
Patch

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

> Source/WebCore/rendering/RenderIFrame.cpp:-78
> -    if (isScrollable || !style()->width().isFixed()) {
> -        FrameView* view = static_cast<FrameView*>(widget());

I take it we don't need this scrollable-case code?  I would not trust the flattening tests to be comprehensive (that isn't to say we regard this code as sacred or shy away from changing it).

[Tony Chang]

Comment on attachment 164773 [details]
Patch

Sorry, didn't see Eric's comments.

[Eric Seidel (no email)]

Don't let me stand in the way of your commit. :)  Just trying to do my reviewing-due-diligence. :)  (Also since I was the last to touch this code, I guess I'm supposed to make sure it stays working...)

[Tony Chang]

(In reply to comment #11)
> (From update of attachment 164773 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164773&action=review
> 
> > Source/WebCore/rendering/RenderIFrame.cpp:-78
> > -    if (isScrollable || !style()->width().isFixed()) {
> > -        FrameView* view = static_cast<FrameView*>(widget());
> 
> I take it we don't need this scrollable-case code?  I would not trust the flattening tests to be comprehensive (that isn't to say we regard this code as sacred or shy away from changing it).

I believe that is already covered by RenderIFrame::flattenFrame(), which also looks at scrollingMode() and does the isFixed() checks.

[Eric Seidel (no email)]

OK.

[WebKit Review Bot]

Comment on attachment 164773 [details]
Patch

Clearing flags on attachment: 164773

Committed r129046: <http://trac.webkit.org/changeset/129046>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.