Issue 649873006: Enable shadow DOM-based "missing plugin" placeholder.

Issue 649873006: Enable shadow DOM-based "missing plugin" placeholder. (Closed)

Created:
6 years, 1 month ago by jbroman

Modified:
6 years, 1 month ago

Reviewers:
Bernhard Bauer, Lei Zhang, no sievers

CC:
chromium-reviews, creis+watch_chromium.org, nkostylev+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, oshima+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Enable shadow DOM-based "missing plugin" placeholder. This is guarded by a command-line switch. TEST=OverrideCreatePluginTest BUG=364716 Committed: https://crrev.com/73e9f796740df05e414d12a787debb7b2bc0d41c Cr-Commit-Position: refs/heads/master@{#301996}

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : mostly rebase #

Patch Set 4 : missing include (IWYU) #

Patch Set 5 : test for allowed plugin #

Total comments: 6

Patch Set 6 : rebase #

Patch Set 7 : review comments #

Patch Set 8 : missed deleting this in the previous patch set #

Total comments: 6

Patch Set 9 : rebase #

Patch Set 10 : comments #

Patch Set 11 : formatting etc #

Total comments: 17

Patch Set 12 : review comments (thestig, sievers) #

Created: 6 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+297 lines, -14 lines)			Patch
M	chrome/browser/chrome_content_browser_client.cc	View	1 2 3 4 5	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/chromeos/login/chrome_restart_request.cc	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/chrome_renderer.gypi	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	chrome/renderer/chrome_content_renderer_client.h	View	1 2 3 4 5 6	1 chunk	+4 lines, -0 lines	0 comments	Download
M	chrome/renderer/chrome_content_renderer_client.cc	View	1 2 3 4 5 6 7 8 9	3 chunks	+11 lines, -0 lines	0 comments	Download
M	chrome/renderer/chrome_content_renderer_client_browsertest.cc	View	1 2 3 4 5 6 7 8 9 10 11	2 chunks	+118 lines, -0 lines	0 comments	Download
A	chrome/renderer/plugins/shadow_dom_plugin_placeholder.h	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+39 lines, -0 lines	0 comments	Download
A	chrome/renderer/plugins/shadow_dom_plugin_placeholder.cc	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+73 lines, -0 lines	0 comments	Download
M	content/public/renderer/content_renderer_client.h	View	1 2 3 4 5 6	3 chunks	+10 lines, -0 lines	0 comments	Download
M	content/public/renderer/content_renderer_client.cc	View	1 2 3 4 5 6 7 8 9 10 11	7 chunks	+24 lines, -14 lines	0 comments	Download
M	content/renderer/render_frame_impl.h	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+3 lines, -0 lines	0 comments	Download
M	content/renderer/render_frame_impl.cc	View	1 2 3 4 5 6 7 8	2 chunks	+11 lines, -0 lines	0 comments	Download

Messages

Total messages: 16 (3 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

jbroman

This is still guarded by a flag, but it's just the simplest plugin placeholder to ...

6 years, 1 month ago (2014-10-28 17:40:17 UTC) #2

Bernhard Bauer

On 2014/10/28 17:40:17, jbroman wrote: > This is still guarded by a flag, but it's ...

6 years, 1 month ago (2014-10-29 09:53:05 UTC) #3

On 2014/10/28 17:40:17, jbroman wrote:
> This is still guarded by a flag, but it's just the simplest plugin placeholder
> to plumb through to Chrome.
> 
> A few specific questions:
> 
> 1. I'm not sure what the idiomatic way to tell what "kind" of placeholder I
get
> in MissingPluginTest would be. What I've done is a bit of a hack (it assumes
> knowledge that only subclasses of PluginPlaceholderBase will be used, and so
it
> can have methods that distinguish). Another approach would be to look at the
> text of the message. I'm open to ideas.

Yes, I think I like looking at the text of the message better.

> 2. I don't quite understand the naming scheme of ContentRendererClient
methods.
> I'm not convinced "OverrideCreatePluginPlaceholder" is a good name. Should
this
> just be "CreatePluginPlaceholder", or something else?

