Last Comment Bug 538164 - Sync the Applications Prefpane with the latest from mozilla-central
: Sync the Applications Prefpane with the latest from mozilla-central
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.1a1
Assigned To: Philip Chee
:
Mentors:
Depends on:
Blocks: 539713
  Show dependency treegraph
 
Reported: 2010-01-06 07:42 PST by Philip Chee
Modified: 2010-01-16 05:50 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1.0 WIP (3.44 KB, patch)
2010-01-06 07:47 PST, Philip Chee
no flags Details | Diff | Splinter Review
Patch Pv1.0 appPicker.css only (1.06 KB, patch)
2010-01-10 20:57 PST, Philip Chee
no flags Details | Diff | Splinter Review
Patch Av1.0 pref-applications patch [Checkin: Comment 14] (2.92 KB, patch)
2010-01-10 21:03 PST, Philip Chee
kairo: review+
neil: superreview+
Details | Diff | Splinter Review
Patch Pv1.1 appPicker.css fix bit rot [Checkin: Comment 11] (1.10 KB, patch)
2010-01-11 19:36 PST, Philip Chee
neil: review+
neil: superreview+
Details | Diff | Splinter Review

Description Philip Chee 2010-01-06 07:42:18 PST

    
Comment 1 Philip Chee 2010-01-06 07:47:35 PST
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.
Comment 2 neil@parkwaycc.co.uk 2010-01-06 13:57:47 PST
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 2010-01-06 20:45:13 PST
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 2010-01-08 05:47:44 PST
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 neil@parkwaycc.co.uk 2010-01-09 09:13:52 PST
(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 neil@parkwaycc.co.uk 2010-01-09 09:31:04 PST
Oh and would you mind commenting the 1.9.2 rules for ease of later removal?
Comment 7 Philip Chee 2010-01-10 20:57:42 PST
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.
Comment 8 Philip Chee 2010-01-10 21:03:46 PST
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.
Comment 9 neil@parkwaycc.co.uk 2010-01-11 09:20:20 PST
(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 2010-01-11 19:36:53 PST
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.
Comment 11 Justin Wood (:Callek) 2010-01-13 22:15:02 PST
$ 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
Comment 12 Robert Kaiser 2010-01-14 06:22:00 PST
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 2010-01-14 08:31:49 PST
> 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) 2010-01-16 05:50:10 PST
Comment on attachment 421003 [details] [diff] [review]
Patch Av1.0 pref-applications patch
[Checkin: Comment 14]


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

Note You need to log in before you can comment on or make changes to this bug.