Bug 160735

Created attachment 285726 [details] WIP

Created attachment 285798 [details] WIP

Attachment 285798 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 50 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 285848 [details] Patch

Created attachment 285851 [details] Patch

Comment on attachment 285851 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285851&action=review > Source/WebCore/editing/FrameSelection.cpp:2094 > + ASSERT(startOffset >= 0 && endOffset >= 0); The autos hide whether these are ints or unsigned so I have no idea whether the assertion is useful. > Source/WebCore/platform/DragImage.cpp:179 > + auto startOffset = start.deprecatedEditingOffset(); > + auto endOffset = end.deprecatedEditingOffset(); > + ASSERT(startOffset >= 0 && endOffset >= 0); Ditto. > Source/WebCore/platform/graphics/Latin1TextIterator.h:62 > + unsigned m_currentCharacter; > + unsigned m_lastCharacter; These could be renamed to current/lastIndex perhaps? > Source/WebCore/platform/graphics/SurrogatePairAwareTextIterator.h:65 > + unsigned m_currentCharacter; > + unsigned m_lastCharacter; > + unsigned m_endCharacter; Ditto > Source/WebCore/platform/graphics/WidthIterator.cpp:111 > + ASSERT(glyphBufferSize >= lastGlyphCount); I would flip this: ASSERT(lastGlyphCount <= glyphBufferSize); > Source/WebCore/rendering/InlineTextBox.cpp:139 > + // FIXME: We should only be checking if sPos >= ePos here, because those are the > + // indices used to actually generate the selection rect. Allowing us past this guard > + // on any other condition creates zero-width selection rects. But what's the proposed solution? Should this reference a bug? > Source/WebCore/rendering/InlineTextBox.cpp:175 > + ASSERT(static_cast<int>(selectionStart) >= 0 && static_cast<int>(selectionEnd) >= 0); This will never fire, right? > Source/WebCore/rendering/InlineTextBox.cpp:206 > + // FIXME: We should only be checking if sPos >= ePos here, because those are the > + // indices used to actually generate the selection rect. Allowing us past this guard > + // on any other condition creates zero-width selection rects. But what's the proposed solution? Should this reference a bug? > Source/WebCore/rendering/InlineTextBox.cpp:597 > +unsigned InlineTextBox::clampedOffset(unsigned x) const Blank line above pls. > Source/WebCore/rendering/RenderView.cpp:917 > + auto newEndPos = node->offsetInCharacters() ? node->maxCharacterOffset() : node->countChildNodes(); > + ASSERT(newEndPos >= 0); Can't tell whether the assertion is useful. > Source/WebCore/rendering/RenderView.h:321 > RenderObject* m_selectionUnsplitStart; > RenderObject* m_selectionUnsplitEnd; Maybe move this class to initializers? > Source/WebCore/rendering/svg/RenderSVGInlineText.cpp:135 > + ASSERT(caretOffset > 0); Nope.

Comment on attachment 285851 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=285851&action=review >> Source/WebCore/rendering/RenderView.h:321 >> RenderObject* m_selectionUnsplitEnd; > > Maybe move this class to initializers? I don't understand what you mean by this. Could you explain it?

Created attachment 285863 [details] Patch for committing

Created attachment 285868 [details] Patch for committing

Created attachment 285870 [details] Patch for committing

Comment on attachment 285870 [details] Patch for committing Clearing flags on attachment: 285870 Committed r204400: <http://trac.webkit.org/changeset/204400>

