Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
988023005:
Implementing directional selection strategy in Blink. (Closed)
|
Created:
5 years, 9 months ago by mfomitchev Modified:
5 years, 8 months ago Reviewers:
yoichio, sgurun-gerrit only, 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. |
DescriptionImplementing directional selection strategy in Blink.
This lays the groundwork for supporting several "strategies" for how selection changes in response to moveRangeSelectionExtent(), which is used by Chrome when touch selection handles are dragged. In addition to the default strategy, which is the old behavior, this also implements direction-based strategy which is basically "expand by word, shrink by character". The plan is to hook this up to a Chrome flag (see https://codereview.chromium.org/1006003003) so that we have an option of doing A/B testing with Finch to compare different strategies.
BUG=451255
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194339
Patch Set 1 #
Total comments: 3
Patch Set 2 : Adding WebSettings support, addressing nits, general cleanup. #Patch Set 3 : Expanded a couple of comments. #
Total comments: 6
Patch Set 4 : Addressing review feedback. #
Total comments: 19
Patch Set 5 : Addressing some of the review feedback. #Patch Set 6 : Addressing feedback. #
Total comments: 32
Patch Set 7 : Addressing review feedback. #Patch Set 8 : Missing file. #
Total comments: 21
Patch Set 9 : Addressing feedback. #Patch Set 10 : Putting GranularityStrategy into separate files. #
Total comments: 18
Patch Set 11 : Addressing feedback. #
Total comments: 16
Patch Set 12 : Addressing nits #
Messages
Total messages: 58 (9 generated)
mfomitchev@chromium.org changed reviewers: + jdduke@chromium.org, yoichio@chromium.org, yosin@chromium.org
Concept of selection strategy is nice if we can it also covers things done in blink EventHandler. In blink EventHandler, we have number of functions which have higher level concept not fit onto layer of EventHandler, I think EventHandler should be low level thing: - selectClosestWordOrLinkFromMouseEvent - expandSelectionToRespectUserSelectAll - updateSelectionForMouseDownDispatchingSelectStart - selectClosestWordFromHitTestResult - selectClosestMisspellingFromHitTestResult - ... and more ... In other words, I would like decuple EventHandler from editing, and connect EventHandler and FrameSeleciton with small set API. It would be grate selection strategy covers mouse and touch. https://codereview.chromium.org/988023005/diff/1/Source/core/editing/FrameSel... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/988023005/diff/1/Source/core/editing/FrameSel... Source/core/editing/FrameSelection.cpp:164: const VisiblePosition& base = oldSel.isBaseFirst() ? oldSel.visibleStart() : oldSel.visibleEnd(); nit: Just use |visibleBase()|. https://codereview.chromium.org/988023005/diff/1/Source/core/editing/FrameSel... Source/core/editing/FrameSelection.cpp:165: const VisiblePosition& oldExtent = oldSel.isBaseFirst() ? oldSel.visibleEnd() : oldSel.visibleStart(); nit: Just use |visibleExtent()|. https://codereview.chromium.org/988023005/diff/1/Source/core/editing/FrameSel... Source/core/editing/FrameSelection.cpp:2055: const VisiblePosition base = m_selection.isBaseFirst() ? nit: Just use |visibleBase()|.
Thanks for the quick response! My intention was to make GranularityStrategy an internal class in FrameSelection, just to encapsulate the logic for how the selection grows when moveRangeSelectionExtent is called (i.e. when a handle is being dragged). If you think it would make sense to expand it's mandate to cover other interactions, I am happy to hear that. Does this mean that you are okay with the general approach in this CL? Do you think I need to change anything in the design to make it possible to add new functionality to GranularityStrategy later? If you think overall this is on the right track, I can clean this up and send out a real review. One comment: My thinking with GranularityStrategy was that it would be set on startup and then used indefinitely. Are you thinking along the same lines, or do you think we need to change the selection strategy depending on the context (mouse/touch/etc)?
On 2015/03/10 02:57:02, mfomitchev wrote: > Do you think I need to change anything in the design to make it possible to add new functionality to GranularityStrategy > later? At this time, not sure. I think we may need to add some for other devices. > If you think overall this is on the right track, I can clean this up and > send out a real review. Please go ahead. (^_^)b > One comment: My thinking with GranularityStrategy was that it would be set on > startup and then used indefinitely. Are you thinking along the same lines, or do > you think we need to change the selection strategy depending on the context > (mouse/touch/etc)? I think this should be runtime configurable to support device docking, e.g. attach keyboard and mice to smart phone/tablet, MS surface, and multiple device, Chrome OS Pixel.
I do not understand the purpose. If GranularityStrategy is used only in moveRangeSelectionExtent, you just add as an argument. Anything that we can not achieve with current moveRangeSelectionExtent and this patch enables? Is not it enough to call moveRangeSelectionExtent with appropriate granularity?
On 2015/03/10 06:15:39, yoichio wrote: > I do not understand the purpose. > > Anything that we can not achieve with current moveRangeSelectionExtent and this > patch enables? > Is not it enough to call moveRangeSelectionExtent with appropriate granularity? Unfortunately no. The problem is that we don't always know the word boundaries on the browser side, so we don't always know when to switch from character granularity to word granularity. E.g. consider the case when we start from a fairly big selection spawning a couple of sentences. We shrink the selection by a few words (so we switch to character granularity), and then we start expanding the selection. We won't know when to switch to word granularity, because we don't know where the word boundaries are. Initially I thought we may be able to solve this by passing more information down to Chrome from Blink, but after discussing with jdduke@ I am pretty convinced that would be an uphill battle. Things like transforms, mixed RTL/LTR, JS changing the text, etc would all be a pain to deal with. Trust me, if there was a good way to do it on the browser side, that would be my preference, since I am much more comfortable with the Chrome code myself (plus I've already implemented most of that in the prototypes work: https://codereview.chromium.org/903143003, https://codereview.chromium.org/829913004). > If GranularityStrategy is used only in moveRangeSelectionExtent, you just add as > an argument. It would be difficult to deal with granularity strategy changing at arbitrary points of time. That would basically mean that we'd need to keep all the state information for all the granularity strategies updated at all times, which would be kind of wasteful. There's no reason to add this sort of complexity at this point IMO - setting the granularity once on startup satisfies our needs right now.
Your intention is modifying selection granularity considering selection history, right?
On 2015/03/11 02:00:43, yoichio wrote: > Your intention is modifying selection granularity considering selection history, > right? Selection context. If everything goes right, I don't believe we need to consider the selection history, as the implementation should be able to determine whether the selection is contracting/expanding given the current state and the new desired extent. Correct me if I'm wrong, Mikhail.
Correct. Roughly: expand by word, contract by character. The caveat is that once you contract and then start expanding - the switch to word granularity should occur only once you cross the word boundary (until then you stay in char granularity).
I got your idea and almost looks good:) Dragging selection is classified to 4 cases: 1. Expand in word 2. Expand beyond word 3. Contract in word 4. Contract beyond word We can decide these selection behavior considering current |m_selection| and the argument |extentPosition|. Since Selection is modified with granularity, we represents Selection range with |VisibleSelection::start/end|, not |base/extend|. For each case, we should consider previous granularity(let me say them Char and Word shortly). 1. Expand in word - 1-a. If previous is by Char, select by Char - 1-b. If previous is by Word, select by Word 2. Expand beyond word - 2-a. If previous is by Char, select by Word - 2-b. If previous is by Word, select by Word 3. Contract in word - 3-a. If previous is by Char, select by Char - 3-b. If previous is by Word, select by Char 4. Contract beyond word - 4-a. If previous is by Char, select by Char - 4-b. If previous is by Word, select by Char Thus (only) case 2-a and 2-b depend on previous granularity. You can implement Strategy with only granularity state.
On 2015/03/12 04:48:05, yoichio wrote: > I got your idea and almost looks good:) > > Dragging selection is classified to 4 cases: > 1. Expand in word > 2. Expand beyond word > 3. Contract in word > 4. Contract beyond word > We can decide these selection behavior considering current |m_selection| and the > argument |extentPosition|. > Since Selection is modified with granularity, we represents Selection range with > |VisibleSelection::start/end|, not |base/extend|. > > For each case, we should consider previous granularity(let me say them Char and > Word shortly). > 1. Expand in word > - 1-a. If previous is by Char, select by Char > - 1-b. If previous is by Word, select by Word > 2. Expand beyond word > - 2-a. If previous is by Char, select by Word > - 2-b. If previous is by Word, select by Word > 3. Contract in word > - 3-a. If previous is by Char, select by Char > - 3-b. If previous is by Word, select by Char > 4. Contract beyond word > - 4-a. If previous is by Char, select by Char > - 4-b. If previous is by Word, select by Char > > Thus (only) case 2-a and 2-b depend on previous granularity. > > You can implement Strategy with only granularity state. I think we need a bit more info than the granularity state: - To determine whether the selection is contracting, we need the cached extentPosition (m_extentPosition) from the previous call to moveRangeSelectionExtent. We can't just look at the selection, because it might be extending up to the end of the word, so that won't tell me enough. - m_wordBoundary is a bit more tricky. It would seem we could just look at whether there is a word boundary between the old extent position (m_extentPosition) and the new extent position (extentPosition passed in moveRangeSelectionExtent). However there is an edge case when this breaks down. Normally if the extent is on the word boundary in char granularity and we extend the selection further beyond the word boundary - we should switch to word granularity. There's one situation when that's not the case - it is if we've shrunk the selection all the way to the beginning of the word and then extend it from there. We are still on the word boundary and extending selection beyond, but in this case we should stay in char granularity. This is why we also keep track of m_wordBoundary.
> - To determine whether the selection is contracting, we need the cached > extentPosition (m_extentPosition) from the previous call to > moveRangeSelectionExtent. We can't just look at the selection, because it might > be extending up to the end of the word, so that won't tell me enough. > VisibleSelection has 4 Positions about Selection: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... VisibleSelection::VisibleSelection(Position, Position) sets Base == Start and Extent == End (if you select backwardly, Start and End switch to another). Selected text is decided by Start and End, which can be different value from Base/Extent. What you need to do is - set selection range to m_selection.start/end - set |extentPosition| to m_selection.extent and use the value as previous handle position. You might need to code VisibleSelection to achieve that. > - m_wordBoundary is a bit more tricky. It would seem we could just look at > whether there is a word boundary between the old extent position > (m_extentPosition) and the new extent position (extentPosition passed in > moveRangeSelectionExtent). However there is an edge case when this breaks down. > Normally if the extent is on the word boundary in char granularity and we extend > the selection further beyond the word boundary - we should switch to word > granularity. There's one situation when that's not the case - it is if we've > shrunk the selection all the way to the beginning of the word and then extend it > from there. We are still on the word boundary and extending selection beyond, > but in this case we should stay in char granularity. This is why we also keep > track of m_wordBoundary. I see. If we select "ab|c> def", "ab|c >def", and then "ab|c d>ef", we should take Word granularity but shrink "ab|c >def" then "ab|c d>ef" should be Char. right? It sounds hysteresis... However, using m_wordBoundary looks little bit strange. How about to split the granularity state to ExtendingByChar, ShrinkingByChar and ExtendingWord instead? When you go beyond a boundary, if previous is ExtendingByChar, go with ExtendingWord and if previous is ShrinkingByChar, go ExtendingByChar.
On 2015/03/13 07:28:27, yoichio wrote: > > - To determine whether the selection is contracting, we need the cached > > extentPosition (m_extentPosition) from the previous call to > > moveRangeSelectionExtent. We can't just look at the selection, because it > might > > be extending up to the end of the word, so that won't tell me enough. > > > > VisibleSelection has 4 Positions about Selection: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > VisibleSelection::VisibleSelection(Position, Position) sets Base == Start and > Extent == End (if you select backwardly, Start and End switch to another). > Selected text is decided by Start and End, which can be different value from > Base/Extent. > What you need to do is > - set selection range to m_selection.start/end > - set |extentPosition| to m_selection.extent and use the value as previous > handle position. > You might need to code VisibleSelection to achieve that. I see. So if I understand correctly, you are suggesting we leverage the difference between start/end vs base/extent in VisibleSelection and then we don't have to store m_extentPosition in the strategy, we can just use extent from VisibleSelection? And we'd switch between word and char granularity when calling setSelection instead of always using char granularity as we do now? Now, the desired behavior is such that we may have the base in the middle of the word, and it should stay there even when selection expands by word. The problem is that there seems to be no way to apply the granularity in VisibleSelection to just one end of the selection. As far as I can see, the granularity is always applied to both start and end, which is not what we want. So I think we'd have to add new functionality to VisibleSelection's API to satisfy our needs here, and I am not sure if it's a good idea? The requirements may still change, and we will likely want to experiment with a few different strategies, so changing VisibleSelection's API seems a bit heavy handed.. unless of course these changes would be useful elsewhere? > > > - m_wordBoundary is a bit more tricky. It would seem we could just look at > > whether there is a word boundary between the old extent position > > (m_extentPosition) and the new extent position (extentPosition passed in > > moveRangeSelectionExtent). However there is an edge case when this breaks > down. > > Normally if the extent is on the word boundary in char granularity and we > extend > > the selection further beyond the word boundary - we should switch to word > > granularity. There's one situation when that's not the case - it is if we've > > shrunk the selection all the way to the beginning of the word and then extend > it > > from there. We are still on the word boundary and extending selection beyond, > > but in this case we should stay in char granularity. This is why we also keep > > track of m_wordBoundary. > I see. If we select "ab|c> def", "ab|c >def", and then "ab|c d>ef", we should > take Word granularity but > shrink "ab|c >def" then "ab|c d>ef" should be Char. right? Yes, pretty much. In the second case we shrink "ab|c d>ef" to "ab|c >def" and then expand back to "ab|c d>ef". The idea is that if you are in char granularity - you can move within the same word (including bounds) however you want, and you still stay in char granularity. This is not something that is obviously better from looking at the code, but it does feel more natural when you actually play with this on a device. m_wordBoundary basically track the "current word". > It sounds hysteresis... > However, using m_wordBoundary looks little bit strange. > How about to split the granularity state to ExtendingByChar, ShrinkingByChar and > ExtendingWord instead? > When you go beyond a boundary, if previous is ExtendingByChar, go with > ExtendingWord > and if previous is ShrinkingByChar, go ExtendingByChar. Yeah, something like that will probably work, but that would impact everybody who uses TextGranularity rather than keeping the impact constrained to moveRangeSelectionExtent/granularity strategy. E.g. if we don't plumb this through for mouse, that would seem inconsistent.. and if we do - we'd end up doing work which is unnecessary at this point. It would also seems inconsistent that we have extending/shrinking states for char, but not for all the other granularity levels. Finally, I am not sure if it would be easy to tell if the selection is extending or shrinking in some cases - e.g. when selection is manipulated from JS.. We could do something similar just within DirectionGranularityStrategy - we could keep track of whether the selection has shrunk or expanded in the previous 'iteration'. However that seems less natural to me and would likely be more complicated in terms of code.. What really matters is whether we stay within the same word or not (and not what the previous action was), so keeping track of the current word (via the word boundary or via other means) seems to make the most sense.
I cleaned up the patch and added WebSettings support. Please take another look.
> I see. So if I understand correctly, you are suggesting we leverage the > difference between start/end vs base/extent in VisibleSelection and then we > don't have to store m_extentPosition in the strategy, we can just use extent > from VisibleSelection? Yes you can. > And we'd switch between word and char granularity when > calling setSelection instead of always using char granularity as we do now? Yes, you should set appropriate granularity to setSelection but it is not used to modify selection but to notify selection intent. > Now, > the desired behavior is such that we may have the base in the middle of the > word, and it should stay there even when selection expands by word. The problem > is that there seems to be no way to apply the granularity in VisibleSelection to > just one end of the selection. As far as I can see, the granularity is always > applied to both start and end, which is not what we want. > So I think we'd have > to add new functionality to VisibleSelection's API to satisfy our needs here, > and I am not sure if it's a good idea? The requirements may still change, and we > will likely want to experiment with a few different strategies, so changing > VisibleSelection's API seems a bit heavy handed.. unless of course these changes > would be useful elsewhere? You would create new function setWithoutValidation(base, extent, start, end, granularity): https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... I guess this direct insertion would help your another strategy. > We could do something similar just within DirectionGranularityStrategy - we > could keep track of whether the selection has shrunk or expanded in the previous > 'iteration'. This is what I suggest.
https://codereview.chromium.org/988023005/diff/40001/Source/core/editing/Fram... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/988023005/diff/40001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:100: virtual VisiblePosition moveRangeSelectionExtent(const VisiblePosition&, FrameSelection*); Could you use |const VisibleSelection& currentSelection| instead of FrameSelection*? https://codereview.chromium.org/988023005/diff/40001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:138: VisiblePosition m_wordBoundary; Please initialize these variables. https://codereview.chromium.org/988023005/diff/40001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:198: m_extentPosition = extentPosition; I guess we need to update m_wordBoundary, m_granularity and m_extentPosition at most once but code flow doesn't looks.
On 2015/03/16 04:49:16, yoichio wrote: > > I see. So if I understand correctly, you are suggesting we leverage the > > difference between start/end vs base/extent in VisibleSelection and then we > > don't have to store m_extentPosition in the strategy, we can just use extent > > from VisibleSelection? > Yes you can. > > > And we'd switch between word and char granularity when > > calling setSelection instead of always using char granularity as we do now? > Yes, you should set appropriate granularity to setSelection but it is not used > to modify selection but to notify selection intent. > > > Now, > > the desired behavior is such that we may have the base in the middle of the > > word, and it should stay there even when selection expands by word. The > problem > > is that there seems to be no way to apply the granularity in VisibleSelection > to > > just one end of the selection. As far as I can see, the granularity is always > > applied to both start and end, which is not what we want. > > So I think we'd have > > to add new functionality to VisibleSelection's API to satisfy our needs here, > > and I am not sure if it's a good idea? The requirements may still change, and > we > > will likely want to experiment with a few different strategies, so changing > > VisibleSelection's API seems a bit heavy handed.. unless of course these > changes > > would be useful elsewhere? > You would create new function setWithoutValidation(base, extent, start, end, > granularity): > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > I guess this direct insertion would help your another strategy. There's a FIXME next to setWithoutValidation saying that doing this is basically a hack and we only do it because we sometimes use VisibleSelection to store values for a potential 'Undo' operation rather than use it to represent an active selection. In our case this does represent an active selection, so breaking the invariant seems wrong.. https://codereview.chromium.org/988023005/diff/40001/Source/core/editing/Fram... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/988023005/diff/40001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:100: virtual VisiblePosition moveRangeSelectionExtent(const VisiblePosition&, FrameSelection*); On 2015/03/16 04:49:25, yoichio wrote: > Could you use |const VisibleSelection& currentSelection| instead of > FrameSelection*? Done. https://codereview.chromium.org/988023005/diff/40001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:138: VisiblePosition m_wordBoundary; On 2015/03/16 04:49:25, yoichio wrote: > Please initialize these variables. We do initialize m_granularity in the constructor. Are you suggesting we also initialize m_wordBoundary and m_extentPosition? Default constructor works just fine for them, and we don't usually initialize object fields explicitly when the default constructor does the job - e.g. m_originalBase, m_selection in FrameSelection. https://codereview.chromium.org/988023005/diff/40001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:198: m_extentPosition = extentPosition; On 2015/03/16 04:49:25, yoichio wrote: > I guess we need to update m_wordBoundary, m_granularity and m_extentPosition at > most once > but code flow doesn't looks. Ok, updated the code so that we only do it once. See if you like the new structure better.
> There's a FIXME next to setWithoutValidation saying that doing this is basically > a hack and we only do it because we sometimes use VisibleSelection to store > values for a potential 'Undo' operation rather than use it to represent an > active selection. In our case this does represent an active selection, so > breaking the invariant seems wrong.. > Breaking invariant means you call setWithoutValidation after initialize VisibleSelection with base/extend. This comment seems old because setWithoutValidation is used to set actual selection. My concern is that is it OK to set extend which differ from actual user extend position? yosin@, WDYT? > > https://codereview.chromium.org/988023005/diff/40001/Source/core/editing/Fram... n.cpp#newcode138 > Source/core/editing/FrameSelection.cpp:138: VisiblePosition m_wordBoundary; > On 2015/03/16 04:49:25, yoichio wrote: > > Please initialize these variables. > > We do initialize m_granularity in the constructor. Are you suggesting we also > initialize m_wordBoundary and m_extentPosition? Default constructor works just > fine for them, and we don't usually initialize object fields explicitly when the > default constructor does the job - e.g. m_originalBase, m_selection in > FrameSelection. I see. https://codereview.chromium.org/988023005/diff/60001/Source/core/editing/Fram... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/988023005/diff/60001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:167: int extentBaseOrder = comparePositions(extentPosition, base); I wonder why this function looks procedural. It is hard to catch full order of each position in this code. First, you can use VisibleSelection::isBaseFirst() to detect current direction. Let me assume direction is right. If we have string "abc_def_hgi", we should have "ab|c_d>ef\_hgi" ('|' is base, '>' is old extent and '\' is m_wordBoundary). Then we have 4 cases - |extentPosition| is before base - |extentPosition| is between base and oldExtent - |extentPosition| is between oldExtent and m_wordBoundary - |extentPosition| is after m_wordBoundary I miss on-boundary condition but it can be 4 cases. Does it make sense? https://codereview.chromium.org/988023005/diff/60001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:176: if (prevWordBoundary.isNull()) { We can early return in this case.
On 2015/03/17 05:54:41, yoichio wrote: > > There's a FIXME next to setWithoutValidation saying that doing this is > basically > > a hack and we only do it because we sometimes use VisibleSelection to store > > values for a potential 'Undo' operation rather than use it to represent an > > active selection. In our case this does represent an active selection, so > > breaking the invariant seems wrong.. > > > Breaking invariant means you call setWithoutValidation after initialize > VisibleSelection with > base/extend. > This comment seems old because setWithoutValidation is used to set actual > selection. > My concern is that is it OK to set extend which differ from actual user extend > position? > yosin@, WDYT? If the comment is obsolete and it's okay to set start/end/base/extent on VisibleSelection which don't correspond to any "standard" granularity, then we can do this. yosin@ - would love to hear your take on this. https://codereview.chromium.org/988023005/diff/60001/Source/core/editing/Fram... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/988023005/diff/60001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:167: int extentBaseOrder = comparePositions(extentPosition, base); On 2015/03/17 05:54:41, yoichio wrote: > I wonder why this function looks procedural. > It is hard to catch full order of each position in this code. > First, you can use VisibleSelection::isBaseFirst() to detect current direction. > Let me assume direction is right. > If we have string "abc_def_hgi", we should have "ab|c_d>ef\_hgi" ('|' is base, > '>' is old extent and '\' is m_wordBoundary). > Then we have 4 cases > - |extentPosition| is before base > - |extentPosition| is between base and oldExtent > - |extentPosition| is between oldExtent and m_wordBoundary > - |extentPosition| is after m_wordBoundary > I miss on-boundary condition but it can be 4 cases. Does it make sense? Ok, lets enumerate your cases one to four from top to bottom. In case (1) (extentPosition is before base) we also need to recalculate the word boundary, i.e. calculate the word boundary on the "other" side of the base. This is the case when extentBaseOrderSwitched == true (lines 184-187). Then this breaks up into two subcases: when extentPosition crosses the updated word boundary, and when it doesn't. Lets call this 1.1 and 1.2 respectively. Now, we don't actually handle these two subcases separately: once the word boundary is recalculated and the granularity is reset to char (lines 184-187), handling of (1.1) and (1.2) is merged with handling of cases cases (2) and (4) as we'll see below. Cases (4) and (1.1) are handled in lines 194-197. Cases (2) and (1.2) is lines 198-204. Note that 1.2 is basically a no-op - we've already reset the granularity to char, so we don't need to do anything. The remaining case is case (3) - we don't need to change anything other than save the new extent position (which is always done in line 207), so no special code for it. Makes sense? https://codereview.chromium.org/988023005/diff/60001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:176: if (prevWordBoundary.isNull()) { We can't actually. extentPosition can still be anywhere relative to prevWordBoundary and oldExtent, so we need to do the full analysis as always.
https://codereview.chromium.org/988023005/diff/60001/Source/core/editing/Fram... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/988023005/diff/60001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:100: virtual VisiblePosition moveRangeSelectionExtent(const VisiblePosition&, const VisibleSelection&); Since, |moveRangeSelectionExtent()| returns position rather than changing position in selection. We could name |granularExtent()| or better explaining return value. https://codereview.chromium.org/988023005/diff/60001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:138: VisiblePosition m_wordBoundary; |VisiblePosition| isn't update at DOM mutation or style updates. It is dangerous to keep it in object. I recommend to calculate it every time we need to compute extent position. Other options are: - Update |m_wordBoundary| and |m_extentPosition| every DOM mutation and style updates. Note: |m_selection| updates for DOM mutation, see didUpdateCharacterData(), didMergeTextNode(), etc. But, not for style - Use |Range| object. Since, |VisiblePostion| depends on DOM position and Layout tree, it is fragile. We don't recommend to keep in heap. https://codereview.chromium.org/988023005/diff/60001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:167: int extentBaseOrder = comparePositions(extentPosition, base); On 2015/03/17 17:01:16, mfomitchev wrote: > On 2015/03/17 05:54:41, yoichio wrote: > > I wonder why this function looks procedural. > > It is hard to catch full order of each position in this code. > > First, you can use VisibleSelection::isBaseFirst() to detect current > direction. > > Let me assume direction is right. > > If we have string "abc_def_hgi", we should have "ab|c_d>ef\_hgi" ('|' is base, > > '>' is old extent and '\' is m_wordBoundary). > > Then we have 4 cases > > - |extentPosition| is before base > > - |extentPosition| is between base and oldExtent > > - |extentPosition| is between oldExtent and m_wordBoundary > > - |extentPosition| is after m_wordBoundary > > I miss on-boundary condition but it can be 4 cases. Does it make sense? > > Ok, lets enumerate your cases one to four from top to bottom. > In case (1) (extentPosition is before base) we also need to recalculate the word > boundary, i.e. calculate the word boundary on the "other" side of the base. This > is the case when extentBaseOrderSwitched == true (lines 184-187). Then this > breaks up into two subcases: when extentPosition crosses the updated word > boundary, and when it doesn't. Lets call this 1.1 and 1.2 respectively. Now, we > don't actually handle these two subcases separately: once the word boundary is > recalculated and the granularity is reset to char (lines 184-187), handling of > (1.1) and (1.2) is merged with handling of cases cases (2) and (4) as we'll see > below. > > Cases (4) and (1.1) are handled in lines 194-197. Cases (2) and (1.2) is lines > 198-204. Note that 1.2 is basically a no-op - we've already reset the > granularity to char, so we don't need to do anything. > > The remaining case is case (3) - we don't need to change anything other than > save the new extent position (which is always done in line 207), so no special > code for it. > > Makes sense? It is better to ask |VisibleSelection| for DOM order of base/extent. VisibleSelection newSelection(selection); newSelection.setExtent(extentPosition); Then you can use |newSelection.isBaseFirst()| https://codereview.chromium.org/988023005/diff/60001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:179: prevWordBoundary = nextWordBound(oldExtent, extentBaseOrder, false); How about factor out |FrameSelection::modifyXXX()| function to return position rather than modifying selection? Then use here? https://codereview.chromium.org/988023005/diff/60001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:2057: if (isNone() || comparePositions(base, extentPosition) == 0) nit: |base == extentPosision| https://codereview.chromium.org/988023005/diff/60001/Source/core/editing/Fram... File Source/core/editing/FrameSelection.h (right): https://codereview.chromium.org/988023005/diff/60001/Source/core/editing/Fram... Source/core/editing/FrameSelection.h:130: // is set to the same position as the current base, this function will do nothing. nit: Option: It is super nice if this line fit in < 80 chars. https://codereview.chromium.org/988023005/diff/60001/Source/core/editing/Fram... Source/core/editing/FrameSelection.h:279: GranularityStrategy& granularityStrategy(); nit: |const GranularityStrategy|? https://codereview.chromium.org/988023005/diff/60001/Source/core/editing/Fram... Source/core/editing/FrameSelection.h:279: GranularityStrategy& granularityStrategy(); nit: |const| modifier for |granularityStrategy()|?
> On 2015/03/17 05:54:41, yoichio wrote:
> > I wonder why this function looks procedural.
> > It is hard to catch full order of each position in this code.
> > First, you can use VisibleSelection::isBaseFirst() to detect current
> direction.
> > Let me assume direction is right.
> > If we have string "abc_def_hgi", we should have "ab|c_d>ef\_hgi" ('|' is
base,
> > '>' is old extent and '\' is m_wordBoundary).
> > Then we have 4 cases
> > - |extentPosition| is before base
> > - |extentPosition| is between base and oldExtent
> > - |extentPosition| is between oldExtent and m_wordBoundary
> > - |extentPosition| is after m_wordBoundary
> > I miss on-boundary condition but it can be 4 cases. Does it make sense?
>
> Ok, lets enumerate your cases one to four from top to bottom.
> In case (1) (extentPosition is before base) we also need to recalculate the
word
> boundary, i.e. calculate the word boundary on the "other" side of the base.
This
> is the case when extentBaseOrderSwitched == true (lines 184-187). Then this
> breaks up into two subcases: when extentPosition crosses the updated word
> boundary, and when it doesn't. Lets call this 1.1 and 1.2 respectively. Now,
we
> don't actually handle these two subcases separately: once the word boundary is
> recalculated and the granularity is reset to char (lines 184-187), handling of
> (1.1) and (1.2) is merged with handling of cases cases (2) and (4) as we'll
see
> below.
>
> Cases (4) and (1.1) are handled in lines 194-197. Cases (2) and (1.2) is lines
> 198-204. Note that 1.2 is basically a no-op - we've already reset the
> granularity to char, so we don't need to do anything.
>
> The remaining case is case (3) - we don't need to change anything other than
> save the new extent position (which is always done in line 207), so no special
> code for it.
>
> Makes sense?
>
Current code seems working well. I don't doubt code behavior but looking.
I'm considering if people can catch right behavior from code easily.
In my mind, this code should form following:
< comparePositions(base, extent, .. ) >
if (<|extentPosition| is before base>) {}
else if (<|extentPosition| is between base and oldExtent>) {}
else if (<|extentPosition| is between oldExtent and m_wordBoundary>) {}
else { //> > - |extentPosition| is after m_wordBoundary
}
WDYT?
Thanks for the feedback! I addressed some of the comments. I will address the rest of after we resolve the question of what's the best way to account for the possibility of a DOM mutation or style update affecting the cached values. https://codereview.chromium.org/988023005/diff/60001/Source/core/editing/Fram... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/988023005/diff/60001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:100: virtual VisiblePosition moveRangeSelectionExtent(const VisiblePosition&, const VisibleSelection&); On 2015/03/18 03:44:54, Yosi_UTC9 wrote: > Since, |moveRangeSelectionExtent()| returns position rather than changing > position in selection. We could name |granularExtent()| or better explaining > return value. Renamed and expanded on the comment. I wanted to capture the fact that this also updates the internal state of the strategy, so I opted for |updateExtent|. I can change to getGranularExtent or calculateGranularExtent if you prefer. https://codereview.chromium.org/988023005/diff/60001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:138: VisiblePosition m_wordBoundary; Ok. I really only need m_wordBoundary to determine the "current word" when oldExtent is on word boundary. Instead of saving m_wordBoundary I could save whether the "current word" is on the right or on the left of the extent (when extent is on the boundary). Does that sound okay? For m_extentPosition perhaps we can use yoichio's suggestion and add a new function setWithoutValidation(base, extent, start, end) to VisibleSelection and save user extent there, while setting the start/end where we want them to be. I had concerns about doing that since such a selection may not satisfy any of the granularity invariants, but yoichio suggested that may be okay (see the top part of comment #18). What do you think? If we leverage m_selection instead of saving m_extentPosition - are we safe? You mention that m_selection doesn't update for style changes, but if m_selection gets corrupted we are screwed anyway, right? Also, to put this into perspective, it's probably not a big deal if we apply the strategy incorrectly in some weird rare edge case. The worst thing that can happen is that we'll expand the selection with the wrong granularity, which is not the end of the world. And the error should get corrected easily - typically when the user moves the handle again. https://codereview.chromium.org/988023005/diff/60001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:2057: if (isNone() || comparePositions(base, extentPosition) == 0) On 2015/03/18 03:44:54, Yosi_UTC9 wrote: > nit: |base == extentPosision| Done. https://codereview.chromium.org/988023005/diff/60001/Source/core/editing/Fram... File Source/core/editing/FrameSelection.h (right): https://codereview.chromium.org/988023005/diff/60001/Source/core/editing/Fram... Source/core/editing/FrameSelection.h:130: // is set to the same position as the current base, this function will do nothing. On 2015/03/18 03:44:55, Yosi_UTC9 wrote: > nit: Option: It is super nice if this line fit in < 80 chars. Done. https://codereview.chromium.org/988023005/diff/60001/Source/core/editing/Fram... Source/core/editing/FrameSelection.h:279: GranularityStrategy& granularityStrategy(); On 2015/03/18 03:44:55, Yosi_UTC9 wrote: > nit: |const| modifier for |granularityStrategy()|? Is this the same as the comment below? Or are you suggesting we declair as GranularityStrategy& granularityStrategy() const? The latter wouldn't be possible because we modify m_granularityStrategy. https://codereview.chromium.org/988023005/diff/60001/Source/core/editing/Fram... Source/core/editing/FrameSelection.h:279: GranularityStrategy& granularityStrategy(); On 2015/03/18 03:44:55, Yosi_UTC9 wrote: > nit: |const GranularityStrategy|? We'd have to declare all GranularityStrategy as const to do that, which we can't do.. I've changed this to return a constant pointer instead of a reference. We lose the intrinsic non-null guarantee of a reference, but const restriction is probably more important. See if you like it.
@yosin - can you please comment on the questions in https://codereview.chromium.org/988023005/diff/60001/Source/core/editing/Fram... ?
https://codereview.chromium.org/988023005/diff/60001/Source/core/editing/Fram... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/988023005/diff/60001/Source/core/editing/Fram... Source/core/editing/FrameSelection.cpp:138: VisiblePosition m_wordBoundary; On 2015/03/19 01:24:39, mfomitchev wrote: > Ok. I really only need m_wordBoundary to determine the "current word" when > oldExtent is on word boundary. Instead of saving m_wordBoundary I could save > whether the "current word" is on the right or on the left of the extent (when > extent is on the boundary). Does that sound okay? > > For m_extentPosition perhaps we can use yoichio's suggestion and add a new > function setWithoutValidation(base, extent, start, end) to VisibleSelection and > save user extent there, while setting the start/end where we want them to be. I > had concerns about doing that since such a selection may not satisfy any of the > granularity invariants, but yoichio suggested that may be okay (see the top part > of comment #18). What do you think? If we leverage m_selection instead of saving > m_extentPosition - are we safe? You mention that m_selection doesn't update for > style changes, but if m_selection gets corrupted we are screwed anyway, right? > > Also, to put this into perspective, it's probably not a big deal if we apply the > strategy incorrectly in some weird rare edge case. The worst thing that can > happen is that we'll expand the selection with the wrong granularity, which is > not the end of the world. And the error should get corrected easily - typically > when the user moves the handle again. Saving |Position|, |VisibleSelection|, |VisiblePosition| is unsafe. Only |Range| object is safe after mutation. If you want to remember |VisiblePosition|, using |Range| then compute |VisiblePosition| at use. Or, relocate |VisiblePosition| in strategy object for each mutation.
Based on yosin's feedback I am now using setEndRespectingGranularity/setStartRespectingGranularity to avoid caching the extentPosition in the strategy object. I've also replaced m_wordBoundary with m_lastMoveShrunkSelection and simplified the code. Can you please take another look?
Also please note this relies on https://codereview.chromium.org/1089283002/, which I chose to submit as a separate patch.
mfomitchev@chromium.org changed reviewers: + rbyers@chromium.org
Could you try to avoid use term "default"? It is better to name current behavior to something. https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Fra... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:134: VisiblePosition nextWordBound(const VisiblePosition& /*pos*/, int /*direction*/, bool /*nextIfOnBound*/); nit: Could you use enum for |nextIfOnBound|? In Blink, we prefer enum over bool for parameter. https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:134: VisiblePosition nextWordBound(const VisiblePosition& /*pos*/, int /*direction*/, bool /*nextIfOnBound*/); nit: It is better to use |SelectionDirection|, defined in "VisibleSelection.h", rather than |int|. https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:167: const VisiblePosition& base = selection.visibleBase(); nit: It is better to use |VisiblePostion| as value rather than reference, since |VisibleSelection::visibleBase()| returns |VisiblePosition| as value. https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:172: ASSERT(extentBaseOrder != 0); I'm not sure this assertion is always satisfied. https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:174: bool extentBaseOrderSwitched = extentBaseOrder * oldExtentBaseOrder < 0; This is too smart conditional expression. Could you write explicitly? https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:183: bool thisMoveShrunkSelection = extentBaseOrder * comparePositions(extentPosition, selection.visibleExtent()) < 0; ditto as L174. https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:194: bool expandedBeyondWordBoundary = extentBaseOrder > 0 ? comparePositions(extentPosition, currentWordBoundary) > 0 This conditional expression looks complex. It is better to use if-statement or introducing named function. https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:2040: GranularityStrategy* const FrameSelection::granularityStrategy() nit: omit |const| https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Tex... File Source/core/editing/TextGranularity.h (right): https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Tex... Source/core/editing/TextGranularity.h:45: enum SelectionStrategy { nit: Please move |SelectionStrategy| to its own file. https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Vis... File Source/core/editing/VisibleSelection.cpp (right): https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Vis... Source/core/editing/VisibleSelection.cpp:352: void VisibleSelection::setStartRespectingGranularity(TextGranularity granularity, EWordSide defaultWordSide) I feel strange that using term default for parameter name. This function moves start to specified side of word. https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Vis... File Source/core/editing/VisibleSelection.h (right): https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Vis... Source/core/editing/VisibleSelection.h:187: && a.isDirectional() == b.isDirectional() && a.base() == b.base() && a.extent() == b.extent(); Note: I assume this change will be kicked out from this patch, since another patch has this change.
https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Fra... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:2045: // right in the constructor - the correct settings may not be set yet. and you're sure granularityStrategy won't ever be called too early? Is it worth at least adding an ASSERT above to ensure the type you're returning still matches the current value of the setting (verifying nobody will try to change the setting for the lifetime of the frame)? Or do you just want to support the strategy changing with the setting? https://codereview.chromium.org/988023005/diff/100001/public/web/WebLocalFrame.h File public/web/WebLocalFrame.h (left): https://codereview.chromium.org/988023005/diff/100001/public/web/WebLocalFram... public/web/WebLocalFrame.h:123: virtual void moveRangeSelectionExtent(const WebPoint&, TextGranularity = CharacterGranularity) = 0; Are there already no callers in chromium that supply this? Obviously we can't land this blink API change until no-one is depending on it. https://codereview.chromium.org/988023005/diff/100001/public/web/WebSettings.h File public/web/WebSettings.h (right): https://codereview.chromium.org/988023005/diff/100001/public/web/WebSettings.... public/web/WebSettings.h:96: enum SelectionStrategyType { Since this is the blink public API, please add good comments here describing these different modes.
mfomitchev@google.com changed reviewers: + mfomitchev@google.com
https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Fra... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:2045: // right in the constructor - the correct settings may not be set yet. Yeah, that's a good point. I don't really understand the lifecycle here very well, so I am not sure. I think it's best to just re-init m_granularityStrategy to the proper type here if there's a discrepancy... Also, we don't need to init m_granularityStrategy for Clear() and SetSelection(), so I can make it so granularityStrategy() is only used by moveRangeSelectionExtent, which should decrease the likelihood of this called too soon. Does this sound good? https://codereview.chromium.org/988023005/diff/100001/public/web/WebLocalFrame.h File public/web/WebLocalFrame.h (left): https://codereview.chromium.org/988023005/diff/100001/public/web/WebLocalFram... public/web/WebLocalFrame.h:123: virtual void moveRangeSelectionExtent(const WebPoint&, TextGranularity = CharacterGranularity) = 0; Nope. This parameter was just added by yoichio recently per my request when we were still trying to implement the strategy in chrome. I have a patch that plumbs through the IPC for it, but it never landed. So InputMsg_MoveRangeSelectionExtent only takes one parameter. https://codereview.chromium.org/988023005/diff/100001/public/web/WebSettings.h File public/web/WebSettings.h (right): https://codereview.chromium.org/988023005/diff/100001/public/web/WebSettings.... public/web/WebSettings.h:96: enum SelectionStrategyType { On 2015/04/16 19:09:48, Rick Byers wrote: > Since this is the blink public API, please add good comments here describing > these different modes. Acknowledged.
https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Fra... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:2045: // right in the constructor - the correct settings may not be set yet. On 2015/04/16 19:33:00, mfomitchev1 wrote: > Yeah, that's a good point. I don't really understand the lifecycle here very > well, so I am not sure. I think it's best to just re-init m_granularityStrategy > to the proper type here if there's a discrepancy... Also, we don't need to init > m_granularityStrategy for Clear() and SetSelection(), so I can make it so > granularityStrategy() is only used by moveRangeSelectionExtent, which should > decrease the likelihood of this called too soon. Does this sound good? Sure, sounds good. https://codereview.chromium.org/988023005/diff/100001/public/web/WebLocalFrame.h File public/web/WebLocalFrame.h (left): https://codereview.chromium.org/988023005/diff/100001/public/web/WebLocalFram... public/web/WebLocalFrame.h:123: virtual void moveRangeSelectionExtent(const WebPoint&, TextGranularity = CharacterGranularity) = 0; On 2015/04/16 19:33:00, mfomitchev1 wrote: > Nope. This parameter was just added by yoichio recently per my request when we > were still trying to implement the strategy in chrome. I have a patch that > plumbs through the IPC for it, but it never landed. So > InputMsg_MoveRangeSelectionExtent only takes one parameter. Acknowledged.
https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Fra... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:134: VisiblePosition nextWordBound(const VisiblePosition& /*pos*/, int /*direction*/, bool /*nextIfOnBound*/); On 2015/04/16 04:11:26, Yosi_UTC9 wrote: > nit: Could you use enum for |nextIfOnBound|? In Blink, we prefer enum over bool > for parameter. Done. https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:134: VisiblePosition nextWordBound(const VisiblePosition& /*pos*/, int /*direction*/, bool /*nextIfOnBound*/); On 2015/04/16 04:11:26, Yosi_UTC9 wrote: > nit: Could you use enum for |nextIfOnBound|? In Blink, we prefer enum over bool > for parameter. I created a local ESearchDirection instead. SelectionDirection ahs 4 values, and we only need two (DirectionForward, DirectionBackward). DirectionRight/DirectionLeft are also non-trivial to support, since we don't have RTL information readily available from the strategy object. I think supporting them would make the code more convoluted. https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:167: const VisiblePosition& base = selection.visibleBase(); On 2015/04/16 04:11:26, Yosi_UTC9 wrote: > nit: It is better to use |VisiblePostion| as value rather than reference, since > |VisibleSelection::visibleBase()| returns |VisiblePosition| as value. Done. https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:172: ASSERT(extentBaseOrder != 0); On 2015/04/16 04:11:26, Yosi_UTC9 wrote: > I'm not sure this assertion is always satisfied. I think it is - we short-circuit in FrameSelection::moveRangeSelectionExtent if base == extentPosition. That said, the algorithm should work fine regardless, so I have removed the ASSERT. https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:174: bool extentBaseOrderSwitched = extentBaseOrder * oldExtentBaseOrder < 0; On 2015/04/16 04:11:26, Yosi_UTC9 wrote: > This is too smart conditional expression. Could you write explicitly? Done. https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:183: bool thisMoveShrunkSelection = extentBaseOrder * comparePositions(extentPosition, selection.visibleExtent()) < 0; On 2015/04/16 04:11:26, Yosi_UTC9 wrote: > ditto as L174. Done. https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:194: bool expandedBeyondWordBoundary = extentBaseOrder > 0 ? comparePositions(extentPosition, currentWordBoundary) > 0 On 2015/04/16 04:11:26, Yosi_UTC9 wrote: > This conditional expression looks complex. It is better to use if-statement or > introducing named function. Done. https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:2040: GranularityStrategy* const FrameSelection::granularityStrategy() On 2015/04/16 04:11:26, Yosi_UTC9 wrote: > nit: omit |const| Done. https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:2045: // right in the constructor - the correct settings may not be set yet. On 2015/04/16 19:35:10, Rick Byers wrote: > On 2015/04/16 19:33:00, mfomitchev1 wrote: > > Yeah, that's a good point. I don't really understand the lifecycle here very > > well, so I am not sure. I think it's best to just re-init > m_granularityStrategy > > to the proper type here if there's a discrepancy... Also, we don't need to > init > > m_granularityStrategy for Clear() and SetSelection(), so I can make it so > > granularityStrategy() is only used by moveRangeSelectionExtent, which should > > decrease the likelihood of this called too soon. Does this sound good? > > Sure, sounds good. Done. https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Tex... File Source/core/editing/TextGranularity.h (right): https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Tex... Source/core/editing/TextGranularity.h:45: enum SelectionStrategy { On 2015/04/16 04:11:26, Yosi_UTC9 wrote: > nit: Please move |SelectionStrategy| to its own file. Done. https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Vis... File Source/core/editing/VisibleSelection.cpp (right): https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Vis... Source/core/editing/VisibleSelection.cpp:352: void VisibleSelection::setStartRespectingGranularity(TextGranularity granularity, EWordSide defaultWordSide) On 2015/04/16 04:11:26, Yosi_UTC9 wrote: > I feel strange that using term default for parameter name. This function moves > start to specified side of word. Removed 'default' https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Vis... File Source/core/editing/VisibleSelection.h (right): https://codereview.chromium.org/988023005/diff/100001/Source/core/editing/Vis... Source/core/editing/VisibleSelection.h:187: && a.isDirectional() == b.isDirectional() && a.base() == b.base() && a.extent() == b.extent(); On 2015/04/16 04:11:26, Yosi_UTC9 wrote: > Note: I assume this change will be kicked out from this patch, since another > patch has this change. Done. https://codereview.chromium.org/988023005/diff/100001/public/web/WebSettings.h File public/web/WebSettings.h (right): https://codereview.chromium.org/988023005/diff/100001/public/web/WebSettings.... public/web/WebSettings.h:96: enum SelectionStrategyType { On 2015/04/16 19:09:48, Rick Byers wrote: > Since this is the blink public API, please add good comments here describing > these different modes. Done.
mfomitchev@chromium.org changed reviewers: + sgurun@chromium.org - mfomitchev@google.com
I recommend to use "git cl try" before asking review. https://codereview.chromium.org/988023005/diff/140001/Source/core/editing/Fra... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/988023005/diff/140001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:96: class GranularityStrategy { Is it better to move XXXStrategy to another file? FrameSelection.cpp is big 2100+. https://codereview.chromium.org/988023005/diff/140001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:99: virtual SelectionStrategy GetType() const; As of link error, we want to put "= 0". https://codereview.chromium.org/988023005/diff/140001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:100: virtual void Clear(); ditto https://codereview.chromium.org/988023005/diff/140001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:103: virtual VisibleSelection updateExtent(const VisiblePosition& extentPosition, const VisibleSelection&); ditto https://codereview.chromium.org/988023005/diff/140001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:109: class CharacterGranularityStrategy : public GranularityStrategy { nit: "final" https://codereview.chromium.org/988023005/diff/140001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:126: class DirectionGranularityStrategy : public GranularityStrategy { nit: final https://codereview.chromium.org/988023005/diff/140001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:135: enum EWordBoundAdjust {CurrentPosIfOnBound = false, NextBoundIfOnBound = true}; nit: we don't need to assign value for enum fields in this case. https://codereview.chromium.org/988023005/diff/140001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:135: enum EWordBoundAdjust {CurrentPosIfOnBound = false, NextBoundIfOnBound = true}; nit: better to use |enum class|. https://codereview.chromium.org/988023005/diff/140001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:136: enum ESearchDirection {SearchBackwards = 0, SearchForward = 1}; nit: we don't need to assign value for enum fields in this case. https://codereview.chromium.org/988023005/diff/140001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:136: enum ESearchDirection {SearchBackwards = 0, SearchForward = 1}; nit: better to use |enum class|.
https://codereview.chromium.org/988023005/diff/140001/Source/core/editing/Fra... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/988023005/diff/140001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:96: class GranularityStrategy { On 2015/04/17 04:24:50, Yosi_UTC9 wrote: > Is it better to move XXXStrategy to another file? FrameSelection.cpp is big > 2100+. It's your call. The upside is reducing the file size making it easier to navigate inside FrameSelection.cpp. The downside is that we'd need to create extra files, making it harder to navigate through the editing file set. We'd also be opening up GranularityStrategy to be used from elsewhere which we don't necessarily want. If we put it in separate files - would we put all the three strategy classes in the same .h? Or create three separate .h and three separate .cpp? Let me know what you prefer. https://codereview.chromium.org/988023005/diff/140001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:99: virtual SelectionStrategy GetType() const; On 2015/04/17 04:24:50, Yosi_UTC9 wrote: > As of link error, we want to put "= 0". Done. https://codereview.chromium.org/988023005/diff/140001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:100: virtual void Clear(); On 2015/04/17 04:24:50, Yosi_UTC9 wrote: > ditto Done. https://codereview.chromium.org/988023005/diff/140001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:103: virtual VisibleSelection updateExtent(const VisiblePosition& extentPosition, const VisibleSelection&); On 2015/04/17 04:24:50, Yosi_UTC9 wrote: > ditto Done. https://codereview.chromium.org/988023005/diff/140001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:109: class CharacterGranularityStrategy : public GranularityStrategy { On 2015/04/17 04:24:50, Yosi_UTC9 wrote: > nit: "final" Done. https://codereview.chromium.org/988023005/diff/140001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:126: class DirectionGranularityStrategy : public GranularityStrategy { On 2015/04/17 04:24:50, Yosi_UTC9 wrote: > nit: final Done. https://codereview.chromium.org/988023005/diff/140001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:135: enum EWordBoundAdjust {CurrentPosIfOnBound = false, NextBoundIfOnBound = true}; On 2015/04/17 04:24:51, Yosi_UTC9 wrote: > nit: we don't need to assign value for enum fields in this case. Done. https://codereview.chromium.org/988023005/diff/140001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:135: enum EWordBoundAdjust {CurrentPosIfOnBound = false, NextBoundIfOnBound = true}; On 2015/04/17 04:24:51, Yosi_UTC9 wrote: > nit: better to use |enum class|. Neat! Thanks for the tip! https://codereview.chromium.org/988023005/diff/140001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:136: enum ESearchDirection {SearchBackwards = 0, SearchForward = 1}; On 2015/04/17 04:24:50, Yosi_UTC9 wrote: > nit: we don't need to assign value for enum fields in this case. Done. https://codereview.chromium.org/988023005/diff/140001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:136: enum ESearchDirection {SearchBackwards = 0, SearchForward = 1}; On 2015/04/17 04:24:50, Yosi_UTC9 wrote: > nit: better to use |enum class|. Done.
https://codereview.chromium.org/988023005/diff/140001/Source/core/editing/Fra... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/988023005/diff/140001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:96: class GranularityStrategy { On 2015/04/17 14:41:49, mfomitchev wrote: > On 2015/04/17 04:24:50, Yosi_UTC9 wrote: > > Is it better to move XXXStrategy to another file? FrameSelection.cpp is big > > 2100+. > > It's your call. The upside is reducing the file size making it easier to > navigate inside FrameSelection.cpp. The downside is that we'd need to create > extra files, making it harder to navigate through the editing file set. We'd > also be opening up GranularityStrategy to be used from elsewhere which we don't > necessarily want. We've moved classes to their own files for TextIterator, http://crrev.com/824913005 In this case, one class, FullyClippedStateStack, isn't used out side of TextIterator, but it was also moved. In my experience, this change helps us re-factoring of TextIterator. > If we put it in separate files - would we put all the three > strategy classes in the same .h? Or create three separate .h and three separate > .cpp? Let me know what you prefer. One .cpp and .h are enough at this time.
On 2015/04/20 01:25:55, Yosi_UTC9 wrote: > https://codereview.chromium.org/988023005/diff/140001/Source/core/editing/Fra... > File Source/core/editing/FrameSelection.cpp (right): > > https://codereview.chromium.org/988023005/diff/140001/Source/core/editing/Fra... > Source/core/editing/FrameSelection.cpp:96: class GranularityStrategy { > On 2015/04/17 14:41:49, mfomitchev wrote: > > On 2015/04/17 04:24:50, Yosi_UTC9 wrote: > > > Is it better to move XXXStrategy to another file? FrameSelection.cpp is big > > > 2100+. > > > > It's your call. The upside is reducing the file size making it easier to > > navigate inside FrameSelection.cpp. The downside is that we'd need to create > > extra files, making it harder to navigate through the editing file set. We'd > > also be opening up GranularityStrategy to be used from elsewhere which we > don't > > necessarily want. > > We've moved classes to their own files for TextIterator, > http://crrev.com/824913005 > In this case, one class, FullyClippedStateStack, isn't used out side of > TextIterator, > but it was also moved. In my experience, this change helps us re-factoring of > TextIterator. > > > If we put it in separate files - would we put all the three > > strategy classes in the same .h? Or create three separate .h and three > separate > > .cpp? Let me know what you prefer. > One .cpp and .h are enough at this time. Done. PTAL
Almost looks good. https://codereview.chromium.org/988023005/diff/180001/Source/core/editing/Gra... File Source/core/editing/GranularityStrategy.cpp (right): https://codereview.chromium.org/988023005/diff/180001/Source/core/editing/Gra... Source/core/editing/GranularityStrategy.cpp:1: /* nit: Let's use the short version of licence comment. http://dev.chromium.org/blink/coding-style#TOC-License https://codereview.chromium.org/988023005/diff/180001/Source/core/editing/Gra... Source/core/editing/GranularityStrategy.cpp:58: if (direction == ESearchDirection::SearchForward) Q: How about using if-statement rather than ternary operator? if (direction == ESearchDirection::SearchForward) { if (wordBoundAdjust == EBoundAdjust::CurrentPosIfOnBound) return endOfWord(pos, LeftWordIfOnBoundary); return endOfWord(pos, RightWordIfOnBoundary); } if (wordBoundAdjust == EBoundAdjust::CurrentPosIfOnBound) return startOfWord(pos, RightWordIfOnBoundary); return startOfWord(pos, LeftWordIfOnBoundary); === Fumm... still looks complex... https://codereview.chromium.org/988023005/diff/180001/Source/core/editing/Gra... File Source/core/editing/GranularityStrategy.h (right): https://codereview.chromium.org/988023005/diff/180001/Source/core/editing/Gra... Source/core/editing/GranularityStrategy.h:1: /* nit: Let's use the short version of licence comment. http://dev.chromium.org/blink/coding-style#TOC-License https://codereview.chromium.org/988023005/diff/180001/Source/core/editing/Gra... Source/core/editing/GranularityStrategy.h:41: virtual ~GranularityStrategy() { }; Let's move inline virtual functions into .cpp files: http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in... https://codereview.chromium.org/988023005/diff/180001/Source/core/editing/Gra... Source/core/editing/GranularityStrategy.h:55: ~CharacterGranularityStrategy() override { }; nit: |final|. This comment is applied rest of |override| in |final| class. https://codereview.chromium.org/988023005/diff/180001/Source/core/editing/Gra... Source/core/editing/GranularityStrategy.h:77: enum class EBoundAdjust {CurrentPosIfOnBound, NextBoundIfOnBound}; I think we don't need to have prefix "E". Since, this enum class is a private member and won't conflict with future classes. https://codereview.chromium.org/988023005/diff/180001/Source/core/editing/Sel... File Source/core/editing/SelectionStrategy.h (right): https://codereview.chromium.org/988023005/diff/180001/Source/core/editing/Sel... Source/core/editing/SelectionStrategy.h:1: /* nit: Let's use the short version of licence comment. http://dev.chromium.org/blink/coding-style#TOC-License https://codereview.chromium.org/988023005/diff/180001/Source/core/editing/Sel... Source/core/editing/SelectionStrategy.h:36: enum SelectionStrategy { nit: Let's use |enum class| for better type checking. https://codereview.chromium.org/988023005/diff/180001/public/web/WebSettings.h File public/web/WebSettings.h (right): https://codereview.chromium.org/988023005/diff/180001/public/web/WebSettings.... public/web/WebSettings.h:98: enum SelectionStrategyType { nit: Let's use enum class for better type checking. Also, we can write: |SelectionStrategyType::Character|.
https://codereview.chromium.org/988023005/diff/180001/Source/core/editing/Gra... File Source/core/editing/GranularityStrategy.cpp (right): https://codereview.chromium.org/988023005/diff/180001/Source/core/editing/Gra... Source/core/editing/GranularityStrategy.cpp:1: /* On 2015/04/21 01:59:51, Yosi_UTC9 wrote: > nit: Let's use the short version of licence comment. > http://dev.chromium.org/blink/coding-style#TOC-License Done. https://codereview.chromium.org/988023005/diff/180001/Source/core/editing/Gra... Source/core/editing/GranularityStrategy.cpp:58: if (direction == ESearchDirection::SearchForward) I moved the ternary operator out and added a bool to resolve wordBoundAdjust - let me know if you like it. https://codereview.chromium.org/988023005/diff/180001/Source/core/editing/Gra... File Source/core/editing/GranularityStrategy.h (right): https://codereview.chromium.org/988023005/diff/180001/Source/core/editing/Gra... Source/core/editing/GranularityStrategy.h:1: /* On 2015/04/21 01:59:52, Yosi_UTC9 wrote: > nit: Let's use the short version of licence comment. > http://dev.chromium.org/blink/coding-style#TOC-License Done - for all 3 new files. https://codereview.chromium.org/988023005/diff/180001/Source/core/editing/Gra... Source/core/editing/GranularityStrategy.h:41: virtual ~GranularityStrategy() { }; On 2015/04/21 01:59:52, Yosi_UTC9 wrote: > Let's move inline virtual functions into .cpp files: > http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in... Done. https://codereview.chromium.org/988023005/diff/180001/Source/core/editing/Gra... Source/core/editing/GranularityStrategy.h:55: ~CharacterGranularityStrategy() override { }; On 2015/04/21 01:59:52, Yosi_UTC9 wrote: > nit: |final|. > This comment is applied rest of |override| in |final| class. Done. https://codereview.chromium.org/988023005/diff/180001/Source/core/editing/Gra... Source/core/editing/GranularityStrategy.h:77: enum class EBoundAdjust {CurrentPosIfOnBound, NextBoundIfOnBound}; On 2015/04/21 01:59:52, Yosi_UTC9 wrote: > I think we don't need to have prefix "E". Since, this enum class is a private > member and won't conflict with future classes. Done. https://codereview.chromium.org/988023005/diff/180001/Source/core/editing/Sel... File Source/core/editing/SelectionStrategy.h (right): https://codereview.chromium.org/988023005/diff/180001/Source/core/editing/Sel... Source/core/editing/SelectionStrategy.h:1: /* On 2015/04/21 01:59:52, Yosi_UTC9 wrote: > nit: Let's use the short version of licence comment. > http://dev.chromium.org/blink/coding-style#TOC-License Done. https://codereview.chromium.org/988023005/diff/180001/Source/core/editing/Sel... Source/core/editing/SelectionStrategy.h:36: enum SelectionStrategy { On 2015/04/21 01:59:52, Yosi_UTC9 wrote: > nit: Let's use |enum class| for better type checking. Done. https://codereview.chromium.org/988023005/diff/180001/public/web/WebSettings.h File public/web/WebSettings.h (right): https://codereview.chromium.org/988023005/diff/180001/public/web/WebSettings.... public/web/WebSettings.h:98: enum SelectionStrategyType { On 2015/04/21 01:59:52, Yosi_UTC9 wrote: > nit: Let's use enum class for better type checking. > > Also, we can write: |SelectionStrategyType::Character|. Done.
yosin: Can you please LGTM if the CL looks good to you? Thanks!
lgtm w/ small nits. https://codereview.chromium.org/988023005/diff/200001/Source/core/editing/Fra... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/988023005/diff/200001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:226: if (m_granularityStrategy && (options & FrameSelection::DoNotClearStrategy) == 0) { nit: We don't need to have "{}". https://codereview.chromium.org/988023005/diff/200001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:234: nit: No need to have an extra blank line. https://codereview.chromium.org/988023005/diff/200001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:1944: const VisiblePosition base = m_selection.visibleBase(); nit: We don't need to have |const|. https://codereview.chromium.org/988023005/diff/200001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:1946: if (isNone() || base == extentPosition) nit: Can we test |isNone()| before calling |VisibleSelection.visibleBase()|? https://codereview.chromium.org/988023005/diff/200001/Source/core/editing/Gra... File Source/core/editing/GranularityStrategy.cpp (right): https://codereview.chromium.org/988023005/diff/200001/Source/core/editing/Gra... Source/core/editing/GranularityStrategy.cpp:12: ///////////////////////////////////////////////////////////////////// nit: We don't need have HR like banner comments. https://codereview.chromium.org/988023005/diff/200001/Source/core/editing/Gra... Source/core/editing/GranularityStrategy.cpp:86: // Determine the boundary of the 'current word', i.e. the boundary extending beyond which OPTIONAL: It is nice to format comment less than 80 chars in line to align Chromium coding style. https://codereview.chromium.org/988023005/diff/200001/Source/core/editing/Gra... File Source/core/editing/GranularityStrategy.h (right): https://codereview.chromium.org/988023005/diff/200001/Source/core/editing/Gra... Source/core/editing/GranularityStrategy.h:21: protected: nit: Please insert a blank line before |protected:| https://codereview.chromium.org/988023005/diff/200001/Source/core/editing/Gra... Source/core/editing/GranularityStrategy.h:50: private: nit: Please insert a blank line before |private:|
https://codereview.chromium.org/988023005/diff/200001/Source/core/editing/Fra... File Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/988023005/diff/200001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:226: if (m_granularityStrategy && (options & FrameSelection::DoNotClearStrategy) == 0) { On 2015/04/23 03:38:47, Yosi_UTC9 wrote: > nit: We don't need to have "{}". Done. https://codereview.chromium.org/988023005/diff/200001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:234: On 2015/04/23 03:38:47, Yosi_UTC9 wrote: > nit: No need to have an extra blank line. Done. https://codereview.chromium.org/988023005/diff/200001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:1944: const VisiblePosition base = m_selection.visibleBase(); On 2015/04/23 03:38:47, Yosi_UTC9 wrote: > nit: We don't need to have |const|. Done. https://codereview.chromium.org/988023005/diff/200001/Source/core/editing/Fra... Source/core/editing/FrameSelection.cpp:1946: if (isNone() || base == extentPosition) On 2015/04/23 03:38:47, Yosi_UTC9 wrote: > nit: Can we test |isNone()| before calling |VisibleSelection.visibleBase()|? Done. https://codereview.chromium.org/988023005/diff/200001/Source/core/editing/Gra... File Source/core/editing/GranularityStrategy.cpp (right): https://codereview.chromium.org/988023005/diff/200001/Source/core/editing/Gra... Source/core/editing/GranularityStrategy.cpp:12: ///////////////////////////////////////////////////////////////////// On 2015/04/23 03:38:47, Yosi_UTC9 wrote: > nit: We don't need have HR like banner comments. Done. https://codereview.chromium.org/988023005/diff/200001/Source/core/editing/Gra... Source/core/editing/GranularityStrategy.cpp:86: // Determine the boundary of the 'current word', i.e. the boundary extending beyond which On 2015/04/23 03:38:47, Yosi_UTC9 wrote: > OPTIONAL: It is nice to format comment less than 80 chars in line to align > Chromium coding style. Done. https://codereview.chromium.org/988023005/diff/200001/Source/core/editing/Gra... File Source/core/editing/GranularityStrategy.h (right): https://codereview.chromium.org/988023005/diff/200001/Source/core/editing/Gra... Source/core/editing/GranularityStrategy.h:21: protected: On 2015/04/23 03:38:47, Yosi_UTC9 wrote: > nit: Please insert a blank line before |protected:| Done. https://codereview.chromium.org/988023005/diff/200001/Source/core/editing/Gra... Source/core/editing/GranularityStrategy.h:50: private: On 2015/04/23 03:38:47, Yosi_UTC9 wrote: > nit: Please insert a blank line before |private:| Done.
Source/core/editing rubber stamp LGTM (yosin@ is the expert) remaining Source/core and Source/public LGTM
mfomitchev@chromium.org changed reviewers: + aelias@chromium.org
aelias@chromium.org: Please review changes in Source/web
Source/web lgtm
The CQ bit was checked by mfomitchev@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org Link to the patchset: https://codereview.chromium.org/988023005/#ps220001 (title: "Addressing nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/988023005/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/android_chromium_gn_comp...)
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/988023005/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://src.chromium.org/viewvc/blink?view=rev&revision=194339 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
