Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(353)

Issue 597753003: Implement find in page support for top level BrowserPlugins. (Closed)

Created:
6 years, 2 months ago by raymes
Modified:
6 years, 2 months ago
CC:
chrome-apps-syd-reviews_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, nasko+codewatch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Implement find in page support for top level BrowserPlugins. Right now BrowserPlugins don't handle find in page. This CL adds support for find in page in MIMEHandlerView BrowserPlugin instances. Find in page is only handled for "full page" plugins, that is when the BrowserPlugin as loaded at the top level. The reason is because supporting find in embedded BrowserPlugins would require recursively searching which is far more complicated to implement and we (jam@, fsamuel@) have decided to defer implementing for the time being. Detecting whether the BrowserPlugin is loaded at the top level requires detecting whether the main frame is a "plugin document" in blink, which needs to be sent to the browser from the renderer. It isn't sufficient to determine whether the BrowserPlugin is merely loaded in the main frame, because it may be <embed>ed inside a html document in the main frame. In that case we don't want find in page to search the document. BUG=303491 Committed: https://crrev.com/0fa0be50a2ca75472c27113b43bd5b3bc42d7629 Cr-Commit-Position: refs/heads/master@{#299358}

Patch Set 1 #

Total comments: 6

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Total comments: 2

Patch Set 6 : #

