Closed
Bug 538164
Opened 15 years ago
Closed 15 years ago
Sync the Applications Prefpane with the latest from mozilla-central
Categories
(SeaMonkey :: Preferences, defect)
SeaMonkey
Preferences
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.1a1
People
(Reporter: philip.chee, Assigned: philip.chee)
References
Details
Attachments
(2 files, 2 obsolete files)
2.92 KB,
patch
|
kairo
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
1.10 KB,
patch
|
neil
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•15 years ago
|
||
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•15 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•15 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•15 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•15 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•15 years ago
|
||
Oh and would you mind commenting the 1.9.2 rules for ease of later removal?
Assignee | ||
Comment 7•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
Attachment #421003 -
Flags: superreview?(neil) → superreview+
Comment 9•15 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•15 years ago
|
||
>> 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•15 years ago
|
Attachment #421189 -
Flags: superreview?(neil)
Attachment #421189 -
Flags: superreview+
Attachment #421189 -
Flags: review?(neil)
Attachment #421189 -
Flags: review+
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Whiteboard: [c-n patch in comment 10]
Updated•15 years ago
|
Attachment #421003 -
Attachment is obsolete: true
Attachment #421003 -
Flags: review?(kairo)
Comment 11•15 years ago
|
||
$ 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: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1a1
Assignee | ||
Updated•15 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•15 years ago
|
Attachment #421003 -
Attachment is obsolete: false
Attachment #421003 -
Flags: review?(kairo)
Assignee | ||
Updated•15 years ago
|
Status: REOPENED → ASSIGNED
Comment 12•15 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•15 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•15 years ago
|
Attachment #421003 -
Attachment description: Patch Av1.0 pref-applications patch. → [for checkin] Patch Av1.0 pref-applications patch.
Assignee | ||
Updated•15 years ago
|
Whiteboard: [c-n patch in comment 11] → [c-n patch in comment 12]
Updated•15 years ago
|
Attachment #421189 -
Attachment description: Patch Pv1.1 appPicker.css fix bit rot. → Patch Pv1.1 appPicker.css fix bit rot
[Checkin: Comment 11]
Comment 14•15 years ago
|
||
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]
Updated•15 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 15 years ago → 15 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.
Description
•