Looks like this patch broke the GTK Debug tests bot. For instance fast/forms/input-text-paste-maxlength.html now crashes the WebProcess: https://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug%20(Tests)/r204408%20(10632)/fast/forms/input-text-paste-maxlength-crash-log.txt Thread 1 (Thread 0x7f4976d6a940 (LWP 45002)): #0 0x00007f49868ac334 in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:323 #1 0x00007f498eb7800d in (anonymous namespace)::HarfBuzzShaper::selectionRect (this=0x7fffa92bfe70, point=..., height=0, from=0, to=5) at ../../Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp:661 #2 0x00007f498eb57b8c in (anonymous namespace)::FontCascade::adjustSelectionRectForComplexText (this=0x7f4976257018, run=..., selectionRect=..., from=0, to=7) at ../../Source/WebCore/platform/graphics/cairo/FontCairoHarfbuzzNG.cpp:93 #3 0x00007f498e385b62 in (anonymous namespace)::FontCascade::adjustSelectionRectForText (this=0x7f4976257018, run=..., selectionRect=..., from=0, to=...) at ../../Source/WebCore/platform/graphics/FontCascade.cpp:505 #4 0x00007f498e4f66ba in (anonymous namespace)::InlineTextBox::positionForOffset (this=0x7f4976259a80, offset=7) at ../../Source/WebCore/rendering/InlineTextBox.cpp:1016 #5 0x00007f498e6ff30b in (anonymous namespace)::RenderText::localCaretRect (this=0x7f497625aae0, inlineBox=0x7f4976259a80, caretOffset=7, extraWidthToEndOfLine=0x0) at ../../Source/WebCore/rendering/RenderText.cpp:451 #6 0x00007f498dd5a36d in (anonymous namespace)::VisiblePosition::localCaretRect (this=0x7fffa92c0350, renderer=@0x7fffa92c0288: 0x7f497625aae0) at ../../Source/WebCore/editing/VisiblePosition.cpp:660 #7 0x00007f498dd72726 in (anonymous namespace)::localCaretRectInRendererForCaretPainting (caretPosition=..., caretPainter=@0x7fffa92c0318: 0x7f498d0a0352 <(anonymous namespace)::Frame::document() const+30>) at ../../Source/WebCore/editing/htmlediting.cpp:1269 #8 0x00007f498dd1053d in (anonymous namespace)::CaretBase::updateCaretRect (this=0x7f49763bf230, document=0x7f49762ac000, caretPosition=...) at ../../Source/WebCore/editing/FrameSelection.cpp:1559 #9 0x00007f498dd10917 in (anonymous namespace)::FrameSelection::recomputeCaretRect (this=0x7f49763bf230) at ../../Source/WebCore/editing/FrameSelection.cpp:1613 #10 0x00007f498dd12fa4 in (anonymous namespace)::FrameSelection::updateAppearance (this=0x7f49763bf230) at ../../Source/WebCore/editing/FrameSelection.cpp:2033 #11 0x00007f498dd09889 in (anonymous namespace)::FrameSelection::updateAndRevealSelection (this=0x7f49763bf230, intent=...) at ../../Source/WebCore/editing/FrameSelection.cpp:380 #12 0x00007f498dd14c69 in (anonymous namespace)::FrameSelection::updateAppearanceAfterLayout (this=0x7f49763bf230) at ../../Source/WebCore/editing/FrameSelection.cpp:2386 #13 0x00007f498e1f7cd5 in (anonymous namespace)::FrameView::performPostLayoutTasks (this=0x7f49345fdb00) at ../../Source/WebCore/page/FrameView.cpp:3206 #14 0x00007f498e1f1f01 in (anonymous namespace)::FrameView::layout (this=0x7f49345fdb00, allowSubtree=true) at ../../Source/WebCore/page/FrameView.cpp:1523 #15 0x00007f498db5e712 in (anonymous namespace)::Document::updateLayout (this=0x7f49762ac000) at ../../Source/WebCore/dom/Document.cpp:2009 #16 0x00007f498db5e814 in (anonymous namespace)::Document::updateLayoutIgnorePendingStylesheets (this=0x7f49762ac000, runPostLayoutTasks=(anonymous namespace)::Document::RunPostLayoutTasks::Asynchronously) at ../../Source/WebCore/dom/Document.cpp:2041 #17 0x00007f498dd03ce6 in (anonymous namespace)::Editor::Command::execute (this=0x7fffa92c0a30, parameter="(null)", triggeringEvent=0x0) at ../../Source/WebCore/editing/EditorCommand.cpp:1778 #18 0x00007f498db6a452 in (anonymous namespace)::Document::execCommand (this=0x7f49762ac000, commandName="SelectAll", userInterface=false, value="(null)") at ../../Source/WebCore/dom/Document.cpp:4964 #19 0x00007f498ee3b17e in (anonymous namespace)::jsDocumentPrototypeFunctionExecCommand (state=0x7fffa92c0b20) at DerivedSources/WebCore/JSDocument.cpp:4633 #20 0x00007f4935ffe028 in ?? () #21 0x00007fffa92c0ba0 in ?? () #22 0x00007f49864dc8e8 in llint_entry () at ../../Source/WTF/wtf/RefPtr.h:79

(In reply to comment #12) > Looks like this patch broke the GTK Debug tests bot. I'm working on this in https://bugs.webkit.org/show_bug.cgi?id=160818