Bug 60254

Created attachment 93888 [details] Patch v1 An important piece of work toward my SVG Fonts rewrite patch (see master bug 59085).

Comment on attachment 93888 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=93888&action=review It seems that you really want TextRuns to be pointers so you can use virtual function dispatch. Since they aren't, you're adding this "extra data" pointer. I think "extra data" is not a very descriptive name for what you're doing. Maybe this is some sort of observer for the text run? It doesn't seem to be extra storage. > Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:400 > + if (RenderTextRun::ExtraData* extraData = textRun.extraData()) > + extraData->setActivePaintingResource(0); Why wouldn't this just be a setActivePaintingResource method on RenderTextRun? Why do callers need to know of the existence of ExtraData? The RenderTextRun object can make the call through the pointer internally.

Comment on attachment 93888 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=93888&action=review > Source/WebCore/platform/graphics/TextRun.h:107 > + virtual bool isRenderTextRun() const { return false; } This feels very strange. You shouldn't be asking this "extra data" member what type the thing holding onto it is. This is all one big hack around not having virtual dispatch for these stack objects it seems. Does the TextRun itself need to have these members? Or can there be a SVGTextRun which has a TextRun instead of is-a TextRun?

Comment on attachment 93888 [details] Patch v1 It seems all of the places which currently use TextRuns and grab at these SVG-specific members, don't really need to use TextRuns. They could use some other class which has-a TextRun, instead of is-a TextRun.

(In reply to comment #4) > (From update of attachment 93888 [details]) > It seems all of the places which currently use TextRuns and grab at these SVG-specific members, don't really need to use TextRuns. They could use some other class which has-a TextRun, instead of is-a TextRun. There is just one place: SVGFonts.cpp, and it doesn't have access to anything but TextRuns. After my SVGFonts rewrite, the SVG Fonts code is completly integrated within FontFastPath, where we only deal with TextRuns. The ExtraData "hack" is exactly because of the fact, that I didn't want to make TextRuns refcounted, and add virtual functions. The idea is to optimize for non-SVGFonts usage patterns, so I am afraid of making text runs refcounted and add virtual functions just for the sake of SVG Fonts, hence the whole ExtraData concept. I'd like to hear more opinions.

(In reply to comment #3) > (From update of attachment 93888 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=93888&action=review > > > Source/WebCore/platform/graphics/TextRun.h:107 > > + virtual bool isRenderTextRun() const { return false; } > > This feels very strange. You shouldn't be asking this "extra data" member what type the thing holding onto it is. This is all one big hack around not having virtual dispatch for these stack objects it seems. > > Does the TextRun itself need to have these members? Or can there be a SVGTextRun which has a TextRun instead of is-a TextRun? To rephrase: I could add a new class FooTextRun, that holds a RenderObject* and a TextRun object, as you said. The problem is that code in platform/ can't use FooTextRun directly, as it depends on RenderObject, and we'd have the same layering violation again. I'm not saying your approach isn't good, but it induces layering problems, that's why I decided to go the ExtraData way, minimizing the cost for platform fonts...

I think the approach is good. ExtraData might be better called RenderingContext or similar. I don't think needs to be refcounted, OwnPtr should do just fine. To really eliminate the layering violation the SVG calls in platform text drawing code should be replaced with virtual calls to RenderingContext: drawTextUsingSVGFont() -> run.renderingContext()->drawText() and similar.

I'd like to see the full patch that eliminates the layering violation fully to validate we are going to right direction. From my reading it shouldn't be that complex.

(In reply to comment #7) > I think the approach is good. > > ExtraData might be better called RenderingContext or similar. Ok, I tried to make it generic, but why not. > > I don't think needs to be refcounted, OwnPtr should do just fine. Nope, TextRuns can be copied, and OwnPtr's operator= is private and not defined, so we have to use refcounting, if we want to avoid allocating new ExtraData objects upon assigning TextRuns. > > To really eliminate the layering violation the SVG calls in platform text drawing code should be replaced with virtual calls to RenderingContext: > > drawTextUsingSVGFont() -> run.renderingContext()->drawText() and similar. As discussed on IRC, this will be part of the SVG Fonts rewrite, I don't want to invest time in the current SVGFont.cpp code to kill the violation completely.

(In reply to comment #9) Actually I don't think you need RenderTextRun at all. It has almost no functionality. constructTextRun() can choose wether to construct ExtraData/SVGRenderingContext or not. The patch would get much smaller and easier to review. > As discussed on IRC, this will be part of the SVG Fonts rewrite, I don't want to invest time in the current SVGFont.cpp code to kill the violation completely. The bug title indicates that this is about eliminating the violations. I don't see the point otherwise.

(In reply to comment #9) > (In reply to comment #7) > > I think the approach is good. > > > > ExtraData might be better called RenderingContext or similar. > Ok, I tried to make it generic, but why not. > > > > > I don't think needs to be refcounted, OwnPtr should do just fine. > Nope, TextRuns can be copied, and OwnPtr's operator= is private and not defined, so we have to use refcounting, if we want to avoid allocating new ExtraData objects upon assigning TextRuns. I take it TextRuns are copied? Should they be?

(In reply to comment #10) > (In reply to comment #9) > > Actually I don't think you need RenderTextRun at all. It has almost no functionality. constructTextRun() can choose wether to construct ExtraData/SVGRenderingContext or not. The patch would get much smaller and easier to review. I tested this idea, and it works like a charme. I called it AbstractRenderingContext, a sub-class of TextRun, and a TextRunRenderingContext class in rendering, inheriting from TextRun::AbstractRenderingContext. I sucessfully moved the drawTextUsingSVGFont() etc. functions in there, and implemented them in rendering/svg/SVGTextRunRenderingContext.cpp (moved from svg/SVGFont.cpp). So Font now uses the AbstractRenderingContext, if present, to delegate drawing/measuring there. It doesn't rely anymore on Font::drawTextUsingSVGFOnt which was implemented in svg/SVGFont.cpp, which is a clear violation. We still have a violation in SimpleFontData, which stores a SVGFontData object from svg/SVGFontData.cpp. I could solve it exactly the same way as above. > > > As discussed on IRC, this will be part of the SVG Fonts rewrite, I don't want to invest time in the current SVGFont.cpp code to kill the violation completely. > > The bug title indicates that this is about eliminating the violations. I don't see the point otherwise. You're right, I've patched the existing SVG Fonts code, so you can see the full scope of the patch. Preparing a new patch soon..

Created attachment 94218 [details] Patch v2

Attachment 94218 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1 Source/WebCore/svg/SVGFontData.cpp:26: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 36 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 94218 [details] Patch v2 Attachment 94218 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8721306

Comment on attachment 94218 [details] Patch v2 Attachment 94218 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8720320

Created attachment 94223 [details] Patch v3

Comment on attachment 94218 [details] Patch v2 Attachment 94218 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/8721346

Comment on attachment 94223 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=94223&action=review Looks good basically, I have some naming nits. > Source/WebCore/GNUmakefile.list.am:3086 > + Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp \ > Source/WebCore/rendering/svg/SVGTextMetrics.cpp \ > Source/WebCore/rendering/svg/SVGTextMetrics.h \ > Source/WebCore/rendering/svg/SVGTextQuery.cpp \ > Source/WebCore/rendering/svg/SVGTextQuery.h \ > Source/WebCore/rendering/TableLayout.h \ > + Source/WebCore/rendering/TextRunRenderingContext.h \ Why TextRunRenderingContext.h vs. SVGTextRunRenderingContext.cpp? > Source/WebCore/platform/graphics/TextRun.h:111 > + class AbstractRenderingContext : public RefCounted<AbstractRenderingContext> { I would just leave out the word "Abstract" here. > Source/WebCore/platform/graphics/TextRun.h:114 > + public: > + virtual ~AbstractRenderingContext() { } > + virtual bool isWebCoreRenderingContext() const = 0; All likely clients are in WebCore (as is this class) so name WebCoreRenderingContext doesn't really differentiate. Maybe just remove the whole function for now, you only have one concrete implementation so casting is always safe. To do this in really non-layer violating way for multiple implementations, you would need to have "int type" field, with enum values defined outside platform. > Source/WebCore/rendering/RenderCombineText.cpp:96 > - TextRun run = TextRun(String(text())); > + TextRun run = RenderBlock::constructTextRun(this, originalFont(), String(text()), style()); I know it is now in this patch but having constructTextRun in RenderBlock seems random. Why isn't it a free standing function? > Source/WebCore/rendering/TextRunRenderingContext.h:32 > +class TextRunRenderingContext : public TextRun::AbstractRenderingContext { > +public: I think this should be called SVGTextRunRenderingContext since it has a very specific purpose. > Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:389 > + TextRun::AbstractRenderingContext* abstractRenderingContext = textRun.renderingContext(); > + if (abstractRenderingContext && abstractRenderingContext->isWebCoreRenderingContext()) > + static_cast<TextRunRenderingContext*>(abstractRenderingContext)->setActivePaintingResource(m_paintingResource); As mentioned above isWebCoreRenderingContext() naming is weird and also unnecessary here. > Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:82 > +template<typename SVGTextRunData> > +struct SVGTextRunWalker { > + typedef bool (*SVGTextRunWalkerCallback)(const SVGGlyph&, SVGTextRunData&); > + typedef void (*SVGTextRunWalkerMissingGlyphCallback)(const TextRun&, SVGTextRunData&); > + > + SVGTextRunWalker(const SVGFontData* fontData, SVGFontElement* fontElement, SVGTextRunData& data, > + SVGTextRunWalkerCallback callback, SVGTextRunWalkerMissingGlyphCallback missingGlyphCallback) Instead of using function pointers you could make the callbacks template parameters for more inlining. Probably not for this patch.

(In reply to comment #19) > (From update of attachment 94223 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=94223&action=review > > Looks good basically, I have some naming nits. Great. > > > Source/WebCore/GNUmakefile.list.am:3086 > > + Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp \ > > Source/WebCore/rendering/svg/SVGTextMetrics.cpp \ > > Source/WebCore/rendering/svg/SVGTextMetrics.h \ > > Source/WebCore/rendering/svg/SVGTextQuery.cpp \ > > Source/WebCore/rendering/svg/SVGTextQuery.h \ > > Source/WebCore/rendering/TableLayout.h \ > > + Source/WebCore/rendering/TextRunRenderingContext.h \ > > Why TextRunRenderingContext.h vs. SVGTextRunRenderingContext.cpp? Fixed. > > > Source/WebCore/platform/graphics/TextRun.h:111 > > + class AbstractRenderingContext : public RefCounted<AbstractRenderingContext> { > > I would just leave out the word "Abstract" here. Fixed, same for AbstractFontData. > > > Source/WebCore/platform/graphics/TextRun.h:114 > > + public: > > + virtual ~AbstractRenderingContext() { } > > + virtual bool isWebCoreRenderingContext() const = 0; > > All likely clients are in WebCore (as is this class) so name WebCoreRenderingContext doesn't really differentiate. Maybe just remove the whole function for now, you only have one concrete implementation so casting is always safe. Ditto. > > To do this in really non-layer violating way for multiple implementations, you would need to have "int type" field, with enum values defined outside platform. Right, but as there's just one consumer at the moment, I'll leave it as-is. > > > Source/WebCore/rendering/RenderCombineText.cpp:96 > > - TextRun run = TextRun(String(text())); > > + TextRun run = RenderBlock::constructTextRun(this, originalFont(), String(text()), style()); > > I know it is now in this patch but having constructTextRun in RenderBlock seems random. Why isn't it a free standing function? A free standing function was disliked previously, so I added it to RenderBlock. Can we leave it as-is for now, and eventually move it, if we reach consensus? > > > Source/WebCore/rendering/TextRunRenderingContext.h:32 > > +class TextRunRenderingContext : public TextRun::AbstractRenderingContext { > > +public: > > I think this should be called SVGTextRunRenderingContext since it has a very specific purpose. Fixed. > > > Source/WebCore/rendering/svg/SVGInlineTextBox.cpp:389 > > + TextRun::AbstractRenderingContext* abstractRenderingContext = textRun.renderingContext(); > > + if (abstractRenderingContext && abstractRenderingContext->isWebCoreRenderingContext()) > > + static_cast<TextRunRenderingContext*>(abstractRenderingContext)->setActivePaintingResource(m_paintingResource); > > As mentioned above isWebCoreRenderingContext() naming is weird and also unnecessary here. Fixed. > > > Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:82 > > +template<typename SVGTextRunData> > > +struct SVGTextRunWalker { > > + typedef bool (*SVGTextRunWalkerCallback)(const SVGGlyph&, SVGTextRunData&); > > + typedef void (*SVGTextRunWalkerMissingGlyphCallback)(const TextRun&, SVGTextRunData&); > > + > > + SVGTextRunWalker(const SVGFontData* fontData, SVGFontElement* fontElement, SVGTextRunData& data, > > + SVGTextRunWalkerCallback callback, SVGTextRunWalkerMissingGlyphCallback missingGlyphCallback) > > Instead of using function pointers you could make the callbacks template parameters for more inlining. Probably not for this patch. The whole code is gone, as soon as the SVG Fonts rewrite patch lands, no more use of callbacks or function pointers in the new code. Preparing a new patch soon.

Created attachment 94599 [details] Patch v4 Fixed Anttis comments.

Comment on attachment 94599 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=94599&action=review r=me > Source/WebCore/rendering/svg/SVGTextRunRenderingContext.h:38 > + > + RenderObject* context() const { return m_context; } This could have some more specific name than "context", especially since the class itself is called *Context.

(In reply to comment #22) > (From update of attachment 94599 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=94599&action=review > > r=me > > > Source/WebCore/rendering/svg/SVGTextRunRenderingContext.h:38 > > + > > + RenderObject* context() const { return m_context; } > > This could have some more specific name than "context", especially since the class itself is called *Context. Renamed to renderer(). Thank for the review, landed in r87152.

Landed a win build fix in r87156.

Landed follow-up build fix in r87158.

This patch seems to have caused a regression on Chromium Webkit/Linux; see https://bugs.webkit.org/show_bug.cgi?id=61370