Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1759853003:
Merge contiguous anonymous blocks after removing a float or out-of-pos-element between them (Closed)
|
Created:
4 years, 9 months ago by rhogan Modified:
4 years, 9 months ago Reviewers:
mstensho (USE GERRIT) CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/src.git@585604-4 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMerge contiguous anonymous blocks after removing a float or out-of-pos-element between them
BUG=592734
Committed: https://crrev.com/59d3539fc7ce60355ccf52c00733d33dde5e118a
Cr-Commit-Position: refs/heads/master@{#381087}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Updated #
Total comments: 3
Patch Set 3 : Updated #
Messages
Total messages: 22 (4 generated)
robhogan@gmail.com changed reviewers: + mstensho@opera.com
https://codereview.chromium.org/1759853003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right): https://codereview.chromium.org/1759853003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBlock.cpp:254: void LayoutBlock::collapseAnonymousBlockChildAfterMerge(LayoutObject* prev, LayoutObject* next, bool canMergeAnonymousBlocks) I don't see a good alternative to passing canMergeAnonymousBlocks here. https://codereview.chromium.org/1759853003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBlock.cpp:352: parent->collapseAnonymousBlockChildAfterMerge(prev, next, true); Was forced into the use of parent-> by canCollapseAnonymousBlockChild() being protected and sub-classed in a lot of places, so I can't call it from a static function.
Is the bug number right? It's marked as fixed. I also think you should limit this change to only take place when styleDidChange() is called (when something goes out of normal flow). There should be no way for a float or out-of-flow positioned object to end up outside an adjacent anonymous block otherwise. https://codereview.chromium.org/1759853003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/block/float/add-float-back-to-anonymous-block-previous.html (right): https://codereview.chromium.org/1759853003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/block/float/add-float-back-to-anonymous-block-previous.html:7: <div style="position: absolute; top: 50px; width: 500px;"> How about removing the out-of-flow positioning, setting an id="container" and checkLayout("#container")? I think that would look pretty. https://codereview.chromium.org/1759853003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/block/float/add-float-back-to-anonymous-block-previous.html:12: <div id="float" data-offset-y=50></div> Duplicate ID #float. https://codereview.chromium.org/1759853003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/block/float/add-float-back-to-anonymous-block-previous.html:18: document.getElementById("div").style.float = 'none'; Why do you need to unfloat it dynamically first? Seems like an unnecessary step. Couldn't #div just be un-floated initially, then layout, and then set style.float="left" on it? https://codereview.chromium.org/1759853003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBlock.h (right): https://codereview.chromium.org/1759853003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBlock.h:459: void collapseAnonymousBlockChildAfterMerge(LayoutObject* prev, LayoutObject* next, bool canMergeAnonymousBlocks); Please add a blank line before "protected:"
On 2016/03/04 at 08:46:24, mstensho wrote: > Is the bug number right? It's marked as fixed. > > I also think you should limit this change to only take place when styleDidChange() is called (when something goes out of normal flow). There should be no way for a float or out-of-flow positioned object to end up outside an adjacent anonymous block otherwise. Sure - but here we're dealing with floating/out-of-flow siblings that can be moved into newly adjacent anonymous blocks. So I wouldn't rule out the need to merge two anonymous blocks arising for any caller of this function. Though I do think it only currently happens when called from styleDidChange.
On 2016/03/05 at 12:13:13, rhogan wrote: > Sure - but here we're dealing with floating/out-of-flow siblings that can be moved into newly adjacent anonymous blocks. So I wouldn't rule out the need to merge two anonymous blocks arising for any caller of this function. Though I do think it only currently happens when called from styleDidChange. Addressed your other comments in the updated CL.
Is the bug number right? It's marked as fixed.
On 2016/03/07 at 20:16:54, mstensho wrote: > Is the bug number right? It's marked as fixed. It's a follow on from that bug. I guess I could create a new one.
Description was changed from ========== Merge contiguous anonymous blocks after removing a float or out-of-pos-element between them BUG=585064 ========== to ========== Merge contiguous anonymous blocks after removing a float or out-of-pos-element between them BUG=592734 ==========
On 2016/03/07 20:51:19, rhogan wrote: > On 2016/03/07 at 20:16:54, mstensho wrote: > > Is the bug number right? It's marked as fixed. > > It's a follow on from that bug. I guess I could create a new one. Oh, you just did! No need, really, if you think it belongs together (it just wasn't obvious to me, but that's fine). But since you did, please link the bugs together somehow (put a reference to that fixed bug or something).
Done! On Mon, 7 Mar 2016 20:59 , <mstensho@opera.com> wrote: > On 2016/03/07 20:51:19, rhogan wrote: > > On 2016/03/07 at 20:16:54, mstensho wrote: > > > Is the bug number right? It's marked as fixed. > > > > It's a follow on from that bug. I guess I could create a new one. > > Oh, you just did! No need, really, if you think it belongs together (it > just > wasn't obvious to me, but that's fine). But since you did, please link the > bugs > together somehow (put a reference to that fixed bug or something). > > https://codereview.chromium.org/1759853003/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Done! On Mon, 7 Mar 2016 20:59 , <mstensho@opera.com> wrote: > On 2016/03/07 20:51:19, rhogan wrote: > > On 2016/03/07 at 20:16:54, mstensho wrote: > > > Is the bug number right? It's marked as fixed. > > > > It's a follow on from that bug. I guess I could create a new one. > > Oh, you just did! No need, really, if you think it belongs together (it > just > wasn't obvious to me, but that's fine). But since you did, please link the > bugs > together somehow (put a reference to that fixed bug or something). > > https://codereview.chromium.org/1759853003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I don't understand (or understand the need for) half of the (mostly ancient) code that you're moving around here, so I'd like to clean that up first, if that okay with you. See: https://codereview.chromium.org/1778463002/ https://codereview.chromium.org/1778483002/ You can ignore all my review comments here if you agree with my clean-up plan. https://codereview.chromium.org/1759853003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right): https://codereview.chromium.org/1759853003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBlock.cpp:301: if (prev->childrenInline() != next->childrenInline()) { What is this code supposed to handle? When do we have anonymous blocks with block children? Inline continuations (and is that even mergeable)? I added an ASSERT(0) here and ran through all LayoutTests/. No crashes. So this piece of code lacks coverage, if it's possible to exercise it at all. OK, did some digging. https://bugs.webkit.org/show_bug.cgi?id=39615, implement basic support for -webkit-column-span. This code was added for the old multicol implementation, which has been removed. No need to keep this around to complicate the code. https://codereview.chromium.org/1759853003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBlock.cpp:316: // of "this". we null out prev or next so that is not used later in the function. This comment needs an update. https://codereview.chromium.org/1759853003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBlock.cpp:795: collapseAnonymousBlockChildAfterMerge(prev, next, canMergeAnonymousBlocks); A merge has only taken place if canMergeAnonymousBlocks is set, so this is a bit confusing.
On 2016/03/08 at 10:29:42, mstensho wrote: > I don't understand (or understand the need for) half of the (mostly ancient) code that you're moving around here, so I'd like to clean that up first, if that okay with you. > > See: > https://codereview.chromium.org/1778463002/ > https://codereview.chromium.org/1778483002/ > Sweet - I'll let you do that first I guess.
On 2016/03/09 20:20:42, rhogan wrote: > On 2016/03/08 at 10:29:42, mstensho wrote: > > I don't understand (or understand the need for) half of the (mostly ancient) > code that you're moving around here, so I'd like to clean that up first, if that > okay with you. > > > > See: > > https://codereview.chromium.org/1778463002/ > > https://codereview.chromium.org/1778483002/ > > > > Sweet - I'll let you do that first I guess. Those CLs have landed, so it would be interesting to see what this CL would look like now. I guess there'll be no need for LayoutBlock::collapseAnonymousBlockChildAfterMerge()?
On 2016/03/11 at 20:45:09, mstensho wrote: > On 2016/03/09 20:20:42, rhogan wrote: > > On 2016/03/08 at 10:29:42, mstensho wrote: > > > I don't understand (or understand the need for) half of the (mostly ancient) > > code that you're moving around here, so I'd like to clean that up first, if that > > okay with you. > > > > > > See: > > > https://codereview.chromium.org/1778463002/ > > > https://codereview.chromium.org/1778483002/ > > > > > > > Sweet - I'll let you do that first I guess. > > Those CLs have landed, so it would be interesting to see what this CL would look like now. I guess there'll be no need for LayoutBlock::collapseAnonymousBlockChildAfterMerge()? Want to take another look?
lgtm, very nice!
The CQ bit was checked by robhogan@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1759853003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1759853003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Merge contiguous anonymous blocks after removing a float or out-of-pos-element between them BUG=592734 ========== to ========== Merge contiguous anonymous blocks after removing a float or out-of-pos-element between them BUG=592734 Committed: https://crrev.com/59d3539fc7ce60355ccf52c00733d33dde5e118a Cr-Commit-Position: refs/heads/master@{#381087} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/59d3539fc7ce60355ccf52c00733d33dde5e118a Cr-Commit-Position: refs/heads/master@{#381087} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
