Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1323513006:
Upstream ScriptRunIterator for segmenting text runs by script (Closed)
|
Created:
5 years, 3 months ago by drott Modified:
5 years, 3 months ago CC:
krit, drott+blinkwatch_chromium.org, Rik, dshwang, jbroman, Justin Novosad, danakj, pdr+graphicswatchlist_chromium.org, f(malita), Stephen Chennney, rwlbuis Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionUpstream ScriptRunIterator for segmenting text runs by script
ScriptRunIterator takes a pointer to a UTF-16 text run, and a starting
script value, then consume() can be called on it to retrieve the limit
and script value of the next segmented script run. It takes care of
matching brackets when resolving script runs.
This functionality is needed for changing HarfBuzzShaper.cpp so that
we do not need to pre-split and store the TextRun into HarfBuzzRuns.
We can improve our script segmentation and integrate script splitting
and shaping in one loop.
Original code written by Doug Felt, big thanks!
BUG=526095
R=eae,behdad
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201722
Patch Set 1 #
Total comments: 7
Patch Set 2 : Blink naming conventions, most review comments addressed #Patch Set 3 : Match format string to argument in error logging #Patch Set 4 : Fix size_t logging #Patch Set 5 : No support for constexpr on Win, unused variable fix for empty test #Patch Set 6 : Generate UTF-8 encoded symbols for strings on Windows #Patch Set 7 : Attempting to fix windows build by using explicit utf-8 literals #Patch Set 8 : Hex escapes seem to be the safest cross-platform bet #
Total comments: 2
Patch Set 9 : Fix Win link step, more verbose primaryScript comparison #Patch Set 10 : Clang build fix attempt #Patch Set 11 : Additional review comments addressed, new linkage attempt for kMaxScripts constant #
Messages
Total messages: 61 (26 generated)
This looks great! Thanks for doing this drott! https://codereview.chromium.org/1323513006/diff/1/Source/platform/fonts/Scrip... File Source/platform/fonts/ScriptRunIterator.cpp (right): https://codereview.chromium.org/1323513006/diff/1/Source/platform/fonts/Scrip... Source/platform/fonts/ScriptRunIterator.cpp:123: // The initial value of m_aheadCharacter is not used, use ffff to be distinctive. Why FFFF instead of 0? https://codereview.chromium.org/1323513006/diff/1/Source/platform/fonts/Scrip... Source/platform/fonts/ScriptRunIterator.cpp:134: m_currentSet.append(USCRIPT_COMMON); Why would we append commonn to the set here? https://codereview.chromium.org/1323513006/diff/1/Source/platform/fonts/Scrip... Source/platform/fonts/ScriptRunIterator.cpp:232: auto cur_it = m_currentSet.begin(); curIt, or better yet, currentScript https://codereview.chromium.org/1323513006/diff/1/Source/platform/fonts/Scrip... File Source/platform/fonts/ScriptRunIterator.h (right): https://codereview.chromium.org/1323513006/diff/1/Source/platform/fonts/Scrip... Source/platform/fonts/ScriptRunIterator.h:24: // This maintains a reference to data. It must exist for the lifetime of this Wrap at 80 col. https://codereview.chromium.org/1323513006/diff/1/Source/platform/fonts/Scrip... Source/platform/fonts/ScriptRunIterator.h:49: static const int kMaxBrackets = 32; Could you please add a comment explaining why we limit it to 32? https://codereview.chromium.org/1323513006/diff/1/Source/platform/fonts/Scrip... Source/platform/fonts/ScriptRunIterator.h:65: class PLATFORM_EXPORT ScriptData { What's the relationship between ScriptRunIterator and ScriptData? Does the iterator iterate over scripts in a ScriptData object? https://codereview.chromium.org/1323513006/diff/1/Source/platform/fonts/Scrip... File Source/platform/fonts/ScriptRunIteratorTest.cpp (right): https://codereview.chromium.org/1323513006/diff/1/Source/platform/fonts/Scrip... Source/platform/fonts/ScriptRunIteratorTest.cpp:36: class MockScriptData : public ScriptData { Do we really need to mock out the data object? It would be nice if we could use the actual implementation and real content instead of mocking out half of the logic. As it is this test is more of a test for the mock then the actual implementation.
On 2015/08/28 21:10:25, eae wrote: > This looks great! Thanks for doing this drott! Thanks for the review and positive feedback! > https://codereview.chromium.org/1323513006/diff/1/Source/platform/fonts/Scrip... > File Source/platform/fonts/ScriptRunIterator.cpp (right): > > https://codereview.chromium.org/1323513006/diff/1/Source/platform/fonts/Scrip... > Source/platform/fonts/ScriptRunIterator.cpp:123: // The initial value of > m_aheadCharacter is not used, use ffff to be distinctive. > Why FFFF instead of 0? No particular reason, perhaps easier debugging. Changed to 0. > https://codereview.chromium.org/1323513006/diff/1/Source/platform/fonts/Scrip... > Source/platform/fonts/ScriptRunIterator.cpp:134: > m_currentSet.append(USCRIPT_COMMON); > Why would we append commonn to the set here? We need a starting point for the m_currentSet. The core logic is that there are always two sets maintained while processing the input string, m_currentSet is the list of possible scripts for the current character, whereas the next set is the set of possible scripts for the next character we're looking at and trying to resolve with. The mergeSets method is responsible for finding the best intersection of the two, with special handling for "common" and "inherited". The first value of m_currentSet is assigned "common" before any characters are processed so that the actual first encountered character "wins" and m_currentSet is assigned from that. ScriptData makes sure that the sets have special ordering where "common" and "inherited" appear as first values, see below. > https://codereview.chromium.org/1323513006/diff/1/Source/platform/fonts/Scrip... > Source/platform/fonts/ScriptRunIterator.cpp:232: auto cur_it = > m_currentSet.begin(); > curIt, or better yet, currentScript Renamed the iterators in that function, but kept the "It" suffix, since there are direct iterator comparisons which I found a bit more readable when the suffix stays. There were a couple of other variables not named according to Blink conventions as well, fixed those. > https://codereview.chromium.org/1323513006/diff/1/Source/platform/fonts/Scrip... > File Source/platform/fonts/ScriptRunIterator.h (right): > > https://codereview.chromium.org/1323513006/diff/1/Source/platform/fonts/Scrip... > Source/platform/fonts/ScriptRunIterator.h:24: // This maintains a reference to > data. It must exist for the lifetime of this > Wrap at 80 col. Done, is that the rule for comments? > https://codereview.chromium.org/1323513006/diff/1/Source/platform/fonts/Scrip... > Source/platform/fonts/ScriptRunIterator.h:49: static const int kMaxBrackets = > 32; > Could you please add a comment explaining why we limit it to 32? Done, we limit to prevent excessive growth of the brackets stack when processing long texts. > https://codereview.chromium.org/1323513006/diff/1/Source/platform/fonts/Scrip... > Source/platform/fonts/ScriptRunIterator.h:65: class PLATFORM_EXPORT ScriptData { > What's the relationship between ScriptRunIterator and ScriptData? Does the > iterator iterate over scripts in a ScriptData object? ScriptRunIterator iterates over the input text, script run by script run. ScriptData is a wrapper the returns a set of scripts for a particular character - retrieved from the characters primary script and script extensions, as per ICU / Unicode data. ScriptData maintains a certain priority order of the returned values, which are essential for mergeSets method to work correctly. > https://codereview.chromium.org/1323513006/diff/1/Source/platform/fonts/Scrip... > File Source/platform/fonts/ScriptRunIteratorTest.cpp (right): > > https://codereview.chromium.org/1323513006/diff/1/Source/platform/fonts/Scrip... > Source/platform/fonts/ScriptRunIteratorTest.cpp:36: class MockScriptData : > public ScriptData { > Do we really need to mock out the data object? It would be nice if we could use > the actual implementation and real content instead of mocking out half of the > logic. As it is this test is more of a test for the mock then the actual > implementation. Yes, there are a couple of CHECK_MOCK_RUNS tests which use the MockScriptData provider, and a couple of CHECK_RUNS that work with actual values. I agree it would be best to replace the CHECK_MOCK_RUNS cases with real world data or actual test cases or transform them by finding suitable real world codepoints matching the test mock runs. Since this is potentially quite time consuming work on the test cases - I'd leave it up to you whether this should be done before landing this addition or whether we can do that separately.
The CQ bit was checked by drott@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1323513006/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1323513006/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by drott@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1323513006/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1323513006/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by drott@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1323513006/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1323513006/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #6 (id:100001) has been deleted
The CQ bit was checked by drott@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1323513006/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1323513006/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by drott@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1323513006/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1323513006/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Hex escapes seem to be the safest cross-platform bet
The CQ bit was checked by drott@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1323513006/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1323513006/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
behdad@google.com changed reviewers: + behdad@google.com
lgtm Thanks Dominik. This looks great. We should, later on, port this to C and make a standalone implementation of the algorithm, and just copy into Blink. https://codereview.chromium.org/1323513006/diff/160001/Source/platform/fonts/... File Source/platform/fonts/ScriptRunIterator.cpp (right): https://codereview.chromium.org/1323513006/diff/160001/Source/platform/fonts/... Source/platform/fonts/ScriptRunIterator.cpp:60: if (primaryScript > USCRIPT_INHERITED) { This relies on order of USCRIPT values. Can you rewrite more verbosely please? https://codereview.chromium.org/1323513006/diff/160001/Source/platform/fonts/... Source/platform/fonts/ScriptRunIterator.cpp:83: if (dst.at(0) == USCRIPT_LATIN || dst.at(i) < dst.at(0)) { Note to self. This algorithm uses the order that scripts were encoded... so the USCRIPT order is of significance... When porting this algorithm to use HarfBuzz script for outside uses I have to figure out how to do something like that...
The CQ bit was checked by drott@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1323513006/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1323513006/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Clang build fix attempt
The CQ bit was checked by drott@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1323513006/190001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1323513006/190001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Address inconsistent DLL linkage warning
The CQ bit was checked by drott@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1323513006/210001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1323513006/210001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #11 (id:210001) has been deleted
drott@chromium.org changed reviewers: + brettw@chromium.org, dpranke@google.com
brettw@, dpranke@: I'm really struggling getting this to build an link everywhere - would you have a piece of advice on what I could do with the kMaxScriptCount constant? Thanks very much in advance.
When you put the assignment of the constant in the header, you're not actually giving the variable a memory address. This is OK as long as the compiler can inline each use. But when you mark the class as exported, you're declaring that external .dlls can read those variables and hence it has to have a memory address. I think here the compiler is telling you it's getting confused by these two things. Try deleting the = 20 from the header and adding to the .cpp file: const int ScriptData::kMaxScriptCount = 20; In the test you seem to be doing the above. Was this to fix a compiler error? The gtest macros may be taking the address of it which will cause this issue, but giving some random class' constants storage in a test file is definitely wrong. I think when you do the above you can delete the one in the test file.
On 2015/09/01 17:14:20, brettw wrote: > When you put the assignment of the constant in the header, you're not actually > giving the variable a memory address. This is OK as long as the compiler can > inline each use. But when you mark the class as exported, you're declaring that > external .dlls can read those variables and hence it has to have a memory > address. I think here the compiler is telling you it's getting confused by these > two things. > > Try deleting the = 20 from the header and adding to the .cpp file: > const int ScriptData::kMaxScriptCount = 20; > > In the test you seem to be doing the above. Was this to fix a compiler error? > The gtest macros may be taking the address of it which will cause this issue, > but giving some random class' constants storage in a test file is definitely > wrong. I think when you do the above you can delete the one in the test file. Or just make the const static inline?
Additional review comments addressed, new linkage attempt for kMaxScripts constant
The CQ bit was checked by drott@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1323513006/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1323513006/230001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/09/01 17:14:20, brettw wrote: > When you put the assignment of the constant in the header, you're not actually > giving the variable a memory address. This is OK as long as the compiler can > inline each use. But when you mark the class as exported, you're declaring that > external .dlls can read those variables and hence it has to have a memory > address. I think here the compiler is telling you it's getting confused by these > two things. Thanks very much for explaining what's going wrong here, this helped a lot. > Try deleting the = 20 from the header and adding to the .cpp file: > const int ScriptData::kMaxScriptCount = 20; > > In the test you seem to be doing the above. Was this to fix a compiler error? Yes. > The gtest macros may be taking the address of it which will cause this issue, > but giving some random class' constants storage in a test file is definitely > wrong. I think when you do the above you can delete the one in the test file. Your suggestions worked, all green, great! Thanks indeed.
LGTM
The CQ bit was checked by drott@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from behdad@google.com Link to the patchset: https://codereview.chromium.org/1323513006/#ps230001 (title: "Additional review comments addressed, new linkage attempt for kMaxScripts constant")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1323513006/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1323513006/230001
Message was sent while issue was closed.
Committed patchset #11 (id:230001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201722 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
