Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
209353003:
Clean up handling of autosizing state changes. (Closed)
|
Created:
6 years, 9 months ago by skobes Modified:
6 years, 9 months ago CC:
blink-reviews, zoltan1, dsinclair, sof, eae+blinkwatch, leviw+renderwatch, dglazkov+blink, adamk+blink_chromium.org, jchaffraix+rendering, Inactive, bemjb+rendering_chromium.org, pdr., rune+blink, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/blink.git@crap_base Visibility:
Public. |
DescriptionClean up handling of autosizing state changes.
All page-level state changes now go through updatePageInfo, which marks blocks
as needing layout if their multipliers might need updating.
This simplifies the flow and ensures the page updates correctly on orientation
changes and movements of the text scaling slider in accessibility settings.
BUG=353309, 347384
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169997
Patch Set 1 #
Total comments: 13
Patch Set 2 : Address review comments. #
Total comments: 13
Patch Set 3 : Address review comments. #Patch Set 4 : Update TestExpectations. #Patch Set 5 : Fix segfault in unit test. #
Messages
Total messages: 45 (0 generated)
Based on http://crrev.com/208693006.
https://codereview.chromium.org/209353003/diff/1/LayoutTests/fast/text-autosi... File LayoutTests/fast/text-autosizing/font-scale-factor-change.html (right): https://codereview.chromium.org/209353003/diff/1/LayoutTests/fast/text-autosi... LayoutTests/fast/text-autosizing/font-scale-factor-change.html:19: setTimeout(function() { This test is failing for me locally so I can't confirm this, but I suspect this setTimeout(..., 0) can be replaced with forcing a layout using: var forceLayout = document.body.offsetTop https://codereview.chromium.org/209353003/diff/1/Source/core/frame/FrameView.cpp File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/209353003/diff/1/Source/core/frame/FrameView.... Source/core/frame/FrameView.cpp:1757: if (isMainFrame()) { Is this testable? https://codereview.chromium.org/209353003/diff/1/Source/core/rendering/FastTe... File Source/core/rendering/FastTextAutosizer.cpp (right): https://codereview.chromium.org/209353003/diff/1/Source/core/rendering/FastTe... Source/core/rendering/FastTextAutosizer.cpp:490: applyMultiplier(renderer, 1); Do we need to setNeedsLayout here? Normally, applyMultiplier is called while in layout where the height changing doesn't matter. When it's called outside of layout, it can cause the renderobject to need layout because the height can change. I suspect applyMultiplier should assert that the renderobject needs layout. https://codereview.chromium.org/209353003/diff/1/Source/core/rendering/FastTe... Source/core/rendering/FastTextAutosizer.cpp:503: renderer->parent()->setNeedsLayout(); setNeedsLayout already walks up the parent chain so I don't think this parent one is needed.
https://codereview.chromium.org/209353003/diff/1/LayoutTests/fast/text-autosi... File LayoutTests/fast/text-autosizing/font-scale-factor-change-expected.html (right): https://codereview.chromium.org/209353003/diff/1/LayoutTests/fast/text-autosi... LayoutTests/fast/text-autosizing/font-scale-factor-change-expected.html:12: <div style="font-size: 32px"> Usually we use relative style instead of fixed font-size, e.g. style="font-size: 2.5rem" also explaining in text why this is the correct value for multiplier. Also it is not clear from the name for the test "font-scale-factor-change" what it is actually testing. In a sense all text autosizing tests test for font-scale-factor changes, so I think it would be good to be more specific here. https://codereview.chromium.org/209353003/diff/1/LayoutTests/fast/text-autosi... File LayoutTests/fast/text-autosizing/font-scale-factor-change.html (right): https://codereview.chromium.org/209353003/diff/1/LayoutTests/fast/text-autosi... LayoutTests/fast/text-autosizing/font-scale-factor-change.html:19: setTimeout(function() { On 2014/03/24 01:03:18, pdr wrote: > This test is failing for me locally so I can't confirm this, but I suspect this > setTimeout(..., 0) can be replaced with forcing a layout using: > var forceLayout = document.body.offsetTop Alternatively, in some tests we use: element = document.getElementById("divId"); forceLayout = element.offsetHeight; https://codereview.chromium.org/209353003/diff/1/Source/core/rendering/FastTe... File Source/core/rendering/FastTextAutosizer.cpp (right): https://codereview.chromium.org/209353003/diff/1/Source/core/rendering/FastTe... Source/core/rendering/FastTextAutosizer.cpp:497: void FastTextAutosizer::invalidateMultipliers() nit: the name seems a bit confusing, it is not directly clear what the difference is between invalidateMultipliers and resetMultipliers. maybe setAllTextNeedsLayout() is a better name?
Lgtm, but no tests? We really need tests otherwise someone will just delete it.
On 2014/03/25 01:18:35, esprehn wrote: > Lgtm, but no tests? We really need tests otherwise someone will just delete it. Err ignore me, fail from my phone.
https://codereview.chromium.org/209353003/diff/1/LayoutTests/fast/text-autosi... File LayoutTests/fast/text-autosizing/font-scale-factor-change-expected.html (right): https://codereview.chromium.org/209353003/diff/1/LayoutTests/fast/text-autosi... LayoutTests/fast/text-autosizing/font-scale-factor-change-expected.html:12: <div style="font-size: 32px"> On 2014/03/24 15:24:27, timvolodine wrote: > Usually we use relative style instead of fixed font-size, e.g. style="font-size: > 2.5rem" also explaining in text why this is the correct value for multiplier. Done. > Also it is not clear from the name for the test "font-scale-factor-change" what > it is actually testing. In a sense all text autosizing tests test for > font-scale-factor changes, so I think it would be good to be more specific here. This is the only test that changes the font scale factor dynamically, as far as I know. I added some explanatory text. https://codereview.chromium.org/209353003/diff/1/LayoutTests/fast/text-autosi... File LayoutTests/fast/text-autosizing/font-scale-factor-change.html (right): https://codereview.chromium.org/209353003/diff/1/LayoutTests/fast/text-autosi... LayoutTests/fast/text-autosizing/font-scale-factor-change.html:19: setTimeout(function() { On 2014/03/24 01:03:18, pdr wrote: > This test is failing for me locally so I can't confirm this, but I suspect this > setTimeout(..., 0) can be replaced with forcing a layout using: > var forceLayout = document.body.offsetTop I'm testing that dynamically changing the font scale factor causes everything to be laid out correctly. If I force a layout it kind of defeats the point of the test. https://codereview.chromium.org/209353003/diff/1/Source/core/frame/FrameView.cpp File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/209353003/diff/1/Source/core/frame/FrameView.... Source/core/frame/FrameView.cpp:1757: if (isMainFrame()) { On 2014/03/24 01:03:18, pdr wrote: > Is this testable? This notifies the FTA when the layout size changes, so I think a lot of tests would fail if it wasn't there. Did you have a specific test in mind? https://codereview.chromium.org/209353003/diff/1/Source/core/rendering/FastTe... File Source/core/rendering/FastTextAutosizer.cpp (right): https://codereview.chromium.org/209353003/diff/1/Source/core/rendering/FastTe... Source/core/rendering/FastTextAutosizer.cpp:490: applyMultiplier(renderer, 1); On 2014/03/24 01:03:18, pdr wrote: > Do we need to setNeedsLayout here? Normally, applyMultiplier is called while in > layout where the height changing doesn't matter. When it's called outside of > layout, it can cause the renderobject to need layout because the height can > change. > > I suspect applyMultiplier should assert that the renderobject needs layout. You're right, applyMultiplier assumes it is inside layout by using setStyleInternal instead of setStyle. I've updated it to take a param to distinguish this case. https://codereview.chromium.org/209353003/diff/1/Source/core/rendering/FastTe... Source/core/rendering/FastTextAutosizer.cpp:497: void FastTextAutosizer::invalidateMultipliers() On 2014/03/24 15:24:27, timvolodine wrote: > nit: the name seems a bit confusing, it is not directly clear what the > difference is between invalidateMultipliers and resetMultipliers. maybe > setAllTextNeedsLayout() is a better name? Done. https://codereview.chromium.org/209353003/diff/1/Source/core/rendering/FastTe... Source/core/rendering/FastTextAutosizer.cpp:503: renderer->parent()->setNeedsLayout(); On 2014/03/24 01:03:18, pdr wrote: > setNeedsLayout already walks up the parent chain so I don't think this parent > one is needed. Done. (This was an unsuccessful initial attempt to fix the line height bug.)
https://codereview.chromium.org/209353003/diff/20001/Source/core/rendering/Fa... File Source/core/rendering/FastTextAutosizer.cpp (right): https://codereview.chromium.org/209353003/diff/20001/Source/core/rendering/Fa... Source/core/rendering/FastTextAutosizer.cpp:856: renderer->setStyle(style.release()); This can cause us to go back through quite a bit of extra code, including styleDidChange and FTA::record. Is that intended? I expected this to just setNeedsLayout or adjust the layout triggers in RenderStyle. https://codereview.chromium.org/209353003/diff/20001/Source/core/rendering/Fa... Source/core/rendering/FastTextAutosizer.cpp:860: toRenderBlock(renderer)->invalidateLineHeight(); Is this lineheight change supposed to be part of this patch? https://codereview.chromium.org/209353003/diff/20001/Source/core/rendering/Fa... File Source/core/rendering/FastTextAutosizer.h (right): https://codereview.chromium.org/209353003/diff/20001/Source/core/rendering/Fa... Source/core/rendering/FastTextAutosizer.h:211: void applyMultiplier(RenderObject*, float, bool relayout = false); Can you use an enum here instead of a bool? https://codereview.chromium.org/209353003/diff/20001/Source/core/rendering/Fa... Source/core/rendering/FastTextAutosizer.h:232: bool m_updatePageInfoDeferred; m_updatePageInfoDeferred isn't used in this patch.
https://codereview.chromium.org/209353003/diff/20001/LayoutTests/fast/text-au... File LayoutTests/fast/text-autosizing/font-scale-factor-change-expected.html (right): https://codereview.chromium.org/209353003/diff/20001/LayoutTests/fast/text-au... LayoutTests/fast/text-autosizing/font-scale-factor-change-expected.html:12: <div style="font-size: 2rem"> so the 2 is due to setFontScaleFactor(2)? I understand there is no actual autosizing (i.e. multiplier 320/320=1) involved here? if that's the case maybe add something to the explaining text. https://codereview.chromium.org/209353003/diff/20001/Source/core/frame/FrameV... File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/209353003/diff/20001/Source/core/frame/FrameV... Source/core/frame/FrameView.cpp:377: if (m_frame->settings()->textAutosizingEnabled()) { should this be higher up, i.e. if (isMainFrame() && m_frame->settings()->textAuto...). Otherwise the code behind mainFrameWidthChanded later in the function can be called even if textAutosizingEnabled() is false?
https://codereview.chromium.org/209353003/diff/20001/LayoutTests/fast/text-au... File LayoutTests/fast/text-autosizing/font-scale-factor-change-expected.html (right): https://codereview.chromium.org/209353003/diff/20001/LayoutTests/fast/text-au... LayoutTests/fast/text-autosizing/font-scale-factor-change-expected.html:12: <div style="font-size: 2rem"> On 2014/03/25 13:08:14, timvolodine wrote: > so the 2 is due to setFontScaleFactor(2)? I understand there is no actual > autosizing (i.e. multiplier 320/320=1) involved here? if that's the case maybe > add something to the explaining text. I'm not following what you mean here. Autosizing is how the font scale factor is implemented. https://codereview.chromium.org/209353003/diff/20001/Source/core/frame/FrameV... File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/209353003/diff/20001/Source/core/frame/FrameV... Source/core/frame/FrameView.cpp:377: if (m_frame->settings()->textAutosizingEnabled()) { On 2014/03/25 13:08:14, timvolodine wrote: > should this be higher up, i.e. if (isMainFrame() && > m_frame->settings()->textAuto...). Otherwise the code behind > mainFrameWidthChanded later in the function can be called even if > textAutosizingEnabled() is false? Done. It doesn't really matter since FTA does nothing when it is disabled, but I guess it could save a couple of cycles. Also renamed the variable to reflect the new semantics. https://codereview.chromium.org/209353003/diff/20001/Source/core/rendering/Fa... File Source/core/rendering/FastTextAutosizer.cpp (right): https://codereview.chromium.org/209353003/diff/20001/Source/core/rendering/Fa... Source/core/rendering/FastTextAutosizer.cpp:856: renderer->setStyle(style.release()); On 2014/03/25 04:23:41, pdr wrote: > This can cause us to go back through quite a bit of extra code, including > styleDidChange and FTA::record. Is that intended? > > I expected this to just setNeedsLayout or adjust the layout triggers in > RenderStyle. There's no way for setStyleInternal to trigger a layout, it's literally just setting a RefPtr. We could setNeedsLayout manually but it seems safer to use setStyle when we are touching style outside of layout. That way RenderObject is responsible for the logic to mark the block as needing layout. For example, setStyle will mark the pref widths dirty. FTA has some issues with pref widths, but at least we can make sure they are correct when we resetMultipliers. :) Also note that the old autosizer does all multiplier changes with setStyle. https://codereview.chromium.org/209353003/diff/20001/Source/core/rendering/Fa... Source/core/rendering/FastTextAutosizer.cpp:860: toRenderBlock(renderer)->invalidateLineHeight(); On 2014/03/25 04:23:41, pdr wrote: > Is this lineheight change supposed to be part of this patch? This patch is branched from the lineheight change. https://codereview.chromium.org/209353003/diff/20001/Source/core/rendering/Fa... File Source/core/rendering/FastTextAutosizer.h (right): https://codereview.chromium.org/209353003/diff/20001/Source/core/rendering/Fa... Source/core/rendering/FastTextAutosizer.h:211: void applyMultiplier(RenderObject*, float, bool relayout = false); On 2014/03/25 04:23:41, pdr wrote: > Can you use an enum here instead of a bool? Done. https://codereview.chromium.org/209353003/diff/20001/Source/core/rendering/Fa... Source/core/rendering/FastTextAutosizer.h:232: bool m_updatePageInfoDeferred; On 2014/03/25 04:23:41, pdr wrote: > m_updatePageInfoDeferred isn't used in this patch. Done.
+ojan for Source/web OWNERS.
Source/web LGTM
The CQ bit was checked by pdr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skobes@chromium.org/209353003/50001
The CQ bit was unchecked by skobes@chromium.org
Got an LG from Philip via chat, so I'll go ahead and submit this. Tim, if you have more comments I'll be happy to address them in a follow up patch.
The CQ bit was checked by skobes@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skobes@chromium.org/209353003/50001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on mac_blink_rel
The CQ bit was checked by skobes@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skobes@chromium.org/209353003/170001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_rel
On 2014/03/25 22:28:20, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > tryserver.blink on linux_blink_rel The property-access-on-cached-* tests are broken on trunk, being fixed in http://crrev.com/211863002. Will retry once that lands.
The CQ bit was checked by skobes@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skobes@chromium.org/209353003/170001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_dbg
The CQ bit was checked by skobes@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skobes@chromium.org/209353003/170001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on mac_blink_rel
The CQ bit was checked by skobes@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skobes@chromium.org/209353003/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on win_blink_compile_dbg
The CQ bit was checked by skobes@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skobes@chromium.org/209353003/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on win_blink_rel
The CQ bit was checked by skobes@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skobes@chromium.org/209353003/280001
Message was sent while issue was closed.
Change committed as 169997
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/212163006/ by vsevik@chromium.org. The reason for reverting is: This might have caused android tests failure: http://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20%28dbg... Speculatively reverting it..
Message was sent while issue was closed.
https://codereview.chromium.org/209353003/diff/20001/LayoutTests/fast/text-au... File LayoutTests/fast/text-autosizing/font-scale-factor-change-expected.html (right): https://codereview.chromium.org/209353003/diff/20001/LayoutTests/fast/text-au... LayoutTests/fast/text-autosizing/font-scale-factor-change-expected.html:12: <div style="font-size: 2rem"> On 2014/03/25 18:25:26, skobes wrote: > On 2014/03/25 13:08:14, timvolodine wrote: > > so the 2 is due to setFontScaleFactor(2)? I understand there is no actual > > autosizing (i.e. multiplier 320/320=1) involved here? if that's the case maybe > > add something to the explaining text. > > I'm not following what you mean here. Autosizing is how the font scale factor > is implemented. What I meant is that you are testing the accessibility setting only. Usually we use width=800 for autosizing resulting in 800/320=2.5 multiplier. You seem to use width=320, so if there was no call to setFontScaleFactor(2) the text would not be autosized. In that this test differs from the others and that is what I was trying to clarify. But this is just a nit ;), as it does not impact code or functionality. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
