| Summary: | Refactor InlineTextBox::emphasisMarkExistsAndIsAbove() | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||||||||||
| Component: | Layout and Rendering | Assignee: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | bfulgham, darin, dbates, ews-watchlist, sabouhallawa, simon.fraser, webkit-bug-importer, zalan | ||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=186968 | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
David Kilzer (:ddkilzer)
2018-06-29 16:37:56 PDT
After giving it a try, this makes the code much more readable: - bool emphasisMarkExistsAndIsAbove(const RenderStyle&, std::optional<bool>& isAbove) const; + bool emphasisMarkExists(const RenderStyle&) const; + bool emphasisMarkIsAbove(const RenderStyle&) const; I also tried doing this: - bool emphasisMarkExistsAndIsAbove(const RenderStyle&) const; + std::optional<bool> emphasisMarkExistsAndIsAbove(const RenderStyle&) const; However, naming the variable that was the result of that method was hard, and the code that called the method read pretty poorly. It was also more difficult to reason about how the code in the original method should be broken up. Created attachment 343991 [details]
Patch v1
Comment on attachment 343991 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=343991&action=review > Source/WebCore/rendering/InlineFlowBox.cpp:1577 > + auto& textBox = downcast<InlineTextBox>(*child); > const RenderStyle& childLineStyle = child->lineStyle(); If you want me to change the second line to something like this, let me know: auto& childLineStyle = textBox->lineStyle(); Comment on attachment 343991 [details] Patch v1 Attachment 343991 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8393577 New failing tests: http/tests/preload/onload_event.html Created attachment 344008 [details]
Archive of layout-test-results from ews205 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 343991 [details]
Patch v1
Windows crash is unrelated.
Comment on attachment 343991 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=343991&action=review > Source/WebCore/rendering/InlineTextBox.cpp:390 > + ASSERT(emphasisMarkExists(style)); Since we are calling emphasisMarkExists() in the ASSERT statement and since it has the same assertions below ASSERT(!((emphasisPosition & TextEmphasisPosition::Over) && (emphasisPosition & TextEmphasisPosition::Under))); ASSERT(!((emphasisPosition & TextEmphasisPosition::Left) && (emphasisPosition & TextEmphasisPosition::Right))); I would suggest removing them from here and have them only in emphasisMarkExists(). > Source/WebCore/rendering/InlineTextBox.cpp:392 > + auto emphasisPosition = style.textEmphasisPosition(); I think it will help if you make these definitions: static const OptionSet<TextEmphasisPosition> emphasisPositionHorizontalMask { TextEmphasisPosition::Left, TextEmphasisPosition::Right }; auto emphasisPositionHorizontalValue = emphasisPosition & emphasisPositionHorizontalMask > Source/WebCore/rendering/InlineTextBox.cpp:394 > + ASSERT(!((emphasisPosition & TextEmphasisPosition::Left) && (emphasisPosition & TextEmphasisPosition::Right))); So this assertion can now be simplified like this: ASSERT(emphasisPositionHorizontalValue != emphasisPositionHorizontalMask); > Source/WebCore/rendering/InlineTextBox.cpp:396 > + if (emphasisPositionHasNeitherLeftNorRight(emphasisPosition)) And this can be: if (!emphasisPositionHorizontalValue) And I think you can get rid of emphasisPositionHasNeitherLeftNorRight() in this case. > Source/WebCore/rendering/InlineTextBox.cpp:402 > + return emphasisPosition.contains(TextEmphasisPosition::Right); And this can be: return emphasisPositionHorizontalValue == TextEmphasisPosition::Right; > Source/WebCore/rendering/InlineTextBox.cpp:1025 > - bool hasTextEmphasis = emphasisMarkExistsAndIsAbove(lineStyle, emphasisMarkAbove); > + bool hasTextEmphasis = emphasisMarkExists(lineStyle); Maybe you can get rid of the local variable hasTextEmphasis since it is referenced only once. Comment on attachment 343991 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=343991&action=review >> Source/WebCore/rendering/InlineFlowBox.cpp:1577 >> const RenderStyle& childLineStyle = child->lineStyle(); > > If you want me to change the second line to something like this, let me know: > > auto& childLineStyle = textBox->lineStyle(); Will change this here... > Source/WebCore/rendering/InlineFlowBox.cpp:1625 > const RenderStyle& childLineStyle = child->lineStyle(); ...and here. Created attachment 344024 [details]
Patch v2
Created attachment 344026 [details]
Patch v3
Comment on attachment 344026 [details]
Patch v3
The change makes the code more error prone since now we can (accidentally) call emphasisMarkIsAbove() even when there's no emphasis. it asserts in debug, but in release the result looks undefined to me. Currently all the call sites ensure that this won't ever happen, but nothing prevents me from adding a new call to emphasisMarkIsAbove() without checking emphasisMarkExists() first (and unless we already have a regression test case that would exercise this new codepath with the condition of not having emphasis mark, the code would just be landed and produce undefined behavior in release environment).
If this cleanup is believed to be an improvement, I think we should at least add emphasisMarkExists() check in emphasisMarkIsAbove() and turn the return bool into a 3state (maybe optional).
Comment on attachment 344026 [details] Patch v3 Attachment 344026 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8399569 New failing tests: http/tests/security/contentSecurityPolicy/video-with-https-url-allowed-by-csp-media-src-star.html http/tests/security/canvas-remote-read-remote-video-redirect.html http/tests/preload/onload_event.html Created attachment 344036 [details]
Archive of layout-test-results from ews206 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
(In reply to David Kilzer (:ddkilzer) from comment #1) > After giving it a try, this makes the code much more readable: > > - bool emphasisMarkExistsAndIsAbove(const RenderStyle&, > std::optional<bool>& isAbove) const; > + bool emphasisMarkExists(const RenderStyle&) const; > + bool emphasisMarkIsAbove(const RenderStyle&) const; > > I also tried doing this: > > - bool emphasisMarkExistsAndIsAbove(const RenderStyle&) const; > + std::optional<bool> emphasisMarkExistsAndIsAbove(const RenderStyle&) > const; > > However, naming the variable that was the result of that method was hard, > and the code that called the method read pretty poorly. It was also more > difficult to reason about how the code in the original method should be > broken up. After Zalan's Comment #11, I went back and retried the second suggestion above. This time, it made much more sense than splitting the function into two, and I still think the code is more readable than before. Created attachment 344047 [details]
Patch v4
Comment on attachment 344047 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=344047&action=review > Source/WebCore/rendering/InlineTextBox.cpp:369 > + std::optional<bool> isAbove; This can probably just be a `bool` since it's always set in the code path below. Opinions? Comment on attachment 344047 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=344047&action=review > Source/WebCore/ChangeLog:15 > + Refactor emphasisMarkExistsAndIsAbove() to return a > + std::optional<bool> instead of returning a bool and taking a > + std::optional<bool> argument. The state returned is now: > + - std::nullopt => emphasis mark doesn't exist or is suppressed. > + - false => emphasis mark exists and is not suppressed, but is not above. > + - true => emphasis mark exists and is not suppressed, and is above. Great economy in expressing only the three states we need to here. I’m not totally on board with representing these two booleans as std::optional<bool>. The idea that the optional is what expresses "exists and is not suppressed" and the bool is what expresses "is above or not" is a pretty subtle one. I think a good solution would be to use an enum for above/below and have this be an optional enum instead of an optional boolean. I suspect that if we did that the function might even be able to just be named emphasisMark, and not have the "exists" and "is above" in the name. But the enum would be slightly awkward when trying to compare with an isFlippedLinesWritingMode boolean so it wouldn’t be a lot better. > Source/WebCore/rendering/InlineTextBox.cpp:362 > + static NeverDestroyed<OptionSet<TextEmphasisPosition>> emphasisPositionHorizontalMask(std::initializer_list<TextEmphasisPosition>({ TextEmphasisPosition::Left, TextEmphasisPosition::Right })); This is just a constant integer value. There is no need to store it in a static. Even if we did store it in a static, there is no need to use NeverDestroyed, since there is no non-trivial destructor. And the explicit std::initializer_list here is particularly unfortunate. I am not sure why it’s needed, but probably due to the unnecessary NeverDestroyed. We should just write this: OptionSet<TextEmphasisPosition> horizontalMask { TextEmphasisPosition::Left, TextEmphasisPosition::Right }; Or maybe add a "const" if we think that will help clarify or even possibly generate slightly better code. I think the shorter name I am suggesting here is OK given how short this function is. No need to keep saying "emphasis" over and over again in the names of local variables. And maybe no need to say position either. But tastes may differ and you might want a longer name. I think we should take the word "emphasis" out of all the names of the local variables in this function. >> Source/WebCore/rendering/InlineTextBox.cpp:369 >> + std::optional<bool> isAbove; > > This can probably just be a `bool` since it's always set in the code path below. Opinions? Yes, I agree. Comment on attachment 344047 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=344047&action=review >> Source/WebCore/ChangeLog:15 >> + - true => emphasis mark exists and is not suppressed, and is above. > > Great economy in expressing only the three states we need to here. I’m not totally on board with representing these two booleans as std::optional<bool>. The idea that the optional is what expresses "exists and is not suppressed" and the bool is what expresses "is above or not" is a pretty subtle one. I think a good solution would be to use an enum for above/below and have this be an optional enum instead of an optional boolean. I suspect that if we did that the function might even be able to just be named emphasisMark, and not have the "exists" and "is above" in the name. > > But the enum would be slightly awkward when trying to compare with an isFlippedLinesWritingMode boolean so it wouldn’t be a lot better. I'm going to keep the std::optional<bool> for this patch since it makes the comparison with isFlippedLinesWritingMode easier. >> Source/WebCore/rendering/InlineTextBox.cpp:362 >> + static NeverDestroyed<OptionSet<TextEmphasisPosition>> emphasisPositionHorizontalMask(std::initializer_list<TextEmphasisPosition>({ TextEmphasisPosition::Left, TextEmphasisPosition::Right })); > > This is just a constant integer value. There is no need to store it in a static. Even if we did store it in a static, there is no need to use NeverDestroyed, since there is no non-trivial destructor. And the explicit std::initializer_list here is particularly unfortunate. I am not sure why it’s needed, but probably due to the unnecessary NeverDestroyed. We should just write this: > > OptionSet<TextEmphasisPosition> horizontalMask { TextEmphasisPosition::Left, TextEmphasisPosition::Right }; > > Or maybe add a "const" if we think that will help clarify or even possibly generate slightly better code. > > I think the shorter name I am suggesting here is OK given how short this function is. No need to keep saying "emphasis" over and over again in the names of local variables. And maybe no need to say position either. But tastes may differ and you might want a longer name. I think we should take the word "emphasis" out of all the names of the local variables in this function. Will fix before landing. >>> Source/WebCore/rendering/InlineTextBox.cpp:369 >>> + std::optional<bool> isAbove; >> >> This can probably just be a `bool` since it's always set in the code path below. Opinions? > > Yes, I agree. Will fix before landing. Committed r233441: <https://trac.webkit.org/changeset/233441> |