Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1320373003:
Add documentation to several LayoutObject functions (Closed)
|
Created:
5 years, 3 months ago by Julien - ping for review Modified:
5 years, 3 months ago CC:
blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/remotes/origin/master Project:
blink Visibility:
Public. |
DescriptionAdd documentation to several LayoutObject functions
Added some function-level documentation to:
- createObject
- willBeDestroyed
- virtualChildren
Those help explain some design decision of what they
are used for.
While documenting createObject, added a missing
ASSERT_NOT_REACHED to the implementation to state
our assumption clearly.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201584
Patch Set 1 #
Total comments: 11
Patch Set 2 : Updated after Morten's comments. #
Messages
Total messages: 13 (3 generated)
jchaffraix@chromium.org changed reviewers: + eae@chromium.org, mstensho@opera.com
LGTM Awesome!
https://codereview.chromium.org/1320373003/diff/1/Source/core/layout/LayoutOb... File Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/1320373003/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:348: // m_isInline below. This means that "display: inline-table" creates "below", as in "during initialization"? https://codereview.chromium.org/1320373003/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:351: // Ideally every Element::createLayoutObject would call this function to I either don't understand or don't agree. Isn't it great that we have Element::createLayoutObject overrides, so that we don't have to cram everything in here, and only worry about the generics? If you don't think so, come over here, and I'll show you the Presto code. :) https://codereview.chromium.org/1320373003/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:1467: // of the current line (inline) or generate a new formatting context Doesn't necessarily generate a new formatting context if it's false. Example: <div> To say something CSS lingo compatible here, I could suggest "inline-level" vs. "block-level".
https://codereview.chromium.org/1320373003/diff/1/Source/core/layout/LayoutOb... File Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/1320373003/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:348: // m_isInline below. This means that "display: inline-table" creates On 2015/08/31 at 19:48:07, mstensho wrote: > "below", as in "during initialization"? "below" as in ↓ (unicode FTW!) Changed anyway as during initialization is a better phrasing. https://codereview.chromium.org/1320373003/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:351: // Ideally every Element::createLayoutObject would call this function to On 2015/08/31 at 19:48:07, mstensho wrote: > I either don't understand or don't agree. > > Isn't it great that we have Element::createLayoutObject overrides, so that we don't have to cram everything in here, and only worry about the generics? If you don't think so, come over here, and I'll show you the Presto code. :) No Presto code open showroom please :) We have a lot of bugs where people don't understand why display: block on <iframe> doesn't work. Granted it's probably a corner-case but there should be a cleaner mid-way between cramming everything in createObject (Presto) and not replying to display change half of the time (Blink). Happy to rephrase the idea is that we should be consistent with our treatment of 'display' by having createLayoutObject systematically call into this (probably passing in some context along to handle the tag specific information). https://codereview.chromium.org/1320373003/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:1467: // of the current line (inline) or generate a new formatting context On 2015/08/31 at 19:48:07, mstensho wrote: > Doesn't necessarily generate a new formatting context if it's false. Example: <div> > > To say something CSS lingo compatible here, I could suggest "inline-level" vs. "block-level". Fair point, changed to some better CSS lingo: // This boolean represents whether the LayoutObject is 'inline-level' // (a CSS concept). Inline-level boxes are laid out inside a line. If // unset, the box is 'block-level' and thus stack on top of its // siblings (think of paragraph).
https://codereview.chromium.org/1320373003/diff/1/Source/core/layout/LayoutOb... File Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/1320373003/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:348: // m_isInline below. This means that "display: inline-table" creates On 2015/08/31 22:38:53, Julien Chaffraix - PST wrote: > On 2015/08/31 at 19:48:07, mstensho wrote: > > "below", as in "during initialization"? > > "below" as in ↓ (unicode FTW!) > > Changed anyway as during initialization is a better phrasing. ☑ https://codereview.chromium.org/1320373003/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:351: // Ideally every Element::createLayoutObject would call this function to On 2015/08/31 22:38:53, Julien Chaffraix - PST wrote: > On 2015/08/31 at 19:48:07, mstensho wrote: > > I either don't understand or don't agree. > > > > Isn't it great that we have Element::createLayoutObject overrides, so that we > don't have to cram everything in here, and only worry about the generics? If you > don't think so, come over here, and I'll show you the Presto code. :) > > No Presto code open showroom please :) > > We have a lot of bugs where people don't understand why display: block on > <iframe> doesn't work. At the same time, those people should be thankful that display:table-row doesn't work on <iframe>. :) Right, so LayoutIFrame, for instance, forgets to set m_isInline = false? Hm.. no, that doesn't seem to be the case. This just works: before <iframe src="http://bitten.servebeer.com/" style="display:block;"></iframe> after > Granted it's probably a corner-case but there should be a > cleaner mid-way between cramming everything in createObject (Presto) and not > replying to display change half of the time (Blink). > > Happy to rephrase the idea is that we should be consistent with our treatment of > 'display' by having createLayoutObject systematically call into this (probably > passing in some context along to handle the tag specific information). In my mind, createObject() can only do harm if the Element has a better idea about what to create. If we introduce a separation between display-outer and display-inner, it could make sense to let createObject() deal with the display-outer part, though. Then we could make things like <input style="display:table-cell;"> work. https://codereview.chromium.org/1320373003/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:1467: // of the current line (inline) or generate a new formatting context On 2015/08/31 22:38:53, Julien Chaffraix - PST wrote: > On 2015/08/31 at 19:48:07, mstensho wrote: > > Doesn't necessarily generate a new formatting context if it's false. Example: > <div> > > > > To say something CSS lingo compatible here, I could suggest "inline-level" vs. > "block-level". > > Fair point, changed to some better CSS lingo: > > // This boolean represents whether the LayoutObject is 'inline-level' > // (a CSS concept). Inline-level boxes are laid out inside a line. If > // unset, the box is 'block-level' and thus stack on top of its > // siblings (think of paragraph). Nice!
https://codereview.chromium.org/1320373003/diff/1/Source/core/layout/LayoutOb... File Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/1320373003/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:351: // Ideally every Element::createLayoutObject would call this function to On 2015/09/01 at 07:56:23, mstensho wrote: > On 2015/08/31 22:38:53, Julien Chaffraix - PST wrote: > > On 2015/08/31 at 19:48:07, mstensho wrote: > > > I either don't understand or don't agree. > > > > > > Isn't it great that we have Element::createLayoutObject overrides, so that we > > don't have to cram everything in here, and only worry about the generics? If you > > don't think so, come over here, and I'll show you the Presto code. :) > > > > No Presto code open showroom please :) > > > > We have a lot of bugs where people don't understand why display: block on > > <iframe> doesn't work. > > At the same time, those people should be thankful that display:table-row doesn't work on <iframe>. :) Actually that's what I am advocating. Currently we only extract the inline-level / block-level information from the style, which confuses people. > Right, so LayoutIFrame, for instance, forgets to set m_isInline = false? Hm.. no, that doesn't seem to be the case. This just works: > > before > <iframe src="http://bitten.servebeer.com/" style="display:block;"></iframe> > after <3 :) > > Granted it's probably a corner-case but there should be a > > cleaner mid-way between cramming everything in createObject (Presto) and not > > replying to display change half of the time (Blink). > > > > Happy to rephrase the idea is that we should be consistent with our treatment of > > 'display' by having createLayoutObject systematically call into this (probably > > passing in some context along to handle the tag specific information). > > In my mind, createObject() can only do harm if the Element has a better idea about what to create. If we introduce a separation between display-outer and display-inner, it could make sense to let createObject() deal with the display-outer part, though. Then we could make things like <input style="display:table-cell;"> work. I think we agree but somehow talk past each other: I am not advocating for adding more logic to LayoutObject::createObject but making sure Element's createLayoutObject take style into account by calling createObject (while keeping their element specific logic). I haven't outlined a plan to make that work but put that more as a statement of intention / design note. I do think this comment represents the consensus on createObject and what should be done but maybe I am misinterpreting your objections. Should I just remove it or can we amend it in some ways?
lgtm https://codereview.chromium.org/1320373003/diff/1/Source/core/layout/LayoutOb... File Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/1320373003/diff/1/Source/core/layout/LayoutOb... Source/core/layout/LayoutObject.h:351: // Ideally every Element::createLayoutObject would call this function to On 2015/09/01 16:18:29, Julien Chaffraix - PST wrote: > On 2015/09/01 at 07:56:23, mstensho wrote: > > On 2015/08/31 22:38:53, Julien Chaffraix - PST wrote: > > > On 2015/08/31 at 19:48:07, mstensho wrote: > > > > I either don't understand or don't agree. > > > > > > > > Isn't it great that we have Element::createLayoutObject overrides, so that > we > > > don't have to cram everything in here, and only worry about the generics? If > you > > > don't think so, come over here, and I'll show you the Presto code. :) > > > > > > No Presto code open showroom please :) > > > > > > We have a lot of bugs where people don't understand why display: block on > > > <iframe> doesn't work. > > > > At the same time, those people should be thankful that display:table-row > doesn't work on <iframe>. :) > > Actually that's what I am advocating. Currently we only extract the inline-level > / block-level information from the style, which confuses people. Okay, I cannot think of a lot else to extract, but HTMLRubyElement::createLayoutObject() may be an interesting case, for instance. We should at least be able to display ruby as inline-block, but we only handle "inline" and "block". Then again, one might argue that the whole approach here is wrong and that we should just implement the Ruby CSS spec instead and support ruby specific display values and just add ruby { display:ruby; } to html.css, so maybe yet another bad example. Anyway, my brain starts to act up when I think of <iframe style="display:table-row;">, but let's not talk more about that for now. :) > > Right, so LayoutIFrame, for instance, forgets to set m_isInline = false? Hm.. > no, that doesn't seem to be the case. This just works: > > > > before > > <iframe src="http://bitten.servebeer.com/" style="display:block;"></iframe> > > after > > <3 :) > > > > Granted it's probably a corner-case but there should be a > > > cleaner mid-way between cramming everything in createObject (Presto) and not > > > replying to display change half of the time (Blink). > > > > > > Happy to rephrase the idea is that we should be consistent with our > treatment of > > > 'display' by having createLayoutObject systematically call into this > (probably > > > passing in some context along to handle the tag specific information). > > > > In my mind, createObject() can only do harm if the Element has a better idea > about what to create. If we introduce a separation between display-outer and > display-inner, it could make sense to let createObject() deal with the > display-outer part, though. Then we could make things like <input > style="display:table-cell;"> work. > > I think we agree but somehow talk past each other: I am not advocating for > adding more logic to LayoutObject::createObject but making sure Element's > createLayoutObject take style into account by calling createObject (while > keeping their element specific logic). I haven't outlined a plan to make that > work but put that more as a statement of intention / design note. > > I do think this comment represents the consensus on createObject and what should > be done but maybe I am misinterpreting your objections. Should I just remove it > or can we amend it in some ways? Let's just keep it as it is.
The CQ bit was checked by jchaffraix@chromium.org
Thanks Morten!
The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org Link to the patchset: https://codereview.chromium.org/1320373003/#ps20001 (title: "Updated after Morten's comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1320373003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1320373003/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201584 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
