Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2583343003:
[views] Changes iteration over |children_| to use range-based for loops (Closed)
Description[views] Changes iteration over |children_| to use range-based for loops
BUG=NONE
TEST=covered by existing tests
Committed: https://crrev.com/ebc65542d481179168a67b83a3151da26daaaa54
Cr-Commit-Position: refs/heads/master@{#439909}
Patch Set 1 #
Total comments: 10
Patch Set 2 : [views] Changes iteration over |children_| to use range-based for loops (nits) #Messages
Total messages: 25 (15 generated)
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
varkha@chromium.org changed reviewers: + sadrul@chromium.org
sadrul, sky@ thought it would be good to use range-based for loops (see [1]) so I've done it elsewhere in views::View. Can you please take a look? This should not be changing any behaviour. Thanks! [1] https://codereview.chromium.org/2561253002/diff/40001/ui/views/view.cc#newcod...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Change all of the 'auto child' to 'auto* child' to make it obvious that child is a pointer when reading the code. With that, lgtm https://codereview.chromium.org/2583343003/diff/1/ui/views/view.cc File ui/views/view.cc (right): https://codereview.chromium.org/2583343003/diff/1/ui/views/view.cc#newcode558 ui/views/view.cc:558: for (auto child : children_) { auto* child So it's obvious when reading the code that child is a pointer. https://codereview.chromium.org/2583343003/diff/1/ui/views/view.cc#newcode624 ui/views/view.cc:624: for (auto child : children_) { ditto https://codereview.chromium.org/2583343003/diff/1/ui/views/view.cc#newcode654 ui/views/view.cc:654: for (auto child : children_) same https://codereview.chromium.org/2583343003/diff/1/ui/views/view.cc#newcode1439 ui/views/view.cc:1439: for (auto child : children_) I think we should have {} here https://codereview.chromium.org/2583343003/diff/1/ui/views/view.cc#newcode2405 ui/views/view.cc:2405: child->PropagateDeviceScaleFactorChanged(device_scale_factor); I wonder why these last three are iterated in reverse order. (I am not suggesting we should change it, just wondering).
Thanks sadrul@! +ben@ for reasoning of the reverse iteration order in: - PropagateThemeChanged - PropagateLocaleChanged - PropagateDeviceScaleFactorChanged https://codereview.chromium.org/2583343003/diff/1/ui/views/view.cc File ui/views/view.cc (right): https://codereview.chromium.org/2583343003/diff/1/ui/views/view.cc#newcode558 ui/views/view.cc:558: for (auto child : children_) { On 2016/12/20 17:35:25, sadrul wrote: > auto* child > > So it's obvious when reading the code that child is a pointer. Done. https://codereview.chromium.org/2583343003/diff/1/ui/views/view.cc#newcode624 ui/views/view.cc:624: for (auto child : children_) { On 2016/12/20 17:35:25, sadrul wrote: > ditto Done. https://codereview.chromium.org/2583343003/diff/1/ui/views/view.cc#newcode654 ui/views/view.cc:654: for (auto child : children_) On 2016/12/20 17:35:25, sadrul wrote: > same Done. https://codereview.chromium.org/2583343003/diff/1/ui/views/view.cc#newcode1439 ui/views/view.cc:1439: for (auto child : children_) On 2016/12/20 17:35:25, sadrul wrote: > I think we should have {} here Done. https://codereview.chromium.org/2583343003/diff/1/ui/views/view.cc#newcode2405 ui/views/view.cc:2405: child->PropagateDeviceScaleFactorChanged(device_scale_factor); On 2016/12/20 17:35:25, sadrul wrote: > I wonder why these last three are iterated in reverse order. (I am not > suggesting we should change it, just wondering). Not sure. I guess the desire was to go in visibility order so it is possible that there is something subtle about the propagation order that favors children that are above others to receive the notifications first. This goes all the way to https://codereview.chromium.org/27317 so maybe ben@ would know.
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ben@chromium.org changed reviewers: + ben@chromium.org
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2583343003/#ps20001 (title: "[views] Changes iteration over |children_| to use range-based for loops (nits)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1482270891097080,
"parent_rev": "fbdbe796ca884843baf674058809e96c8edf11a0", "commit_rev":
"bd27aa4cbfa4795986a5045b976c6f49d63d8a73"}
Message was sent while issue was closed.
Description was changed from ========== [views] Changes iteration over |children_| to use range-based for loops BUG=NONE TEST=covered by existing tests ========== to ========== [views] Changes iteration over |children_| to use range-based for loops BUG=NONE TEST=covered by existing tests Review-Url: https://codereview.chromium.org/2583343003 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [views] Changes iteration over |children_| to use range-based for loops BUG=NONE TEST=covered by existing tests Review-Url: https://codereview.chromium.org/2583343003 ========== to ========== [views] Changes iteration over |children_| to use range-based for loops BUG=NONE TEST=covered by existing tests Committed: https://crrev.com/ebc65542d481179168a67b83a3151da26daaaa54 Cr-Commit-Position: refs/heads/master@{#439909} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ebc65542d481179168a67b83a3151da26daaaa54 Cr-Commit-Position: refs/heads/master@{#439909}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2594533003/ by shimazu@chromium.org. The reason for reverting is: The following three tests on sync_integration_tests failed by this CL: TwoClientThemesSyncTest.CustomThenSyncNative_E2ETest TwoClientThemesSyncTest.CycleOptions_E2ETest SingleClientThemesSyncTest.NativeTheme This patch changed to use iterator instead of [] operator, but |children_| seems to be changed somewhere during the iteration..
Message was sent while issue was closed.
On 2016/12/21 04:01:54, shimazu wrote: > A revert of this CL (patchset #2 id:20001) has been created in > https://codereview.chromium.org/2594533003/ by mailto:shimazu@chromium.org. > > The reason for reverting is: The following three tests on sync_integration_tests > failed by this CL: > TwoClientThemesSyncTest.CustomThenSyncNative_E2ETest > TwoClientThemesSyncTest.CycleOptions_E2ETest > SingleClientThemesSyncTest.NativeTheme > > This patch changed to use iterator instead of [] operator, but |children_| seems > to be changed somewhere during the iteration.. Yes, shimazu@ I believe your analysis is correct. We mutate |children_| while inside View::PropagateNativeThemeChanged() which invalidates the iterator. Still it seems we are just being lucky with the current code because the sequence I am seeing is - start iteration in PropagateNativeThemeChanged (2 children) - iterate over child[0] - advance the index (to 1) - enter recursion into child[1] - remove child[0] from |children_| <== this is a problem worth investigating - add another child at index 0 <== we are lucky to not invalidate the count - return from recursion into child[1] - advance index, check against the cached count (2) and return I spoke offline with sadrul@ and I will try and investigate where and why we are mutating |children_| while in the context of the iteration. I doubt the code was written to expect this.
Message was sent while issue was closed.
Update - this is succeeded with Change to avoid the troublesome scenario - https://codereview.chromium.org/2597733002 Re-landing this CL - https://codereview.chromium.org/2591323002/ |
