ssia
Created attachment 345070 [details] Patch
Created attachment 345079 [details] Patch
Created attachment 345276 [details] Patch
Comment on attachment 345276 [details] Patch Attachment 345276 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8579492 New failing tests: http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html
Created attachment 345297 [details] Archive of layout-test-results from ews201 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews201 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 345329 [details] Patch
Created attachment 345345 [details] Patch
Comment on attachment 345345 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=345345&action=review > Source/WebCore/Sources.txt:1237 > +layout/inlineformatting/textlayout/ContentProvider.cpp > +layout/inlineformatting/textlayout/simple/SimpleContentProvider.cpp > +layout/inlineformatting/textlayout/simple/SimpleLineBreaker.cpp Do we need such deep paths at this point? Also I suppose ContentProvide won't be explicitly about 'text layout' in the future as it will cover non-text inline layout too? > Source/WebCore/layout/inlineformatting/textlayout/ContentProvider.h:42 > +class ContentProvider { Layout::ContentProvider is bit generic. Maybe the name could include word 'inline'? InlineProvider or something. > Source/WebCore/layout/inlineformatting/textlayout/simple/SimpleContentProvider.h:40 > +class SimpleContentProvider { Naming, similarly.
Comment on attachment 345345 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=345345&action=review >> Source/WebCore/layout/inlineformatting/textlayout/ContentProvider.h:42 >> +class ContentProvider { > > Layout::ContentProvider is bit generic. Maybe the name could include word 'inline'? InlineProvider or something. Or InlineContentProvider? >> Source/WebCore/layout/inlineformatting/textlayout/simple/SimpleContentProvider.h:40 >> +class SimpleContentProvider { > > Naming, similarly. It is bit weird that 'SimpleContentProvider' does not subclass 'ContentProvider'. If there is no clear separation of purpose between them it might be better to just subclass.
> It is bit weird that 'SimpleContentProvider' does not subclass > 'ContentProvider'. If there is no clear separation of purpose between them > it might be better to just subclass. The idea here is that SimpleContentProvider (really really misleading naming) is an implementation detail and the clients don not see this object at all. The client would use the ContentProvider::Iterator interface to get the text runs and ContentProvider would either create an SimpleContentProvider or a ComplexContentProvide to find those runs.
Created attachment 345374 [details] Patch
Comment on attachment 345374 [details] Patch Clearing flags on attachment: 345374 Committed r234000: <https://trac.webkit.org/changeset/234000>
All reviewed patches have been landed. Closing bug.
<rdar://problem/42397834>
Comment on attachment 345374 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=345374&action=review > Source/WebCore/layout/inlineformatting/textlayout/Runs.h:108 > + ConstVectorIterator(const Vector<T>&); It is weird that we check if (current()) to know whether we at the end of the vector or not. I would suggest making this through the bool casting operator instead: // This will allow experssions like "while (m_textContentIterator)" operator const bool() const { return m_index < m_content->size(); } > Source/WebCore/layout/inlineformatting/textlayout/Runs.h:110 > + const T* current() const; If we look at this iterator as a pointer to a vector, then this method should be the overloading of the "*" operator. This is compatible with the vector iterator: // This allows expressions like "*m_textContentIterator" operator const T *() const { assert(m_index < m_content->size()); return &m_content->at(m_index); } > Source/WebCore/layout/inlineformatting/textlayout/Runs.h:111 > + ConstVectorIterator<T>& operator++(); Post increment operator? // This allows expression like "*m_textContentIterator++". ConstVectorIterator<T> operator++(int) { auto temp = *this; ++*this; return temp; } > Source/WebCore/layout/inlineformatting/textlayout/TextContentProvider.cpp:96 > + m_textContentIterator.reset(); > + while (auto* textItem = m_textContentIterator.current()) { > + if (contains(from, *textItem)) > + return textItem->start; > + ++m_textContentIterator; > + } for (m_textContentIterator.reset(); m_textContentIterator; ++m_textContentIterator) { if (contains(from, *m_textContentIterator)) return (*m_textContentIterator).start; } > Source/WebCore/layout/inlineformatting/textlayout/TextContentProvider.cpp:123 > + while (length) { > + textItem = m_textContentIterator.current(); > + if (!textItem) { > + ASSERT_NOT_REACHED(); > + break; > + } while (length && m_textContentIterator) { And you can assert after the loop ASSERT(!length) > Source/WebCore/layout/inlineformatting/textlayout/simple/SimpleLineBreaker.cpp:416 > + auto* lineConstraint = m_lineConstraintIterator.current(); > + if (!lineConstraint) { > + ASSERT_NOT_REACHED(); > + return { }; > + } > + > + while (lineConstraint->verticalPosition && verticalPosition > *lineConstraint->verticalPosition) { > + lineConstraint = (++m_lineConstraintIterator).current(); > + if (!lineConstraint) { > + // The vertical position of the last entry in the constraint list is supposed to be infinite. > + ASSERT_NOT_REACHED(); > + return { }; > + } > + } > + > + return *lineConstraint; while (m_lineConstraintIterator) { auto lineConstraint = *m_lineConstraintIterator++; if (!lineConstraint.verticalPosition || verticalPosition <= *lineConstraint.verticalPosition) return lineConstraint; } ASSERT_NOT_REACHED(); return { };