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

Issue 522783002: Add a custom element to own structure of shadow DOM plugin placeholders. (Closed)

Created:
6 years, 3 months ago by jbroman
Modified:
6 years, 3 months ago
CC:
arv+blink, blink-reviews, blink-reviews-html_chromium.org, Inactive, dglazkov+blink
Project:
blink
Visibility:
Public.

Description

Add a custom element to own structure of shadow DOM plugin placeholders. This creates PluginPlaceholderElement, which manages a subtree of elements which represent plugin placeholder content and exposes an interface to manipulate this content. It is expected that this interface will grow to encompass more than a single string in the future. This change also adds an Internals method which uses this, so that its appearance can be covered by layout tests, and adds one such layout test. Ultimately, this class will be manipulated by a glue class which controls this content on behalf of the embedder. BUG=364716 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182014

Patch Set 1 #

Total comments: 14

Patch Set 2 : Blink-in-JS #

Total comments: 12

Patch Set 3 : haraken comments #

Patch Set 4 : rebase (broken) #

Patch Set 5 : fix due to binding changes #

Total comments: 1

Patch Set 6 : plaintext message #

Patch Set 7 : rebase #

Total comments: 10

Patch Set 8 : rebase #

Patch Set 9 : review comments #

Patch Set 10 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+198 lines, -15 lines) Patch
A LayoutTests/fast/plugins/plugin-placeholder-not-exposed.html View 1 1 chunk +8 lines, -0 lines 0 comments Download
A LayoutTests/fast/plugins/plugin-placeholder-not-exposed-expected.txt View 1 1 chunk +12 lines, -0 lines 0 comments Download
A LayoutTests/fast/plugins/plugin-placeholder-structured.html View 1 2 3 4 5 1 chunk +9 lines, -0 lines 0 comments Download
A LayoutTests/fast/plugins/plugin-placeholder-structured-expected.html View 1 2 3 4 5 1 chunk +34 lines, -0 lines 0 comments Download
M Source/core/BUILD.gn View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/core.gypi View 1 2 3 4 5 6 7 8 9 2 chunks +3 lines, -0 lines 0 comments Download
M Source/core/core_generated.gyp View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/html/HTMLPlugInElement.h View 1 chunk +1 line, -1 line 0 comments Download
A Source/core/html/shadow/PluginPlaceholderElement.h View 1 2 3 4 5 6 7 8 1 chunk +26 lines, -0 lines 0 comments Download
A Source/core/html/shadow/PluginPlaceholderElement.cpp View 1 2 3 4 5 6 7 8 1 chunk +33 lines, -0 lines 0 comments Download
A + Source/core/html/shadow/PluginPlaceholderElement.idl View 1 2 3 4 5 6 7 8 1 chunk +4 lines, -3 lines 0 comments Download
M Source/core/html/shadow/PluginPlaceholderElement.js View 1 2 3 4 5 6 7 8 9 1 chunk +38 lines, -7 lines 0 comments Download
M Source/core/testing/Internals.h View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/testing/Internals.cpp View 1 2 3 4 5 6 7 2 chunks +25 lines, -3 lines 0 comments Download
M Source/core/testing/Internals.idl View 1 2 3 4 5 6 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 29 (5 generated)
jbroman
jbroman@chromium.org changed reviewers: + esprehn@chromium.org
6 years, 3 months ago (2014-08-29 15:30:07 UTC) #1
jbroman
6 years, 3 months ago (2014-08-29 15:30:07 UTC) #2
dglazkov
On 2014/08/29 at 15:30:07, jbroman wrote: > Is this a good fit for Blink-in-JS?
6 years, 3 months ago (2014-08-29 16:01:11 UTC) #3
esprehn
I think this just wants to be blink-in-js, but we can do this if you ...
6 years, 3 months ago (2014-08-29 21:13:26 UTC) #4
jbroman
On 2014/08/29 16:01:11, dglazkov wrote: > Is this a good fit for Blink-in-JS? On 2014/08/29 ...
6 years, 3 months ago (2014-08-30 13:55:31 UTC) #5
jbroman
Replying to comments on this patchset nonetheless. https://codereview.chromium.org/522783002/diff/1/LayoutTests/TestExpectations File LayoutTests/TestExpectations (right): https://codereview.chromium.org/522783002/diff/1/LayoutTests/TestExpectations#newcode1093 LayoutTests/TestExpectations:1093: crbug.com/364716 fast/plugins/plugin-placeholder-structured.html ...
6 years, 3 months ago (2014-08-30 13:55:49 UTC) #6
jbroman
Re-implemented on top of Blink-in-JS; PTAL. (It's a pretty similar amount of code, but I ...
6 years, 3 months ago (2014-08-31 12:51:09 UTC) #7
haraken
https://codereview.chromium.org/522783002/diff/20001/Source/core/html/shadow/PluginPlaceholderElement.cpp File Source/core/html/shadow/PluginPlaceholderElement.cpp (right): https://codereview.chromium.org/522783002/diff/20001/Source/core/html/shadow/PluginPlaceholderElement.cpp#newcode32 Source/core/html/shadow/PluginPlaceholderElement.cpp:32: void PluginPlaceholderElement::initializePlaceholderElements() This helper function looks redundant. https://codereview.chromium.org/522783002/diff/20001/Source/core/html/shadow/PluginPlaceholderElement.idl File ...
6 years, 3 months ago (2014-08-31 14:53:34 UTC) #9
jbroman
[+bauerb since one of the comments concerns the embedder interface this will ultimately plug into] ...
6 years, 3 months ago (2014-08-31 16:53:50 UTC) #11
jbroman
On 2014/08/31 16:53:50, jbroman wrote: > On 2014/08/31 14:53:34, haraken wrote: > > > > ...
6 years, 3 months ago (2014-08-31 16:56:21 UTC) #12
jbroman
https://codereview.chromium.org/522783002/diff/20001/Source/core/html/shadow/PluginPlaceholderElement.js File Source/core/html/shadow/PluginPlaceholderElement.js (right): https://codereview.chromium.org/522783002/diff/20001/Source/core/html/shadow/PluginPlaceholderElement.js#newcode44 Source/core/html/shadow/PluginPlaceholderElement.js:44: set: function(html) { this.messageElement.innerHTML = html; }, On 2014/08/31 ...
6 years, 3 months ago (2014-08-31 19:00:51 UTC) #13
haraken
https://codereview.chromium.org/522783002/diff/20001/Source/core/html/shadow/PluginPlaceholderElement.idl File Source/core/html/shadow/PluginPlaceholderElement.idl (right): https://codereview.chromium.org/522783002/diff/20001/Source/core/html/shadow/PluginPlaceholderElement.idl#newcode9 Source/core/html/shadow/PluginPlaceholderElement.idl:9: [ImplementedInPrivateScript, OnlyExposedToPrivateScript] void initializePlaceholderElements(); On 2014/08/31 16:53:49, jbroman wrote: ...
6 years, 3 months ago (2014-09-01 00:47:38 UTC) #14
jbroman
Rebased and discovered some changes needed due to bindings changes. 1. DEFINE_WRAPPERTYPEINFO() is now needed. ...
6 years, 3 months ago (2014-09-02 21:47:52 UTC) #16
esprehn
Don't use innerHTML, please build a proper API. We should never stuff html in the ...
6 years, 3 months ago (2014-09-02 23:12:00 UTC) #17
haraken
On 2014/09/02 23:12:00, esprehn wrote: > Don't use innerHTML, please build a proper API. We ...
6 years, 3 months ago (2014-09-03 02:40:37 UTC) #18
jbroman
On 2014/09/03 02:40:37, haraken wrote: > On 2014/09/02 23:12:00, esprehn wrote: > > Don't use ...
6 years, 3 months ago (2014-09-03 14:26:06 UTC) #19
jbroman
On 2014/09/03 14:26:06, jbroman wrote: > On 2014/09/03 02:40:37, haraken wrote: > > On 2014/09/02 ...
6 years, 3 months ago (2014-09-05 14:16:02 UTC) #20
esprehn
Looks much better. https://codereview.chromium.org/522783002/diff/140001/Source/core/html/shadow/PluginPlaceholderElement.cpp File Source/core/html/shadow/PluginPlaceholderElement.cpp (right): https://codereview.chromium.org/522783002/diff/140001/Source/core/html/shadow/PluginPlaceholderElement.cpp#newcode15 Source/core/html/shadow/PluginPlaceholderElement.cpp:15: : HTMLDivElement(document) We really need to ...
6 years, 3 months ago (2014-09-09 04:13:30 UTC) #21
jbroman
https://codereview.chromium.org/522783002/diff/140001/Source/core/html/shadow/PluginPlaceholderElement.cpp File Source/core/html/shadow/PluginPlaceholderElement.cpp (right): https://codereview.chromium.org/522783002/diff/140001/Source/core/html/shadow/PluginPlaceholderElement.cpp#newcode15 Source/core/html/shadow/PluginPlaceholderElement.cpp:15: : HTMLDivElement(document) On 2014/09/09 04:13:30, esprehn wrote: > We ...
6 years, 3 months ago (2014-09-09 14:05:23 UTC) #23
esprehn
Lgtm
6 years, 3 months ago (2014-09-12 19:36:35 UTC) #24
jbroman
On 2014/09/12 19:36:35, esprehn wrote: > Lgtm tl;dr: Will try again to land this shortly; ...
6 years, 3 months ago (2014-09-13 00:40:31 UTC) #25
jbroman
On 2014/09/13 00:40:31, jbroman wrote: > On 2014/09/12 19:36:35, esprehn wrote: > > Lgtm > ...
6 years, 3 months ago (2014-09-15 21:17:21 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/522783002/220001
6 years, 3 months ago (2014-09-15 21:17:43 UTC) #28
commit-bot: I haz the power
6 years, 3 months ago (2014-09-15 22:48:16 UTC) #29
Message was sent while issue was closed.
Committed patchset #10 (id:220001) as 182014

Powered by Google App Engine
This is Rietveld 408576698