Add tab characters support in complex path

Add tab characters support in complex path

This patch adds Tab characters support in complex path.

Tab characters have different advances depends on the tab-size CSS
property and the current position, which is not suitable for Harfbuzz
to handle. This patch lets CachingWordShapeIterator to split tab
characters into separate runs before passing to HarffBuzzShaper.

Two layout tests are copies of existing tests with "text-rendering:
optimizeLegibility" added. These tests can be removed when simple path
is unified with complex path (crbug.com/404597).

BUG=422670
TEST=css3/tab-size-complex-path.html
TEST=fast/css/tab-size-complex-path.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200331

[kojii, 2015-07-20 07:41:18]

kojii@chromium.org changed reviewers:
+ drott@chromium.org, eae@chromium.org

[kojii, 2015-07-20 07:41:19]

PTAL.

The diff gets a little larger than expected.

The other possibility is to split tabs out before we call
HarfBuzzShaper::shapeResult(), such as in CachingWordShapeIterator, or even
higher in the layer. HarfBuzzShaper::shapeResult() currently has two callers:
CachingWordShapeIterator and Font::offsetForPositionForComplexText(), so if
we're going that route, we'll need to modify both, or change
Font::offsetForPositionForComplexText() to also use CachingWordShapeIterator and
we never use HarfBuzzShaper directly.

Alternatively, I'm fine to land this for now, and refine after we removed the
simple shaper.

WDYT?

[eae, 2015-07-20 17:39:24]

Seems reasonable but is a little weird given that tabs are handled outside of
harfbuzz.
Perhaps it would make more sense to change offsetForPositionForComplexText to
use the cachingwordshaper and then implement this in CachingWordShapeIterator
instead?

https://codereview.chromium.org/1242213002/diff/80001/Source/platform/fonts/s...
File Source/platform/fonts/shaping/HarfBuzzShaper.cpp (right):

