Bug 107890 – Add a user agent stylesheet for plugins

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Dean Jackson]

In preparation for a Shadow DOM on plugins, provide a UA stylesheet that can be overridden by the RenderTheme.

[Radar WebKit Bug Importer]

<rdar://problem/13083415>

[Dean Jackson]

Created attachment 184640 [details]
Patch

[Jon Lee]

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 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 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 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 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 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]

Created attachment 184766 [details]
Patch

[Alexey Proskuryakov]

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]

Committed r140863: <http://trac.webkit.org/changeset/140863>

[Dean Jackson]

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.