New issue
Advanced search Search tips

Issue 655950 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 656926



Sign in to add a comment

Replace for loops with |arraysize| with for each loops where appropriate in autofill component

Project Member Reported by jdoerrie@chromium.org, Oct 14 2016

Issue description

Currently, there are many places in the codebase where a for loop involving an explicit loop variable and the |arraysize| macro are used to iterate over a given array. 

With the advent of C++11 and range based for loops, this can now be done using language features only. This has the advantage that the code is more concise, and arguably more readable and robust. Also it removes the need to include the "base/macro.h" header for this particular purpose. However, not all for loops involving |arraysize| can be replaced, for example this is not possible when the loop variable is used for logging or to index another array.

This is a change that is reasonable across the whole Chromium codebase, but for now we limit it to the autofill component.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Oct 17 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/54e466c9fd2ff04136f8f305615910e8f9085642

commit 54e466c9fd2ff04136f8f305615910e8f9085642
Author: jdoerrie <jdoerrie@chromium.org>
Date: Mon Oct 17 10:46:07 2016

Replace for loops with |arraysize| with for each loops

This change replaces for-loops involving a loop variable and the
|arraysize| macro with a C++11 for-each-loop where appropriate.

A few usages of |arraysize| in a for-loop remain, in these the loop
variable is used as well. This can be simply logging the iteration
number or indexing another array of the same size.

BUG= 655950 

