Bug 107890 - Add a user agent stylesheet for plugins
Summary: Add a user agent stylesheet for plugins
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-01-24 17:57 PST by Dean Jackson
Modified: 2013-01-25 13:57 PST (History)
15 users (show)

See Also:


Attachments
Patch (17.71 KB, patch)
2013-01-24 19:11 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (18.23 KB, patch)
2013-01-25 09:48 PST, Dean Jackson
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 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.
Comment 1 Radar WebKit Bug Importer 2013-01-24 17:57:51 PST
<rdar://problem/13083415>
Comment 2 Dean Jackson 2013-01-24 19:11:55 PST
Created attachment 184640 [details]
Patch
Comment 3 Jon Lee 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.
Comment 4 Jon Lee 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.
Comment 5 Jon Lee 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.
Comment 6 Jon Lee 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.
Comment 7 Alexey Proskuryakov 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.
Comment 8 Dean Jackson 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.
Comment 9 Dean Jackson 2013-01-25 09:48:53 PST
Created attachment 184766 [details]
Patch
Comment 10 Alexey Proskuryakov 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.
Comment 11 Dean Jackson 2013-01-25 13:56:28 PST
Committed r140863: <http://trac.webkit.org/changeset/140863>
Comment 12 Dean Jackson 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.