Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1500973002:
This patch adds NextSibling() to FrameTreeNode. (Closed)
|
Created:
5 years ago by paulmeyer Modified:
4 years, 7 months ago Reviewers:
Charlie Reis CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org, Fady Samuel, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThis patch adds NextSibling() to FrameTreeNode. This will be used (in addition to PreviousSibling()) by FindRequestManager as part of the multi-process find-in-page implementation. (https://codereview.chromium.org/1959183002/)
BUG=457440
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/322777fb2949adea7de55d0cc085b440548730b4
Cr-Commit-Position: refs/heads/master@{#393974}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rebased and addressed comments. #
Total comments: 2
Patch Set 3 : Rebased and addressed comment. #
Messages
Total messages: 35 (16 generated)
Description was changed from ========== This patch adds NextSibling() to FrameTreeNode. This will be used (in addition to PreviousSibling()) by FindRequestManager as part of the multi-process find-in-page implementation. BUG=457440 ========== to ========== This patch adds NextSibling() to FrameTreeNode. This will be used (in addition to PreviousSibling()) by FindRequestManager as part of the multi-process find-in-page implementation. BUG=457440 ==========
paulmeyer@chromium.org changed reviewers: + clamy@chromium.org
paulmeyer@chromium.org changed reviewers: - clamy@chromium.org
paulmeyer@chromium.org changed reviewers: + creis@chromium.org
The CQ bit was checked by paulmeyer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1500973002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1500973002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by paulmeyer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1500973002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1500973002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks. Is there a CL that will use this which is ready for review? We tend not to add new APIs until they're needed, but I'm ok with it if there's a caller lined up (and close to approved), in the interest of keeping the CLs smaller. https://codereview.chromium.org/1500973002/diff/1/content/browser/frame_host/... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1500973002/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree_node.cc:391: return offset >= parent_->child_count() ? nullptr Don't we need to check for less than 0 as well? Seems like we'd get rollover to MAX_INT for offset, which isn't good.
On 2015/12/09 22:29:15, Charlie Reis wrote: > Thanks. > > Is there a CL that will use this which is ready for review? We tend not to add > new APIs until they're needed, but I'm ok with it if there's a caller lined up > (and close to approved), in the interest of keeping the CLs smaller. > > https://codereview.chromium.org/1500973002/diff/1/content/browser/frame_host/... > File content/browser/frame_host/frame_tree_node.cc (right): > > https://codereview.chromium.org/1500973002/diff/1/content/browser/frame_host/... > content/browser/frame_host/frame_tree_node.cc:391: return offset >= > parent_->child_count() ? nullptr > Don't we need to check for less than 0 as well? Seems like we'd get rollover to > MAX_INT for offset, which isn't good. There is a followup CL that will use this, but it's not ready for review yet. I thought I'd put this one out there just because it was small and self-contained, but it can wait until the other CL is ready.
https://codereview.chromium.org/1500973002/diff/1/content/browser/frame_host/... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1500973002/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree_node.cc:391: return offset >= parent_->child_count() ? nullptr On 2015/12/09 22:29:15, Charlie Reis wrote: > Don't we need to check for less than 0 as well? Seems like we'd get rollover to > MAX_INT for offset, which isn't good. I wasn't checking for less than zero specifically because |offset| is unsigned. We could get rollover to MAX_INT, but why is that not good? I actually asked another engineer about this when I coded it because it did seem a bit weird to me, but he said it was fine and I couldn't think of a good reason why it wouldn't be fine either.
https://codereview.chromium.org/1500973002/diff/1/content/browser/frame_host/... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1500973002/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree_node.cc:391: return offset >= parent_->child_count() ? nullptr On 2015/12/09 22:38:43, Paul Meyer wrote: > On 2015/12/09 22:29:15, Charlie Reis wrote: > > Don't we need to check for less than 0 as well? Seems like we'd get rollover > to > > MAX_INT for offset, which isn't good. > > I wasn't checking for less than zero specifically because |offset| is unsigned. > We could get rollover to MAX_INT, but why is that not good? > > I actually asked another engineer about this when I coded it because it did seem > a bit weird to me, but he said it was fine and I couldn't think of a good reason > why it wouldn't be fine either. That would preclude any analyses which check for rollover, plus it's subtle. (It's also wrong in extreme cases, if you have enough children and a possibly large negative offset.) It's better to just be explicit about it and add the < 0 check.
Description was changed from ========== This patch adds NextSibling() to FrameTreeNode. This will be used (in addition to PreviousSibling()) by FindRequestManager as part of the multi-process find-in-page implementation. BUG=457440 ========== to ========== This patch adds NextSibling() to FrameTreeNode. This will be used (in addition to PreviousSibling()) by FindRequestManager as part of the multi-process find-in-page implementation. BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by paulmeyer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1500973002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1500973002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1500973002/diff/1/content/browser/frame_host/... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1500973002/diff/1/content/browser/frame_host/... content/browser/frame_host/frame_tree_node.cc:391: return offset >= parent_->child_count() ? nullptr On 2015/12/09 23:49:48, Charlie Reis wrote: > On 2015/12/09 22:38:43, Paul Meyer wrote: > > On 2015/12/09 22:29:15, Charlie Reis wrote: > > > Don't we need to check for less than 0 as well? Seems like we'd get > rollover > > to > > > MAX_INT for offset, which isn't good. > > > > I wasn't checking for less than zero specifically because |offset| is > unsigned. > > We could get rollover to MAX_INT, but why is that not good? > > > > I actually asked another engineer about this when I coded it because it did > seem > > a bit weird to me, but he said it was fine and I couldn't think of a good > reason > > why it wouldn't be fine either. > > That would preclude any analyses which check for rollover, plus it's subtle. > (It's also wrong in extreme cases, if you have enough children and a possibly > large negative offset.) > > It's better to just be explicit about it and add the < 0 check. Okay, done.
Can you reference the CL that will be using this in the CL description? I'm assuming it's almost ready to land? Also, sorry about the nit below, but I think we should fix it. Otherwise looks good! https://codereview.chromium.org/1500973002/diff/20001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1500973002/diff/20001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:498: size_t offset = i + relative_offset; You're still doing the rollover (or rather, rollunder). Simple cases like PreviousSibling of the first frame will hit it. I understand that it's well defined, but it just seems like we should avoid it. Nick suggests CheckedNumerics, but I think we can just rephrase by moving the negative relative_offset check before this line. Or we could move the whole bounds check before and do the i+relative_offset twice (skipping the declaration of offset), since additions are cheap.
Description was changed from ========== This patch adds NextSibling() to FrameTreeNode. This will be used (in addition to PreviousSibling()) by FindRequestManager as part of the multi-process find-in-page implementation. BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== This patch adds NextSibling() to FrameTreeNode. This will be used (in addition to PreviousSibling()) by FindRequestManager as part of the multi-process find-in-page implementation. (https://codereview.chromium.org/1959183002/) BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/1500973002/diff/20001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1500973002/diff/20001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:498: size_t offset = i + relative_offset; On 2016/05/13 23:18:38, Charlie Reis (slow to reply) wrote: > You're still doing the rollover (or rather, rollunder). Simple cases like > PreviousSibling of the first frame will hit it. I understand that it's well > defined, but it just seems like we should avoid it. > > Nick suggests CheckedNumerics, but I think we can just rephrase by moving the > negative relative_offset check before this line. Or we could move the whole > bounds check before and do the i+relative_offset twice (skipping the declaration > of offset), since additions are cheap. Done.
Thanks, LGTM.
The CQ bit was checked by paulmeyer@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1500973002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1500973002/60001
Message was sent while issue was closed.
Description was changed from ========== This patch adds NextSibling() to FrameTreeNode. This will be used (in addition to PreviousSibling()) by FindRequestManager as part of the multi-process find-in-page implementation. (https://codereview.chromium.org/1959183002/) BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== This patch adds NextSibling() to FrameTreeNode. This will be used (in addition to PreviousSibling()) by FindRequestManager as part of the multi-process find-in-page implementation. (https://codereview.chromium.org/1959183002/) BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== This patch adds NextSibling() to FrameTreeNode. This will be used (in addition to PreviousSibling()) by FindRequestManager as part of the multi-process find-in-page implementation. (https://codereview.chromium.org/1959183002/) BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== This patch adds NextSibling() to FrameTreeNode. This will be used (in addition to PreviousSibling()) by FindRequestManager as part of the multi-process find-in-page implementation. (https://codereview.chromium.org/1959183002/) BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/322777fb2949adea7de55d0cc085b440548730b4 Cr-Commit-Position: refs/heads/master@{#393974} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/322777fb2949adea7de55d0cc085b440548730b4 Cr-Commit-Position: refs/heads/master@{#393974} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
