RESOLVED FIXED 107890
Add a user agent stylesheet for plugins
https://bugs.webkit.org/show_bug.cgi?id=107890
Summary Add a user agent stylesheet for plugins
Dean Jackson
Reported 2013-01-24 17:57:10 PST
In preparation for a Shadow DOM on plugins, provide a UA stylesheet that can be overridden by the RenderTheme.
Attachments
Patch (17.71 KB, patch)
2013-01-24 19:11 PST, Dean Jackson
no flags
Patch (18.23 KB, patch)
2013-01-25 09:48 PST, Dean Jackson
ap: review+
Radar WebKit Bug Importer
Comment 1 2013-01-24 17:57:51 PST
Dean Jackson
Comment 2 2013-01-24 19:11:55 PST
Jon Lee
Comment 3 2013-01-24 19:36:09 PST
Comment on attachment 184640 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184640&action=review An unofficial r=me. > Source/WebCore/rendering/RenderTheme.h:89 > // adjust the default CSS rules in html.css, quirks.css, or mediaControls.css Should also mention plugIns.css.
Jon Lee
Comment 4 2013-01-24 19:36:17 PST
Comment on attachment 184640 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184640&action=review An unofficial r=me. > Source/WebCore/rendering/RenderTheme.h:89 > // adjust the default CSS rules in html.css, quirks.css, or mediaControls.css Should also mention plugIns.css.
Jon Lee
Comment 5 2013-01-24 19:36:18 PST
Comment on attachment 184640 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184640&action=review An unofficial r=me. > Source/WebCore/rendering/RenderTheme.h:89 > // adjust the default CSS rules in html.css, quirks.css, or mediaControls.css Should also mention plugIns.css.
Jon Lee
Comment 6 2013-01-24 19:36:19 PST
Comment on attachment 184640 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184640&action=review An unofficial r=me. > Source/WebCore/rendering/RenderTheme.h:89 > // adjust the default CSS rules in html.css, quirks.css, or mediaControls.css Should also mention plugIns.css.
Alexey Proskuryakov
Comment 7 2013-01-24 22:07:59 PST
Comment on attachment 184640 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184640&action=review > Source/WebCore/css/plugIns.css:13 > + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY APPLE *COMPUTER*? > Source/WebCore/css/plugIns.css:25 > +/* plug-ins */ Is this helpful? > Source/WebCore/page/ChromeClient.h:379 > + virtual String plugInStartLabelSubtitle() const { return String(); }; > + virtual String plugInExtraStyleSheet() const { return String(); }; Please remove the trailing semicolons. > Source/WebCore/rendering/RenderTheme.h:92 > + virtual String extraPlugInsStyleSheet() { return String(); }; Please remove the trailing semicolon. > Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageUIClient.cpp:198 > + RefPtr<WebString> styleSheet = adoptRef(toImpl(m_client.plugInExtraStyleSheet(m_client.clientInfo))); This looks suspicious. Does the result of toImpl really need to be adopted? The function does not have "copy" or "create" in the name.
Dean Jackson
Comment 8 2013-01-25 09:41:52 PST
Comment on attachment 184640 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184640&action=review >> Source/WebCore/css/plugIns.css:13 >> + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY > > APPLE *COMPUTER*? Fixed. >> Source/WebCore/css/plugIns.css:25 >> +/* plug-ins */ > > Is this helpful? Not at the moment, but the other UA CSS files have similar comments at the top. I'll remove it. >> Source/WebCore/page/ChromeClient.h:379 >> + virtual String plugInExtraStyleSheet() const { return String(); }; > > Please remove the trailing semicolons. Done. >>>>> Source/WebCore/rendering/RenderTheme.h:89 >>>>> // adjust the default CSS rules in html.css, quirks.css, or mediaControls.css >>>> >>>> Should also mention plugIns.css. >>> >>> Should also mention plugIns.css. >> >> Should also mention plugIns.css. > > Should also mention plugIns.css. Done. >> Source/WebCore/rendering/RenderTheme.h:92 >> + virtual String extraPlugInsStyleSheet() { return String(); }; > > Please remove the trailing semicolon. Done. C&P from the line below, so I fixed it there too. >> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageUIClient.cpp:198 >> + RefPtr<WebString> styleSheet = adoptRef(toImpl(m_client.plugInExtraStyleSheet(m_client.clientInfo))); > > This looks suspicious. Does the result of toImpl really need to be adopted? The function does not have "copy" or "create" in the name. I was confused because the other methods in this class that return Strings adopt the result of toImpl - generateFileForUpload and shouldGenerateFileForUpload, even without "copy" or "create" in the name. I've removed it.
Dean Jackson
Comment 9 2013-01-25 09:48:53 PST
Alexey Proskuryakov
Comment 10 2013-01-25 10:05:49 PST
Comment on attachment 184766 [details] Patch > I was confused because the other methods in this class that return Strings adopt the result of toImpl - generateFileForUpload and shouldGenerateFileForUpload, even without "copy" or "create" in the name. I've removed it. I don't know which is correct, but it would be easy to tell from checking the code, or stepping through it. Seems likely that there is something to fix here, if only naming.
Dean Jackson
Comment 11 2013-01-25 13:56:28 PST
Dean Jackson
Comment 12 2013-01-25 13:57:28 PST
Landed with names altered to prepend "create". I forgot to remove the comment in the plugIns.css file, but that will go in the next commit.
Note You need to log in before you can comment on or make changes to this bug.