Bug 65297

Created attachment 102209 [details] Patch

Comment on attachment 102209 [details] Patch This seems nifty.

Comment on attachment 102209 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102209&action=review review- because I think you will make at least one change based on my comments. > Source/WebCore/css/CSSPrimitiveValue.h:122 > + bool isLength() { return isUnitTypeLength(m_type); } This should be a const member function. > Source/WebCore/css/CSSStyleSelector.cpp:194 > + for (CSSValueListIterator i = CSSValueListIterator(value); i.hasValue(); i.next()) { \ This seems a little wordy. Since the constructor is not marked explicit you can do without the type name: [...] CSSValueListIterator i = value [...] Same at many other call sites. > Source/WebCore/css/CSSStyleSelector.cpp:4454 > - m_style->setTextShadow(shadowData.release(), i /* add to the list if this is not the firsty entry */); > + m_style->setTextShadow(shadowData.release(), !i.index() /* add to the list if this is not the firsty entry */); I suggest fixing the "firsty" typo and changing to a "//" comment if you are touching this line. > Source/WebCore/css/CSSStyleSelector.cpp:5269 > - Length width; > - Length height; > + Length width, height; This is an anti-pattern in WebKit code and so should not be changed. > Source/WebCore/css/CSSStyleSelector.cpp:5272 > + CSSValueListIterator iterator = CSSValueListIterator(value); > + switch (iterator.length()) { This use is not iteration at all. So already, right out of the gate, the iterator class does two separate things. I think this should not be using iterator. You could build the iterator out of a base class that can be used just to wrap the list if you like. > Source/WebCore/css/CSSValueList.cpp:94 > for (size_t index = 0; index < m_values.size(); index++) > - newList->append(item(index)); > + newList->append(itemWithoutBoundsCheck(index)); It seems strange to use m_values on one line and then call itemWithoutBoundsCheck on the next. Can we just index into m_values instead? > Source/WebCore/css/CSSValueList.h:51 > - CSSValue* item(unsigned); > + CSSValue* item(unsigned index) { return index < m_values.size() ? m_values[index].get() : 0; } > CSSValue* itemWithoutBoundsCheck(unsigned index) { return m_values[index].get(); } Seems these should take size_t instead of unsigned. > Source/WebCore/css/CSSValueList.h:76 > +class CSSValueListIterator { An iterator seems OK but not great to me; a “why” comment about the value of the iterator would be welcome. It seems the main value offered is the integral call to isValueList and the type check. Another is the use of itemWithoutBoundsCheck function without having to be so wordy about it. > Source/WebCore/css/CSSValueList.h:78 > + CSSValueListIterator(CSSValue* value) : m_position(0) { m_list = value->isValueList() ? static_cast<CSSValueList*>(value) : 0; } We don’t need this to work with a 0 for CSSValue*? > Source/WebCore/css/CSSValueList.h:79 > + bool hasValue() { return m_list && m_position < m_list->length(); } I’m not really all that pleased with hasValue as the name for this. I don’t think it’s really natural to ask if an iterator "has a value". It makes more sense to ask if an iterator “is at the end of the things it iterates”. I understand that it’s nice to have the sense reversed, but a name like atEnd seems better and more straightforward. Or maybe hasMoreValues is a good name? This should be a const member function. > Source/WebCore/css/CSSValueList.h:80 > + CSSValue* value() { ASSERT(hasValue()); return m_list->itemWithoutBoundsCheck(m_position); } This should be a const member function. > Source/WebCore/css/CSSValueList.h:82 > + CSSValue* first() { ASSERT(length() > 0); return m_list->itemWithoutBoundsCheck(0); } > + CSSValue* second() { ASSERT(length() > 1); return m_list->itemWithoutBoundsCheck(1); } These are strange. They have nothing to do with iteration, so don’t seem to belong on an iterator. They would make more sense as CSSValueList member functions. I see how you want to use the iterator also as just a type-checked pointer, but that seems too “clever” to me and makes this a dual-purpose class. > Source/WebCore/css/CSSValueList.h:83 > + void next() { m_position++; } A function like this should have a verb for its name. Perhaps “advance”? It also should assert that we are not advancing past the end. > Source/WebCore/css/CSSValueList.h:84 > + size_t index() { return m_position; } This should be a const member function. > Source/WebCore/css/CSSValueList.h:85 > + size_t length() { return m_list ? m_list->length() : 0; } You could write the hasValue function more simply using this function. return m_position < length(); This should be a const member function. > Source/WebCore/css/CSSValueList.h:87 > + CSSValueList* m_list; A CSSValueList is a reference-counted object, and this holds a non-RefPtr pointer to it. This hides potential lifetime issues inside the class, which is not a step in the right direction.

Really great concrete feedback, thanks Darin! I'll be back with another patch shortly.

Comment on attachment 102209 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102209&action=review >> Source/WebCore/css/CSSPrimitiveValue.h:122 >> + bool isLength() { return isUnitTypeLength(m_type); } > > This should be a const member function. done. >> Source/WebCore/css/CSSStyleSelector.cpp:194 >> + for (CSSValueListIterator i = CSSValueListIterator(value); i.hasValue(); i.next()) { \ > > This seems a little wordy. Since the constructor is not marked explicit you can do without the type name: > > [...] CSSValueListIterator i = value [...] > > Same at many other call sites. nice. done. >> Source/WebCore/css/CSSStyleSelector.cpp:4454 >> + m_style->setTextShadow(shadowData.release(), !i.index() /* add to the list if this is not the firsty entry */); > > I suggest fixing the "firsty" typo and changing to a "//" comment if you are touching this line. done. >> Source/WebCore/css/CSSStyleSelector.cpp:5269 >> + Length width, height; > > This is an anti-pattern in WebKit code and so should not be changed. done. >> Source/WebCore/css/CSSStyleSelector.cpp:5272 > > This use is not iteration at all. So already, right out of the gate, the iterator class does two separate things. I think this should not be using iterator. You could build the iterator out of a base class that can be used just to wrap the list if you like. Ok, I've added a separate wrapper class CSSValueListExpander that handles these cases. >> Source/WebCore/css/CSSValueList.cpp:94 >> + newList->append(itemWithoutBoundsCheck(index)); > > It seems strange to use m_values on one line and then call itemWithoutBoundsCheck on the next. Can we just index into m_values instead? done. >> Source/WebCore/css/CSSValueList.h:51 >> CSSValue* itemWithoutBoundsCheck(unsigned index) { return m_values[index].get(); } > > Seems these should take size_t instead of unsigned. done. >> Source/WebCore/css/CSSValueList.h:76 > > An iterator seems OK but not great to me; a “why” comment about the value of the iterator would be welcome. It seems the main value offered is the integral call to isValueList and the type check. Another is the use of itemWithoutBoundsCheck function without having to be so wordy about it. Added a minimal comment. You may want further changes. >> Source/WebCore/css/CSSValueList.h:78 >> + CSSValueListIterator(CSSValue* value) : m_position(0) { m_list = value->isValueList() ? static_cast<CSSValueList*>(value) : 0; } > > We don’t need this to work with a 0 for CSSValue*? done. >> Source/WebCore/css/CSSValueList.h:79 >> + bool hasValue() { return m_list && m_position < m_list->length(); } > > I’m not really all that pleased with hasValue as the name for this. I don’t think it’s really natural to ask if an iterator "has a value". It makes more sense to ask if an iterator “is at the end of the things it iterates”. I understand that it’s nice to have the sense reversed, but a name like atEnd seems better and more straightforward. Or maybe hasMoreValues is a good name? > > This should be a const member function. Changed the name to hasMore. I liked hasMoreValues as a suggestion, but given how often it is used I think we can get away with hasMore. Change to const member function done. >> Source/WebCore/css/CSSValueList.h:80 >> + CSSValue* value() { ASSERT(hasValue()); return m_list->itemWithoutBoundsCheck(m_position); } > > This should be a const member function. done. >> Source/WebCore/css/CSSValueList.h:82 >> + CSSValue* second() { ASSERT(length() > 1); return m_list->itemWithoutBoundsCheck(1); } > > These are strange. They have nothing to do with iteration, so don’t seem to belong on an iterator. They would make more sense as CSSValueList member functions. I see how you want to use the iterator also as just a type-checked pointer, but that seems too “clever” to me and makes this a dual-purpose class. Moved these to new class CSSValueListExpander to avoid the dual-purposing issue. >> Source/WebCore/css/CSSValueList.h:84 >> + size_t index() { return m_position; } > > This should be a const member function. done. >> Source/WebCore/css/CSSValueList.h:85 >> + size_t length() { return m_list ? m_list->length() : 0; } > > You could write the hasValue function more simply using this function. > > return m_position < length(); > > This should be a const member function. done. good idea. >> Source/WebCore/css/CSSValueList.h:87 >> + CSSValueList* m_list; > > A CSSValueList is a reference-counted object, and this holds a non-RefPtr pointer to it. This hides potential lifetime issues inside the class, which is not a step in the right direction. Hmm... I thought about that, and ended up here because CSSValueListIterator is intended to be only ever stack allocated and short-lived, and also because CSSStyleSelector::applyProperty itself (the main user) doesn't have a RefPtr either. Is there a better solution I haven't thought of, or a way to enforce the stack-allocation-only intent?

Created attachment 102230 [details] Patch

Comment on attachment 102209 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102209&action=review >>> Source/WebCore/css/CSSValueList.h:85 >>> + size_t length() { return m_list ? m_list->length() : 0; } >> >> You could write the hasValue function more simply using this function. >> >> return m_position < length(); >> >> This should be a const member function. > > done. good idea. It also occurs to me that this should be private on the iterator, since an iterator doesn’t have a length. It can be public on the list wrapper. >>> Source/WebCore/css/CSSValueList.h:87 >>> + CSSValueList* m_list; >> >> A CSSValueList is a reference-counted object, and this holds a non-RefPtr pointer to it. This hides potential lifetime issues inside the class, which is not a step in the right direction. > > Hmm... I thought about that, and ended up here because CSSValueListIterator is intended to be only ever stack allocated and short-lived, and also because CSSStyleSelector::applyProperty itself (the main user) doesn't have a RefPtr either. > Is there a better solution I haven't thought of, or a way to enforce the stack-allocation-only intent? You’re asking the right question. What we want to know is how we can write this so that it’s hard to use it improperly. What we will need for the long term may be some kind of smart pointer class we can use instead of a raw pointer that can help us check at compile time and runtime that it’s OK to use the raw pointer. In many, many places and many cases where we use raw pointers we later learn that we need to use RefPtr instead because of more complex flow or reentrancy. In the mean time I think we need to make sure there is some kind of comment about the usage. The raw pointer thing may seem harmless when writing the code but it’s one of the most dangerous idioms across the code base.

Comment on attachment 102230 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102230&action=review OK as is, some small ideas for improvements. > Source/WebCore/css/CSSValueList.h:79 > + CSSValueListIterator(CSSValue* value) : m_position(0) { m_list = (value && value->isValueList()) ? static_cast<CSSValueList*>(value) : 0; } Could use construction syntax instead of assignment for m_list. If you added one more function to CSSValueListExpander to get the value of the nth element in the list, then you could use CSSValueListExpander as the type of the m_list data member, which would save code in the constructor and obviate the need for a length function in this class. > Source/WebCore/css/CSSValueList.h:84 > + size_t length() const { return m_list ? m_list->length() : 0; } Should be private. > Source/WebCore/css/CSSValueList.h:92 > + CSSValueListExpander(CSSValue* value) { m_list = (value && value->isValueList()) ? static_cast<CSSValueList*>(value) : 0; } Could use construction syntax instead of assignment for m_list. Not sure that “expander” is quite the right name for this. It does two things: Type check that this is a list, and add convenience functions for accessing elements of the list. Can’t think of a better name, though. Maybe inspector?

Thanks again for the feedback Darin, I've incorporated all of those suggestions into the patch that I'll try to land shortly.

Created attachment 102316 [details] Patch for landing

Attachment 102316 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/css/CSSValueList.h:80: Missing space inside { }. [whitespace/braces] [5] Source/WebCore/css/CSSValueList.h:94: Missing space inside { }. [whitespace/braces] [5] Total errors found: 2 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 102336 [details] Patch for landing

Comment on attachment 102336 [details] Patch for landing Rejecting attachment 102336 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-03', '--port..." exit_code: 2 Last 500 characters of output: ce.html = IMAGE fast/text/international/thai-line-breaks.html = IMAGE fast/transforms/shadows.html = IMAGE platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE Regressions: Unexpected tests timed out : (1) http/tests/inspector/resource-tree/resource-tree-reload.html = TIMEOUT Regressions: Unexpected text diff mismatch : (3) fast/forms/implicit-submission.html = TEXT fast/js/parseInt.html = TEXT transitions/mismatched-shadow-styles.html = TEXT Full output: http://queues.webkit.org/results/9267422

Comment on attachment 102336 [details] Patch for landing Attachment 102336 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9290287 New failing tests: fast/repaint/shadow-multiple-strict-vertical.html http/tests/inspector/resource-tree/resource-tree-reload.html fast/box-shadow/spread-multiple-normal.html fast/repaint/shadow-multiple-strict-horizontal.html fast/repaint/shadow-multiple-vertical.html fast/box-shadow/spread-multiple-inset.html fast/forms/input-spinbutton-capturing.html svg/css/text-shadow-multiple.xhtml fast/transforms/shadows.html fast/forms/input-number-large-padding.html fast/box-shadow/inset.html fast/forms/validation-message-appearance.html fast/repaint/shadow-multiple-horizontal.html fast/forms/input-number-events.html fast/forms/implicit-submission.html

Created attachment 102485 [details] Patch for landing

Comment on attachment 102485 [details] Patch for landing Rejecting attachment 102485 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-03', '--port..." exit_code: 2 Last 500 characters of output: 1f90c0929e70f0229ce21dbc72240c2460a477d3 r92102 = e43fd95032e0e1b01413d2d857606a1aaf8e9ba4 Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/origin/master. Updating chromium port dependencies using gclient... ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/9283491

That one does look like a bit error. :)

Comment on attachment 102485 [details] Patch for landing Clearing flags on attachment: 102485 Committed r92106: <http://trac.webkit.org/changeset/92106>

All reviewed patches have been landed. Closing bug.