Review-Url: https://codereview.chromium.org/2417783004
Cr-Commit-Position: refs/heads/master@{#425651}

[modify] https://crrev.com/54e466c9fd2ff04136f8f305615910e8f9085642/components/autofill/content/renderer/form_cache.cc
[modify] https://crrev.com/54e466c9fd2ff04136f8f305615910e8f9085642/components/autofill/core/browser/address_i18n_unittest.cc
[modify] https://crrev.com/54e466c9fd2ff04136f8f305615910e8f9085642/components/autofill/core/browser/address_unittest.cc
[modify] https://crrev.com/54e466c9fd2ff04136f8f305615910e8f9085642/components/autofill/core/browser/autofill_data_util.cc
[modify] https://crrev.com/54e466c9fd2ff04136f8f305615910e8f9085642/components/autofill/core/browser/autofill_ie_toolbar_import_win.cc
[modify] https://crrev.com/54e466c9fd2ff04136f8f305615910e8f9085642/components/autofill/core/browser/autofill_manager_unittest.cc
[modify] https://crrev.com/54e466c9fd2ff04136f8f305615910e8f9085642/components/autofill/core/browser/autofill_merge_unittest.cc
[modify] https://crrev.com/54e466c9fd2ff04136f8f305615910e8f9085642/components/autofill/core/browser/autofill_profile.cc
[modify] https://crrev.com/54e466c9fd2ff04136f8f305615910e8f9085642/components/autofill/core/browser/contact_info_unittest.cc
[modify] https://crrev.com/54e466c9fd2ff04136f8f305615910e8f9085642/components/autofill/core/browser/credit_card.cc
[modify] https://crrev.com/54e466c9fd2ff04136f8f305615910e8f9085642/components/autofill/core/browser/credit_card_unittest.cc
[modify] https://crrev.com/54e466c9fd2ff04136f8f305615910e8f9085642/components/autofill/core/browser/phone_field_unittest.cc
[modify] https://crrev.com/54e466c9fd2ff04136f8f305615910e8f9085642/components/autofill/core/browser/phone_number_i18n_unittest.cc
[modify] https://crrev.com/54e466c9fd2ff04136f8f305615910e8f9085642/components/autofill/core/browser/state_names.cc
[modify] https://crrev.com/54e466c9fd2ff04136f8f305615910e8f9085642/components/autofill/core/browser/ui/card_unmask_prompt_controller_impl_unittest.cc
[modify] https://crrev.com/54e466c9fd2ff04136f8f305615910e8f9085642/components/autofill/core/browser/validation_unittest.cc
[modify] https://crrev.com/54e466c9fd2ff04136f8f305615910e8f9085642/components/autofill/core/browser/webdata/autofill_table_unittest.cc
[modify] https://crrev.com/54e466c9fd2ff04136f8f305615910e8f9085642/components/autofill/core/common/autofill_regexes_unittest.cc
[modify] https://crrev.com/54e466c9fd2ff04136f8f305615910e8f9085642/components/autofill/core/common/autofill_util_unittest.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Oct 17 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e00a337f9c62d1d32c315ecacc63cec380a36e27

commit e00a337f9c62d1d32c315ecacc63cec380a36e27
Author: henrika <henrika@chromium.org>
Date: Mon Oct 17 12:16:06 2016

Revert of Replace for loops with |arraysize| with for each loops (patchset #2 id:20001 of https://codereview.chromium.org/2417783004/ )

Reason for revert:
Speculative revert from sheriff. Suspect that the CL breaks components_unittests on Mac.

https://uberchromegw.corp.google.com/i/chromium.mac/builders/Mac10.9%20Tests%20%28dbg%29/builds/31721/steps/components_unittests%20on%20Mac-10.9

Not sure. Will revert the revert if it does not help.

Original issue's description:
> Replace for loops with |arraysize| with for each loops
>
> This change replaces for-loops involving a loop variable and the
> |arraysize| macro with a C++11 for-each-loop where appropriate.
>
> A few usages of |arraysize| in a for-loop remain, in these the loop
> variable is used as well. This can be simply logging the iteration
> number or indexing another array of the same size.
>
> BUG= 655950 
>
> Committed: https://crrev.com/54e466c9fd2ff04136f8f305615910e8f9085642
> Cr-Commit-Position: refs/heads/master@{#425651}

TBR=vabr@chromium.org,jdoerrie@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 655950 

Review-Url: https://codereview.chromium.org/2424793002
Cr-Commit-Position: refs/heads/master@{#425660}

[modify] https://crrev.com/e00a337f9c62d1d32c315ecacc63cec380a36e27/components/autofill/content/renderer/form_cache.cc
[modify] https://crrev.com/e00a337f9c62d1d32c315ecacc63cec380a36e27/components/autofill/core/browser/address_i18n_unittest.cc
[modify] https://crrev.com/e00a337f9c62d1d32c315ecacc63cec380a36e27/components/autofill/core/browser/address_unittest.cc
[modify] https://crrev.com/e00a337f9c62d1d32c315ecacc63cec380a36e27/components/autofill/core/browser/autofill_data_util.cc
[modify] https://crrev.com/e00a337f9c62d1d32c315ecacc63cec380a36e27/components/autofill/core/browser/autofill_ie_toolbar_import_win.cc
[modify] https://crrev.com/e00a337f9c62d1d32c315ecacc63cec380a36e27/components/autofill/core/browser/autofill_manager_unittest.cc
[modify] https://crrev.com/e00a337f9c62d1d32c315ecacc63cec380a36e27/components/autofill/core/browser/autofill_merge_unittest.cc
[modify] https://crrev.com/e00a337f9c62d1d32c315ecacc63cec380a36e27/components/autofill/core/browser/autofill_profile.cc
[modify] https://crrev.com/e00a337f9c62d1d32c315ecacc63cec380a36e27/components/autofill/core/browser/contact_info_unittest.cc
[modify] https://crrev.com/e00a337f9c62d1d32c315ecacc63cec380a36e27/components/autofill/core/browser/credit_card.cc
[modify] https://crrev.com/e00a337f9c62d1d32c315ecacc63cec380a36e27/components/autofill/core/browser/credit_card_unittest.cc
[modify] https://crrev.com/e00a337f9c62d1d32c315ecacc63cec380a36e27/components/autofill/core/browser/phone_field_unittest.cc
[modify] https://crrev.com/e00a337f9c62d1d32c315ecacc63cec380a36e27/components/autofill/core/browser/phone_number_i18n_unittest.cc
[modify] https://crrev.com/e00a337f9c62d1d32c315ecacc63cec380a36e27/components/autofill/core/browser/state_names.cc
[modify] https://crrev.com/e00a337f9c62d1d32c315ecacc63cec380a36e27/components/autofill/core/browser/ui/card_unmask_prompt_controller_impl_unittest.cc
[modify] https://crrev.com/e00a337f9c62d1d32c315ecacc63cec380a36e27/components/autofill/core/browser/validation_unittest.cc
[modify] https://crrev.com/e00a337f9c62d1d32c315ecacc63cec380a36e27/components/autofill/core/browser/webdata/autofill_table_unittest.cc
[modify] https://crrev.com/e00a337f9c62d1d32c315ecacc63cec380a36e27/components/autofill/core/common/autofill_regexes_unittest.cc
[modify] https://crrev.com/e00a337f9c62d1d32c315ecacc63cec380a36e27/components/autofill/core/common/autofill_util_unittest.cc

Even after the revert I can still reproduce the issue on OSX 10.10 dbg. We do not have a builder for this configuration, so I don't know since when this issue occurs. I attached a more detailed stack trace I was able to get with lldb.


stacktrace.txt
4.5 KB View Download

Comment 5 by vabr@chromium.org, Oct 18 2016

Blockedon: 656926
Project Member

Comment 6 by bugdroid1@chromium.org, Nov 1 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9bd6ef5d4c1f52c0cbc5da2075f19c10577e337b

commit 9bd6ef5d4c1f52c0cbc5da2075f19c10577e337b
Author: Yoonjae.Cho92 <Yoonjae.Cho92@gmail.com>
Date: Tue Nov 01 18:11:07 2016

Replace for each loops
BUG= 655950 

Review-Url: https://codereview.chromium.org/2459243002
Cr-Commit-Position: refs/heads/master@{#429052}

[modify] https://crrev.com/9bd6ef5d4c1f52c0cbc5da2075f19c10577e337b/AUTHORS
[modify] https://crrev.com/9bd6ef5d4c1f52c0cbc5da2075f19c10577e337b/net/http/http_util.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Nov 3 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/188854d02b406012ec48a550ffcdd273c8b14d53

commit 188854d02b406012ec48a550ffcdd273c8b14d53
Author: jdoerrie <jdoerrie@chromium.org>
Date: Thu Nov 03 14:41:57 2016

Reland of place for loops with |arraysize| with for each loops (patchset #1 id:1 of https://codereview.chromium.org/2424793002/ )

Reason for revert:
Relanding, because the flaky unittest was fixed:  http://crbug.com/656926 

Original issue's description:
> Revert of Replace for loops with |arraysize| with for each loops (patchset #2 id:20001 of https://codereview.chromium.org/2417783004/ )
>
> Reason for revert:
> Speculative revert from sheriff. Suspect that the CL breaks components_unittests on Mac.
>
> https://uberchromegw.corp.google.com/i/chromium.mac/builders/Mac10.9%20Tests%20%28dbg%29/builds/31721/steps/components_unittests%20on%20Mac-10.9
>
> Not sure. Will revert the revert if it does not help.
>
> Original issue's description:
> > Replace for loops with |arraysize| with for each loops
> >
> > This change replaces for-loops involving a loop variable and the
> > |arraysize| macro with a C++11 for-each-loop where appropriate.
> >
> > A few usages of |arraysize| in a for-loop remain, in these the loop
> > variable is used as well. This can be simply logging the iteration
> > number or indexing another array of the same size.
> >
> > BUG= 655950 
> >
> > Committed: https://crrev.com/54e466c9fd2ff04136f8f305615910e8f9085642
> > Cr-Commit-Position: refs/heads/master@{#425651}
>
> TBR=vabr@chromium.org,jdoerrie@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG= 655950 
>
> Committed: https://crrev.com/e00a337f9c62d1d32c315ecacc63cec380a36e27
> Cr-Commit-Position: refs/heads/master@{#425660}

TBR=vabr@chromium.org,henrika@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 655950 

Review-Url: https://codereview.chromium.org/2478463002
Cr-Commit-Position: refs/heads/master@{#429578}

[modify] https://crrev.com/188854d02b406012ec48a550ffcdd273c8b14d53/components/autofill/content/renderer/form_cache.cc
[modify] https://crrev.com/188854d02b406012ec48a550ffcdd273c8b14d53/components/autofill/core/browser/address_i18n_unittest.cc
[modify] https://crrev.com/188854d02b406012ec48a550ffcdd273c8b14d53/components/autofill/core/browser/address_unittest.cc
[modify] https://crrev.com/188854d02b406012ec48a550ffcdd273c8b14d53/components/autofill/core/browser/autofill_data_util.cc
[modify] https://crrev.com/188854d02b406012ec48a550ffcdd273c8b14d53/components/autofill/core/browser/autofill_ie_toolbar_import_win.cc
[modify] https://crrev.com/188854d02b406012ec48a550ffcdd273c8b14d53/components/autofill/core/browser/autofill_manager_unittest.cc
[modify] https://crrev.com/188854d02b406012ec48a550ffcdd273c8b14d53/components/autofill/core/browser/autofill_merge_unittest.cc
[modify] https://crrev.com/188854d02b406012ec48a550ffcdd273c8b14d53/components/autofill/core/browser/autofill_profile.cc
[modify] https://crrev.com/188854d02b406012ec48a550ffcdd273c8b14d53/components/autofill/core/browser/contact_info_unittest.cc
[modify] https://crrev.com/188854d02b406012ec48a550ffcdd273c8b14d53/components/autofill/core/browser/credit_card.cc
[modify] https://crrev.com/188854d02b406012ec48a550ffcdd273c8b14d53/components/autofill/core/browser/credit_card_unittest.cc
[modify] https://crrev.com/188854d02b406012ec48a550ffcdd273c8b14d53/components/autofill/core/browser/phone_field_unittest.cc
[modify] https://crrev.com/188854d02b406012ec48a550ffcdd273c8b14d53/components/autofill/core/browser/phone_number_i18n_unittest.cc
[modify] https://crrev.com/188854d02b406012ec48a550ffcdd273c8b14d53/components/autofill/core/browser/state_names.cc
[modify] https://crrev.com/188854d02b406012ec48a550ffcdd273c8b14d53/components/autofill/core/browser/ui/card_unmask_prompt_controller_impl_unittest.cc
[modify] https://crrev.com/188854d02b406012ec48a550ffcdd273c8b14d53/components/autofill/core/browser/validation_unittest.cc
[modify] https://crrev.com/188854d02b406012ec48a550ffcdd273c8b14d53/components/autofill/core/browser/webdata/autofill_table_unittest.cc
[modify] https://crrev.com/188854d02b406012ec48a550ffcdd273c8b14d53/components/autofill/core/common/autofill_regexes_unittest.cc
[modify] https://crrev.com/188854d02b406012ec48a550ffcdd273c8b14d53/components/autofill/core/common/autofill_util_unittest.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Nov 3 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6dea603ddfb631a7645e7ca95ea47e5a1291db29

commit 6dea603ddfb631a7645e7ca95ea47e5a1291db29
Author: Yoonjae.Cho92 <Yoonjae.Cho92@gmail.com>
Date: Thu Nov 03 17:22:52 2016

Replace for each loop

BUG= 655950 

Review-Url: https://codereview.chromium.org/2470273003
Cr-Commit-Position: refs/heads/master@{#429627}

[modify] https://crrev.com/6dea603ddfb631a7645e7ca95ea47e5a1291db29/net/log/net_log_util.cc

Status: Fixed (was: Started)
Summary: Replace for loops with |arraysize| with for each loops where appropriate in autofill component (was: Replace for loops with |arraysize| with for each loops where appropriate)

Comment 10 by ma...@chromium.org, Nov 10 2016

Thanks very much for the contributions to Autofill!
Project Member

Comment 11 by bugdroid1@chromium.org, Nov 10 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/39b139c1ed407353fd64683bdf69d6762ed09175

commit 39b139c1ed407353fd64683bdf69d6762ed09175
Author: sanghee.lee1992 <sanghee.lee1992@gmail.com>
Date: Thu Nov 10 20:08:46 2016

Switch to C++ 11 for-loop in l10n_util.cc

This change is to replace for-loop involving arraysize() with
C+11 for-loop.

file : ui/base/l10n/l10n_util.cc

BUG= 655950 

Review-Url: https://codereview.chromium.org/2486053002
Cr-Commit-Position: refs/heads/master@{#431333}

[modify] https://crrev.com/39b139c1ed407353fd64683bdf69d6762ed09175/ui/base/l10n/l10n_util.cc

Sign in to add a comment