Select/unselect text on http://www.loopinsight.com/2016/04/05/the-loop-magazine-app-is-dead/
rdar://problem/25578952
Created attachment 276099 [details] Patch
Comment on attachment 276099 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276099&action=review > Source/WebCore/ChangeLog:12 > + Unable to test. Range selection does not trigger the bug. What do you mean by this? Do you mean setting the selection using window.getSelection() doesn't trigger the bug? Can you use eventSender to trigger it?
(In reply to comment #3) > Comment on attachment 276099 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=276099&action=review > > > Source/WebCore/ChangeLog:12 > > + Unable to test. Range selection does not trigger the bug. > > What do you mean by this? Do you mean setting the selection using > window.getSelection() doesn't trigger the bug? Can you use eventSender to > trigger it? I'll try that.
Comment on attachment 276099 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276099&action=review Code change looks fine, but it’s pretty important to add the test case. I can’t find the bug fix, with all the refactoring. Maybe the TextPainter.cpp part is the bug fix and the rest is just refactoring? I suggest landing all the refactoring first, without the bug fix, if we don’t have the test yet. r=me for the refactoring > Source/WebCore/rendering/InlineTextBox.cpp:603 > + return std::make_pair(0, m_len); I think this can be: return { 0, m_len }; > Source/WebCore/rendering/InlineTextBox.cpp:606 > + int selectionStart; > + int selectionEnd; Seems like this could just be start and end. > Source/WebCore/rendering/InlineTextBox.cpp:607 > + renderer().selectionStartEnd(selectionStart, selectionEnd); Peculiar that this uses two out arguments and doesn’t even have get in its name. Should fix that. > Source/WebCore/rendering/InlineTextBox.cpp:614 > + return std::make_pair(selectionStart, selectionEnd); Should be: return { selectionStart, selectionEnd }; But I would combine the last three lines: return { std::max(selectionStart - m_start, 0), std::min<int>(selectionEnd - m_start, m_len) }; > Source/WebCore/rendering/InlineTextBox.cpp:681 > + int selectionEnd = std::min(endPos - offset, static_cast<int>(m_len)); I think it’s slightly nicer to write this like this: int selectionEnd = std::min<int>(endPos - offset, m_len); > Source/WebCore/rendering/InlineTextBox.h:123 > + std::pair<int, int> selectionStartEnd() const; In other cases like this we sometimes use structures with names for the members rather than using a tuple. Sure would be nice to get rid of some those pairs of local variables and arguments using the same structure.
Created attachment 276119 [details] Patch
(In reply to comment #5) > Comment on attachment 276099 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=276099&action=review > > Code change looks fine, but it’s pretty important to add the test case. > > I can’t find the bug fix, with all the refactoring. Maybe the > TextPainter.cpp part is the bug fix and the rest is just refactoring? > > I suggest landing all the refactoring first, without the bug fix, if we > don’t have the test yet. r=me for the refactoring > > > Source/WebCore/rendering/InlineTextBox.cpp:603 > > + return std::make_pair(0, m_len); > > I think this can be: > > return { 0, m_len }; > > > Source/WebCore/rendering/InlineTextBox.cpp:606 > > + int selectionStart; > > + int selectionEnd; > > Seems like this could just be start and end. > > > Source/WebCore/rendering/InlineTextBox.cpp:607 > > + renderer().selectionStartEnd(selectionStart, selectionEnd); > > Peculiar that this uses two out arguments and doesn’t even have get in its > name. Should fix that. > > > Source/WebCore/rendering/InlineTextBox.cpp:614 > > + return std::make_pair(selectionStart, selectionEnd); > > Should be: > > return { selectionStart, selectionEnd }; > > But I would combine the last three lines: > > return { std::max(selectionStart - m_start, 0), > std::min<int>(selectionEnd - m_start, m_len) }; > > > Source/WebCore/rendering/InlineTextBox.cpp:681 > > + int selectionEnd = std::min(endPos - offset, static_cast<int>(m_len)); > > I think it’s slightly nicer to write this like this: > > int selectionEnd = std::min<int>(endPos - offset, m_len); > > > Source/WebCore/rendering/InlineTextBox.h:123 > > + std::pair<int, int> selectionStartEnd() const; > > In other cases like this we sometimes use structures with names for the > members rather than using a tuple. Sure would be nice to get rid of some > those pairs of local variables and arguments using the same structure. Thanks for the comments. I'll land the refactoring patch separately. (bug 156459)
Comment on attachment 276119 [details] Patch Attachment 276119 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1134148 New failing tests: fast/text/text-disappear-on-deselect.html
Created attachment 276123 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.10.5
Created attachment 276124 [details] Patch
> > Source/WebCore/rendering/InlineTextBox.cpp:607 > > + renderer().selectionStartEnd(selectionStart, selectionEnd); > > Peculiar that this uses two out arguments and doesn’t even have get in its > name. Should fix that. > > Source/WebCore/rendering/InlineTextBox.h:123 > > + std::pair<int, int> selectionStartEnd() const; > > In other cases like this we sometimes use structures with names for the > members rather than using a tuple. Sure would be nice to get rid of some > those pairs of local variables and arguments using the same structure. https://bugs.webkit.org/show_bug.cgi?id=156467
Comment on attachment 276124 [details] Patch r193857 claimed "no change in functionality" so I'm confused.
Comment on attachment 276124 [details] Patch Clearing flags on attachment: 276124 Committed r199304: <http://trac.webkit.org/changeset/199304>
All reviewed patches have been landed. Closing bug.