Chromium Code Reviews
|
|
|
Created:
5 years ago by mfomitchev Modified:
4 years, 10 months ago Reviewers:
yoichio, leviw_travelin_and_unemployed, aelias_OOO_until_Jul13, jdduke (slow), Rick Byers, yosin_UTC9 CC:
blink-reviews Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionImproving direction-based selection strategy.
This implements two improvements to the direction-based selection strategy:
1. The "offset":
If the selection expands beyond extent (in word granularity) and then the user
shrinks the selection - previously the granularity would change to character and
the end of the selection would jump to the extent, so the visible selection could
change quite significantly even if the user moved the extent by a little bit. Now
the selection only changes by the amount the user moved the extent from previous
position. Effectively we are saving the delta between extent and the visible
start/end in word granularity and preserving it as we switch to character granularity.
2. When the selection is expanding, previously we would expand the selection to the
end of the word as soon as the extent moved beyond the first word character. This is
the default behavior of Word granularity. With the new behavior, we only expand
the selection to the end of the word when the extent moves beyond the middle of the word.
More details in comments to DirectionGranularityStrategy in GranularityStrategy.h
BUG=451255, 498389
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196772
Patch Set 1 #
Total comments: 1
Patch Set 2 : Some cleanup #Patch Set 3 : Fixing a couple of edge cases and adding tests. Still needs cleanup. #Patch Set 4 : Selection changes when word middle is passed + tests + comments + cleanup.w #Patch Set 5 : Removing unused method. #
Total comments: 4
Patch Set 6 : Adding StrategyState enum to replace m_cleared, m_lastMoveShrunkSelection. #
Total comments: 20
Patch Set 7 : Addressing rotated text + comment cleanup #
Total comments: 2
Patch Set 8 : Not storing absolute coordinates + optimizations. #Patch Set 9 : Rebase plus adding CORE_EXPORT. #Patch Set 10 : Fixing the slow extend and expanding tests a bit. #
Total comments: 31
Patch Set 11 : Expanding unit tests #Patch Set 12 : Removing duplicate tests. #Patch Set 13 : Addressing remaining review feedback. #
Total comments: 15
Patch Set 14 : Addressing review feedback. #Patch Set 15 : Trace for moveRangeSelectionExtent. #
Total comments: 4
Patch Set 16 : Removing anonymous namespace and marking functions static instead. #
Messages
Total messages: 38 (5 generated)
https://codereview.chromium.org/1123563003/diff/1/Source/core/editing/Granula... File Source/core/editing/GranularityStrategy.cpp (right): https://codereview.chromium.org/1123563003/diff/1/Source/core/editing/Granula... Source/core/editing/GranularityStrategy.cpp:195: newSelection.setWithoutValidation(newSelection.base(), extentPosition.deepEquivalent(), newSelection.start(), newSelection.end()); Note we actually only need to set the extent without changing start/end/base, so a less general method to do that directly would also work if that's preferable.
mfomitchev@chromium.org changed reviewers: + jdduke@chromium.org, rbyers@chromium.org, yoichio@chromium.org, yosin@google.com
@yosin, yoichio: ping
mfomitchev@chromium.org changed reviewers: + yosin@chromium.org - yosin@google.com
+yosin@chromium.org instead of yosin@google.com
Cleaned up version, ready for review. Selection now advances when the middle of the word is passed. See the CL description for more details. yosin/yoichio - can you PTAL?
Just a high level q, is any of this code traced? Should we add a top level trace for the primary functions used to update the (user manipulated) selection?
I think passing |WebPoint|, point in contents-space, to Blink from Chromium in
API isn't good for selection, since points in selection aren't tracked by
contents modification, e.g. changing font-size.
BTW, can we have PlatfromEvent::SelecitonHandle{Press,Move} instead of passing
|WebPoint| via LocalFrame API? I think introducing selection handle events
allows us implementing selection handle handling modular without exposing API to
Chromium from Blink.
https://codereview.chromium.org/1123563003/diff/80001/Source/core/editing/Gra...
File Source/core/editing/GranularityStrategy.h (right):
https://codereview.chromium.org/1123563003/diff/80001/Source/core/editing/Gra...
Source/core/editing/GranularityStrategy.h:101: IntPoint m_extentPoint;
Contents-Space point is fragile and not tracked, e.g. changing content before
extent or rotating screen changes point for extent. If we really need point, we
should calculate on demand.
https://codereview.chromium.org/1123563003/diff/80001/Source/core/editing/Gra...
Source/core/editing/GranularityStrategy.h:106: bool m_cleared;
Can we introduce |enum State| or another name to unify
|m_lastMoveShrunkSelection| and |m_cleared| for describing behavior in state
transition?
> Just a high level q, is any of this code traced? Should we add a top level
trace
> for the primary functions used to update the (user manipulated) selection?
I am not aware of any tracing for this in Blink. yosin - do you know? I guess
tracing in Blink would be better than in Chrome, since we would be able to track
JS-originated changes as well as user-initiated changes.
> I think passing |WebPoint|, point in contents-space, to Blink from Chromium in
> API isn't good for selection, since points in selection aren't tracked by
> contents modification, e.g. changing font-size.
> BTW, can we have PlatfromEvent::SelecitonHandle{Press,Move} instead of passing
> |WebPoint| via LocalFrame API? I think introducing selection handle events
> allows us implementing selection handle handling modular > without exposing
API to
> Chromium from Blink.
Perhaps this would make sense. We should discuss more - can we break this
discussion out into a separate email thread? This sounds like a pretty big
change, and this CL doesn't need to depend on it. We should try to land this for
M44..
https://codereview.chromium.org/1123563003/diff/80001/Source/core/editing/Gra...
File Source/core/editing/GranularityStrategy.h (right):
https://codereview.chromium.org/1123563003/diff/80001/Source/core/editing/Gra...
Source/core/editing/GranularityStrategy.h:101: IntPoint m_extentPoint;
How can we calculate it? This IntPoint corresponds to the extentPoint from the
previous updateExtent call, so we can't determine it based on the current state.
Also saving VisiblePosition wouldn't have enough accuracy since there are
several pixels corresponding to the same position and we need pixel accuracy.
Note that in practice all these state variables are only used during a single
handle drag. Every time a new handle drag is started, the selection is reset
(https://code.google.com/p/chromium/codesearch#chromium/src/ui/touch_selection...),
which clears the strategy's state
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
A rotation will interrupt the drag, as would many other user actions. If JS
modifies the content while we are dragging the handle, then the stored state
could get invalid, but that should be pretty unusual. Even then, the
consequences of invalid state are not severe - selection can be set with an
incorrect granularity, but it is easy to recover by moving the handle.
https://codereview.chromium.org/1123563003/diff/80001/Source/core/editing/Gra...
Source/core/editing/GranularityStrategy.h:106: bool m_cleared;
On 2015/05/18 07:47:03, Yosi_UTC9 wrote:
> Can we introduce |enum State| or another name to unify
> |m_lastMoveShrunkSelection| and |m_cleared| for describing behavior in state
> transition?
Done.
mfomitchev@chromium.org changed reviewers: + leviw@chromium.org
+Levi who is also expert in this code and had some thoughts on the CL
First pass of comments. https://codereview.chromium.org/1123563003/diff/100001/Source/core/editing/Fr... File Source/core/editing/FrameSelectionTest.cpp (right): https://codereview.chromium.org/1123563003/diff/100001/Source/core/editing/Fr... Source/core/editing/FrameSelectionTest.cpp:76: return pos.absoluteCaretBounds().minXMinYCorner(); The minXMinYCorner is definitely what you want? How does this behave with transforms? What about vertical text? https://codereview.chromium.org/1123563003/diff/100001/Source/core/editing/Fr... Source/core/editing/FrameSelectionTest.cpp:86: letterPos.push_back(visiblePositionToContentsPoint(VisiblePosition(Position(text, i)))); Nit: Generally it would be cheaper to iterate on VisiblePositions (next and previous) instead of constructing a new one every time. It would also avoid converting to content points for un-rendered positions (between code points or in collapsed whitespace). It's okay for testing code. https://codereview.chromium.org/1123563003/diff/100001/Source/core/editing/Fr... Source/core/editing/FrameSelectionTest.cpp:217: TEST_F(FrameSelectionTest, MoveRangeSelectionExtentDirectionExpand) Do we have tests for vertical writing modes or transformed text? https://codereview.chromium.org/1123563003/diff/100001/Source/core/editing/Gr... File Source/core/editing/GranularityStrategy.cpp (right): https://codereview.chromium.org/1123563003/diff/100001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:94: VisiblePosition oldOffsetExtentPosition = visiblePositionForContentsPoint(oldOffsetExtentPoint, frame); As discussed in our meeting, hit testing is expensive. Ideally we'd cache the old position and convert its visible position into absolute coordinates to compare. This would have the benefit of tracking the position if it moves for some reason. It would also mean we could get into the case where the node we're offset into gets blown away, so we'd want to make sure we use the same logic as Selection to blow away all this state if an endpoint is removed. https://codereview.chromium.org/1123563003/diff/100001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:98: int dx = extentPoint.x() - m_extentPoint.x(); This is just wrong in the presence of transforms or vertical text. https://codereview.chromium.org/1123563003/diff/100001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:124: bool extentBaseOrderSwitched = !arePositionsInOrder(offsetExtentPosition, base, oldExtentBaseOrder); Is it possible to avoid three calls to comparePositions? That function can also be super expensive. https://codereview.chromium.org/1123563003/diff/100001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:151: bool expandedBeyondWordBoundary = arePositionsInOrder(offsetExtentPosition, currentWordBoundary, extentBaseOrder); More comparePositions... https://codereview.chromium.org/1123563003/diff/100001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:158: && arePositionsInOrder(oldOffsetExtentPosition, offsetExtentPosition, extentBaseOrder); And *another* comparePositions. This needs to be cleaned up. https://codereview.chromium.org/1123563003/diff/100001/Source/core/editing/Gr... File Source/core/editing/GranularityStrategy.h (right): https://codereview.chromium.org/1123563003/diff/100001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.h:100: // Returns true if the two supplies positions are in the same order as s/supplies/supplied/ https://codereview.chromium.org/1123563003/diff/100001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.h:101: // the order defined by the supplied int based on camparePositions. s/campare/compare/ I find this comment confusing, but I understand what you're trying to say. https://codereview.chromium.org/1123563003/diff/100001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.h:115: // Offset applied to the extent. Offset in pixels? How does this work with transforms or vertical text?
https://codereview.chromium.org/1123563003/diff/100001/Source/core/editing/Fr... File Source/core/editing/FrameSelectionTest.cpp (right): https://codereview.chromium.org/1123563003/diff/100001/Source/core/editing/Fr... Source/core/editing/FrameSelectionTest.cpp:76: return pos.absoluteCaretBounds().minXMinYCorner(); On 2015/05/20 20:24:37, leviw wrote: > The minXMinYCorner is definitely what you want? How does this behave with > transforms? What about vertical text? I don't think y matters here, but I do want minX. The offset feature doesn't work with rotated text and I can't think of an easy way to make it work with text that has different rotation transforms on different parts of text. I have a simple patch that sets offset to 0 every time we detect a non-zero y component to the difference in positions. I haven't had a chance to test it yet (will do it later today), but I guess I will publish it just the same to get your feedback on the approach. Rotated text is an edge case, so as long a we behave sanely with it, I think it should be okay. https://codereview.chromium.org/1123563003/diff/100001/Source/core/editing/Fr... Source/core/editing/FrameSelectionTest.cpp:86: letterPos.push_back(visiblePositionToContentsPoint(VisiblePosition(Position(text, i)))); On 2015/05/20 20:24:37, leviw wrote: > Nit: Generally it would be cheaper to iterate on VisiblePositions (next and > previous) instead of constructing a new one every time. It would also avoid > converting to content points for un-rendered positions (between code points or > in collapsed whitespace). It's okay for testing code. Acknowledged. I will clean it up after the rest of the changes. https://codereview.chromium.org/1123563003/diff/100001/Source/core/editing/Fr... Source/core/editing/FrameSelectionTest.cpp:217: TEST_F(FrameSelectionTest, MoveRangeSelectionExtentDirectionExpand) On 2015/05/20 20:24:37, leviw wrote: > Do we have tests for vertical writing modes or transformed text? See comment above https://codereview.chromium.org/1123563003/diff/100001/Source/core/editing/Gr... File Source/core/editing/GranularityStrategy.cpp (right): https://codereview.chromium.org/1123563003/diff/100001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:94: VisiblePosition oldOffsetExtentPosition = visiblePositionForContentsPoint(oldOffsetExtentPoint, frame); Let me think about/work on this. https://codereview.chromium.org/1123563003/diff/100001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:98: int dx = extentPoint.x() - m_extentPoint.x(); Yup. https://codereview.chromium.org/1123563003/diff/100001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:124: bool extentBaseOrderSwitched = !arePositionsInOrder(offsetExtentPosition, base, oldExtentBaseOrder); Let me think about this as well. https://codereview.chromium.org/1123563003/diff/100001/Source/core/editing/Gr... File Source/core/editing/GranularityStrategy.h (right): https://codereview.chromium.org/1123563003/diff/100001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.h:100: // Returns true if the two supplies positions are in the same order as On 2015/05/20 20:24:37, leviw wrote: > s/supplies/supplied/ Done. https://codereview.chromium.org/1123563003/diff/100001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.h:101: // the order defined by the supplied int based on camparePositions. How's the new version? https://codereview.chromium.org/1123563003/diff/100001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.h:115: // Offset applied to the extent. On 2015/05/20 20:24:37, leviw wrote: > Offset in pixels? How does this work with transforms or vertical text? Yes. See response to the first comment in FrameSelectionTest.cpp
I'm not sure I agree with the assertion that rotated text is an edge-case. Can we get UX to comment on that behavior? Also: if you're using the top-left caret position, you'll bail when text on the same line changes sizes. You'd want to use the baseline position instead. https://codereview.chromium.org/1123563003/diff/120001/Source/core/editing/Gr... File Source/core/editing/GranularityStrategy.h (right): https://codereview.chromium.org/1123563003/diff/120001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.h:101: // the int parameter: This isn't exactly what I meant. I'd probably go with something like this: // Order is specified using the same contract as comparePositions. bool arePositionsInSpecifiedOrder(const vp&, const vp&, int specifiedOrder)
> Also: if you're using the top-left caret position, you'll bail when text on the > same line changes sizes. You'd want to use the baseline position instead. Yeah, that's what I am doing in updateExtent. The top-left position is only used in tests. I should probably change that for consistency though.
On 2015/05/20 at 22:16:49, mfomitchev wrote: > > Also: if you're using the top-left caret position, you'll bail when text on the > > same line changes sizes. You'd want to use the baseline position instead. > > Yeah, that's what I am doing in updateExtent. The top-left position is only used in tests. I should probably change that for consistency though. We should also have tests for that behavior.
@leviw: PTAL - please let me know if the algorithm looks okay now, and then I can add some more unit tests. - Storing location of the extent point relative to extent's VisiblePosition (m_subPositionCorrection) instead of storing it in absolute coordinates. - Getting rid of one of the three hit tests. Unfortunately I don't see how to reduce the number of hit tests to one in all cases. - Reducing the number of comparePositions calls. Typically we are now doing two. If the extent/base order changes, we are doing three. https://codereview.chromium.org/1123563003/diff/120001/Source/core/editing/Gr... File Source/core/editing/GranularityStrategy.h (right): https://codereview.chromium.org/1123563003/diff/120001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.h:101: // the int parameter: On 2015/05/20 22:01:01, leviw wrote: > This isn't exactly what I meant. > > I'd probably go with something like this: > // Order is specified using the same contract as comparePositions. > bool arePositionsInSpecifiedOrder(const vp&, const vp&, int specifiedOrder) Done.
https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Fr... File Source/core/editing/FrameSelectionTest.cpp (right): https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Fr... Source/core/editing/FrameSelectionTest.cpp:268: int y = letterPos[29].y(); This is a really synthetic version of the test I'm looking for. How about a test case with a rotation. With a transform. With a scale. With an inline image with a vertical-position that isn't "baseline." With a line of text with multiple font sizes. https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... File Source/core/editing/GranularityStrategy.cpp (right): https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:55: m_subPositionCorrection.setHeight(0); m_subPositionCorrection = IntSize(); https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:93: IntPoint oldOffsetExtentPoint = oldExtentCaretBounds.minXMaxYCorner() + m_subPositionCorrection; There are subtle reasons for why you're using minXMaxYCorner. They should be documented. Perhaps a static inline method with a handy name that just returns minXMaxYCorner() but supplies context? https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:115: || (newOffsetExtentCaretBounds.maxY() - oldExtentCaretBounds.maxY()) != 0) { (newOffsetExtentCaretBounds.maxY() - oldExtentCaretBounds.maxY()) != 0 is the same as newOffsetExtentCaretBounds.maxY() != oldExtentCaretBounds.maxY(). You should swap the order of these conditions since the common case is the 2nd part will be true if the 1st is, and checking it is much cheaper. https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:123: // Do not allow selection and extent to be at the same position I'm a little confused by this statement. Are you not allowing them to be the same position, or are you just not running the rest of this algorithm if they are? https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:174: expandedBeyondWordBoundary = arePositionsInSpecifiedOrder(newOffsetExtentPosition, wordBoundary, newExtentBaseOrder); So worst case we have to do 3 calls to comparePositions? Ideally we'd have some performance tests so that we're not just divining whether this sort of thing is acceptable. https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:213: m_subPositionCorrection = extentPoint + IntSize(m_offset, 0) - newSelectionExtent.absoluteCaretBounds().minXMaxYCorner(); This is super magical and needs some explanation. It'd be nice if instead of storing m_offset as an int, we stored it as an IntSize so we could assert we didn't operate on caret positions that vary in Y position. https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... File Source/core/editing/GranularityStrategy.h (right): https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.h:112: // The vector defining the location of the extent point in contents-space What vector? This content doesn't help me understand what m_subPositionCorrection is when looking at this header. https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.h:115: // Offset applied to the extent point. This is always going to be an x offset, right? Not width, or Y, or anything with transforms? And you did say it's in pixels, right? Could be be clearer than just "offset"?
https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Fr... File Source/core/editing/FrameSelectionTest.cpp (right): https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Fr... Source/core/editing/FrameSelectionTest.cpp:268: int y = letterPos[29].y(); On 2015/05/26 20:25:50, leviw wrote: > This is a really synthetic version of the test I'm looking for. How about a test > case with a rotation. With a transform. With a scale. With an inline image with > a vertical-position that isn't "baseline." With a line of text with multiple > font sizes. I will work on this. Scale/non-rotate transform shouldn't really make a difference to the algorithm as long as the hit testing works correctly though. From FrameSelection's point of view I don't think working on a scaled text vs. working on a text with a different font/font-size makes any difference... https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... File Source/core/editing/GranularityStrategy.cpp (right): https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:55: m_subPositionCorrection.setHeight(0); On 2015/05/26 20:25:50, leviw wrote: > m_subPositionCorrection = IntSize(); Acknowledged. https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:93: IntPoint oldOffsetExtentPoint = oldExtentCaretBounds.minXMaxYCorner() + m_subPositionCorrection; On 2015/05/26 20:25:50, leviw wrote: > There are subtle reasons for why you're using minXMaxYCorner. They should be > documented. Perhaps a static inline method with a handy name that just returns > minXMaxYCorner() but supplies context? Acknowledged. https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:115: || (newOffsetExtentCaretBounds.maxY() - oldExtentCaretBounds.maxY()) != 0) { On 2015/05/26 20:25:50, leviw wrote: > (newOffsetExtentCaretBounds.maxY() - oldExtentCaretBounds.maxY()) != 0 is the > same as newOffsetExtentCaretBounds.maxY() != oldExtentCaretBounds.maxY(). > > You should swap the order of these conditions since the common case is the 2nd > part will be true if the 1st is, and checking it is much cheaper. Acknowledged. https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:123: // Do not allow selection and extent to be at the same position On 2015/05/26 20:25:50, leviw wrote: > I'm a little confused by this statement. Are you not allowing them to be the > same position, or are you just not running the rest of this algorithm if they > are? This should read "base and extent", not "selection and extent". How about I change it to: // Do not allow empty selection The idea is basically that if extentPoint corresponds to the same position as the base, we just leave the selection the way it was. Note this functionality was always there. CharacterGranularityStrategy::updateExtent behaves this way as well. https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:174: expandedBeyondWordBoundary = arePositionsInSpecifiedOrder(newOffsetExtentPosition, wordBoundary, newExtentBaseOrder); On 2015/05/26 20:25:50, leviw wrote: > So worst case we have to do 3 calls to comparePositions? > > Ideally we'd have some performance tests so that we're not just divining whether > this sort of thing is acceptable. Right. Typically we will have two tests. We will only have 3 tests if the update moves the extent to the other side of the base, i.e. when the handles get crossed, to use Android's terminology. We will only have one 3-test update per each crossing of the handles. Native Android intentionally doesn't support handle crossing, and they want us to remove support for it as well, so this case may even go away. Note the current code actually does four comparePositions.. Either way, doing a perf test sounds like a good idea, considering we are also sometimes doing more hit testing. Would you be okay with me landing this without the perf test initially and working on the test afterwards? https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:213: m_subPositionCorrection = extentPoint + IntSize(m_offset, 0) - newSelectionExtent.absoluteCaretBounds().minXMaxYCorner(); On 2015/05/26 20:25:50, leviw wrote: > This is super magical and needs some explanation. > > It'd be nice if instead of storing m_offset as an int, we stored it as an > IntSize so we could assert we didn't operate on caret positions that vary in Y > position. I like storing the offset as an int since it makes it explicit from the definition that it is along a single axis. (Perhaps I should call it m_offset_x though). We can do an assert either way. We can use a temp local IntSize variable for that. https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... File Source/core/editing/GranularityStrategy.h (right): https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.h:112: // The vector defining the location of the extent point in contents-space On 2015/05/26 20:25:50, leviw wrote: > What vector? This content doesn't help me understand what > m_subPositionCorrection is when looking at this header. Is there a more appropriate class than IntSize to represent a vector in Blink? In Chromium there are separate gfx::Vector2d and gfx::Size classes, but I only found IntSize in Blink. This is a vector between two points. The start of the vector is the bottom-left of the caret rect of the VisiblePosition corresponding to the selection extent (which btw always matches either selection start or selection end). The end of the vector is the extentPoint moved horizontally by |offset|. I'll try to explain this better. https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.h:115: // Offset applied to the extent point. On 2015/05/26 20:25:50, leviw wrote: > This is always going to be an x offset, right? Not width, or Y, or anything with > transforms? And you did say it's in pixels, right? Could be be clearer than just > "offset"? Acknowledged.
https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Fr... File Source/core/editing/FrameSelectionTest.cpp (right): https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Fr... Source/core/editing/FrameSelectionTest.cpp:268: int y = letterPos[29].y(); On 2015/05/26 22:32:46, mfomitchev wrote: > On 2015/05/26 20:25:50, leviw wrote: > > This is a really synthetic version of the test I'm looking for. How about a > test > > case with a rotation. With a transform. With a scale. With an inline image > with > > a vertical-position that isn't "baseline." With a line of text with multiple > > font sizes. > > I will work on this. > Scale/non-rotate transform shouldn't really make a difference to the algorithm > as long as the hit testing works correctly though. From FrameSelection's point > of view I don't think working on a scaled text vs. working on a text with a > different font/font-size makes any difference... The way your previous code worked, different font-sizes would have changed the points you were comparing. We should ensure we don't break that later. You should at a minimum have rotations and 3rd transforms that do and don't effect the y position (translateZ(0), scale(-1, 1)). https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... File Source/core/editing/GranularityStrategy.cpp (right): https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:123: // Do not allow selection and extent to be at the same position On 2015/05/26 22:32:46, mfomitchev wrote: > On 2015/05/26 20:25:50, leviw wrote: > > I'm a little confused by this statement. Are you not allowing them to be the > > same position, or are you just not running the rest of this algorithm if they > > are? > > This should read "base and extent", not "selection and extent". How about I > change it to: > // Do not allow empty selection > The idea is basically that if extentPoint corresponds to the same position as > the base, we just leave the selection the way it was. Note this functionality > was always there. CharacterGranularityStrategy::updateExtent behaves this way > as well. SGTM. https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:174: expandedBeyondWordBoundary = arePositionsInSpecifiedOrder(newOffsetExtentPosition, wordBoundary, newExtentBaseOrder); On 2015/05/26 22:32:47, mfomitchev wrote: > On 2015/05/26 20:25:50, leviw wrote: > > So worst case we have to do 3 calls to comparePositions? > > > > Ideally we'd have some performance tests so that we're not just divining > whether > > this sort of thing is acceptable. > > Right. Typically we will have two tests. We will only have 3 tests if the update > moves the extent to the other side of the base, i.e. when the handles get > crossed, to use Android's terminology. We will only have one 3-test update per > each crossing of the handles. > > Native Android intentionally doesn't support handle crossing, and they want us > to remove support for it as well, so this case may even go away. > > Note the current code actually does four comparePositions.. This function can at most call it three times, right? line 174 and 172 are mutually exclusive. > > Either way, doing a perf test sounds like a good idea, considering we are also > sometimes doing more hit testing. Would you be okay with me landing this without > the perf test initially and working on the test afterwards? The issue with putting this change before a perf test is that we won't have a baseline before, so we just simply won't know what if any regression in performance this test is causing (unless we have 2 different versions and the capability to turn this functionality on and off at runtime). https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... File Source/core/editing/GranularityStrategy.h (right): https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.h:112: // The vector defining the location of the extent point in contents-space On 2015/05/26 22:32:47, mfomitchev wrote: > On 2015/05/26 20:25:50, leviw wrote: > > What vector? This content doesn't help me understand what > > m_subPositionCorrection is when looking at this header. > > Is there a more appropriate class than IntSize to represent a vector in Blink? > In Chromium there are separate gfx::Vector2d and gfx::Size classes, but I only > found IntSize in Blink. > > This is a vector between two points. The start of the vector is the bottom-left > of the caret rect of the VisiblePosition corresponding to the selection extent > (which btw always matches either selection start or selection end). The end of > the vector is the extentPoint moved horizontally by |offset|. > > I'll try to explain this better. IntSize is the most appropriate in Blink, but we don't usually refer to these solely as a "vector," because of the Vector<> container.
https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Fr... File Source/core/editing/FrameSelectionTest.cpp (right): https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Fr... Source/core/editing/FrameSelectionTest.cpp:268: int y = letterPos[29].y(); On 2015/05/26 22:47:48, leviw wrote: > On 2015/05/26 22:32:46, mfomitchev wrote: > > On 2015/05/26 20:25:50, leviw wrote: > > > This is a really synthetic version of the test I'm looking for. How about a > > test > > > case with a rotation. With a transform. With a scale. With an inline image > > with > > > a vertical-position that isn't "baseline." With a line of text with multiple > > > font sizes. > > > > I will work on this. > > Scale/non-rotate transform shouldn't really make a difference to the algorithm > > as long as the hit testing works correctly though. From FrameSelection's point > > of view I don't think working on a scaled text vs. working on a text with a > > different font/font-size makes any difference... > > The way your previous code worked, different font-sizes would have changed the > points you were comparing. We should ensure we don't break that later. Sorry, I don't understand this. Which code and which points are you referring to? https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... File Source/core/editing/GranularityStrategy.cpp (right): https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:174: expandedBeyondWordBoundary = arePositionsInSpecifiedOrder(newOffsetExtentPosition, wordBoundary, newExtentBaseOrder); On 2015/05/26 22:47:49, leviw wrote: > On 2015/05/26 22:32:47, mfomitchev wrote: > > On 2015/05/26 20:25:50, leviw wrote: > > > So worst case we have to do 3 calls to comparePositions? > > > > > > Ideally we'd have some performance tests so that we're not just divining > > whether > > > this sort of thing is acceptable. > > > > Right. Typically we will have two tests. We will only have 3 tests if the > update > > moves the extent to the other side of the base, i.e. when the handles get > > crossed, to use Android's terminology. We will only have one 3-test update per > > each crossing of the handles. > > > > Native Android intentionally doesn't support handle crossing, and they want us > > to remove support for it as well, so this case may even go away. > > > > Note the current code actually does four comparePositions.. > > This function can at most call it three times, right? line 174 and 172 are > mutually exclusive. I didn't word this very well. When I said "current code" I meant current code in Blink, i.e. before this CL. As for this CL - yes, three times at most, but typically two as I explained above. > > > > > Either way, doing a perf test sounds like a good idea, considering we are also > > sometimes doing more hit testing. Would you be okay with me landing this > without > > the perf test initially and working on the test afterwards? > > The issue with putting this change before a perf test is that we won't have a > baseline before, so we just simply won't know what if any regression in > performance this test is causing (unless we have 2 different versions and the > capability to turn this functionality on and off at runtime). We can control which selection strategy is used by using kTouchTextSelectionStrategy Chrome flag or SelectionStrategyType Blink WebSetting. The "character" selection strategy is basically the old behavior which can provide the baseline. https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... File Source/core/editing/GranularityStrategy.h (right): https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.h:112: // The vector defining the location of the extent point in contents-space On 2015/05/26 22:47:49, leviw wrote: > On 2015/05/26 22:32:47, mfomitchev wrote: > > On 2015/05/26 20:25:50, leviw wrote: > > > What vector? This content doesn't help me understand what > > > m_subPositionCorrection is when looking at this header. > > > > Is there a more appropriate class than IntSize to represent a vector in Blink? > > In Chromium there are separate gfx::Vector2d and gfx::Size classes, but I only > > found IntSize in Blink. > > > > This is a vector between two points. The start of the vector is the > bottom-left > > of the caret rect of the VisiblePosition corresponding to the selection extent > > (which btw always matches either selection start or selection end). The end of > > the vector is the extentPoint moved horizontally by |offset|. > > > > I'll try to explain this better. > > IntSize is the most appropriate in Blink, but we don't usually refer to these > solely as a "vector," because of the Vector<> container. > Acknowledged.
@leviw: I added a bunch of new tests, can you please take a quick look to see if the coverage looks okay to you now? The tests are now all in GranularityStrategyTest.cpp. I think I covered all the scenarios you brought up except for inline images. That one is actually causing some funny behavior - nothing too terrible, but I want to spend a bit of time to see if I can improve it for M44. The main algorithm still needs some cleanup.
On 2015/06/02 at 21:30:47, mfomitchev wrote: > @leviw: I added a bunch of new tests, can you please take a quick look to see if the coverage looks okay to you now? The tests are now all in GranularityStrategyTest.cpp. I think I covered all the scenarios you brought up except for inline images. That one is actually causing some funny behavior - nothing too terrible, but I want to spend a bit of time to see if I can improve it for M44. This test coverage makes me much more comfortable, thank you for your work! Great to hear this may have uncovered weirdness with inline images. Let me know when you've got these working satisfactorily and have the algorithm cleaned up, I'm happy to take a full look. > > The main algorithm still needs some cleanup.
https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... File Source/core/editing/GranularityStrategy.cpp (right): https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:55: m_subPositionCorrection.setHeight(0); On 2015/05/26 20:25:50, leviw wrote: > m_subPositionCorrection = IntSize(); Done. https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:93: IntPoint oldOffsetExtentPoint = oldExtentCaretBounds.minXMaxYCorner() + m_subPositionCorrection; On 2015/05/26 20:25:50, leviw wrote: > There are subtle reasons for why you're using minXMaxYCorner. They should be > documented. Perhaps a static inline method with a handy name that just returns > minXMaxYCorner() but supplies context? Done. https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:115: || (newOffsetExtentCaretBounds.maxY() - oldExtentCaretBounds.maxY()) != 0) { On 2015/05/26 22:32:47, mfomitchev wrote: > On 2015/05/26 20:25:50, leviw wrote: > > (newOffsetExtentCaretBounds.maxY() - oldExtentCaretBounds.maxY()) != 0 is the > > same as newOffsetExtentCaretBounds.maxY() != oldExtentCaretBounds.maxY(). > > > > You should swap the order of these conditions since the common case is the 2nd > > part will be true if the 1st is, and checking it is much cheaper. > > Acknowledged. Done. https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:123: // Do not allow selection and extent to be at the same position On 2015/05/26 22:47:49, leviw wrote: > On 2015/05/26 22:32:46, mfomitchev wrote: > > On 2015/05/26 20:25:50, leviw wrote: > > > I'm a little confused by this statement. Are you not allowing them to be the > > > same position, or are you just not running the rest of this algorithm if > they > > > are? > > > > This should read "base and extent", not "selection and extent". How about I > > change it to: > > // Do not allow empty selection > > The idea is basically that if extentPoint corresponds to the same position as > > the base, we just leave the selection the way it was. Note this functionality > > was always there. CharacterGranularityStrategy::updateExtent behaves this way > > as well. > > SGTM. Done. https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... File Source/core/editing/GranularityStrategy.h (right): https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.h:112: // The vector defining the location of the extent point in contents-space On 2015/05/26 20:25:50, leviw wrote: > What vector? This content doesn't help me understand what > m_subPositionCorrection is when looking at this header. Done. https://codereview.chromium.org/1123563003/diff/180001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.h:115: // Offset applied to the extent point. On 2015/05/26 20:25:50, leviw wrote: > This is always going to be an x offset, right? Not width, or Y, or anything with > transforms? And you did say it's in pixels, right? Could be be clearer than just > "offset"? Done.
https://codereview.chromium.org/1123563003/diff/240001/Source/core/editing/Fr... File Source/core/editing/FrameSelection.h (right): https://codereview.chromium.org/1123563003/diff/240001/Source/core/editing/Fr... Source/core/editing/FrameSelection.h:131: // is set to the same position as the current base, this function will do If the new extent is 'resolved' to the same position, maybe? https://codereview.chromium.org/1123563003/diff/240001/Source/core/editing/Fr... File Source/core/editing/FrameSelectionTest.cpp (right): https://codereview.chromium.org/1123563003/diff/240001/Source/core/editing/Fr... Source/core/editing/FrameSelectionTest.cpp:125: // TODO: Do I need this? It'd be apparent quickly (at least on Debug) if you try running the tests without it. You won't really end up doing much more work either way, though. https://codereview.chromium.org/1123563003/diff/240001/Source/core/editing/Gr... File Source/core/editing/GranularityStrategy.cpp (right): https://codereview.chromium.org/1123563003/diff/240001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:216: m_subPositionCorrection = extentPoint + IntSize(m_offset, 0) - positionLocation(newSelectionExtent); I think we need a better variable name for m_subPositionCorrection to make this line easy to parse. https://codereview.chromium.org/1123563003/diff/240001/Source/core/editing/Gr... File Source/core/editing/GranularityStrategy.h (right): https://codereview.chromium.org/1123563003/diff/240001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.h:104: static IntPoint positionLocation(const VisiblePosition& vp) { return vp.absoluteCaretBounds().minXMaxYCorner(); } Why not put this in the cpp file? https://codereview.chromium.org/1123563003/diff/240001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.h:107: bool arePositionsInSpecifiedOrder(const VisiblePosition& vp1, const VisiblePosition& vp2, int specifiedOrder); Nit: you don't need the 'vp1' and 'vp2' names here. https://codereview.chromium.org/1123563003/diff/240001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.h:120: // Horizontal offset in pixels in content space applied to the extent point. You use "content space" repeatedly in this CL, but generally it seems like you're using what in blink we refer to as absolute coordinates (hence absoluteCaretBounds() and LayoutObject::localToAbsolute()). https://codereview.chromium.org/1123563003/diff/240001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.h:125: // The start of the vector is the bottom-left of the caret rect of the A couple things: - Again I don't think you should call this a vector. - Saying this has a start and end makes it sound like there's both a start and and offset in this, but really it's just a width and height. This comment seems misleading to me.
https://codereview.chromium.org/1123563003/diff/240001/Source/core/editing/Gr... File Source/core/editing/GranularityStrategy.h (right): https://codereview.chromium.org/1123563003/diff/240001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.h:125: // The start of the vector is the bottom-left of the caret rect of the On 2015/06/03 22:29:48, leviw wrote: > A couple things: > - Again I don't think you should call this a vector. > - Saying this has a start and end makes it sound like there's both a start and > and offset in this, but really it's just a width and height. This comment seems > misleading to me. If I call it Euclidean vector, or geometric vector, or spatial vector, would that be okay? And then say initial point and terminal point instead of start and end?
On 2015/06/03 at 22:36:55, mfomitchev wrote: > https://codereview.chromium.org/1123563003/diff/240001/Source/core/editing/Gr... > File Source/core/editing/GranularityStrategy.h (right): > > https://codereview.chromium.org/1123563003/diff/240001/Source/core/editing/Gr... > Source/core/editing/GranularityStrategy.h:125: // The start of the vector is the bottom-left of the caret rect of the > On 2015/06/03 22:29:48, leviw wrote: > > A couple things: > > - Again I don't think you should call this a vector. > > - Saying this has a start and end makes it sound like there's both a start and > > and offset in this, but really it's just a width and height. This comment seems > > misleading to me. > > If I call it Euclidean vector, or geometric vector, or spatial vector, would that be okay? And then say initial point and terminal point instead of start and end? I think it'd be more useful just to say the purpose of this and how it's used. Reading the description, it's not clear to me what the purpose of this variable is without having a lot of context.
https://codereview.chromium.org/1123563003/diff/240001/Source/core/editing/Fr... File Source/core/editing/FrameSelection.h (right): https://codereview.chromium.org/1123563003/diff/240001/Source/core/editing/Fr... Source/core/editing/FrameSelection.h:131: // is set to the same position as the current base, this function will do On 2015/06/03 22:29:47, leviw wrote: > If the new extent is 'resolved' to the same position, maybe? Done. https://codereview.chromium.org/1123563003/diff/240001/Source/core/editing/Fr... File Source/core/editing/FrameSelectionTest.cpp (right): https://codereview.chromium.org/1123563003/diff/240001/Source/core/editing/Fr... Source/core/editing/FrameSelectionTest.cpp:125: // TODO: Do I need this? On 2015/06/03 22:29:47, leviw wrote: > It'd be apparent quickly (at least on Debug) if you try running the tests > without it. You won't really end up doing much more work either way, though. Sorry, this was a reminder for myself to check something which I forgot to remove. https://codereview.chromium.org/1123563003/diff/240001/Source/core/editing/Gr... File Source/core/editing/GranularityStrategy.cpp (right): https://codereview.chromium.org/1123563003/diff/240001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:216: m_subPositionCorrection = extentPoint + IntSize(m_offset, 0) - positionLocation(newSelectionExtent); On 2015/06/03 22:29:48, leviw wrote: > I think we need a better variable name for m_subPositionCorrection to make this > line easy to parse. Done. https://codereview.chromium.org/1123563003/diff/240001/Source/core/editing/Gr... File Source/core/editing/GranularityStrategy.h (right): https://codereview.chromium.org/1123563003/diff/240001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.h:104: static IntPoint positionLocation(const VisiblePosition& vp) { return vp.absoluteCaretBounds().minXMaxYCorner(); } On 2015/06/03 22:29:48, leviw wrote: > Why not put this in the cpp file? Ok. I've put this and a couple of other methods in an anonymous namespace in the cpp file. This is the pattern typically used in chromium in cases when a method doesn't use any of the class's state. https://codereview.chromium.org/1123563003/diff/240001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.h:107: bool arePositionsInSpecifiedOrder(const VisiblePosition& vp1, const VisiblePosition& vp2, int specifiedOrder); On 2015/06/03 22:29:48, leviw wrote: > Nit: you don't need the 'vp1' and 'vp2' names here. Done. https://codereview.chromium.org/1123563003/diff/240001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.h:120: // Horizontal offset in pixels in content space applied to the extent point. On 2015/06/03 22:29:48, leviw wrote: > You use "content space" repeatedly in this CL, but generally it seems like > you're using what in blink we refer to as absolute coordinates (hence > absoluteCaretBounds() and LayoutObject::localToAbsolute()). Done.. I was trying to use the terminology established here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/1123563003/diff/240001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.h:125: // The start of the vector is the bottom-left of the caret rect of the On 2015/06/03 22:29:48, leviw wrote: > A couple things: > - Again I don't think you should call this a vector. > - Saying this has a start and end makes it sound like there's both a start and > and offset in this, but really it's just a width and height. This comment seems > misleading to me. Done.
Switch your local methods over to just being marked static instead of anonymous namespace and I think we can go ahead and get this landed. https://codereview.chromium.org/1123563003/diff/280001/Source/core/editing/Gr... File Source/core/editing/GranularityStrategy.cpp (right): https://codereview.chromium.org/1123563003/diff/280001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:12: namespace { We just mark these as static in Blink and don't bother with anonymous namespace. https://codereview.chromium.org/1123563003/diff/280001/Source/core/editing/Gr... File Source/core/editing/GranularityStrategy.h (right): https://codereview.chromium.org/1123563003/diff/280001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.h:114: IntSize m_diffExtentPointFromExtentPosition; I don't love this name, but it's better.
> Switch your local methods over to just being marked static instead of anonymous > namespace and I think we can go ahead and get this landed. Yay! https://codereview.chromium.org/1123563003/diff/280001/Source/core/editing/Gr... File Source/core/editing/GranularityStrategy.cpp (right): https://codereview.chromium.org/1123563003/diff/280001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.cpp:12: namespace { On 2015/06/08 18:09:56, leviw wrote: > We just mark these as static in Blink and don't bother with anonymous namespace. Done. https://codereview.chromium.org/1123563003/diff/280001/Source/core/editing/Gr... File Source/core/editing/GranularityStrategy.h (right): https://codereview.chromium.org/1123563003/diff/280001/Source/core/editing/Gr... Source/core/editing/GranularityStrategy.h:114: IntSize m_diffExtentPointFromExtentPosition; On 2015/06/08 18:09:57, leviw wrote: > I don't love this name, but it's better. Yeah, I don't love it either, but I can't come up with anything better ATM unfortunately :/ It's a bit of a struggle because we already have one "offset".. There should be a better way to handle these two offsets together. I will think more about this and address it in a future CL.
LGTM. Thanks for putting up with all the rounds :)
mfomitchev@chromium.org changed reviewers: + aelias@chromium.org
@leviw: Thanks for the review! aelias@chromium.org: Can you please review changes in WebLocalFrameImpl.cpp
Source/web lgtm
The CQ bit was checked by mfomitchev@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1123563003/300001
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as https://src.chromium.org/viewvc/blink?view=rev&revision=196772 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
