554214 - Update crashed-plugin UI code for Lorentz branch

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Justin Dolske [:Dolske]]

The various big of the crashed-plugin code have been backported from trunk to the Lorentz branch by bsmedberg, a few further fixups are needed:

* Add support for english string fallbacks, so that locales/langpacks that have not added these strings will not be busted.
* Fix a few bits of code that were using trunk-specific idioms, and broke on 1.9.2.
* Move some theme stuff around, so that the plugin UI is not completely unstyled when using an existing 1.9.2.* compatible theme, but still all themes to customize this UI.

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Patch (strings), v.1

Comment 2

[Justin Dolske [:Dolske]]

Attachment: Patch (observer), v.1

Comment 3

[Justin Dolske [:Dolske]]

Attachment: Patch (theme), v.1

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 434112 [details] [diff] [review]
Patch (strings), v.1

>diff --git a/browser/components/preferences/advanced.xul b/browser/components/preferences/advanced.xul

> <!ENTITY % advancedDTD SYSTEM "chrome://browser/locale/preferences/advanced.dtd">
> %advancedDTD;

>+<!ENTITY submitCrashes.label "Submit crash reports">
>+<!ENTITY submitCrashes.accesskey "S">

I think I asked you this in another bug, but I take it you've ensured that the behavior here is intentionally and deterministically first-reference-wins?

>diff --git a/toolkit/mozapps/plugins/content/pluginProblem.xml b/toolkit/mozapps/plugins/content/pluginProblem.xml

>+<!ENTITY report.submitting   "Sending reportâ¦">

encoding error or bugzilla fail?

Comment 5

[Justin Dolske [:Dolske]]

(In reply to comment #4)

> >+<!ENTITY submitCrashes.label "Submit crash reports">
> >+<!ENTITY submitCrashes.accesskey "S">
> 
> I think I asked you this in another bug, but I take it you've ensured that the
> behavior here is intentionally and deterministically first-reference-wins?

I definitely verified this when I first tried this (back in one of the early patches for bug 538910), to ensure I had a plan for strings on Lorentz.

I meant to verify it again today, but forgot and clobbered for something else. :/

> >+<!ENTITY report.submitting   "Sending reportâ¦">
> 
> encoding error or bugzilla fail?

Bugzilla fail, it's correct in the UI (and the raw patch when viewing the attachment directly).

Comment 6

[Axel Hecht [:Pike]]

This is somewhat a question for this bug or the bug 554046.

Do we really really need to add strings to toolkit?

It's gonna be painful given that the non-Firefox apps just build all branches against each other with whatever version numbers they come up with. "Painful" as in that "explain this over and over and fix the bustages" might end up being my Q2 goal.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 434114 [details] [diff] [review]
Patch (theme), v.1

>diff --git a/toolkit/themes/pinstripe/mozapps/plugins/pluginProblem.css b/toolkit/themes/pinstripe/mozapps

>+ * Note to themers:
>+ *
>+ * This CSS (and associated XBL/XUL) was added/changed for the landing of
>+ * the new UI for crashed plugins in Gecko 1.9.2.3 (aka Firefox 3.6.3). To
>+ * avoid the new UI from breaking when using existing themes, we've shipped
>+ * the new CSS and images in /content instead of /skin.
>+ *
>+ * If you wish to theme this UI, you should add your CSS to this file and
>+ * override the rules set by the /content CSS:
>+ *   chrome://mozapps/content/plugins/pluginProblemLorentz.css.

Oh, I misread this at first (I thought you were talking about chrome registry overrides). Dao is probably a better reviewer for this approach than I am.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 434114 [details] [diff] [review]
Patch (theme), v.1

>+        <stylesheet src="chrome://mozapps/content/plugins/pluginProblemLorentz.css"/>
>         <stylesheet src="chrome://mozapps/skin/plugins/pluginProblem.css"/>
>         <stylesheet src="chrome://mozapps/content/plugins/pluginProblemContent.css"/>

Why not put it all in pluginProblemContent.css and include that before the skin file? I don't mind the extra file, but I'm asking because I think the skin <stylesheet> should be below the content one anyway.

>+:-moz-type-unsupported .icon {
>+    background-image: url(chrome://mozapps/skin/plugins/pluginGeneric.png);
>+}
>+:-moz-handler-disabled .icon {
>+    background-image: url(chrome://mozapps/skin/plugins/pluginDisabled.png);
>+}
>+:-moz-handler-blocked .icon {
>+    background-image: url(chrome://mozapps/skin/plugins/pluginBlocked.png);

Shouldn't these be in content/ as well?

Comment 9

[Dão Gottwald [:dao]]

And can you just hg move the images from skin/ to content/, and remove them from the theme manifests?

Comment 10

[Justin Dolske [:Dolske]]

(In reply to comment #8)

> Why not put it all in pluginProblemContent.css and include that before the skin
> file? I don't mind the extra file, but I'm asking because I think the skin
> <stylesheet> should be below the content one anyway.

I'd prefer to keep things in a separate file, to make it easier to backport any future modifications.

> Shouldn't these be in content/ as well?

These icons already exist. I guess I should copy them to content lest a theme replace them with differently-sized versions, though...

> And can you just hg move the images from skin/ to content/, and remove them
> from the theme manifests?

Yeah (at least for the new icons). I was thinking having them in skin would make it easier for themes compatible with 3.6 and 3.7, but that doesn't make any sense now that I think about it. :)

Comment 11

[Dão Gottwald [:dao]]

(In reply to comment #10)
> > Shouldn't these be in content/ as well?
> 
> These icons already exist.

They could be named or otherwise organized differently in a third-party theme, as far as I can see. We just expect the stylesheets to exist usually.

Comment 12

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Updated with previous comments. Luckily shorlander just finished a round of icons that are not platform-specific, which helps out a bit here.

Comment 13

[Justin Dolske [:Dolske]]

Pushed:
(strings) http://hg.mozilla.org/projects/firefox-lorentz/rev/30ca0b454785
(observers) http://hg.mozilla.org/projects/firefox-lorentz/rev/9cb16dfb7020
(theme) http://hg.mozilla.org/projects/firefox-lorentz/rev/f722c81626b4

Note that these changes are Lorentz branch-specific, and don't need to land on trunk.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed

Comment 15

[Benjamin Smedberg]

Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430