Sync the Applications Prefpane with the latest from mozilla-central

RESOLVED FIXED in seamonkey2.1a1

Status

SeaMonkey
Preferences
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Philip Chee, Assigned: Philip Chee)

Tracking

Trunk
seamonkey2.1a1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

8 years ago
Created attachment 420330 [details] [diff] [review]
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.
Attachment #420330 - Flags: ui-review?(neil)
Attachment #420330 - Flags: review?(iann_bugzilla)

Comment 2

8 years ago
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.
(Assignee)

Comment 3

8 years ago
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?
(Assignee)

Comment 4

8 years ago
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.
Attachment #420330 - Flags: review?(iann_bugzilla) → review?(kairo)

Comment 5

8 years ago
(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

8 years ago
Oh and would you mind commenting the 1.9.2 rules for ease of later removal?
(Assignee)

Comment 7

8 years ago
Created attachment 421002 [details] [diff] [review]
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.
Attachment #421002 - Flags: superreview?(neil)
Attachment #421002 - Flags: review?(neil)
(Assignee)

Comment 8

8 years ago
Created attachment 421003 [details] [diff] [review]
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.
Attachment #420330 - Attachment is obsolete: true
Attachment #421003 - Flags: superreview?(neil)
Attachment #421003 - Flags: review?(kairo)
Attachment #420330 - Flags: ui-review?(neil)
Attachment #420330 - Flags: review?(kairo)

Updated

8 years ago
Attachment #421003 - Flags: superreview?(neil) → superreview+

Comment 9

8 years ago
(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.
(Assignee)

Comment 10

8 years ago
Created attachment 421189 [details] [diff] [review]
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.
Attachment #421002 - Attachment is obsolete: true
Attachment #421189 - Flags: superreview?(neil)
Attachment #421189 - Flags: review?(neil)
Attachment #421002 - Flags: superreview?(neil)
Attachment #421002 - Flags: review?(neil)

Updated

8 years ago
Attachment #421189 - Flags: superreview?(neil)
Attachment #421189 - Flags: superreview+
Attachment #421189 - Flags: review?(neil)
Attachment #421189 - Flags: review+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Whiteboard: [c-n patch in comment 10]

Updated

8 years ago
Attachment #421003 - Attachment is obsolete: true
Attachment #421003 - Flags: review?(kairo)
$ 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 <philip.chee@gmail.com>
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
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1a1
(Assignee)

Updated

8 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

8 years ago
Attachment #421003 - Attachment is obsolete: false
Attachment #421003 - Flags: review?(kairo)
(Assignee)

Updated

8 years ago
Status: REOPENED → ASSIGNED

Comment 12

8 years ago
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.
Attachment #421003 - Flags: review?(kairo) → review+
(Assignee)

Comment 13

8 years ago
> 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
Keywords: checkin-needed
Whiteboard: [c-n patch in comment 10] → [c-n patch in comment 11]
(Assignee)

Updated

8 years ago
Attachment #421003 - Attachment description: Patch Av1.0 pref-applications patch. → [for checkin] Patch Av1.0 pref-applications patch.
(Assignee)

Updated

8 years ago
Whiteboard: [c-n patch in comment 11] → [c-n patch in comment 12]
(Assignee)

Updated

8 years ago
Blocks: 539713
Attachment #421189 - Attachment description: Patch Pv1.1 appPicker.css fix bit rot. → Patch Pv1.1 appPicker.css fix bit rot [Checkin: Comment 11]
Comment on attachment 421003 [details] [diff] [review]
Patch Av1.0 pref-applications patch
[Checkin: Comment 14]


http://hg.mozilla.org/comm-central/rev/1a1863d3d7ef
Attachment #421003 - Attachment description: [for checkin] Patch Av1.0 pref-applications patch. → Patch Av1.0 pref-applications patch [Checkin: Comment 14]
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [c-n patch in comment 12]
You need to log in before you can comment on or make changes to this bug.