Add iterator object to iterate efficiently over an Element's attributes

Add iterator object to iterate efficiently over an Element's attributes

Add iterator object to iterate efficiently over an Element's attributes instead
of using ElementData::length() / ElementData::attributeItem(index).

This is a bit more efficient as it avoids implicitly calling
ElementData::isUnique() for each loop iteration.

This is based on the following WebKit revision by benjamin@webkit.org:
http://trac.webkit.org/changeset/162394

R=esprehn@chromium.org, adamk@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175583

[esprehn, 2014-05-28 20:03:54]

Can we just remove the AttributeIteratorAccessor class and use attributeCount()
instead? This class doesn't seem like it adds value.

https://codereview.chromium.org/298253009/diff/1/Source/core/css/SelectorChec...
File Source/core/css/SelectorChecker.cpp (right):

https://codereview.chromium.org/298253009/diff/1/Source/core/css/SelectorChec...
Source/core/css/SelectorChecker.cpp:495: AttributeIteratorAccessor attributes =
element.attributesIterator();
This accessor thing is kind of weird, why can't Element have the iterator
methods?

https://codereview.chromium.org/298253009/diff/1/Source/core/dom/ElementData.cpp
File Source/core/dom/ElementData.cpp (right):

https://codereview.chromium.org/298253009/diff/1/Source/core/dom/ElementData....
Source/core/dom/ElementData.cpp:137: if (shouldIgnoreAttributeCase &&
equalIgnoringCase(name, it->localName()))
Why doesn't this side use the equalPossiblyIgnoringCase method?

https://codereview.chromium.org/298253009/diff/1/Source/core/dom/ElementData.h
File Source/core/dom/ElementData.h (right):

https://codereview.chromium.org/298253009/diff/1/Source/core/dom/ElementData....
Source/core/dom/ElementData.h:3: * Copyright (C) 2014 Apple Inc. All rights
reserved.
Do we really need to add Apple copyrights?