https://codereview.chromium.org/1242213002/diff/80001/Source/platform/fonts/s...
Source/platform/fonts/shaping/HarfBuzzShaper.cpp:751: static inline bool
updateCharacterInfo(const Font* font, UChar32 character, const TextRun& textRun,
Update makes it sound like it has state, how about something like this:

struct RunGlypInfo {
  const SimpleFontData* fontData;
  UScriptCode script;
}

RunGlypInfo glyphInfoForChar(cont char* font, UChar32 character, TextRun&
textRun)
{
...
}

https://codereview.chromium.org/1242213002/diff/80001/Source/platform/fonts/s...
Source/platform/fonts/shaping/HarfBuzzShaper.cpp:780: if (character ==
tabulationCharacter && textRun.allowTabs()) {
How about adding UNLIKELY macros around the tab checks to prime to branch
predictor?

if (UNLIKELY(character == tabulationCharacter && textRun.allowTabs())) {

https://codereview.chromium.org/1242213002/diff/80001/Source/platform/fonts/s...
Source/platform/fonts/shaping/HarfBuzzShaper.cpp:796: if (character ==
tabulationCharacter && textRun.allowTabs())
I'd move this check out of the treatAsZeroWidthSpace block,

if (character == tabulationCharacter && textRun.allowTabs()) {
...
} else if (Character::treatAsZeroWidthSpace(character)) {
...
}

https://codereview.chromium.org/1242213002/diff/80001/Source/platform/fonts/s...
Source/platform/fonts/shaping/HarfBuzzShaper.cpp:943: if (run.character ==
tabulationCharacter && m_textRun.allowTabs()) {
Is this really needed? Isn't the extra condition in the loop below enough to
keep the tab runs separate?

[kojii, 2015-07-21 01:11:48]

On 2015/07/20 17:39:24, eae wrote:
> Seems reasonable but is a little weird given that tabs are handled outside of
> harfbuzz.
> Perhaps it would make more sense to change offsetForPositionForComplexText to
> use the cachingwordshaper and then implement this in CachingWordShapeIterator
> instead?

Thank you for the feedback, agreed, I'll try that route. Possible downside of
the route are:
* CachingWordShapeIterator must build a new ShapeResult by itself (not only
runs).
* HarffBuzzShaper not being able to use without caching looks a bit weird. Maybe
just naming.

I'll try that patch, and appreciate your comments which looks more reasonable.

And thank you for your comments to code for the patch we might not use, learned
a lot!!

https://codereview.chromium.org/1242213002/diff/80001/Source/platform/fonts/s...
File Source/platform/fonts/shaping/HarfBuzzShaper.cpp (right):

https://codereview.chromium.org/1242213002/diff/80001/Source/platform/fonts/s...
Source/platform/fonts/shaping/HarfBuzzShaper.cpp:751: static inline bool
updateCharacterInfo(const Font* font, UChar32 character, const TextRun& textRun,
On 2015/07/20 17:39:24, eae wrote:
> Update makes it sound like it has state, how about something like this:
> 
> struct RunGlypInfo {
>   const SimpleFontData* fontData;
>   UScriptCode script;
> }
> 
> RunGlypInfo glyphInfoForChar(cont char* font, UChar32 character, TextRun&
> textRun)
> {
> ...
> }

Glad you suggested this! I once did this but it made the patch larger, and my
mentor told me to make patch smaller. I'll prioritize cleaner code than smaller
patch in future ;-)

https://codereview.chromium.org/1242213002/diff/80001/Source/platform/fonts/s...
Source/platform/fonts/shaping/HarfBuzzShaper.cpp:780: if (character ==
tabulationCharacter && textRun.allowTabs()) {
On 2015/07/20 17:39:24, eae wrote:
> How about adding UNLIKELY macros around the tab checks to prime to branch
> predictor?
> 
> if (UNLIKELY(character == tabulationCharacter && textRun.allowTabs())) {

Thought UNLIKELY() is the past. Thanks, learned one more thing.

https://codereview.chromium.org/1242213002/diff/80001/Source/platform/fonts/s...
Source/platform/fonts/shaping/HarfBuzzShaper.cpp:796: if (character ==
tabulationCharacter && textRun.allowTabs())
On 2015/07/20 17:39:24, eae wrote:
> I'd move this check out of the treatAsZeroWidthSpace block,
> 
> if (character == tabulationCharacter && textRun.allowTabs()) {
> ...
> } else if (Character::treatAsZeroWidthSpace(character)) {
> ...
> }

That's better to read. treatAsZeroWidthSpace() checks tab, so we check tab twice
then. It's rare code so I tried less CPU cycles but I'm fine either way.

https://codereview.chromium.org/1242213002/diff/80001/Source/platform/fonts/s...
Source/platform/fonts/shaping/HarfBuzzShaper.cpp:943: if (run.character ==
tabulationCharacter && m_textRun.allowTabs()) {
On 2015/07/20 17:39:24, eae wrote:
> Is this really needed? Isn't the extra condition in the loop below enough to
> keep the tab runs separate?

Yeah, this check is needed if the first run is tab, and the second is not, and
the second has the same font and USCRIPT_COMMON; e.g., "\t//".

Maybe I could use USCRIPT_INVALID or something some sentry-like value for tab
runs.

[eae, 2015-07-21 01:59:12]

On Mon, Jul 20, 2015 at 6:11 PM,  <kojii@chromium.org> wrote:
> Thank you for the feedback, agreed, I'll try that route. Possible downside
> of
> the route are:
> * CachingWordShapeIterator must build a new ShapeResult by itself (not only
> runs).
> * HarffBuzzShaper not being able to use without caching looks a bit weird.
> Maybe just naming.

True. SegmentingShaper or WordShaper might be better names? We should
probably do the rename as a separate pass if we decide to go down that
route.

> I'll try that patch, and appreciate your comments which looks more
> reasonable.

Thanks, if it turns out to be too much work or not a good idea then
this approach (modifying harfbuzzshaper) is entirely personable and
somehting we can go back to.


> Glad you suggested this! I once did this but it made the patch larger,
> and my mentor told me to make patch smaller. I'll prioritize cleaner
> code than smaller patch in future ;-)

:) - it's all about tradeoffs, for this particular code I'd optimize
for clarity as it is hard enough to understand already!

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[kojii, 2015-07-23 05:57:09]

Patchset #10 (id:180001) has been deleted

[kojii, 2015-07-23 05:58:46]

Patchset #9 (id:160001) has been deleted

[kojii, 2015-07-31 04:16:44]

PTAL. Moved to CachingWordShapeIterator, and this looks promising.

Notes on landing:
* This CL is likely to conflict with fallback fix but resolving conflicts should
not change much and I can handle that after I'm back.
https://codereview.chromium.org/1244973003
* Font::offsetForPositionForComplexText should also be done to complete the
work, but I do not think the order of landing matters.
https://codereview.chromium.org/1248453004

[kojii, 2015-08-10 02:39:52]

Rebase

[kojii, 2015-08-10 14:08:32]

PTAL.

[eae, 2015-08-10 17:24:08]

Nice!

https://codereview.chromium.org/1242213002/diff/240001/Source/platform/fonts/...
File Source/platform/fonts/shaping/CachingWordShapeIterator.h (right):

https://codereview.chromium.org/1242213002/diff/240001/Source/platform/fonts/...
Source/platform/fonts/shaping/CachingWordShapeIterator.h:83: return true;
Shouldn't this be "return *wordResult != nullptr;"?

https://codereview.chromium.org/1242213002/diff/240001/Source/platform/fonts/...
Source/platform/fonts/shaping/CachingWordShapeIterator.h:120: }
Missing "return nullptr"?

https://codereview.chromium.org/1242213002/diff/240001/Source/platform/fonts/...
Source/platform/fonts/shaping/CachingWordShapeIterator.h:139: *wordResult =
shapeWordUntilCharacter(tabulationCharacter);
Shouldn't this still segment on space? I.e. shape until either a space or a
tabulation character is encountered?

[kojii, 2015-08-11 07:22:21]

PTAL. Please see inline for more:

https://codereview.chromium.org/1242213002/diff/240001/Source/platform/fonts/...
File Source/platform/fonts/shaping/CachingWordShapeIterator.h (right):

https://codereview.chromium.org/1242213002/diff/240001/Source/platform/fonts/...
Source/platform/fonts/shaping/CachingWordShapeIterator.h:83: return true;
On 2015/08/10 17:24:07, eae wrote:
> Shouldn't this be "return *wordResult != nullptr;"?

Done. The fix was moved to nextUntilCharacterOrTab() below so that compiler can
generate jmp instead of call.

Also applied the same fix for spaceCharacter case (moved to nextWord() below.)
These fixes change the behavior of next() slightly, I hope to be more correct.

https://codereview.chromium.org/1242213002/diff/240001/Source/platform/fonts/...
Source/platform/fonts/shaping/CachingWordShapeIterator.h:120: }
On 2015/08/10 17:24:07, eae wrote:
> Missing "return nullptr"?

The loop exits only with "return", so it's not necessary. I added
ASSERT_NOT_REACHED() for clarification, but compiler claimed that the code is
not reachable.

https://codereview.chromium.org/1242213002/diff/240001/Source/platform/fonts/...
Source/platform/fonts/shaping/CachingWordShapeIterator.h:139: *wordResult =
shapeWordUntilCharacter(tabulationCharacter);
On 2015/08/10 17:24:07, eae wrote:
> Shouldn't this still segment on space? I.e. shape until either a space or a
> tabulation character is encountered?

Good point, we need to do that if we want to cache words in allowTabs(). I
wasn't trying to do so, but after trying, it wasn't as complicated as thought.

PS13 adds this capability. Now we need to check allowTabs() on every next()
call. Is this ok? If you don't think the cost is worth the value, I can revert
to PS12 (which reflects your other comments.)

It turns out that <select> implies allowTabs(), so I needed to rebaseline for
sub-pixel changes caused by the segmenting. Image diff below:
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...

[eae, 2015-08-11 16:32:21]

LGTM

I think this is fine. If there is a performance regression we could always
templatize it.

[eae, 2015-08-11 16:32:27]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2015-08-11 16:32:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1242213002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1242213002/300001

[commit-bot: I haz the power, 2015-08-11 16:39:05]

Committed patchset #14 (id:300001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200331

[ebraminio, 2015-08-11 17:50:09]

Thank you for fixing this :)

> Good point, we need to do that if we want to cache words in allowTabs(). I
> wasn't trying to do so, but after trying, it wasn't as complicated as thought.
Is it possible that would be filed as a bug? Currently editing, specially Arabic
script on Android is slowwww and <textarea> is an allowTabs AFAIK.

[kojii, 2015-08-12 00:50:23]

On 2015/08/11 17:50:09, ebraminio wrote:
> Thank you for fixing this :)

Thank you for the words ;)

