Bug 77800

Created attachment 125468 [details] Patch

Created attachment 125470 [details] Patch

Comment on attachment 125470 [details] Patch Attachment 125470 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11420609

Comment on attachment 125470 [details] Patch Attachment 125470 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11418636

Comment on attachment 125470 [details] Patch Attachment 125470 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11420610

Comment on attachment 125470 [details] Patch Gathered some feedback on IRC. WIll make an updated version of the patch.

Comment on attachment 125470 [details] Patch Attachment 125470 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11419595

Created attachment 125480 [details] Patch

Comment on attachment 125480 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125480&action=review > Source/WebCore/ChangeLog:20 > + No new tests. (OOPS!) Nit: Remove this line. > Source/WebCore/editing/EditingStyle.cpp:836 > + // FIXME: Can it really ignore invalid style or animation svg attributes? > + if (!element->hasAttributesWithoutUpdate()) This is definitely not right. Could you just change it to hasAttributes()? > Source/WebCore/editing/markup.cpp:109 > + unsigned length = e->attributeCount(); Isn't e->attributeCount() inlined now? Maybe we don't this local variable anymore. > Source/WebCore/html/parser/HTMLFormattingElementList.cpp:43 > +static inline size_t safeAttributeCount(Element* element) I don't think "safe" is appropriate adjective here. I'd call this function attributeCountWithoutUpdate instead. > Source/WebCore/inspector/DOMEditor.cpp:165 > + while (oldElement->attributeCount()) > + oldElement->removeAttribute(0); We don't have some method to remove all attributes? This looks like the most inefficient thing we can do here. > Source/WebCore/inspector/DOMEditor.cpp:173 > + size_t numAttrs = newElement->attributeCount(); > for (size_t i = 0; i < numAttrs; ++i) { > - const Attribute* attribute = newAttributeMap->attributeItem(i); > + const Attribute* attribute = newElement->attributeItem(i); > oldElement->setAttribute(attribute->name(), attribute->value()); > } We should have an Element method for this.

Comment on attachment 125480 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125480&action=review Thanks for the comments Rniwa. I have some questions: >> Source/WebCore/editing/markup.cpp:109 >> + unsigned length = e->attributeCount(); > > Isn't e->attributeCount() inlined now? Maybe we don't this local variable anymore. Should I inline other similar cases in this patch as well? >> Source/WebCore/inspector/DOMEditor.cpp:165 >> + oldElement->removeAttribute(0); > > We don't have some method to remove all attributes? This looks like the most inefficient thing we can do here. Could the will/didModifyAttribute calls affect the number of attributes (lead to another attribute addition or removal)? In this case this approach is less worse than you think. I worked with this assumption since I wasn't familiar with the code underneat. Regardless of that, I do agree we can wrap this in a function and at least improve to be equivalent to: "while (hasAttributes) removeAttribute(attributeCount() -1);".

(In reply to comment #10) > (From update of attachment 125480 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=125480&action=review > >> Source/WebCore/editing/markup.cpp:109 > >> + unsigned length = e->attributeCount(); > > > > Isn't e->attributeCount() inlined now? Maybe we don't this local variable anymore. > > Should I inline other similar cases in this patch as well? Maybe. It's your call. > >> Source/WebCore/inspector/DOMEditor.cpp:165 > >> + oldElement->removeAttribute(0); > > > > We don't have some method to remove all attributes? This looks like the most inefficient thing we can do here. > > Could the will/didModifyAttribute calls affect the number of attributes (lead to another attribute addition or removal)? In this case this approach is less worse than you think. I worked with this assumption since I wasn't familiar with the code underneat. That's a good point. > Regardless of that, I do agree we can wrap this in a function and at least improve to be equivalent to: "while (hasAttributes) removeAttribute(attributeCount() -1);". Yeah, wrapping these logics in some helper function will help us understand various assumptions in a more centralized way.

(In reply to comment #11) > Yeah, wrapping these logics in some helper function will help us understand various assumptions in a more centralized way. Note I'm just suggesting this for a follow up, not to be merged with this patch.

Created attachment 125541 [details] Patch

Rebased version incorporating some of rniwa suggestions. Let's see if the EWS are fine with it. Regarding the attributeCount(), I think is a bit soon to assume this function will be always just an accessor to a member variable. Since "attribute count" conceptually depends on lazily updated data (the invalid attributes), we may want in the future to make attributeCount() make sure the data is updated. Ryosuke, could you take another look?

Comment on attachment 125541 [details] Patch Attachment 125541 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11432078 New failing tests: inspector/elements/set-outer-html-2.html inspector/elements/set-outer-html.html

Created attachment 125628 [details] Patch

Created attachment 125630 [details] Patch

(In reply to comment #15) > (From update of attachment 125541 [details]) > Attachment 125541 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/11432078 > > New failing tests: > inspector/elements/set-outer-html-2.html > inspector/elements/set-outer-html.html I've tried to reuse existing logic of Element::setAttributesFromElement() to avoid manually copying, but its code inside NamedNodeMap doesn't take Inspector into account (will/didModifyAttribute). I prefer to handle this as a separate change too. So the current patch includes a FIXME for this case too.

Comment on attachment 125630 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125630&action=review > Source/WebCore/dom/Element.h:135 > + // This variant will not update the potentially invalid attributes. To be used when not interested > + // in style attribute or one of the SVG animation attributes. > + bool hasAttributesWithoutUpdate() const; Instead of adding comments, should we rename the function to hasAttributesWithoutUpdatingStyleAndSVGAnimations()? > Source/WebCore/dom/Element.h:160 > + // Internal methods that assume the existence of attribute storage, one should use hasAttributes() > + // before calling them. > + size_t attributeCount() const; > + Attribute* attributeItem(unsigned index) const; > + Attribute* getAttributeItem(const QualifiedName&) const; > + void removeAttribute(unsigned index); Can we eventually make them private/protected? > Source/WebCore/xml/XPathNodeSet.cpp:216 > + Element* e = toElement(n); Please spell-out element.

Committed r106833: <http://trac.webkit.org/changeset/106833>

(In reply to comment #19) > (From update of attachment 125630 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=125630&action=review > > > Source/WebCore/dom/Element.h:135 > > + // This variant will not update the potentially invalid attributes. To be used when not interested > > + // in style attribute or one of the SVG animation attributes. > > + bool hasAttributesWithoutUpdate() const; > > Instead of adding comments, should we rename the function to hasAttributesWithoutUpdatingStyleAndSVGAnimations()? We had a follow up discussion in IRC and I created bug 77892, adding Darin to the CC. > > Source/WebCore/dom/Element.h:160 > > + // Internal methods that assume the existence of attribute storage, one should use hasAttributes() > > + // before calling them. > > + size_t attributeCount() const; > > + Attribute* attributeItem(unsigned index) const; > > + Attribute* getAttributeItem(const QualifiedName&) const; > > + void removeAttribute(unsigned index); > > Can we eventually make them private/protected? Ack. I'll keep this in mind. > > Source/WebCore/xml/XPathNodeSet.cpp:216 > > + Element* e = toElement(n); > > Please spell-out element. Fixed.

(In reply to comment #20) > Committed r106833: <http://trac.webkit.org/changeset/106833> Reopen, because it broke the Mac builds.

Sorry, but it is absolutely innocent, r106932 is the culprit. ( r106932 and r106833 are so similar and they modified the problematic file :) )