Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1605863002:
Restart search in page when new text is found. (Closed)
|
Created:
4 years, 11 months ago by dvadym Modified:
4 years, 10 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dcheng, dglazkov+blink, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-blink_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRestart search in page when new text is found.
This patch is a partial solution to the problem, when DOM was changed and
Find Next In Page fails to show correct results that corresponding to the
current DOM.
For context, some details how Find in Page works:
1. When the user initializes new search RenderFrameImpl::OnFind is called, in
order not to block UI it quickly returns the first result and schedules new full
search task (it's called scoping request in code).
2. The full search task runs and adds all occurrences of the searched text as
markers in DocumentMarkerController. These markers are presented to the user as
search results.
3. When the user starts FindNext in browser, again RenderFrameImpl::OnFind is
called, it runs search from the place of previous search result and in
TextFinder it's called DocumentMarkerController::setMarkersActive to set the
current result. No new markers to DocumentMarkerController are added.
Current FindNext implementation (TextFinder) uses current DOM and gives correct
results, the problem is that DocumentMarkerController::setMarkersActive silently
fails to add marker when it receives a search result in content that was added
after full search in 2.
The proposed solution in this patch:
1. When DocumentMarkerController::setMarkersActive fails to find marker that
corresponds to a current FindNext result (that means that it's new part of DOM,
that was loaded after the initial full search), it adds this result range as
active marker and returns false to TextFinder.
2. TextFinder receives information that the new text found, it stores restore
point to the following full search (so search will continue from the same
place). And returns to RenderFrameImpl activeNow=false.
3. RenderFrameImpl receives that activeNow=false restarts full search (in a
separate task, the same as with original search).
This patch doesn't solve all problems, namely
1. In order to see new results the user should use FindNext till new results are
found.
2. If something is removed from DOM, new search is not restarted (it's hard to
find out about this from DocumentMarkerController), so the user will see
incorrect number of results, but the user will still see correct highlighted
search results in a page (the same behaviour as now).
but it makes problems much less severe.
BUG=2220
Committed: https://crrev.com/22cbbba006181412d51841e6bf3a7b53d9b653df
Cr-Commit-Position: refs/heads/master@{#376994}
Patch Set 1 #Patch Set 2 : Rebase, clean-up #
Total comments: 10
Patch Set 3 : Uncommenting tests, cleaning logic #Patch Set 4 : Rebase #Patch Set 5 : Rebase #Patch Set 6 : Rebase fix #Patch Set 7 : Compilation fix #Patch Set 8 : Tests added #Patch Set 9 : Renaming, clean-up #Patch Set 10 : Clean up #Patch Set 11 : small clean up #
Total comments: 12
Patch Set 12 : Reviewer's comments addressed #Patch Set 13 : Tiny style fix #Patch Set 14 : Rebase #Patch Set 15 : Rebase #
Total comments: 6
Patch Set 16 : Comments addressed #Patch Set 17 : Rebase #
Total comments: 4
Patch Set 18 : Comments addressed #Patch Set 19 : Rebase and added "==nullptr" in WebLocalFrameImpl #Patch Set 20 : Rebase #
Total comments: 2
Patch Set 21 : Added default value for local var #Messages
Total messages: 57 (24 generated)
Description was changed from ========== Restart search in page when new text is found. BUG=2220 ========== to ========== Restart search in page when new text is found. This patch is a partial solution to the problem, when DOM was changed and Find Next In Page fails to show correct results that corresponding to the current DOM. For context, some details how Find in Page works: 1.When the user initializes new search RenderViewImpl::OnFind is called, in order not to block UI it quickly returns the first result and schedules new full search task (it's called scoping request in code). 2.The full search task runs and adds all marks all occurrences of the searched text, by adding them in as markers in DocumentMarkerController. 3.When the user starts FindNext in browser, again RenderViewImpl::OnFind is called, it runs search from the place of previous search result and in TextFinder it's called DocumentMarkerController::setMarkersActive to set the current result. Current FindNext implementation (TextFinder) uses current DOM and gives correct results, the problem is that DocumentMarkerController::setMarkersActive silently fails to add marker when it receives a search result in content that was added after full search in 2. The proposed solution in this patch: 1.When DocumentMarkerController::setMarkersActive fails to find marker that corresponds to a current FindNext result (that means that it's new part of DOM, that was loaded after the initial full search), it adds this range as active markers and returns false to TextFinder. 2.TextFinder receives information that the new text found, it stores restore point to the following full search (so search will continue from the same place). And returns to RenderViewImpl newTextFound=true. 3.RenderViewImpl receives that newTextFound=true restarts full search (in separate task, the same as with original search). This patch doesn't solve all problems, namely 1.In order to show new results user should use FindNext till new results are found. 2.If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController), so the user will see incorrect number of results, but the user will see correct search results in a page (the same behaviour as now). The failing tests are commented, since it's not final version yet. BUG=2220 ==========
Description was changed from ========== Restart search in page when new text is found. This patch is a partial solution to the problem, when DOM was changed and Find Next In Page fails to show correct results that corresponding to the current DOM. For context, some details how Find in Page works: 1.When the user initializes new search RenderViewImpl::OnFind is called, in order not to block UI it quickly returns the first result and schedules new full search task (it's called scoping request in code). 2.The full search task runs and adds all marks all occurrences of the searched text, by adding them in as markers in DocumentMarkerController. 3.When the user starts FindNext in browser, again RenderViewImpl::OnFind is called, it runs search from the place of previous search result and in TextFinder it's called DocumentMarkerController::setMarkersActive to set the current result. Current FindNext implementation (TextFinder) uses current DOM and gives correct results, the problem is that DocumentMarkerController::setMarkersActive silently fails to add marker when it receives a search result in content that was added after full search in 2. The proposed solution in this patch: 1.When DocumentMarkerController::setMarkersActive fails to find marker that corresponds to a current FindNext result (that means that it's new part of DOM, that was loaded after the initial full search), it adds this range as active markers and returns false to TextFinder. 2.TextFinder receives information that the new text found, it stores restore point to the following full search (so search will continue from the same place). And returns to RenderViewImpl newTextFound=true. 3.RenderViewImpl receives that newTextFound=true restarts full search (in separate task, the same as with original search). This patch doesn't solve all problems, namely 1.In order to show new results user should use FindNext till new results are found. 2.If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController), so the user will see incorrect number of results, but the user will see correct search results in a page (the same behaviour as now). The failing tests are commented, since it's not final version yet. BUG=2220 ========== to ========== Restart search in page when new text is found. This patch is a partial solution to the problem, when DOM was changed and Find Next In Page fails to show correct results that corresponding to the current DOM. For context, some details how Find in Page works: 1.When the user initializes new search RenderViewImpl::OnFind is called, in order not to block UI it quickly returns the first result and schedules new full search task (it's called scoping request in code). 2.The full search task runs and adds all occurrences of the searched text, by adding them in as markers in DocumentMarkerController. 3.When the user starts FindNext in browser, again RenderViewImpl::OnFind is called, it runs search from the place of previous search result and in TextFinder it's called DocumentMarkerController::setMarkersActive to set the current result. No new markers to DocumentMarkerController are added. Current FindNext implementation (TextFinder) uses current DOM and gives correct results, the problem is that DocumentMarkerController::setMarkersActive silently fails to add marker when it receives a search result in content that was added after full search in 2. The proposed solution in this patch: 1.When DocumentMarkerController::setMarkersActive fails to find marker that corresponds to a current FindNext result (that means that it's new part of DOM, that was loaded after the initial full search), it adds this range as active markers and returns false to TextFinder. 2.TextFinder receives information that the new text found, it stores restore point to the following full search (so search will continue from the same place). And returns to RenderViewImpl newTextFound=true. 3.RenderViewImpl receives that newTextFound=true restarts full search (in separate task, the same as with original search). This patch doesn't solve all problems, namely 1.In order to show new results user should use FindNext till new results are found. 2.If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController), so the user will see incorrect number of results, but the user will see correct search results in a page (the same behaviour as now). The failing tests are commented, since it's not final version yet. BUG=2220 ==========
Description was changed from ========== Restart search in page when new text is found. This patch is a partial solution to the problem, when DOM was changed and Find Next In Page fails to show correct results that corresponding to the current DOM. For context, some details how Find in Page works: 1.When the user initializes new search RenderViewImpl::OnFind is called, in order not to block UI it quickly returns the first result and schedules new full search task (it's called scoping request in code). 2.The full search task runs and adds all occurrences of the searched text, by adding them in as markers in DocumentMarkerController. 3.When the user starts FindNext in browser, again RenderViewImpl::OnFind is called, it runs search from the place of previous search result and in TextFinder it's called DocumentMarkerController::setMarkersActive to set the current result. No new markers to DocumentMarkerController are added. Current FindNext implementation (TextFinder) uses current DOM and gives correct results, the problem is that DocumentMarkerController::setMarkersActive silently fails to add marker when it receives a search result in content that was added after full search in 2. The proposed solution in this patch: 1.When DocumentMarkerController::setMarkersActive fails to find marker that corresponds to a current FindNext result (that means that it's new part of DOM, that was loaded after the initial full search), it adds this range as active markers and returns false to TextFinder. 2.TextFinder receives information that the new text found, it stores restore point to the following full search (so search will continue from the same place). And returns to RenderViewImpl newTextFound=true. 3.RenderViewImpl receives that newTextFound=true restarts full search (in separate task, the same as with original search). This patch doesn't solve all problems, namely 1.In order to show new results user should use FindNext till new results are found. 2.If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController), so the user will see incorrect number of results, but the user will see correct search results in a page (the same behaviour as now). The failing tests are commented, since it's not final version yet. BUG=2220 ========== to ========== Restart search in page when new text is found. This patch is a partial solution to the problem, when DOM was changed and Find Next In Page fails to show correct results that corresponding to the current DOM. For context, some details how Find in Page works: 1.When the user initializes new search RenderViewImpl::OnFind is called, in order not to block UI it quickly returns the first result and schedules new full search task (it's called scoping request in code). 2.The full search task runs and adds all occurrences of the searched text, by adding them in as markers in DocumentMarkerController. 3.When the user starts FindNext in browser, again RenderViewImpl::OnFind is called, it runs search from the place of previous search result and in TextFinder it's called DocumentMarkerController::setMarkersActive to set the current result. No new markers to DocumentMarkerController are added. Current FindNext implementation (TextFinder) uses current DOM and gives correct results, the problem is that DocumentMarkerController::setMarkersActive silently fails to add marker when it receives a search result in content that was added after full search in 2. The proposed solution in this patch: 1.When DocumentMarkerController::setMarkersActive fails to find marker that corresponds to a current FindNext result (that means that it's new part of DOM, that was loaded after the initial full search), it adds this result range as active marker and returns false to TextFinder. 2.TextFinder receives information that the new text found, it stores restore point to the following full search (so search will continue from the same place). And returns to RenderViewImpl newTextFound=true. 3.RenderViewImpl receives that newTextFound=true restarts full search (in a separate task, the same as with original search). This patch doesn't solve all problems, namely 1.In order to show new results user should use FindNext till new results are found. 2.If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController), so the user will see incorrect number of results, but the user will see correct search results in a page (the same behaviour as now). but it makes problems much less visible. The failing tests are commented, since it's not final version yet. BUG=2220 ==========
Description was changed from ========== Restart search in page when new text is found. This patch is a partial solution to the problem, when DOM was changed and Find Next In Page fails to show correct results that corresponding to the current DOM. For context, some details how Find in Page works: 1.When the user initializes new search RenderViewImpl::OnFind is called, in order not to block UI it quickly returns the first result and schedules new full search task (it's called scoping request in code). 2.The full search task runs and adds all occurrences of the searched text, by adding them in as markers in DocumentMarkerController. 3.When the user starts FindNext in browser, again RenderViewImpl::OnFind is called, it runs search from the place of previous search result and in TextFinder it's called DocumentMarkerController::setMarkersActive to set the current result. No new markers to DocumentMarkerController are added. Current FindNext implementation (TextFinder) uses current DOM and gives correct results, the problem is that DocumentMarkerController::setMarkersActive silently fails to add marker when it receives a search result in content that was added after full search in 2. The proposed solution in this patch: 1.When DocumentMarkerController::setMarkersActive fails to find marker that corresponds to a current FindNext result (that means that it's new part of DOM, that was loaded after the initial full search), it adds this result range as active marker and returns false to TextFinder. 2.TextFinder receives information that the new text found, it stores restore point to the following full search (so search will continue from the same place). And returns to RenderViewImpl newTextFound=true. 3.RenderViewImpl receives that newTextFound=true restarts full search (in a separate task, the same as with original search). This patch doesn't solve all problems, namely 1.In order to show new results user should use FindNext till new results are found. 2.If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController), so the user will see incorrect number of results, but the user will see correct search results in a page (the same behaviour as now). but it makes problems much less visible. The failing tests are commented, since it's not final version yet. BUG=2220 ========== to ========== Restart search in page when new text is found. This patch is a partial solution to the problem, when DOM was changed and Find Next In Page fails to show correct results that corresponding to the current DOM. For context, some details how Find in Page works: 1.When the user initializes new search RenderViewImpl::OnFind is called, in order not to block UI it quickly returns the first result and schedules new full search task (it's called scoping request in code). 2.The full search task runs and adds all occurrences of the searched text as markers in DocumentMarkerController. These markers are presented to the user as search results. 3.When the user starts FindNext in browser, again RenderViewImpl::OnFind is called, it runs search from the place of previous search result and in TextFinder it's called DocumentMarkerController::setMarkersActive to set the current result. No new markers to DocumentMarkerController are added. Current FindNext implementation (TextFinder) uses current DOM and gives correct results, the problem is that DocumentMarkerController::setMarkersActive silently fails to add marker when it receives a search result in content that was added after full search in 2. The proposed solution in this patch: 1.When DocumentMarkerController::setMarkersActive fails to find marker that corresponds to a current FindNext result (that means that it's new part of DOM, that was loaded after the initial full search), it adds this result range as active marker and returns false to TextFinder. 2.TextFinder receives information that the new text found, it stores restore point to the following full search (so search will continue from the same place). And returns to RenderViewImpl newTextFound=true. 3.RenderViewImpl receives that newTextFound=true restarts full search (in a separate task, the same as with original search). This patch doesn't solve all problems, namely 1.In order to show new results user should use FindNext till new results are found. 2.If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController), so the user will see incorrect number of results, but the user will see correct search results in a page (the same behaviour as now). but it makes problems much less visible. The failing tests are commented, since it's not final version yet. BUG=2220 ==========
Description was changed from ========== Restart search in page when new text is found. This patch is a partial solution to the problem, when DOM was changed and Find Next In Page fails to show correct results that corresponding to the current DOM. For context, some details how Find in Page works: 1.When the user initializes new search RenderViewImpl::OnFind is called, in order not to block UI it quickly returns the first result and schedules new full search task (it's called scoping request in code). 2.The full search task runs and adds all occurrences of the searched text as markers in DocumentMarkerController. These markers are presented to the user as search results. 3.When the user starts FindNext in browser, again RenderViewImpl::OnFind is called, it runs search from the place of previous search result and in TextFinder it's called DocumentMarkerController::setMarkersActive to set the current result. No new markers to DocumentMarkerController are added. Current FindNext implementation (TextFinder) uses current DOM and gives correct results, the problem is that DocumentMarkerController::setMarkersActive silently fails to add marker when it receives a search result in content that was added after full search in 2. The proposed solution in this patch: 1.When DocumentMarkerController::setMarkersActive fails to find marker that corresponds to a current FindNext result (that means that it's new part of DOM, that was loaded after the initial full search), it adds this result range as active marker and returns false to TextFinder. 2.TextFinder receives information that the new text found, it stores restore point to the following full search (so search will continue from the same place). And returns to RenderViewImpl newTextFound=true. 3.RenderViewImpl receives that newTextFound=true restarts full search (in a separate task, the same as with original search). This patch doesn't solve all problems, namely 1.In order to show new results user should use FindNext till new results are found. 2.If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController), so the user will see incorrect number of results, but the user will see correct search results in a page (the same behaviour as now). but it makes problems much less visible. The failing tests are commented, since it's not final version yet. BUG=2220 ========== to ========== Restart search in page when new text is found. This patch is a partial solution to the problem, when DOM was changed and Find Next In Page fails to show correct results that corresponding to the current DOM. For context, some details how Find in Page works: 1.When the user initializes new search RenderViewImpl::OnFind is called, in order not to block UI it quickly returns the first result and schedules new full search task (it's called scoping request in code). 2.The full search task runs and adds all occurrences of the searched text as markers in DocumentMarkerController. These markers are presented to the user as search results. 3.When the user starts FindNext in browser, again RenderViewImpl::OnFind is called, it runs search from the place of previous search result and in TextFinder it's called DocumentMarkerController::setMarkersActive to set the current result. No new markers to DocumentMarkerController are added. Current FindNext implementation (TextFinder) uses current DOM and gives correct results, the problem is that DocumentMarkerController::setMarkersActive silently fails to add marker when it receives a search result in content that was added after full search in 2. The proposed solution in this patch: 1.When DocumentMarkerController::setMarkersActive fails to find marker that corresponds to a current FindNext result (that means that it's new part of DOM, that was loaded after the initial full search), it adds this result range as active marker and returns false to TextFinder. 2.TextFinder receives information that the new text found, it stores restore point to the following full search (so search will continue from the same place). And returns to RenderViewImpl newTextFound=true. 3.RenderViewImpl receives that newTextFound=true restarts full search (in a separate task, the same as with original search). This patch doesn't solve all problems, namely 1.In order to see new results the user should use FindNext till new results are found. 2.If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController), so the user will see incorrect number of results, but the user will see correct highlighted search results in a page (the same behaviour as now). but it makes problems much less visible. The failing tests are commented, since it's not final version yet. BUG=2220 ==========
Description was changed from ========== Restart search in page when new text is found. This patch is a partial solution to the problem, when DOM was changed and Find Next In Page fails to show correct results that corresponding to the current DOM. For context, some details how Find in Page works: 1.When the user initializes new search RenderViewImpl::OnFind is called, in order not to block UI it quickly returns the first result and schedules new full search task (it's called scoping request in code). 2.The full search task runs and adds all occurrences of the searched text as markers in DocumentMarkerController. These markers are presented to the user as search results. 3.When the user starts FindNext in browser, again RenderViewImpl::OnFind is called, it runs search from the place of previous search result and in TextFinder it's called DocumentMarkerController::setMarkersActive to set the current result. No new markers to DocumentMarkerController are added. Current FindNext implementation (TextFinder) uses current DOM and gives correct results, the problem is that DocumentMarkerController::setMarkersActive silently fails to add marker when it receives a search result in content that was added after full search in 2. The proposed solution in this patch: 1.When DocumentMarkerController::setMarkersActive fails to find marker that corresponds to a current FindNext result (that means that it's new part of DOM, that was loaded after the initial full search), it adds this result range as active marker and returns false to TextFinder. 2.TextFinder receives information that the new text found, it stores restore point to the following full search (so search will continue from the same place). And returns to RenderViewImpl newTextFound=true. 3.RenderViewImpl receives that newTextFound=true restarts full search (in a separate task, the same as with original search). This patch doesn't solve all problems, namely 1.In order to see new results the user should use FindNext till new results are found. 2.If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController), so the user will see incorrect number of results, but the user will see correct highlighted search results in a page (the same behaviour as now). but it makes problems much less visible. The failing tests are commented, since it's not final version yet. BUG=2220 ========== to ========== Restart search in page when new text is found. This patch is a partial solution to the problem, when DOM was changed and Find Next In Page fails to show correct results that corresponding to the current DOM. For context, some details how Find in Page works: 1.When the user initializes new search RenderViewImpl::OnFind is called, in order not to block UI it quickly returns the first result and schedules new full search task (it's called scoping request in code). 2.The full search task runs and adds all occurrences of the searched text as markers in DocumentMarkerController. These markers are presented to the user as search results. 3.When the user starts FindNext in browser, again RenderViewImpl::OnFind is called, it runs search from the place of previous search result and in TextFinder it's called DocumentMarkerController::setMarkersActive to set the current result. No new markers to DocumentMarkerController are added. Current FindNext implementation (TextFinder) uses current DOM and gives correct results, the problem is that DocumentMarkerController::setMarkersActive silently fails to add marker when it receives a search result in content that was added after full search in 2. The proposed solution in this patch: 1.When DocumentMarkerController::setMarkersActive fails to find marker that corresponds to a current FindNext result (that means that it's new part of DOM, that was loaded after the initial full search), it adds this result range as active marker and returns false to TextFinder. 2.TextFinder receives information that the new text found, it stores restore point to the following full search (so search will continue from the same place). And returns to RenderViewImpl newTextFound=true. 3.RenderViewImpl receives that newTextFound=true restarts full search (in a separate task, the same as with original search). This patch doesn't solve all problems, namely 1.In order to see new results the user should use FindNext till new results are found. 2.If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController), so the user will see incorrect number of results, but the user will see correct highlighted search results in a page (the same behaviour as now). but it makes problems much less visible. The failing tests are commented, no new tests for current patch were added, since it's not final version yet. BUG=2220 ==========
dvadym@chromium.org changed reviewers: + finnur@chromium.org
Hi Finnur, Could you please have a look at this CL? This CL is revive of our discussion from crbug.com/346271, that time I tried to approach it as my first bug, but it was too hard. I decided to try it once more. What do you think about approach in this CL? It's close to an option in your comment crbug.com/346271#c5: 1) Reset the search, somehow, which is what you are thinking of. Here search is reset when a new occurrence in DOM is found. Regards, Vadym
Do you have a good sense of why the current tests are failing?
On 2016/01/20 13:24:56, Finnur wrote: > Do you have a good sense of why the current tests are failing? They are not failing, I've changed interfaces of TextFinder::find and WebFrame::find, so they are not compiled (I don't know why but they are in chrome target, so not commenting them I even can't build Chrome). I believe they should pass. I can fix them, but I'm not sure what the best way of change of interfaces, in the current patch it is to return bool through parameters, another options is change return value from bool to some enum, which will indicate that a new text is found. Do you have some preference?
Haven't looked at the code yet, so maybe this doesn't apply -- but I'm just thinking out loud about what you wrote in the description... > 2.If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController) If the document *hasn't* changed, then FindNext would always discover matches in the same order as the scoping effort does, right? You also know which match is currently active. So can't you use that knowledge to detect this case too? Like, lets say you search for 'a' and find three matches, and scoping builds an array of matches which we'll call 1, 2, and 3. If 1 is currently active and you execute FindNext, and find 3, then new scoping is warranted because 2 was expected, right? > I can fix them, but I'm not sure what the best way of change of interfaces It would make me feel more confident in the patch if you fixed them. I'll also try to remember to comment on the interface change as I review.
> 2.If something is removed from DOM, new search is not restarted (it's hard to > find out about this from DocumentMarkerController) > > If the document *hasn't* changed, then FindNext would always discover matches in > the same order as the scoping effort does, right? You also know which match is > currently active. So can't you use that knowledge to detect this case too? > In Principle yes, but as far I can see it will require much more work. The reason is that DocumentMarkerController, which is the only place where all occurences are stored, stores them are unordered and it would require to make quite essential changes in it. And it seems that the case with deleting is not so important, since search results are not hidden from the user (but of course number of results is incorrect). > > I can fix them, but I'm not sure what the best way of change of interfaces > > It would make me feel more confident in the patch if you fixed them. I'll also > try to remember to comment on the interface change as I review. Sure, no problem, I'll fix these tests. Does such change of interfaces look reasonable?
I would be OK with making an interface change along those lines (see the nits, though). But I'm not a Webkit owner, so you'd still need someone to bless that. https://codereview.chromium.org/1605863002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/1605863002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:684: } On the face of it, this looks a bit weird -- adding a marker but returning false. I'm trying to wrap my head around why this is needed (been a long time since I looked at the DocumentMarkerController class). Can you fill me in? Maybe a comment would be in order? https://codereview.chromium.org/1605863002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:714: return true; Out of curiosity... why not return false here? https://codereview.chromium.org/1605863002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/TextFinder.cpp (right): https://codereview.chromium.org/1605863002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/TextFinder.cpp:176: // to find the active rect for us and report it back to the UI. Nit: This comment would now need updating too. https://codereview.chromium.org/1605863002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/TextFinder.cpp:693: return true; Wait... why not return false? https://codereview.chromium.org/1605863002/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebFrame.h (right): https://codereview.chromium.org/1605863002/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/web/WebFrame.h:568: bool& newTextFound) = 0; style: The style guide says |out| parameters should be pointers, not references. Also, |newTextFound| is a bit ambiguous in the context of find in page. I think I would prefer something like |nowActive|. Also, to answer your question: I don't see a reason to do an enum for this, this is basically a binary result, at least that's the amount of information we care about.
> In Principle yes, but as far I can see it will require much more work. After seeing the code I agree. The initial version of Find *didn't* have the DocumentMarker support for Webkit, so I was still going by the notion that Find kept track of all the matches (which is what it used to do). :)
Meant to say: The initial version of Find didn't have the DocumentMarker support *from* Webkit...
Description was changed from ========== Restart search in page when new text is found. This patch is a partial solution to the problem, when DOM was changed and Find Next In Page fails to show correct results that corresponding to the current DOM. For context, some details how Find in Page works: 1.When the user initializes new search RenderViewImpl::OnFind is called, in order not to block UI it quickly returns the first result and schedules new full search task (it's called scoping request in code). 2.The full search task runs and adds all occurrences of the searched text as markers in DocumentMarkerController. These markers are presented to the user as search results. 3.When the user starts FindNext in browser, again RenderViewImpl::OnFind is called, it runs search from the place of previous search result and in TextFinder it's called DocumentMarkerController::setMarkersActive to set the current result. No new markers to DocumentMarkerController are added. Current FindNext implementation (TextFinder) uses current DOM and gives correct results, the problem is that DocumentMarkerController::setMarkersActive silently fails to add marker when it receives a search result in content that was added after full search in 2. The proposed solution in this patch: 1.When DocumentMarkerController::setMarkersActive fails to find marker that corresponds to a current FindNext result (that means that it's new part of DOM, that was loaded after the initial full search), it adds this result range as active marker and returns false to TextFinder. 2.TextFinder receives information that the new text found, it stores restore point to the following full search (so search will continue from the same place). And returns to RenderViewImpl newTextFound=true. 3.RenderViewImpl receives that newTextFound=true restarts full search (in a separate task, the same as with original search). This patch doesn't solve all problems, namely 1.In order to see new results the user should use FindNext till new results are found. 2.If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController), so the user will see incorrect number of results, but the user will see correct highlighted search results in a page (the same behaviour as now). but it makes problems much less visible. The failing tests are commented, no new tests for current patch were added, since it's not final version yet. BUG=2220 ========== to ========== Restart search in page when new text is found. This patch is a partial solution to the problem, when DOM was changed and Find Next In Page fails to show correct results that corresponding to the current DOM. For context, some details how Find in Page works: 1.When the user initializes new search RenderViewImpl::OnFind is called, in order not to block UI it quickly returns the first result and schedules new full search task (it's called scoping request in code). 2.The full search task runs and adds all occurrences of the searched text as markers in DocumentMarkerController. These markers are presented to the user as search results. 3.When the user starts FindNext in browser, again RenderViewImpl::OnFind is called, it runs search from the place of previous search result and in TextFinder it's called DocumentMarkerController::setMarkersActive to set the current result. No new markers to DocumentMarkerController are added. Current FindNext implementation (TextFinder) uses current DOM and gives correct results, the problem is that DocumentMarkerController::setMarkersActive silently fails to add marker when it receives a search result in content that was added after full search in 2. The proposed solution in this patch: 1.When DocumentMarkerController::setMarkersActive fails to find marker that corresponds to a current FindNext result (that means that it's new part of DOM, that was loaded after the initial full search), it adds this result range as active marker and returns false to TextFinder. 2.TextFinder receives information that the new text found, it stores restore point to the following full search (so search will continue from the same place). And returns to RenderViewImpl newTextFound=true. 3.RenderViewImpl receives that newTextFound=true restarts full search (in a separate task, the same as with original search). This patch doesn't solve all problems, namely 1.In order to see new results the user should use FindNext till new results are found. 2.If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController), so the user will see incorrect number of results, but the user will see correct highlighted search results in a page (the same behaviour as now). but it makes problems much less visible. BUG=2220 ==========
Hi Finnur, Thanks a lot for comments! I've addressed all of them and made some code clean-up. Also I've fixed all tests and added new ones for newly added code (it took more time to add new tests that I thought previously :)). Sure I know that it's needed to ask somebody from Blink owners to review this patch. Could you please have a look? Regards, Vadym https://codereview.chromium.org/1605863002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/1605863002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:684: } On 2016/01/20 14:14:03, Finnur wrote: > On the face of it, this looks a bit weird -- adding a marker but returning > false. I'm trying to wrap my head around why this is needed (been a long time > since I looked at the DocumentMarkerController class). > > Can you fill me in? Maybe a comment would be in order? At first I thought to add markers here when there are no appropriate markers. Thanks for comments, I've realized that it's better not to add them, since they will be added on search restart. https://codereview.chromium.org/1605863002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:714: return true; On 2016/01/20 14:14:03, Finnur wrote: > Out of curiosity... why not return false here? Agree, fixed. https://codereview.chromium.org/1605863002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/TextFinder.cpp (right): https://codereview.chromium.org/1605863002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/TextFinder.cpp:176: // to find the active rect for us and report it back to the UI. On 2016/01/20 14:14:03, Finnur wrote: > Nit: This comment would now need updating too. Done. https://codereview.chromium.org/1605863002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/TextFinder.cpp:693: return true; On 2016/01/20 14:14:03, Finnur wrote: > Wait... why not return false? Done. https://codereview.chromium.org/1605863002/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebFrame.h (right): https://codereview.chromium.org/1605863002/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/web/WebFrame.h:568: bool& newTextFound) = 0; On 2016/01/20 14:14:03, Finnur wrote: > style: The style guide says |out| parameters should be pointers, not references. > > Also, |newTextFound| is a bit ambiguous in the context of find in page. I think > I would prefer something like |nowActive|. > > Also, to answer your question: I don't see a reason to do an enum for this, this > is basically a binary result, at least that's the amount of information we care > about. Thanks, I've changed to pointer and renamed to activeNow in whole patch.
Description was changed from ========== Restart search in page when new text is found. This patch is a partial solution to the problem, when DOM was changed and Find Next In Page fails to show correct results that corresponding to the current DOM. For context, some details how Find in Page works: 1.When the user initializes new search RenderViewImpl::OnFind is called, in order not to block UI it quickly returns the first result and schedules new full search task (it's called scoping request in code). 2.The full search task runs and adds all occurrences of the searched text as markers in DocumentMarkerController. These markers are presented to the user as search results. 3.When the user starts FindNext in browser, again RenderViewImpl::OnFind is called, it runs search from the place of previous search result and in TextFinder it's called DocumentMarkerController::setMarkersActive to set the current result. No new markers to DocumentMarkerController are added. Current FindNext implementation (TextFinder) uses current DOM and gives correct results, the problem is that DocumentMarkerController::setMarkersActive silently fails to add marker when it receives a search result in content that was added after full search in 2. The proposed solution in this patch: 1.When DocumentMarkerController::setMarkersActive fails to find marker that corresponds to a current FindNext result (that means that it's new part of DOM, that was loaded after the initial full search), it adds this result range as active marker and returns false to TextFinder. 2.TextFinder receives information that the new text found, it stores restore point to the following full search (so search will continue from the same place). And returns to RenderViewImpl newTextFound=true. 3.RenderViewImpl receives that newTextFound=true restarts full search (in a separate task, the same as with original search). This patch doesn't solve all problems, namely 1.In order to see new results the user should use FindNext till new results are found. 2.If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController), so the user will see incorrect number of results, but the user will see correct highlighted search results in a page (the same behaviour as now). but it makes problems much less visible. BUG=2220 ========== to ========== Restart search in page when new text is found. This patch is a partial solution to the problem, when DOM was changed and Find Next In Page fails to show correct results that corresponding to the current DOM. For context, some details how Find in Page works: 1.When the user initializes new search RenderViewImpl::OnFind is called, in order not to block UI it quickly returns the first result and schedules new full search task (it's called scoping request in code). 2.The full search task runs and adds all occurrences of the searched text as markers in DocumentMarkerController. These markers are presented to the user as search results. 3.When the user starts FindNext in browser, again RenderViewImpl::OnFind is called, it runs search from the place of previous search result and in TextFinder it's called DocumentMarkerController::setMarkersActive to set the current result. No new markers to DocumentMarkerController are added. Current FindNext implementation (TextFinder) uses current DOM and gives correct results, the problem is that DocumentMarkerController::setMarkersActive silently fails to add marker when it receives a search result in content that was added after full search in 2. The proposed solution in this patch: 1.When DocumentMarkerController::setMarkersActive fails to find marker that corresponds to a current FindNext result (that means that it's new part of DOM, that was loaded after the initial full search), it adds this result range as active marker and returns false to TextFinder. 2.TextFinder receives information that the new text found, it stores restore point to the following full search (so search will continue from the same place). And returns to RenderViewImpl activeNow=true. 3.RenderViewImpl receives that activeNow=true restarts full search (in a separate task, the same as with original search). This patch doesn't solve all problems, namely 1.In order to see new results the user should use FindNext till new results are found. 2.If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController), so the user will see incorrect number of results, but the user will see correct highlighted search results in a page (the same behaviour as now). but it makes problems much less visible. BUG=2220 ==========
Description was changed from ========== Restart search in page when new text is found. This patch is a partial solution to the problem, when DOM was changed and Find Next In Page fails to show correct results that corresponding to the current DOM. For context, some details how Find in Page works: 1.When the user initializes new search RenderViewImpl::OnFind is called, in order not to block UI it quickly returns the first result and schedules new full search task (it's called scoping request in code). 2.The full search task runs and adds all occurrences of the searched text as markers in DocumentMarkerController. These markers are presented to the user as search results. 3.When the user starts FindNext in browser, again RenderViewImpl::OnFind is called, it runs search from the place of previous search result and in TextFinder it's called DocumentMarkerController::setMarkersActive to set the current result. No new markers to DocumentMarkerController are added. Current FindNext implementation (TextFinder) uses current DOM and gives correct results, the problem is that DocumentMarkerController::setMarkersActive silently fails to add marker when it receives a search result in content that was added after full search in 2. The proposed solution in this patch: 1.When DocumentMarkerController::setMarkersActive fails to find marker that corresponds to a current FindNext result (that means that it's new part of DOM, that was loaded after the initial full search), it adds this result range as active marker and returns false to TextFinder. 2.TextFinder receives information that the new text found, it stores restore point to the following full search (so search will continue from the same place). And returns to RenderViewImpl activeNow=true. 3.RenderViewImpl receives that activeNow=true restarts full search (in a separate task, the same as with original search). This patch doesn't solve all problems, namely 1.In order to see new results the user should use FindNext till new results are found. 2.If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController), so the user will see incorrect number of results, but the user will see correct highlighted search results in a page (the same behaviour as now). but it makes problems much less visible. BUG=2220 ========== to ========== Restart search in page when new text is found. This patch is a partial solution to the problem, when DOM was changed and Find Next In Page fails to show correct results that corresponding to the current DOM. For context, some details how Find in Page works: 1.When the user initializes new search RenderViewImpl::OnFind is called, in order not to block UI it quickly returns the first result and schedules new full search task (it's called scoping request in code). 2.The full search task runs and adds all occurrences of the searched text as markers in DocumentMarkerController. These markers are presented to the user as search results. 3.When the user starts FindNext in browser, again RenderViewImpl::OnFind is called, it runs search from the place of previous search result and in TextFinder it's called DocumentMarkerController::setMarkersActive to set the current result. No new markers to DocumentMarkerController are added. Current FindNext implementation (TextFinder) uses current DOM and gives correct results, the problem is that DocumentMarkerController::setMarkersActive silently fails to add marker when it receives a search result in content that was added after full search in 2. The proposed solution in this patch: 1.When DocumentMarkerController::setMarkersActive fails to find marker that corresponds to a current FindNext result (that means that it's new part of DOM, that was loaded after the initial full search), it adds this result range as active marker and returns false to TextFinder. 2.TextFinder receives information that the new text found, it stores restore point to the following full search (so search will continue from the same place). And returns to RenderViewImpl activeNow=true. 3.RenderViewImpl receives that activeNow=true restarts full search (in a separate task, the same as with original search). This patch doesn't solve all problems, namely 1.In order to see new results the user should use FindNext till new results are found. 2.If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController), so the user will see incorrect number of results, but the user will see correct highlighted search results in a page (the same behaviour as now). but it makes problems much less severe. BUG=2220 ==========
Description was changed from ========== Restart search in page when new text is found. This patch is a partial solution to the problem, when DOM was changed and Find Next In Page fails to show correct results that corresponding to the current DOM. For context, some details how Find in Page works: 1.When the user initializes new search RenderViewImpl::OnFind is called, in order not to block UI it quickly returns the first result and schedules new full search task (it's called scoping request in code). 2.The full search task runs and adds all occurrences of the searched text as markers in DocumentMarkerController. These markers are presented to the user as search results. 3.When the user starts FindNext in browser, again RenderViewImpl::OnFind is called, it runs search from the place of previous search result and in TextFinder it's called DocumentMarkerController::setMarkersActive to set the current result. No new markers to DocumentMarkerController are added. Current FindNext implementation (TextFinder) uses current DOM and gives correct results, the problem is that DocumentMarkerController::setMarkersActive silently fails to add marker when it receives a search result in content that was added after full search in 2. The proposed solution in this patch: 1.When DocumentMarkerController::setMarkersActive fails to find marker that corresponds to a current FindNext result (that means that it's new part of DOM, that was loaded after the initial full search), it adds this result range as active marker and returns false to TextFinder. 2.TextFinder receives information that the new text found, it stores restore point to the following full search (so search will continue from the same place). And returns to RenderViewImpl activeNow=true. 3.RenderViewImpl receives that activeNow=true restarts full search (in a separate task, the same as with original search). This patch doesn't solve all problems, namely 1.In order to see new results the user should use FindNext till new results are found. 2.If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController), so the user will see incorrect number of results, but the user will see correct highlighted search results in a page (the same behaviour as now). but it makes problems much less severe. BUG=2220 ========== to ========== Restart search in page when new text is found. This patch is a partial solution to the problem, when DOM was changed and Find Next In Page fails to show correct results that corresponding to the current DOM. For context, some details how Find in Page works: 1.When the user initializes new search RenderViewImpl::OnFind is called, in order not to block UI it quickly returns the first result and schedules new full search task (it's called scoping request in code). 2.The full search task runs and adds all occurrences of the searched text as markers in DocumentMarkerController. These markers are presented to the user as search results. 3.When the user starts FindNext in browser, again RenderViewImpl::OnFind is called, it runs search from the place of previous search result and in TextFinder it's called DocumentMarkerController::setMarkersActive to set the current result. No new markers to DocumentMarkerController are added. Current FindNext implementation (TextFinder) uses current DOM and gives correct results, the problem is that DocumentMarkerController::setMarkersActive silently fails to add marker when it receives a search result in content that was added after full search in 2. The proposed solution in this patch: 1.When DocumentMarkerController::setMarkersActive fails to find marker that corresponds to a current FindNext result (that means that it's new part of DOM, that was loaded after the initial full search), it adds this result range as active marker and returns false to TextFinder. 2.TextFinder receives information that the new text found, it stores restore point to the following full search (so search will continue from the same place). And returns to RenderViewImpl activeNow=true. 3.RenderViewImpl receives that activeNow=true restarts full search (in a separate task, the same as with original search). This patch doesn't solve all problems, namely 1.In order to see new results the user should use FindNext till new results are found. 2.If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController), so the user will see incorrect number of results, but the user will still see correct highlighted search results in a page (the same behaviour as now). but it makes problems much less severe. BUG=2220 ==========
LGTM, modulo nits. Thanks for doing this! > 3.RenderViewImpl receives that activeNow=true You mean: activeNow = false, right? https://codereview.chromium.org/1605863002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp (right): https://codereview.chromium.org/1605863002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp:260: RefPtrWillBeRawPtr<Range> range = Range::create(document(), toPositionInDOMTree(ephemeralRange.startPosition()), toPositionInDOMTree(ephemeralRange.endPosition())); nit: Break up into two lines for readability? https://codereview.chromium.org/1605863002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/TextFinder.cpp (right): https://codereview.chromium.org/1605863002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/web/TextFinder.cpp:169: setMarkerActive(m_activeMatch.get(), true); Not as verbose and probably a bit clearer: bool isActive = setMarkerActive(m_activeMatch.get(), true); if (activeNow) *activeNow = isActive; Has benefits below too. https://codereview.chromium.org/1605863002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/web/TextFinder.cpp:177: if (!options.findNext || activeSelection || (activeNow && !*activeNow)) { Replace (activeNow && !*activeNow) with !isActive https://codereview.chromium.org/1605863002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/web/TextFinder.cpp:179: // due to a selection, or Find-next which found newly added text, so we Suggest: "or new matches were found during Find-next due to DOM alteration (that couldn't be set as active)" https://codereview.chromium.org/1605863002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/1605863002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:4110: WebScriptSource("document.body.insertBefore(document.createTextNode('bar5 foo5'), document.getElementsByTagName('textarea')[0]);")); Nit: I would probably try to break this up into more lines for readability. https://codereview.chromium.org/1605863002/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebFrame.h (left): https://codereview.chromium.org/1605863002/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/web/WebFrame.h:561: const WebFindOptions& options, Nit: Why delete this?
Description was changed from ========== Restart search in page when new text is found. This patch is a partial solution to the problem, when DOM was changed and Find Next In Page fails to show correct results that corresponding to the current DOM. For context, some details how Find in Page works: 1.When the user initializes new search RenderViewImpl::OnFind is called, in order not to block UI it quickly returns the first result and schedules new full search task (it's called scoping request in code). 2.The full search task runs and adds all occurrences of the searched text as markers in DocumentMarkerController. These markers are presented to the user as search results. 3.When the user starts FindNext in browser, again RenderViewImpl::OnFind is called, it runs search from the place of previous search result and in TextFinder it's called DocumentMarkerController::setMarkersActive to set the current result. No new markers to DocumentMarkerController are added. Current FindNext implementation (TextFinder) uses current DOM and gives correct results, the problem is that DocumentMarkerController::setMarkersActive silently fails to add marker when it receives a search result in content that was added after full search in 2. The proposed solution in this patch: 1.When DocumentMarkerController::setMarkersActive fails to find marker that corresponds to a current FindNext result (that means that it's new part of DOM, that was loaded after the initial full search), it adds this result range as active marker and returns false to TextFinder. 2.TextFinder receives information that the new text found, it stores restore point to the following full search (so search will continue from the same place). And returns to RenderViewImpl activeNow=true. 3.RenderViewImpl receives that activeNow=true restarts full search (in a separate task, the same as with original search). This patch doesn't solve all problems, namely 1.In order to see new results the user should use FindNext till new results are found. 2.If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController), so the user will see incorrect number of results, but the user will still see correct highlighted search results in a page (the same behaviour as now). but it makes problems much less severe. BUG=2220 ========== to ========== Restart search in page when new text is found. This patch is a partial solution to the problem, when DOM was changed and Find Next In Page fails to show correct results that corresponding to the current DOM. For context, some details how Find in Page works: 1.When the user initializes new search RenderViewImpl::OnFind is called, in order not to block UI it quickly returns the first result and schedules new full search task (it's called scoping request in code). 2.The full search task runs and adds all occurrences of the searched text as markers in DocumentMarkerController. These markers are presented to the user as search results. 3.When the user starts FindNext in browser, again RenderViewImpl::OnFind is called, it runs search from the place of previous search result and in TextFinder it's called DocumentMarkerController::setMarkersActive to set the current result. No new markers to DocumentMarkerController are added. Current FindNext implementation (TextFinder) uses current DOM and gives correct results, the problem is that DocumentMarkerController::setMarkersActive silently fails to add marker when it receives a search result in content that was added after full search in 2. The proposed solution in this patch: 1.When DocumentMarkerController::setMarkersActive fails to find marker that corresponds to a current FindNext result (that means that it's new part of DOM, that was loaded after the initial full search), it adds this result range as active marker and returns false to TextFinder. 2.TextFinder receives information that the new text found, it stores restore point to the following full search (so search will continue from the same place). And returns to RenderViewImpl activeNow=false. 3.RenderViewImpl receives that activeNow=false restarts full search (in a separate task, the same as with original search). This patch doesn't solve all problems, namely 1.In order to see new results the user should use FindNext till new results are found. 2.If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController), so the user will see incorrect number of results, but the user will still see correct highlighted search results in a page (the same behaviour as now). but it makes problems much less severe. BUG=2220 ==========
Thanks Finnur for comments! I've addressed them. > You mean: activeNow = false, right? Sure, thanks. https://codereview.chromium.org/1605863002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp (right): https://codereview.chromium.org/1605863002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp:260: RefPtrWillBeRawPtr<Range> range = Range::create(document(), toPositionInDOMTree(ephemeralRange.startPosition()), toPositionInDOMTree(ephemeralRange.endPosition())); On 2016/01/29 17:47:49, Finnur wrote: > nit: Break up into two lines for readability? Done. https://codereview.chromium.org/1605863002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/TextFinder.cpp (right): https://codereview.chromium.org/1605863002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/web/TextFinder.cpp:169: setMarkerActive(m_activeMatch.get(), true); On 2016/01/29 17:47:50, Finnur wrote: > Not as verbose and probably a bit clearer: > > bool isActive = setMarkerActive(m_activeMatch.get(), true); > if (activeNow) > *activeNow = isActive; > > Has benefits below too. Thanks, it makes code clearer. https://codereview.chromium.org/1605863002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/web/TextFinder.cpp:177: if (!options.findNext || activeSelection || (activeNow && !*activeNow)) { On 2016/01/29 17:47:49, Finnur wrote: > Replace > (activeNow && !*activeNow) > with > !isActive Done. https://codereview.chromium.org/1605863002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/web/TextFinder.cpp:179: // due to a selection, or Find-next which found newly added text, so we On 2016/01/29 17:47:50, Finnur wrote: > Suggest: "or new matches were found during Find-next due to DOM alteration (that > couldn't be set as active)" Done. https://codereview.chromium.org/1605863002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/1605863002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:4110: WebScriptSource("document.body.insertBefore(document.createTextNode('bar5 foo5'), document.getElementsByTagName('textarea')[0]);")); On 2016/01/29 17:47:50, Finnur wrote: > Nit: I would probably try to break this up into more lines for readability. Done. https://codereview.chromium.org/1605863002/diff/200001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebFrame.h (left): https://codereview.chromium.org/1605863002/diff/200001/third_party/WebKit/pub... third_party/WebKit/public/web/WebFrame.h:561: const WebFindOptions& options, On 2016/01/29 17:47:50, Finnur wrote: > Nit: Why delete this? Style checker in git cl upload says that a name should be omitted according to style (probably because options is the part of type name WebFindOptions). I've checked other functions in this file and arguments WebURLRequest, WebURLLoaderOptions, WebPoint are without names. So it seems to be ok according to style.
Thanks. Still LGTM.
dvadym@chromium.org changed reviewers: + jochen@chromium.org
Hi Jochen, Could you please review this CL? Regards, Vadym
jochen@chromium.org changed reviewers: + esprehn@chromium.org
esprehn@ is a better reviewer for this
esprehn@ Could you please review this CL?
Description was changed from ========== Restart search in page when new text is found. This patch is a partial solution to the problem, when DOM was changed and Find Next In Page fails to show correct results that corresponding to the current DOM. For context, some details how Find in Page works: 1.When the user initializes new search RenderViewImpl::OnFind is called, in order not to block UI it quickly returns the first result and schedules new full search task (it's called scoping request in code). 2.The full search task runs and adds all occurrences of the searched text as markers in DocumentMarkerController. These markers are presented to the user as search results. 3.When the user starts FindNext in browser, again RenderViewImpl::OnFind is called, it runs search from the place of previous search result and in TextFinder it's called DocumentMarkerController::setMarkersActive to set the current result. No new markers to DocumentMarkerController are added. Current FindNext implementation (TextFinder) uses current DOM and gives correct results, the problem is that DocumentMarkerController::setMarkersActive silently fails to add marker when it receives a search result in content that was added after full search in 2. The proposed solution in this patch: 1.When DocumentMarkerController::setMarkersActive fails to find marker that corresponds to a current FindNext result (that means that it's new part of DOM, that was loaded after the initial full search), it adds this result range as active marker and returns false to TextFinder. 2.TextFinder receives information that the new text found, it stores restore point to the following full search (so search will continue from the same place). And returns to RenderViewImpl activeNow=false. 3.RenderViewImpl receives that activeNow=false restarts full search (in a separate task, the same as with original search). This patch doesn't solve all problems, namely 1.In order to see new results the user should use FindNext till new results are found. 2.If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController), so the user will see incorrect number of results, but the user will still see correct highlighted search results in a page (the same behaviour as now). but it makes problems much less severe. BUG=2220 ========== to ========== Restart search in page when new text is found. This patch is a partial solution to the problem, when DOM was changed and Find Next In Page fails to show correct results that corresponding to the current DOM. For context, some details how Find in Page works: 1.When the user initializes new search RenderFrameImpl::OnFind is called, in order not to block UI it quickly returns the first result and schedules new full search task (it's called scoping request in code). 2.The full search task runs and adds all occurrences of the searched text as markers in DocumentMarkerController. These markers are presented to the user as search results. 3.When the user starts FindNext in browser, again RenderFrameImpl::OnFind is called, it runs search from the place of previous search result and in TextFinder it's called DocumentMarkerController::setMarkersActive to set the current result. No new markers to DocumentMarkerController are added. Current FindNext implementation (TextFinder) uses current DOM and gives correct results, the problem is that DocumentMarkerController::setMarkersActive silently fails to add marker when it receives a search result in content that was added after full search in 2. The proposed solution in this patch: 1.When DocumentMarkerController::setMarkersActive fails to find marker that corresponds to a current FindNext result (that means that it's new part of DOM, that was loaded after the initial full search), it adds this result range as active marker and returns false to TextFinder. 2.TextFinder receives information that the new text found, it stores restore point to the following full search (so search will continue from the same place). And returns to RenderFrameImpl activeNow=false. 3.RenderFrameImpl receives that activeNow=false restarts full search (in a separate task, the same as with original search). This patch doesn't solve all problems, namely 1.In order to see new results the user should use FindNext till new results are found. 2.If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController), so the user will see incorrect number of results, but the user will still see correct highlighted search results in a page (the same behaviour as now). but it makes problems much less severe. BUG=2220 ==========
Hi esprehn, friendly ping. Could you please have a look at this CL?
Description was changed from ========== Restart search in page when new text is found. This patch is a partial solution to the problem, when DOM was changed and Find Next In Page fails to show correct results that corresponding to the current DOM. For context, some details how Find in Page works: 1.When the user initializes new search RenderFrameImpl::OnFind is called, in order not to block UI it quickly returns the first result and schedules new full search task (it's called scoping request in code). 2.The full search task runs and adds all occurrences of the searched text as markers in DocumentMarkerController. These markers are presented to the user as search results. 3.When the user starts FindNext in browser, again RenderFrameImpl::OnFind is called, it runs search from the place of previous search result and in TextFinder it's called DocumentMarkerController::setMarkersActive to set the current result. No new markers to DocumentMarkerController are added. Current FindNext implementation (TextFinder) uses current DOM and gives correct results, the problem is that DocumentMarkerController::setMarkersActive silently fails to add marker when it receives a search result in content that was added after full search in 2. The proposed solution in this patch: 1.When DocumentMarkerController::setMarkersActive fails to find marker that corresponds to a current FindNext result (that means that it's new part of DOM, that was loaded after the initial full search), it adds this result range as active marker and returns false to TextFinder. 2.TextFinder receives information that the new text found, it stores restore point to the following full search (so search will continue from the same place). And returns to RenderFrameImpl activeNow=false. 3.RenderFrameImpl receives that activeNow=false restarts full search (in a separate task, the same as with original search). This patch doesn't solve all problems, namely 1.In order to see new results the user should use FindNext till new results are found. 2.If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController), so the user will see incorrect number of results, but the user will still see correct highlighted search results in a page (the same behaviour as now). but it makes problems much less severe. BUG=2220 ========== to ========== Restart search in page when new text is found. This patch is a partial solution to the problem, when DOM was changed and Find Next In Page fails to show correct results that corresponding to the current DOM. For context, some details how Find in Page works: 1. When the user initializes new search RenderFrameImpl::OnFind is called, in order not to block UI it quickly returns the first result and schedules new full search task (it's called scoping request in code). 2. The full search task runs and adds all occurrences of the searched text as markers in DocumentMarkerController. These markers are presented to the user as search results. 3.When the user starts FindNext in browser, again RenderFrameImpl::OnFind is called, it runs search from the place of previous search result and in TextFinder it's called DocumentMarkerController::setMarkersActive to set the current result. No new markers to DocumentMarkerController are added. Current FindNext implementation (TextFinder) uses current DOM and gives correct results, the problem is that DocumentMarkerController::setMarkersActive silently fails to add marker when it receives a search result in content that was added after full search in 2. The proposed solution in this patch: 1. When DocumentMarkerController::setMarkersActive fails to find marker that corresponds to a current FindNext result (that means that it's new part of DOM, that was loaded after the initial full search), it adds this result range as active marker and returns false to TextFinder. 2. TextFinder receives information that the new text found, it stores restore point to the following full search (so search will continue from the same place). And returns to RenderFrameImpl activeNow=false. 3. RenderFrameImpl receives that activeNow=false restarts full search (in a separate task, the same as with original search). This patch doesn't solve all problems, namely 1. In order to see new results the user should use FindNext till new results are found. 2. If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController), so the user will see incorrect number of results, but the user will still see correct highlighted search results in a page (the same behaviour as now). but it makes problems much less severe. BUG=2220 ==========
Description was changed from ========== Restart search in page when new text is found. This patch is a partial solution to the problem, when DOM was changed and Find Next In Page fails to show correct results that corresponding to the current DOM. For context, some details how Find in Page works: 1. When the user initializes new search RenderFrameImpl::OnFind is called, in order not to block UI it quickly returns the first result and schedules new full search task (it's called scoping request in code). 2. The full search task runs and adds all occurrences of the searched text as markers in DocumentMarkerController. These markers are presented to the user as search results. 3.When the user starts FindNext in browser, again RenderFrameImpl::OnFind is called, it runs search from the place of previous search result and in TextFinder it's called DocumentMarkerController::setMarkersActive to set the current result. No new markers to DocumentMarkerController are added. Current FindNext implementation (TextFinder) uses current DOM and gives correct results, the problem is that DocumentMarkerController::setMarkersActive silently fails to add marker when it receives a search result in content that was added after full search in 2. The proposed solution in this patch: 1. When DocumentMarkerController::setMarkersActive fails to find marker that corresponds to a current FindNext result (that means that it's new part of DOM, that was loaded after the initial full search), it adds this result range as active marker and returns false to TextFinder. 2. TextFinder receives information that the new text found, it stores restore point to the following full search (so search will continue from the same place). And returns to RenderFrameImpl activeNow=false. 3. RenderFrameImpl receives that activeNow=false restarts full search (in a separate task, the same as with original search). This patch doesn't solve all problems, namely 1. In order to see new results the user should use FindNext till new results are found. 2. If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController), so the user will see incorrect number of results, but the user will still see correct highlighted search results in a page (the same behaviour as now). but it makes problems much less severe. BUG=2220 ========== to ========== Restart search in page when new text is found. This patch is a partial solution to the problem, when DOM was changed and Find Next In Page fails to show correct results that corresponding to the current DOM. For context, some details how Find in Page works: 1. When the user initializes new search RenderFrameImpl::OnFind is called, in order not to block UI it quickly returns the first result and schedules new full search task (it's called scoping request in code). 2. The full search task runs and adds all occurrences of the searched text as markers in DocumentMarkerController. These markers are presented to the user as search results. 3. When the user starts FindNext in browser, again RenderFrameImpl::OnFind is called, it runs search from the place of previous search result and in TextFinder it's called DocumentMarkerController::setMarkersActive to set the current result. No new markers to DocumentMarkerController are added. Current FindNext implementation (TextFinder) uses current DOM and gives correct results, the problem is that DocumentMarkerController::setMarkersActive silently fails to add marker when it receives a search result in content that was added after full search in 2. The proposed solution in this patch: 1. When DocumentMarkerController::setMarkersActive fails to find marker that corresponds to a current FindNext result (that means that it's new part of DOM, that was loaded after the initial full search), it adds this result range as active marker and returns false to TextFinder. 2. TextFinder receives information that the new text found, it stores restore point to the following full search (so search will continue from the same place). And returns to RenderFrameImpl activeNow=false. 3. RenderFrameImpl receives that activeNow=false restarts full search (in a separate task, the same as with original search). This patch doesn't solve all problems, namely 1. In order to see new results the user should use FindNext till new results are found. 2. If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController), so the user will see incorrect number of results, but the user will still see correct highlighted search results in a page (the same behaviour as now). but it makes problems much less severe. BUG=2220 ==========
I don't think this is a good solution, you should use the dom tree version to detect dom mutation and invalidate the found list of results. Any time the version doesn't match you should then scan for the first new result from the current position, and then schedule a task to find all the positions again. I don't think we need all this plumbing back out of the system. https://codereview.chromium.org/1605863002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): https://codereview.chromium.org/1605863002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:83: bool setMarkersActive(Range*, bool); this needs a comment, what does the bool return mean? https://codereview.chromium.org/1605863002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/TextFinder.h (right): https://codereview.chromium.org/1605863002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/TextFinder.h:59: bool wrapWithinFrame, WebRect* selectionRect, bool* activeNow); = nullptr and then don't change the tons of callsites to include nullptr https://codereview.chromium.org/1605863002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/TextFinder.h:160: bool setMarkerActive(Range*, bool active); what does the return mean?
Thanks for comments esprehn! PTAL Yeah I agree that listening DOM changes are ultimate solution to the problem in search-in-page with changing DOM, but 1.It will require overhead for listening and re-searching on DOM changes, even if the user doesn't need this search results. 2.It will require much more complex implementation that the current patch (including some optimizations to do it smartly in order not to load CPU significantly). On other hand using information from DocumentMarkerController is basically for free, we anyway know from it when we find something new. Now we simply don't take it into consideration. And this implementation is very simple and non-risky. I took this as 20% project. I don't have much time to make more significant changes. I just want to fix the most annoying part of this bug. Listening changes in DOM it's basically the first approach from bug discussion https://code.google.com/p/chromium/issues/detail?id=2220#c6, this patch is the second approach. About other your comments (about scanning and new result from the current position). It already work in this patch. We have mechanism for restarting search, it's used when the user select some region, then we start new search on a whole page, and showing active match starting from the selection. This mechanism is reused here, namely by checking activeNow in a line if (options.findNext && current_selection.isNull() && active_now) in render_frame_impl.cc, search restarting is triggered and in a line if (!options.findNext || activeSelection || !isActive) in TextFinder.cc it's triggered that active match is calculated from the current position. Regards, Vadym https://codereview.chromium.org/1605863002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): https://codereview.chromium.org/1605863002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:83: bool setMarkersActive(Range*, bool); On 2016/02/10 01:17:08, esprehn wrote: > this needs a comment, what does the bool return mean? Done. https://codereview.chromium.org/1605863002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/TextFinder.h (right): https://codereview.chromium.org/1605863002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/TextFinder.h:59: bool wrapWithinFrame, WebRect* selectionRect, bool* activeNow); On 2016/02/10 01:17:08, esprehn wrote: > = nullptr > > and then don't change the tons of callsites to include nullptr Done. https://codereview.chromium.org/1605863002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/TextFinder.h:160: bool setMarkerActive(Range*, bool active); On 2016/02/10 01:17:08, esprehn wrote: > what does the return mean? I've added a comment: Returns true if at least one such marker found.
Perfect is the enemy of good. I'm supportive of this patch because it solves an annoying problem we have today in a way that is consistent with the current Find in Page mechanism. I particularly like that the majority of this patch (in lines of code) is new/augmented tests and the actual changes it makes to Chromium are very limited. It would be great if someone were willing to spend the effort to solve this using DOM mutation events but I don't see that person stepping up. I also don't see this patch adding considerable work for a person that wants to step up. In this case, therefore, as someone said: I'd rather ride a good horse than keep waiting for a unicorn. :)
creis@chromium.org changed reviewers: + paulmeyer@chromium.org
On 2016/02/11 10:08:01, Finnur wrote: > Perfect is the enemy of good. > > I'm supportive of this patch because it solves an annoying problem we have today > in a way that is consistent with the current Find in Page mechanism. I > particularly like that the majority of this patch (in lines of code) is > new/augmented tests and the actual changes it makes to Chromium are very > limited. > > It would be great if someone were willing to spend the effort to solve this > using DOM mutation events but I don't see that person stepping up. I also don't > see this patch adding considerable work for a person that wants to step up. In > this case, therefore, as someone said: I'd rather ride a good horse than keep > waiting for a unicorn. :) Adding Paul Meyer, who's currently refactoring Find-in-Page for OOPIF support (https://crbug.com/457440) and fixing some related issues along the way. Not sure if that counts as a unicorn, but he might be interested in this CL. :)
On 2016/02/11 19:44:31, Charlie Reis wrote: > On 2016/02/11 10:08:01, Finnur wrote: > > Perfect is the enemy of good. > > > > I'm supportive of this patch because it solves an annoying problem we have > today > > in a way that is consistent with the current Find in Page mechanism. I > > particularly like that the majority of this patch (in lines of code) is > > new/augmented tests and the actual changes it makes to Chromium are very > > limited. > > > > It would be great if someone were willing to spend the effort to solve this > > using DOM mutation events but I don't see that person stepping up. I also > don't > > see this patch adding considerable work for a person that wants to step up. In > > this case, therefore, as someone said: I'd rather ride a good horse than keep > > waiting for a unicorn. :) > > Adding Paul Meyer, who's currently refactoring Find-in-Page for OOPIF support > (https://crbug.com/457440) and fixing some related issues along the way. Not > sure if that counts as a unicorn, but he might be interested in this CL. :) Thanks Charlie for adding me. I had no idea this was being worked on, but I commend you for trying to combat a 4-digit bug. What an ancient monster to slay! Regarding my work... the initial implementation won't directly affect (or correct) this specific problem, though I had been thinking about it. I'm glad I know about this work now though, because I will definitely have to work it into my multi-process implementation if it lands. If you want another opinion about whether or not go ahead with this, I personally feel like this patch is a step in the right direction. Even if it isn’t perfect, it seems to do enough good with little enough harm to be worthwhile overall.
https://codereview.chromium.org/1605863002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/TextFinder.cpp (right): https://codereview.chromium.org/1605863002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/web/TextFinder.cpp:166: bool isActive = setMarkerActive(m_activeMatch.get(), true); nit: Please move this block down to right above the if statement below, so that the declaration of |isActive| is closer to its use. https://codereview.chromium.org/1605863002/diff/320001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebLocalFrame.h (right): https://codereview.chromium.org/1605863002/diff/320001/third_party/WebKit/pub... third_party/WebKit/public/web/WebLocalFrame.h:239: bool* activeNow) = 0; nit: Similar to what you did with TestFinder::find(), could you overload this function so that you do not have to add nullptr to all of the callsites that don't need |activeNow|?
Thanks Paul for comments! I've addressed them. https://codereview.chromium.org/1605863002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/TextFinder.cpp (right): https://codereview.chromium.org/1605863002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/web/TextFinder.cpp:166: bool isActive = setMarkerActive(m_activeMatch.get(), true); On 2016/02/12 13:58:49, Paul Meyer wrote: > nit: Please move this block down to right above the if statement below, so that > the declaration of |isActive| is closer to its use. Done. https://codereview.chromium.org/1605863002/diff/320001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebLocalFrame.h (right): https://codereview.chromium.org/1605863002/diff/320001/third_party/WebKit/pub... third_party/WebKit/public/web/WebLocalFrame.h:239: bool* activeNow) = 0; On 2016/02/12 13:58:49, Paul Meyer wrote: > nit: Similar to what you did with TestFinder::find(), could you overload this > function so that you do not have to add nullptr to all of the callsites that > don't need |activeNow|? Sure, Done.
Hi esprehn, friendly ping. Could you please have another look at this CL?
Should active_now be called markers_found? I think this feels like technical debt, and we should really just fix the way find in page works at a more fundamental level. It's sad that find in page doesn't have someone who owns it and makes it more awesome every release. :) lgtm
FWIW, I agree on technical debt and this needing proper ownership. I won't stand in the way of this patch, but we all need to realize that this is just a bandaid on the already pretty broken code.
On 2016/02/18 19:16:03, esprehn wrote: > Should active_now be called markers_found? > > I think this feels like technical debt, and we should really just fix the way > find in page works at a more fundamental level. It's sad that find in page > doesn't have someone who owns it and makes it more awesome every release. :) > > lgtm Thanks esprehn for review! In the first version active_now was called new_text_found (very similar to markers_found, but with negate meaning). Then in comment #13 Finnur said > Also, |newTextFound| is a bit ambiguous in the context of find in page. I think > I would prefer something like |nowActive|. Now I also think that a name like active_now is better, since it reveals less details of inside implementation of search classes (namely about markers). markers_found is a little bit ambiguous which exactly marker are found. If you think that markers_found is better, no problem I can change.
The CQ bit was checked by dvadym@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org, esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/1605863002/#ps380001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1605863002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1605863002/380001
content lgtm with comment https://codereview.chromium.org/1605863002/diff/380001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1605863002/diff/380001/content/renderer/rende... content/renderer/render_frame_impl.cc:5062: bool active_now; please add a default value
The CQ bit was unchecked by dvadym@chromium.org
Thanks Jochen for comments! https://codereview.chromium.org/1605863002/diff/380001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1605863002/diff/380001/content/renderer/rende... content/renderer/render_frame_impl.cc:5062: bool active_now; On 2016/02/23 14:34:26, jochen wrote: > please add a default value Done.
The CQ bit was checked by dvadym@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org, esprehn@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/1605863002/#ps400001 (title: "Added default value for local var")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1605863002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1605863002/400001
Message was sent while issue was closed.
Committed patchset #21 (id:400001)
Message was sent while issue was closed.
Description was changed from ========== Restart search in page when new text is found. This patch is a partial solution to the problem, when DOM was changed and Find Next In Page fails to show correct results that corresponding to the current DOM. For context, some details how Find in Page works: 1. When the user initializes new search RenderFrameImpl::OnFind is called, in order not to block UI it quickly returns the first result and schedules new full search task (it's called scoping request in code). 2. The full search task runs and adds all occurrences of the searched text as markers in DocumentMarkerController. These markers are presented to the user as search results. 3. When the user starts FindNext in browser, again RenderFrameImpl::OnFind is called, it runs search from the place of previous search result and in TextFinder it's called DocumentMarkerController::setMarkersActive to set the current result. No new markers to DocumentMarkerController are added. Current FindNext implementation (TextFinder) uses current DOM and gives correct results, the problem is that DocumentMarkerController::setMarkersActive silently fails to add marker when it receives a search result in content that was added after full search in 2. The proposed solution in this patch: 1. When DocumentMarkerController::setMarkersActive fails to find marker that corresponds to a current FindNext result (that means that it's new part of DOM, that was loaded after the initial full search), it adds this result range as active marker and returns false to TextFinder. 2. TextFinder receives information that the new text found, it stores restore point to the following full search (so search will continue from the same place). And returns to RenderFrameImpl activeNow=false. 3. RenderFrameImpl receives that activeNow=false restarts full search (in a separate task, the same as with original search). This patch doesn't solve all problems, namely 1. In order to see new results the user should use FindNext till new results are found. 2. If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController), so the user will see incorrect number of results, but the user will still see correct highlighted search results in a page (the same behaviour as now). but it makes problems much less severe. BUG=2220 ========== to ========== Restart search in page when new text is found. This patch is a partial solution to the problem, when DOM was changed and Find Next In Page fails to show correct results that corresponding to the current DOM. For context, some details how Find in Page works: 1. When the user initializes new search RenderFrameImpl::OnFind is called, in order not to block UI it quickly returns the first result and schedules new full search task (it's called scoping request in code). 2. The full search task runs and adds all occurrences of the searched text as markers in DocumentMarkerController. These markers are presented to the user as search results. 3. When the user starts FindNext in browser, again RenderFrameImpl::OnFind is called, it runs search from the place of previous search result and in TextFinder it's called DocumentMarkerController::setMarkersActive to set the current result. No new markers to DocumentMarkerController are added. Current FindNext implementation (TextFinder) uses current DOM and gives correct results, the problem is that DocumentMarkerController::setMarkersActive silently fails to add marker when it receives a search result in content that was added after full search in 2. The proposed solution in this patch: 1. When DocumentMarkerController::setMarkersActive fails to find marker that corresponds to a current FindNext result (that means that it's new part of DOM, that was loaded after the initial full search), it adds this result range as active marker and returns false to TextFinder. 2. TextFinder receives information that the new text found, it stores restore point to the following full search (so search will continue from the same place). And returns to RenderFrameImpl activeNow=false. 3. RenderFrameImpl receives that activeNow=false restarts full search (in a separate task, the same as with original search). This patch doesn't solve all problems, namely 1. In order to see new results the user should use FindNext till new results are found. 2. If something is removed from DOM, new search is not restarted (it's hard to find out about this from DocumentMarkerController), so the user will see incorrect number of results, but the user will still see correct highlighted search results in a page (the same behaviour as now). but it makes problems much less severe. BUG=2220 Committed: https://crrev.com/22cbbba006181412d51841e6bf3a7b53d9b653df Cr-Commit-Position: refs/heads/master@{#376994} ==========
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/22cbbba006181412d51841e6bf3a7b53d9b653df Cr-Commit-Position: refs/heads/master@{#376994} |