I agree, the naming is a bit weird here. For OverrideCreatePlugin(), it makes
sense: It's the RenderFrame's job to create the plugin, but the embedder can
provide a different plugin to override that. Here, the RenderFrame on its own
does not actually create a plugin placeholder, so the embedder doesn't actually
override anything; it rather turns the plugin placeholder feature on. So, yeah,
just CreatePluginPlaceholder() seems like a good name.

> 3. I'm loath to add more logic (especially duplicate logic) in
> ChromeContentRendererClient proper. Should this be delegated to a separate
> source file in chrome/renderer/plugins/ or similar?

That might be a good idea. The logic is not going to be duplicate forever
though, right?

> When/if this LGTY, I'll go and ask for review by remaining owners.

https://codereview.chromium.org/649873006/diff/80001/chrome/renderer/chrome_c...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/649873006/diff/80001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.cc:581: return
CreateShadowDOMPlaceholderForMissingPlugin().release();
Urr... if this method passes ownership of its return value anyway, why don't we
make it return a scoped_ptr straight away?

https://codereview.chromium.org/649873006/diff/80001/chrome/renderer/chrome_c...
File chrome/renderer/chrome_content_renderer_client_browsertest.cc (right):

https://codereview.chromium.org/649873006/diff/80001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client_browsertest.cc:82:
~ScopedMockPluginInfoFilter() { sink_.RemoveFilter(this); }
I think this should be override?

https://codereview.chromium.org/649873006/diff/80001/chrome/renderer/plugins/...
File chrome/renderer/plugins/shadow_dom_plugin_placeholder.cc (right):

https://codereview.chromium.org/649873006/diff/80001/chrome/renderer/plugins/...
chrome/renderer/plugins/shadow_dom_plugin_placeholder.cc:18: namespace {
Can you move the anonymous namespace methods to the top of the file?

jbroman

Thanks for the speedy response. On 2014/10/29 09:53:05, Bernhard Bauer wrote: > On 2014/10/28 17:40:17, ...

6 years, 1 month ago (2014-10-29 17:14:44 UTC) #4

Thanks for the speedy response.

On 2014/10/29 09:53:05, Bernhard Bauer wrote:
> On 2014/10/28 17:40:17, jbroman wrote:
> > This is still guarded by a flag, but it's just the simplest plugin
placeholder
> > to plumb through to Chrome.
> > 
> > A few specific questions:
> > 
> > 1. I'm not sure what the idiomatic way to tell what "kind" of placeholder I
> get
> > in MissingPluginTest would be. What I've done is a bit of a hack (it assumes
> > knowledge that only subclasses of PluginPlaceholderBase will be used, and so
> it
> > can have methods that distinguish). Another approach would be to look at the
> > text of the message. I'm open to ideas.
> 
> Yes, I think I like looking at the text of the message better.

Done.

> > 2. I don't quite understand the naming scheme of ContentRendererClient
> methods.
> > I'm not convinced "OverrideCreatePluginPlaceholder" is a good name. Should
> this
> > just be "CreatePluginPlaceholder", or something else?
> 
> I agree, the naming is a bit weird here. For OverrideCreatePlugin(), it makes
> sense: It's the RenderFrame's job to create the plugin, but the embedder can
> provide a different plugin to override that. Here, the RenderFrame on its own
> does not actually create a plugin placeholder, so the embedder doesn't
actually
> override anything; it rather turns the plugin placeholder feature on. So,
yeah,
> just CreatePluginPlaceholder() seems like a good name.

Done.

> > 3. I'm loath to add more logic (especially duplicate logic) in
> > ChromeContentRendererClient proper. Should this be delegated to a separate
> > source file in chrome/renderer/plugins/ or similar?
> 
> That might be a good idea. The logic is not going to be duplicate forever
> though, right?

No, I don't expect it to be duplicated forever. I've tried putting the
new code, at least, in chrome/renderer/plugins/shadow_dom_plugin_placeholder.cc.
It's pretty small right now, but this seems a reasonable enough home to me.
WDYT?

https://codereview.chromium.org/649873006/diff/80001/chrome/renderer/chrome_c...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/649873006/diff/80001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.cc:581: return
CreateShadowDOMPlaceholderForMissingPlugin().release();
On 2014/10/29 09:53:05, Bernhard Bauer wrote:
> Urr... if this method passes ownership of its return value anyway, why don't
we
> make it return a scoped_ptr straight away?

