Bug 61309 – Need a callback for when the preferred rendered size may have changed.

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[David Levin]

So that the host can adjust its presentation if desired when this happens. (The contents size changed isn't sufficient because it doesn't capture when preferred rendered size becomes smaller than the view size -- i.e. when scrollbars aren't needed).

[David Levin]

Created attachment 94493 [details]
Patch

[WebKit Review Bot]

Comment on attachment 94493 [details]
Patch

Attachment 94493 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8723916

New failing tests:
fast/forms/004.html
fast/forms/HTMLOptionElement_label01.html
fast/forms/HTMLOptionElement_label04.html
fast/forms/HTMLOptionElement_label03.html
fast/forms/HTMLOptionElement_label07.html
fast/forms/HTMLOptionElement_label05.html
fast/forms/005.html
fast/forms/007.html
fast/forms/002.html
fast/forms/box-shadow-override.html
fast/forms/HTMLOptionElement_label02.html
fast/forms/HTMLOptionElement_label06.html
fast/forms/basic-inputs.html
fast/forms/autofocus-opera-003.html
fast/forms/006.html
fast/forms/basic-buttons.html
fast/forms/basic-selects.html
fast/forms/button-align.html
fast/forms/003.html
fast/forms/blankbuttons.html

[WebKit Review Bot]

Created attachment 94496 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[David Levin]

Created attachment 94503 [details]
Patch

[WebKit Review Bot]

Comment on attachment 94503 [details]
Patch

Attachment 94503 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8732196

New failing tests:
fast/forms/004.html
fast/forms/HTMLOptionElement_label01.html
fast/forms/HTMLOptionElement_label04.html
fast/forms/HTMLOptionElement_label03.html
fast/forms/HTMLOptionElement_label07.html
fast/forms/HTMLOptionElement_label05.html
fast/forms/005.html
fast/forms/007.html
fast/forms/002.html
fast/forms/box-shadow-override.html
fast/forms/HTMLOptionElement_label02.html
fast/forms/HTMLOptionElement_label06.html
fast/forms/basic-inputs.html
fast/forms/autofocus-opera-003.html
fast/forms/006.html
fast/forms/basic-buttons.html
fast/forms/basic-selects.html
fast/forms/button-align.html
fast/forms/003.html
fast/forms/blankbuttons.html

[WebKit Review Bot]

Created attachment 94515 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[James Robinson]

Comment on attachment 94503 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:490
> +    if (page)
> +        page->chrome()->renderedSizeMayHaveChanged(frame());

It looks like this will end up being called on nearly every layout, so potentially 100hz+.  Are you sure you want this many calls?

Additionally there's a check in FrameView::layout() that avoids calling this on subtree layouts (http://trac.webkit.org/browser/trunk/Source/WebCore/page/FrameView.cpp#L1006).  Are you sure it's OK to not call the notification in this case?

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:584
> +void ChromeClientImpl::contentsSizeChanged(WebCore::Frame* frame, const WebCore::IntSize& size) const

we have a 'using namespace WebCore;' declaration in this file so you don't need the WebCore::

[David Levin]

(In reply to comment #7)
> (From update of attachment 94503 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94503&action=review
> 
> > Source/WebCore/page/FrameView.cpp:490
> > +    if (page)
> > +        page->chrome()->renderedSizeMayHaveChanged(frame());
> 
> It looks like this will end up being called on nearly every layout, so potentially 100hz+.  Are you sure you want this many calls?

Well the only way that I see to avoid doing the call is to do the calculation to determine if it should be called but that calculation would involve a bit of work that would be nice to avoid.

Instead the called method can decided what to do. In my case, I plan to set a short timer (10ms) when this callback happens. Then when the timer fires, the size may be measured. (Of course, if the callback is happening at 100Hz+ then that callback wouldn't happen so maybe I should just make the timer 0ms. I mostly just care about doing the size measurements and potential adjustments after the layout is completely done instead of in the middle of it.)


> 
> Additionally there's a check in FrameView::layout() that avoids calling this on subtree layouts (http://trac.webkit.org/browser/trunk/Source/WebCore/page/FrameView.cpp#L1006).  Are you sure it's OK to not call the notification in this case?

I only care about the top level view. (If rendering a subtree doesn't affect the layout of the root, then I don't care in that way I think it is similar to setContentsSize.)


> 
> > Source/WebKit/chromium/src/ChromeClientImpl.cpp:584
> > +void ChromeClientImpl::contentsSizeChanged(WebCore::Frame* frame, const WebCore::IntSize& size) const
> 
> we have a 'using namespace WebCore;' declaration in this file so you don't need the WebCore::
 Will fix. I need to get rid of changes in that file.

I need to fix the declaration in Source/WebKit/win/WebCoreSupport/WebChromeClient.h to have the WebCore prefix as well.

[David Levin]

Created attachment 94704 [details]
Patch

[David Levin]

Addressed feedback. Fixed issue with != vs ==. :(

[James Robinson]

> > 
> > Additionally there's a check in FrameView::layout() that avoids calling this on subtree layouts (http://trac.webkit.org/browser/trunk/Source/WebCore/page/FrameView.cpp#L1006).  Are you sure it's OK to not call the notification in this case?
> 
> I only care about the top level view. (If rendering a subtree doesn't affect the layout of the root, then I don't care in that way I think it is similar to setContentsSize.)
> 
> 

I'm not completely convinced about this since this code cares about the preferred size.  For example, imagine a <div> at some point in the dom that is absolutely positioned to extend below all of the other visible content in the document.  Even if you only did a subtree layout within this div that might change the preferred height of the document.  Could you try some tests to see if this happens?

[David Levin]

Created attachment 94879 [details]
Patch

[James Robinson]

Comment on attachment 94879 [details]
Patch

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

I think the implementation is fine, but the interface could use a bit of polish.  Can you get Darin F's input before committing as well?  He likes to look at public WebKit API changes.

> Source/WebCore/page/ChromeClient.h:164
> +        // Indicates that the preferred dimensions for the rendered HTML contents may have changed.

this comment+name aren't super helpful, in particular it's really unclear how this is supposed to related to contentsSizeChanged().  maybe this should be contentsPreferredSizeMayHaveChanged()?

why not pass the size out to the caller now instead of making them call back in to ask for the new preferred contents size?

> Source/WebCore/page/ChromeClient.h:165
> +        virtual void renderedSizeMayHaveChanged(Frame*) const = 0;

did you consider just giving this an empty impl here instead of declaring it pure virtual?

> Source/WebKit/efl/ChangeLog:9
> +        * WebCoreSupport/ChromeClientEfl.h:
> +        (WebCore::ChromeClientEfl::renderedSizeMayHaveChanged): Stubbed out renderedSizeMayHaveChanged.

if the call wasn't declared pure virtual then you wouldn't need to stub it out here (and everywhere else that implements ChromeClient)

[David Levin]

(In reply to comment #13)
> (From update of attachment 94879 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94879&action=review
> 
> I think the implementation is fine, but the interface could use a bit of polish.  Can you get Darin F's input before committing as well?  He likes to look at public WebKit API changes.


Will do.

> > Source/WebCore/page/ChromeClient.h:164
> > +        // Indicates that the preferred dimensions for the rendered HTML contents may have changed.
> 
> this comment+name aren't super helpful, in particular it's really unclear how this is supposed to related to contentsSizeChanged().  maybe this should be contentsPreferredSizeMayHaveChanged()?

Good point. It is a bit long... but I can do that name. (I'll change it.)

> 
> why not pass the size out to the caller now instead of making them call back in to ask for the new preferred contents size?

The width calculation (to my memory) takes some effort. (It isn't just a look-up) so it isn't worth doing this cost for clients who don't want this info (and even chromium won't use the size info immediately -- it will only do it after a short timeout).


> > Source/WebCore/page/ChromeClient.h:165
> > +        virtual void renderedSizeMayHaveChanged(Frame*) const = 0;
> 
> did you consider just giving this an empty impl here instead of declaring it pure virtual?
Will do. (Was trying to follow http://trac.webkit.org/changeset/43918/trunk/WebCore/page/ChromeClient.h but it doesn't matter for this and it appears to no longer be the goal.)

[Darin Fisher (:fishd, Google)]

Comment on attachment 94879 [details]
Patch

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

> Source/WebKit/chromium/public/WebFrameClient.h:311
> +    virtual void renderedSizeMayHaveChanged(WebFrame*) { }

Hmm... this is adding the term "renderedSize", but the comment seems
to indicate that that is a synonym for "preferred size".

We already have WebFrame::contentsPreferredWidth(), which appears to
be what you should query when you receive this notification.  I think
it would therefore help if these two functions had related names.

{didChange,mayHaveChanged}ContentsPreferred{Width,Size}?

I wonder why we have a notification here that is not certain of change?
Is it because we need to run layout() again in order for us to know if
the size did actually change?  Is the embedder required to call layout()
before querying contentsPreferredWidth?  If so, then I understand why
the notification is a "mayHaveChanged" and not a "didChange", but if not,
then we could test the contentsPreferredWidth before generating the
notification and only generate the notification if something changed.

As you can see, the API as present leaves a lot of questions unanswered.

[David Levin]

Created attachment 95054 [details]
Patch

[David Levin]

I was doing the "mayHaveChanged" thing because I was sure that getting the preferred size took a fair bit of computation.  However, I debugged it further and it appears that the heavy lifting is done already by the layout code.

[David Levin]

Committed as http://trac.webkit.org/changeset/87444.

(And the cr build breaks were fixed before check-in.)

[Adam Klein]

I'm seeing some very odd test failures on the Chromium bots that point at this change:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&group=%40ToT%20-%20chromium.org&tests=fast%2Fcss%2Ffirst-letter-text-fragment-crash.html%2Cfast%2Fcss%2Ffirst-letter-visibility.html

I'm able to reproduce locally on Linux: at r87443, the tests pass.  At r87444, one fails and the other times out.  This change sounded like a refactor, but can you take a look and see if there might be something related?

[Adam Klein]

Seems to be showing up on the webkit waterfall, too. Considering a rollout.

[David Levin]

I should have done a layout test run (or cq run) after doing that final change.

It turns out that computing the preferred size during layout is problematic for two reasons:

1. It may be expensive. Specifically RenderBlock::computePreferredLogicalWidths() is a rather involved function that does a lot of work. It shouldn't be called when this information isn't needed.

2. It may trigger another recalc midway through layout. Here's a sample stack:
#0	0x0247f19a in WebCore::FrameView::scheduleRelayout at FrameView.cpp:1759
#1	0x026d4b31 in WebCore::RenderObject::scheduleRelayout at RenderObject.cpp:2182
#2	0x026f0f7d in WebCore::RenderObject::markContainingBlocksForLayout at RenderObject.h:1050
#3	0x0264caf0 in WebCore::RenderObject::setNeedsLayout at RenderObject.h:939
#4	0x026decb7 in WebCore::RenderObject::setNeedsLayoutAndPrefWidthsRecalc at RenderObject.h:508
#5	0x026e0c9d in WebCore::RenderObjectChildList::removeChildNode at RenderObjectChildList.cpp:72
#6	0x026d5aea in WebCore::RenderObject::removeChild at RenderObject.cpp:333
#7	0x02621586 in WebCore::RenderBlock::updateFirstLetter at RenderBlock.cpp:5358
#8	0x02610803 in WebCore::RenderBlock::computePreferredLogicalWidths at RenderBlock.cpp:4561
#9	0x026507a5 in WebCore::RenderBox::minPreferredLogicalWidth at RenderBox.cpp:667
#10	0x0260f6d9 in WebCore::RenderBlock::computeBlockPreferredLogicalWidths at RenderBlock.cpp:5027
#11	0x02610916 in WebCore::RenderBlock::computePreferredLogicalWidths at RenderBlock.cpp:4572
#12	0x026507a5 in WebCore::RenderBox::minPreferredLogicalWidth at RenderBox.cpp:667
#13	0x0260f6d9 in WebCore::RenderBlock::computeBlockPreferredLogicalWidths at RenderBlock.cpp:5027
#14	0x02610916 in WebCore::RenderBlock::computePreferredLogicalWidths at RenderBlock.cpp:4572
#15	0x026507a5 in WebCore::RenderBox::minPreferredLogicalWidth at RenderBox.cpp:667
#16	0x0260f6d9 in WebCore::RenderBlock::computeBlockPreferredLogicalWidths at RenderBlock.cpp:5027
#17	0x02610916 in WebCore::RenderBlock::computePreferredLogicalWidths at RenderBlock.cpp:4572
#18	0x027281f3 in WebCore::RenderView::computePreferredLogicalWidths at RenderView.cpp:92
#19	0x026507a5 in WebCore::RenderBox::minPreferredLogicalWidth at RenderBox.cpp:667

Given this I'll do a different approach with a clear method name which doesn't require doing this computation.

[David Levin]

Created attachment 95177 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 95177 [details]
Patch

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

> Source/WebKit/chromium/public/WebFrameClient.h:108
> +    virtual void didFinishLayout(WebFrame*) { }

Layout is something that runs to completion, right?  I assume this callback
tells the embedder two things:  1) we may have new layout now, and 2) if you
call layout(), it'll be a no-op.

A slight change to the wording might be didUpdateLayout to convey the fact
that page layout may have been updated.  didFinishLayout suggests that layout
is some asynchronous process, but that's not really accurate, right?

I also assume that if the embedder calls WebWidget::layout() that they should
expect to receive a didUpdateLayout() callback for each frame, unless the frame
didn't need layout, before the call to layout() returns.  Is that correct?

^^^ It would help to document some of these things.

[David Levin]

Created attachment 95188 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 95188 [details]
Patch

R=me, thanks!

[David Levin]

Committed as http://trac.webkit.org/changeset/87521

[Antonio Gomes]

Comment on attachment 95188 [details]
Patch

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

Sorry about the delayed questions :)

> Source/WebCore/page/ChromeClient.h:164
> +        virtual void layoutUpdated(Frame*) const { }

Any reason for it not being a pure virtual like the others?

> Source/WebCore/page/FrameView.cpp:1070
> +            return page->chrome()->client()->layoutUpdated(frame());

should not this be:

return page->chrome()->layoutUpdated(frame());

?

[David Levin]

(In reply to comment #27)
> (From update of attachment 95188 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95188&action=review
> 
> Sorry about the delayed questions :)
> 
> > Source/WebCore/page/ChromeClient.h:164
> > +        virtual void layoutUpdated(Frame*) const { }
> 
> Any reason for it not being a pure virtual like the others?

That was my intent but I changed it due to reviewer feedback (See comment #14.)

I couldn't defend it when it simply isn't true that all other methods in this class are pure virtual (and there is no comment indicating that it should be).

> 
> > Source/WebCore/page/FrameView.cpp:1070
> > +            return page->chrome()->client()->layoutUpdated(frame());
> 
> should not this be:
> 
> return page->chrome()->layoutUpdated(frame());
> 
> ?

Maybe :)

Perhaps you could why you think this should be different (so I may agree). It looks like most other methods in here do something similar to what the patch does.

[Antonio Gomes]

(In reply to comment #28)
> (In reply to comment #27)
> > (From update of attachment 95188 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=95188&action=review
> > 
> > Sorry about the delayed questions :)
> > 
> > > Source/WebCore/page/ChromeClient.h:164
> > > +        virtual void layoutUpdated(Frame*) const { }
> > 
> > Any reason for it not being a pure virtual like the others?
> 
> That was my intent but I changed it due to reviewer feedback (See comment #14.)
> 
> I couldn't defend it when it simply isn't true that all other methods in this class are pure virtual (and there is no comment indicating that it should be).


Are you talking about this:

"
> > Source/WebCore/page/ChromeClient.h:165
> > +        virtual void renderedSizeMayHaveChanged(Frame*) const = 0;
> 
> did you consider just giving this an empty impl here instead of declaring it pure virtual?
> 
> > Source/WebKit/efl/ChangeLog:9
> > +        * WebCoreSupport/ChromeClientEfl.h:
> > +        (WebCore::ChromeClientEfl::renderedSizeMayHaveChanged): Stubbed out renderedSizeMayHaveChanged.
> 
> if the call wasn't declared pure virtual then you wouldn't need to stub it out here (and everywhere else that implements ChromeClient)
"

I thought it was a general practice to stub out methods like these for all ChromeClientXXX files, but I am not sure ...

It was just harder for me it find out the existence of this usefull method in the the Chrome interface since it had no a Client correspondent :)

[David Levin]

(In reply to comment #29)
> Are you talking about this:

Yes.

> I thought it was a general practice to stub out methods like these for all ChromeClientXXX files, but I am not sure ...

Ditto for me.