https://codereview.chromium.org/298253009/diff/1/Source/core/dom/ElementData....
Source/core/dom/ElementData.h:67: class AttributeIteratorAccessor {
This class seems unnecessary.

https://codereview.chromium.org/298253009/diff/1/Source/core/dom/ElementData....
Source/core/dom/ElementData.h:108: AttributeIteratorAccessor
attributesIterator() const;
Can we just add attributesBegin() and attributesEnd() instead? Why do we need
this class?

[Inactive, 2014-05-29 15:14:01]

Based on my comments, please let me know if you still think I should get rid of
AttributeIteratorAccessor in this CL (then maybe we can introduce it back when
we switch to c++11).

https://codereview.chromium.org/298253009/diff/1/Source/core/dom/ElementData.cpp
File Source/core/dom/ElementData.cpp (right):

https://codereview.chromium.org/298253009/diff/1/Source/core/dom/ElementData....
Source/core/dom/ElementData.cpp:137: if (shouldIgnoreAttributeCase &&
equalIgnoringCase(name, it->localName()))
On 2014/05/28 20:03:54, esprehn wrote:
> Why doesn't this side use the equalPossiblyIgnoringCase method?

This is the "slow" case method, it is only called if:
- The case should be ignored (if case here)
- One of the attributes had a prefix (else case here)

The caller of this method already has a fast path doing exact string match,
therefore, we only want to check if the unprefixed-attribute is match if
shouldIgnoreAttributeCase is true, and do a case insensitive check.

https://codereview.chromium.org/298253009/diff/1/Source/core/dom/ElementData.h
File Source/core/dom/ElementData.h (right):

https://codereview.chromium.org/298253009/diff/1/Source/core/dom/ElementData....
Source/core/dom/ElementData.h:3: * Copyright (C) 2014 Apple Inc. All rights
reserved.
On 2014/05/28 20:03:54, esprehn wrote:
> Do we really need to add Apple copyrights?

I don't know what the exact rules are but it seems (to me) like the right thing
to do since I am porting over once of their patches.

FYI, I have done this so far when porting patches over to Blink.

https://codereview.chromium.org/298253009/diff/1/Source/core/dom/ElementData....
Source/core/dom/ElementData.h:67: class AttributeIteratorAccessor {
On 2014/05/28 20:03:54, esprehn wrote:
> This class seems unnecessary.

The main reason for this class (besides avoiding multiple calls to isUnique() as
explained below), is that this will let us use range iterators when we
eventually start using C++11 (http://www.stroustrup.com/C++11FAQ.html#for):
for (const Attribute& attribute : attributesIterator())

For the range-for statement to work, the object needs to have begin() and end()
methods defines (i.e. not attributesBegin() / attributesEnd()). Defining those
on ElementData may be a bit confusing. Defining those on Element would
definitely be confusing as it would not be obvious the iterators are for the
Element's attributes.

https://codereview.chromium.org/298253009/diff/1/Source/core/dom/ElementData....
Source/core/dom/ElementData.h:108: AttributeIteratorAccessor
attributesIterator() const;
On 2014/05/28 20:03:54, esprehn wrote:
> Can we just add attributesBegin() and attributesEnd() instead? Why do we need
> this class?

If we do this then we will call ElementData::isUnique() twice, once in
attributesBegin() and once in attributesEnd(), right? Instead of now calling it
only once when calling attributesData().

[Inactive, 2014-05-29 15:17:59]

https://codereview.chromium.org/298253009/diff/1/Source/core/dom/ElementData.h
File Source/core/dom/ElementData.h (right):

https://codereview.chromium.org/298253009/diff/1/Source/core/dom/ElementData....
Source/core/dom/ElementData.h:3: * Copyright (C) 2014 Apple Inc. All rights
reserved.
On 2014/05/29 15:14:01, Chris Dumez wrote:
> On 2014/05/28 20:03:54, esprehn wrote:
> > Do we really need to add Apple copyrights?
> 
> I don't know what the exact rules are but it seems (to me) like the right
thing
> to do since I am porting over once of their patches.
> 
> FYI, I have done this so far when porting patches over to Blink.

Also, this CL seems to hint that I am not the only one doing this:
https://codereview.chromium.org/305653007/

[Inactive, 2014-05-30 13:16:02]

https://codereview.chromium.org/298253009/diff/1/Source/core/dom/ElementData.h
File Source/core/dom/ElementData.h (right):

https://codereview.chromium.org/298253009/diff/1/Source/core/dom/ElementData....
Source/core/dom/ElementData.h:108: AttributeIteratorAccessor
attributesIterator() const;
On 2014/05/29 15:14:01, Chris Dumez wrote:
> On 2014/05/28 20:03:54, esprehn wrote:
> > Can we just add attributesBegin() and attributesEnd() instead? Why do we
need
> > this class?
> 
> If we do this then we will call ElementData::isUnique() twice, once in
> attributesBegin() and once in attributesEnd(), right? Instead of now calling
it
> only once when calling attributesData().

Note that we had issues before with callers not caching attributeCount() before
the loop and thus calling ElementData::isUnique() unnecessarily for every loop
iteration. The proposal at the time was to introduce iterators to avoid this.
However, if attributesEnd() hides a call to ElementData::isUnique(), then the
same type of error is also possible if the caller fails to cache the end
iterator before the loop.

[Inactive, 2014-06-02 20:48:48]

esprehn, ping review? I'd really like your feedback on this.

[esprehn, 2014-06-05 01:10:56]

On 2014/06/02 at 20:48:48, ch.dumez wrote:
> esprehn, ping review? I'd really like your feedback on this.

I'll look over this asap, sorry for the delay.

[Inactive, 2014-06-05 02:43:03]

On 2014/06/05 01:10:56, esprehn wrote:
> On 2014/06/02 at 20:48:48, ch.dumez wrote:
> > esprehn, ping review? I'd really like your feedback on this.
> 
> I'll look over this asap, sorry for the delay.

Thanks, no problem.

[esprehn, 2014-06-05 05:30:37]

lgtm

[Inactive, 2014-06-05 12:19:01]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-06-05 12:20:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/298253009/1

[commit-bot: I haz the power, 2014-06-05 12:20:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-05 12:20:33]

Failed to apply patch for Source/core/inspector/DOMPatchSupport.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/inspector/DOMPatchSupport.cpp
  Hunk #2 FAILED at 431.
  1 out of 2 hunks FAILED -- saving rejects to file
Source/core/inspector/DOMPatchSupport.cpp.rej

Patch:       Source/core/inspector/DOMPatchSupport.cpp
Index: Source/core/inspector/DOMPatchSupport.cpp
diff --git a/Source/core/inspector/DOMPatchSupport.cpp
b/Source/core/inspector/DOMPatchSupport.cpp
index
8afcb0dbce2770433d52c5ab64d16a8787576fd7..5a046fd6e2fcb348a64416daf203f621dea2f998
100644
--- a/Source/core/inspector/DOMPatchSupport.cpp
+++ b/Source/core/inspector/DOMPatchSupport.cpp
@@ -199,10 +199,10 @@ bool DOMPatchSupport::innerPatchNode(Digest* oldDigest,
Digest* newDigest, Excep
 
         // FIXME: Create a function in Element for copying properties.
cloneDataFromElement() is close but not enough for this case.
         if (newElement->hasAttributesWithoutUpdate()) {
-            size_t numAttrs = newElement->attributeCount();
-            for (size_t i = 0; i < numAttrs; ++i) {
-                const Attribute& attribute = newElement->attributeItem(i);
-                if (!m_domEditor->setAttribute(oldElement,
attribute.name().localName(), attribute.value(), exceptionState))
+            AttributeIteratorAccessor attributes =
newElement->attributesIterator();
+            AttributeConstIterator end = attributes.end();
+            for (AttributeConstIterator it = attributes.begin(); it != end;
++it) {
+                if (!m_domEditor->setAttribute(oldElement,
it->name().localName(), it->value(), exceptionState))
                     return false;
             }
         }
@@ -431,12 +431,12 @@ PassOwnPtr<DOMPatchSupport::Digest>
DOMPatchSupport::createDigest(Node* node, Un
         Element* element = toElement(node);
 
         if (element->hasAttributesWithoutUpdate()) {
-            size_t numAttrs = element->attributeCount();
             OwnPtr<blink::WebCryptoDigestor> attrsDigestor =
createDigestor(HashAlgorithmSha1);
-            for (size_t i = 0; i < numAttrs; ++i) {
-                const Attribute& attribute = element->attributeItem(i);
-                addStringToDigestor(attrsDigestor.get(),
attribute.name().toString());
-                addStringToDigestor(attrsDigestor.get(),
attribute.value().string());
+            AttributeIteratorAccessor attributes =
element->attributesIterator();
+            AttributeConstIterator end = attributes.end();
+            for (AttributeConstIterator it = attributes.begin(); it != end;
++it) {
+                addStringToDigestor(attrsDigestor.get(),
it->name().toString());
+                addStringToDigestor(attrsDigestor.get(), it->value().string());
             }
             finishDigestor(attrsDigestor.get(), digestResult);
             digest->m_attrsSHA1 = base64Encode(reinterpret_cast<const
char*>(digestResult.data()), 10);

[Inactive, 2014-06-05 13:07:03]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-06-05 13:07:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/298253009/20001

[commit-bot: I haz the power, 2014-06-05 14:13:03]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/10428)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/10794)

[commit-bot: I haz the power, 2014-06-05 15:18:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-05 15:18:11]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/10807)

[Inactive, 2014-06-05 15:42:34]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-06-05 15:43:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/298253009/20001

[commit-bot: I haz the power, 2014-06-05 16:44:25]

Change committed as 175583