Closed Bug 538164 Opened 10 years ago Closed 10 years ago

Sync the Applications Prefpane with the latest from mozilla-central

Categories

(SeaMonkey :: Preferences, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.1a1

People

(Reporter: philip.chee, Assigned: philip.chee)

References

Details

Attachments

(2 files, 2 obsolete files)

No description provided.
Attached patch Patch v1.0 WIP (obsolete) — Splinter Review
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 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.
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 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)
(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.
Oh and would you mind commenting the 1.9.2 rules for ease of later removal?
Attached patch Patch Pv1.0 appPicker.css only (obsolete) — Splinter Review
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)
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)
Attachment #421003 - Flags: superreview?(neil) → superreview+
(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.
>> 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)
Attachment #421189 - Flags: superreview?(neil)
Attachment #421189 - Flags: superreview+
Attachment #421189 - Flags: review?(neil)
Attachment #421189 - Flags: review+
Keywords: checkin-needed
Whiteboard: [c-n patch in comment 10]
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
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1a1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #421003 - Attachment is obsolete: false
Attachment #421003 - Flags: review?(kairo)
Status: REOPENED → ASSIGNED
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+
> 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]
Attachment #421003 - Attachment description: Patch Av1.0 pref-applications patch. → [for checkin] Patch Av1.0 pref-applications patch.
Whiteboard: [c-n patch in comment 11] → [c-n patch in comment 12]
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
Closed: 10 years ago10 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.