|
|
Created:
4 years, 10 months ago by Alexander Semashko Modified:
4 years, 9 months ago CC:
chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, nasko+codewatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake NodeIterator more STL-friendly.
This allows it to be used in various algorithms.
BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/49ed0e3a1623c8524d4eae827da2ae37bfba06dc
Cr-Commit-Position: refs/heads/master@{#381807}
Patch Set 1 #
Messages
Total messages: 27 (8 generated)
Description was changed from ========== Make NodeIterator more STL-friendly. This allows it to be used in various algorithms. BUG= ========== to ========== Make NodeIterator more STL-friendly. This allows it to be used in various algorithms. BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
ahest@yandex-team.ru changed reviewers: + clamy@chromium.org
PTAL
clamy@chromium.org changed reviewers: + dcheng@chromium.org
@dcheng: could you PTAL since you introduced the iterator in the ifrst place?
Is there a use case where this would come in handy?
On 2016/03/14 20:28:18, dcheng wrote: > Is there a use case where this would come in handy? If applied to ConstNodes also (which is eligible for a similar change), this would allow to change FrameTree::IsLoading() to use std::any_of. Also there is a bunch of places where it is needed for downstream Yandex browser. Generally, this just brings some typedefs that std algorithms need, do you see any downsides in it?
I don't think we'd actually want to rewrite things like FrameTree::IsLoading(). Instead of looking like this: for (const FrameTreeNode* node : ConstNodes()) { if (node->IsLoading()) return true; } return false; We'd have this: const auto& nodes = ConstNodes(); return std::any_of(nodes.begin(), nodes.end(), [](const FrameTreeNode* node) { return node->IsLoading(); }); which is only one fewer line, and much more verbose. In principle though, this CL lgtm.
On 2016/03/14 23:11:18, dcheng wrote: > I don't think we'd actually want to rewrite things like FrameTree::IsLoading(). > Instead of looking like this: > for (const FrameTreeNode* node : ConstNodes()) { > if (node->IsLoading()) > return true; > } > return false; > > We'd have this: > const auto& nodes = ConstNodes(); > return std::any_of(nodes.begin(), nodes.end(), [](const FrameTreeNode* node) { > return node->IsLoading(); > }); > > which is only one fewer line, and much more verbose. > > In principle though, this CL lgtm. Yes, this case is too trivial to be a good example, but there are probably better ones to come soon. Ok, since you lgtmed the CL, would you mind if I update it with a change to ConstNodeIterator?
dcheng@chromium.org changed reviewers: + nick@chromium.org
On 2016/03/14 at 23:34:05, ahest wrote: > On 2016/03/14 23:11:18, dcheng wrote: > > I don't think we'd actually want to rewrite things like FrameTree::IsLoading(). > > Instead of looking like this: > > for (const FrameTreeNode* node : ConstNodes()) { > > if (node->IsLoading()) > > return true; > > } > > return false; > > > > We'd have this: > > const auto& nodes = ConstNodes(); > > return std::any_of(nodes.begin(), nodes.end(), [](const FrameTreeNode* node) { > > return node->IsLoading(); > > }); > > > > which is only one fewer line, and much more verbose. > > > > In principle though, this CL lgtm. > > Yes, this case is too trivial to be a good example, but there are probably better ones to come soon. Ok, since you lgtmed the CL, would you mind if I update it with a change to ConstNodeIterator? I think we're actually planning on removing ConstNodeIterator: it's mostly duplicated code (with the exception of a few small things), so if we don't have to maintain two copies, that'd be better.
The CQ bit was checked by ahest@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1728253003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1728253003/1
On 2016/03/14 23:35:51, dcheng wrote: > On 2016/03/14 at 23:34:05, ahest wrote: > > On 2016/03/14 23:11:18, dcheng wrote: > > > I don't think we'd actually want to rewrite things like > FrameTree::IsLoading(). > > > Instead of looking like this: > > > for (const FrameTreeNode* node : ConstNodes()) { > > > if (node->IsLoading()) > > > return true; > > > } > > > return false; > > > > > > We'd have this: > > > const auto& nodes = ConstNodes(); > > > return std::any_of(nodes.begin(), nodes.end(), [](const FrameTreeNode* > node) { > > > return node->IsLoading(); > > > }); > > > > > > which is only one fewer line, and much more verbose. > > > > > > In principle though, this CL lgtm. > > > > Yes, this case is too trivial to be a good example, but there are probably > better ones to come soon. Ok, since you lgtmed the CL, would you mind if I > update it with a change to ConstNodeIterator? > > I think we're actually planning on removing ConstNodeIterator: it's mostly > duplicated code (with the exception of a few small things), so if we don't have > to maintain two copies, that'd be better. Is it planned to reimplement it (e.g. by templatizing) or to just drop the possibility to iterate a const FrameTree?
On 2016/03/14 at 23:45:17, ahest wrote: > On 2016/03/14 23:35:51, dcheng wrote: > > On 2016/03/14 at 23:34:05, ahest wrote: > > > On 2016/03/14 23:11:18, dcheng wrote: > > > > I don't think we'd actually want to rewrite things like > > FrameTree::IsLoading(). > > > > Instead of looking like this: > > > > for (const FrameTreeNode* node : ConstNodes()) { > > > > if (node->IsLoading()) > > > > return true; > > > > } > > > > return false; > > > > > > > > We'd have this: > > > > const auto& nodes = ConstNodes(); > > > > return std::any_of(nodes.begin(), nodes.end(), [](const FrameTreeNode* > > node) { > > > > return node->IsLoading(); > > > > }); > > > > > > > > which is only one fewer line, and much more verbose. > > > > > > > > In principle though, this CL lgtm. > > > > > > Yes, this case is too trivial to be a good example, but there are probably > > better ones to come soon. Ok, since you lgtmed the CL, would you mind if I > > update it with a change to ConstNodeIterator? > > > > I think we're actually planning on removing ConstNodeIterator: it's mostly > > duplicated code (with the exception of a few small things), so if we don't have > > to maintain two copies, that'd be better. > > Is it planned to reimplement it (e.g. by templatizing) or to just drop the possibility to iterate a const FrameTree? Since the use of const method qualifiers in the content public API is discouraged, it's just going to be removed, as it's a lot of boilerplate just to support one const method.
On 2016/03/14 23:48:29, dcheng wrote: > On 2016/03/14 at 23:45:17, ahest wrote: > > On 2016/03/14 23:35:51, dcheng wrote: > > > On 2016/03/14 at 23:34:05, ahest wrote: > > > > On 2016/03/14 23:11:18, dcheng wrote: > > > > > I don't think we'd actually want to rewrite things like > > > FrameTree::IsLoading(). > > > > > Instead of looking like this: > > > > > for (const FrameTreeNode* node : ConstNodes()) { > > > > > if (node->IsLoading()) > > > > > return true; > > > > > } > > > > > return false; > > > > > > > > > > We'd have this: > > > > > const auto& nodes = ConstNodes(); > > > > > return std::any_of(nodes.begin(), nodes.end(), [](const FrameTreeNode* > > > node) { > > > > > return node->IsLoading(); > > > > > }); > > > > > > > > > > which is only one fewer line, and much more verbose. > > > > > > > > > > In principle though, this CL lgtm. > > > > > > > > Yes, this case is too trivial to be a good example, but there are probably > > > better ones to come soon. Ok, since you lgtmed the CL, would you mind if I > > > update it with a change to ConstNodeIterator? > > > > > > I think we're actually planning on removing ConstNodeIterator: it's mostly > > > duplicated code (with the exception of a few small things), so if we don't > have > > > to maintain two copies, that'd be better. > > > > Is it planned to reimplement it (e.g. by templatizing) or to just drop the > possibility to iterate a const FrameTree? > > Since the use of const method qualifiers in the content public API is > discouraged, it's just going to be removed, as it's a lot of boilerplate just to > support one const method. Ok, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Link to ConstNodeRange removal CL: where https://codereview.chromium.org/1802163002/
Nick, WDYT about this CL? It is still missing an owner's lgtm.
lgtm
The CQ bit was checked by ahest@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1728253003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1728253003/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Make NodeIterator more STL-friendly. This allows it to be used in various algorithms. BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Make NodeIterator more STL-friendly. This allows it to be used in various algorithms. BUG= CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/49ed0e3a1623c8524d4eae827da2ae37bfba06dc Cr-Commit-Position: refs/heads/master@{#381807} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/49ed0e3a1623c8524d4eae827da2ae37bfba06dc Cr-Commit-Position: refs/heads/master@{#381807} |