Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2322403002:
SkPDF: Implement /ActualText to make text extraction correct. (Closed)
|
Created:
4 years, 3 months ago by hal.canary Modified:
4 years, 3 months ago CC:
reviews_skia.org, RikC Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionSkPDF: Implement /ActualText to make text extraction correct.
For old API: no change.
For new API: LTR text is perfectly extracted, RTL needs better testing.
CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-Clang-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot,Test-Ubuntu-Clang-GCE-CPU-AVX2-x86_64-Debug-MSAN-Trybot
BUG=skia:5434
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2322403002
Committed: https://skia.googlesource.com/skia/+/f59d18a4ea5ad56975502faf50eae43418ea9c2a
Patch Set 1 #Patch Set 2 : rebased #Patch Set 3 : make internalDrawText 182 lines #Patch Set 4 : comments, fix win build #Patch Set 5 : one more win fix #Patch Set 6 : ReversedChars #
Total comments: 2
Patch Set 7 : fix Reversed #Patch Set 8 : SkPDFUtils::WriteUTF16beHex #
Total comments: 12
Patch Set 9 : comments from bungeman@ #
Total comments: 24
Patch Set 10 : comments from tomhudson@ #Patch Set 11 : rebase #Patch Set 12 : asserts, bounds check before read, not after #
Messages
Total messages: 65 (48 generated)
Description was changed from ========== SkPDF: Implement /ActualText to make text extraction correct. BUG=skia:5434 ========== to ========== SkPDF: Implement /ActualText to make text extraction correct. BUG=skia:5434 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2322403002 ==========
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:20001) has been deleted
Description was changed from ========== SkPDF: Implement /ActualText to make text extraction correct. BUG=skia:5434 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2322403002 ========== to ========== SkPDF: Implement /ActualText to make text extraction correct. For old API: no change. For new API: LTR text is perfectly extracted, RTL needs better testing. BUG=skia:5434 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2322403002 ==========
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
halcanary@google.com changed reviewers: + bungeman@google.com, tomhudson@google.com
PTAL.
internalDrawText() is now 250 lines long with lots of deep conditional structures. Can we break any of those out into meaningful independent functions, or is there too much data being passed through?
On 2016/09/12 19:28:56, tomhudson wrote: > internalDrawText() is now 250 lines long with lots of deep conditional > structures. Can we break any of those out into meaningful independent functions, yes. > or is there too much data being passed through? this is why it's non-trvivial.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/12 19:30:56, Hal Canary wrote: > On 2016/09/12 19:28:56, tomhudson wrote: > > internalDrawText() is now 250 lines long with lots of deep conditional > > structures. Can we break any of those out into meaningful independent > functions, > > yes. > take a look at patch set 3.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by halcanary@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2322403002/diff/120001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/2322403002/diff/120001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:1164: // TODO: move into SkPDFUtils.h Any reason not to do this now?
On 2016/09/13 18:28:25, bungeman-skia wrote: > https://codereview.chromium.org/2322403002/diff/120001/src/pdf/SkPDFDevice.cpp > File src/pdf/SkPDFDevice.cpp (right): > > https://codereview.chromium.org/2322403002/diff/120001/src/pdf/SkPDFDevice.cp... > src/pdf/SkPDFDevice.cpp:1164: // TODO: move into SkPDFUtils.h > Any reason not to do this now? I was going to say "to not make this CL any bigger", but it looks like Hal has neatly precluded that argument.
Mostly nits. If it works, lgtm. https://codereview.chromium.org/2322403002/diff/160001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/2322403002/diff/160001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:929: // harfbuzz), iterate over the clusters. nit: this is a doc comment (not an implementation comment) so should be /* ... */ https://codereview.chromium.org/2322403002/diff/160001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:932: Clusterator(int glyphCount = 0) Should be marked explicit. Is there a good reason to make the parameter optional? https://codereview.chromium.org/2322403002/diff/160001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:934: , fGlyphCount(SkToU32(glyphCount)) Seems strange to take an int for glyphCount and then cast it to store it. https://codereview.chromium.org/2322403002/diff/160001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:1046: // (via primitive shaping), while preserving glyph-to-character nit: doc comment https://codereview.chromium.org/2322403002/diff/160001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:1299: SK_AT_SCOPE_EXIT(if (actualText) { glyphPositioner.flush(); out->writeText("EMC\n"); } ); I really wish there were a better way to structure this, but I can't think of anything at the moment. https://codereview.chromium.org/2322403002/diff/160001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:1308: unichar != map_glyph(glyphToUnicode, glyphs[index])) { // test single Unichar map The '{' being at the end of this line makes it difficult to see where the end of the conditional is, especially since this line is indented the same amount as the lines below.
https://codereview.chromium.org/2322403002/diff/120001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/2322403002/diff/120001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:1164: // TODO: move into SkPDFUtils.h On 2016/09/13 18:28:24, bungeman-skia wrote: > Any reason not to do this now? Only an effort to keep this CL small. Fixed in patchset 8. https://codereview.chromium.org/2322403002/diff/160001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/2322403002/diff/160001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:929: // harfbuzz), iterate over the clusters. On 2016/09/13 22:08:05, bungeman-skia wrote: > nit: this is a doc comment (not an implementation comment) so should be /* ... > */ Done. https://codereview.chromium.org/2322403002/diff/160001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:932: Clusterator(int glyphCount = 0) On 2016/09/13 22:08:05, bungeman-skia wrote: > Should be marked explicit. Is there a good reason to make the parameter > optional? Done. I also want an "empty Clusterator" constructor https://codereview.chromium.org/2322403002/diff/160001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:934: , fGlyphCount(SkToU32(glyphCount)) On 2016/09/13 22:08:05, bungeman-skia wrote: > Seems strange to take an int for glyphCount and then cast it to store it. Done. https://codereview.chromium.org/2322403002/diff/160001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:1046: // (via primitive shaping), while preserving glyph-to-character On 2016/09/13 22:08:05, bungeman-skia wrote: > nit: doc comment Done. https://codereview.chromium.org/2322403002/diff/160001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:1299: SK_AT_SCOPE_EXIT(if (actualText) { glyphPositioner.flush(); out->writeText("EMC\n"); } ); On 2016/09/13 22:08:05, bungeman-skia wrote: > I really wish there were a better way to structure this, but I can't think of > anything at the moment. Acknowledged. https://codereview.chromium.org/2322403002/diff/160001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:1308: unichar != map_glyph(glyphToUnicode, glyphs[index])) { // test single Unichar map On 2016/09/13 22:08:05, bungeman-skia wrote: > The '{' being at the end of this line makes it difficult to see where the end of > the conditional is, especially since this line is indented the same amount as > the lines below. Done.
The CQ bit was checked by bungeman@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2322403002/diff/180001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/2322403002/diff/180001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:947: // work for clusters produced by HarfBuzz This is surprising? What does harfbuzz output that is !fReversedChars look like? How do we test? https://codereview.chromium.org/2322403002/diff/180001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:951: struct Cluster { Nit: Clusterator constructor parameters are nearly the same as Cluster fields, but read in a different order? https://codereview.chromium.org/2322403002/diff/180001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:958: bool reversedChars() const { return fReversedChars; } What's the *meaning* of this bool? Is it context that anybody reading this code will have? https://codereview.chromium.org/2322403002/diff/180001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:963: fGlyphCount = 0; You want to do this test before you check for empty? It sounds seems like if you iterate calling next() you can get stuck repeatedly returning this sentinel rather than the empty cluster? https://codereview.chromium.org/2322403002/diff/180001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:1001: if (nextCluster > cluster) { // LTR text If this is the test for LTR vs RTL, what's the meaning of fReversedChars? https://codereview.chromium.org/2322403002/diff/180001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:1049: static Clusterator make_clusterator( nit, for consistency: why not put this in the anonymous namespace like all the above, instead of declaring it static? Both are valid approaches to the same end, but do we need to use both back-to-back in new code in the same file? https://codereview.chromium.org/2322403002/diff/180001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:1090: clusters[clusterIndex++] = SkToU32(txtPtr - utf8Text); You're storing a 32-bit cast of a pointer math result in the clusters array? So it's an array of offsets? Nit: can we name it clusterOffsets? clusterIndices? Or put in a snippet of documentation that it represents offsets in the fUtf8textStorage array it's bundled together with? https://codereview.chromium.org/2322403002/diff/180001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:1226: // are passed to drawText() (most clients pass glyphs or a textblob). So what *is* the use case where no glyphs are passed to drawText() and we still care? https://codereview.chromium.org/2322403002/diff/180001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:1282: const SkGlyphID maxGlyphID = metrics->fLastGlyphID; This should be the same as glyphToUnicode.count()? Or is this the range-of-glyphIDs-we-accept-as-legal-but-will-return -1 from-map_glyph()-for? https://codereview.chromium.org/2322403002/diff/180001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:1295: while (Clusterator::Cluster c = clusterator.next()) { Ugh - this looks like a nicely-separable chunk of code, but I guess it does need 6 or 7 inputs + |this|, so isn't a good candidate for pulling out. :(
The CQ bit was checked by halcanary@google.com to run a CQ dry run
https://codereview.chromium.org/2322403002/diff/180001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/2322403002/diff/180001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:947: // work for clusters produced by HarfBuzz On 2016/09/14 16:15:03, tomhudson wrote: > This is surprising? What does harfbuzz output that is !fReversedChars look like? > How do we test? Done. // This is a cheap heuristic for /ReversedChars which seems to // work for clusters produced by HarfBuzz, which either // increase from zero (LTR) or decrease to zero (RTL). // "ReversedChars" is how PDF deals with RTL text. https://codereview.chromium.org/2322403002/diff/180001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:951: struct Cluster { On 2016/09/14 16:15:03, tomhudson wrote: > Nit: Clusterator constructor parameters are nearly the same as Cluster fields, > but read in a different order? Fixed. It is now in the same order, and I packed it correctly. https://codereview.chromium.org/2322403002/diff/180001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:958: bool reversedChars() const { return fReversedChars; } On 2016/09/14 16:15:02, tomhudson wrote: > What's the *meaning* of this bool? Is it context that anybody reading this code > will have? Done. // True if this looks like right-to-left text. https://codereview.chromium.org/2322403002/diff/180001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:963: fGlyphCount = 0; On 2016/09/14 16:15:03, tomhudson wrote: > You want to do this test before you check for empty? It sounds seems like if you > iterate calling next() you can get stuck repeatedly returning this sentinel > rather than the empty cluster? It works either way (check it yourself). Done https://codereview.chromium.org/2322403002/diff/180001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:1001: if (nextCluster > cluster) { // LTR text On 2016/09/14 16:15:03, tomhudson wrote: > If this is the test for LTR vs RTL, what's the meaning of fReversedChars? I wrote this before fReversedChars. It still works. https://codereview.chromium.org/2322403002/diff/180001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:1049: static Clusterator make_clusterator( On 2016/09/14 16:15:03, tomhudson wrote: > nit, for consistency: why not put this in the anonymous namespace like all the > above, instead of declaring it static? Both are valid approaches to the same > end, but do we need to use both back-to-back in new code in the same file? Skia style seems to prefer static functions over functions in anonymous namespaces. https://codereview.chromium.org/2322403002/diff/180001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:1090: clusters[clusterIndex++] = SkToU32(txtPtr - utf8Text); On 2016/09/14 16:15:02, tomhudson wrote: > You're storing a 32-bit cast of a pointer math result in the clusters array? > So it's an array of offsets? > > Nit: can we name it clusterOffsets? clusterIndices? Or put in a snippet of > documentation that it represents offsets in the fUtf8textStorage array it's > bundled together with? // The clusters[] array is an array of offsets into utf8Text[], // one offset for each glyph. See SkTextBlobBuilder for more info. https://codereview.chromium.org/2322403002/diff/180001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:1226: // are passed to drawText() (most clients pass glyphs or a textblob). On 2016/09/14 16:15:03, tomhudson wrote: > So what *is* the use case where no glyphs are passed to drawText() and we still > care? SkPaint::TextEncoding has been a part of our API forever. I want to support it correctly until someone officially deprecates it. https://codereview.chromium.org/2322403002/diff/180001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:1282: const SkGlyphID maxGlyphID = metrics->fLastGlyphID; On 2016/09/14 16:15:02, tomhudson wrote: > This should be the same as glyphToUnicode.count()? Or is this the > range-of-glyphIDs-we-accept-as-legal-but-will-return -1 from-map_glyph()-for? fGlyphToUnicode is not guaranteed to be populated. https://codereview.chromium.org/2322403002/diff/180001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:1295: while (Clusterator::Cluster c = clusterator.next()) { On 2016/09/14 16:15:02, tomhudson wrote: > Ugh - this looks like a nicely-separable chunk of code, but I guess it does need > 6 or 7 inputs + |this|, so isn't a good candidate for pulling out. :( Acknowledged.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2322403002/diff/180001/src/pdf/SkPDFDevice.cpp File src/pdf/SkPDFDevice.cpp (right): https://codereview.chromium.org/2322403002/diff/180001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:963: fGlyphCount = 0; On 2016/09/15 20:27:17, Hal Canary wrote: > On 2016/09/14 16:15:03, tomhudson wrote: > > You want to do this test before you check for empty? It sounds seems like if > you > > iterate calling next() you can get stuck repeatedly returning this sentinel > > rather than the empty cluster? > > It works either way (check it yourself). > > Done Only because you added the && fGlyphCount term? https://codereview.chromium.org/2322403002/diff/180001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:1001: if (nextCluster > cluster) { // LTR text On 2016/09/15 20:27:16, Hal Canary wrote: > On 2016/09/14 16:15:03, tomhudson wrote: > > If this is the test for LTR vs RTL, what's the meaning of fReversedChars? > > I wrote this before fReversedChars. It still works. Yes, but now we have two different tests/flags for the same quantity. If they mean the same thing, can we use fReversedChars here? https://codereview.chromium.org/2322403002/diff/180001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:1090: clusters[clusterIndex++] = SkToU32(txtPtr - utf8Text); On 2016/09/15 20:27:17, Hal Canary wrote: > On 2016/09/14 16:15:02, tomhudson wrote: > > You're storing a 32-bit cast of a pointer math result in the clusters array? > > So it's an array of offsets? > > > > Nit: can we name it clusterOffsets? clusterIndices? Or put in a snippet of > > documentation that it represents offsets in the fUtf8textStorage array it's > > bundled together with? > > // The clusters[] array is an array of offsets into utf8Text[], > // one offset for each glyph. See SkTextBlobBuilder for more info. Slight preference for improved names over comments, but OK. https://codereview.chromium.org/2322403002/diff/180001/src/pdf/SkPDFDevice.cp... src/pdf/SkPDFDevice.cpp:1282: const SkGlyphID maxGlyphID = metrics->fLastGlyphID; On 2016/09/15 20:27:17, Hal Canary wrote: > On 2016/09/14 16:15:02, tomhudson wrote: > > This should be the same as glyphToUnicode.count()? Or is this the > > range-of-glyphIDs-we-accept-as-legal-but-will-return -1 from-map_glyph()-for? > > fGlyphToUnicode is not guaranteed to be populated. Acknowledged.
The CQ bit was checked by halcanary@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bungeman@google.com, tomhudson@google.com Link to the patchset: https://codereview.chromium.org/2322403002/#ps220001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== SkPDF: Implement /ActualText to make text extraction correct. For old API: no change. For new API: LTR text is perfectly extracted, RTL needs better testing. BUG=skia:5434 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2322403002 ========== to ========== SkPDF: Implement /ActualText to make text extraction correct. For old API: no change. For new API: LTR text is perfectly extracted, RTL needs better testing. BUG=skia:5434 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2322403002 Committed: https://skia.googlesource.com/skia/+/dbd16345a5b2b824f2696af791bb0f01304cf549 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:220001) as https://skia.googlesource.com/skia/+/dbd16345a5b2b824f2696af791bb0f01304cf549
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:220001) has been created in https://codereview.chromium.org/2338213008/ by halcanary@google.com. The reason for reverting is: MSAN, ASAN errors.
Message was sent while issue was closed.
Description was changed from ========== SkPDF: Implement /ActualText to make text extraction correct. For old API: no change. For new API: LTR text is perfectly extracted, RTL needs better testing. BUG=skia:5434 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2322403002 Committed: https://skia.googlesource.com/skia/+/dbd16345a5b2b824f2696af791bb0f01304cf549 ========== to ========== SkPDF: Implement /ActualText to make text extraction correct. For old API: no change. For new API: LTR text is perfectly extracted, RTL needs better testing. BUG=skia:5434 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2322403002 Committed: https://skia.googlesource.com/skia/+/dbd16345a5b2b824f2696af791bb0f01304cf549 ==========
Description was changed from ========== SkPDF: Implement /ActualText to make text extraction correct. For old API: no change. For new API: LTR text is perfectly extracted, RTL needs better testing. BUG=skia:5434 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2322403002 Committed: https://skia.googlesource.com/skia/+/dbd16345a5b2b824f2696af791bb0f01304cf549 ========== to ========== SkPDF: Implement /ActualText to make text extraction correct. For old API: no change. For new API: LTR text is perfectly extracted, RTL needs better testing. CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-Clang-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot, Test-Ubuntu-Clang-GCE-CPU-AVX2-x86_64-Debug-MSAN-Trybot BUG=skia:5434 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2322403002 ==========
Description was changed from ========== SkPDF: Implement /ActualText to make text extraction correct. For old API: no change. For new API: LTR text is perfectly extracted, RTL needs better testing. CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-Clang-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot, Test-Ubuntu-Clang-GCE-CPU-AVX2-x86_64-Debug-MSAN-Trybot BUG=skia:5434 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2322403002 ========== to ========== SkPDF: Implement /ActualText to make text extraction correct. For old API: no change. For new API: LTR text is perfectly extracted, RTL needs better testing. CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-Clang-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot,Test-Ubuntu-Clang-GCE-CPU-AVX2-x86_64-Debug-MSAN-Trybot BUG=skia:5434 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2322403002 ==========
The CQ bit was checked by halcanary@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bungeman@google.com, tomhudson@google.com Link to the patchset: https://codereview.chromium.org/2322403002/#ps240001 (title: "asserts, bounds check before read, not after")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #13 (id:260001) has been deleted
The CQ bit was checked by halcanary@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bungeman@google.com, tomhudson@google.com Link to the patchset: https://codereview.chromium.org/2322403002/#ps240001 (title: "asserts, bounds check before read, not after")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== SkPDF: Implement /ActualText to make text extraction correct. For old API: no change. For new API: LTR text is perfectly extracted, RTL needs better testing. CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-Clang-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot,Test-Ubuntu-Clang-GCE-CPU-AVX2-x86_64-Debug-MSAN-Trybot BUG=skia:5434 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2322403002 ========== to ========== SkPDF: Implement /ActualText to make text extraction correct. For old API: no change. For new API: LTR text is perfectly extracted, RTL needs better testing. CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-Clang-GCE-CPU-AVX2-x86_64-Debug-ASAN-Trybot,Test-Ubuntu-Clang-GCE-CPU-AVX2-x86_64-Debug-MSAN-Trybot BUG=skia:5434 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2322403002 Committed: https://skia.googlesource.com/skia/+/f59d18a4ea5ad56975502faf50eae43418ea9c2a ==========
Message was sent while issue was closed.
Committed patchset #12 (id:240001) as https://skia.googlesource.com/skia/+/f59d18a4ea5ad56975502faf50eae43418ea9c2a |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
