Bug 143600 – Use modern for-loops in Document

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Joonghun Park]

Use modern for-loops in Document.

[Joonghun Park]

Created attachment 250507 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 250507 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Use modern for-loops in Document

This title is misleading, I can't see any range-based 
loop, but replacing explicit types with autos.

> Source/WebCore/dom/Document.cpp:4441
> -    HashSet<Element*>::iterator end = m_privateBrowsingStateChangedElements.end();
> -    for (HashSet<Element*>::iterator it = m_privateBrowsingStateChangedElements.begin(); it != end; ++it)
> +    for (HashSet<Element*>::iterator it = m_privateBrowsingStateChangedElements.begin(), end = m_privateBrowsingStateChangedElements.end(); it != end; ++it)

why don't you use auto here similar to other places?

[Joonghun Park]

(In reply to comment #2)
> Comment on attachment 250507 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250507&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Use modern for-loops in Document
> 
> This title is misleading, I can't see any range-based 
> loop, but replacing explicit types with autos.
> 
Oh, I mis-used the term 'modern for-loops'. I will change the title to 'Replace explicit types with auto in Document'. :)
> > Source/WebCore/dom/Document.cpp:4441
> > -    HashSet<Element*>::iterator end = m_privateBrowsingStateChangedElements.end();
> > -    for (HashSet<Element*>::iterator it = m_privateBrowsingStateChangedElements.begin(); it != end; ++it)
> > +    for (HashSet<Element*>::iterator it = m_privateBrowsingStateChangedElements.begin(), end = m_privateBrowsingStateChangedElements.end(); it != end; ++it)
> 
> why don't you use auto here similar to other places?
I will apply your comment :) Thank you~

[Joonghun Park]

Created attachment 250517 [details]
Patch

[Darin Adler]

Comment on attachment 250517 [details]
Patch

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

All of these *could* use modern for loops. And should.

> Source/WebCore/dom/Document.cpp:3771
>      HashSet<NodeIterator*> nodeIteratorsList = m_nodeIterators;

Really strange to call this “list” when it’s a set, not a list.

