Last Comment Bug 590725 - Convert suite/ files for content XUL being killed
: Convert suite/ files for content XUL being killed
Status: VERIFIED FIXED
: regression
Product: SeaMonkey
Classification: Client Software
Component: Feed Discovery and Preview (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: seamonkey2.1b1
Assigned To: Justin Wood (:Callek)
:
Mentors:
: 590659 (view as bug list)
Depends on: kill-remote-xul
Blocks: SmTestFail
  Show dependency treegraph
 
Reported: 2010-08-25 15:05 PDT by Robert Kaiser (not working on stability any more)
Modified: 2011-06-20 15:10 PDT (History)
6 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
b1+


Attachments
Do it, certError and Feeds (22.27 KB, patch)
2010-08-25 16:44 PDT, Justin Wood (:Callek)
no flags Details | Diff | Review
some missed changes. (24.00 KB, patch)
2010-08-25 21:10 PDT, Justin Wood (:Callek)
neil: review-
Details | Diff | Review
certError fix [Checked in: Comment 11] (7.93 KB, patch)
2010-08-26 13:43 PDT, neil@parkwaycc.co.uk
bugspam.Callek: review+
Details | Diff | Review
address review comments. (22.48 KB, patch)
2010-08-26 19:06 PDT, Justin Wood (:Callek)
neil: review-
Details | Diff | Review
This should be it (20.46 KB, patch)
2010-09-01 20:41 PDT, Justin Wood (:Callek)
stefanh: feedback+
Details | Diff | Review
With fewer hacks [Checked in: Comment 25] (20.69 KB, patch)
2010-09-02 02:54 PDT, neil@parkwaycc.co.uk
bugspam.Callek: review+
Details | Diff | Review
Firefox port of offsetTop hack removal (1.51 KB, patch)
2010-11-22 04:00 PST, neil@parkwaycc.co.uk
gavin.sharp: review+
Details | Diff | Review

Description Robert Kaiser (not working on stability any more) 2010-08-25 15:05:54 PDT
Bug 546857 killed content XUL, we need to convert at least our feed preview and cert error pages as done there for FF so that they still work.
Comment 1 Robert Kaiser (not working on stability any more) 2010-08-25 15:48:09 PDT
*** Bug 590659 has been marked as a duplicate of this bug. ***
Comment 2 Justin Wood (:Callek) 2010-08-25 16:44:53 PDT
Created attachment 469272 [details] [diff] [review]
Do it, certError and Feeds

This should do it, I have tested very sparsely so far.
Comment 3 Justin Wood (:Callek) 2010-08-25 21:10:50 PDT
Created attachment 469330 [details] [diff] [review]
some missed changes.

Do a bit more changes to satisfy certError needs
Comment 4 neil@parkwaycc.co.uk 2010-08-26 12:58:44 PDT
Comment on attachment 469330 [details] [diff] [review]
some missed changes.

>diff --git a/suite/common/certError.xhtml b/suite/common/certError.xhtml
I don't like what Firefox have done for their certError changes, so I'll try to write my own version based on feeds. (And you missed some stuff anyway.)

>+#feedSubscribeLine {
>+  -moz-binding: url(subscribe.xml#feedreaderUI);
Nit: prefer full chrome: URL

>     <link rel="stylesheet"
>           href="chrome://communicator/skin/feed-subscribe.css"
>           type="text/css"
>           media="all"/>
>+    <link rel="stylesheet"
>+          href="chrome://communicator/content/feeds/subscribe.css"
>+          type="text/css"
>+          media="all"/>
Nit: content CSS first please

>+<?xml version="1.0" encoding="utf-8"?>
[I didn't think encoding was needed, utf-8 should be the default]

>+  <!ENTITY % globalDTD
>+    SYSTEM "chrome://global/locale/global.dtd">
>+  %globalDTD;
Not used. (So you can use the DOCTYPE shorthand.)

>    content/communicator/feeds/subscribe.xhtml                       (feeds/subscribe.xhtml)
>+   content/communicator/feeds/subscribe.xml                         (feeds/subscribe.xml)
>    content/communicator/feeds/subscribe.js                          (feeds/subscribe.js)
>+   content/communicator/feeds/subscribe.css                         (feeds/subscribe.css)
Ideally this would be in alphabetical order...

>+  _handlersMenuList: null,
[I wonder why this one was cached. I would have cached feedSubscribeLine.]

>+    this._document.getElementById("feedSubscribeLine").offsetTop;
offsetTop is deprecated ;-) Please use getBoundingClientRect() instead.

>+.alwaysUse {
>+  padding: 5px;
This wasn't in our previous CSS, and doesn't belong here either.

>+.handlersMenuList > menupopup > menuitem {
>+  -moz-padding-start: 23px;
No point, since all of our menuitems are iconic, aren't they?

>+.handlersMenuList > menupopup > menuitem.menuitem-iconic {
You can abbreviate this to .menuitem-iconic since nothing else in the binding will have that class.

>+  -moz-padding-start: 2px;
Nit: I prefer 4px, but I only tried this on Windows.

>+.handlersMenuList > menupopup > .menuitem-iconic  > .menu-iconic-left {
And this can be abbreviated to .menu-iconic-left for the same reason.

>+  min-width: 16px;
.menu-iconic-icon has a width of 16px so we probably don't need this.

>+  -moz-padding-end: 2px;
Nit: I prefer 3px, but again, only tested on Windows.

>+.messengerFeedsMenuItem {
>+  list-style-image: url("chrome://communicator/skin/icons/feedIcon16.png");
KaiRo made this obsolete when he landed Places, but forgot to remove it. Boo!

>+menupopup:-moz-locale-dir("rtl") {
No quotes around rtl.

>+++ b/suite/themes/modern/communicator/feed-subscribe-ui.css
This is almost all completely wrong, except for the rtl, which is only partially wrong.
Comment 5 neil@parkwaycc.co.uk 2010-08-26 13:00:48 PDT
Actually there's one thing that it would be nice to add to the CSS - currently the font for the XUL bit is wrong (particularly noticable with the menulist) so it would be handy if you could set the vbox font to message-box (both themes).
Comment 6 Robert Kaiser (not working on stability any more) 2010-08-26 13:33:13 PDT
(In reply to comment #4)
> >diff --git a/suite/common/certError.xhtml b/suite/common/certError.xhtml
> I don't like what Firefox have done for their certError changes, so I'll try to
> write my own version based on feeds. (And you missed some stuff anyway.)

Well, a fix should land the days before yesterday, but that said, could you, in this case, please backport all your changes to our side to theirs? Porting adaptations from them is already hard enough with how we are diverging, and I still think it's wrong that this even lives in app-specific code anyhow.
Comment 7 neil@parkwaycc.co.uk 2010-08-26 13:43:36 PDT
Created attachment 469596 [details] [diff] [review]
certError fix
[Checked in: Comment 11]

I'd prefer xbl:inherits="id" but I thought that would get r- ;-)
Comment 8 Justin Wood (:Callek) 2010-08-26 13:45:56 PDT
Comment on attachment 469596 [details] [diff] [review]
certError fix
[Checked in: Comment 11]

>diff --git a/suite/common/certError.css b/suite/common/certError.css
>new file mode 100644
>--- /dev/null
>+++ b/suite/common/certError.css
>@@ -0,0 +1,3 @@
>+span {
>+  -moz-binding: url("chrome://communicator/content/certError.xml#button");
>+}

nit: #getMeOutOfHereButton

So we don't get any weird surprises later by someone adding a span in here.
Comment 9 Justin Wood (:Callek) 2010-08-26 19:05:21 PDT
(In reply to comment #4)
> Comment on attachment 469330 [details] [diff] [review]
> some missed changes.
> 
> >+<?xml version="1.0" encoding="utf-8"?>
> [I didn't think encoding was needed, utf-8 should be the default]

I assume you wanted me to drop encoding="..."

> Not used. (So you can use the DOCTYPE shorthand.)

No idea what this "shorthand" is, but I dropped the unused part.

> >+    this._document.getElementById("feedSubscribeLine").offsetTop;
> offsetTop is deprecated ;-) Please use getBoundingClientRect() instead.

Of course, offsetTop and getBoundingClientRect have very different semantics, and offsetTop (per MDC) does not say deprecated. I did not change this.

> >+.messengerFeedsMenuItem {
> >+  list-style-image: url("chrome://communicator/skin/icons/feedIcon16.png");
> KaiRo made this obsolete when he landed Places, but forgot to remove it. Boo!

Not sure what you were asking me to do here, but I think you meant to remove it from the css.

> >+++ b/suite/themes/modern/communicator/feed-subscribe-ui.css
> This is almost all completely wrong, except for the rtl, which is only
> partially wrong.

Not sure what you were asking me to do here either, so I did the same changes as the classic .css

Hope this is satisfactory.

I'm unsure if the tabbrowser.xml and the utilityOverlay.js changes are still needed/correct with the other patch by you.
Comment 10 Justin Wood (:Callek) 2010-08-26 19:06:04 PDT
Created attachment 469738 [details] [diff] [review]
address review comments.
Comment 11 Justin Wood (:Callek) 2010-08-26 19:34:11 PDT
Comment on attachment 469596 [details] [diff] [review]
certError fix
[Checked in: Comment 11]

http://hg.mozilla.org/comm-central/rev/b9b8db196a54
Comment 12 Jens Hatlak (:InvisibleSmiley) 2010-08-26 22:40:31 PDT
Err, 2.0 Branch?!
Comment 13 neil@parkwaycc.co.uk 2010-08-27 14:28:37 PDT
(In reply to comment #9)
>(In reply to comment #4)
>>(From update of attachment 469330 [details] [diff] [review])
>>>+<?xml version="1.0" encoding="utf-8"?>
>>[I didn't think encoding was needed, utf-8 should be the default]
>I assume you wanted me to drop encoding="..."
Actually I ended up confusing you. We seem to assume utf-8 for XUL and XBL files, but not for xhtml files, so that change wasn't needed.

>> Not used. (So you can use the DOCTYPE shorthand.)
> No idea what this "shorthand" is, but I dropped the unused part.
<!DOCTYPE <tagname> SYSTEM "chrome://<package>/locale/<file>">

> > >+    this._document.getElementById("feedSubscribeLine").offsetTop;
> > offsetTop is deprecated ;-) Please use getBoundingClientRect() instead.
> Of course, offsetTop and getBoundingClientRect have very different semantics
Neither of which we are using here; we're actually using them to flush layout.

> and offsetTop (per MDC) does not say deprecated. I did not change this.
Hmm, well I'm sure getBoundingClientRect was added to replace something...

> > >+.messengerFeedsMenuItem {
> > >+  list-style-image: url("chrome://communicator/skin/icons/feedIcon16.png");
> > KaiRo made this obsolete when he landed Places, but forgot to remove it. Boo!
> Not sure what you were asking me to do here, but I think you meant to remove it
> from the css.
Correct!

> > >+++ b/suite/themes/modern/communicator/feed-subscribe-ui.css
> > This is almost all completely wrong, except for the rtl, which is only
> > partially wrong.
> Not sure what you were asking me to do here either, so I did the same changes
> as the classic .css
No, it nearly all has to go.

> I'm unsure if the tabbrowser.xml and the utilityOverlay.js changes are still
> needed/correct with the other patch by you.
No, they are not. (Did you not test it?)
Comment 14 neil@parkwaycc.co.uk 2010-08-27 14:34:55 PDT
Comment on attachment 469738 [details] [diff] [review]
address review comments.

>+        <div id="feedSubscribeLine"></div>
In addition to my replies to your comments, one thing I forgot to mention before is that this is an XHTML document, and therefore does not need to use HTML end tag syntax.

>diff --git a/suite/themes/classic/communicator/feed-subscribe-ui.css b/suite/themes/classic/communicator/feed-subscribe-ui.css
>new file mode 100644
>--- /dev/null
>+++ b/suite/themes/classic/communicator/feed-subscribe-ui.css
>@@ -0,0 +1,12 @@
>+.menuitem-iconic {
>+  -moz-padding-start: 4px;
>+}
>+
>+.menu-iconic-left {
>+  display: -moz-box;
>+  -moz-padding-end: 3px;
>+}
[I'm rebuilding my Linux build for bug 591078, so I hope be able to test this on Linux tonight.]

>diff --git a/suite/themes/modern/communicator/feed-subscribe-ui.css b/suite/themes/modern/communicator/feed-subscribe-ui.css
>new file mode 100644
>--- /dev/null
>+++ b/suite/themes/modern/communicator/feed-subscribe-ui.css
>@@ -0,0 +1,12 @@
>+.menuitem-iconic {
>+  -moz-padding-start: 4px;
>+}
>+
>+.menu-iconic-left {
>+  display: -moz-box;
>+  -moz-padding-end: 3px;
>+}
[So just to clarify, we don't need these two rules, just the third one.]
Comment 15 Philip Chee 2010-08-27 18:45:37 PDT
> Hmm, well I'm sure getBoundingClientRect was added to replace something...
IIRC document.height/document.width
Comment 16 neil@parkwaycc.co.uk 2010-08-28 12:54:26 PDT
Comment on attachment 469738 [details] [diff] [review]
address review comments.

>+<?xml version="1.0">
Oops, missing a ?
r- for this and the other comments above.

>diff --git a/suite/themes/classic/communicator/feed-subscribe-ui.css b/suite/themes/classic/communicator/feed-subscribe-ui.css
>new file mode 100644
>--- /dev/null
>+++ b/suite/themes/classic/communicator/feed-subscribe-ui.css
>@@ -0,0 +1,12 @@
>+.menuitem-iconic {
>+  -moz-padding-start: 4px;
>+}
>+
>+.menu-iconic-left {
>+  display: -moz-box;
>+  -moz-padding-end: 3px;
>+}
So, these figures are too high for Linux, which wants them to be 1px :-(
I guess the old figure of 2px was the best available compromise...
Sorry to mess you around like that.
Comment 17 Serge Gautherie (:sgautherie) 2010-09-01 01:09:16 PDT
Ftr:

http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey&maxdate=1282766141&hours=24&legend=0&norules=1
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1282727715.1282728818.12246.gz
Linux comm-central-trunk debug test mochitests-5/5 on 2010/08/25 02:15:15
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1282737209.1282738153.1829.gz
Linux comm-central-trunk debug test mochitests-5/5 on 2010/08/25 04:53:29
{
231 ERROR TEST-UNEXPECTED-FAIL | /tests/suite/browser/test/test_bug364677.html | Feed served as text/xml without a channel/link should have been sniffed - got undefined, expected "feedHandler"
}
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a758e294ed9e&tochange=cec16a1741fb
(Bug 546857 Part 6)
Comment 18 Justin Wood (:Callek) 2010-09-01 20:39:11 PDT
(In reply to comment #13)
> (In reply to comment #9)
> >(In reply to comment #4)
> >>(From update of attachment 469330 [details] [diff] [review])
> >>>+<?xml version="1.0" encoding="utf-8"?>
> >>[I didn't think encoding was needed, utf-8 should be the default]
> >I assume you wanted me to drop encoding="..."
> Actually I ended up confusing you. We seem to assume utf-8 for XUL and XBL
> files, but not for xhtml files, so that change wasn't needed.

Err ok, fixed.

> >> Not used. (So you can use the DOCTYPE shorthand.)
> > No idea what this "shorthand" is, but I dropped the unused part.
> <!DOCTYPE <tagname> SYSTEM "chrome://<package>/locale/<file>">

Ahh yes, fixed.

> > > >+    this._document.getElementById("feedSubscribeLine").offsetTop;
> > > offsetTop is deprecated ;-) Please use getBoundingClientRect() instead.
> > Of course, offsetTop and getBoundingClientRect have very different semantics
> Neither of which we are using here; we're actually using them to flush layout.

And per bz, offsetTop is not deprecated or going away, and one vs the other doesn't really matter for a layout-flush-use. So I'd rather stay compat with Firefox here. Did not change. [frankly, I'm unsure on why we even _need_ to flush here, but thats another bug/story entirely]

> > I'm unsure if the tabbrowser.xml and the utilityOverlay.js changes are still
> > needed/correct with the other patch by you.
> No, they are not. (Did you not test it?)

I hadn't at the time of posting, but I reverted them.

(In reply to comment #14)
> >+++ b/suite/themes/modern/communicator/feed-subscribe-ui.css
> [So just to clarify, we don't need these two rules, just the third one.]

Ok.

(In reply to comment #16)
> >+<?xml version="1.0">
> Oops, missing a ?

Fixed

> >diff --git a/suite/themes/classic/communicator/feed-subscribe-ui.css b/suite/themes/classic/communicator/feed-subscribe-ui.css
> So, these figures are too high for Linux, which wants them to be 1px :-(
> I guess the old figure of 2px was the best available compromise...
> Sorry to mess you around like that.

I understand, and fixed.

Attaching patch in just a moment (about to test it with mochi 5)
Comment 19 Justin Wood (:Callek) 2010-09-01 20:41:06 PDT
Created attachment 471394 [details] [diff] [review]
This should be it
Comment 20 Justin Wood (:Callek) 2010-09-01 20:53:52 PDT
(In reply to comment #18)
> Attaching patch in just a moment (about to test it with mochi 5)

And we no longer hit the failure relating to this bug with that suite.
Comment 21 neil@parkwaycc.co.uk 2010-09-02 02:38:03 PDT
Comment on attachment 471394 [details] [diff] [review]
This should be it

>diff --git a/suite/common/feeds/subscribe.css b/suite/common/feeds/subscribe.css
>new file mode 100644
>--- /dev/null
>+++ b/suite/common/feeds/subscribe.css
>@@ -0,0 +1,3 @@
>+#feedSubscribeLine {
>+  -moz-binding: url(chrome://communicator/content/feeds/subscribe.xml#feedreaderUI);
>+}
Sorry for not noticing before but all your new files have DOS line endings :-(

>+  <binding id="feedreaderUI">
I suddenly realised how to get rid of that offsetTop hack. What we need to do is to add a <constructor> to this binding, and get that to init the UI.
Comment 22 neil@parkwaycc.co.uk 2010-09-02 02:54:52 PDT
Created attachment 471449 [details] [diff] [review]
With fewer hacks [Checked in: Comment 25]

Gavin, did you want the offsetTop hack removal changes (basically the bulk of the interdiff between this patch and attachment 471394 [details] [diff] [review], but this also has some whitespace cleanup and also a couple of font changes) ported?
Comment 23 Stefan [:stefanh] (away until May 28) 2010-09-04 16:29:15 PDT
Comment on attachment 471394 [details] [diff] [review]
This should be it

Looks good from a mac pov.
Comment 24 Robert Kaiser (not working on stability any more) 2010-09-04 16:41:32 PDT
Subscribing to feeds is completely broken currently, could you guys please get this in really soon now, preferably yesterday?
Comment 25 Serge Gautherie (:sgautherie) 2010-09-04 20:18:47 PDT
Comment on attachment 471449 [details] [diff] [review]
With fewer hacks [Checked in: Comment 25]

http://hg.mozilla.org/comm-central/rev/e7918600229d
Comment 26 Serge Gautherie (:sgautherie) 2010-09-04 20:25:22 PDT
(In reply to comment #17)
> 231 ERROR TEST-UNEXPECTED-FAIL | /tests/suite/browser/test/test_bug364677.html
> | Feed served as text/xml without a channel/link should have been sniffed - got
> undefined, expected "feedHandler"

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1283650257.1283651482.3646.gz
Linux comm-central-trunk debug test mochitests-5/5 on 2010/09/04 18:30:57
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1283655300.1283656282.20969.gz
OS X 10.5 comm-central-trunk debug test mochitests-5/5 on 2010/09/04 19:55:00

Test (and related leak) fixed :-)

V.Fixed
Comment 27 neil@parkwaycc.co.uk 2010-11-22 04:00:17 PST
Created attachment 492293 [details] [diff] [review]
Firefox port of offsetTop hack removal
Comment 28 Serge Gautherie (:sgautherie) 2010-12-13 22:23:48 PST
Gavin, ping for feedback and review.
Comment 29 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-20 15:10:41 PDT
Comment on attachment 492293 [details] [diff] [review]
Firefox port of offsetTop hack removal

This looks fine, but we apparently do not have test coverage for any of this UI, so I'm not very comfortable making changes to it. Can you write a test (or do you have a SeaMonkey test that I could port?)

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