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).
Created attachment 94493 [details] Patch
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
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
Created attachment 94503 [details] Patch
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
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
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::
(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.
Created attachment 94704 [details] Patch
Addressed feedback. Fixed issue with != vs ==. :(
> > > > 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?
Created attachment 94879 [details] Patch
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)
(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.)
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.
Created attachment 95054 [details] Patch
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.
Committed as http://trac.webkit.org/changeset/87444. (And the cr build breaks were fixed before check-in.)
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?
Seems to be showing up on the webkit waterfall, too. Considering a rollout.
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.
Created attachment 95177 [details] Patch
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.
Created attachment 95188 [details] Patch
Comment on attachment 95188 [details] Patch R=me, thanks!
Committed as http://trac.webkit.org/changeset/87521
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()); ?
(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.
(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 :)
(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.