Patch Set 7 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+132 lines, -7 lines) Patch
M content/browser/browser_plugin/browser_plugin_embedder.h View 1 2 3 4 5 6 2 chunks +11 lines, -2 lines 0 comments Download
M content/browser/browser_plugin/browser_plugin_embedder.cc View 1 2 3 3 chunks +26 lines, -0 lines 0 comments Download
M content/browser/browser_plugin/browser_plugin_guest.h View 1 2 3 4 5 6 2 chunks +8 lines, -0 lines 0 comments Download
M content/browser/browser_plugin/browser_plugin_guest.cc View 1 2 3 4 5 6 3 chunks +9 lines, -0 lines 0 comments Download
M content/browser/web_contents/web_contents_impl.cc View 1 2 3 1 chunk +5 lines, -0 lines 0 comments Download
M content/common/browser_plugin/browser_plugin_messages.h View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M content/public/browser/browser_plugin_guest_delegate.h View 1 2 3 4 5 6 1 chunk +7 lines, -0 lines 0 comments Download
M content/public/browser/browser_plugin_guest_delegate.cc View 1 2 3 1 chunk +7 lines, -0 lines 0 comments Download
M content/renderer/browser_plugin/browser_plugin.cc View 1 2 3 4 5 6 2 chunks +8 lines, -0 lines 0 comments Download
M extensions/browser/api/web_view/web_view_internal_api.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.h View 1 2 3 4 5 6 2 chunks +16 lines, -0 lines 0 comments Download
M extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc View 1 2 3 4 5 2 chunks +28 lines, -0 lines 0 comments Download
M extensions/browser/guest_view/web_view/web_view_guest.h View 1 2 3 4 5 6 1 chunk +3 lines, -3 lines 0 comments Download
M extensions/browser/guest_view/web_view/web_view_guest.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 34 (9 generated)
raymes
Hey Fady, PTAL at this simple approach which only works with the assumption that the ...
6 years, 2 months ago (2014-09-25 06:59:06 UTC) #2
Fady Samuel
https://codereview.chromium.org/597753003/diff/1/content/browser/browser_plugin/browser_plugin_embedder.cc File content/browser/browser_plugin/browser_plugin_embedder.cc (right): https://codereview.chromium.org/597753003/diff/1/content/browser/browser_plugin/browser_plugin_embedder.cc#newcode194 content/browser/browser_plugin/browser_plugin_embedder.cc:194: // within a page? On 2014/09/25 06:59:06, raymes wrote: ...
6 years, 2 months ago (2014-09-25 10:41:29 UTC) #3
Fady Samuel
https://codereview.chromium.org/597753003/diff/1/content/browser/browser_plugin/browser_plugin_embedder.cc File content/browser/browser_plugin/browser_plugin_embedder.cc (right): https://codereview.chromium.org/597753003/diff/1/content/browser/browser_plugin/browser_plugin_embedder.cc#newcode194 content/browser/browser_plugin/browser_plugin_embedder.cc:194: // within a page? On 2014/09/25 06:59:06, raymes wrote: ...
6 years, 2 months ago (2014-09-25 10:41:30 UTC) #4
raymes
Thanks for your help! I think this is closer to what you were thinking but ...
6 years, 2 months ago (2014-09-29 05:01:34 UTC) #5
Fady Samuel
https://codereview.chromium.org/597753003/diff/1/content/browser/browser_plugin/browser_plugin_embedder.cc File content/browser/browser_plugin/browser_plugin_embedder.cc (right): https://codereview.chromium.org/597753003/diff/1/content/browser/browser_plugin/browser_plugin_embedder.cc#newcode194 content/browser/browser_plugin/browser_plugin_embedder.cc:194: // within a page? On 2014/09/29 05:01:34, raymes wrote: ...
6 years, 2 months ago (2014-09-29 12:01:34 UTC) #6
raymes
PTAL!
6 years, 2 months ago (2014-10-01 22:07:10 UTC) #7
raymes
Friendly ping :) On Wed Oct 01 2014 at 4:07:11 PM <raymes@chromium.org> wrote: > PTAL! ...
6 years, 2 months ago (2014-10-02 21:19:14 UTC) #8
Fady Samuel
lgtm with nit. https://codereview.chromium.org/597753003/diff/80001/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc File extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc (right): https://codereview.chromium.org/597753003/diff/80001/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc#newcode175 extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc:175: embedder_web_contents()->GetDelegate()->FindReply(embedder_web_contents(), It might be the case ...
6 years, 2 months ago (2014-10-06 20:03:54 UTC) #9
raymes
https://codereview.chromium.org/597753003/diff/80001/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc File extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc (right): https://codereview.chromium.org/597753003/diff/80001/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc#newcode175 extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc:175: embedder_web_contents()->GetDelegate()->FindReply(embedder_web_contents(), On 2014/10/06 20:03:54, Fady Samuel wrote: > It ...
6 years, 2 months ago (2014-10-06 20:39:42 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/597753003/100001
6 years, 2 months ago (2014-10-06 20:43:14 UTC) #12
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_gn_dbg/builds/5411) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel/builds/16882) win_chromium_x64_rel_swarming ...
6 years, 2 months ago (2014-10-06 21:15:37 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/597753003/120001
6 years, 2 months ago (2014-10-06 21:37:56 UTC) #16
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/15800)
6 years, 2 months ago (2014-10-06 22:23:41 UTC) #18
raymes
+OWNERS jam for content/browser/web_contents/web_contents_impl.cc content/public/browser/browser_plugin_guest_delegate.cc content/public/browser/browser_plugin_guest_delegate.h dcheng for content/common/browser_plugin/browser_plugin_messages.h kalman for extensions/*
6 years, 2 months ago (2014-10-06 22:34:42 UTC) #20
raymes
+OWNERS jam for content/browser/web_contents/web_contents_impl.cc content/public/browser/browser_plugin_guest_delegate.cc content/public/browser/browser_plugin_guest_delegate.h dcheng for content/common/browser_plugin/browser_plugin_messages.h kalman for extensions/*
6 years, 2 months ago (2014-10-06 22:35:23 UTC) #22
raymes
+rockot for extensions/* since kalman is OOO
6 years, 2 months ago (2014-10-06 22:39:58 UTC) #24
not at google - send to devlin
Fady you're an owner of extensions/.../guest_view right? So the only thing my lgtm grants is ...
6 years, 2 months ago (2014-10-07 04:21:44 UTC) #25
Fady Samuel
On 2014/10/07 04:21:44, kalman wrote: > Fady you're an owner of extensions/.../guest_view right? So the ...
6 years, 2 months ago (2014-10-07 05:02:04 UTC) #26
jam
lgtm
6 years, 2 months ago (2014-10-08 22:28:00 UTC) #27
raymes
On 2014/10/08 22:28:00, jam wrote: > lgtm ping dcheng for messages :)
6 years, 2 months ago (2014-10-09 17:51:25 UTC) #28
dcheng
LGTM. It might to add some more detail to the CL description about the approach ...
6 years, 2 months ago (2014-10-13 18:14:30 UTC) #29
raymes
Thanks dcheng! The commit message definitely needed more detail, I forgot to update it from ...
6 years, 2 months ago (2014-10-13 18:52:27 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/597753003/120001
6 years, 2 months ago (2014-10-13 18:53:57 UTC) #32
commit-bot: I haz the power
Committed patchset #7 (id:120001)
6 years, 2 months ago (2014-10-13 20:38:16 UTC) #33
commit-bot: I haz the power
6 years, 2 months ago (2014-10-13 20:39:03 UTC) #34
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/0fa0be50a2ca75472c27113b43bd5b3bc42d7629
Cr-Commit-Position: refs/heads/master@{#299358}

Powered by Google App Engine
This is Rietveld 408576698