Fix find-in-page re-scope across frame boundaries.

Fix find-in-page re-scope across frame boundaries.

There is a bug that can cause a frame to be re-scoped (all matches searched again) when issuing a find next request across a frame boundary, such as from the last match in a frame, which might need to wrap back around to the first match in the same frame. If there are not many matches in that frame, this is not really noticeable except for the ticks in the tickbar sometimes disappearing for a single frame. However, on a page with a huge number of results, this can dramatically slow down the find request and you can clearly see all of the ticks in the tickbar regenerate.

This patch adjusts the conditions under which a frame is determined to need scoping in WebLocalFrameImpl::requestFind(), in order to not re-scope across frame boundaries but continue to scope in all the cases where it IS needed.

BUG=618937, 627799
Committed: https://crrev.com/b6f12c557fb9b1468ba6f6da5f55cc7cefc045b0
Cr-Commit-Position: refs/heads/master@{#412025}

[paulmeyer, 2016-07-27 18:44:12]

Description was changed from

==========
Fix find-in-page re-scope across frame boundaries.

There is a bug that can cause a frame to be re-scoped (all matches searched
again) when issuing a find next request across a frame boundary, such as from
the last match in the frame, which might need to wrap back around to the firs
match. If there are not many matches in that frame, this is not really
noticeable excpet for the ticks in the tickbar sometimes disappearing for a
single frame. However, on a page with a huge number of results, this can
seriously slow down the find request and you can see all of the ticks in the
tickbar regenerate.

This patch removes the check for |is_active| in RenderFrameImpl::OnFind(), since
that check is meaningless now that FindRequestManager handles this case in the
browser. This fixes the bug described here, but introduces a new (but rarely
occuring) bug that can cause find-in-page to fail if a huge number of requests
are sent at once, and the requests cross a frame boundary from a frame that has
been searched completely to a frame that is still scoping. The fix for this is
to check whether the frame is still scoping during OnFind().

BUG=618937,627799
==========

to

==========
Fix find-in-page re-scope across frame boundaries.

There is a bug that can cause a frame to be re-scoped (all matches searched
again) when issuing a find next request across a frame boundary, such as from
the last match in a frame, which might need to wrap back around to the first
match in the same frame. If there are not many matches in that frame, this is
not really noticeable except for the ticks in the tickbar sometimes disappearing
for a single frame. However, on a page with a huge number of results, this can
dramatically slow down the find request and you can clearly see all of the ticks
in the tickbar regenerate.

This patch removes the check for |is_active| in RenderFrameImpl::OnFind(), since
that check is meaningless now that FindRequestManager handles this case in the
browser. This fixes the bug described here, but introduces a new (but rarely
occurring) bug that can cause find-in-page to fail if a huge number of requests
are sent at once, and the requests cross a frame boundary from a frame that has
been searched completely to a frame that is still scoping. The fix for this is
to check whether the frame is still scoping during OnFind().

BUG=618937,627799
==========

[paulmeyer, 2016-07-27 18:49:21]

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

[dcheng, 2016-07-28 02:11:45]

https://codereview.chromium.org/2186113002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2186113002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:5245: !frame_->scopingInProgress()) {
Is this an urgent fix? I think we should explore moving these bits back into
Blink rather than exposing more functions in the public API. WDYT?

[paulmeyer, 2016-08-03 14:27:45]

On 2016/07/28 02:11:45, dcheng (OOO Aug 1 - Aug 11) wrote:
>
https://codereview.chromium.org/2186113002/diff/1/content/renderer/render_fra...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/2186113002/diff/1/content/renderer/render_fra...
> content/renderer/render_frame_impl.cc:5245: !frame_->scopingInProgress()) {
> Is this an urgent fix? I think we should explore moving these bits back into
> Blink rather than exposing more functions in the public API. WDYT?

That sounds reasonable. I have another patch almost ready that will both remove
the need for exposing this new API, as well as remove three existing
find-related Blink APIs.

[paulmeyer, 2016-08-08 12:44:32]

The CQ bit was checked by paulmeyer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-08 12:44:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[paulmeyer, 2016-08-08 13:33:13]

Description was changed from

==========
Fix find-in-page re-scope across frame boundaries.

There is a bug that can cause a frame to be re-scoped (all matches searched
again) when issuing a find next request across a frame boundary, such as from
the last match in a frame, which might need to wrap back around to the first
match in the same frame. If there are not many matches in that frame, this is
not really noticeable except for the ticks in the tickbar sometimes disappearing
for a single frame. However, on a page with a huge number of results, this can
dramatically slow down the find request and you can clearly see all of the ticks
in the tickbar regenerate.

This patch removes the check for |is_active| in RenderFrameImpl::OnFind(), since
that check is meaningless now that FindRequestManager handles this case in the
browser. This fixes the bug described here, but introduces a new (but rarely
occurring) bug that can cause find-in-page to fail if a huge number of requests
are sent at once, and the requests cross a frame boundary from a frame that has
been searched completely to a frame that is still scoping. The fix for this is
to check whether the frame is still scoping during OnFind().

BUG=618937,627799
==========

to

==========
Fix find-in-page re-scope across frame boundaries.

There is a bug that can cause a frame to be re-scoped (all matches searched
again) when issuing a find next request across a frame boundary, such as from
the last match in a frame, which might need to wrap back around to the first
match in the same frame. If there are not many matches in that frame, this is
not really noticeable except for the ticks in the tickbar sometimes disappearing
for a single frame. However, on a page with a huge number of results, this can
dramatically slow down the find request and you can clearly see all of the ticks
in the tickbar regenerate.

This patch removes the check for |isActive| in WebLocalFrameImpl::requestFind(),
since that check is not needed now that FindRequestManager handles this case in
the browser. This removal fixes the bug described here, but introduces a new
(but rarely occurring) bug that can cause find-in-page to fail if a huge number
of requests are sent at once, and the requests cross a frame boundary from a
frame that has been searched completely to a frame that is still scoping. The
fix for this is to check whether the frame is still scoping requestFind().

BUG=618937,627799
==========

[paulmeyer, 2016-08-08 13:34:04]

paulmeyer@chromium.org changed reviewers:
- dcheng@chromium.org

[paulmeyer, 2016-08-08 13:52:37]

paulmeyer@chromium.org changed reviewers:
+ esprehn@chromium.org

[paulmeyer, 2016-08-08 13:52:38]

-dcheng@ (vacation)

+esprehn@

[commit-bot: I haz the power, 2016-08-08 14:09:29]

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

[commit-bot: I haz the power, 2016-08-08 14:09:30]

Dry run: This issue passed the CQ dry run.

[esprehn, 2016-08-10 04:14:37]

Description was changed from

==========
Fix find-in-page re-scope across frame boundaries.

There is a bug that can cause a frame to be re-scoped (all matches searched
again) when issuing a find next request across a frame boundary, such as from
the last match in a frame, which might need to wrap back around to the first
match in the same frame. If there are not many matches in that frame, this is
not really noticeable except for the ticks in the tickbar sometimes disappearing
for a single frame. However, on a page with a huge number of results, this can
dramatically slow down the find request and you can clearly see all of the ticks
in the tickbar regenerate.

This patch removes the check for |isActive| in WebLocalFrameImpl::requestFind(),
since that check is not needed now that FindRequestManager handles this case in
the browser. This removal fixes the bug described here, but introduces a new
(but rarely occurring) bug that can cause find-in-page to fail if a huge number
of requests are sent at once, and the requests cross a frame boundary from a
frame that has been searched completely to a frame that is still scoping. The
fix for this is to check whether the frame is still scoping requestFind().

BUG=618937,627799
==========

to

==========
Fix find-in-page re-scope across frame boundaries.

There is a bug that can cause a frame to be re-scoped (all matches searched
again) 
when issuing a find next request across a frame boundary, such as from the last 
match in a frame, which might need to wrap back around to the first match in the

same frame. If there are not many matches in that frame, this is not really 
noticeable except for the ticks in the tickbar sometimes disappearing for a
single 
frame. However, on a page with a huge number of results, this can dramatically 
slow down the find request and you can clearly see all of the ticks in the
tickbar 
regenerate.

This patch removes the check for |isActive| in WebLocalFrameImpl::requestFind(),

since that check is not needed now that FindRequestManager handles this case in 
the browser. This removal fixes the bug described here, but introduces a new
(but 
rarely occurring) bug that can cause find-in-page to fail if a huge number of 
requests are sent at once, and the requests cross a frame boundary from a frame 
that has been searched completely to a frame that is still scoping. The fix for 
this is to check whether the frame is still scoping requestFind().

BUG=618937,627799
==========

[esprehn, 2016-08-10 04:16:45]

What is "scoping" ? I worry a lot we're doubling down on a lot of complexity
around isActive and friends and that something much simpler for find in page in
general is the real solution.

[paulmeyer, 2016-08-10 14:22:19]

On 2016/08/10 04:16:45, esprehn wrote:
> What is "scoping" ? I worry a lot we're doubling down on a lot of complexity
> around isActive and friends and that something much simpler for find in page
in
> general is the real solution.

"scoping" just means searching the frame for matches. I'm personally not sure
why it's called that, but the term was around before I started working on this.

As for reducing complexity, I'm always for that! There are a few non-FIP things
on my plate that I need to get to shortly, but I'd love to come back and improve
FIP some more in the future.

For now though, were there any specific concerns you have about this fix? Bug
627799 is fairly pressing at the moment and I'd like to get this fix in as soon
as possible.

[esprehn, 2016-08-11 09:38:29]

What about tests? I guess lgtm, but without some tests we're just going to keep
breaking find in page.

[esprehn, 2016-08-11 09:40:01]

https://codereview.chromium.org/2186113002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/2186113002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1996: if (options.findNext
&& currentSelection.isNull() && !textFinder()->scopingInProgress()) {
Actually this means the activeNow bool in this function is just ignored, why is
that correct? What's the purpose of activeNow if we don't use it?

[paulmeyer, 2016-08-12 14:22:21]

Description was changed from

==========
Fix find-in-page re-scope across frame boundaries.

There is a bug that can cause a frame to be re-scoped (all matches searched
again) 
when issuing a find next request across a frame boundary, such as from the last 
match in a frame, which might need to wrap back around to the first match in the

same frame. If there are not many matches in that frame, this is not really 
noticeable except for the ticks in the tickbar sometimes disappearing for a
single 
frame. However, on a page with a huge number of results, this can dramatically 
slow down the find request and you can clearly see all of the ticks in the
tickbar 
regenerate.

This patch removes the check for |isActive| in WebLocalFrameImpl::requestFind(),

since that check is not needed now that FindRequestManager handles this case in 
the browser. This removal fixes the bug described here, but introduces a new
(but 
rarely occurring) bug that can cause find-in-page to fail if a huge number of 
requests are sent at once, and the requests cross a frame boundary from a frame 
that has been searched completely to a frame that is still scoping. The fix for 
this is to check whether the frame is still scoping requestFind().

BUG=618937,627799
==========

to

==========
Fix find-in-page re-scope across frame boundaries.

There is a bug that can cause a frame to be re-scoped (all matches searched
again) 
when issuing a find next request across a frame boundary, such as from the last 
match in a frame, which might need to wrap back around to the first match in the

same frame. If there are not many matches in that frame, this is not really 
noticeable except for the ticks in the tickbar sometimes disappearing for a
single 
frame. However, on a page with a huge number of results, this can dramatically 
slow down the find request and you can clearly see all of the ticks in the
tickbar 
regenerate.

This patch adjusts the conditions under which a frame is determined to need
scoping in WebLocalFrameImpl::requestFind(), in order to not re-scope across
frame boundaries but continue to scope in all the cases where it IS needed.

BUG=618937,627799
==========

[paulmeyer, 2016-08-12 17:14:58]

Patchset #3 (id:40001) has been deleted

[paulmeyer, 2016-08-12 17:29:48]

Description was changed from

==========
Fix find-in-page re-scope across frame boundaries.

There is a bug that can cause a frame to be re-scoped (all matches searched
again) 
when issuing a find next request across a frame boundary, such as from the last 
match in a frame, which might need to wrap back around to the first match in the

same frame. If there are not many matches in that frame, this is not really 
noticeable except for the ticks in the tickbar sometimes disappearing for a
single 
frame. However, on a page with a huge number of results, this can dramatically 
slow down the find request and you can clearly see all of the ticks in the
tickbar 
regenerate.

This patch adjusts the conditions under which a frame is determined to need
scoping in WebLocalFrameImpl::requestFind(), in order to not re-scope across
frame boundaries but continue to scope in all the cases where it IS needed.

BUG=618937,627799
==========

to

==========
Fix find-in-page re-scope across frame boundaries.

There is a bug that can cause a frame to be re-scoped (all matches searched
again) when issuing a find next request across a frame boundary, such as from
the last match in a frame, which might need to wrap back around to the first
match in the same frame. If there are not many matches in that frame, this is
not really noticeable except for the ticks in the tickbar sometimes disappearing
for a single 
frame. However, on a page with a huge number of results, this can dramatically
slow down the find request and you can clearly see all of the ticks in the
tickbar regenerate.

This patch adjusts the conditions under which a frame is determined to need
scoping in WebLocalFrameImpl::requestFind(), in order to not re-scope across
frame boundaries but continue to scope in all the cases where it IS needed.

BUG=618937,627799
==========

[paulmeyer, 2016-08-12 17:30:02]

Description was changed from

==========
Fix find-in-page re-scope across frame boundaries.

There is a bug that can cause a frame to be re-scoped (all matches searched
again) when issuing a find next request across a frame boundary, such as from
the last match in a frame, which might need to wrap back around to the first
match in the same frame. If there are not many matches in that frame, this is
not really noticeable except for the ticks in the tickbar sometimes disappearing
for a single 
frame. However, on a page with a huge number of results, this can dramatically
slow down the find request and you can clearly see all of the ticks in the
tickbar regenerate.

This patch adjusts the conditions under which a frame is determined to need
scoping in WebLocalFrameImpl::requestFind(), in order to not re-scope across
frame boundaries but continue to scope in all the cases where it IS needed.

BUG=618937,627799
==========

to

==========
Fix find-in-page re-scope across frame boundaries.

There is a bug that can cause a frame to be re-scoped (all matches searched
again) when issuing a find next request across a frame boundary, such as from
the last match in a frame, which might need to wrap back around to the first
match in the same frame. If there are not many matches in that frame, this is
not really noticeable except for the ticks in the tickbar sometimes disappearing
for a single frame. However, on a page with a huge number of results, this can
dramatically slow down the find request and you can clearly see all of the ticks
in the tickbar regenerate.

This patch adjusts the conditions under which a frame is determined to need
scoping in WebLocalFrameImpl::requestFind(), in order to not re-scope across
frame boundaries but continue to scope in all the cases where it IS needed.

BUG=618937,627799
==========

[paulmeyer, 2016-08-12 18:24:36]

Patchset #4 (id:80001) has been deleted

[paulmeyer, 2016-08-12 18:30:30]

On 2016/08/11 09:40:01, esprehn wrote:
>
https://codereview.chromium.org/2186113002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):
> 
>
https://codereview.chromium.org/2186113002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1996: if (options.findNext
> && currentSelection.isNull() && !textFinder()->scopingInProgress()) {
> Actually this means the activeNow bool in this function is just ignored, why
is
> that correct? What's the purpose of activeNow if we don't use it?

I've added a test.

Also, I noticed that |ActiveNow| wasn't used anymore in my original patch, but
forgot to remove it again after the refactor. That being said, I see that
without being used here, it's ONLY ever used in tests, in which case it doesn't
make sense to have the parameter at all. Because of this, I looked into the
original patch that added |activeNow| to better understand it, and it turns out
that there is some functionality that would be missing if I take it out
completely. I didn't realize that before since there are NO TESTS that test this
functionality (there really should be, so I think I'll just write one myself in
a subsequent CL). In any case, I've reworked the conditions under which scoping
occurs a bit more so that I can keep |activeNow| but still not re-scope at
unnecessary times.

PTAL.

[paulmeyer, 2016-08-15 13:44:18]

The CQ bit was checked by paulmeyer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-15 13:44:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-15 13:46:01]

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