> > Good point, we need to do that if we want to cache words in allowTabs(). I
> > wasn't trying to do so, but after trying, it wasn't as complicated as
thought.
> Is it possible that would be filed as a bug? Currently editing, specially
Arabic
> script on Android is slowwww and <textarea> is an allowTabs AFAIK.

The landed patch does enable the word cache for allowTabs, so it should be as
fast as !allowTabs.

Please feel free to file a bug if it's still slow after this patch is in the
channel you're using. Blink does a lot more than line layout when editing; e.g.,
carets and selection has a lot of direction-specific code, so the cause could be
in somewhere else.

[ebraminio, 2015-08-12 07:16:50]

> The landed patch does enable the word cache for allowTabs, so it should be as
> fast as !allowTabs.

Great!

> Please feel free to file a bug if it's still slow after this patch is in the
> channel you're using. Blink does a lot more than line layout when editing;
e.g.,
> carets and selection has a lot of direction-specific code, so the cause could
be
> in somewhere else.

As an user my testing is very subjective but I feel Arabic text editing
performance on Android is already boosted with this on new Chromium for Android
version now I am testing so no worries anymore, thank you :)

[cbiesinger, 2015-08-13 21:11:16]

I think this is causing crashes on Mac:

Assertion failed: (bool (status_or & kCTRunStatusRightToLeft) == backward),
function _hb_coretext_shape, file
../../third_party/harfbuzz-ng/src/hb-coretext.cc, line 1040.


https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Mac10_6__...

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.6%20%28d...

[ebraminio, 2015-08-13 21:15:12]

On 2015/08/13 21:11:16, cbiesinger wrote:
> I think this is causing crashes on Mac:
> 
> Assertion failed: (bool (status_or & kCTRunStatusRightToLeft) == backward),
> function _hb_coretext_shape, file
> ../../third_party/harfbuzz-ng/src/hb-coretext.cc, line 1040.
> 
> 
>
https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Mac10_6__...
> 
>
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.6%20%28d...
Mac 10.6 issue is already known
https://code.google.com/p/chromium/issues/detail?id=419769 and not
related/specific to this

[cbiesinger, 2015-08-13 21:15:46]

Filed crbug.com/520685 on that

[ebraminio, 2015-08-13 21:17:48]

> not related/specific to this

AFAIK

[kojii, 2015-08-14 02:25:47]

On 2015/08/13 at 21:17:48, ebraminio wrote:
> > not related/specific to this
> 
> AFAIK

Thank you cbiesinger and ebraminio, yeah, I've seen this assertion failure in
other bugs. I'll have a look.