WIP: Implement CanvasRenderingContext2D direction attribute
Created attachment 236518 [details] Patch
Created attachment 236575 [details] Patch
Please take a look, thank you!
Link to WHATWG specificiation URL: http://www.whatwg.org/specs/web-apps/current-work/multipage/scripting.html#dom-context-2d-direction
Comment on attachment 236575 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236575&action=review If you’re going to do all that whitespace trimming, maybe that can be a separate patch that you land first. It’s not great to combine that with the substantive change. Test coverage is not sufficient. There are bugs in the code that the tests did not detect. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:107 > +static bool parseDirection(const String& direction) A function that does the isValidDirection operation should not be named “parse” because it doesn’t do the parsing. It doesn’t return the value of what’s parsed. So please don’t name it “parse”. But also, it doesn’t make sense to have this function at all. The code just below the one call site for this function handles all the three direction values anyway, so this function should not be added. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:109 > + return direction == "inherit" ? true : direction == "rtl" ? true : direction == "ltr" ? true : false; The || operator does the same thing as "? true :" so you should use that: return direction == "inherit" || direction == "rtl" || direction == "ltr"; > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:121 > + setDirection("inherit"); We don’t want to pass ourselves a string that we need to parse. We should find another way to share the code that reads the direction from the canvas and write it into the state. Probably simply: m_stateStack.first().m_direction = inheritedDirection(*canvas); The inheritedDirection function would take an HTMLCanvasElement& and also be used in the setDirection function. This work needs to be done in CanvasRenderingContext2D::reset as well, not just in the constructor. Need test coverage for the behavior of reset. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2169 > + if (!parseDirection(direction)) > + return; There’s no need for this. Instead the function should compute the direction first, returning if the direction string is invalid. Then it can call realizeSaves, and only then it can set modifiableState().m_direction. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2173 > + modifiableState().m_direction = canvas()->computedStyle() ? canvas()->computedStyle()->direction() : LTR; The computedStyle function does a lot of work. It’s not a good idea to call it twice. Please use a local variable. > Source/WebCore/html/canvas/CanvasRenderingContext2D.h:263 > + TextDirection m_direction; The default constructor for State needs to initialize this. We don’t want to leave it uninitialized. The copy constructor for State and the assignment operator for state need to copy this. > LayoutTests/ChangeLog:15 > + * fast/canvas/canvas-direction-inherited-rtl-expected.txt: Added. > + * fast/canvas/canvas-direction-inherited-rtl.html: Added. > + * fast/canvas/canvas-direction-inherited-unspecified-expected.txt: Added. > + * fast/canvas/canvas-direction-inherited-unspecified.html: Added. > + * fast/canvas/canvas-direction-rtl-override-with-ltr-expected.txt: Added. > + * fast/canvas/canvas-direction-rtl-override-with-ltr.html: Added. > + * fast/canvas/canvas-direction-unspecified-override-with-rtl-expected.txt: Added. > + * fast/canvas/canvas-direction-unspecified-override-with-rtl.html: Added. These should not all be separate tests. You should combine them into a single test. Also, we need more test coverage. For example, we need to test invalid values such as "INHERITED", "RTL", and "LTR" to make sure that the handling of direction is case sensitive.
Created attachment 236594 [details] Patch
Thank you Darin for the review. Have updated the patch with all your comments incorporated. Please take a look.
(In reply to comment #7) > Thank you Darin for the review. Have updated the patch with all your comments incorporated. Please take a look. A friendly ping! Thank you.
Comment on attachment 236594 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236594&action=review review+ but please don’t check in those PNG files > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:165 > m_stateStack.first() = State(); > + m_stateStack.first().m_direction = inheritedDirection(*canvas()); Might be cleaner to have a State constructor that takes a TextDirection to make these two be a single line. Using constructor delegation we could have the default constructor call the one that takes a TextDirection, so we wouldn’t have to repeat them. Or we could simply have the TextDirection argument have a default value of LTR and have a single constructor be both the default one and the one that takes TextDirection. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2168 > + return (state().m_direction == RTL) ? "rtl" : "ltr"; For better performance it should be ASCIILiteral("rtl") : ASCIILiteral("ltr"); > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2171 > +void CanvasRenderingContext2D::setDirection(const String& s) Would be nicer to name the argument "string" or "directionString" rather than just the letter "s". We frown on the use of letters instead of words for variable names. > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2176 > + if (s != "inherit" && s != "rtl" && s != "ltr") > + return; > + > + TextDirection direction = (s == "inherit") ? inheritedDirection(*canvas()) : (s == "rtl") ? RTL : LTR; Would be nicer to combine the direction computation with the validation: if (s == "inherit") direction = inheritedDirection(*canvas()); else if (s == "rtl") direction = RTL; else if (s == "ltr") direction = LTR; else return; That way we do only 3 string equality comparisons instead of 5. > LayoutTests/ChangeLog:11 > + * fast/canvas/canvas-direction-expected.txt: Added. > + * fast/canvas/canvas-direction.html: Added. > + * platform/gtk/fast/canvas/canvas-direction-expected.png: Added. > + * platform/mac/fast/canvas/canvas-direction-expected.png: Added. I don’t understand why we have the expected.png files here. If this is a text-only test, no png file should be written out or read. > LayoutTests/fast/canvas/canvas-direction.html:53 > + testRunner.dumpAsText(true); I don’t think the dumpAsText function takes an argument, does it? Should indent by 4 spaces, not 2.
Created attachment 236778 [details] Patch
Comment on attachment 236778 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236778&action=review > Source/WebCore/html/canvas/CanvasRenderingContext2D.h:232 > + State(TextDirection = LTR); Might be ever so slightly better to put an "explicit" here.
Created attachment 236780 [details] Patch
(In reply to comment #11) > (From update of attachment 236778 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=236778&action=review > > > Source/WebCore/html/canvas/CanvasRenderingContext2D.h:232 > > + State(TextDirection = LTR); > > Might be ever so slightly better to put an "explicit" here. Thank you very much for your help and time for the quick reviews :)
Comment on attachment 236780 [details] Patch Clearing flags on attachment: 236780 Committed r172723: <http://trac.webkit.org/changeset/172723>
All reviewed patches have been landed. Closing bug.