515777 - move css files, hiddenWindow.html to jar

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[(dormant account)]

Attachment: renaming of urls

This patch highlighted a fun issue:
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSLoader.cpp#1933
causes

###!!! ASSERTION: must be a file:// url: 'Error', file /home/taras/work/mozilla-central/netwerk/base/src/nsURLHelper.cpp, line 159                       

to appear because a resource://gre-resources/ url can't be compared to any other url because it fails in nsResURL::EnsureFile to net_GetFileFromURLSpec() because there is no underlying nsIFile in jar urls :(

I think the fix is to make PR_TRUE param( indicating aSupportsFileURL) to false when resource:// url is pointing to a jar

Comment 2

[Arpad Borsos [:Swatinem]]

--- a/parser/htmlparser/src/nsExpatDriver.cpp
+++ b/parser/htmlparser/src/nsExpatDriver.cpp
@@ -291,8 +291,8 @@ static const nsCatalogData kCatalogTable
   { "-//W3C//DTD XHTML 1.0 Frameset//EN",        "xhtml11.dtd", nsnull },
   { "-//W3C//DTD XHTML Basic 1.0//EN",           "xhtml11.dtd", nsnull },
   { "-//W3C//DTD XHTML 1.1 plus MathML 2.0//EN", "mathml.dtd",  "resource://gre/res/mathml.css" },
-  { "-//W3C//DTD XHTML 1.1 plus MathML 2.0 plus SVG 1.1//EN", "mathml.dtd", "resource://gre/res/mathml.css" },
-  { "-//W3C//DTD MathML 2.0//EN",                "mathml.dtd",  "resource://gre/res/mathml.css" },
+  { "-//W3C//DTD XHTML 1.1 plus MathML 2.0 plus SVG 1.1//EN", "mathml.dtd", "resource://gre-resources/mathml.css" },
+  { "-//W3C//DTD MathML 2.0//EN",                "mathml.dtd",  "resource://gre-resources/mathml.css" },
   { "-//WAPFORUM//DTD XHTML Mobile 1.0//EN",     "xhtml11.dtd", nsnull },

You have missed one.

Comment 3

[(dormant account)]

Attachment: move stuff

Comment 4

[(dormant account)]

Comment on attachment 401347 [details] [diff] [review]
move stuff

Thanks Arpad, that was what made the try server so unhappy

Comment 5

[Benjamin Smedberg]

Comment on attachment 401347 [details] [diff] [review]
move stuff

This should get sr from somebody in layout, I think.

Comment 6

[Boris Zbarsky [:bzbarsky]]

We always ship toolkit.jar, even in embedding scenarios (e.g. Camino), right?

After this patch, where will one need to rebuild to pick up changes to ua.css and company?  If it's different from what it was before (which I suspect it is, since the answer on Mac/Linux used to be "nowhere"), we should probably announce that change somewhere.

Looks ok if we always ship toolkit.jar.

Comment 7

[(dormant account)]

(In reply to comment #6)

> 
> Looks ok if we always ship toolkit.jar.

we do.

Comment 8

[Phil Ringnalda (:philor)]

Taras: in what sense are you using "we" there? I just looked in a Camino.dmg, and didn't find a toolkit.jar, so I assume you must mean "anyone who doesn't ship a toolkit.jar is already screwed with mozilla-central, and this'll just be another case of that"?

Comment 9

[(not currently active) Ted Mielczarek]

I think Camino is going to have other issues switching to m-c, but there isn't much that we can do to fix them at the moment.

Comment 10

[Phil Ringnalda (:philor)]

If embedding still worked, how would it work? Would these things just need to be added to /embedding/config/embed-jar.mn? Well, and of course, it and everything else in that directory updated, since virtually nobody ever does :)

Comment 11

[Benjamin Smedberg]

embedding/config must be removed. Camino will need to use standard XULRunner-type packaging if they have any hope of using -central.

Comment 12

[Phil Ringnalda (:philor)]

You had me at "removed" - filed bug 518632.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 401347 [details] [diff] [review]
move stuff

sr=me assuming the "announce" thing I mentioned happens.

Comment 14

[(dormant account)]

http://hg.mozilla.org/mozilla-central/rev/e22bef491d84

Comment 15

[Daniel Holbert [:dholbert]]

This checkin caused bustage in the reftest "414123.xhtml" on all platforms -- the testcase is non-italic, and the reference case is italic.

It also caused bustage in the reftest "non-spacing-accent-1.xhtml" on linux & windows -- both the testcase & reference case now render as blank, which fails because it's a "!=" reftest.

Sample logs:
LINUX:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1255040295.1255042738.21455.gz
MAC:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1255042963.1255044831.11752.gz
WINDOWS:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1255041560.1255044299.6007.gz

Comment 16

[Daniel Holbert [:dholbert]]

(In reply to comment #15)
> This checkin caused bustage in the reftest "414123.xhtml" on all platforms --
> the testcase is non-italic, and the reference case is italic.

(not sure what expected rendering is -- I'm just reporting the difference I see from glancing at the reftest snapshots)

Comment 17

[(dormant account)]

backed out in http://hg.mozilla.org/mozilla-central/rev/30ab84dda860

Comment 18

[Daniel Holbert [:dholbert]]

So, both of the failed reftests involve MathML, so the underlying problem probably relates to MathML.

Focusing on that, it looks like you might've missed one s/res/resource/ here:

71 nsresult
72 nsMathMLElement::BindToTree(nsIDocument* aDocument, nsIContent* aParent,
73                             nsIContent* aBindingParent,
74                             PRBool aCompileEventHandlers)
75 {
76   static const char kMathMLStyleSheetURI[] = "resource://gre/res/mathml.css";

http://mxr.mozilla.org/mozilla-central/source/content/mathml/content/src/nsMathMLElement.cpp#76

(not sure if that change will fix it, though)

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to comment #18)
> So, both of the failed reftests involve MathML, so the underlying problem
> probably relates to MathML.
> 
> Focusing on that, it looks like you might've missed one s/res/resource/ here:

Sorry, I meant "missed one gre/res --> gre-resources"

Comment 20

[(dormant account)]

Attachment: rev2 with dholbert's fix

This is ready for checkin, passed the try server.

Comment 21

[(dormant account)]

http://hg.mozilla.org/mozilla-central/rev/d761860c50c7

Comment 22

[Boris Zbarsky [:bzbarsky]]

This caused bug 522050.

Comment 23

[Boris Zbarsky [:bzbarsky]]

So... I'd assumed you'd grepped or mxr'ed for these files and changed the references to them.  This clearly didn't happen at least for viewsource.css (see bug 522229.  What about the others?

Comment 24

[Phil Ringnalda (:philor)]

The extra res/ in http://mxr.mozilla.org/mozilla-central/source/parser/htmlparser/src/nsExpatDriver.cpp#293 certainly _looks_ like it's wrong, but other than that and viewsource (and the forms stuff until mxr catches up), mxr doesn't see anything untoward in <http://mxr.mozilla.org/mozilla-central/search?string=\%2Fres\%2F&regexp=on&find=&findi=&filter=res\%2F%28hidden|mathml|ua|html\.c|quirk|forms|viewsource|mathml|loading-|broken-%29>