Bug 126250 - Convert some of WebCore/dom over to range-for loops
Summary: Convert some of WebCore/dom over to range-for loops
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Csaba Osztrogonác
URL:
Keywords:
Depends on: 126256
Blocks:
  Show dependency treegraph
 
Reported: 2013-12-26 17:55 PST by Sam Weinig
Modified: 2015-06-26 07:45 PDT (History)
8 users (show)

See Also:


Attachments
Patch (37.25 KB, patch)
2013-12-26 18:06 PST, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (12.92 KB, patch)
2015-06-18 07:03 PDT, Csaba Osztrogonác
darin: review+
Details | Formatted Diff | Diff
patch for landing (12.82 KB, patch)
2015-06-26 04:23 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2013-12-26 17:55:57 PST
Convert some of WebCore/dom over to range-for loops
Comment 1 Sam Weinig 2013-12-26 18:06:17 PST
Created attachment 220042 [details]
Patch
Comment 2 Andreas Kling 2013-12-26 18:15:13 PST
Comment on attachment 220042 [details]
Patch

r=me

I love auto* instead of just auto for pointers. We should make that a style rule.
Comment 3 Sam Weinig 2013-12-26 18:52:33 PST
Committed r161096: <http://trac.webkit.org/changeset/161096>
Comment 4 Csaba Osztrogonác 2013-12-26 23:57:35 PST
(In reply to comment #3)
> Committed r161096: <http://trac.webkit.org/changeset/161096>
FYI: It made all tests crash. Please fix it.
Comment 5 WebKit Commit Bot 2013-12-27 00:20:48 PST
Re-opened since this is blocked by bug 126256
Comment 6 Alexey Proskuryakov 2013-12-27 00:24:01 PST
> FYI: It made all tests crash. Please fix it.

I don't think that it's OK to leave tests crashing for any period of time, rolled out in <https://trac.webkit.org/r161097>.

This crashes in both debug and release. Example debug crash (assertion failure): http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r161096%20(1233)/canvas/philip/tests/2d.composite.uncovered.fill.copy-crash-log.txt

Full results:

http://build.webkit.org/results/Apple%20Mavericks%20Release%20WK2%20(Tests)/r161096%20(1785)/results.html

http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r161096%20(1233)/results.html
Comment 7 Csaba Osztrogonác 2015-02-26 06:39:49 PST
Are you going to update and relande this patch in the near future?
I checked some random part of the patch, they are still valid.
Comment 8 Csaba Osztrogonác 2015-06-18 07:03:15 PDT
Created attachment 255112 [details]
Patch

rebased to top of trunk, patch for EWS
Comment 9 Darin Adler 2015-06-19 11:12:40 PDT
Comment on attachment 255112 [details]
Patch

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

> Source/WebCore/dom/ContainerNode.cpp:458
> +        treeScope().adoptIfNeeded(&child.get());

We have been using child.ptr() instead of &child.get(). Not sure it’s better, but it is one less & character.

> Source/WebCore/dom/ContainerNode.cpp:466
> +                appendChildToContainer(&child.get(), *this);

Ditto.

> Source/WebCore/dom/ContainerNode.cpp:711
> +        treeScope().adoptIfNeeded(&child.get());

Ditto.

> Source/WebCore/dom/ContainerNode.cpp:716
> +            appendChildToContainer(&child.get(), *this);

Ditto.

> Source/WebCore/dom/MutationObserverInterestGroup.cpp:71
> +    for (auto& observerOptionsPair : m_observers) {

Could use an even shorter name here, but I guess this is OK.

> Source/WebCore/dom/Node.cpp:234
> +    for (auto stringSizePair : perTagCount)

This should be auto&; we don’t want to copy all these strings and churn their reference counts for no good reason, even in this logging code.
Comment 10 Csaba Osztrogonác 2015-06-26 04:23:30 PDT
Created attachment 255629 [details]
patch for landing
Comment 11 WebKit Commit Bot 2015-06-26 06:02:25 PDT
Comment on attachment 255629 [details]
patch for landing

Clearing flags on attachment: 255629

Committed r185994: <http://trac.webkit.org/changeset/185994>