538164 - Sync the Applications Prefpane with the latest from mozilla-central

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Comment 1

[Philip Chee]

Attachment: Patch v1.0 WIP

Asking for ui-review from Neil on the modern appPicker.css changes.

+listitem-iconic > .listcell-icon {
+listitem-iconic > .listcell-icon,
+listitem-iconic > .listcell-label {

I'm using these child selectors so that these rules are ignored on 1.9.2. Better suggestions welcome.

Comment 2

[[e-mail]]

Comment on attachment 420330 [details] [diff] [review]
Patch v1.0 WIP

>+listitem-iconic > .listcell-icon {
1.9.2 appPicker doesn't use iconic listcells does it?

>+  height: 32px;
>+  width: 32px;
I think I prefer width before height. (And possibly margin first?)

>+listitem-iconic > .listcell-label {
>+  margin: 5px;
D'oh; it's !important in listbox.css; an oversight when I patched toolkit. Fortunately there's some padding instead so it's not as if we need it.

Comment 3

[Philip Chee]

Note I don't have a 1.9.2 tree and there don't seem to be any nightly builds of comm-192 so I am testing the CSS against SeaMonkey 2.0.x

> (From update of attachment 420330 [details] [diff] [review])
>>+listitem-iconic > .listcell-icon {
> 1.9.2 appPicker doesn't use iconic listcells does it?

Right, it doesn't. This allows me to restrict the additional CSS to 1.9.3 only. I would like to restrict #content-icon as well but I couldn't find a simple way of doing this.
 
>>+  height: 32px;
>>+  width: 32px;
> I think I prefer width before height. (And possibly margin first?)

But when you rewrote the winstripe CSS as part of Bug 501095 you put height first and margin last?
 
>>+listitem-iconic > .listcell-label {
>>+  margin: 5px;
> D'oh; it's !important in listbox.css; an oversight when I patched toolkit.
> Fortunately there's some padding instead so it's not as if we need it.

Sorry I don't understand this comment do you want me to remove this rule from the patch?

Comment 4

[Philip Chee]

Comment on attachment 420330 [details] [diff] [review]
Patch v1.0 WIP

Changing the review request from IanN to KaiRo after discussing this with IanN over IRC.

Comment 5

[[e-mail]]

(In reply to comment #3)
>> (From update of attachment 420330 [details] [diff] [review] [details])
>>>+listitem-iconic > .listcell-icon {
>> 1.9.2 appPicker doesn't use iconic listcells does it?
>Right, it doesn't. This allows me to restrict the additional CSS to 1.9.3 only.
Actually your misuse of listitem-iconic (tag rule) means that the CSS doesn't work on 1.9.3 either, but the point is that you don't need .listitem-iconic (class rule) to restrict it to 1.9.3 anyway.

> I would like to restrict #content-icon as well but I couldn't find a simple way
> of doing this.
Since they're 32px anyway, it makes no difference.

>>>+  height: 32px;
>>>+  width: 32px;
>> I think I prefer width before height. (And possibly margin first?)
>But when you rewrote the winstripe CSS as part of Bug 501095 you put height
>first and margin last?
My bad, width before height is correct in toolkit too.

>>>+listitem-iconic > .listcell-label {
>>>+  margin: 5px;
>>D'oh; it's !important in listbox.css; an oversight when I patched toolkit.
>>Fortunately there's some padding instead so it's not as if we need it.
>Sorry I don't understand this comment do you want me to remove this rule from
>the patch?
I don't know yet, let's see what my reviewer says in bug 501095.

Comment 6

[[e-mail]]

Oh and would you mind commenting the 1.9.2 rules for ease of later removal?

Comment 7

[Philip Chee]

Attachment: Patch Pv1.0 appPicker.css only

Splitting out the modern/global/appPicker.css into a separate patch (thank you mqueue).

Changes since the last patch:

> Oh and would you mind commenting the 1.9.2 rules for ease of later removal?
1. Added comments delineating the 1.9.2 rules.
2. Moved the 1.9.3 specific rules to the end of the file to make the patch simpler.
3. Added a dotted bottom border to the listitems to 1.9.3 from 1.9.1./1.9.2.

Comment 8

[Philip Chee]

Attachment: Patch Av1.0 pref-applications patch [Checkin: Comment 14]

pref-applications patch. Sync the Applications Prefpane with the latest from mozilla-central.

1. Ignores Thunderbird-only workaround patches.
2. Ports only patches that apply to 1.9.2 as well as 1.9.3. I will file a follow up bug for 1.9.3 specific changes only once comm-1.9.2 branches.

Comment 9

[[e-mail]]

(In reply to comment #7)
> 3. Added a dotted bottom border to the listitems to 1.9.3 from 1.9.1./1.9.2.
I can't think why I even mentioned it before, toolkit doesn't have one.

Anyway given the review in bug 501095 this patch has bitrotted.

Comment 10

[Philip Chee]

Attachment: Patch Pv1.1 appPicker.css fix bit rot [Checkin: Comment 11]

>> 3. Added a dotted bottom border to the listitems to 1.9.3 from 1.9.1./1.9.2.
> I can't think why I even mentioned it before, toolkit doesn't have one.
You did indirectly in the modern global update bug when you pointed out to me that you meant the border for the (rich)listbox and not the listitems. But you let me keep the dotted border anyway on the listrows anyway.

> Anyway given the review in bug 501095 this patch has bitrotted.
Bit rot fixed.

Comment 11

[Justin Wood (:Callek)]

$ hg outgoing -v && hg push
running "ssh hg.mozilla.org "hg -R comm-central/ serve --stdio""
comparing with ssh://hg.mozilla.org/comm-central/
searching for changes
changeset:   4697:fbdd6b0f6953
tag:         tip
user:        Philip Chee <[e-mail]>
date:        Thu Jan 14 01:13:44 2010 -0500
files:       suite/themes/modern/global/appPicker.css
description:
Bug 538164 Part 2 Sync the modern appPicker.css with winstripe.
SEAMONKEY ONLY, r+sr=NeilAway


pushing to ssh://hg.mozilla.org/comm-central/
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 1 changes to 1 files

Comment 12

[Robert Kaiser]

Comment on attachment 421003 [details] [diff] [review]
Patch Av1.0 pref-applications patch
[Checkin: Comment 14]

>+    // Notify observers that the UI is now ready
>+    Components.classes["@mozilla.org/observer-service;1"]
>+              .getService(Components.interfaces.nsIObserverService)
>+              .notifyObservers(window, "app-handler-pane-loaded", null);

Is there a consumer of this? This smells like something we usually add mainly for tests to use, if there is an actual test for the pane, we probably should port that as well.

If so, as I said, we really should do that, but it can be done in a followup.

Comment 13

[Philip Chee]

> Is there a consumer of this? This smells like something we usually add mainly
> for tests to use, if there is an actual test for the pane, we probably should
> port that as well.

Right we should port m-c/browser/components/preferences/tests/browser_bug410900.js as well

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 421003 [details] [diff] [review]
Patch Av1.0 pref-applications patch
[Checkin: Comment 14]


http://hg.mozilla.org/comm-central/rev/1a1863d3d7ef