WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
NEW
88580
RenderObject should not know about first-line element
https://bugs.webkit.org/show_bug.cgi?id=88580
Summary
RenderObject should not know about first-line element
Igor Trindade Oliveira
Reported
2012-06-07 15:09:23 PDT
RenderObject has some code related with first-line. This code should be in RenderBlockLineLayout or RenderBoxModelObject.
Attachments
exploratory patch
(60.51 KB, patch)
2012-06-07 15:59 PDT
,
Igor Trindade Oliveira
no flags
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Igor Trindade Oliveira
Comment 1
2012-06-07 15:59:20 PDT
Created
attachment 146408
[details]
exploratory patch
Levi Weintraub
Comment 2
2012-06-07 16:49:22 PDT
Comment on
attachment 146408
[details]
exploratory patch View in context:
https://bugs.webkit.org/attachment.cgi?id=146408&action=review
> Source/WebCore/rendering/InlineBox.h:278 > + RenderStyle* styleForFirstLine(bool isStyleForFirstLine) const > + { > + RenderStyle* style = m_renderer->style(); > + if (isStyleForFirstLine) { > + if (m_renderer->isText()) > + style = toRenderBoxModelObject(m_renderer->parent())->styleForFirstLine(isStyleForFirstLine); > + else > + style = boxModelObject()->styleForFirstLine(isStyleForFirstLine); > + } > + return style; > + }
This naming confuses me. If the function is called styleForFirstLine I don't expect to call it with a bool saying whether I actually want the first line. Also, this is too much code to be written in the definition. I'd rather see something like what you see in RenderObject::preservesNewline() where you state it's inline, implement it in the header, but have the implementation at the end of the file.
> Source/WebCore/rendering/InlineBox.h:296 > + EVerticalAlign verticalAlign() const > + { > + RenderBoxModelObject* object; > + if (m_renderer->isText()) > + object = toRenderBoxModelObject(m_renderer->parent()); > + else > + object = boxModelObject(); > + return object->styleForFirstLine(m_bitfields.firstLine())->verticalAlign(); > + }
Ditto.
Julien Chaffraix
Comment 3
2012-06-12 15:59:24 PDT
Comment on
attachment 146408
[details]
exploratory patch View in context:
https://bugs.webkit.org/attachment.cgi?id=146408&action=review
>> Source/WebCore/rendering/InlineBox.h:278 >> + } > > This naming confuses me. If the function is called styleForFirstLine I don't expect to call it with a bool saying whether I actually want the first line. > > Also, this is too much code to be written in the definition. I'd rather see something like what you see in RenderObject::preservesNewline() where you state it's inline, implement it in the header, but have the implementation at the end of the file.
For the boolean, this is unfortunately the current state of our code where we get a boolean telling us if we are on the first line. I would have preferred more differentiated code paths but that's likely too difficult to implement as part of this change. The naming could indeed love some massaging. It's more a firstLineAwareStyle() than really a styleForFirstLine().
> Source/WebCore/rendering/InlineTextBox.cpp:922 > - renderer()->getTextDecorationColors(deco, underline, overline, linethrough, true, isFirstLineStyle()); > + toRenderBoxModelObject(renderer()->parent())->getTextDecorationColors(deco, underline, overline, linethrough, true, isFirstLineStyle());
Looks like an change in behavior.
> Source/WebCore/rendering/RenderBR.cpp:45 > + RenderStyle* s = toRenderBoxModelObject(parent())->styleForFirstLine(firstLine);
There is a lot of toRenderBoxModelObject(someRenderer)->styleForFirstLine. Could it be possible to reduce the number of such casts? One idea that pops up is to change helper functions to take a RenderBoxModelObject instead of a RenderObject.
Ryosuke Niwa
Comment 4
2012-06-12 18:43:53 PDT
(In reply to
comment #3
)
> For the boolean, this is unfortunately the current state of our code where we get a boolean telling us if we are on the first line. I would have preferred more differentiated code paths but that's likely too difficult to implement as part of this change. > > The naming could indeed love some massaging. It's more a firstLineAwareStyle() than really a styleForFirstLine().
I like that!
Levi Weintraub
Comment 5
2012-06-13 10:37:01 PDT
(In reply to
comment #4
)
> (In reply to
comment #3
) > > For the boolean, this is unfortunately the current state of our code where we get a boolean telling us if we are on the first line. I would have preferred more differentiated code paths but that's likely too difficult to implement as part of this change. > > > > The naming could indeed love some massaging. It's more a firstLineAwareStyle() than really a styleForFirstLine(). > > I like that!
Seconded! I wasn't saying we can't take a boolean, I was just saying that the name styleForFirstLine didn't actually seem to describe what the function did. firstLineAwareStyle is clearer.
Eric Seidel (no email)
Comment 6
2012-06-20 05:47:03 PDT
(In reply to
comment #0
)
> RenderObject has some code related with first-line. This code should be in RenderBlockLineLayout or RenderBoxModelObject.
I support the idea of ripping subclass-specific code out of RenderObject, but it's not clear to me what you're trying to do here.
Igor Trindade Oliveira
Comment 7
2012-06-20 08:05:48 PDT
I am just moving the methods uncachedFirstLineStyle and firstLineStyleSlowCase out of the RenderObject, however doing that i need to change the code in lot of places, specially because RenderObject::style(bool) is also called by RenderText object, so in some cases i need to do: renderObject->isText() ? toRenderBoxModelObject(renderObject->parent())->firstLineStyle() : toRenderBoxModelObject(renderObject)->firstLineStyle(); This code is valid because firstLineStyleSlowCase() already does: const RenderObject* renderer = isText() ? parent() : this; In other parts of the code, i agree that probably the casts are not correctly(e.g. getTextDecorartion) but i am fixing case by case. (In reply to
comment #6
)
> (In reply to
comment #0
) > > RenderObject has some code related with first-line. This code should be in RenderBlockLineLayout or RenderBoxModelObject. > > I support the idea of ripping subclass-specific code out of RenderObject, but it's not clear to me what you're trying to do here.
Eric Seidel (no email)
Comment 8
2012-06-20 08:25:20 PDT
Comment on
attachment 146408
[details]
exploratory patch View in context:
https://bugs.webkit.org/attachment.cgi?id=146408&action=review
> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapper.mm:690 > + RenderBoxModelObject* boxModelObject = renderer->isText() ? toRenderBoxModelObject(renderer->parent()) : toRenderBoxModelObject(renderer);
Isn't there a boxModelObject() function on a renderer which does this?
Bruno Abinader (history only)
Comment 9
2012-08-23 08:12:02 PDT
Adding
bug 93829
as it _might_ change its behavior.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug