CanvasRenderingContext2D direction should reflect change in Element's dir attribute
Created attachment 236862 [details] Patch
Comment on attachment 236862 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236862&action=review Doesn’t work right with save/restore. After this sequence: context.direction = "inherit"; context.save(); context.direction = "rtl"; context.restore(); the context will be in non-inherited state, but should be in inherited state. The correct way to fix this is to change m_direction in State to have a separate state for inherit, so perhaps not be a TextDirection at all, or to add a boolean there, or have it be an Optional<TextDirection> or something like that. Need test cases that cover the behavior with save/restore. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:124 > m_stateStack.first().m_direction = inheritedDirection(*canvas); This is no longer a useful thing to do. It wastefully computes a direction that will never be used since drawTextInternal will re-compute the direction every time. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2294 > + if (m_hasInheritedDirection && computedStyle) If computedStyle is null then we should use LTR, not the direction that already happens to be in the state. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2295 > + modifiableState().m_direction = computedStyle->direction(); It’s not helpful to store a new direction in the context, since we just have to recompute it every time anyway.
Created attachment 236920 [details] Patch
Created attachment 236922 [details] Patch
Created attachment 236926 [details] Patch
(In reply to comment #5) > Created an attachment (id=236926) [details] > Patch Cleaned up the State constructor which I missed in the previous one.
Comment on attachment 236926 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236926&action=review > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2181 > + TextDirection direction; > + switch (state().m_direction) { > + case DirectionInherit: > + direction = inheritedDirection(*canvas()); > + break; > + case DirectionRTL: > + direction = RTL; > + break; > + case DirectionLTR: > + direction = LTR; > + break; > + default: > + ASSERT_NOT_REACHED(); > + break; > + } Would be nicer to share this switch statement with drawTextInternal. But it might require too much rearranging to do that cleanly. Also, best pattern for such switch statements is to not have a default. The way we do that is to put the switch into a function, use return, and then put the ASSERT_NOT_REACHED after the switch statement. This makes it so that if we add a new enumeration value, we get a compile time warning rather than just a runtime assertion. > Source/WebCore/html/canvas/CanvasRenderingContext2D.h:231 > + enum Direction { Why not use "enum class" instead of enum? We are doing that in most new code, I believe. > Source/WebCore/html/canvas/CanvasRenderingContext2D.h:232 > + DirectionInherit = -1, Seems unnecessary to give this a special value of -1. We should just let the enum values be the default ones starting with zero; no reason not to!
Created attachment 236966 [details] Patch
(In reply to comment #7) > (From update of attachment 236926 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=236926&action=review > > > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2181 > > + TextDirection direction; > > + switch (state().m_direction) { > > + case DirectionInherit: > > + direction = inheritedDirection(*canvas()); > > + break; > > + case DirectionRTL: > > + direction = RTL; > > + break; > > + case DirectionLTR: > > + direction = LTR; > > + break; > > + default: > > + ASSERT_NOT_REACHED(); > > + break; > > + } > > Would be nicer to share this switch statement with drawTextInternal. But it might require too much rearranging to do that cleanly. > Done. > Also, best pattern for such switch statements is to not have a default. The way we do that is to put the switch into a function, use return, and then put the ASSERT_NOT_REACHED after the switch statement. This makes it so that if we add a new enumeration value, we get a compile time warning rather than just a runtime assertion. Done. > > > Source/WebCore/html/canvas/CanvasRenderingContext2D.h:231 > > + enum Direction { > > Why not use "enum class" instead of enum? We are doing that in most new code, I believe. > Done. > > Source/WebCore/html/canvas/CanvasRenderingContext2D.h:232 > > + DirectionInherit = -1, > > Seems unnecessary to give this a special value of -1. We should just let the enum values be the default ones starting with zero; no reason not to! Done.
Comment on attachment 236966 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236966&action=review If this becomes a performance problem we might some day want to add code to cache the inherited direction. It’s not trivial invalidate such a cache correctly, but the extra cost of updating style and calling computedStyle every time we draw text might make canvas text drawing slow enough that such an optimization would be worth while. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2158 > +TextDirection CanvasRenderingContext2D::toTextDirection(Direction direction, RenderStyle*& computedStyle) const The inline keyword needs to be here on the definition, rather than in the header file on the declaration. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2161 > + canvas()->document().updateStyleIfNeeded(); > + computedStyle = canvas()->computedStyle(); This work is wasteful when 1) the caller is not going to use computedStyle and 2) the direction is not Inherit. One way to fix that is to change the computedStyle out argument to make it optional; change it to a RenderStyle** with a default of nullptr. It would also complicate the logic a bit but it would make the direction getter significantly more efficient when the direction is set explicitly to LTR or RTL. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2176 > + RenderStyle* computedStyle = nullptr; I don’t think you need to initialize this, since it’s an out parameter. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2295 > + RenderStyle* computedStyle = nullptr; Ditto. > Source/WebCore/html/canvas/CanvasRenderingContext2D.h:234 > + Rtl, > + Ltr These should be all capitals. We capitalize acronyms. > Source/WebCore/html/canvas/CanvasRenderingContext2D.h:348 > + inline TextDirection toTextDirection(Direction, RenderStyle*& computedStyle) const; The inline keyword should be omitted here. If you do a little research you’ll find that information in C++ documentation.
Comment on attachment 236966 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236966&action=review >> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2158 >> +TextDirection CanvasRenderingContext2D::toTextDirection(Direction direction, RenderStyle*& computedStyle) const > > The inline keyword needs to be here on the definition, rather than in the header file on the declaration. Will change it. >> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2161 >> + computedStyle = canvas()->computedStyle(); > > This work is wasteful when 1) the caller is not going to use computedStyle and 2) the direction is not Inherit. One way to fix that is to change the computedStyle out argument to make it optional; change it to a RenderStyle** with a default of nullptr. It would also complicate the logic a bit but it would make the direction getter significantly more efficient when the direction is set explicitly to LTR or RTL. Sure will change it. In fact had done it locally but was using the pointer reference for default argument. Should have used this logic of double pointer. Will change it accordingly. Thank you for all the effort you are taking during this review. >> Source/WebCore/html/canvas/CanvasRenderingContext2D.h:234 >> + Ltr > > These should be all capitals. We capitalize acronyms. The style checker did complain about all things being in capital, hence I ended up making it this way. Earlier, with just the enum, these were prefixed with Direction (DirectionInherit, DirectionRTL and DirectionLTR). With enum class being strict about using fully qualified name, I dropped the prefix. Is it OK to ignore style checker error for this case? Or do you suggest some other alternative. >> Source/WebCore/html/canvas/CanvasRenderingContext2D.h:348 >> + inline TextDirection toTextDirection(Direction, RenderStyle*& computedStyle) const; > > The inline keyword should be omitted here. If you do a little research you’ll find that information in C++ documentation. Sure, will change it.
Comment on attachment 236966 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236966&action=review >>> Source/WebCore/html/canvas/CanvasRenderingContext2D.h:234 >>> + Ltr >> >> These should be all capitals. We capitalize acronyms. > > The style checker did complain about all things being in capital, hence I ended up making it this way. Earlier, with just the enum, these were prefixed with Direction (DirectionInherit, DirectionRTL and DirectionLTR). With enum class being strict about using fully qualified name, I dropped the prefix. Is it OK to ignore style checker error for this case? Or do you suggest some other alternative. The style checker is wrong. It’s OK to ignore the error and we should also get someone to fix the style checker.
(In reply to comment #10) > (From update of attachment 236966 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=236966&action=review > > If this becomes a performance problem we might some day want to add code to cache the inherited direction. It’s not trivial invalidate such a cache correctly, but the extra cost of updating style and calling computedStyle every time we draw text might make canvas text drawing slow enough that such an optimization would be worth while. Sure, shall we put a "FIXME" which summarizes above performance concern in the code? > > > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2161 > > + canvas()->document().updateStyleIfNeeded(); > > + computedStyle = canvas()->computedStyle(); > > This work is wasteful when 1) the caller is not going to use computedStyle and 2) the direction is not Inherit. I think current version of toTextDirection is trying to overdo things such as updateStyleIfNeeded. Its better if the call site makes the call to updateStyleIfNeeded() explicit and toTextDirection just takes care of the conversion. WDYT? For point 2, the computed Style is used in computing override (using isOverride function) which is irrespective of direction. So I think we still need to get the computedStyle for this to be correct. Is this understanding correct? Uploaded a new patch reflecting the same, PTAL. Thanks!
Created attachment 237081 [details] Patch
Attachment 237081 [details] did not pass style-queue: ERROR: Source/WebCore/html/canvas/CanvasRenderingContext2D.h:233: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] ERROR: Source/WebCore/html/canvas/CanvasRenderingContext2D.h:234: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 2 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 237081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=237081&action=review > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2170 > + RenderStyle* style = canvas()->computedStyle(); > + if (computedStyle) > + *computedStyle = style; > + switch (direction) { > + case Direction::Inherit: > + return style ? style->direction() : LTR; > + case Direction::RTL: > + return RTL; > + case Direction::LTR: > + return LTR; > + } If computedStyle is null and direction is not Inherit, we’d prefer not to call canvas() or computedStyle() at all. Reorganizing so this is done would be slightly ugly, but I think we should do it.
Comment on attachment 236966 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236966&action=review >>>> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2161 >>>> + computedStyle = canvas()->computedStyle(); >>> >>> This work is wasteful when 1) the caller is not going to use computedStyle and 2) the direction is not Inherit. One way to fix that is to change the computedStyle out argument to make it optional; change it to a RenderStyle** with a default of nullptr. It would also complicate the logic a bit but it would make the direction getter significantly more efficient when the direction is set explicitly to LTR or RTL. >> >> Sure will change it. In fact had done it locally but was using the pointer reference for default argument. Should have used this logic of double pointer. Will change it accordingly. Thank you for all the effort you are taking during this review. > > I think current version of toTextDirection is trying to overdo things such as updateStyleIfNeeded. Its better if the call site makes the call to updateStyleIfNeeded() explicit and toTextDirection just takes care of the conversion. WDYT? > > For point 2, the computed Style is used in computing override (using isOverride function) which is irrespective of direction. So I think we still need to get the computedStyle for this to be correct. Is this understanding correct? > > Uploaded a new patch reflecting the same, PTAL. Thanks! I don’t see how the call site could decide about updateStyleIfNeeded. In at least one case, it needs to know if the direction is Inherit. It’s true that in CanvasRenderingContext2D::drawTextInternal updateStyleIfNeeded will have been called, and the RenderStyle is needed. It’s CanvasRenderingContext2D::direction that needs the lighter weight version and would want to do updateStyleIfNeeded and get the RenderStyle only for the Inherit case.
Comment on attachment 237081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=237081&action=review > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2177 > + canvas()->document().updateStyleIfNeeded(); This is only needed when direction is Inherit. It’s wasteful to always do it.
Created attachment 237145 [details] Patch
Attachment 237145 [details] did not pass style-queue: ERROR: Source/WebCore/html/canvas/CanvasRenderingContext2D.h:233: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] ERROR: Source/WebCore/html/canvas/CanvasRenderingContext2D.h:234: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 2 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 237145 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=237145&action=review OK, lets land this one. > LayoutTests/fast/canvas/canvas-direction.html:220 > + [{ text: 'Left-to-Right text', direction: 'ltr' }, > + { text: 'Right-to-Left text', direction: 'rtl' }, > + { text: 'Right-to-Left text', direction: 'rtl' }, > + { text: 'Left-to-Right text', direction: 'ltr' }, > + { text: 'Right-to-Left text', direction: 'rtl' }, > + { text: 'Right-to-Left text', direction: 'rtl' }]); We don’t do this kind of indenting in the WebKit. We never want to have to reindent if we rename something.
Comment on attachment 237145 [details] Patch Clearing flags on attachment: 237145 Committed r172995: <http://trac.webkit.org/changeset/172995>
All reviewed patches have been landed. Closing bug.