It wasn't clear to me when it should be released from scoped_ptr. Certainly
scoped_ptr doesn't exist in Blink, so it has to be a WebPluginPlaceholder* when
it crosses that boundary. It seemed to me that the convention for in the
content/public/ was to return raw pointers, but I wasn't sure.

It also necessitates completing the definition of blink::WebPluginPlaceholder
for ~scoped_ptr to compile, but such is life.

(as an aside, though, not using smart pointers here has already caused me to
leak memory in my tests -- evidence that they are a good idea)

I'll make it scoped_ptr here and unwrap it in content::RenderFrameImpl, then,
okay? (On the Blink side, it's quickly wrapped in an OwnPtr again.)

https://codereview.chromium.org/649873006/diff/80001/chrome/renderer/chrome_c...
File chrome/renderer/chrome_content_renderer_client_browsertest.cc (right):

https://codereview.chromium.org/649873006/diff/80001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client_browsertest.cc:82:
~ScopedMockPluginInfoFilter() { sink_.RemoveFilter(this); }
On 2014/10/29 09:53:05, Bernhard Bauer wrote:
> I think this should be override?

Done.

https://codereview.chromium.org/649873006/diff/80001/chrome/renderer/plugins/...
File chrome/renderer/plugins/shadow_dom_plugin_placeholder.cc (right):

https://codereview.chromium.org/649873006/diff/80001/chrome/renderer/plugins/...
chrome/renderer/plugins/shadow_dom_plugin_placeholder.cc:18: namespace {
On 2014/10/29 09:53:05, Bernhard Bauer wrote:
> Can you move the anonymous namespace methods to the top of the file?

Done.

Bernhard Bauer

https://codereview.chromium.org/649873006/diff/140001/chrome/renderer/chrome_content_renderer_client.cc File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/649873006/diff/140001/chrome/renderer/chrome_content_renderer_client.cc#newcode556 chrome/renderer/chrome_content_renderer_client.cc:556: const blink::WebPluginParams& orig_params) { TBH, I would even move ...

6 years, 1 month ago (2014-10-29 17:45:54 UTC) #5

jbroman

6 years, 1 month ago (2014-10-29 18:14:40 UTC) #6

Lei Zhang

chrome/ lgtm with some nits https://codereview.chromium.org/649873006/diff/200001/chrome/renderer/chrome_content_renderer_client_browsertest.cc File chrome/renderer/chrome_content_renderer_client_browsertest.cc (right): https://codereview.chromium.org/649873006/diff/200001/chrome/renderer/chrome_content_renderer_client_browsertest.cc#newcode19 chrome/renderer/chrome_content_renderer_client_browsertest.cc:19: #include "content/public/renderer/render_frame_observer.h" not needed? ...

6 years, 1 month ago (2014-10-29 22:12:39 UTC) #10

no sievers

lgtm https://codereview.chromium.org/649873006/diff/200001/content/public/renderer/content_renderer_client.cc File content/public/renderer/content_renderer_client.cc (right): https://codereview.chromium.org/649873006/diff/200001/content/public/renderer/content_renderer_client.cc#newcode38 content/public/renderer/content_renderer_client.cc:38: return NULL; nit: while you are in here.. ...

6 years, 1 month ago (2014-10-29 22:54:04 UTC) #11

jbroman

https://codereview.chromium.org/649873006/diff/200001/chrome/renderer/chrome_content_renderer_client_browsertest.cc File chrome/renderer/chrome_content_renderer_client_browsertest.cc (right): https://codereview.chromium.org/649873006/diff/200001/chrome/renderer/chrome_content_renderer_client_browsertest.cc#newcode19 chrome/renderer/chrome_content_renderer_client_browsertest.cc:19: #include "content/public/renderer/render_frame_observer.h" On 2014/10/29 22:12:39, Lei Zhang wrote: > ...

6 years, 1 month ago (2014-10-29 23:10:48 UTC) #12

commit-bot: I haz the power

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

6 years, 1 month ago (2014-10-29 23:39:55 UTC) #14

commit-bot: I haz the power

6 years, 1 month ago (2014-10-30 00:44:41 UTC) #16

Message was sent while issue was closed.

Patchset 12 (id:??) landed as
https://crrev.com/73e9f796740df05e414d12a787debb7b2bc0d41c
Cr-Commit-Position: refs/heads/master@{#301996}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages