Issue 698533003: Implement support for closing shadow DOM plugin placeholders.

Issue 698533003: Implement support for closing shadow DOM plugin placeholders. (Closed)

Created:
6 years, 1 month ago by jbroman

Modified:
6 years, 1 month ago

Reviewers:
esprehn, Mike West

CC:
arv+blink, Bernhard Bauer, blink-reviews, blink-reviews-html_chromium.org, Inactive, dglazkov+blink

Base URL:
svn://svn.chromium.org/blink/trunk

Project:
blink

Visibility:
Public.

More Reviews

Description

Implement support for closing shadow DOM plugin placeholders. This is based on the current heuristic in plugins::PluginPlaceholder::HidePlugin (components/plugins/renderer/plugin_placeholder.cc). Tests are added to ensure that the placeholder closing code works as expected, and to verify that the newly-added WebPluginPlaceholder method is respected. A crude UI is currently used; showing the X graphics currently used is blocked on deciding how to load image resources in this environment. BUG=364716 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184852

Patch Set 1 #

Total comments: 9

Patch Set 2 : rebase #

Patch Set 3 : mkwst comments #

Created: 6 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+258 lines, -5 lines)			Patch
A	LayoutTests/fast/plugins/plugin-placeholder-close.html	View		1 chunk	+104 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/plugins/plugin-placeholder-close-expected.txt	View		1 chunk	+23 lines, -0 lines	0 comments	Download
M	LayoutTests/fast/plugins/plugin-placeholder-structured.html	View		1 chunk	+9 lines, -4 lines	0 comments	Download
M	LayoutTests/fast/plugins/plugin-placeholder-structured-expected.html	View		1 chunk	+11 lines, -0 lines	0 comments	Download
M	Source/core/html/shadow/PluginPlaceholderElement.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/html/shadow/PluginPlaceholderElement.cpp	View		1 chunk	+6 lines, -0 lines	0 comments	Download
M	Source/core/html/shadow/PluginPlaceholderElement.idl	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	Source/core/html/shadow/PluginPlaceholderElement.js	View	1 2	1 chunk	+55 lines, -0 lines	0 comments	Download
M	Source/core/plugins/testing/DictionaryPluginPlaceholder.h	View		1 chunk	+4 lines, -0 lines	0 comments	Download
M	Source/web/PluginPlaceholderImpl.cpp	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/web/PluginPlaceholderImplTest.cpp	View	1 2	3 chunks	+39 lines, -1 line	0 comments	Download
M	public/web/WebPluginPlaceholder.h	View		1 chunk	+3 lines, -0 lines	0 comments	Download

Messages

Total messages: 13 (2 generated)

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

jbroman

I'm not thrilled with this heuristic, but it's what we do now and it seems ...

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

jbroman

On 2014/10/31 17:37:09, esprehn wrote: > Can you just use a data url for the ...

6 years, 1 month ago (2014-10-31 17:49:47 UTC) #4

Mike West

Regarding the image resources, I think it's reasonable for private script to be considered part ...

6 years, 1 month ago (2014-11-02 14:01:44 UTC) #5

jbroman

On 2014/11/02 14:01:44, Mike West wrote: > Regarding the image resources, I think it's reasonable ...

6 years, 1 month ago (2014-11-03 16:20:36 UTC) #6

On 2014/11/02 14:01:44, Mike West wrote:
> Regarding the image resources, I think it's reasonable for private script to
be
> considered part of the browser, and to use the same mechanisms for bypassing
CSP
> that other browser features use. A resource scheme certainly fits that mold.

I just need to figure out how to get the security@ thread to proceed, as
nobody's
been responding to it for weeks now. Part of the reason I don't want to block
this
CL on a resolution of that. :)

https://codereview.chromium.org/698533003/diff/1/Source/core/html/shadow/Plug...
File Source/core/html/shadow/PluginPlaceholderElement.js (right):

https://codereview.chromium.org/698533003/diff/1/Source/core/html/shadow/Plug...
Source/core/html/shadow/PluginPlaceholderElement.js:38: closeButton.id =
'plugin-placeholder-close-button';
On 2014/11/02 14:01:44, Mike West wrote:
> Are you sure that only one of these can exist per document?

Shadow roots scope IDs within them; since (except in unit tests) this is always
inside a user agent shadow root, this is fine. I know there will only be one
close button per plugin placeholder.

In particular, the ID map is a property of TreeScope, which is a base class of
both Document and ShadowRoot. And I do know that there will only not be another
#plugin-placeholder-close-button in the same tree scope.

https://codereview.chromium.org/698533003/diff/1/Source/core/html/shadow/Plug...
Source/core/html/shadow/PluginPlaceholderElement.js:39: closeButton.textContent
= 'Close';
On 2014/11/02 14:01:44, Mike West wrote:
> Do we have a localization scheme in place for UI generated by private script?

Not yet, as far as I know. This button may get replaced with an X graphic like
we currently have, which would mitigate the need for a localized label for this
(which is part of the reason I haven't plumbed it through in this CL), at least
until I figure out how to make the accessibility features propagate into the
shadow root (and then I will likely need a localized label once again).

In any event, some resolution will be required before this ships, but I think
that is a logically separate change from this one.

https://codereview.chromium.org/698533003/diff/1/Source/core/html/shadow/Plug...
Source/core/html/shadow/PluginPlaceholderElement.js:68: // 4) Seems to have a
good / low false negative rate at this time.
On 2014/11/02 14:01:44, Mike West wrote:
> Nit: Please add a note here pointing to the heuristic's source. We're likely
to
> update or the other at some point; noting the source is the best we can do to
> prevent skew.

Done. Once this version ships, I expect the other code to go away.

https://codereview.chromium.org/698533003/diff/1/Source/web/PluginPlaceholder...
File Source/web/PluginPlaceholderImplTest.cpp (right):

https://codereview.chromium.org/698533003/diff/1/Source/web/PluginPlaceholder...
Source/web/PluginPlaceholderImplTest.cpp:123: if (closeButton)
On 2014/11/02 14:01:44, Mike West wrote:
> Why the `if` here? When is the close button indeterminate?

This is premature on my part, I suppose. It cannot happen right now, though
PluginPlaceholderElement might in the future not bother creating the button for
non-closeable placeholders.

But that's a bridge we can cross when we get to it, so I'll just assert it as
above.

Mike West

I'll take a closer look at your comments in the morning (sorry, running out the ...

6 years, 1 month ago (2014-11-03 16:35:46 UTC) #7

jbroman

On 2014/11/03 16:35:46, Mike West wrote: > I'll take a closer look at your comments ...

6 years, 1 month ago (2014-11-03 16:43:53 UTC) #8

jbroman

On 2014/11/04 15:10:56, Mike West wrote: > LGTM, thanks for filing the bugs. Thanks. Will ...

6 years, 1 month ago (2014-11-04 20:52:21 UTC) #10

commit-bot: I haz the power

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

6 years, 1 month ago (2014-11-05 07:11:55 UTC) #12

commit-bot: I haz the power

6 years, 1 month ago (2014-11-05 07:15:52 UTC) #13

Message was sent while issue was closed.

Committed patchset #3 (id:40001) as 184852

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