Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 8 months ago by fs Modified:
5 years, 7 months ago Reviewers:
pdr., hyunjunekim2 CC:
blink-reviews Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionFix "mouse" position selection in svg/text/resources/SelectionTestCase.js
eventSender.mouseMoveTo(...) truncates the coordinates it is passed, so
fractional coordinates could end being adjust by almost an entire pixel,
which could yield incorrect results.
Round the end-points such that they should reside within the intended
selection area.
BUG=477545
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194604
Patch Set 1 #
Total comments: 2
Created: 5 years, 8 months ago
Messages
Total messages: 12 (3 generated)
fs@opera.com changed reviewers: + pdr@chromium.org
Maybe with this we can make progress in that other review...
LGTM
hyunjune.kim@samsung.com changed reviewers: + hyunjune.kim@samsung.com
https://codereview.chromium.org/1108033002/diff/1/LayoutTests/svg/text/resour... File LayoutTests/svg/text/resources/SelectionTestCase.js (right): https://codereview.chromium.org/1108033002/diff/1/LayoutTests/svg/text/resour... LayoutTests/svg/text/resources/SelectionTestCase.js:74: absStartPos.x = Math.ceil(absStartPos.x); Could you change like this? absStartPos.x = Math.ceil(absStartPos.x) + 4;
https://codereview.chromium.org/1108033002/diff/1/LayoutTests/svg/text/resour... File LayoutTests/svg/text/resources/SelectionTestCase.js (right): https://codereview.chromium.org/1108033002/diff/1/LayoutTests/svg/text/resour... LayoutTests/svg/text/resources/SelectionTestCase.js:74: absStartPos.x = Math.ceil(absStartPos.x); On 2015/04/28 at 02:22:07, hyunjunekim2 wrote: > Could you change like this? > absStartPos.x = Math.ceil(absStartPos.x) + 4; Why?
On 2015/04/28 03:08:39, pdr wrote: > https://codereview.chromium.org/1108033002/diff/1/LayoutTests/svg/text/resour... > File LayoutTests/svg/text/resources/SelectionTestCase.js (right): > > https://codereview.chromium.org/1108033002/diff/1/LayoutTests/svg/text/resour... > LayoutTests/svg/text/resources/SelectionTestCase.js:74: absStartPos.x = > Math.ceil(absStartPos.x); > On 2015/04/28 at 02:22:07, hyunjunekim2 wrote: > > Could you change like this? > > absStartPos.x = Math.ceil(absStartPos.x) + 4; > > Why? pdr, fs, In https://codereview.chromium.org/1072403007/, i updated PS 19 was removed |floor|, but occur to fail on this test case base on Ubuntu. So I suggest this one for getting some advises.
On 2015/04/28 at 05:31:41, hyunjune.kim wrote: > On 2015/04/28 03:08:39, pdr wrote: > > https://codereview.chromium.org/1108033002/diff/1/LayoutTests/svg/text/resour... > > File LayoutTests/svg/text/resources/SelectionTestCase.js (right): > > > > https://codereview.chromium.org/1108033002/diff/1/LayoutTests/svg/text/resour... > > LayoutTests/svg/text/resources/SelectionTestCase.js:74: absStartPos.x = > > Math.ceil(absStartPos.x); > > On 2015/04/28 at 02:22:07, hyunjunekim2 wrote: > > > Could you change like this? > > > absStartPos.x = Math.ceil(absStartPos.x) + 4; > > > > Why? > pdr, fs, > In https://codereview.chromium.org/1072403007/, i updated PS 19 was removed |floor|, but occur to fail on this test case > base on Ubuntu. So I suggest this one for getting some advises. I was just wondering why you chose "+ 4". Where does 4 come from?
On 2015/04/28 05:34:35, pdr wrote: > On 2015/04/28 at 05:31:41, hyunjune.kim wrote: > > On 2015/04/28 03:08:39, pdr wrote: > > > > https://codereview.chromium.org/1108033002/diff/1/LayoutTests/svg/text/resour... > > > File LayoutTests/svg/text/resources/SelectionTestCase.js (right): > > > > > > > https://codereview.chromium.org/1108033002/diff/1/LayoutTests/svg/text/resour... > > > LayoutTests/svg/text/resources/SelectionTestCase.js:74: absStartPos.x = > > > Math.ceil(absStartPos.x); > > > On 2015/04/28 at 02:22:07, hyunjunekim2 wrote: > > > > Could you change like this? > > > > absStartPos.x = Math.ceil(absStartPos.x) + 4; > > > > > > Why? > > pdr, fs, > > In https://codereview.chromium.org/1072403007/, i updated PS 19 was removed > |floor|, but occur to fail on this test case > > base on Ubuntu. So I suggest this one for getting some advises. > > I was just wondering why you chose "+ 4". Where does 4 come from? pdr, Sorry, this is experimental. If select "+ 3", occur to fail.
The CQ bit was checked by fs@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1108033002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://src.chromium.org/viewvc/blink?view=rev&revision=194604 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
