Use C++11 range-based loop for core/clipboard, core/dom and core/testing

Use C++11 range-based loop for core/clipboard, core/dom and core/testing

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

[zhaoze.zhou, 2014-12-11 18:21:29]

Patchset #3 (id:40001) has been deleted

[zhaoze.zhou, 2014-12-11 18:21:34]

Patchset #2 (id:20001) has been deleted

[zhaoze.zhou, 2014-12-16 15:20:02]

zhaoze.zhou@partner.samsung.com changed reviewers:
+ rob.buis@samsung.com

[Julien - ping for review, 2014-12-16 16:37:21]

jchaffraix@chromium.org changed reviewers:
+ jchaffraix@chromium.org

[Julien - ping for review, 2014-12-16 16:37:22]

This patch shouldn't land without good variable names. The type of the variables
are not iterators anymore :(

https://codereview.chromium.org/796913002/diff/60001/Source/core/clipboard/Da...
File Source/core/clipboard/DataObject.cpp (right):

https://codereview.chromium.org/796913002/diff/60001/Source/core/clipboard/Da...
Source/core/clipboard/DataObject.cpp:52: for (const auto& it : types) {
s/it/moreMeaningFullVariableName/ :(

[zhaoze.zhou, 2014-12-16 19:47:35]

https://codereview.chromium.org/796913002/diff/60001/Source/core/clipboard/Da...
File Source/core/clipboard/DataObject.cpp (right):

https://codereview.chromium.org/796913002/diff/60001/Source/core/clipboard/Da...
Source/core/clipboard/DataObject.cpp:52: for (const auto& it : types) {
On 2014/12/16 16:37:21, Julien Chaffraix - OOO til 1-1 wrote:
> s/it/moreMeaningFullVariableName/ :(

Done.

[dcheng, 2014-12-16 19:52:50]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2014-12-16 19:52:51]

https://codereview.chromium.org/796913002/diff/80001/Source/core/dom/Document...
File Source/core/dom/DocumentMarkerController.cpp (right):

https://codereview.chromium.org/796913002/diff/80001/Source/core/dom/Document...
Source/core/dom/DocumentMarkerController.cpp:447: for (DocumentMarker* marker  :
markersFor(node)) {
Nit: space.

https://codereview.chromium.org/796913002/diff/80001/Source/core/dom/ElementD...
File Source/core/dom/ElementData.cpp (right):

https://codereview.chromium.org/796913002/diff/80001/Source/core/dom/ElementD...
Source/core/dom/ElementData.cpp:117: for (auto attribute : attributes) {
Please don't use bare 'auto'; without looking at AttributeCollection, it's hard
for me to tell if this is a cheap pointer copy or not.

https://codereview.chromium.org/796913002/diff/80001/Source/core/dom/Fullscre...
File Source/core/dom/Fullscreen.cpp (right):

https://codereview.chromium.org/796913002/diff/80001/Source/core/dom/Fullscre...
Source/core/dom/Fullscreen.cpp:342: for (auto descendant : descendants) {
Doesn't this end up ref and unref'ing everything in the container, since the
deque holds a RefPtr<>?

https://codereview.chromium.org/796913002/diff/80001/Source/core/dom/Mutation...
File Source/core/dom/MutationObserver.cpp (right):

https://codereview.chromium.org/796913002/diff/80001/Source/core/dom/Mutation...
Source/core/dom/MutationObserver.cpp:151: for (auto registration :
registrations)
Similar comment for all the remaining autos. I would prefer to see these as
auto&/auto* to make it clear that we're not actually performing expensive value
copies or shuffling ref counts around for no reason.

[zhaoze.zhou, 2014-12-16 22:39:55]

Patchset #4 (id:100001) has been deleted

[zhaoze.zhou, 2014-12-16 23:08:33]

Patchset #4 (id:120001) has been deleted

[zhaoze.zhou, 2014-12-16 23:27:57]

https://codereview.chromium.org/796913002/diff/80001/Source/core/dom/Document...
File Source/core/dom/DocumentMarkerController.cpp (right):

https://codereview.chromium.org/796913002/diff/80001/Source/core/dom/Document...
Source/core/dom/DocumentMarkerController.cpp:447: for (DocumentMarker* marker  :
markersFor(node)) {
On 2014/12/16 19:52:51, dcheng wrote:
> Nit: space.

Done.

https://codereview.chromium.org/796913002/diff/80001/Source/core/dom/ElementD...
File Source/core/dom/ElementData.cpp (right):

https://codereview.chromium.org/796913002/diff/80001/Source/core/dom/ElementD...
Source/core/dom/ElementData.cpp:117: for (auto attribute : attributes) {
On 2014/12/16 19:52:51, dcheng wrote:
> Please don't use bare 'auto'; without looking at AttributeCollection, it's
hard
> for me to tell if this is a cheap pointer copy or not.

Done.

[Julien - ping for review, 2015-01-02 09:42:03]

lgtm with the comments addressed. Daniel should double-check too.

https://codereview.chromium.org/796913002/diff/140001/Source/core/clipboard/D...
File Source/core/clipboard/DataObject.cpp (right):

https://codereview.chromium.org/796913002/diff/140001/Source/core/clipboard/D...
Source/core/clipboard/DataObject.cpp:52: for (const auto& type : types) {
|type| is a String so we should keep the type (auto is better reserved for very
long names AFAICT).

https://codereview.chromium.org/796913002/diff/140001/Source/core/dom/Documen...
File Source/core/dom/DocumentMarkerController.cpp (right):

https://codereview.chromium.org/796913002/diff/140001/Source/core/dom/Documen...
Source/core/dom/DocumentMarkerController.cpp:447: for (DocumentMarker* marker :
markersFor(node)) {
const DocumentMarker* (it was a const iterator).

https://codereview.chromium.org/796913002/diff/140001/Source/core/dom/Documen...
Source/core/dom/DocumentMarkerController.cpp:721: for (DocumentMarker* marker :
markersFor(node)) {
const DocumentMarker* (again const iterator)

https://codereview.chromium.org/796913002/diff/140001/Source/core/dom/Element...
File Source/core/dom/ElementData.cpp (right):

https://codereview.chromium.org/796913002/diff/140001/Source/core/dom/Element...
Source/core/dom/ElementData.cpp:117: for (auto& attribute : attributes) {
const auto&

or better:

const Attribute*

https://codereview.chromium.org/796913002/diff/140001/Source/core/dom/StyleEn...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/796913002/diff/140001/Source/core/dom/StyleEn...
Source/core/dom/StyleEngine.cpp:349: for (auto& scope : treeScopes) {
auto == TreeScope*

I think it's OK to do a pointer copy here as it makes it really clear what's
going on and write:

for (TreeScope* scope : treeScopes) {

https://codereview.chromium.org/796913002/diff/140001/Source/core/dom/StyleEn...
Source/core/dom/StyleEngine.cpp:389: for (auto* treeScope : treeScopes) {
Similar comment.

https://codereview.chromium.org/796913002/diff/140001/Source/core/testing/Int...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/796913002/diff/140001/Source/core/testing/Int...
Source/core/testing/Internals.cpp:727: for (auto& marker : markers) {
const auto&

[zhaoze.zhou, 2015-01-05 15:17:57]

https://codereview.chromium.org/796913002/diff/140001/Source/core/clipboard/D...
File Source/core/clipboard/DataObject.cpp (right):

https://codereview.chromium.org/796913002/diff/140001/Source/core/clipboard/D...
Source/core/clipboard/DataObject.cpp:52: for (const auto& type : types) {
On 2015/01/02 09:42:02, Julien Chaffraix - CET wrote:
> |type| is a String so we should keep the type (auto is better reserved for
very
> long names AFAICT).

Done.

https://codereview.chromium.org/796913002/diff/140001/Source/core/dom/Documen...
File Source/core/dom/DocumentMarkerController.cpp (right):

https://codereview.chromium.org/796913002/diff/140001/Source/core/dom/Documen...
Source/core/dom/DocumentMarkerController.cpp:447: for (DocumentMarker* marker :
markersFor(node)) {
On 2015/01/02 09:42:02, Julien Chaffraix - CET wrote:
> const DocumentMarker* (it was a const iterator).

it's strange that this one can not mark as const. compiler will complain
foundMarkers.append(marker);

https://codereview.chromium.org/796913002/diff/140001/Source/core/dom/Documen...
Source/core/dom/DocumentMarkerController.cpp:721: for (DocumentMarker* marker :
markersFor(node)) {
On 2015/01/02 09:42:02, Julien Chaffraix - CET wrote:
> const DocumentMarker* (again const iterator)

Done.

https://codereview.chromium.org/796913002/diff/140001/Source/core/dom/Element...
File Source/core/dom/ElementData.cpp (right):

https://codereview.chromium.org/796913002/diff/140001/Source/core/dom/Element...
Source/core/dom/ElementData.cpp:117: for (auto& attribute : attributes) {
On 2015/01/02 09:42:02, Julien Chaffraix - CET wrote:
> const auto&
> 
> or better:
> 
> const Attribute*

Done.

https://codereview.chromium.org/796913002/diff/140001/Source/core/dom/StyleEn...
File Source/core/dom/StyleEngine.cpp (right):

https://codereview.chromium.org/796913002/diff/140001/Source/core/dom/StyleEn...
Source/core/dom/StyleEngine.cpp:349: for (auto& scope : treeScopes) {
On 2015/01/02 09:42:02, Julien Chaffraix - CET wrote:
> auto == TreeScope*
> 
> I think it's OK to do a pointer copy here as it makes it really clear what's
> going on and write:
> 
> for (TreeScope* scope : treeScopes) {

Done.

https://codereview.chromium.org/796913002/diff/140001/Source/core/dom/StyleEn...
Source/core/dom/StyleEngine.cpp:389: for (auto* treeScope : treeScopes) {
On 2015/01/02 09:42:02, Julien Chaffraix - CET wrote:
> Similar comment.

Done.

https://codereview.chromium.org/796913002/diff/140001/Source/core/testing/Int...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/796913002/diff/140001/Source/core/testing/Int...
Source/core/testing/Internals.cpp:727: for (auto& marker : markers) {
On 2015/01/02 09:42:02, Julien Chaffraix - CET wrote:
> const auto&

Done.

[dcheng, 2015-01-05 22:01:10]

https://codereview.chromium.org/796913002/diff/140001/Source/core/dom/Documen...
File Source/core/dom/DocumentMarkerController.cpp (right):

https://codereview.chromium.org/796913002/diff/140001/Source/core/dom/Documen...
Source/core/dom/DocumentMarkerController.cpp:447: for (DocumentMarker* marker :
markersFor(node)) {
On 2015/01/05 15:17:57, zhaoze.zhou wrote:
> On 2015/01/02 09:42:02, Julien Chaffraix - CET wrote:
> > const DocumentMarker* (it was a const iterator).
> 
> it's strange that this one can not mark as const. compiler will complain
> foundMarkers.append(marker);

This makes sense--DocumentMarkerVectors is a container of pointers to mutable
DocumentMarkers.

https://codereview.chromium.org/796913002/diff/160001/Source/core/clipboard/D...
File Source/core/clipboard/DataObject.cpp (right):

https://codereview.chromium.org/796913002/diff/160001/Source/core/clipboard/D...
Source/core/clipboard/DataObject.cpp:52: for (const String type : types) {
const String&.

This is copying strings; while String is a refcounted type in Blink, not copying
is even cheaper than a (relatively) cheap copy.

https://codereview.chromium.org/796913002/diff/160001/Source/core/dom/Element...
File Source/core/dom/ElementData.cpp (right):

https://codereview.chromium.org/796913002/diff/160001/Source/core/dom/Element...
Source/core/dom/ElementData.cpp:117: for (const Attribute attribute :
attributes) {
Isn't this copying Attribute? I think this should be const Attribute&

[zhaoze.zhou, 2015-01-09 02:30:05]

https://codereview.chromium.org/796913002/diff/160001/Source/core/clipboard/D...
File Source/core/clipboard/DataObject.cpp (right):

https://codereview.chromium.org/796913002/diff/160001/Source/core/clipboard/D...
Source/core/clipboard/DataObject.cpp:52: for (const String type : types) {
On 2015/01/05 22:01:09, dcheng wrote:
> const String&.
> 
> This is copying strings; while String is a refcounted type in Blink, not
copying
> is even cheaper than a (relatively) cheap copy.

Done.

https://codereview.chromium.org/796913002/diff/160001/Source/core/dom/Element...
File Source/core/dom/ElementData.cpp (right):

https://codereview.chromium.org/796913002/diff/160001/Source/core/dom/Element...
Source/core/dom/ElementData.cpp:117: for (const Attribute attribute :
attributes) {
On 2015/01/05 22:01:10, dcheng wrote:
> Isn't this copying Attribute? I think this should be const Attribute&

Done.

[Julien - ping for review, 2015-01-09 09:28:52]

Sorry I missed the description but we should be a bit more specific about what
you touch. You did similar changes that stated the directories touched and it's
a good idea to do it here too.

Also my personal preference is not to include BUG=None as it's irrelevant
information that distracts from the rest of the description. We are deeply
inconsistent on that so that's a nit.

[zhaoze.zhou, 2015-01-12 05:12:48]

On 2015/01/09 09:28:52, Julien Chaffraix - CET wrote:
> Sorry I missed the description but we should be a bit more specific about what
> you touch. You did similar changes that stated the directories touched and
it's
> a good idea to do it here too.
> 
> Also my personal preference is not to include BUG=None as it's irrelevant
> information that distracts from the rest of the description. We are deeply
> inconsistent on that so that's a nit.

thanks. :)

[Julien - ping for review, 2015-01-12 10:02:27]

The CQ bit was checked by jchaffraix@chromium.org

[Julien - ping for review, 2015-01-12 10:02:28]

Awesome, thanks for fixing the description.

[commit-bot: I haz the power, 2015-01-12 10:03:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/796913002/180001

[commit-bot: I haz the power, 2015-01-12 10:03:37]

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

[commit-bot: I haz the power, 2015-01-12 10:03:38]

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

Patch:       Source/core/dom/StyleEngine.cpp
Index: Source/core/dom/StyleEngine.cpp
diff --git a/Source/core/dom/StyleEngine.cpp b/Source/core/dom/StyleEngine.cpp
index
3cf7628102fe7cb30228dd8be63d995839a14378..af68e4470dbd128ae6e271ae7b92b0996e382688
100644
--- a/Source/core/dom/StyleEngine.cpp
+++ b/Source/core/dom/StyleEngine.cpp
@@ -347,8 +347,8 @@ bool
StyleEngine::shouldUpdateShadowTreeStyleSheetCollection(StyleResolverUpdate
 
 void StyleEngine::clearMediaQueryRuleSetOnTreeScopeStyleSheets(TreeScopeSet
treeScopes)
 {
-    for (TreeScopeSet::iterator it = treeScopes.begin(); it !=
treeScopes.end(); ++it) {
-        TreeScope& treeScope = **it;
+    for (TreeScope* scope : treeScopes) {
+        TreeScope& treeScope = *scope;
         ASSERT(treeScope != m_document);
         ShadowTreeStyleSheetCollection* collection =
static_cast<ShadowTreeStyleSheetCollection*>(styleSheetCollectionFor(treeScope));
         ASSERT(collection);
@@ -387,8 +387,7 @@ void
StyleEngine::updateActiveStyleSheets(StyleResolverUpdateMode updateMode)
         TreeScopeSet treeScopes = updateMode == FullStyleUpdate ?
m_activeTreeScopes : m_dirtyTreeScopes;
         HashSet<TreeScope*> treeScopesRemoved;
 
-        for (TreeScopeSet::iterator it = treeScopes.begin(); it !=
treeScopes.end(); ++it) {
-            TreeScope* treeScope = *it;
+        for (TreeScope* treeScope : treeScopes) {
             ASSERT(treeScope != m_document);
             ShadowTreeStyleSheetCollection* collection =
static_cast<ShadowTreeStyleSheetCollection*>(styleSheetCollectionFor(*treeScope));
             ASSERT(collection);

[zhaoze.zhou, 2015-01-14 04:42:01]

On 2015/01/12 10:03:38, I haz the power (commit-bot) wrote:
> Failed to apply patch for Source/core/dom/StyleEngine.cpp:
> While running patch -p1 --forward --force --no-backup-if-mismatch;
>   patching file Source/core/dom/StyleEngine.cpp
>   Hunk #1 FAILED at 347.
>   Hunk #2 FAILED at 387.
>   2 out of 2 hunks FAILED -- saving rejects to file
> Source/core/dom/StyleEngine.cpp.rej
> 
> Patch:       Source/core/dom/StyleEngine.cpp
> Index: Source/core/dom/StyleEngine.cpp
> diff --git a/Source/core/dom/StyleEngine.cpp b/Source/core/dom/StyleEngine.cpp
> index
>
3cf7628102fe7cb30228dd8be63d995839a14378..af68e4470dbd128ae6e271ae7b92b0996e382688
> 100644
> --- a/Source/core/dom/StyleEngine.cpp
> +++ b/Source/core/dom/StyleEngine.cpp
> @@ -347,8 +347,8 @@ bool
> StyleEngine::shouldUpdateShadowTreeStyleSheetCollection(StyleResolverUpdate
>  
>  void StyleEngine::clearMediaQueryRuleSetOnTreeScopeStyleSheets(TreeScopeSet
> treeScopes)
>  {
> -    for (TreeScopeSet::iterator it = treeScopes.begin(); it !=
> treeScopes.end(); ++it) {
> -        TreeScope& treeScope = **it;
> +    for (TreeScope* scope : treeScopes) {
> +        TreeScope& treeScope = *scope;
>          ASSERT(treeScope != m_document);
>          ShadowTreeStyleSheetCollection* collection =
>
static_cast<ShadowTreeStyleSheetCollection*>(styleSheetCollectionFor(treeScope));
>          ASSERT(collection);
> @@ -387,8 +387,7 @@ void
> StyleEngine::updateActiveStyleSheets(StyleResolverUpdateMode updateMode)
>          TreeScopeSet treeScopes = updateMode == FullStyleUpdate ?
> m_activeTreeScopes : m_dirtyTreeScopes;
>          HashSet<TreeScope*> treeScopesRemoved;
>  
> -        for (TreeScopeSet::iterator it = treeScopes.begin(); it !=
> treeScopes.end(); ++it) {
> -            TreeScope* treeScope = *it;
> +        for (TreeScope* treeScope : treeScopes) {
>              ASSERT(treeScope != m_document);
>              ShadowTreeStyleSheetCollection* collection =
>
static_cast<ShadowTreeStyleSheetCollection*>(styleSheetCollectionFor(*treeScope));
>              ASSERT(collection);

Rebased!

[zhaoze.zhou, 2015-01-14 04:42:15]

The CQ bit was checked by zhaoze.zhou@partner.samsung.com

[commit-bot: I haz the power, 2015-01-14 04:42:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/796913002/200001

[commit-bot: I haz the power, 2015-01-14 05:33:47]

Committed patchset #7 (id:200001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188359

[inferno, 2015-01-30 04:52:26]

On 2015/01/14 05:33:47, I haz the power (commit-bot) wrote:
> Committed patchset #7 (id:200001) as
> https://src.chromium.org/viewvc/blink?view=rev&revision=188359

This regressed and caused a bad security bug
[https://code.google.com/p/chromium/issues/detail?id=453279]. What is your email
that i can use in issue tracker to cc. Please contact me at inferno@chromium.org