Bug 57665

Created attachment 87937 [details] Patch (v1)

Attachment 87937 [details] did not build on qt: Build output: http://queues.webkit.org/results/8185320

Attachment 87937 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8320314

editing/Editor.cpp:127: error: ‘bool WebCore::markersHaveIdenticalDescription(const WTF::Vector<WebCore::DocumentMarker, 0u>&)’ defined but not used Sounds like that function has to be moved inside the #if.

Created attachment 88065 [details] Patch (v2) Fixed a build failure.

Comment on attachment 88065 [details] Patch (v2) View in context: https://bugs.webkit.org/attachment.cgi?id=88065&action=review > Source/WebCore/dom/DocumentMarkerController.cpp:351 > + if (node == startContainer && node == endContainer) { > + // The range spans only one node. > + if (it->endOffset > static_cast<unsigned>(range->startOffset()) && it->startOffset < static_cast<unsigned>(range->endOffset())) > + foundMarkers.append(*it); > + } else { > + if (node == startContainer) { > + if (it->endOffset > static_cast<unsigned>(range->startOffset())) > + foundMarkers.append(*it); > + } else if (node == endContainer) { > + if (it->startOffset < static_cast<unsigned>(range->endOffset())) > + foundMarkers.append(*it); > + } else > + foundMarkers.append(*it); I think that with continue you could write this with less repeated code: if (node == startContainer && it->endOffset <= static_cast<unsigned>(range->startOffset())) continue; if (node == endContainer && it->startOffset >= static_cast<unsigned>(range->endOffset())) continue; foundMarkers.append(*it); Cuts it down from 13 lines of code to 5 and I think it doesn’t lose clarity. If this logic was copied from another function, I suggest the same simplification for that other function. > Source/WebCore/editing/Editor.cpp:2451 > + Range* range = replacedRange.get(); I don’t think this local variable does us much good. > Source/WebCore/editing/Editor.cpp:2784 > + DocumentMarker::MarkerType markerType = markerTypesToAdd[i]; > + Range* range = replacementRange.get(); > if (m_correctionPanelInfo.panelType == CorrectionPanelInfo::PanelTypeReversion) > - markers->addMarker(replacementRange.get(), markerTypesToAdd[i]); > - else > - markers->addMarker(replacementRange.get(), markerTypesToAdd[i], m_correctionPanelInfo.replacedString); > + markers->addMarker(range, markerType); > + else { > + if (markerType == DocumentMarker::Replacement || markerType == DocumentMarker::Autocorrected) > + markers->addMarker(range, markerType, m_correctionPanelInfo.replacedString); > + else > + markers->addMarker(range, markerType); > + } I think the logic here could be streamlined a bit. The different branches of the if statement look almost the same to me.

Created attachment 88129 [details] Patch (v3)

Comment on attachment 88129 [details] Patch (v3) View in context: https://bugs.webkit.org/attachment.cgi?id=88129&action=review > Source/WebCore/dom/DocumentMarker.h:40 > + // Text has been modified by spell correction, reversion of spell correction or other types of substitution. This would be better worded as "or other type" rather than "or other types". > Source/WebCore/dom/DocumentMarker.h:41 > + // On some platforms, this prevents the text to be autocorrected again. On post Snow Leopard Mac OS X, Correct grammar would be "prevents the text from being autocorrected". > Source/WebCore/dom/DocumentMarker.h:54 > // On some platforms, this prevents the text to be spellchecked again. Grammar: should be "from being spellchecked" rather than "to be spellchecked".

Comment on attachment 88129 [details] Patch (v3) Rejecting attachment 88129 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'build'..." exit_code: 2 Last 500 characters of output: env XCODE_VERSION_MINOR 0320 setenv YACC /Developer/usr/bin/yacc /bin/sh -c /mnt/git/webkit-commit-queue/WebKitBuild/WebCore.build/Debug/WebCore.build/Script-5DF50887116F3077005202AB.sh ** BUILD FAILED ** The following build commands failed: WebCore: CompileC /mnt/git/webkit-commit-queue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/Editor.o /mnt/git/webkit-commit-queue/Source/WebCore/editing/Editor.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2 (1 failure) Full output: http://queues.webkit.org/results/8329408

Created attachment 88315 [details] Patch (v4) Resovled conflict with TOT. Rearranged some code to fix build failure on SnowLeopard.

The commit-queue encountered the following flaky tests while processing attachment 88315 [details]: media/invalid-media-url-crash.html bug 51138 (authors: inferno@chromium.org and jamesr@chromium.org) The commit-queue is continuing to process your patch.

Comment on attachment 88315 [details] Patch (v4) Rejecting attachment 88315 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'land-a..." exit_code: 1 Last 500 characters of output: M Source/WebKit/chromium/public/WebContextMenuData.h r83008 = aaf08a4819113308ab54f5e0c3073aa0f941412b (refs/remotes/trunk) M Tools/ChangeLog M Tools/Scripts/webkitpy/layout_tests/port/chromium.py M Tools/Scripts/webkitpy/layout_tests/port/test.py M Tools/Scripts/webkitpy/layout_tests/rebaseline_chromium_webkit_tests.py r83009 = 8487ee9f890e2780cd7a6371494ec208403cad35 (refs/remotes/trunk) First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/trunk. Full output: http://queues.webkit.org/results/8341030

Created attachment 88409 [details] Patch (v5) Resovled conflict with Added missing "Reviewed by ..." line in previous patch.

(In reply to comment #13) > Created an attachment (id=88409) [details] > Patch (v5) > > Resovled conflict with Added missing "Reviewed by ..." line in previous patch. Correction, it should read: Added missing "Reviewed by ..." line in previous patch.

Comment on attachment 88409 [details] Patch (v5) Clearing flags on attachment: 88409 Committed r83060: <http://trac.webkit.org/changeset/83060>

All reviewed patches have been landed. Closing bug.