[commit-bot: I haz the power, 2016-08-15 13:46:02]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[paulmeyer, 2016-08-15 16:35:13]

The CQ bit was checked by paulmeyer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-15 16:35:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[paulmeyer, 2016-08-15 16:39:44]

Patchset #4 (id:100001) has been deleted

[commit-bot: I haz the power, 2016-08-15 16:41:46]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2016-08-15 16:44:04]

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

[commit-bot: I haz the power, 2016-08-15 16:44:05]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[paulmeyer, 2016-08-15 17:51:12]

The CQ bit was checked by paulmeyer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-15 17:51:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-15 19:19:45]

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

[commit-bot: I haz the power, 2016-08-15 19:19:45]

Dry run: This issue passed the CQ dry run.

[paulmeyer, 2016-08-15 19:56:27]

The CQ bit was checked by paulmeyer@chromium.org

[paulmeyer, 2016-08-15 19:56:27]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/2186113002/#ps120001
(title: "Rebased")

[commit-bot: I haz the power, 2016-08-15 19:57:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-15 20:01:05]

Description was changed from

==========
Fix find-in-page re-scope across frame boundaries.

There is a bug that can cause a frame to be re-scoped (all matches searched
again) when issuing a find next request across a frame boundary, such as from
the last match in a frame, which might need to wrap back around to the first
match in the same frame. If there are not many matches in that frame, this is
not really noticeable except for the ticks in the tickbar sometimes disappearing
for a single frame. However, on a page with a huge number of results, this can
dramatically slow down the find request and you can clearly see all of the ticks
in the tickbar regenerate.