> Source/WebCore/dom/Document.cpp:3772
> +    for (auto it = nodeIteratorsList.begin(), nodeIteratorsEnd = nodeIteratorsList.end(); it != nodeIteratorsEnd; ++it) {

This could be converted to a modern for loop:

    for (auto& iterator : nodeIteratorsList) {
        if (iterator->root() == node) {
            detachNodeIterator(iterator);

etc. Unless I am missing something.

> Source/WebCore/dom/Document.cpp:3816
> +    for (auto it = m_nodeIterators.begin(), nodeIteratorsEnd = m_nodeIterators.end(); it != nodeIteratorsEnd; ++it)

Same here.

> Source/WebCore/dom/Document.cpp:3820
> +        for (auto it = m_ranges.begin(), rangesEnd = m_ranges.end(); it != rangesEnd; ++it)

Same here.

> Source/WebCore/dom/Document.cpp:3871
> +        for (auto it = m_ranges.begin(), end = m_ranges.end(); it != end; ++it)

Same here.

> Source/WebCore/dom/Document.cpp:4375
> +    for (auto i = m_documentSuspensionCallbackElements.begin(), end = m_documentSuspensionCallbackElements.end(); i != end; ++i)

Same here.

> Source/WebCore/dom/Document.cpp:4394
> +    for (auto i = elements.begin(), end = elements.end(); i != end; ++i)

Same here.

> Source/WebCore/dom/Document.cpp:4441
> +    for (auto it = m_privateBrowsingStateChangedElements.begin(), end = m_privateBrowsingStateChangedElements.end(); it != end; ++it)

Same here.

> Source/WebCore/dom/Document.cpp:4471
> +    for (auto it = m_captionPreferencesChangedElements.begin(), end = m_captionPreferencesChangedElements.end(); it != end; ++it)

Same here.

> Source/WebCore/dom/Document.cpp:5451
> +    for (auto i = descendants.begin(); i != descendants.end(); ++i) {

Same here.

[Joonghun Park]

(In reply to comment #5)
> Comment on attachment 250517 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250517&action=review
> 
> All of these *could* use modern for loops. And should.
> 
You're right. I revised all of the things in the previous patches and more than that.
 
> > Source/WebCore/dom/Document.cpp:3771
> >      HashSet<NodeIterator*> nodeIteratorsList = m_nodeIterators;
> 
> Really strange to call this “list” when it’s a set, not a list.
> 
I changed list to set.
> > Source/WebCore/dom/Document.cpp:3772
> > +    for (auto it = nodeIteratorsList.begin(), nodeIteratorsEnd = nodeIteratorsList.end(); it != nodeIteratorsEnd; ++it) {
> 
> This could be converted to a modern for loop:
> 
>     for (auto& iterator : nodeIteratorsList) {
>         if (iterator->root() == node) {
>             detachNodeIterator(iterator);
> 
> etc. Unless I am missing something.
> 
Yepp, you're right. In addition I use auto* because its type is NodeInterator*.

> > Source/WebCore/dom/Document.cpp:3816
> > +    for (auto it = m_nodeIterators.begin(), nodeIteratorsEnd = m_nodeIterators.end(); it != nodeIteratorsEnd; ++it)
> 
> Same here.
> 
> > Source/WebCore/dom/Document.cpp:3820
> > +        for (auto it = m_ranges.begin(), rangesEnd = m_ranges.end(); it != rangesEnd; ++it)
> 
> Same here.
> 
> > Source/WebCore/dom/Document.cpp:3871
> > +        for (auto it = m_ranges.begin(), end = m_ranges.end(); it != end; ++it)
> 
> Same here.
> 
> > Source/WebCore/dom/Document.cpp:4375
> > +    for (auto i = m_documentSuspensionCallbackElements.begin(), end = m_documentSuspensionCallbackElements.end(); i != end; ++i)
> 
> Same here.
> 
> > Source/WebCore/dom/Document.cpp:4394
> > +    for (auto i = elements.begin(), end = elements.end(); i != end; ++i)
> 
> Same here.
> 
> > Source/WebCore/dom/Document.cpp:4441
> > +    for (auto it = m_privateBrowsingStateChangedElements.begin(), end = m_privateBrowsingStateChangedElements.end(); it != end; ++it)
> 
> Same here.
> 
> > Source/WebCore/dom/Document.cpp:4471
> > +    for (auto it = m_captionPreferencesChangedElements.begin(), end = m_captionPreferencesChangedElements.end(); it != end; ++it)
> 
> Same here.
> 
> > Source/WebCore/dom/Document.cpp:5451
> > +    for (auto i = descendants.begin(); i != descendants.end(); ++i) {
> 
> Same here.

Thank you for your kind review and teaching, Darin :)

[Joonghun Park]

Created attachment 250549 [details]
Patch

[Joonghun Park]

Created attachment 250553 [details]
Patch

[Joonghun Park]

Created attachment 250555 [details]
Patch

[Darin Adler]

Comment on attachment 250555 [details]
Patch

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

review- because of the mistake below where we do return instead of break; other comments are all just ways to slightly improve

> Source/WebCore/dom/Document.cpp:2819
> +            return;

This needs to be "break", not "return"!

Also, I suggest putting it at the end of the loop body rather than the start; either is OK but I think it makes better logical sense at the end.

Or we could change it to only check after setting href or target like this:

    ... {
        href = &value;
        if (target)
            break;
    }

    ... {
        target = &value
        if (href)
            break;
    }

> Source/WebCore/dom/Document.cpp:3773
> +    HashSet<NodeIterator*> nodeIteratorsSet = m_nodeIterators;

Could use auto here; not sure it would be better. Could also change this to use copyToVector instead of copying into another set. I believe that would be more efficient than copying the set, although the code would read as less elegant.

> Source/WebCore/dom/Document.cpp:3784
>      if (!m_ranges.isEmpty()) {

This if statement should be removed. The loop in the body will already efficiently do nothing if m_ranges is empty.

> Source/WebCore/dom/Document.cpp:3792
>      if (!m_ranges.isEmpty()) {

This if statement should be removed. The loop in the body will already efficiently do nothing if m_ranges is empty.

> Source/WebCore/dom/Document.cpp:3821
>      if (!m_ranges.isEmpty()) {

This if statement should be removed. The loop in the body will already efficiently do nothing if m_ranges is empty.

> Source/WebCore/dom/Document.cpp:3872
>      if (!m_ranges.isEmpty()) {

This if statement should be removed. The loop in the body will already efficiently do nothing if m_ranges is empty.

[Joonghun Park]

(In reply to comment #10)
> Comment on attachment 250555 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250555&action=review
> 
> review- because of the mistake below where we do return instead of break;
> other comments are all just ways to slightly improve
> 
> > Source/WebCore/dom/Document.cpp:2819
> > +            return;
> 
> This needs to be "break", not "return"!
> 
> Also, I suggest putting it at the end of the loop body rather than the
> start; either is OK but I think it makes better logical sense at the end.
> 
> Or we could change it to only check after setting href or target like this:
> 
>     ... {
>         href = &value;
>         if (target)
>             break;
>     }
> 
>     ... {
>         target = &value
>         if (href)
>             break;
>     }
> 
Ouch, I corrected the mistake I made. And your suggestion is really cool :)
I applied your idea.
> > Source/WebCore/dom/Document.cpp:3773
> > +    HashSet<NodeIterator*> nodeIteratorsSet = m_nodeIterators;
> 
> Could use auto here; not sure it would be better. Could also change this to
> use copyToVector instead of copying into another set. I believe that would
> be more efficient than copying the set, although the code would read as less
> elegant.
> 
I revised it to use vector instead of HashSet. I agree with you that it will be more efficient than to use HashSet.
> > Source/WebCore/dom/Document.cpp:3784
> >      if (!m_ranges.isEmpty()) {
> 
> This if statement should be removed. The loop in the body will already
> efficiently do nothing if m_ranges is empty.
> 
I applied this change.
> > Source/WebCore/dom/Document.cpp:3792
> >      if (!m_ranges.isEmpty()) {
> 
> This if statement should be removed. The loop in the body will already
> efficiently do nothing if m_ranges is empty.
> 
I applied this change.
> > Source/WebCore/dom/Document.cpp:3821
> >      if (!m_ranges.isEmpty()) {
> 
> This if statement should be removed. The loop in the body will already
> efficiently do nothing if m_ranges is empty.
> 
I applied this change.
> > Source/WebCore/dom/Document.cpp:3872
> >      if (!m_ranges.isEmpty()) {
> 
> This if statement should be removed. The loop in the body will already
> efficiently do nothing if m_ranges is empty.
I applied this change.

[Joonghun Park]

Created attachment 250559 [details]
Patch

[Joonghun Park]

Created attachment 250561 [details]
Patch

[Darin Adler]

Comment on attachment 250561 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:5017
> +    for (auto it : m_textAutoSizedNodes) {

It would be more efficient to use auto& here; using auto means we copy both the key and value each time. It’s not great to name the loop variable “it” because it’s not an iterator.

> Source/WebCore/dom/Document.cpp:5018
> +        RefPtr<TextAutoSizingValue> value = it.value;

I wonder why we use a RefPtr here instead of a raw pointer. Seems like it might be unneeded reference count churn. The only reason to use a RefPtr would be if there is some risk that adjustNodeSizes might cause the value to be released, but if it does, then we have a bigger problem because if m_textAutoSizedNodes is modified while we are iterating it we have a problem.

> Source/WebCore/dom/Document.cpp:5029
>      unsigned count = nodesForRemoval.size();
>      for (unsigned i = 0; i < count; i++)

This should use a modern for loop.

[Joonghun Park]

(In reply to comment #14)
> Comment on attachment 250561 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250561&action=review
> 
> > Source/WebCore/dom/Document.cpp:5017
> > +    for (auto it : m_textAutoSizedNodes) {
> 
> It would be more efficient to use auto& here; using auto means we copy both
> the key and value each time. It’s not great to name the loop variable “it”
> because it’s not an iterator.
> 
I revised the name to keyValuePair instead of it.
> > Source/WebCore/dom/Document.cpp:5018
> > +        RefPtr<TextAutoSizingValue> value = it.value;
> 
> I wonder why we use a RefPtr here instead of a raw pointer. Seems like it
> might be unneeded reference count churn. The only reason to use a RefPtr
> would be if there is some risk that adjustNodeSizes might cause the value to
> be released, but if it does, then we have a bigger problem because if
> m_textAutoSizedNodes is modified while we are iterating it we have a problem.
> 
When I see in the adjustNodeSizes(), it seems that there is no such place in which releases the value itself, so I think RefPtr was used because it's simply the type of the HashMap's value. I changed it to raw pointer to avoid refCount churn as you pointed out.
> > Source/WebCore/dom/Document.cpp:5029
> >      unsigned count = nodesForRemoval.size();
> >      for (unsigned i = 0; i < count; i++)
> 
> This should use a modern for loop.
I changed this to use modern for loop.

Thank you for your review :)

[Joonghun Park]

Created attachment 250610 [details]
Patch

[Darin Adler]

Comment on attachment 250610 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:6100
> +    for (auto& keyValuePair : *targets) {

I noticed that this loop’s body only accesses the keys, so it could be:

    for (auto& key : targets->keys()) {

[Joonghun Park]

(In reply to comment #17)
> Comment on attachment 250610 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250610&action=review
> 
> > Source/WebCore/dom/Document.cpp:6100
> > +    for (auto& keyValuePair : *targets) {
> 
> I noticed that this loop’s body only accesses the keys, so it could be:
> 
>     for (auto& key : targets->keys()) {

The type of EventTargetSet is HashCountedSet<Node*>, so keys() is not a member of HashCountedSet.
If we have a function which return the member HashMap like 
hashMap() { return m_impl; } in HashCountedSet, 
then it would be possible to write as following,
    for (auto* key : targets->hashMap().keys())
but currently there is no function in HashCountedSet.
IMHO, I couldn't find a way to apply your comment. :)

[Darin Adler]

(In reply to comment #18)
> (In reply to comment #17)
> > Comment on attachment 250610 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=250610&action=review
> > 
> > > Source/WebCore/dom/Document.cpp:6100
> > > +    for (auto& keyValuePair : *targets) {
> > 
> > I noticed that this loop’s body only accesses the keys, so it could be:
> > 
> >     for (auto& key : targets->keys()) {
> 
> The type of EventTargetSet is HashCountedSet<Node*>, so keys() is not a
> member of HashCountedSet.
> If we have a function which return the member HashMap like 
> hashMap() { return m_impl; } in HashCountedSet, 
> then it would be possible to write as following,
>     for (auto* key : targets->hashMap().keys())
> but currently there is no function in HashCountedSet.
> IMHO, I couldn't find a way to apply your comment. :)

Makes sense. Some day we could add adapters to HashCountedSet like the ones we have for HashMap, but for now I understand that this can’t be done. We should not expose the HashMap, though.

[WebKit Commit Bot]

Comment on attachment 250610 [details]
Patch

Clearing flags on attachment: 250610

Committed r182782: <http://trac.webkit.org/changeset/182782>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.