Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(337)

Issue 1394833002: Find In Page doesn't correctly highlight wrapped text. (Closed)

Created:
5 years, 2 months ago by Sk.kumar
Modified:
5 years, 2 months ago
Reviewers:
pdr., eae, wkorman
CC:
blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), jbroman, Justin Novosad, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Find In Page doesn't correctly highlight wrapped text. With "white-space: pre-wrap" texts which consist line break, we get 3 ranges for documentMarker (+ve, +ve) (-ve, +ve) and (-ve, +ve). So, when we pass -ve offset to SimpleShaper::advance which takes unsigned value, it converts the -ve offset to very large unsigned value. Because of this, Font::selectionRectForText "width" comes as "0", hence no highlight after line break. Changed SimpleShaper::advance to take int instead of unsigned. BUG=521047 Please Note: It was regression caused by https://codereview.chromium.org/608413002 Committed: https://crrev.com/bb8a0772750a23c96d210d8e54cee4b4327611ad Cr-Commit-Position: refs/heads/master@{#353100}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+34 lines, -4 lines) Patch
A third_party/WebKit/LayoutTests/fast/repaint/text-match-pre-wrapped-text.html View 1 chunk +21 lines, -0 lines 0 comments Download
A third_party/WebKit/LayoutTests/fast/repaint/text-match-pre-wrapped-text-expected.html View 1 chunk +7 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/fonts/shaping/SimpleShaper.h View 1 chunk +3 lines, -1 line 0 comments Download
M third_party/WebKit/Source/platform/fonts/shaping/SimpleShaper.cpp View 1 chunk +3 lines, -3 lines 0 comments Download

Messages

Total messages: 8 (2 generated)
Sk.kumar
pdr@, Please review. Please note: this is same as https://codereview.chromium.org/1305953003/ But due to blink merge, ...
5 years, 2 months ago (2015-10-08 13:17:00 UTC) #2
wkorman
lgtm LGTM but I have no OWNERS so you'll still need pdr/eae.
5 years, 2 months ago (2015-10-08 17:00:55 UTC) #3
pdr.
lgtm
5 years, 2 months ago (2015-10-08 17:26:14 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1394833002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1394833002/1
5 years, 2 months ago (2015-10-08 17:26:45 UTC) #6
commit-bot: I haz the power
Committed patchset #1 (id:1)
5 years, 2 months ago (2015-10-08 18:38:35 UTC) #7
commit-bot: I haz the power
5 years, 2 months ago (2015-10-08 18:40:27 UTC) #8
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/bb8a0772750a23c96d210d8e54cee4b4327611ad
Cr-Commit-Position: refs/heads/master@{#353100}

Powered by Google App Engine
This is Rietveld 408576698