This patch adjusts the conditions under which a frame is determined to need
scoping in WebLocalFrameImpl::requestFind(), in order to not re-scope across
frame boundaries but continue to scope in all the cases where it IS needed.

BUG=618937,627799
==========

to

==========
Fix find-in-page re-scope across frame boundaries.

There is a bug that can cause a frame to be re-scoped (all matches searched
again) when issuing a find next request across a frame boundary, such as from
the last match in a frame, which might need to wrap back around to the first
match in the same frame. If there are not many matches in that frame, this is
not really noticeable except for the ticks in the tickbar sometimes disappearing
for a single frame. However, on a page with a huge number of results, this can
dramatically slow down the find request and you can clearly see all of the ticks
in the tickbar regenerate.

This patch adjusts the conditions under which a frame is determined to need
scoping in WebLocalFrameImpl::requestFind(), in order to not re-scope across
frame boundaries but continue to scope in all the cases where it IS needed.

BUG=618937,627799
==========

[commit-bot: I haz the power, 2016-08-15 20:01:08]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2016-08-15 20:04:46]

Description was changed from

==========
Fix find-in-page re-scope across frame boundaries.

There is a bug that can cause a frame to be re-scoped (all matches searched
again) when issuing a find next request across a frame boundary, such as from
the last match in a frame, which might need to wrap back around to the first
match in the same frame. If there are not many matches in that frame, this is
not really noticeable except for the ticks in the tickbar sometimes disappearing
for a single frame. However, on a page with a huge number of results, this can
dramatically slow down the find request and you can clearly see all of the ticks
in the tickbar regenerate.

