Make Edit>Speech>Speak Selection work for PDFs on Mac

Make Edit>Speech>Speak Selection work for PDFs on Mac

This makes Speak Selection from the menu work for PDFs on Mac. It also
simplifies the code slightly by creating a function which gets the "full page"
guest if there is one.

BUG=476787
Committed: https://crrev.com/5491de1829c10ec6d2a1e8ed1abea274cde12ab7
Cr-Commit-Position: refs/heads/master@{#325584}

[raymes, 2015-03-30 23:02:03]

raymes@chromium.org changed reviewers:
+ fsamuel@chromium.org

[raymes, 2015-03-30 23:02:04]

Hey Fady, this CL isn't ready to land but I need some help figuring out how to
fix the problem below.

Thanks!

https://codereview.chromium.org/1042743002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/1042743002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:117:
static_cast<WebContentsImpl*>(render_widget_host_view->GetWebContents());
Fady: WebContentsImpl* can't be referenced from here and I'm not sure of the
best way around this. Do you know how this should be done?

[Fady Samuel, 2015-03-31 15:40:23]

https://codereview.chromium.org/1042743002/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/1042743002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:117:
static_cast<WebContentsImpl*>(render_widget_host_view->GetWebContents());
On 2015/03/30 23:02:03, raymes wrote:
> Fady: WebContentsImpl* can't be referenced from here and I'm not sure of the
> best way around this. Do you know how this should be done?

How about adding GetFullPageGuest(embedder) to BrowserPluginGuestManager?

https://codereview.chromium.org/1042743002/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:122:
content::BrowserPluginGuest* guest = embedder->GetFullPageGuest();
How about adding GetFullPageGuest(embedder) to BrowserPluginGuestManager?

[raymes, 2015-04-01 05:05:40]

raymes@chromium.org changed reviewers:
+ sammc@chromium.org

[raymes, 2015-04-01 05:05:40]

Thanks Fady! That did the trick. PTAL

[Sam McNally, 2015-04-01 06:04:50]

https://codereview.chromium.org/1042743002/diff/80001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://codereview.chromium.org/1042743002/diff/80001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_embedder.cc:186: return
guest_contents->GetBrowserPluginGuest();
Check for a null guest_contents?

https://codereview.chromium.org/1042743002/diff/80001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

https://codereview.chromium.org/1042743002/diff/80001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_embedder.h:70: BrowserPluginGuest*
GetFullPageGuest();
Comment?

https://codereview.chromium.org/1042743002/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1042743002/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2564: BrowserPluginGuest*
guest = browser_plugin_embedder_->GetFullPageGuest();
Is this the right behavior for all guest types?

[Fady Samuel, 2015-04-01 21:54:47]

lgtm once comments are addressed.

[Fady Samuel, 2015-04-01 21:55:14]

https://chromiumcodereview.appspot.com/1042743002/diff/80001/content/public/b...
File content/public/browser/browser_plugin_guest_manager.h (right):

https://chromiumcodereview.appspot.com/1042743002/diff/80001/content/public/b...
content/public/browser/browser_plugin_guest_manager.h:33: virtual WebContents*
GetFullPageGuest(WebContents* embedder_web_contents);
nit: Add comment as to what this does.

[raymes, 2015-04-13 11:00:30]

raymes@chromium.org changed reviewers:
+ asvitkine@chromium.org, jam@chromium.org

[raymes, 2015-04-13 11:00:31]

+jam@ for OWNERS for
content/browser/web_contents/web_contents_impl.cc
content/public/browser/browser_plugin_guest_manager.*

+asvitkine@chromium.org for
content/browser/renderer_host/render_widget_host_view_mac.mm

https://codereview.chromium.org/1042743002/diff/80001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_embedder.cc (right):

https://codereview.chromium.org/1042743002/diff/80001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_embedder.cc:186: return
guest_contents->GetBrowserPluginGuest();
On 2015/04/01 06:04:49, Sam McNally wrote:
> Check for a null guest_contents?

Done.

https://codereview.chromium.org/1042743002/diff/80001/content/browser/browser...
File content/browser/browser_plugin/browser_plugin_embedder.h (right):

https://codereview.chromium.org/1042743002/diff/80001/content/browser/browser...
content/browser/browser_plugin/browser_plugin_embedder.h:70: BrowserPluginGuest*
GetFullPageGuest();
On 2015/04/01 06:04:49, Sam McNally wrote:
> Comment?

Done.

https://codereview.chromium.org/1042743002/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1042743002/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:2564: BrowserPluginGuest*
guest = browser_plugin_embedder_->GetFullPageGuest();
On 2015/04/01 06:04:49, Sam McNally wrote:
> Is this the right behavior for all guest types?

Yes, only the MimeHandlerViewGuest should turn true for Find below, so for other
guests it will drop through to the default behavior

https://codereview.chromium.org/1042743002/diff/80001/content/public/browser/...
File content/public/browser/browser_plugin_guest_manager.h (right):

https://codereview.chromium.org/1042743002/diff/80001/content/public/browser/...
content/public/browser/browser_plugin_guest_manager.h:33: virtual WebContents*
GetFullPageGuest(WebContents* embedder_web_contents);
On 2015/04/01 21:55:14, Fady Samuel wrote:
> nit: Add comment as to what this does.

Done.

[jam, 2015-04-13 14:40:08]

https://codereview.chromium.org/1042743002/diff/100001/content/public/browser...
File content/public/browser/browser_plugin_guest_manager.h (right):

https://codereview.chromium.org/1042743002/diff/100001/content/public/browser...
content/public/browser/browser_plugin_guest_manager.h:36: virtual WebContents*
GetFullPageGuest(WebContents* embedder_web_contents);
this appears to be called only by code in content, so it should be added on the
internal implementation class instead of the public interface

[Alexei Svitkine (slow), 2015-04-13 15:08:16]

Sweet, happy to see this being worked on! Can you associate the change with a
crbug?

https://codereview.chromium.org/1042743002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/1042743002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:114: WebContents*
web_contents = render_widget_host_view->GetWebContents();
Do you need a null check on the return value before using it below?

https://codereview.chromium.org/1042743002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:116:
web_contents->GetBrowserContext()->GetGuestManager();
Also, perhaps a null check on this is needed too?

(For example, seems GetGuestManager() can return null if ENABLE_EXTENSIONS is
false - though I don't know how much we care about that configuration outside of
mobile platforms...)

[raymes, 2015-04-14 00:29:29]

On 2015/04/13 14:40:08, jam wrote:
>
https://codereview.chromium.org/1042743002/diff/100001/content/public/browser...
> File content/public/browser/browser_plugin_guest_manager.h (right):
> 
>
https://codereview.chromium.org/1042743002/diff/100001/content/public/browser...
> content/public/browser/browser_plugin_guest_manager.h:36: virtual WebContents*
> GetFullPageGuest(WebContents* embedder_web_contents);
> this appears to be called only by code in content, so it should be added on
the
> internal implementation class instead of the public interface

jam: The implementation class lives in extensions/ (guest_view_manager.h) so I'm
not quite sure how that would work?

[raymes, 2015-04-14 00:37:03]

Done, thanks!

https://codereview.chromium.org/1042743002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/1042743002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:114: WebContents*
web_contents = render_widget_host_view->GetWebContents();
On 2015/04/13 15:08:15, Alexei Svitkine wrote:
> Do you need a null check on the return value before using it below?

Done.

https://codereview.chromium.org/1042743002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_mac.mm:116:
web_contents->GetBrowserContext()->GetGuestManager();
On 2015/04/13 15:08:15, Alexei Svitkine wrote:
> Also, perhaps a null check on this is needed too?
> 
> (For example, seems GetGuestManager() can return null if ENABLE_EXTENSIONS is
> false - though I don't know how much we care about that configuration outside
of
> mobile platforms...)

Done.

[Sam McNally, 2015-04-14 01:06:48]

lgtm

[Alexei Svitkine (slow), 2015-04-14 17:08:05]

lgtm

[raymes, 2015-04-15 04:32:57]

jam@: I guess browser_plugin_guest_manager.h is basically a delegate. Is it ok
to land this?

[raymes, 2015-04-16 02:34:08]

jam: ping

On Wed, 15 Apr 2015 at 14:32 <raymes@chromium.org> wrote:

> jam@: I guess browser_plugin_guest_manager.h is basically a delegate. Is
> it
> ok
> to land this?
>
> https://codereview.chromium.org/1042743002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jam, 2015-04-16 14:45:30]

sorry for delay, I didn't see the updates (please ping me if I take more than a
few hours next time)
lgtm

https://codereview.chromium.org/1042743002/diff/100001/content/public/browser...
File content/public/browser/browser_plugin_guest_manager.h (right):

https://codereview.chromium.org/1042743002/diff/100001/content/public/browser...
content/public/browser/browser_plugin_guest_manager.h:36: virtual WebContents*
GetFullPageGuest(WebContents* embedder_web_contents);
On 2015/04/13 14:40:07, jam wrote:
> this appears to be called only by code in content, so it should be added on
the
> internal implementation class instead of the public interface

oh I missed that, sorry

[raymes, 2015-04-16 23:18:22]

The CQ bit was checked by raymes@chromium.org

[raymes, 2015-04-16 23:18:22]

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

[commit-bot: I haz the power, 2015-04-16 23:20:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1042743002/120001

[commit-bot: I haz the power, 2015-04-17 02:01:36]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-04-17 02:02:39]

Patchset 7 (id:??) landed as
https://crrev.com/5491de1829c10ec6d2a1e8ed1abea274cde12ab7
Cr-Commit-Position: refs/heads/master@{#325584}