Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2700613003:
Enable find-in-page across GuestViews. (Closed)
|
Created:
3 years, 10 months ago by paulmeyer Modified:
3 years, 8 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, extensions-reviews_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, chromium-apps-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable find-in-page across GuestViews.
This patch enables find-in-page to work across GuestViews, including
WebViews and PDFs, as explained in this design doc:
https://drive.google.com/open?id=1tl1L99oTgqQxolV7jRvDLzFQ9K251rQ_E16mOwB-BuU.
Specifically, this will allow find-in-page to work with embedded PDFs,
which has been a (very) longstanding bug in Chrome.
This patch also cleans up code that was previously used to route find
requests to the guest WebContents in the case of full-page GuestViews.
This shortcut is no longer needed, as this patch implements a more general
solution to traversing frames across all WebContentses during a find
session.
BUG=55421
Review-Url: https://codereview.chromium.org/2700613003
Cr-Commit-Position: refs/heads/master@{#462327}
Committed: https://chromium.googlesource.com/chromium/src/+/9dedb9f32fca0666761f83c405c5959c148ea751
Patch Set 1 #
Total comments: 22
Patch Set 2 : Rebased. #Patch Set 3 : Addressed comments by ncarter@. #Patch Set 4 : Added TODO. #Patch Set 5 : Rebased. #Patch Set 6 : Small fix. #Messages
Total messages: 84 (73 generated)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Description was changed from ========== WIP find-in-page across GuestViews. ========== to ========== WIP DO NOT REVIEW. Enable find-in-page across GuestViews. This patch enables find-in-page to work across GuestViews, including WebViews and PDFs, as explained in this design doc: https://drive.google.com/open?id=1tl1L99oTgqQxolV7jRvDLzFQ9K251rQ_E16mOwB-BuU. Specifically, this will allow find-in-page to work with embedded PDFs, which has been a (very) longstanding bug in Chrome. This patch also cleans up code that was previously used to route find requests to the guest WebContents in the case of full-page GuestViews. This shortcut is no longer needed, as this patch implements a more general solution to traversing frames across all WebContentses during a find session. BUG=55421 ==========
Description was changed from ========== WIP DO NOT REVIEW. Enable find-in-page across GuestViews. This patch enables find-in-page to work across GuestViews, including WebViews and PDFs, as explained in this design doc: https://drive.google.com/open?id=1tl1L99oTgqQxolV7jRvDLzFQ9K251rQ_E16mOwB-BuU. Specifically, this will allow find-in-page to work with embedded PDFs, which has been a (very) longstanding bug in Chrome. This patch also cleans up code that was previously used to route find requests to the guest WebContents in the case of full-page GuestViews. This shortcut is no longer needed, as this patch implements a more general solution to traversing frames across all WebContentses during a find session. BUG=55421 ========== to ========== Enable find-in-page across GuestViews. This patch enables find-in-page to work across GuestViews, including WebViews and PDFs, as explained in this design doc: https://drive.google.com/open?id=1tl1L99oTgqQxolV7jRvDLzFQ9K251rQ_E16mOwB-BuU. Specifically, this will allow find-in-page to work with embedded PDFs, which has been a (very) longstanding bug in Chrome. This patch also cleans up code that was previously used to route find requests to the guest WebContents in the case of full-page GuestViews. This shortcut is no longer needed, as this patch implements a more general solution to traversing frames across all WebContentses during a find session. BUG=55421 ==========
paulmeyer@chromium.org changed reviewers: + nick@chromium.org
Description was changed from ========== Enable find-in-page across GuestViews. This patch enables find-in-page to work across GuestViews, including WebViews and PDFs, as explained in this design doc: https://drive.google.com/open?id=1tl1L99oTgqQxolV7jRvDLzFQ9K251rQ_E16mOwB-BuU. Specifically, this will allow find-in-page to work with embedded PDFs, which has been a (very) longstanding bug in Chrome. This patch also cleans up code that was previously used to route find requests to the guest WebContents in the case of full-page GuestViews. This shortcut is no longer needed, as this patch implements a more general solution to traversing frames across all WebContentses during a find session. BUG=55421 ========== to ========== Enable find-in-page across GuestViews. This patch enables find-in-page to work across GuestViews, including WebViews and PDFs, as explained in this design doc: https://drive.google.com/open?id=1tl1L99oTgqQxolV7jRvDLzFQ9K251rQ_E16mOwB-BuU. Specifically, this will allow find-in-page to work with embedded PDFs, which has been a (very) longstanding bug in Chrome. This patch also cleans up code that was previously used to route find requests to the guest WebContents in the case of full-page GuestViews. This shortcut is no longer needed, as this patch implements a more general solution to traversing frames across all WebContentses during a find session. BUG=55421 ==========
Description was changed from ========== Enable find-in-page across GuestViews. This patch enables find-in-page to work across GuestViews, including WebViews and PDFs, as explained in this design doc: https://drive.google.com/open?id=1tl1L99oTgqQxolV7jRvDLzFQ9K251rQ_E16mOwB-BuU. Specifically, this will allow find-in-page to work with embedded PDFs, which has been a (very) longstanding bug in Chrome. This patch also cleans up code that was previously used to route find requests to the guest WebContents in the case of full-page GuestViews. This shortcut is no longer needed, as this patch implements a more general solution to traversing frames across all WebContentses during a find session. BUG=55421 ========== to ========== Enable find-in-page across GuestViews. This patch enables find-in-page to work across GuestViews, including WebViews and PDFs, as explained in this design doc: https://drive.google.com/open?id=1tl1L99oTgqQxolV7jRvDLzFQ9K251rQ_E16mOwB-BuU. Specifically, this will allow find-in-page to work with embedded PDFs, which has been a (very) longstanding bug in Chrome. This patch also cleans up code that was previously used to route find requests to the guest WebContents in the case of full-page GuestViews. This shortcut is no longer needed, as this patch implements a more general solution to traversing frames across all WebContentses during a find session. BUG=55421 ==========
Description was changed from ========== Enable find-in-page across GuestViews. This patch enables find-in-page to work across GuestViews, including WebViews and PDFs, as explained in this design doc: https://drive.google.com/open?id=1tl1L99oTgqQxolV7jRvDLzFQ9K251rQ_E16mOwB-BuU. Specifically, this will allow find-in-page to work with embedded PDFs, which has been a (very) longstanding bug in Chrome. This patch also cleans up code that was previously used to route find requests to the guest WebContents in the case of full-page GuestViews. This shortcut is no longer needed, as this patch implements a more general solution to traversing frames across all WebContentses during a find session. BUG=55421 ========== to ========== Enable find-in-page across GuestViews. This patch enables find-in-page to work across GuestViews, including WebViews and PDFs, as explained in this design doc: https://drive.google.com/open?id=1tl1L99oTgqQxolV7jRvDLzFQ9K251rQ_E16mOwB-BuU. Specifically, this will allow find-in-page to work with embedded PDFs, which has been a (very) longstanding bug in Chrome. This patch also cleans up code that was previously used to route find requests to the guest WebContents in the case of full-page GuestViews. This shortcut is no longer needed, as this patch implements a more general solution to traversing frames across all WebContentses during a find session. BUG=55421 ==========
nick@chromium.org changed reviewers: + lfg@chromium.org
I took an incomplete pass, and haven't fully grokked what's going on here yet. But, sharing the comments I have so far. Let's definitely loop in lfg@ for help with the WebContentsTreeNode changes. https://codereview.chromium.org/2700613003/diff/160001/chrome/test/data/exten... File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right): https://codereview.chromium.org/2700613003/diff/160001/chrome/test/data/exten... chrome/test/data/extensions/platform_apps/web_view/shim/main.js:2708: for (var i in success) { You can use Promise.all here to avoid the |success| array, and stop the test early if there's a failure. function testFindInMultipleWebViews() { var webviews = [new WebView(), new WebView(), new WebView()]; var promises = []; // Search in all WebViews simultaneously. for (var i in webviews) { var webview = webviews[i]; webview.src = testFindPage; promises[i] = new Promise((resolve, reject) => { webview.addEventListener('loadstop', (event) => { webview.find("dog", {}, (results_a) => { embedder.test.assertEq(results_a.numberOfMatches, 100); embedder.test.assertTrue(results_a.selectionRect.width > 0); embedder.test.assertTrue(results_a.selectionRect.height > 0); // Test finding next active matches. webview.find("dog"); webview.find("dog"); webview.find("dog"); webview.find("dog"); webview.find("dog", {}, (results_b) => { embedder.test.assertEq(results_b.activeMatchOrdinal, 6); LOG("Searched WebView " + i + " successfully."); resolve(); }); }); }); }); document.body.appendChild(webviews[i]); } Promise.all(promises) .then(() => { LOG("All searches finished."); embedder.test.succeed(); }) .catch((error) => { LOG("Failing test."); embedder.test.fail(error); }); } https://codereview.chromium.org/2700613003/diff/160001/content/browser/find_r... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/2700613003/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:33: children[i] = node->child_at(i); Do we really need these in the list? GetNextSibling() and GetPreviousSibling() seem like they don't need them, since they use node->NodeSibling(). https://codereview.chromium.org/2700613003/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:40: for (auto* inner_contents : contents->GetWebContentsAndAllInner()) { Isn't GetWebContentsAndAllInner recursive? If so it seems like a mismatch to call something that enumerates all descendents recursively, when we're really just trying to enumerate the next level of the tree? https://codereview.chromium.org/2700613003/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:91: int node_id = WebContentsImpl::FromFrameTreeNode(node)-> int node_id = contents->GetOuterDelegateFrameTreeNodeId(); https://codereview.chromium.org/2700613003/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:97: return outer_contents->GetMainFrame()->frame_tree_node(); Going straight to the MainFrame looks unusual to me; what case is it for? I'd like to have lfg@ review this, since my instincts are not good with the inner/outer/guest stuff. https://codereview.chromium.org/2700613003/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:116: // "it != children.end()". It seems like this could also happen if we hit the line-97 case for a frame that was actually embedded by a subframe in the outer WebContents? https://codereview.chromium.org/2700613003/diff/160001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2700613003/diff/160001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:447: if (contents->node_.OuterContentsFrameTreeNode() == frame) Might be faster to do this? auto ftn_id = frame->frame_tree_node_id(); for (WebContentsImpl* contents : inner_web_contents_) { if (contents->node_.outer_contents_frame_tree_node_id() == ftn_id) return contents; } https://codereview.chromium.org/2700613003/diff/160001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:1918: } Is this an orthogonal bugfix? What implications does it have beyond find-in-page? https://codereview.chromium.org/2700613003/diff/160001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/2700613003/diff/160001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:891: typedef std::unordered_map<int, WebContentsTreeNode*> ChildrenMap; This looks unused. https://codereview.chromium.org/2700613003/diff/160001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:909: // Fills |all_contents| with all inner WebContentsTreeNodes' WebContentses. With this class comment, I would not expect that the implementation is actually recursive. It looks like it ought to just describe the one level of the tree. Could we instead just expose a const getter to expose the inner_web_contents_ vector?
On 2017/03/27 23:21:37, ncarter wrote: > I took an incomplete pass, and haven't fully grokked what's going on here yet. > But, sharing the comments I have so far. > > Let's definitely loop in lfg@ for help with the WebContentsTreeNode changes. I've spent quite a bit of time looking through the patch and thinking about this. There are two main reasons to expose the list of: 1. To traverse the inner WebContents' tree from the outer WebContents, i.e. traverse from root to leaves. 2. To observe all the WebContents through a WebContents observer. We are also adding a lot of glue code that special cases the inner/outer WebContents traversal. I believe we can simplify this. I propose adding a inner_frame_tree_node_id_ to FrameTreeNode, so that inner contents tree traversal can be handled from FrameTree/FrameTreeNode. Then we could possibly add a tree traversal helper class to FrameTree/FrameTreeNode that would take a boolean in order to traverse inner WebContents (or possibly refactor FrameTree::NodeRange in order to accomplish this). This won't fix the observer though, but the implementation in this CL is already broken -- it won't account for new inner WebContents, and when an inner WebContents is destroyed we'll be left with a partially broken FindRequestManager::FrameObserver (it won't crash since the WebContents will never call the observers, but we are left with a dangling pointer to a deleted WebContents). I think we can fix this by exposing FrameCreated/FrameSwapped through FrameTreeNode::Observer. What do you think? Perhaps we could have a quick VC to discuss this together with Nick.
Patchset #2 (id:170001) has been deleted
ncarter: PTAL. lfg: I like the idea of doing traversal from FrameTreeNode, but after thinking over this more, I think the amount of changes that are needed to make that work well are outside the scope of this CL. What I's really like long-term is for all traversal to be possible from within FrameTreeNode, but while traversing outwards is still done through WebContents, and all traversal regarding BrowserPlugin will still have to be done through WebContents, I think it will be cleaner for now to leave the traversale to WebContents. I think once GuestViews can be moved fully off of BrowserPlugin, that would be the right time to transition the cross-guest frame traversal to be entirely handled by FrameTreeNode. We may want to do that VC though if we want to discuss this more. What do you think, Nick? Regarding the possibility of dangling WebContentsObservers, when I was implementing this, I had in mind that it would be very unlikely for guests to be added/removed during a find session; certainly much rarer than for frames in general. That is why I didn't think it would need to be explicitly handled. Also, if one is removed, I don't think it will cause anything wrong to happen (find and find results will still work and be updated correctly), and all the observers will be cleared as soon as the user ends the search, types one more letter, searches for something else, etc. I'm not sure if it is worth expanding any observers to handle this specific case. https://codereview.chromium.org/2700613003/diff/160001/chrome/test/data/exten... File chrome/test/data/extensions/platform_apps/web_view/shim/main.js (right): https://codereview.chromium.org/2700613003/diff/160001/chrome/test/data/exten... chrome/test/data/extensions/platform_apps/web_view/shim/main.js:2708: for (var i in success) { On 2017/03/27 23:21:36, ncarter wrote: > You can use Promise.all here to avoid the |success| array, and stop the test > early if there's a failure. > > function testFindInMultipleWebViews() { > var webviews = [new WebView(), new WebView(), new WebView()]; > var promises = []; > > // Search in all WebViews simultaneously. > for (var i in webviews) { > var webview = webviews[i]; > webview.src = testFindPage; > promises[i] = new Promise((resolve, reject) => { > webview.addEventListener('loadstop', (event) => { > webview.find("dog", {}, (results_a) => { > embedder.test.assertEq(results_a.numberOfMatches, 100); > embedder.test.assertTrue(results_a.selectionRect.width > 0); > embedder.test.assertTrue(results_a.selectionRect.height > 0); > > // Test finding next active matches. > webview.find("dog"); > webview.find("dog"); > webview.find("dog"); > webview.find("dog"); > webview.find("dog", {}, (results_b) => { > embedder.test.assertEq(results_b.activeMatchOrdinal, 6); > LOG("Searched WebView " + i + " successfully."); > resolve(); > }); > }); > }); > }); > document.body.appendChild(webviews[i]); > } > > Promise.all(promises) > .then(() => { > LOG("All searches finished."); > embedder.test.succeed(); > }) > .catch((error) => { > LOG("Failing test."); > embedder.test.fail(error); > }); > } Okay, I've replaced the test with your code mostly, except that you can't use the |webview| and |i| variables the way that you have them captured here (all callbacks will get the same webview in this case), so I've fixed that. https://codereview.chromium.org/2700613003/diff/160001/content/browser/find_r... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/2700613003/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:33: children[i] = node->child_at(i); On 2017/03/27 23:21:36, ncarter wrote: > Do we really need these in the list? > > GetNextSibling() and GetPreviousSibling() seem like they don't need them, since > they use node->NodeSibling(). They may not be used in those functions, but are needed in GetFirstChild() and GetLastChild(), for example. https://codereview.chromium.org/2700613003/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:40: for (auto* inner_contents : contents->GetWebContentsAndAllInner()) { On 2017/03/27 23:21:37, ncarter wrote: > Isn't GetWebContentsAndAllInner recursive? If so it seems like a mismatch to > call something that enumerates all descendents recursively, when we're really > just trying to enumerate the next level of the tree? > I've changed this to just check the next level. https://codereview.chromium.org/2700613003/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:91: int node_id = WebContentsImpl::FromFrameTreeNode(node)-> On 2017/03/27 23:21:36, ncarter wrote: > int node_id = contents->GetOuterDelegateFrameTreeNodeId(); Done. https://codereview.chromium.org/2700613003/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:97: return outer_contents->GetMainFrame()->frame_tree_node(); On 2017/03/27 23:21:37, ncarter wrote: > Going straight to the MainFrame looks unusual to me; what case is it for? > > I'd like to have lfg@ review this, since my instincts are not good with the > inner/outer/guest stuff. The case here is only for the BrowserPluginGuest method of embedding, where a guest WebContents is always considered to belong to the main frame of its embedding (outer) WebContents. I agree that it's a bit weird, but thankfully this kind of stuff will go away once we fully move over to using OOPIF-GuestViews. https://codereview.chromium.org/2700613003/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:116: // "it != children.end()". On 2017/03/27 23:21:37, ncarter wrote: > It seems like this could also happen if we hit the line-97 case for a frame that > was actually embedded by a subframe in the outer WebContents? I don't believe that contributes to this. For the purposes of BrowserPluginGuest guests, the frames should still be consistent, i.e. the main frame of the inner WebContents will be the child of the main frame of the outer WebContents, AND the the outer main frame will be the parent of the inner main frame. https://codereview.chromium.org/2700613003/diff/160001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2700613003/diff/160001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:447: if (contents->node_.OuterContentsFrameTreeNode() == frame) On 2017/03/27 23:21:37, ncarter wrote: > Might be faster to do this? > > auto ftn_id = frame->frame_tree_node_id(); > for (WebContentsImpl* contents : inner_web_contents_) { > if (contents->node_.outer_contents_frame_tree_node_id() == ftn_id) > return contents; > } > Agreed. Done. https://codereview.chromium.org/2700613003/diff/160001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:1918: } On 2017/03/27 23:21:37, ncarter wrote: > Is this an orthogonal bugfix? What implications does it have beyond > find-in-page? Actually, I don't think this is even necessary anymore. I'm going to revert this small change. https://codereview.chromium.org/2700613003/diff/160001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/2700613003/diff/160001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:891: typedef std::unordered_map<int, WebContentsTreeNode*> ChildrenMap; On 2017/03/27 23:21:37, ncarter wrote: > This looks unused. Yes, it is. Removed. https://codereview.chromium.org/2700613003/diff/160001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:909: // Fills |all_contents| with all inner WebContentsTreeNodes' WebContentses. On 2017/03/27 23:21:37, ncarter wrote: > With this class comment, I would not expect that the implementation is actually > recursive. It looks like it ought to just describe the one level of the tree. > > Could we instead just expose a const getter to expose the inner_web_contents_ > vector? I agree that the use in FindRequestManager::GetChildren() only needs to access the next level of inner WebContents, but the other 4 uses definitely do need to access all inner WebContents recursively, such as when sending the StopFinding IPC to all frames in all WebContents. I will update the comment for this method to make it more clear that it is recursive, and also add an accessor to inner_web_contents_ for the case where only the next level is needed.
lgtm https://codereview.chromium.org/2700613003/diff/160001/content/browser/find_r... File content/browser/find_request_manager.cc (right): https://codereview.chromium.org/2700613003/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:33: children[i] = node->child_at(i); On 2017/03/31 18:31:16, paulmeyer wrote: > On 2017/03/27 23:21:36, ncarter wrote: > > Do we really need these in the list? > > > > GetNextSibling() and GetPreviousSibling() seem like they don't need them, > since > > they use node->NodeSibling(). > > They may not be used in those functions, but are needed in GetFirstChild() and > GetLastChild(), for example. My point was that I think you could implement GetFirstChild() and GetLastChild() in such a way that they didn't need GetChildren() to copy the list of children? E.g. GetFirstChild() would be if (node->child_count() > 0) { return node->child_at(0); } else { auto children = GetInnerContentsChildren(); if (children.empty()) return nullptr; return children.back(); } https://codereview.chromium.org/2700613003/diff/160001/content/browser/find_r... content/browser/find_request_manager.cc:116: // "it != children.end()". On 2017/03/31 18:31:16, paulmeyer wrote: > On 2017/03/27 23:21:37, ncarter wrote: > > It seems like this could also happen if we hit the line-97 case for a frame > that > > was actually embedded by a subframe in the outer WebContents? > > I don't believe that contributes to this. For the purposes of BrowserPluginGuest > guests, the frames should still be consistent, i.e. the main frame of the inner > WebContents will be the child of the main frame of the outer WebContents, AND > the the outer main frame will be the parent of the inner main frame. I see, that makes sense.
lgtm with nit. On 2017/03/31 18:31:16, paulmeyer wrote: > ncarter: PTAL. > > lfg: I like the idea of doing traversal from FrameTreeNode, but after thinking > over this more, I think the amount of changes that are needed to make that work > well are outside the scope of this CL. What I's really like long-term is for all > traversal to be possible from within FrameTreeNode, but while traversing > outwards is still done through WebContents, and all traversal regarding > BrowserPlugin will still have to be done through WebContents, I think it will be > cleaner for now to leave the traversale to WebContents. > > I think once GuestViews can be moved fully off of BrowserPlugin, that would be > the right time to transition the cross-guest frame traversal to be entirely > handled by FrameTreeNode. We may want to do that VC though if we want to discuss > this more. What do you think, Nick? That sounds reasonable, can you file a bug add a TODO to do this in the future, so we can keep track of it? You can block the bug on issue 696722 as well. > Regarding the possibility of dangling WebContentsObservers, when I was > implementing this, I had in mind that it would be very unlikely for guests to be > added/removed during a find session; certainly much rarer than for frames in > general. That is why I didn't think it would need to be explicitly handled. > Also, if one is removed, I don't think it will cause anything wrong to happen > (find and find results will still work and be updated correctly), and all the > observers will be cleared as soon as the user ends the search, types one more > letter, searches for something else, etc. I'm not sure if it is worth expanding > any observers to handle this specific case. Ok.
paulmeyer@chromium.org changed reviewers: + thestig@chromium.org
+thestig@ for review of: chrome/browser/apps/guest_view/web_view_browsertest.cc chrome/browser/chrome_find_request_manager_browsertest.cc chrome/test/data/extensions/platform_apps/web_view/shim/main.js pdf/out_of_process_instance.cc
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
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/v2/patch-status/codereview.chromium.or...
On 2017/04/04 14:45:59, paulmeyer wrote: > +thestig@ for review of: > > chrome/browser/apps/guest_view/web_view_browsertest.cc > chrome/browser/chrome_find_request_manager_browsertest.cc > chrome/test/data/extensions/platform_apps/web_view/shim/main.js > pdf/out_of_process_instance.cc lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by paulmeyer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by paulmeyer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by paulmeyer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by paulmeyer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org, lfg@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2700613003/#ps270001 (title: "Small fix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 270001, "attempt_start_ts": 1491440818284910,
"parent_rev": "53c8920dfe5c6e5798b617454b318692250e9134", "commit_rev":
"9dedb9f32fca0666761f83c405c5959c148ea751"}
Message was sent while issue was closed.
Description was changed from ========== Enable find-in-page across GuestViews. This patch enables find-in-page to work across GuestViews, including WebViews and PDFs, as explained in this design doc: https://drive.google.com/open?id=1tl1L99oTgqQxolV7jRvDLzFQ9K251rQ_E16mOwB-BuU. Specifically, this will allow find-in-page to work with embedded PDFs, which has been a (very) longstanding bug in Chrome. This patch also cleans up code that was previously used to route find requests to the guest WebContents in the case of full-page GuestViews. This shortcut is no longer needed, as this patch implements a more general solution to traversing frames across all WebContentses during a find session. BUG=55421 ========== to ========== Enable find-in-page across GuestViews. This patch enables find-in-page to work across GuestViews, including WebViews and PDFs, as explained in this design doc: https://drive.google.com/open?id=1tl1L99oTgqQxolV7jRvDLzFQ9K251rQ_E16mOwB-BuU. Specifically, this will allow find-in-page to work with embedded PDFs, which has been a (very) longstanding bug in Chrome. This patch also cleans up code that was previously used to route find requests to the guest WebContents in the case of full-page GuestViews. This shortcut is no longer needed, as this patch implements a more general solution to traversing frames across all WebContentses during a find session. BUG=55421 Review-Url: https://codereview.chromium.org/2700613003 Cr-Commit-Position: refs/heads/master@{#462327} Committed: https://chromium.googlesource.com/chromium/src/+/9dedb9f32fca0666761f83c405c5... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:270001) as https://chromium.googlesource.com/chromium/src/+/9dedb9f32fca0666761f83c405c5...
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:270001) has been created in https://codereview.chromium.org/2808923003/ by paulmeyer@chromium.org. The reason for reverting is: Suspected cause of crashes: https://bugs.chromium.org/p/chromium/issues/detail?id=709478. |
