Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1959183002:
Multi-Process Find-in-Page. (Closed)
|
Created:
4 years, 7 months ago by paulmeyer Modified:
4 years, 6 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dglazkov+blink, dglazkov, Fady Samuel, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMulti-Process Find-in-Page.
This patch completes the implementation of multi-process find-in-page, as described in the design doc: https://docs.google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zhodPxMOxa8Bg4Wg0/view
BUG=457440
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/c8cb7cbda8636fe756c84b05be2afa70cd9cd3f5
Cr-Commit-Position: refs/heads/master@{#398186}
Patch Set 1 #
Total comments: 36
Patch Set 2 : Rebased and addressed comments. #Patch Set 3 : Rebased and made WebLocalFrame::isFocused() private. #
Total comments: 6
Patch Set 4 : Addressed comments by dcheng@. #
Total comments: 43
Patch Set 5 : Addressed comments by nick@. #
Total comments: 4
Patch Set 6 : Addressed comments by nick@, and a bit of cleanup. #
Total comments: 10
Patch Set 7 : Minor fixes. #Patch Set 8 : Rebased. #Patch Set 9 : Disabled tests on Android Release because of crbug.com/615291. #Messages
Total messages: 63 (25 generated)
Description was changed from ========== Multi-process Find-in-Page. ========== to ========== Multi-process Find-in-Page. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== Multi-process Find-in-Page. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Multi-Process Find-in-Page. This patch completes the implementation of multi-process find-in-page, as described in the following design doc: https://docs.google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zhodPxMOxa8Bg4W... This patch relies on another issue: https://codereview.chromium.org/1500973002/ CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Multi-Process Find-in-Page. This patch completes the implementation of multi-process find-in-page, as described in the following design doc: https://docs.google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zhodPxMOxa8Bg4W... This patch relies on another issue: https://codereview.chromium.org/1500973002/ CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Multi-Process Find-in-Page. This patch completes the implementation of multi-process find-in-page, as described in the design doc: https://docs.google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zhodPxMOxa8Bg4W... This patch relies on another issue: https://codereview.chromium.org/1500973002/ CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Multi-Process Find-in-Page. This patch completes the implementation of multi-process find-in-page, as described in the design doc: https://docs.google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zhodPxMOxa8Bg4W... This patch relies on another issue: https://codereview.chromium.org/1500973002/ CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Multi-Process Find-in-Page. This patch completes the implementation of multi-process find-in-page, as described in the design doc: https://docs.google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zhodPxMOxa8Bg4W... This patch relies on another issue: https://codereview.chromium.org/1500973002/ BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
paulmeyer@chromium.org changed reviewers: + lfg@chromium.org
+lfg@ for initial review.
Just a few nits and question, but mostly LG. I think you can send the patch to owners review. nick@ has volunteered to review the content/ side. https://codereview.chromium.org/1959183002/diff/20001/content/browser/find_re... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/1959183002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:33: RenderFrameHost* GetDeepestLastChild(RenderFrameHost* rfh) { nit: newline https://codereview.chromium.org/1959183002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:136: DCHECK(CheckFrame(rfh)); Should we also check the rfh is in pending_replies_? https://codereview.chromium.org/1959183002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:149: if (!selection_rect.IsEmpty()) { nit: no need for {} https://codereview.chromium.org/1959183002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:206: FinalUpdate(current_session_id_, GetRenderFrameHost(frame_key)); nit: {} https://codereview.chromium.org/1959183002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:356: contents_->ForEachFrame(base::Bind(&FindRequestManager::AddFrame, What happens if a frame is created after a find operation has already started? That frame will never be searched, unless we start a new search? https://codereview.chromium.org/1959183002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:422: return nullptr; Not sure if I follow, shouldn't we return null only when wrap == false? https://codereview.chromium.org/1959183002/diff/20001/content/browser/find_re... File content/browser/find_request_manager.h (right): https://codereview.chromium.org/1959183002/diff/20001/content/browser/find_re... content/browser/find_request_manager.h:216: // compared to |fmr_version_| after polling frames for updates to their Did you mean known_version instead of fmr_version_? https://codereview.chromium.org/1959183002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/1959183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:3802: Any reason we can't assert that the rects are in still order in the adapted test? https://codereview.chromium.org/1959183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:3856: Can we remove find_in_hidden_frame.html from Source/web/tests/data ?
Drive-by. https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:5040: if (frame_->isFocused() || options.findNext) { I think we can just use WebView::focusedFrame() == frame_.
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/1959183002/diff/20001/content/browser/find_re... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/1959183002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:33: RenderFrameHost* GetDeepestLastChild(RenderFrameHost* rfh) { On 2016/05/13 20:15:40, lfg wrote: > nit: newline I actually put them together on purpose since they're overloaded and the comment applies to both. I've seen this done in other places. I'm not sure if there is a specific style violation here, but I couldn't find one. https://codereview.chromium.org/1959183002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:136: DCHECK(CheckFrame(rfh)); On 2016/05/13 20:15:40, lfg wrote: > Should we also check the rfh is in pending_replies_? In this case no, since it is sometimes possible for a frame to report updated information when the browser wasn't explicitly expecting it. We still want to use this new information as long is it is relevant to the current find session. https://codereview.chromium.org/1959183002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:149: if (!selection_rect.IsEmpty()) { On 2016/05/13 20:15:40, lfg wrote: > nit: no need for {} Done. https://codereview.chromium.org/1959183002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:206: FinalUpdate(current_session_id_, GetRenderFrameHost(frame_key)); On 2016/05/13 20:15:40, lfg wrote: > nit: {} Done. https://codereview.chromium.org/1959183002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:356: contents_->ForEachFrame(base::Bind(&FindRequestManager::AddFrame, On 2016/05/13 20:15:40, lfg wrote: > What happens if a frame is created after a find operation has already started? > That frame will never be searched, unless we start a new search? That is correct, and is the same behavior that find-in-page has now. I think it would be nice to add that functionality, but it would be something extra to appear in a different CL. https://codereview.chromium.org/1959183002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:422: return nullptr; On 2016/05/13 20:15:40, lfg wrote: > Not sure if I follow, shouldn't we return null only when wrap == false? This is just checking for the case where we're wrapping and we've traversed every node and gotten back to the one we started with, which essentially means that no valid node was found, so we return nullptr. https://codereview.chromium.org/1959183002/diff/20001/content/browser/find_re... File content/browser/find_request_manager.h (right): https://codereview.chromium.org/1959183002/diff/20001/content/browser/find_re... content/browser/find_request_manager.h:216: // compared to |fmr_version_| after polling frames for updates to their On 2016/05/13 20:15:40, lfg wrote: > Did you mean known_version instead of fmr_version_? Yes I did. There was something called fmr_version_ at some point (not anymore), and I missed changing this reference. https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:5040: if (frame_->isFocused() || options.findNext) { On 2016/05/14 00:03:44, dcheng wrote: > I think we can just use WebView::focusedFrame() == frame_. Actually, that wouldn't work. I tried that at some point, and it led to confusing bugs. It turns out that WebView::focusedFrame() is not named very appropriately, since it sometimes returns the focused frame, and sometimes returns other frames (this isn't a bug though, that's what it's supposed to do, it's just a really misleading name). So, that's why I created the isFocused() method to properly determine whether the frame has focus. This main difference is that isFocused() hooks into FocusController::focusedFrame() instead of FocusController::focusedOrMainFrame(). https://codereview.chromium.org/1959183002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/1959183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:3802: On 2016/05/13 20:15:40, lfg wrote: > Any reason we can't assert that the rects are in still order in the adapted > test? We could still assert the positions of the rects in the one frame, but I don't think that's necessary to do here anymore, since all of the assertions are checked in the new browser test now. https://codereview.chromium.org/1959183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:3856: On 2016/05/13 20:15:40, lfg wrote: > Can we remove find_in_hidden_frame.html from Source/web/tests/data ? Yes. Done.
paulmeyer@chromium.org changed reviewers: + dglazkov@chromium.org
+dglazkov@ for blink changes.
lgtm https://codereview.chromium.org/1959183002/diff/20001/content/browser/find_re... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/1959183002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:33: RenderFrameHost* GetDeepestLastChild(RenderFrameHost* rfh) { On 2016/05/16 15:25:11, paulmeyer wrote: > On 2016/05/13 20:15:40, lfg wrote: > > nit: newline > > I actually put them together on purpose since they're overloaded and the comment > applies to both. I've seen this done in other places. I'm not sure if there is a > specific style violation here, but I couldn't find one. Acknowledged. https://codereview.chromium.org/1959183002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:356: contents_->ForEachFrame(base::Bind(&FindRequestManager::AddFrame, On 2016/05/16 15:25:11, paulmeyer wrote: > On 2016/05/13 20:15:40, lfg wrote: > > What happens if a frame is created after a find operation has already started? > > That frame will never be searched, unless we start a new search? > > That is correct, and is the same behavior that find-in-page has now. I think it > would be nice to add that functionality, but it would be something extra to > appear in a different CL. Acknowledged. https://codereview.chromium.org/1959183002/diff/20001/content/browser/find_re... content/browser/find_request_manager.cc:422: return nullptr; On 2016/05/16 15:25:11, paulmeyer wrote: > On 2016/05/13 20:15:40, lfg wrote: > > Not sure if I follow, shouldn't we return null only when wrap == false? > > This is just checking for the case where we're wrapping and we've traversed > every node and gotten back to the one we started with, which essentially means > that no valid node was found, so we return nullptr. Acknowledged. https://codereview.chromium.org/1959183002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/1959183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:3802: On 2016/05/16 15:25:11, paulmeyer wrote: > On 2016/05/13 20:15:40, lfg wrote: > > Any reason we can't assert that the rects are in still order in the adapted > > test? > > We could still assert the positions of the rects in the one frame, but I don't > think that's necessary to do here anymore, since all of the assertions are > checked in the new browser test now. Acknowledged.
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:5040: if (frame_->isFocused() || options.findNext) { On 2016/05/16 at 15:25:11, paulmeyer wrote: > On 2016/05/14 00:03:44, dcheng wrote: > > I think we can just use WebView::focusedFrame() == frame_. > > Actually, that wouldn't work. I tried that at some point, and it led to confusing bugs. It turns out that WebView::focusedFrame() is not named very appropriately, since it sometimes returns the focused frame, and sometimes returns other frames (this isn't a bug though, that's what it's supposed to do, it's just a really misleading name). > > So, that's why I created the isFocused() method to properly determine whether the frame has focus. This main difference is that isFocused() hooks into FocusController::focusedFrame() instead of FocusController::focusedOrMainFrame(). It's confusing to have isFocused() and focusedFrame() not agree though =) For example, in this particular block, it seems like Blink should be able to do the check itself, so this can just be simplified to: if (options.findNext) { bool result = frame_->find(...); if (result) { SendFindReply(...); } }
Patchset #2 (id:60001) has been deleted
https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:5040: if (frame_->isFocused() || options.findNext) { On 2016/05/16 17:11:02, dcheng wrote: > On 2016/05/16 at 15:25:11, paulmeyer wrote: > > On 2016/05/14 00:03:44, dcheng wrote: > > > I think we can just use WebView::focusedFrame() == frame_. > > > > Actually, that wouldn't work. I tried that at some point, and it led to > confusing bugs. It turns out that WebView::focusedFrame() is not named very > appropriately, since it sometimes returns the focused frame, and sometimes > returns other frames (this isn't a bug though, that's what it's supposed to do, > it's just a really misleading name). > > > > So, that's why I created the isFocused() method to properly determine whether > the frame has focus. This main difference is that isFocused() hooks into > FocusController::focusedFrame() instead of > FocusController::focusedOrMainFrame(). > > It's confusing to have isFocused() and focusedFrame() not agree though =) > > For example, in this particular block, it seems like Blink should be able to do > the check itself, so this can just be simplified to: > > if (options.findNext) { > bool result = frame_->find(...); > if (result) { > SendFindReply(...); > } > } Blink could do the check, but what you put here is not equivalent. If the frame is focused, it should be searched regardless of options.findNext. I also feel that it is clearer what's going on here if we check for focus here. It's easy to see here exactly under which conditions the search is performed.
https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:5040: if (frame_->isFocused() || options.findNext) { On 2016/05/16 at 17:21:10, paulmeyer wrote: > On 2016/05/16 17:11:02, dcheng wrote: > > On 2016/05/16 at 15:25:11, paulmeyer wrote: > > > On 2016/05/14 00:03:44, dcheng wrote: > > > > I think we can just use WebView::focusedFrame() == frame_. > > > > > > Actually, that wouldn't work. I tried that at some point, and it led to > > confusing bugs. It turns out that WebView::focusedFrame() is not named very > > appropriately, since it sometimes returns the focused frame, and sometimes > > returns other frames (this isn't a bug though, that's what it's supposed to do, > > it's just a really misleading name). > > > > > > So, that's why I created the isFocused() method to properly determine whether > > the frame has focus. This main difference is that isFocused() hooks into > > FocusController::focusedFrame() instead of > > FocusController::focusedOrMainFrame(). > > > > It's confusing to have isFocused() and focusedFrame() not agree though =) > > > > For example, in this particular block, it seems like Blink should be able to do > > the check itself, so this can just be simplified to: > > > > if (options.findNext) { > > bool result = frame_->find(...); > > if (result) { > > SendFindReply(...); > > } > > } > > Blink could do the check, but what you put here is not equivalent. If the frame is focused, it should be searched regardless of options.findNext. I also feel that it is clearer what's going on here if we check for focus here. It's easy to see here exactly under which conditions the search is performed. OK, in that case, we don't need the conditionals at all in //content. Just have Blink do all this work: the remainder of this function is just using the Blink public API anyway. Moving this all into Blink will let us reduce the size of the Blink API and let us avoid having multiple ways to get a focused frame. So the rest of this function will just look like: if (frame_->find(...)) SendFindReply(...); return;
https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:5040: if (frame_->isFocused() || options.findNext) { On 2016/05/16 17:31:37, dcheng wrote: > On 2016/05/16 at 17:21:10, paulmeyer wrote: > > On 2016/05/16 17:11:02, dcheng wrote: > > > On 2016/05/16 at 15:25:11, paulmeyer wrote: > > > > On 2016/05/14 00:03:44, dcheng wrote: > > > > > I think we can just use WebView::focusedFrame() == frame_. > > > > > > > > Actually, that wouldn't work. I tried that at some point, and it led to > > > confusing bugs. It turns out that WebView::focusedFrame() is not named very > > > appropriately, since it sometimes returns the focused frame, and sometimes > > > returns other frames (this isn't a bug though, that's what it's supposed to > do, > > > it's just a really misleading name). > > > > > > > > So, that's why I created the isFocused() method to properly determine > whether > > > the frame has focus. This main difference is that isFocused() hooks into > > > FocusController::focusedFrame() instead of > > > FocusController::focusedOrMainFrame(). > > > > > > It's confusing to have isFocused() and focusedFrame() not agree though =) > > > > > > For example, in this particular block, it seems like Blink should be able to > do > > > the check itself, so this can just be simplified to: > > > > > > if (options.findNext) { > > > bool result = frame_->find(...); > > > if (result) { > > > SendFindReply(...); > > > } > > > } > > > > Blink could do the check, but what you put here is not equivalent. If the > frame is focused, it should be searched regardless of options.findNext. I also > feel that it is clearer what's going on here if we check for focus here. It's > easy to see here exactly under which conditions the search is performed. > > OK, in that case, we don't need the conditionals at all in //content. Just have > Blink do all this work: the remainder of this function is just using the Blink > public API anyway. > Moving this all into Blink will let us reduce the size of the Blink API and let > us avoid having multiple ways to get a focused frame. So the rest of this > function will just look like: > > if (frame_->find(...)) > SendFindReply(...); > return; I could move all of this to Blink, but it still wouldn't remove the need for IsFocused(). It's called here and in OnStopFinding. I could move both into WebLocalFrame, but it still makes sense to have the method instead of calling into FocusController the long way twice (though I could make it private). Also, that method isn't a second way to get the focused frame. Right now it's the only way to tell if the frame is focused, since WebView::focusedFrame doesn't do that, and I think that's something useful to be able to know. I agree that it's confusing that they don't agree, but that's only because WebView::focusedFrame is confusing. I think the correct way to fix this would be to rename WebView::focusedFrame to focusedOrMainFrame(), like the underlying method in FocusController that it uses (though that name is also misleading, since the method can also return a frame that is neither of those :P). That would probably be a separate CL though.
https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:5040: if (frame_->isFocused() || options.findNext) { On 2016/05/16 at 19:27:49, paulmeyer wrote: > On 2016/05/16 17:31:37, dcheng wrote: > > On 2016/05/16 at 17:21:10, paulmeyer wrote: > > > On 2016/05/16 17:11:02, dcheng wrote: > > > > On 2016/05/16 at 15:25:11, paulmeyer wrote: > > > > > On 2016/05/14 00:03:44, dcheng wrote: > > > > > > I think we can just use WebView::focusedFrame() == frame_. > > > > > > > > > > Actually, that wouldn't work. I tried that at some point, and it led to > > > > confusing bugs. It turns out that WebView::focusedFrame() is not named very > > > > appropriately, since it sometimes returns the focused frame, and sometimes > > > > returns other frames (this isn't a bug though, that's what it's supposed to > > do, > > > > it's just a really misleading name). > > > > > > > > > > So, that's why I created the isFocused() method to properly determine > > whether > > > > the frame has focus. This main difference is that isFocused() hooks into > > > > FocusController::focusedFrame() instead of > > > > FocusController::focusedOrMainFrame(). > > > > > > > > It's confusing to have isFocused() and focusedFrame() not agree though =) > > > > > > > > For example, in this particular block, it seems like Blink should be able to > > do > > > > the check itself, so this can just be simplified to: > > > > > > > > if (options.findNext) { > > > > bool result = frame_->find(...); > > > > if (result) { > > > > SendFindReply(...); > > > > } > > > > } > > > > > > Blink could do the check, but what you put here is not equivalent. If the > > frame is focused, it should be searched regardless of options.findNext. I also > > feel that it is clearer what's going on here if we check for focus here. It's > > easy to see here exactly under which conditions the search is performed. > > > > OK, in that case, we don't need the conditionals at all in //content. Just have > > Blink do all this work: the remainder of this function is just using the Blink > > public API anyway. > > Moving this all into Blink will let us reduce the size of the Blink API and let > > us avoid having multiple ways to get a focused frame. So the rest of this > > function will just look like: > > > > if (frame_->find(...)) > > SendFindReply(...); > > return; > > I could move all of this to Blink, but it still wouldn't remove the need for IsFocused(). It's called here and in OnStopFinding. I don't think it should be needed in OnStopFinding. It doesn't make sense for the browser to trigger a STOP_FIND_ACTION_ACTIVATE_SELECTION action for a non-focused frame. > I could move both into WebLocalFrame, but it still makes sense to have the method instead of calling into FocusController the long way twice (though I could make it private). At least it won't be on the Blink public API. > > Also, that method isn't a second way to get the focused frame. Right now it's the only way to tell if the frame is focused, since WebView::focusedFrame doesn't do that, and I think that's something useful to be able to know. I agree that it's confusing that they don't agree, but that's only because WebView::focusedFrame is confusing. I think the correct way to fix this would be to rename WebView::focusedFrame to focusedOrMainFrame(), like the underlying method in FocusController that it uses (though that name is also misleading, since the method can also return a frame that is neither of those :P). That would probably be a separate CL though. It provides two ways to do the same thing. One way is to just use WebView::focusedFrame(). The other is to loop through all frames looking for the one with isFocused() == true =)
Patchset #3 (id:100001) has been deleted
https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:5040: if (frame_->isFocused() || options.findNext) { On 2016/05/16 20:04:21, dcheng wrote: > On 2016/05/16 at 19:27:49, paulmeyer wrote: > > On 2016/05/16 17:31:37, dcheng wrote: > > > On 2016/05/16 at 17:21:10, paulmeyer wrote: > > > > On 2016/05/16 17:11:02, dcheng wrote: > > > > > On 2016/05/16 at 15:25:11, paulmeyer wrote: > > > > > > On 2016/05/14 00:03:44, dcheng wrote: > > > > > > > I think we can just use WebView::focusedFrame() == frame_. > > > > > > > > > > > > Actually, that wouldn't work. I tried that at some point, and it led > to > > > > > confusing bugs. It turns out that WebView::focusedFrame() is not named > very > > > > > appropriately, since it sometimes returns the focused frame, and > sometimes > > > > > returns other frames (this isn't a bug though, that's what it's supposed > to > > > do, > > > > > it's just a really misleading name). > > > > > > > > > > > > So, that's why I created the isFocused() method to properly determine > > > whether > > > > > the frame has focus. This main difference is that isFocused() hooks into > > > > > FocusController::focusedFrame() instead of > > > > > FocusController::focusedOrMainFrame(). > > > > > > > > > > It's confusing to have isFocused() and focusedFrame() not agree though > =) > > > > > > > > > > For example, in this particular block, it seems like Blink should be > able to > > > do > > > > > the check itself, so this can just be simplified to: > > > > > > > > > > if (options.findNext) { > > > > > bool result = frame_->find(...); > > > > > if (result) { > > > > > SendFindReply(...); > > > > > } > > > > > } > > > > > > > > Blink could do the check, but what you put here is not equivalent. If the > > > frame is focused, it should be searched regardless of options.findNext. I > also > > > feel that it is clearer what's going on here if we check for focus here. > It's > > > easy to see here exactly under which conditions the search is performed. > > > > > > OK, in that case, we don't need the conditionals at all in //content. Just > have > > > Blink do all this work: the remainder of this function is just using the > Blink > > > public API anyway. > > > Moving this all into Blink will let us reduce the size of the Blink API and > let > > > us avoid having multiple ways to get a focused frame. So the rest of this > > > function will just look like: > > > > > > if (frame_->find(...)) > > > SendFindReply(...); > > > return; > > > > I could move all of this to Blink, but it still wouldn't remove the need for > IsFocused(). It's called here and in OnStopFinding. > > I don't think it should be needed in OnStopFinding. It doesn't make sense for > the browser to trigger a STOP_FIND_ACTION_ACTIVATE_SELECTION action for a > non-focused frame. > > > I could move both into WebLocalFrame, but it still makes sense to have the > method instead of calling into FocusController the long way twice (though I > could make it private). > > At least it won't be on the Blink public API. > > > > > Also, that method isn't a second way to get the focused frame. Right now it's > the only way to tell if the frame is focused, since WebView::focusedFrame > doesn't do that, and I think that's something useful to be able to know. I agree > that it's confusing that they don't agree, but that's only because > WebView::focusedFrame is confusing. I think the correct way to fix this would be > to rename WebView::focusedFrame to focusedOrMainFrame(), like the underlying > method in FocusController that it uses (though that name is also misleading, > since the method can also return a frame that is neither of those :P). That > would probably be a separate CL though. > > It provides two ways to do the same thing. One way is to just use > WebView::focusedFrame(). The other is to loop through all frames looking for the > one with isFocused() == true =) Following our offline discussion, I've made WebLocalFrame::isFocused() private so that it is out of the public API. I agree that it doesn't make sense for STOP_FIND_ACTION_ACTIVATE_SELECTION to be used on a non-focused frame, but I think it's still possible for that action to be requested, so I'm going to keep that check. I moved that block into Blink so that RenderFrameImpl doesn't need to use isFocused(). I also moved the part of RenderFrameImpl::OnFind() that used isFocused() into Blink for the same reason, but I didn't move everything after it because that would alter the order that find replies are sent, which I don't want to do. Does that look good to you?
https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:5040: if (frame_->isFocused() || options.findNext) { On 2016/05/17 at 20:40:46, paulmeyer wrote: > On 2016/05/16 20:04:21, dcheng wrote: > > On 2016/05/16 at 19:27:49, paulmeyer wrote: > > > On 2016/05/16 17:31:37, dcheng wrote: > > > > On 2016/05/16 at 17:21:10, paulmeyer wrote: > > > > > On 2016/05/16 17:11:02, dcheng wrote: > > > > > > On 2016/05/16 at 15:25:11, paulmeyer wrote: > > > > > > > On 2016/05/14 00:03:44, dcheng wrote: > > > > > > > > I think we can just use WebView::focusedFrame() == frame_. > > > > > > > > > > > > > > Actually, that wouldn't work. I tried that at some point, and it led > > to > > > > > > confusing bugs. It turns out that WebView::focusedFrame() is not named > > very > > > > > > appropriately, since it sometimes returns the focused frame, and > > sometimes > > > > > > returns other frames (this isn't a bug though, that's what it's supposed > > to > > > > do, > > > > > > it's just a really misleading name). > > > > > > > > > > > > > > So, that's why I created the isFocused() method to properly determine > > > > whether > > > > > > the frame has focus. This main difference is that isFocused() hooks into > > > > > > FocusController::focusedFrame() instead of > > > > > > FocusController::focusedOrMainFrame(). > > > > > > > > > > > > It's confusing to have isFocused() and focusedFrame() not agree though > > =) > > > > > > > > > > > > For example, in this particular block, it seems like Blink should be > > able to > > > > do > > > > > > the check itself, so this can just be simplified to: > > > > > > > > > > > > if (options.findNext) { > > > > > > bool result = frame_->find(...); > > > > > > if (result) { > > > > > > SendFindReply(...); > > > > > > } > > > > > > } > > > > > > > > > > Blink could do the check, but what you put here is not equivalent. If the > > > > frame is focused, it should be searched regardless of options.findNext. I > > also > > > > feel that it is clearer what's going on here if we check for focus here. > > It's > > > > easy to see here exactly under which conditions the search is performed. > > > > > > > > OK, in that case, we don't need the conditionals at all in //content. Just > > have > > > > Blink do all this work: the remainder of this function is just using the > > Blink > > > > public API anyway. > > > > Moving this all into Blink will let us reduce the size of the Blink API and > > let > > > > us avoid having multiple ways to get a focused frame. So the rest of this > > > > function will just look like: > > > > > > > > if (frame_->find(...)) > > > > SendFindReply(...); > > > > return; > > > > > > I could move all of this to Blink, but it still wouldn't remove the need for > > IsFocused(). It's called here and in OnStopFinding. > > > > I don't think it should be needed in OnStopFinding. It doesn't make sense for > > the browser to trigger a STOP_FIND_ACTION_ACTIVATE_SELECTION action for a > > non-focused frame. > > > > > I could move both into WebLocalFrame, but it still makes sense to have the > > method instead of calling into FocusController the long way twice (though I > > could make it private). > > > > At least it won't be on the Blink public API. > > > > > > > > Also, that method isn't a second way to get the focused frame. Right now it's > > the only way to tell if the frame is focused, since WebView::focusedFrame > > doesn't do that, and I think that's something useful to be able to know. I agree > > that it's confusing that they don't agree, but that's only because > > WebView::focusedFrame is confusing. I think the correct way to fix this would be > > to rename WebView::focusedFrame to focusedOrMainFrame(), like the underlying > > method in FocusController that it uses (though that name is also misleading, > > since the method can also return a frame that is neither of those :P). That > > would probably be a separate CL though. > > > > It provides two ways to do the same thing. One way is to just use > > WebView::focusedFrame(). The other is to loop through all frames looking for the > > one with isFocused() == true =) > > Following our offline discussion, I've made WebLocalFrame::isFocused() private so that it is out of the public API. > > I agree that it doesn't make sense for STOP_FIND_ACTION_ACTIVATE_SELECTION to be used on a non-focused frame, but I think it's still possible for that action to be requested, so I'm going to keep that check. I moved that block into Blink so that RenderFrameImpl doesn't need to use isFocused(). Why would it happen? Doesn't that indicate a bug in the browser process code? > > I also moved the part of RenderFrameImpl::OnFind() that used isFocused() into Blink for the same reason, but I didn't move everything after it because that would alter the order that find replies are sent, which I don't want to do. > > Does that look good to you? I don't think I understand: how does it change the order replies are sent? We're in an async message handler here, so it shouldn't matter if we send the reply out in the middle of the ipc handler or at the end: either way, the message is queued to be sent and not necessarily sent immediately.
https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:5040: if (frame_->isFocused() || options.findNext) { On 2016/05/17 21:07:55, dcheng wrote: > On 2016/05/17 at 20:40:46, paulmeyer wrote: > > On 2016/05/16 20:04:21, dcheng wrote: > > > On 2016/05/16 at 19:27:49, paulmeyer wrote: > > > > On 2016/05/16 17:31:37, dcheng wrote: > > > > > On 2016/05/16 at 17:21:10, paulmeyer wrote: > > > > > > On 2016/05/16 17:11:02, dcheng wrote: > > > > > > > On 2016/05/16 at 15:25:11, paulmeyer wrote: > > > > > > > > On 2016/05/14 00:03:44, dcheng wrote: > > > > > > > > > I think we can just use WebView::focusedFrame() == frame_. > > > > > > > > > > > > > > > > Actually, that wouldn't work. I tried that at some point, and it > led > > > to > > > > > > > confusing bugs. It turns out that WebView::focusedFrame() is not > named > > > very > > > > > > > appropriately, since it sometimes returns the focused frame, and > > > sometimes > > > > > > > returns other frames (this isn't a bug though, that's what it's > supposed > > > to > > > > > do, > > > > > > > it's just a really misleading name). > > > > > > > > > > > > > > > > So, that's why I created the isFocused() method to properly > determine > > > > > whether > > > > > > > the frame has focus. This main difference is that isFocused() hooks > into > > > > > > > FocusController::focusedFrame() instead of > > > > > > > FocusController::focusedOrMainFrame(). > > > > > > > > > > > > > > It's confusing to have isFocused() and focusedFrame() not agree > though > > > =) > > > > > > > > > > > > > > For example, in this particular block, it seems like Blink should be > > > able to > > > > > do > > > > > > > the check itself, so this can just be simplified to: > > > > > > > > > > > > > > if (options.findNext) { > > > > > > > bool result = frame_->find(...); > > > > > > > if (result) { > > > > > > > SendFindReply(...); > > > > > > > } > > > > > > > } > > > > > > > > > > > > Blink could do the check, but what you put here is not equivalent. If > the > > > > > frame is focused, it should be searched regardless of options.findNext. > I > > > also > > > > > feel that it is clearer what's going on here if we check for focus here. > > > It's > > > > > easy to see here exactly under which conditions the search is performed. > > > > > > > > > > OK, in that case, we don't need the conditionals at all in //content. > Just > > > have > > > > > Blink do all this work: the remainder of this function is just using the > > > Blink > > > > > public API anyway. > > > > > Moving this all into Blink will let us reduce the size of the Blink API > and > > > let > > > > > us avoid having multiple ways to get a focused frame. So the rest of > this > > > > > function will just look like: > > > > > > > > > > if (frame_->find(...)) > > > > > SendFindReply(...); > > > > > return; > > > > > > > > I could move all of this to Blink, but it still wouldn't remove the need > for > > > IsFocused(). It's called here and in OnStopFinding. > > > > > > I don't think it should be needed in OnStopFinding. It doesn't make sense > for > > > the browser to trigger a STOP_FIND_ACTION_ACTIVATE_SELECTION action for a > > > non-focused frame. > > > > > > > I could move both into WebLocalFrame, but it still makes sense to have the > > > method instead of calling into FocusController the long way twice (though I > > > could make it private). > > > > > > At least it won't be on the Blink public API. > > > > > > > > > > > Also, that method isn't a second way to get the focused frame. Right now > it's > > > the only way to tell if the frame is focused, since WebView::focusedFrame > > > doesn't do that, and I think that's something useful to be able to know. I > agree > > > that it's confusing that they don't agree, but that's only because > > > WebView::focusedFrame is confusing. I think the correct way to fix this > would be > > > to rename WebView::focusedFrame to focusedOrMainFrame(), like the underlying > > > method in FocusController that it uses (though that name is also misleading, > > > since the method can also return a frame that is neither of those :P). That > > > would probably be a separate CL though. > > > > > > It provides two ways to do the same thing. One way is to just use > > > WebView::focusedFrame(). The other is to loop through all frames looking for > the > > > one with isFocused() == true =) > > > > Following our offline discussion, I've made WebLocalFrame::isFocused() private > so that it is out of the public API. > > > > I agree that it doesn't make sense for STOP_FIND_ACTION_ACTIVATE_SELECTION to > be used on a non-focused frame, but I think it's still possible for that action > to be requested, so I'm going to keep that check. I moved that block into Blink > so that RenderFrameImpl doesn't need to use isFocused(). > > Why would it happen? Doesn't that indicate a bug in the browser process code? > No, not a bug. Not sure if there are other examples, but one is that the <webview> tag find API can stop finding whenever it wants, and specify an activate action with it, whether the active match is in a focused frame or not. If it's in a non-focused frame, this condition will stop it. In any case, I think it makes sense to enforce here that only a selection in the focused frame will be activated, rather than rely on behavior elsewhere. > > > > I also moved the part of RenderFrameImpl::OnFind() that used isFocused() into > Blink for the same reason, but I didn't move everything after it because that > would alter the order that find replies are sent, which I don't want to do. > > > > Does that look good to you? > > I don't think I understand: how does it change the order replies are sent? We're > in an async message handler here, so it shouldn't matter if we send the reply > out in the middle of the ipc handler or at the end: either way, the message is > queued to be sent and not necessarily sent immediately. It changes the order because SendFindReply isn't the only place that a reply can be sent from. For example, increaseMatchCount below will result in a reply as well. If we wrap that all into find(), then that reply will be sent before SendFindReply, whereas now it is sent after.
https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:5040: if (frame_->isFocused() || options.findNext) { On 2016/05/17 at 22:14:35, paulmeyer wrote: > On 2016/05/17 21:07:55, dcheng wrote: > > On 2016/05/17 at 20:40:46, paulmeyer wrote: > > > On 2016/05/16 20:04:21, dcheng wrote: > > > > On 2016/05/16 at 19:27:49, paulmeyer wrote: > > > > > On 2016/05/16 17:31:37, dcheng wrote: > > > > > > On 2016/05/16 at 17:21:10, paulmeyer wrote: > > > > > > > On 2016/05/16 17:11:02, dcheng wrote: > > > > > > > > On 2016/05/16 at 15:25:11, paulmeyer wrote: > > > > > > > > > On 2016/05/14 00:03:44, dcheng wrote: > > > > > > > > > > I think we can just use WebView::focusedFrame() == frame_. > > > > > > > > > > > > > > > > > > Actually, that wouldn't work. I tried that at some point, and it > > led > > > > to > > > > > > > > confusing bugs. It turns out that WebView::focusedFrame() is not > > named > > > > very > > > > > > > > appropriately, since it sometimes returns the focused frame, and > > > > sometimes > > > > > > > > returns other frames (this isn't a bug though, that's what it's > > supposed > > > > to > > > > > > do, > > > > > > > > it's just a really misleading name). > > > > > > > > > > > > > > > > > > So, that's why I created the isFocused() method to properly > > determine > > > > > > whether > > > > > > > > the frame has focus. This main difference is that isFocused() hooks > > into > > > > > > > > FocusController::focusedFrame() instead of > > > > > > > > FocusController::focusedOrMainFrame(). > > > > > > > > > > > > > > > > It's confusing to have isFocused() and focusedFrame() not agree > > though > > > > =) > > > > > > > > > > > > > > > > For example, in this particular block, it seems like Blink should be > > > > able to > > > > > > do > > > > > > > > the check itself, so this can just be simplified to: > > > > > > > > > > > > > > > > if (options.findNext) { > > > > > > > > bool result = frame_->find(...); > > > > > > > > if (result) { > > > > > > > > SendFindReply(...); > > > > > > > > } > > > > > > > > } > > > > > > > > > > > > > > Blink could do the check, but what you put here is not equivalent. If > > the > > > > > > frame is focused, it should be searched regardless of options.findNext. > > I > > > > also > > > > > > feel that it is clearer what's going on here if we check for focus here. > > > > It's > > > > > > easy to see here exactly under which conditions the search is performed. > > > > > > > > > > > > OK, in that case, we don't need the conditionals at all in //content. > > Just > > > > have > > > > > > Blink do all this work: the remainder of this function is just using the > > > > Blink > > > > > > public API anyway. > > > > > > Moving this all into Blink will let us reduce the size of the Blink API > > and > > > > let > > > > > > us avoid having multiple ways to get a focused frame. So the rest of > > this > > > > > > function will just look like: > > > > > > > > > > > > if (frame_->find(...)) > > > > > > SendFindReply(...); > > > > > > return; > > > > > > > > > > I could move all of this to Blink, but it still wouldn't remove the need > > for > > > > IsFocused(). It's called here and in OnStopFinding. > > > > > > > > I don't think it should be needed in OnStopFinding. It doesn't make sense > > for > > > > the browser to trigger a STOP_FIND_ACTION_ACTIVATE_SELECTION action for a > > > > non-focused frame. > > > > > > > > > I could move both into WebLocalFrame, but it still makes sense to have the > > > > method instead of calling into FocusController the long way twice (though I > > > > could make it private). > > > > > > > > At least it won't be on the Blink public API. > > > > > > > > > > > > > > Also, that method isn't a second way to get the focused frame. Right now > > it's > > > > the only way to tell if the frame is focused, since WebView::focusedFrame > > > > doesn't do that, and I think that's something useful to be able to know. I > > agree > > > > that it's confusing that they don't agree, but that's only because > > > > WebView::focusedFrame is confusing. I think the correct way to fix this > > would be > > > > to rename WebView::focusedFrame to focusedOrMainFrame(), like the underlying > > > > method in FocusController that it uses (though that name is also misleading, > > > > since the method can also return a frame that is neither of those :P). That > > > > would probably be a separate CL though. > > > > > > > > It provides two ways to do the same thing. One way is to just use > > > > WebView::focusedFrame(). The other is to loop through all frames looking for > > the > > > > one with isFocused() == true =) > > > > > > Following our offline discussion, I've made WebLocalFrame::isFocused() private > > so that it is out of the public API. > > > > > > I agree that it doesn't make sense for STOP_FIND_ACTION_ACTIVATE_SELECTION to > > be used on a non-focused frame, but I think it's still possible for that action > > to be requested, so I'm going to keep that check. I moved that block into Blink > > so that RenderFrameImpl doesn't need to use isFocused(). > > > > Why would it happen? Doesn't that indicate a bug in the browser process code? > > > > No, not a bug. Not sure if there are other examples, but one is that the <webview> tag find API can stop finding whenever it wants, and specify an activate action with it, whether the active match is in a focused frame or not. If it's in a non-focused frame, this condition will stop it. In any case, I think it makes sense to enforce here that only a selection in the focused frame will be activated, rather than rely on behavior elsewhere. Hmm. OK. > > > > > > > I also moved the part of RenderFrameImpl::OnFind() that used isFocused() into > > Blink for the same reason, but I didn't move everything after it because that > > would alter the order that find replies are sent, which I don't want to do. > > > > > > Does that look good to you? > > > > I don't think I understand: how does it change the order replies are sent? We're > > in an async message handler here, so it shouldn't matter if we send the reply > > out in the middle of the ipc handler or at the end: either way, the message is > > queued to be sent and not necessarily sent immediately. > > It changes the order because SendFindReply isn't the only place that a reply can be sent from. For example, increaseMatchCount below will result in a reply as well. If we wrap that all into find(), then that reply will be sent before SendFindReply, whereas now it is sent after. But find() can just report back directly by calling reportFindInPageMatchCount(), right? So I don't think that should be a problem.
https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:5040: if (frame_->isFocused() || options.findNext) { On 2016/05/17 23:58:45, dcheng wrote: > On 2016/05/17 at 22:14:35, paulmeyer wrote: > > On 2016/05/17 21:07:55, dcheng wrote: > > > On 2016/05/17 at 20:40:46, paulmeyer wrote: > > > > On 2016/05/16 20:04:21, dcheng wrote: > > > > > On 2016/05/16 at 19:27:49, paulmeyer wrote: > > > > > > On 2016/05/16 17:31:37, dcheng wrote: > > > > > > > On 2016/05/16 at 17:21:10, paulmeyer wrote: > > > > > > > > On 2016/05/16 17:11:02, dcheng wrote: > > > > > > > > > On 2016/05/16 at 15:25:11, paulmeyer wrote: > > > > > > > > > > On 2016/05/14 00:03:44, dcheng wrote: > > > > > > > > > > > I think we can just use WebView::focusedFrame() == frame_. > > > > > > > > > > > > > > > > > > > > Actually, that wouldn't work. I tried that at some point, and > it > > > led > > > > > to > > > > > > > > > confusing bugs. It turns out that WebView::focusedFrame() is not > > > named > > > > > very > > > > > > > > > appropriately, since it sometimes returns the focused frame, and > > > > > sometimes > > > > > > > > > returns other frames (this isn't a bug though, that's what it's > > > supposed > > > > > to > > > > > > > do, > > > > > > > > > it's just a really misleading name). > > > > > > > > > > > > > > > > > > > > So, that's why I created the isFocused() method to properly > > > determine > > > > > > > whether > > > > > > > > > the frame has focus. This main difference is that isFocused() > hooks > > > into > > > > > > > > > FocusController::focusedFrame() instead of > > > > > > > > > FocusController::focusedOrMainFrame(). > > > > > > > > > > > > > > > > > > It's confusing to have isFocused() and focusedFrame() not agree > > > though > > > > > =) > > > > > > > > > > > > > > > > > > For example, in this particular block, it seems like Blink > should be > > > > > able to > > > > > > > do > > > > > > > > > the check itself, so this can just be simplified to: > > > > > > > > > > > > > > > > > > if (options.findNext) { > > > > > > > > > bool result = frame_->find(...); > > > > > > > > > if (result) { > > > > > > > > > SendFindReply(...); > > > > > > > > > } > > > > > > > > > } > > > > > > > > > > > > > > > > Blink could do the check, but what you put here is not equivalent. > If > > > the > > > > > > > frame is focused, it should be searched regardless of > options.findNext. > > > I > > > > > also > > > > > > > feel that it is clearer what's going on here if we check for focus > here. > > > > > It's > > > > > > > easy to see here exactly under which conditions the search is > performed. > > > > > > > > > > > > > > OK, in that case, we don't need the conditionals at all in > //content. > > > Just > > > > > have > > > > > > > Blink do all this work: the remainder of this function is just using > the > > > > > Blink > > > > > > > public API anyway. > > > > > > > Moving this all into Blink will let us reduce the size of the Blink > API > > > and > > > > > let > > > > > > > us avoid having multiple ways to get a focused frame. So the rest of > > > this > > > > > > > function will just look like: > > > > > > > > > > > > > > if (frame_->find(...)) > > > > > > > SendFindReply(...); > > > > > > > return; > > > > > > > > > > > > I could move all of this to Blink, but it still wouldn't remove the > need > > > for > > > > > IsFocused(). It's called here and in OnStopFinding. > > > > > > > > > > I don't think it should be needed in OnStopFinding. It doesn't make > sense > > > for > > > > > the browser to trigger a STOP_FIND_ACTION_ACTIVATE_SELECTION action for > a > > > > > non-focused frame. > > > > > > > > > > > I could move both into WebLocalFrame, but it still makes sense to have > the > > > > > method instead of calling into FocusController the long way twice > (though I > > > > > could make it private). > > > > > > > > > > At least it won't be on the Blink public API. > > > > > > > > > > > > > > > > > Also, that method isn't a second way to get the focused frame. Right > now > > > it's > > > > > the only way to tell if the frame is focused, since > WebView::focusedFrame > > > > > doesn't do that, and I think that's something useful to be able to know. > I > > > agree > > > > > that it's confusing that they don't agree, but that's only because > > > > > WebView::focusedFrame is confusing. I think the correct way to fix this > > > would be > > > > > to rename WebView::focusedFrame to focusedOrMainFrame(), like the > underlying > > > > > method in FocusController that it uses (though that name is also > misleading, > > > > > since the method can also return a frame that is neither of those :P). > That > > > > > would probably be a separate CL though. > > > > > > > > > > It provides two ways to do the same thing. One way is to just use > > > > > WebView::focusedFrame(). The other is to loop through all frames looking > for > > > the > > > > > one with isFocused() == true =) > > > > > > > > Following our offline discussion, I've made WebLocalFrame::isFocused() > private > > > so that it is out of the public API. > > > > > > > > I agree that it doesn't make sense for STOP_FIND_ACTION_ACTIVATE_SELECTION > to > > > be used on a non-focused frame, but I think it's still possible for that > action > > > to be requested, so I'm going to keep that check. I moved that block into > Blink > > > so that RenderFrameImpl doesn't need to use isFocused(). > > > > > > Why would it happen? Doesn't that indicate a bug in the browser process > code? > > > > > > > No, not a bug. Not sure if there are other examples, but one is that the > <webview> tag find API can stop finding whenever it wants, and specify an > activate action with it, whether the active match is in a focused frame or not. > If it's in a non-focused frame, this condition will stop it. In any case, I > think it makes sense to enforce here that only a selection in the focused frame > will be activated, rather than rely on behavior elsewhere. > > Hmm. OK. > > > > > > > > > > > I also moved the part of RenderFrameImpl::OnFind() that used isFocused() > into > > > Blink for the same reason, but I didn't move everything after it because > that > > > would alter the order that find replies are sent, which I don't want to do. > > > > > > > > Does that look good to you? > > > > > > I don't think I understand: how does it change the order replies are sent? > We're > > > in an async message handler here, so it shouldn't matter if we send the > reply > > > out in the middle of the ipc handler or at the end: either way, the message > is > > > queued to be sent and not necessarily sent immediately. > > > > It changes the order because SendFindReply isn't the only place that a reply > can be sent from. For example, increaseMatchCount below will result in a reply > as well. If we wrap that all into find(), then that reply will be sent before > SendFindReply, whereas now it is sent after. > > But find() can just report back directly by calling > reportFindInPageMatchCount(), right? So I don't think that should be a problem. find() could call that, but that would still result in that reply coming before rather than after the reply from SendFindReply(). It wouldn't be impossible to make that work, but I don't think that is within the scope of this CL. This CL didn't introduce these calls here, so if they need to be cleaned up, it can be done in a separate CL.
https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:5040: if (frame_->isFocused() || options.findNext) { On 2016/05/18 at 14:10:46, paulmeyer wrote: > On 2016/05/17 23:58:45, dcheng wrote: > > On 2016/05/17 at 22:14:35, paulmeyer wrote: > > > On 2016/05/17 21:07:55, dcheng wrote: > > > > On 2016/05/17 at 20:40:46, paulmeyer wrote: > > > > > On 2016/05/16 20:04:21, dcheng wrote: > > > > > > On 2016/05/16 at 19:27:49, paulmeyer wrote: > > > > > > > On 2016/05/16 17:31:37, dcheng wrote: > > > > > > > > On 2016/05/16 at 17:21:10, paulmeyer wrote: > > > > > > > > > On 2016/05/16 17:11:02, dcheng wrote: > > > > > > > > > > On 2016/05/16 at 15:25:11, paulmeyer wrote: > > > > > > > > > > > On 2016/05/14 00:03:44, dcheng wrote: > > > > > > > > > > > > I think we can just use WebView::focusedFrame() == frame_. > > > > > > > > > > > > > > > > > > > > > > Actually, that wouldn't work. I tried that at some point, and > > it > > > > led > > > > > > to > > > > > > > > > > confusing bugs. It turns out that WebView::focusedFrame() is not > > > > named > > > > > > very > > > > > > > > > > appropriately, since it sometimes returns the focused frame, and > > > > > > sometimes > > > > > > > > > > returns other frames (this isn't a bug though, that's what it's > > > > supposed > > > > > > to > > > > > > > > do, > > > > > > > > > > it's just a really misleading name). > > > > > > > > > > > > > > > > > > > > > > So, that's why I created the isFocused() method to properly > > > > determine > > > > > > > > whether > > > > > > > > > > the frame has focus. This main difference is that isFocused() > > hooks > > > > into > > > > > > > > > > FocusController::focusedFrame() instead of > > > > > > > > > > FocusController::focusedOrMainFrame(). > > > > > > > > > > > > > > > > > > > > It's confusing to have isFocused() and focusedFrame() not agree > > > > though > > > > > > =) > > > > > > > > > > > > > > > > > > > > For example, in this particular block, it seems like Blink > > should be > > > > > > able to > > > > > > > > do > > > > > > > > > > the check itself, so this can just be simplified to: > > > > > > > > > > > > > > > > > > > > if (options.findNext) { > > > > > > > > > > bool result = frame_->find(...); > > > > > > > > > > if (result) { > > > > > > > > > > SendFindReply(...); > > > > > > > > > > } > > > > > > > > > > } > > > > > > > > > > > > > > > > > > Blink could do the check, but what you put here is not equivalent. > > If > > > > the > > > > > > > > frame is focused, it should be searched regardless of > > options.findNext. > > > > I > > > > > > also > > > > > > > > feel that it is clearer what's going on here if we check for focus > > here. > > > > > > It's > > > > > > > > easy to see here exactly under which conditions the search is > > performed. > > > > > > > > > > > > > > > > OK, in that case, we don't need the conditionals at all in > > //content. > > > > Just > > > > > > have > > > > > > > > Blink do all this work: the remainder of this function is just using > > the > > > > > > Blink > > > > > > > > public API anyway. > > > > > > > > Moving this all into Blink will let us reduce the size of the Blink > > API > > > > and > > > > > > let > > > > > > > > us avoid having multiple ways to get a focused frame. So the rest of > > > > this > > > > > > > > function will just look like: > > > > > > > > > > > > > > > > if (frame_->find(...)) > > > > > > > > SendFindReply(...); > > > > > > > > return; > > > > > > > > > > > > > > I could move all of this to Blink, but it still wouldn't remove the > > need > > > > for > > > > > > IsFocused(). It's called here and in OnStopFinding. > > > > > > > > > > > > I don't think it should be needed in OnStopFinding. It doesn't make > > sense > > > > for > > > > > > the browser to trigger a STOP_FIND_ACTION_ACTIVATE_SELECTION action for > > a > > > > > > non-focused frame. > > > > > > > > > > > > > I could move both into WebLocalFrame, but it still makes sense to have > > the > > > > > > method instead of calling into FocusController the long way twice > > (though I > > > > > > could make it private). > > > > > > > > > > > > At least it won't be on the Blink public API. > > > > > > > > > > > > > > > > > > > > Also, that method isn't a second way to get the focused frame. Right > > now > > > > it's > > > > > > the only way to tell if the frame is focused, since > > WebView::focusedFrame > > > > > > doesn't do that, and I think that's something useful to be able to know. > > I > > > > agree > > > > > > that it's confusing that they don't agree, but that's only because > > > > > > WebView::focusedFrame is confusing. I think the correct way to fix this > > > > would be > > > > > > to rename WebView::focusedFrame to focusedOrMainFrame(), like the > > underlying > > > > > > method in FocusController that it uses (though that name is also > > misleading, > > > > > > since the method can also return a frame that is neither of those :P). > > That > > > > > > would probably be a separate CL though. > > > > > > > > > > > > It provides two ways to do the same thing. One way is to just use > > > > > > WebView::focusedFrame(). The other is to loop through all frames looking > > for > > > > the > > > > > > one with isFocused() == true =) > > > > > > > > > > Following our offline discussion, I've made WebLocalFrame::isFocused() > > private > > > > so that it is out of the public API. > > > > > > > > > > I agree that it doesn't make sense for STOP_FIND_ACTION_ACTIVATE_SELECTION > > to > > > > be used on a non-focused frame, but I think it's still possible for that > > action > > > > to be requested, so I'm going to keep that check. I moved that block into > > Blink > > > > so that RenderFrameImpl doesn't need to use isFocused(). > > > > > > > > Why would it happen? Doesn't that indicate a bug in the browser process > > code? > > > > > > > > > > No, not a bug. Not sure if there are other examples, but one is that the > > <webview> tag find API can stop finding whenever it wants, and specify an > > activate action with it, whether the active match is in a focused frame or not. > > If it's in a non-focused frame, this condition will stop it. In any case, I > > think it makes sense to enforce here that only a selection in the focused frame > > will be activated, rather than rely on behavior elsewhere. > > > > Hmm. OK. > > > > > > > > > > > > > > > I also moved the part of RenderFrameImpl::OnFind() that used isFocused() > > into > > > > Blink for the same reason, but I didn't move everything after it because > > that > > > > would alter the order that find replies are sent, which I don't want to do. > > > > > > > > > > Does that look good to you? > > > > > > > > I don't think I understand: how does it change the order replies are sent? > > We're > > > > in an async message handler here, so it shouldn't matter if we send the > > reply > > > > out in the middle of the ipc handler or at the end: either way, the message > > is > > > > queued to be sent and not necessarily sent immediately. > > > > > > It changes the order because SendFindReply isn't the only place that a reply > > can be sent from. For example, increaseMatchCount below will result in a reply > > as well. If we wrap that all into find(), then that reply will be sent before > > SendFindReply, whereas now it is sent after. > > > > But find() can just report back directly by calling > > reportFindInPageMatchCount(), right? So I don't think that should be a problem. > > find() could call that, but that would still result in that reply coming before rather than after the reply from SendFindReply(). It wouldn't be impossible to make that work, but I don't think that is within the scope of this CL. This CL didn't introduce these calls here, so if they need to be cleaned up, it can be done in a separate CL. Are you willing to commit to fixing this up in a followup CL then? I find this logic rather difficult to follow, that's why I'm asking for these changes =)
On 2016/05/18 at 17:43:08, dcheng wrote: > https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... > File content/renderer/render_frame_impl.cc (right): > > https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... > content/renderer/render_frame_impl.cc:5040: if (frame_->isFocused() || options.findNext) { > On 2016/05/18 at 14:10:46, paulmeyer wrote: > > On 2016/05/17 23:58:45, dcheng wrote: > > > On 2016/05/17 at 22:14:35, paulmeyer wrote: > > > > On 2016/05/17 21:07:55, dcheng wrote: > > > > > On 2016/05/17 at 20:40:46, paulmeyer wrote: > > > > > > On 2016/05/16 20:04:21, dcheng wrote: > > > > > > > On 2016/05/16 at 19:27:49, paulmeyer wrote: > > > > > > > > On 2016/05/16 17:31:37, dcheng wrote: > > > > > > > > > On 2016/05/16 at 17:21:10, paulmeyer wrote: > > > > > > > > > > On 2016/05/16 17:11:02, dcheng wrote: > > > > > > > > > > > On 2016/05/16 at 15:25:11, paulmeyer wrote: > > > > > > > > > > > > On 2016/05/14 00:03:44, dcheng wrote: > > > > > > > > > > > > > I think we can just use WebView::focusedFrame() == frame_. > > > > > > > > > > > > > > > > > > > > > > > > Actually, that wouldn't work. I tried that at some point, and > > > it > > > > > led > > > > > > > to > > > > > > > > > > > confusing bugs. It turns out that WebView::focusedFrame() is not > > > > > named > > > > > > > very > > > > > > > > > > > appropriately, since it sometimes returns the focused frame, and > > > > > > > sometimes > > > > > > > > > > > returns other frames (this isn't a bug though, that's what it's > > > > > supposed > > > > > > > to > > > > > > > > > do, > > > > > > > > > > > it's just a really misleading name). > > > > > > > > > > > > > > > > > > > > > > > > So, that's why I created the isFocused() method to properly > > > > > determine > > > > > > > > > whether > > > > > > > > > > > the frame has focus. This main difference is that isFocused() > > > hooks > > > > > into > > > > > > > > > > > FocusController::focusedFrame() instead of > > > > > > > > > > > FocusController::focusedOrMainFrame(). > > > > > > > > > > > > > > > > > > > > > > It's confusing to have isFocused() and focusedFrame() not agree > > > > > though > > > > > > > =) > > > > > > > > > > > > > > > > > > > > > > For example, in this particular block, it seems like Blink > > > should be > > > > > > > able to > > > > > > > > > do > > > > > > > > > > > the check itself, so this can just be simplified to: > > > > > > > > > > > > > > > > > > > > > > if (options.findNext) { > > > > > > > > > > > bool result = frame_->find(...); > > > > > > > > > > > if (result) { > > > > > > > > > > > SendFindReply(...); > > > > > > > > > > > } > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > Blink could do the check, but what you put here is not equivalent. > > > If > > > > > the > > > > > > > > > frame is focused, it should be searched regardless of > > > options.findNext. > > > > > I > > > > > > > also > > > > > > > > > feel that it is clearer what's going on here if we check for focus > > > here. > > > > > > > It's > > > > > > > > > easy to see here exactly under which conditions the search is > > > performed. > > > > > > > > > > > > > > > > > > OK, in that case, we don't need the conditionals at all in > > > //content. > > > > > Just > > > > > > > have > > > > > > > > > Blink do all this work: the remainder of this function is just using > > > the > > > > > > > Blink > > > > > > > > > public API anyway. > > > > > > > > > Moving this all into Blink will let us reduce the size of the Blink > > > API > > > > > and > > > > > > > let > > > > > > > > > us avoid having multiple ways to get a focused frame. So the rest of > > > > > this > > > > > > > > > function will just look like: > > > > > > > > > > > > > > > > > > if (frame_->find(...)) > > > > > > > > > SendFindReply(...); > > > > > > > > > return; > > > > > > > > > > > > > > > > I could move all of this to Blink, but it still wouldn't remove the > > > need > > > > > for > > > > > > > IsFocused(). It's called here and in OnStopFinding. > > > > > > > > > > > > > > I don't think it should be needed in OnStopFinding. It doesn't make > > > sense > > > > > for > > > > > > > the browser to trigger a STOP_FIND_ACTION_ACTIVATE_SELECTION action for > > > a > > > > > > > non-focused frame. > > > > > > > > > > > > > > > I could move both into WebLocalFrame, but it still makes sense to have > > > the > > > > > > > method instead of calling into FocusController the long way twice > > > (though I > > > > > > > could make it private). > > > > > > > > > > > > > > At least it won't be on the Blink public API. > > > > > > > > > > > > > > > > > > > > > > > Also, that method isn't a second way to get the focused frame. Right > > > now > > > > > it's > > > > > > > the only way to tell if the frame is focused, since > > > WebView::focusedFrame > > > > > > > doesn't do that, and I think that's something useful to be able to know. > > > I > > > > > agree > > > > > > > that it's confusing that they don't agree, but that's only because > > > > > > > WebView::focusedFrame is confusing. I think the correct way to fix this > > > > > would be > > > > > > > to rename WebView::focusedFrame to focusedOrMainFrame(), like the > > > underlying > > > > > > > method in FocusController that it uses (though that name is also > > > misleading, > > > > > > > since the method can also return a frame that is neither of those :P). > > > That > > > > > > > would probably be a separate CL though. > > > > > > > > > > > > > > It provides two ways to do the same thing. One way is to just use > > > > > > > WebView::focusedFrame(). The other is to loop through all frames looking > > > for > > > > > the > > > > > > > one with isFocused() == true =) > > > > > > > > > > > > Following our offline discussion, I've made WebLocalFrame::isFocused() > > > private > > > > > so that it is out of the public API. > > > > > > > > > > > > I agree that it doesn't make sense for STOP_FIND_ACTION_ACTIVATE_SELECTION > > > to > > > > > be used on a non-focused frame, but I think it's still possible for that > > > action > > > > > to be requested, so I'm going to keep that check. I moved that block into > > > Blink > > > > > so that RenderFrameImpl doesn't need to use isFocused(). > > > > > > > > > > Why would it happen? Doesn't that indicate a bug in the browser process > > > code? > > > > > > > > > > > > > No, not a bug. Not sure if there are other examples, but one is that the > > > <webview> tag find API can stop finding whenever it wants, and specify an > > > activate action with it, whether the active match is in a focused frame or not. > > > If it's in a non-focused frame, this condition will stop it. In any case, I > > > think it makes sense to enforce here that only a selection in the focused frame > > > will be activated, rather than rely on behavior elsewhere. > > > > > > Hmm. OK. > > > > > > > > > > > > > > > > > > > I also moved the part of RenderFrameImpl::OnFind() that used isFocused() > > > into > > > > > Blink for the same reason, but I didn't move everything after it because > > > that > > > > > would alter the order that find replies are sent, which I don't want to do. > > > > > > > > > > > > Does that look good to you? > > > > > > > > > > I don't think I understand: how does it change the order replies are sent? > > > We're > > > > > in an async message handler here, so it shouldn't matter if we send the > > > reply > > > > > out in the middle of the ipc handler or at the end: either way, the message > > > is > > > > > queued to be sent and not necessarily sent immediately. > > > > > > > > It changes the order because SendFindReply isn't the only place that a reply > > > can be sent from. For example, increaseMatchCount below will result in a reply > > > as well. If we wrap that all into find(), then that reply will be sent before > > > SendFindReply, whereas now it is sent after. > > > > > > But find() can just report back directly by calling > > > reportFindInPageMatchCount(), right? So I don't think that should be a problem. > > > > find() could call that, but that would still result in that reply coming before rather than after the reply from SendFindReply(). It wouldn't be impossible to make that work, but I don't think that is within the scope of this CL. This CL didn't introduce these calls here, so if they need to be cleaned up, it can be done in a separate CL. > > Are you willing to commit to fixing this up in a followup CL then? > > I find this logic rather difficult to follow, that's why I'm asking for these changes =) Also, this wouldn't change the order of operations: we're simply replacing a call to SendFindReply() to reportFindInPageMatchCount(): those two are pretty much the same function, just with slightly different signatures.
https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1959183002/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:5040: if (frame_->isFocused() || options.findNext) { On 2016/05/18 17:43:07, dcheng wrote: > On 2016/05/18 at 14:10:46, paulmeyer wrote: > > On 2016/05/17 23:58:45, dcheng wrote: > > > On 2016/05/17 at 22:14:35, paulmeyer wrote: > > > > On 2016/05/17 21:07:55, dcheng wrote: > > > > > On 2016/05/17 at 20:40:46, paulmeyer wrote: > > > > > > On 2016/05/16 20:04:21, dcheng wrote: > > > > > > > On 2016/05/16 at 19:27:49, paulmeyer wrote: > > > > > > > > On 2016/05/16 17:31:37, dcheng wrote: > > > > > > > > > On 2016/05/16 at 17:21:10, paulmeyer wrote: > > > > > > > > > > On 2016/05/16 17:11:02, dcheng wrote: > > > > > > > > > > > On 2016/05/16 at 15:25:11, paulmeyer wrote: > > > > > > > > > > > > On 2016/05/14 00:03:44, dcheng wrote: > > > > > > > > > > > > > I think we can just use WebView::focusedFrame() == > frame_. > > > > > > > > > > > > > > > > > > > > > > > > Actually, that wouldn't work. I tried that at some point, > and > > > it > > > > > led > > > > > > > to > > > > > > > > > > > confusing bugs. It turns out that WebView::focusedFrame() is > not > > > > > named > > > > > > > very > > > > > > > > > > > appropriately, since it sometimes returns the focused frame, > and > > > > > > > sometimes > > > > > > > > > > > returns other frames (this isn't a bug though, that's what > it's > > > > > supposed > > > > > > > to > > > > > > > > > do, > > > > > > > > > > > it's just a really misleading name). > > > > > > > > > > > > > > > > > > > > > > > > So, that's why I created the isFocused() method to > properly > > > > > determine > > > > > > > > > whether > > > > > > > > > > > the frame has focus. This main difference is that > isFocused() > > > hooks > > > > > into > > > > > > > > > > > FocusController::focusedFrame() instead of > > > > > > > > > > > FocusController::focusedOrMainFrame(). > > > > > > > > > > > > > > > > > > > > > > It's confusing to have isFocused() and focusedFrame() not > agree > > > > > though > > > > > > > =) > > > > > > > > > > > > > > > > > > > > > > For example, in this particular block, it seems like Blink > > > should be > > > > > > > able to > > > > > > > > > do > > > > > > > > > > > the check itself, so this can just be simplified to: > > > > > > > > > > > > > > > > > > > > > > if (options.findNext) { > > > > > > > > > > > bool result = frame_->find(...); > > > > > > > > > > > if (result) { > > > > > > > > > > > SendFindReply(...); > > > > > > > > > > > } > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > Blink could do the check, but what you put here is not > equivalent. > > > If > > > > > the > > > > > > > > > frame is focused, it should be searched regardless of > > > options.findNext. > > > > > I > > > > > > > also > > > > > > > > > feel that it is clearer what's going on here if we check for > focus > > > here. > > > > > > > It's > > > > > > > > > easy to see here exactly under which conditions the search is > > > performed. > > > > > > > > > > > > > > > > > > OK, in that case, we don't need the conditionals at all in > > > //content. > > > > > Just > > > > > > > have > > > > > > > > > Blink do all this work: the remainder of this function is just > using > > > the > > > > > > > Blink > > > > > > > > > public API anyway. > > > > > > > > > Moving this all into Blink will let us reduce the size of the > Blink > > > API > > > > > and > > > > > > > let > > > > > > > > > us avoid having multiple ways to get a focused frame. So the > rest of > > > > > this > > > > > > > > > function will just look like: > > > > > > > > > > > > > > > > > > if (frame_->find(...)) > > > > > > > > > SendFindReply(...); > > > > > > > > > return; > > > > > > > > > > > > > > > > I could move all of this to Blink, but it still wouldn't remove > the > > > need > > > > > for > > > > > > > IsFocused(). It's called here and in OnStopFinding. > > > > > > > > > > > > > > I don't think it should be needed in OnStopFinding. It doesn't make > > > sense > > > > > for > > > > > > > the browser to trigger a STOP_FIND_ACTION_ACTIVATE_SELECTION action > for > > > a > > > > > > > non-focused frame. > > > > > > > > > > > > > > > I could move both into WebLocalFrame, but it still makes sense to > have > > > the > > > > > > > method instead of calling into FocusController the long way twice > > > (though I > > > > > > > could make it private). > > > > > > > > > > > > > > At least it won't be on the Blink public API. > > > > > > > > > > > > > > > > > > > > > > > Also, that method isn't a second way to get the focused frame. > Right > > > now > > > > > it's > > > > > > > the only way to tell if the frame is focused, since > > > WebView::focusedFrame > > > > > > > doesn't do that, and I think that's something useful to be able to > know. > > > I > > > > > agree > > > > > > > that it's confusing that they don't agree, but that's only because > > > > > > > WebView::focusedFrame is confusing. I think the correct way to fix > this > > > > > would be > > > > > > > to rename WebView::focusedFrame to focusedOrMainFrame(), like the > > > underlying > > > > > > > method in FocusController that it uses (though that name is also > > > misleading, > > > > > > > since the method can also return a frame that is neither of those > :P). > > > That > > > > > > > would probably be a separate CL though. > > > > > > > > > > > > > > It provides two ways to do the same thing. One way is to just use > > > > > > > WebView::focusedFrame(). The other is to loop through all frames > looking > > > for > > > > > the > > > > > > > one with isFocused() == true =) > > > > > > > > > > > > Following our offline discussion, I've made WebLocalFrame::isFocused() > > > private > > > > > so that it is out of the public API. > > > > > > > > > > > > I agree that it doesn't make sense for > STOP_FIND_ACTION_ACTIVATE_SELECTION > > > to > > > > > be used on a non-focused frame, but I think it's still possible for that > > > action > > > > > to be requested, so I'm going to keep that check. I moved that block > into > > > Blink > > > > > so that RenderFrameImpl doesn't need to use isFocused(). > > > > > > > > > > Why would it happen? Doesn't that indicate a bug in the browser process > > > code? > > > > > > > > > > > > > No, not a bug. Not sure if there are other examples, but one is that the > > > <webview> tag find API can stop finding whenever it wants, and specify an > > > activate action with it, whether the active match is in a focused frame or > not. > > > If it's in a non-focused frame, this condition will stop it. In any case, I > > > think it makes sense to enforce here that only a selection in the focused > frame > > > will be activated, rather than rely on behavior elsewhere. > > > > > > Hmm. OK. > > > > > > > > > > > > > > > > > > > I also moved the part of RenderFrameImpl::OnFind() that used > isFocused() > > > into > > > > > Blink for the same reason, but I didn't move everything after it because > > > that > > > > > would alter the order that find replies are sent, which I don't want to > do. > > > > > > > > > > > > Does that look good to you? > > > > > > > > > > I don't think I understand: how does it change the order replies are > sent? > > > We're > > > > > in an async message handler here, so it shouldn't matter if we send the > > > reply > > > > > out in the middle of the ipc handler or at the end: either way, the > message > > > is > > > > > queued to be sent and not necessarily sent immediately. > > > > > > > > It changes the order because SendFindReply isn't the only place that a > reply > > > can be sent from. For example, increaseMatchCount below will result in a > reply > > > as well. If we wrap that all into find(), then that reply will be sent > before > > > SendFindReply, whereas now it is sent after. > > > > > > But find() can just report back directly by calling > > > reportFindInPageMatchCount(), right? So I don't think that should be a > problem. > > > > find() could call that, but that would still result in that reply coming > before rather than after the reply from SendFindReply(). It wouldn't be > impossible to make that work, but I don't think that is within the scope of this > CL. This CL didn't introduce these calls here, so if they need to be cleaned up, > it can be done in a separate CL. > > Are you willing to commit to fixing this up in a followup CL then? > > I find this logic rather difficult to follow, that's why I'm asking for these > changes =) Yeah, I could work on cleaning this up a bit in a followup CL, but it'll be prioritized among other work. You can assign a bug to me about it. I can see how this logic is a bit confusing, but all of the lines you're asking about (and their logic) were already here before this CL. I just don't want to confound an already large CL with additional cleanup on code that doesn't need to be changed by this CL. I also don't want to risk potentially changing something in the logic that might introduce subtle bugs at this late stage of such a large implementation.
paulmeyer@chromium.org changed reviewers: + nick@chromium.org
+nick@ for content/ review.
I looked over the Blink changes and they look reasonable to me, but I'm mostly focusing on how the various bits interact and how the public API works. As discussed, we'll reduce the size of the public API in a followup patch. As for the implementation details in Blink, dglazkov@ may want to sanity check them: nothing looks unreasonable, but I'm not an expert on find-in-page either. =) https://codereview.chromium.org/1959183002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/TextFinder.h (right): https://codereview.chromium.org/1959183002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/TextFinder.h:221: bool m_frameScoping; I know you didn't introduce this, but it would be nice if there was a comment somewhere that described what 'scoping' means in this context. https://codereview.chromium.org/1959183002/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebLocalFrame.h (right): https://codereview.chromium.org/1959183002/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/web/WebLocalFrame.h:256: virtual void stopFinding(bool clearSelection, bool activateSelection) = 0; enum instead of two bools (since the bools are mutually exclusive anyway) https://codereview.chromium.org/1959183002/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/web/WebLocalFrame.h:313: // Returns the distance (squared) to the closest find-in-page match from the Maybe name this distanceToNearestFindMatch() to make it clear what it returns. I guess you could call it distanceSquaredToNearestFindMatch(), but that doesn't seem like a super important detail.
Patchset #4 (id:140001) has been deleted
https://codereview.chromium.org/1959183002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/TextFinder.h (right): https://codereview.chromium.org/1959183002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/web/TextFinder.h:221: bool m_frameScoping; On 2016/05/18 21:29:21, dcheng wrote: > I know you didn't introduce this, but it would be nice if there was a comment > somewhere that described what 'scoping' means in this context. I'm not sure if there is a better place for this, but I can put one here. Done. https://codereview.chromium.org/1959183002/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebLocalFrame.h (right): https://codereview.chromium.org/1959183002/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/web/WebLocalFrame.h:256: virtual void stopFinding(bool clearSelection, bool activateSelection) = 0; On 2016/05/18 21:29:21, dcheng wrote: > enum instead of two bools (since the bools are mutually exclusive anyway) Done. https://codereview.chromium.org/1959183002/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/web/WebLocalFrame.h:313: // Returns the distance (squared) to the closest find-in-page match from the On 2016/05/18 21:29:21, dcheng wrote: > Maybe name this distanceToNearestFindMatch() to make it clear what it returns. > > I guess you could call it distanceSquaredToNearestFindMatch(), but that doesn't > seem like a super important detail. Done. I didn't add "squared" because I agree that it isn't important. Some places along the way mention that the distance is squared, and some don't. Since these distances are just used comparatively, the fact that they are squared is just an implementation detail.
https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:73: } Could you move these to FrameTree or FrameTreeNode, and add unittests for them in frame_tree_unittest.cc? You could even just extend FrameTreeTest.Shape with some new asserts. The wrapping logic probably can stay here. FWIW, I understand that the FrameTree::Nodes() ordering is different from this. If I read the wikipedia page right, I think FrameTree::Nodes() does treeorder traversal, and find-in-page needs preorder traversal. I don't think there's any reason why FrameTree::Nodes() couldn't be preorder as well -- the only property we really need for FrameTree::NodeS() is for parents to be visited before children. So, long term I would not mind unifying the two (e.g., implementing the NodeIterator in terms of these tree traversal functions), but doing so doesn't block this CL. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:117: contents_->ForEachFrame(base::Bind( We should skip !IsRenderFrameLive() frames. SendToAllFrames does this automatically. Or, you can consider using a loop here (over FrameTree::Nodes()) instead of ForEach() and a helper function. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:141: if (number_of_matches != -1) { This use of sentinel values makes me wonder if we ought to break this IPC message in half. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:151: if (active_match_ordinal > 0) { Could we rename either active_match_ordinal or active_match_ordinal_? Since the operation isn't a straight copy anymore, it's hard to follow what's going on here. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:154: active_match_ordinal - relative_active_match_ordinal_.second; is the value of "(active_match_ordinal - relative_active_match_ordinal_.second)" always either 1 or -1? https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:160: if (old_rfh && old_rfh != rfh) { the != case here is impossible. We already checked for this on line 152. Likewise, the !old_rfh case ought to be impossible, since we cleanup relative_active_match_ordinal_.first on RenderFrameDeleted. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:172: } Looking at this function, I wondered: what if we try to advance the selection into a subframe that previously reported matches, but for which no matches were found? https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:173: Possibly related to the previous comment: In a debug Windows build, I can repro a renderer crash fairly in consistently with the following document (I didn't gather a stack trace since my MSVC license decided to expire today). Keep searching for variations of 'result' and hit enter/escape/Ctrl+S occasionally: <!DOCTYPE html> <html> <head> <meta name="viewport" content="width=device-width, initial-scale=1.0, maximum-scale=1.0, user-scalable=no" /> </head> <body> RESULT result <script> /* window.setInterval(function() { var ifr = document.createElement('iframe'); ifr.srcdoc = "<script>window.setInterval(function() { document.body.appendChild(document.createTextNode('result'));}, 7000)</sc" + "ript>"; document.body.appendChild(ifr); }, 7000); */ window.setInterval(function() { var ifr = document.createElement('iframe'); ifr.srcdoc = "<body>result result<script>window.setTimeout(function() { document.write('cleared');}, 3000)</sc" + "ript>"; document.body.appendChild(ifr); }, 2000); </script> </body> </html> https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:254: contents_->ForEachFrame(base::Bind( We should skip !IsRenderFrameLive() frames. SendToAllFrames does this automatically. Or, you can consider using a loop here (over FrameTree::Nodes()) instead of ForEach() and a helper function. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:281: contents_->ForEachFrame(base::Bind( We should skip !IsRenderFrameLive() frames. SendToAllFrames does this automatically. Or, you can consider using a loop here (over FrameTree::Nodes()) instead of ForEach() and a helper function. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:307: void FindRequestManager::FrameDeleted(RenderFrameHost* rfh) { Usually if you care about FrameDeleted (which is what happens when an FTN goes away), you care about RenderFrameHostChanged too (which is what happens when an FTN navigates to a different process, changing the value of current_frame_host() for the node). Here I think you probably need overrides for all three: - RenderFrameDeleted to handle the case that a subframe crashes. - FrameDeleted to handle a case that the parent document deletes its iframe element - RenderFrameHostChanged to handle cross-process navigation (just RemoveFrame on (old_frame)). https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:309: } Based on my manual testing of this patch, it seems like we are taking a slight regression vs TOT, in the following two behaviors: - When a new iframe is added during the course of the session, we won't descend into it. - If an iframe exists at the beginning of the session, but initially has no matches, and then later its document is updated to have matches, we will never visit those matches. Even if you delete and re-type the last character of the search text, it seems like there's a case where we skip over a frame with matches in it. Can these be reasonably fixed? The following html snippet demonstrates the above problems. <script> window.setInterval(function() { document.body.appendChild(document.createTextNode('result'));}, 5000); window.setInterval(function() { var ifr = document.createElement('iframe'); ifr.srcdoc = "<script>window.setInterval(function() { document.body.appendChild(document.createTextNode('result'));}, 7000)</sc" + "ript>"; document.body.appendChild(ifr); }, 7000); </script> https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:354: contents_->ForEachFrame(base::Bind(&FindRequestManager::AddFrame, We should skip !IsRenderFrameLive() frames. You can consider using a loop here (over FrameTree::Nodes()) instead of ForEach() and a helper function. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... File content/browser/find_request_manager.h (right): https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.h:21: namespace content { I patched this in. It works great and it handled everything I threw at it, including using devtools to delete random nodes from file:///C:/src/cr/src/content/test/data/cross_site_iframe_factory.html?a(a(a(a(a,a,a,a),a(a,a,a,a),a(a,a,a,a),a(a,a,a,a)),a(a(a,a,a,a),a(a,a,a,a),a(a,a,a,a),a(a,a,a,a)),a(a(a,a,a,a),a(a,a,a,a),a(a,a,a,a),a(a,a,a,a)),a),a(a(a(a,a,a,a),a(a,a,a,a),a(a,a,a,a),a(a,a,a,a)),a(a(a,a,a,a),a(a,a,a,a),a(a,a,a,a),a(a,a,a,a)),a(a(a,a,a,a),a(a,a,a,a),a(a,a,a,a),a(a,a,a,a)),a),a(a(a(a,a,a,a),a(a,a,a,a),a(a,a,a,a),a(a,a,a,a)),a(a(a,a,a,a),a(a,a,a,a),a(a,a,a,a),a(a,a,a,a)),a(a(a,a,a,a),a(a,a,a,a),a(a,a,a,a),a(a,a,a,a)),a),a(a(a(a,a,a,a),a(a,a,a,a),a(a,a,a,a),a(a,a,a,a)),a(a(a,a,a,a),a(a,a,a,a),a(a,a,a,a),a(a,a,a,a)),a(a(a,a,a,a),a(a,a,a,a),a(a,a,a,a),a(a,a,a,a)),a)) Nice work. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.h:34: using FrameKey = std::pair<int, int>; Some places have started to use content/browser/loader/global_routing_id.h for this. Did you consider simply using RenderFrameHostImpl* ptrs as keys instead? It seems like you are cleaning up your FrameKeys at RenderFrameDeleted time anyway, so you don't need to worry about use-after-free. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.h:226: using RectData = std::pair<int, std::vector<gfx::RectF>>; Sometimes we need a pair as a key for a data structure (so that comparison is defined, a la FrameKey), but I don't see a good reason to ever use it like this. Structs with named members produce much more readable, and greppable, code. Please define a struct for this. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.h:256: std::set<FrameKey> pending_replies_; Consider making this a std::unordered_set. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.h:260: // kInvalidId otherwise. This member is somewhat confusing to me. It is a request ID, but the name suggests that its value would be either a match ordinal or a bool. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.h:261: int pending_active_match_ordinal_; Under what circumstances can pending_active_match_ordinal_ ever be a different value from current_request_.id? If current_request_.id can advance past pending_active_match_ordinal_, why is it important that we preserve the older pending_active_match_ordinal_ request id? https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.h:267: std::map<FrameKey, int> matches_per_frame_; I see four different data structures keyed by FrameKey. Would it be worth merging these into a single std::map<FrameKey, FrameState>, where FrameState is a struct or class like: bool is_pending_reply_; bool is_pending_match_rects_reply_; int match_count_; RectData match_rects_data_; ? My preference is for whatever is most readable. If you do merge them, your usage pattern for all these data structures looks like a better fit for unordered_map, since you never iterate over the collection. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.h:275: std::pair<FrameKey, int> relative_active_match_ordinal_; For the same reasons as the other pair, this would be better as two members.
nick@: PTAL https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:73: } On 2016/05/23 21:59:16, ncarter wrote: > Could you move these to FrameTree or FrameTreeNode, and add unittests for them > in frame_tree_unittest.cc? You could even just extend FrameTreeTest.Shape with > some new asserts. > > The wrapping logic probably can stay here. > > FWIW, I understand that the FrameTree::Nodes() ordering is different from this. > If I read the wikipedia page right, I think FrameTree::Nodes() does treeorder > traversal, and find-in-page needs preorder traversal. I don't think there's any > reason why FrameTree::Nodes() couldn't be preorder as well -- the only property > we really need for FrameTree::NodeS() is for parents to be visited before > children. So, long term I would not mind unifying the two (e.g., implementing > the NodeIterator in terms of these tree traversal functions), but doing so > doesn't block this CL. As discussed offline, I can look into moving these and changing the ordering of Nodes() in a future patch. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:117: contents_->ForEachFrame(base::Bind( On 2016/05/23 21:59:16, ncarter wrote: > We should skip !IsRenderFrameLive() frames. > > SendToAllFrames does this automatically. Or, you can consider using a loop here > (over FrameTree::Nodes()) instead of ForEach() and a helper function. I'll just use SendToAllFrames in this case. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:141: if (number_of_matches != -1) { On 2016/05/23 21:59:16, ncarter wrote: > This use of sentinel values makes me wonder if we ought to break this IPC > message in half. As discussed offline, splitting this IPC may not make things simpler, but this can be revisited in the future. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:151: if (active_match_ordinal > 0) { On 2016/05/23 21:59:16, ncarter wrote: > Could we rename either active_match_ordinal or active_match_ordinal_? Since the > operation isn't a straight copy anymore, it's hard to follow what's going on > here. I'd actually really rather keep them the way they are, since they're named as exactly what they are, and that's how these things are referred to in other places. Also, |active_match_ordinal_| is only referred to once in this whole function, so I think it should be okay. I can see how this part of the function was a bit busy though... I've added some comments and spacing to hopefully make it more clear where each result is potentially updated. It's also a bit cleaner already after following your comments about |relative_active_match_ordinal_| and |old_rfh|. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:154: active_match_ordinal - relative_active_match_ordinal_.second; On 2016/05/23 21:59:16, ncarter wrote: > is the value of "(active_match_ordinal - relative_active_match_ordinal_.second)" > always either 1 or -1? No, it could be any arbitrary int. In a large frame, the user can click somewhere else in the same frame and then "find next" to get the next active match to be far away from the previous. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:160: if (old_rfh && old_rfh != rfh) { On 2016/05/23 21:59:16, ncarter wrote: > the != case here is impossible. We already checked for this on line 152. > > Likewise, the !old_rfh case ought to be impossible, since we cleanup > relative_active_match_ordinal_.first on RenderFrameDeleted. Okay, you're right. I can just take out these conditions. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:172: } On 2016/05/23 21:59:16, ncarter wrote: > Looking at this function, I wondered: what if we try to advance the selection > into a subframe that previously reported matches, but for which no matches were > found? That case involves dynamically changing content (since previously existing matches were removed somehow), which find-in-page does not currently handle. My multi-process implementation also doesn't explicitly provide this functionality, though it will never crash or do any worse than the current implementation (the worst that can happen is that the number of matches or active match ordinal might show incorrectly until the next find session). That being said, I just tried this and this particular case actually happens to be handled correctly by my implementation. It breaks the find session in stable Chrome, so that's an improvement. :) What happens in my code is the frame will be entered that previously had matches, but no new active match will be returned (because there are none). This will be caught in FinalUpdate(), which will then look for an active match in the next frame it expects should have one. If that one also no longer has any, this will continue until the next active match is found or no frames are left with matches. The number of matches also gets updated by this, so everything works out. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:173: On 2016/05/23 21:59:16, ncarter wrote: > Possibly related to the previous comment: In a debug Windows build, I can repro > a renderer crash fairly in consistently with the following document (I didn't > gather a stack trace since my MSVC license decided to expire today). Keep > searching for variations of 'result' and hit enter/escape/Ctrl+S occasionally: > > <!DOCTYPE html> > <html> > <head> > <meta name="viewport" content="width=device-width, initial-scale=1.0, > maximum-scale=1.0, user-scalable=no" /> > </head> > <body> > RESULT result > <script> > /* > window.setInterval(function() { > var ifr = document.createElement('iframe'); > ifr.srcdoc = "<script>window.setInterval(function() { > document.body.appendChild(document.createTextNode('result'));}, 7000)</sc" + > "ript>"; > document.body.appendChild(ifr); > }, 7000); > */ > window.setInterval(function() { > var ifr = document.createElement('iframe'); > ifr.srcdoc = "<body>result result<script>window.setTimeout(function() { > document.write('cleared');}, 3000)</sc" + "ript>"; > document.body.appendChild(ifr); > }, 2000); > > </script> > </body> > </html> I was able to repro this. It's this DCHECK in DocumentMarkerController.cpp: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Looks like there's an attempt to clear markers when there are already no markers. This probably happened because there WERE matches in a frame that showed "result result", but then it was changed to "cleared" so there were no markers left. After talking to dcheng@ and roulan@, it looks like this is a reasonable case, and the DCHECK should be changed to an early out. I was able to get this to repro (not with your test page, but with a more direct test) on ToT as well, so it is a separate bug from this CL. I will file a bug and fix in a separate CL. Bug filed: https://bugs.chromium.org/p/chromium/issues/detail?id=615160 https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:254: contents_->ForEachFrame(base::Bind( On 2016/05/23 21:59:16, ncarter wrote: > We should skip !IsRenderFrameLive() frames. > > SendToAllFrames does this automatically. Or, you can consider using a loop here > (over FrameTree::Nodes()) instead of ForEach() and a helper function. I've removed SendGetNearestFindResultIPC() and just do what it did in a loop through FrameTree::Nodes(). https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:281: contents_->ForEachFrame(base::Bind( On 2016/05/23 21:59:16, ncarter wrote: > We should skip !IsRenderFrameLive() frames. > > SendToAllFrames does this automatically. Or, you can consider using a loop here > (over FrameTree::Nodes()) instead of ForEach() and a helper function. I've removed SendFindMatchRectsIPC(). I'll just do what it did in a loop through FrameTree::Nodes(). https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:307: void FindRequestManager::FrameDeleted(RenderFrameHost* rfh) { On 2016/05/23 21:59:16, ncarter wrote: > Usually if you care about FrameDeleted (which is what happens when an FTN goes > away), you care about RenderFrameHostChanged too (which is what happens when an > FTN navigates to a different process, changing the value of current_frame_host() > for the node). > > Here I think you probably need overrides for all three: > - RenderFrameDeleted to handle the case that a subframe crashes. > - FrameDeleted to handle a case that the parent document deletes its iframe > element > - RenderFrameHostChanged to handle cross-process navigation (just RemoveFrame > on (old_frame)). I wasn't aware of that case; thanks for catching that! Done. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:309: } On 2016/05/23 21:59:16, ncarter wrote: > Based on my manual testing of this patch, it seems like we are taking a slight > regression vs TOT, in the following two behaviors: > > - When a new iframe is added during the course of the session, we won't descend > into it. > > - If an iframe exists at the beginning of the session, but initially has no > matches, and then later its document is updated to have matches, we will never > visit those matches. Even if you delete and re-type the last character of the > search text, it seems like there's a case where we skip over a frame with > matches in it. > > Can these be reasonably fixed? The following html snippet demonstrates the above > problems. > > <script> > window.setInterval(function() { > document.body.appendChild(document.createTextNode('result'));}, 5000); > > window.setInterval(function() { > var ifr = document.createElement('iframe'); > ifr.srcdoc = "<script>window.setInterval(function() { > document.body.appendChild(document.createTextNode('result'));}, 7000)</sc" + > "ript>"; > document.body.appendChild(ifr); > }, 7000); > > </script> > > For the first case: As a kind of dynamic content change, this is something that is not handled in all cases on ToT or in my implementation. When adding a frame mid-session, neither ToT nor my implementation will search it. What I think you're seeing is if after adding the new frame you then "find next" past the end of a previous frame, the frame then gets searched. This only happens because of a very recent addition to find-in-page that triggers a new search of the whole page if a new active match is discovered that isn't already in the list of scoped matches. This only happens when a "find next" runs into it on the way to another match (because the whole page is not searched during a "find next" operation). I didn't plan for this functionality initially since it wasn't around when I started, but I have included it now. The only reason you're not seeing it working with my CL in this case is because on ToT, the whole page including text in iframes is treated as one search space. In my implementation, each frame is essentially its own search space, so text in new frames just isn't hit during "find next" operations, and so that new functionality doesn't come into effect. Since this kind of case is not officially supported by find-in-page now, and because the "best effort" partial solution is very new, I would be okay with leaving this case out in this CL (I feel it's just a bit too much to add in to this already large patch). I would really like to add support for this in a future CL though, including searching new frames right when they're added, which ToT does not do. For the second case: I was able to repro this bug on ToT as well, so it is not something new in my CL. It is definitely a bug though. I wouldn't expect matches to be found when they are added, since that was never supported, but I agree that it is a bug for new text not to get searched when starting new find sessions (such as when deleting the last letter in the find bar). I've noticed that the only way I can get these matches to be searched is to close the find bar (which triggers StopFinding()) and then search again. I'm not sure what's causing this bug yet, but it is somewhere in Blink (not in my code). I will try to make a more concise test page and file a bug for this. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:354: contents_->ForEachFrame(base::Bind(&FindRequestManager::AddFrame, On 2016/05/23 21:59:16, ncarter wrote: > We should skip !IsRenderFrameLive() frames. > > You can consider using a loop here (over FrameTree::Nodes()) instead of > ForEach() and a helper function. I'll use a loop over FrameTree::Nodes() here too. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... File content/browser/find_request_manager.h (right): https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.h:21: namespace content { On 2016/05/23 21:59:17, ncarter wrote: > I patched this in. It works great and it handled everything I threw at it, > including using devtools to delete random nodes from > file:///C:/src/cr/src/content/test/data/cross_site_iframe_factory.html?a(a(a(a(a,a,a,a),a(a,a,a,a),a(a,a,a,a),a(a,a,a,a)),a(a(a,a,a,a),a(a,a,a,a),a(a,a,a,a),a(a,a,a,a)),a(a(a,a,a,a),a(a,a,a,a),a(a,a,a,a),a(a,a,a,a)),a),a(a(a(a,a,a,a),a(a,a,a,a),a(a,a,a,a),a(a,a,a,a)),a(a(a,a,a,a),a(a,a,a,a),a(a,a,a,a),a(a,a,a,a)),a(a(a,a,a,a),a(a,a,a,a),a(a,a,a,a),a(a,a,a,a)),a),a(a(a(a,a,a,a),a(a,a,a,a),a(a,a,a,a),a(a,a,a,a)),a(a(a,a,a,a),a(a,a,a,a),a(a,a,a,a),a(a,a,a,a)),a(a(a,a,a,a),a(a,a,a,a),a(a,a,a,a),a(a,a,a,a)),a),a(a(a(a,a,a,a),a(a,a,a,a),a(a,a,a,a),a(a,a,a,a)),a(a(a,a,a,a),a(a,a,a,a),a(a,a,a,a),a(a,a,a,a)),a(a(a,a,a,a),a(a,a,a,a),a(a,a,a,a),a(a,a,a,a)),a)) > > Nice work. Thanks! https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.h:34: using FrameKey = std::pair<int, int>; On 2016/05/23 21:59:17, ncarter wrote: > Some places have started to use content/browser/loader/global_routing_id.h for > this. > > Did you consider simply using RenderFrameHostImpl* ptrs as keys instead? It > seems like you are cleaning up your FrameKeys at RenderFrameDeleted time anyway, > so you don't need to worry about use-after-free. I'm usually wary of holding onto pointers, but since I'm using a WebContentsObserver to ensure the pointers are good anyways, I agree that I might as well use them. :) I've removed the use of FrameKey and switched them all to RenderFrameHost* pointers. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.h:226: using RectData = std::pair<int, std::vector<gfx::RectF>>; On 2016/05/23 21:59:16, ncarter wrote: > Sometimes we need a pair as a key for a data structure (so that comparison is > defined, a la FrameKey), but I don't see a good reason to ever use it like this. > Structs with named members produce much more readable, and greppable, code. > > Please define a struct for this. Done. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.h:256: std::set<FrameKey> pending_replies_; On 2016/05/23 21:59:17, ncarter wrote: > Consider making this a std::unordered_set. Done. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.h:260: // kInvalidId otherwise. On 2016/05/23 21:59:16, ncarter wrote: > This member is somewhat confusing to me. It is a request ID, but the name > suggests that its value would be either a match ordinal or a bool. It was originally a bool, representing whether the FindRequestManager was still waiting for the active match ordinal to be updated. Later on, it became necessary to specifically keep track of which request still needed a reply with an active match. This was before |current_request_.id| existed. I believe now |pending_active_match_ordinal_| will always either be the same as it or kInvalidId, so I can make it a bool again now. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.h:261: int pending_active_match_ordinal_; On 2016/05/23 21:59:17, ncarter wrote: > Under what circumstances can pending_active_match_ordinal_ ever be a different > value from current_request_.id? > > If current_request_.id can advance past pending_active_match_ordinal_, why is it > important that we preserve the older pending_active_match_ordinal_ request id? It won't be different anymore. I'll change this back to a bool. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.h:267: std::map<FrameKey, int> matches_per_frame_; On 2016/05/23 21:59:17, ncarter wrote: > I see four different data structures keyed by FrameKey. > > Would it be worth merging these into a single std::map<FrameKey, FrameState>, > where FrameState is a struct or class like: > > bool is_pending_reply_; > bool is_pending_match_rects_reply_; > int match_count_; > RectData match_rects_data_; > > ? My preference is for whatever is most readable. > > If you do merge them, your usage pattern for all these data structures looks > like a better fit for unordered_map, since you never iterate over the > collection. I think I like these better separately, because some of the pending replies sets and other maps are associated with the separate android-specific operations (and currently in structs related to those), and also because I think the way the pending replies are used is cleaner to have them separate (like clearing, or checking if they are empty). I will change the maps into unordered_maps. https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.h:275: std::pair<FrameKey, int> relative_active_match_ordinal_; On 2016/05/23 21:59:17, ncarter wrote: > For the same reasons as the other pair, this would be better as two members. Done.
Patchset #6 (id:200001) has been deleted
Patchset #5 (id:180001) has been deleted
https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/1959183002/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:309: } On 2016/05/30 15:08:22, paulmeyer wrote: > On 2016/05/23 21:59:16, ncarter wrote: > > Based on my manual testing of this patch, it seems like we are taking a slight > > regression vs TOT, in the following two behaviors: > > > > - When a new iframe is added during the course of the session, we won't > descend > > into it. > > > > - If an iframe exists at the beginning of the session, but initially has no > > matches, and then later its document is updated to have matches, we will never > > visit those matches. Even if you delete and re-type the last character of the > > search text, it seems like there's a case where we skip over a frame with > > matches in it. > > > > Can these be reasonably fixed? The following html snippet demonstrates the > above > > problems. > > > > <script> > > window.setInterval(function() { > > document.body.appendChild(document.createTextNode('result'));}, 5000); > > > > window.setInterval(function() { > > var ifr = document.createElement('iframe'); > > ifr.srcdoc = "<script>window.setInterval(function() { > > document.body.appendChild(document.createTextNode('result'));}, 7000)</sc" + > > "ript>"; > > document.body.appendChild(ifr); > > }, 7000); > > > > </script> > > > > > > For the first case: > > As a kind of dynamic content change, this is something that is not handled in > all cases on ToT or in my implementation. When adding a frame mid-session, > neither ToT nor my implementation will search it. What I think you're seeing is > if after adding the new frame you then "find next" past the end of a previous > frame, the frame then gets searched. This only happens because of a very recent > addition to find-in-page that triggers a new search of the whole page if a new > active match is discovered that isn't already in the list of scoped matches. > This only happens when a "find next" runs into it on the way to another match > (because the whole page is not searched during a "find next" operation). > > I didn't plan for this functionality initially since it wasn't around when I > started, but I have included it now. The only reason you're not seeing it > working with my CL in this case is because on ToT, the whole page including text > in iframes is treated as one search space. In my implementation, each frame is > essentially its own search space, so text in new frames just isn't hit during > "find next" operations, and so that new functionality doesn't come into effect. > > Since this kind of case is not officially supported by find-in-page now, and > because the "best effort" partial solution is very new, I would be okay with > leaving this case out in this CL (I feel it's just a bit too much to add in to > this already large patch). I would really like to add support for this in a > future CL though, including searching new frames right when they're added, which > ToT does not do. > > > For the second case: > > I was able to repro this bug on ToT as well, so it is not something new in my > CL. It is definitely a bug though. > > I wouldn't expect matches to be found when they are added, since that was never > supported, but I agree that it is a bug for new text not to get searched when > starting new find sessions (such as when deleting the last letter in the find > bar). I've noticed that the only way I can get these matches to be searched is > to close the find bar (which triggers StopFinding()) and then search again. > > I'm not sure what's causing this bug yet, but it is somewhere in Blink (not in > my code). I will try to make a more concise test page and file a bug for this. More info about second case: This actually isn't a bug. This behavior is documented: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... "// Determines whether the scoping effort is required for a particular frame. // It is not necessary if the frame is invisible, for example, or if this // is a repeat search that already returned nothing last time the same prefix // was searched. bool shouldScopeMatches(const WTF::String& searchText);" So basically, if the frame was already searched and had no matches, if you try to search it again for a string with the same prefix, it won't bother trying to search because it assumes nothing will be there (dynamic content not supported).
lgtm with two fixes https://codereview.chromium.org/1959183002/diff/220001/content/browser/find_r... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/1959183002/diff/220001/content/browser/find_r... content/browser/find_request_manager.cc:217: // A reply should not be expected from the removed frame. What if activate_.nearest_frame == rfh at this point? I think you need to clear that ptr value out too. https://codereview.chromium.org/1959183002/diff/220001/content/browser/find_r... content/browser/find_request_manager.cc:376: for (FrameTreeNode* node : contents_->GetFrameTree()->Nodes()) This iteration may include nodes for which IsRenderFrameLive() is false. Need to filter those out.
Patchset #6 (id:240001) has been deleted
https://codereview.chromium.org/1959183002/diff/220001/content/browser/find_r... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/1959183002/diff/220001/content/browser/find_r... content/browser/find_request_manager.cc:217: // A reply should not be expected from the removed frame. On 2016/05/31 21:07:31, ncarter wrote: > What if activate_.nearest_frame == rfh at this point? I think you need to clear > that ptr value out too. I think it actually works without clearing it here because it'll definitely be checked before it's used next. I think it's still a good idea to clear it here though, just so that it isn't hanging around in the mean time. https://codereview.chromium.org/1959183002/diff/220001/content/browser/find_r... content/browser/find_request_manager.cc:376: for (FrameTreeNode* node : contents_->GetFrameTree()->Nodes()) On 2016/05/31 21:07:31, ncarter wrote: > This iteration may include nodes for which IsRenderFrameLive() is false. Need to > filter those out. Oh, I misunderstood. I thought you were suggesting that iterating through Nodes() would only hit live nodes. I'll add checks for non-live nodes.
Patchset #6 (id:260001) has been deleted
https://codereview.chromium.org/1959183002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/1959183002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1988: executeCommand(WebString::fromUTF8("Unselect")); Curious -- why is this necessary now? Why wasn't it necessary before? https://codereview.chromium.org/1959183002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1998: if (!doc.isNull()) { It's not possible for a frame to be focused and not have a document, is it? https://codereview.chromium.org/1959183002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2001: element.simulateClick(); Huh. This is also new code? Why do we need to do this now?
LGTM with nits. Ignore my earlier comments, didn't have a chance to look over the whole patch until just now. Also, in the future, consider landing smaller patches :P https://codereview.chromium.org/1959183002/diff/280001/content/browser/find_r... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/1959183002/diff/280001/content/browser/find_r... content/browser/find_request_manager.cc:17: FrameTreeNode* GetDeepestLastChild(FrameTreeNode* node) { Can this live in some common FrameTreeNode place? Seems like this machinery needs to be (and be maintained) next to FrameTreeNode code? https://codereview.chromium.org/1959183002/diff/280001/content/renderer/rende... File content/renderer/render_frame_impl.cc (left): https://codereview.chromium.org/1959183002/diff/280001/content/renderer/rende... content/renderer/render_frame_impl.cc:5104: if (action == STOP_FIND_ACTION_ACTIVATE_SELECTION) { Ohhh, I see you just moved the code from here!
https://codereview.chromium.org/1959183002/diff/280001/content/browser/find_r... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/1959183002/diff/280001/content/browser/find_r... content/browser/find_request_manager.cc:17: FrameTreeNode* GetDeepestLastChild(FrameTreeNode* node) { On 2016/06/02 04:48:57, dglazkov wrote: > Can this live in some common FrameTreeNode place? Seems like this machinery > needs to be (and be maintained) next to FrameTreeNode code? Yeah, it can, and I agree that it should. I had a similar discussion with nick@ about it. I plan to change the iteration order over FrameTree::Nodes() to be the same as the find-in-page search order, for consitency. I'm going to wait to move this and some of the other Traversal code out to FrameTreeNode in that patch (coming soon!). https://codereview.chromium.org/1959183002/diff/280001/content/renderer/rende... File content/renderer/render_frame_impl.cc (left): https://codereview.chromium.org/1959183002/diff/280001/content/renderer/rende... content/renderer/render_frame_impl.cc:5104: if (action == STOP_FIND_ACTION_ACTIVATE_SELECTION) { On 2016/06/02 04:48:57, dglazkov wrote: > Ohhh, I see you just moved the code from here! Acknowledged. https://codereview.chromium.org/1959183002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/1959183002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1988: executeCommand(WebString::fromUTF8("Unselect")); On 2016/06/02 01:08:42, dglazkov wrote: > Curious -- why is this necessary now? Why wasn't it necessary before? I saw that you said I could disregard these comments, but I thought I'd explain a bit anyways. This was done before and now, and it's because this is the functionality of StopFindActionClearSelection (and the content-side enum equivalent). You can specify "clear" as the action you want to take when stopping a find session to prevent the active match from remaining selected/highlighted, which is the default behavior. https://codereview.chromium.org/1959183002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1998: if (!doc.isNull()) { On 2016/06/02 01:08:42, dglazkov wrote: > It's not possible for a frame to be focused and not have a document, is it? I'm actually not sure. It was checked before, so I left the check in. It might be a candidate for removal in a cleanup CL. https://codereview.chromium.org/1959183002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:2001: element.simulateClick(); On 2016/06/02 01:08:42, dglazkov wrote: > Huh. This is also new code? Why do we need to do this now? This is the functionality of the "activate" action. When a find session is stopped with that specified action, a click is simulated on the active match to activate it.
ipc lgtm
paulmeyer@chromium.org changed reviewers: + rbyers@chromium.org
+rbyers@ for OWNER review of test_runner_for_specific_view.cc
On 2016/06/02 18:02:34, paulmeyer wrote: > +rbyers@ for OWNER review of test_runner_for_specific_view.cc test_runner LGTM
The CQ bit was checked by paulmeyer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1959183002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1959183002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Multi-Process Find-in-Page. This patch completes the implementation of multi-process find-in-page, as described in the design doc: https://docs.google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zhodPxMOxa8Bg4W... This patch relies on another issue: https://codereview.chromium.org/1500973002/ BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Multi-Process Find-in-Page. This patch completes the implementation of multi-process find-in-page, as described in the design doc: https://docs.google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zhodPxMOxa8Bg4W... BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by paulmeyer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lfg@chromium.org, nick@chromium.org, dcheng@chromium.org, dglazkov@chromium.org, rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/1959183002/#ps340001 (title: "Disabled tests on Android Release because of crbug.com/615291.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1959183002/340001
Message was sent while issue was closed.
Description was changed from ========== Multi-Process Find-in-Page. This patch completes the implementation of multi-process find-in-page, as described in the design doc: https://docs.google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zhodPxMOxa8Bg4W... BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Multi-Process Find-in-Page. This patch completes the implementation of multi-process find-in-page, as described in the design doc: https://docs.google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zhodPxMOxa8Bg4W... BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #9 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Multi-Process Find-in-Page. This patch completes the implementation of multi-process find-in-page, as described in the design doc: https://docs.google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zhodPxMOxa8Bg4W... BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Multi-Process Find-in-Page. This patch completes the implementation of multi-process find-in-page, as described in the design doc: https://docs.google.com/document/d/12S_6X2MWWLoyJslajcajL2ELU7zhodPxMOxa8Bg4W... BUG=457440 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/c8cb7cbda8636fe756c84b05be2afa70cd9cd3f5 Cr-Commit-Position: refs/heads/master@{#398186} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/c8cb7cbda8636fe756c84b05be2afa70cd9cd3f5 Cr-Commit-Position: refs/heads/master@{#398186} |