This patch adjusts the conditions under which a frame is determined to need
scoping in WebLocalFrameImpl::requestFind(), in order to not re-scope across
frame boundaries but continue to scope in all the cases where it IS needed.

BUG=618937,627799
==========

to

==========
Fix find-in-page re-scope across frame boundaries.

There is a bug that can cause a frame to be re-scoped (all matches searched
again) when issuing a find next request across a frame boundary, such as from
the last match in a frame, which might need to wrap back around to the first
match in the same frame. If there are not many matches in that frame, this is
not really noticeable except for the ticks in the tickbar sometimes disappearing
for a single frame. However, on a page with a huge number of results, this can
dramatically slow down the find request and you can clearly see all of the ticks
in the tickbar regenerate.

This patch adjusts the conditions under which a frame is determined to need
scoping in WebLocalFrameImpl::requestFind(), in order to not re-scope across
frame boundaries but continue to scope in all the cases where it IS needed.

BUG=618937,627799

Committed: https://crrev.com/b6f12c557fb9b1468ba6f6da5f55cc7cefc045b0
Cr-Commit-Position: refs/heads/master@{#412025}
==========

[commit-bot: I haz the power, 2016-08-15 20:04:47]

Patchset 4 (id:??) landed as
https://crrev.com/b6f12c557fb9b1468ba6f6da5f55cc7cefc045b0
Cr-Commit-Position: refs/heads/master@{#412025}

[paulmeyer, 2016-08-15 20:39:16]

On 2016/08/12 18:30:30, paulmeyer wrote:
> On 2016/08/11 09:40:01, esprehn wrote:
> >
>
https://codereview.chromium.org/2186113002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2186113002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1996: if
(options.findNext
> > && currentSelection.isNull() && !textFinder()->scopingInProgress()) {
> > Actually this means the activeNow bool in this function is just ignored, why
> is
> > that correct? What's the purpose of activeNow if we don't use it?
> 
> I've added a test.
> 
> Also, I noticed that |ActiveNow| wasn't used anymore in my original patch, but
> forgot to remove it again after the refactor. That being said, I see that
> without being used here, it's ONLY ever used in tests, in which case it
doesn't
> make sense to have the parameter at all. Because of this, I looked into the
> original patch that added |activeNow| to better understand it, and it turns
out
> that there is some functionality that would be missing if I take it out
> completely. I didn't realize that before since there are NO TESTS that test
this
> functionality (there really should be, so I think I'll just write one myself
in
> a subsequent CL). In any case, I've reworked the conditions under which
scoping
> occurs a bit more so that I can keep |activeNow| but still not re-scope at
> unnecessary times.
> 
> PTAL.

I see that you're away for awhile and I haven't changed much other than adding a
test and addressing your comment about |ActiveNow| since you LGTM'd, so I
committed the patch. Free free to look it over again though and let me know if
you have any concerns with the test or anything else.

[Bret, 2016-08-15 22:13:45]

A revert of this CL (patchset #4 id:120001) has been created in
https://codereview.chromium.org/2241183003/ by bsep@chromium.org.

The reason for reverting is: Multiple bots are timing out on
FindRequestManagerTest.RemoveFrame/1 since this patch landed. No one else has
touched FindRequestManagerTests recently..

[Bret, 2016-08-15 22:16:30]

On 2016/08/15 22:13:45, Bret Sepulveda wrote:
> A revert of this CL (patchset #4 id:120001) has been created in
> https://codereview.chromium.org/2241183003/ by mailto:bsep@chromium.org.
> 
> The reason for reverting is: Multiple bots are timing out on
> FindRequestManagerTest.RemoveFrame/1 since this patch landed. No one else has
> touched FindRequestManagerTests recently..

Example failure:
https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%2...

content_browsertests is failing, reporting
FindRequestManagerTests/FindRequestManagerTest.RemoveFrame/1 as timed out

[paulmeyer, 2016-09-02 19:20:24]

Patchset #6 (id:160001) has been deleted