Created attachment 145604 [details]
Pøtch

Attachment 145604 [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/dom/ElementAttributeData.h:85:  The parameter name "name" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/ElementAttributeData.h:86:  The parameter name "name" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 145604 [details]
Pøtch

Attachment 145604 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12905190

Comment on attachment 145604 [details]
Pøtch

View in context: https://bugs.webkit.org/attachment.cgi?id=145604&action=review

r=me, nice!

> Source/WebCore/dom/Element.cpp:684
>  inline void Element::setAttributeInternal(size_t index, const QualifiedName& name, const AtomicString& value, EInUpdateStyleAttribute inUpdateStyleAttribute)
>  {
> +    mutableAttributeData();

This looks lonely. It would perhaps be nicer to use the returned value instead of accessing attributes directly.

> Source/WebCore/dom/Element.cpp:2123
> +void Element::createMutableAttributeData()
> +{
> +    if (!m_attributeData)
> +        m_attributeData = ElementAttributeData::create();

It would be nicer if this was called ElementAttributeData::createMutable() (same for StylePropertySet::create() too)

> Source/WebCore/dom/ElementAttributeData.cpp:54
> +ElementAttributeData::ElementAttributeData(const Vector<Attribute>& attributes)
> +    : m_isMutable(false)
> +    , m_arraySize(attributes.size())
> +{
> +    for (unsigned i = 0; i < attributes.size(); ++i)
> +        new (&array()[i]) Attribute(attributes[i]);
> +}

Anders tells that we should be able to generalize this pattern into a template. Would be nice to do that at some point as we can probably find other uses.

Committed r119421: <http://trac.webkit.org/changeset/119421>

(In reply to comment #5)
> Committed r119421: <http://trac.webkit.org/changeset/119421>

Hey man, why did you commit it, when the Qt EWS bubble was red?
Could you fix the build?

Re-opened since this is blocked by 88256

Created attachment 151261 [details]
EWSxperiment

Attachment 151261 [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/dom/ElementAttributeData.h:83:  The parameter name "name" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/ElementAttributeData.h:85:  The parameter name "name" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 151261 [details]
EWSxperiment

Attachment 151261 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13165333

Comment on attachment 151261 [details]
EWSxperiment

Attachment 151261 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13157875

Created attachment 151269 [details]
EWSxperiment 2

Attachment 151269 [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/dom/ElementAttributeData.h:83:  The parameter name "name" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/ElementAttributeData.h:85:  The parameter name "name" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 151269 [details]
EWSxperiment 2

Attachment 151269 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13174145

New failing tests:
svg/css/svg-ellipse-render-crash.html

Created attachment 151286 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 151711 [details]
EWSxperiment 3

Comment on attachment 151711 [details]
EWSxperiment 3

View in context: https://bugs.webkit.org/attachment.cgi?id=151711&action=review

r=me, glorious

> Source/WebCore/dom/Element.cpp:244
> +    // FIXME: This shouldn't have to make the ElementAttributeData mutable.
> +    const_cast<Element*>(this)->ensureUpdatedAttributeData();

Is there is really anything wrong with this? The FIXME might be an overkill (and point to over-optimization). Un-consting the function would remove that awkward const_cast.

> Source/WebCore/dom/ElementAttributeData.cpp:69
> +    , m_mutableAttributeVector(new Vector<Attribute, 4>)
> +{
> +    // This copy constructor should only be used by makeMutable() to go from immutable to mutable.
> +    ASSERT(!other.m_isMutable);
> +    for (unsigned i = 0; i < other.m_arraySize; ++i)
> +        m_mutableAttributeVector->append(other.array()[i]);
> +}

This could use reserverInitialCapacity() and uncheckedAppend().

Not sure how to calculate the optimal initial capacity. Maybe just 1 more than the current size?

Committed r122450: <http://trac.webkit.org/changeset/122450>

Re-opened since this is blocked by 91263

Created attachment 153807 [details]
Patch

Okay, let's give this another try. This time with liberal use of the "const" keyword to catch any incorrect ElementAttributeData usage at compile-time.

Comment on attachment 153807 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153807&action=review

> Source/WebCore/ChangeLog:155
> +            Updated code to handle both immutable and mutable source objects. This could 

...or could it?

Comment on attachment 153807 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153807&action=review

> Source/WebCore/dom/Element.cpp:1398
>      RefPtr<Attr> oldAttr = attrIfExists(attr->qualifiedName());

It appears that attrIfExists doesn't update the invalid attributes.

> Source/WebCore/dom/ElementAttributeData.cpp:37
> +    void* slot = WTF::fastMalloc(sizeof(ElementAttributeData) - sizeof(void*) + sizeof(Attribute) * attributes.size());

We don't have a fancy WTF macros to deal with alignments?

> Source/WebCore/dom/ElementAttributeData.cpp:82
> +    for (unsigned i = 0; i < m_arraySize; ++i)
> +        array()[i].~Attribute();

I think it's better to wrap this in the else clause. I would also assert that m_arraySize is 0 when isMutable().

> Source/WebCore/dom/ElementAttributeData.h:140
> +        void* m_attributes;

Why can't we use Attribute* here?

(In reply to comment #22)
> (From update of attachment 153807 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153807&action=review
> 
> > Source/WebCore/dom/Element.cpp:1398
> >      RefPtr<Attr> oldAttr = attrIfExists(attr->qualifiedName());
> 
> It appears that attrIfExists doesn't update the invalid attributes.

Good catch. Fixing that and adding a test so it's covered in the future.

> > Source/WebCore/dom/ElementAttributeData.cpp:37
> > +    void* slot = WTF::fastMalloc(sizeof(ElementAttributeData) - sizeof(void*) + sizeof(Attribute) * attributes.size());
> 
> We don't have a fancy WTF macros to deal with alignments?

Not sure what you're talking about. Subtracting sizeof(void*) is to get the base size of the object excluding the union.

> > Source/WebCore/dom/ElementAttributeData.cpp:82
> > +    for (unsigned i = 0; i < m_arraySize; ++i)
> > +        array()[i].~Attribute();
> 
> I think it's better to wrap this in the else clause. I would also assert that m_arraySize is 0 when isMutable().

Will do.

> > Source/WebCore/dom/ElementAttributeData.h:140
> > +        void* m_attributes;
> 
> Why can't we use Attribute* here?

We could use any type, at the end of the day we have to reinterpret_cast<Attribute*>(&m_attributes) anyway. It would look nicer if the compiler would let us do "Attribute m_attributes[0]" or something like that, but that doesn't fly in a union. :/

Created attachment 154165 [details]
Patch

Created attachment 154209 [details]
Patch

C'mon new EWS.

Dumping IRC log since rniwa pinged out.

rniwa: kling: one more question about the patch
rniwa: kling: what happens if CSSMutableStyle had been created for inline style
rniwa: kling: when the attributes were immutable (i.e. stored in ElementAttributeData)
rniwa: kling: and then attributes got mutable via setAttribute on the element (not via CSSOM wrapper for the inline style)
rniwa: kling: doesn't the m_propertySet in the wrapper get stable in that case?

kling: rniwa: the StylePropertySet pointer doesn't change when making a new mutable ElementAttributeData
kling: rniwa: it inherits the old one from the immutable EAD
kling: rniwa: or what do you mean by stable?
kling: rniwa: if you look at ElementAttributeData(const ElementAttributeData& other), you can see that all members are cloned
kling: rniwa: the source object is discarded after this constructor runs (the immutable EAD has now been replaced by a mutable EAD)
kling: rniwa: (so there aren't multiple EADs pointing to the same m_propertySet)

Comment on attachment 154209 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154209&action=review

> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:410
> +    if (mutablePropertySet != m_propertySet) {

When does this happen?

> Source/WebCore/dom/ElementAttributeData.cpp:235
>      ASSERT(index < length());

Missing ASSERT(isMutable()); here.

> Source/WebCore/dom/StyledElement.cpp:237
> +    bool changes = mutableAttributeData()->inlineStyle()->removeProperty(propertyID);

We can't call ensureInlineStyle() here?

Comment on attachment 154209 [details]
Patch

Attachment 154209 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13341133

New failing tests:
fast/loader/loadInProgress.html
fast/inline/positionedLifetime.html
fast/loader/unload-form-post-about-blank.html

Created attachment 154263 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

(In reply to comment #27)
> (From update of attachment 154209 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154209&action=review
> 
> > Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:410
> > +    if (mutablePropertySet != m_propertySet) {
> 
> When does this happen?

Never! D'oh! When InlineCSSStyleDeclaration is created, we always have a mutable StylePropertySet already. I'll replace this by an assertion that isMutable(). The code can be cleaned up further, I'll do that in a follow-up.

> > Source/WebCore/dom/ElementAttributeData.cpp:235
> >      ASSERT(index < length());
> 
> Missing ASSERT(isMutable()); here.

Good idea.

> > Source/WebCore/dom/StyledElement.cpp:237
> > +    bool changes = mutableAttributeData()->inlineStyle()->removeProperty(propertyID);
> 
> We can't call ensureInlineStyle() here?

Sure we can!

Created attachment 154353 [details]
Patch

Comment on attachment 154353 [details]
Patch

Clearing flags on attachment: 154353

Committed r123636: <http://trac.webkit.org/changeset/123636>

All reviewed patches have been landed.  Closing bug.