WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
Bug 187204
Refactor InlineTextBox::emphasisMarkExistsAndIsAbove()
https://bugs.webkit.org/show_bug.cgi?id=187204
Summary
Refactor InlineTextBox::emphasisMarkExistsAndIsAbove()
David Kilzer (:ddkilzer)
Reported
2018-06-29 16:37:56 PDT
In
Bug 186968 Comment #14
, Said About-Hallawa wrote: Comment on
attachment 343545
[details]
Patch v3 View in context:
https://bugs.webkit.org/attachment.cgi?id=343545&action=review
> Source/WebCore/rendering/InlineTextBox.cpp:361 > -bool InlineTextBox::emphasisMarkExistsAndIsAbove(const RenderStyle& style, bool& above) const > +bool InlineTextBox::emphasisMarkExistsAndIsAbove(const RenderStyle& style, std::optional<bool>& above) const
Here is a simpler approach. I think this function can be split into two functions: bool emphasisMarkExists(const RenderStyle& style) bool emphasisMarkIsAbove(const RenderStyle& style) I checked the code and I found all the callers to this function use the boolean 'above' only if emphasisMarkExistsAndIsAbove() returns true. So I do not think there a need to have the two functions combined into one function.
> Source/WebCore/rendering/InlineTextBox.cpp:1008 > + std::optional<bool> emphasisMarkAbove; > bool hasTextEmphasis = emphasisMarkExistsAndIsAbove(lineStyle, emphasisMarkAbove); > const AtomicString& emphasisMark = hasTextEmphasis ? lineStyle.textEmphasisMarkString() : nullAtom(); > if (!emphasisMark.isEmpty()) > - emphasisMarkOffset = emphasisMarkAbove ? -font.fontMetrics().ascent() - font.emphasisMarkDescent(emphasisMark) : font.fontMetrics().descent() + font.emphasisMarkAscent(emphasisMark); > + emphasisMarkOffset = (emphasisMarkAbove && *emphasisMarkAbove) ? -font.fontMetrics().ascent() - font.emphasisMarkDescent(emphasisMark) : font.fontMetrics().descent() + font.emphasisMarkAscent(emphasisMark);
This can be done using two functions like this: const AtomicString& emphasisMark = emphasisMarkExists(lineStyle) ? lineStyle.textEmphasisMarkString() : nullAtom(); if (!emphasisMark.isEmpty()) emphasisMarkOffset = emphasisMarkIsAbove(lineStyle) ? -font.fontMetrics().ascent() - font.emphasisMarkDescent(emphasisMark) : font.fontMetrics().descent() + font.emphasisMarkAscent(emphasisMark);
Attachments
Patch v1
(10.57 KB, patch)
2018-06-29 20:10 PDT
,
David Kilzer (:ddkilzer)
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews205 for win-future
(12.79 MB, application/zip)
2018-06-30 02:07 PDT
,
EWS Watchlist
no flags
Details
Patch v2
(11.35 KB, patch)
2018-06-30 15:30 PDT
,
David Kilzer (:ddkilzer)
no flags
Details
Formatted Diff
Diff
Patch v3
(11.38 KB, patch)
2018-06-30 15:46 PDT
,
David Kilzer (:ddkilzer)
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews206 for win-future
(12.77 MB, application/zip)
2018-06-30 19:36 PDT
,
EWS Watchlist
no flags
Details
Patch v4
(11.53 KB, patch)
2018-07-01 04:30 PDT
,
David Kilzer (:ddkilzer)
darin
: review+
ddkilzer
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
David Kilzer (:ddkilzer)
Comment 1
2018-06-29 16:40:38 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.
David Kilzer (:ddkilzer)
Comment 2
2018-06-29 20:10:15 PDT
Created
attachment 343991
[details]
Patch v1
David Kilzer (:ddkilzer)
Comment 3
2018-06-29 20:11:06 PDT
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();
EWS Watchlist
Comment 4
2018-06-30 02:06:53 PDT
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
EWS Watchlist
Comment 5
2018-06-30 02:07:04 PDT
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
David Kilzer (:ddkilzer)
Comment 6
2018-06-30 08:02:45 PDT
Comment on
attachment 343991
[details]
Patch v1 Windows crash is unrelated.
Said Abou-Hallawa
Comment 7
2018-06-30 11:10:00 PDT
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.
David Kilzer (:ddkilzer)
Comment 8
2018-06-30 15:27:00 PDT
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.
David Kilzer (:ddkilzer)
Comment 9
2018-06-30 15:30:46 PDT
Created
attachment 344024
[details]
Patch v2
David Kilzer (:ddkilzer)
Comment 10
2018-06-30 15:46:22 PDT
Created
attachment 344026
[details]
Patch v3
zalan
Comment 11
2018-06-30 16:19:55 PDT
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).
EWS Watchlist
Comment 12
2018-06-30 19:36:14 PDT
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
EWS Watchlist
Comment 13
2018-06-30 19:36:26 PDT
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
David Kilzer (:ddkilzer)
Comment 14
2018-07-01 04:28:27 PDT
(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.
David Kilzer (:ddkilzer)
Comment 15
2018-07-01 04:30:01 PDT
Created
attachment 344047
[details]
Patch v4
David Kilzer (:ddkilzer)
Comment 16
2018-07-01 04:35:35 PDT
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?
Darin Adler
Comment 17
2018-07-01 14:15:56 PDT
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.
David Kilzer (:ddkilzer)
Comment 18
2018-07-02 14:57:49 PDT
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.
David Kilzer (:ddkilzer)
Comment 19
2018-07-02 15:28:41 PDT
Committed
r233441
: <
https://trac.webkit.org/changeset/233441
>
Radar WebKit Bug Importer
Comment 20
2018-07-02 15:31:12 PDT
<
rdar://problem/41744398
>
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