Right now all of our SVG layout code lives in mozilla/layout/svg/base/src when it could just live in mozilla/layout/svg (which would match the organization of other layout subdirectories much better) I'm filing this bug on moving the contents of mozilla/layout/svg/base/src into mozilla/layout/svg, and thereby getting rid of "mozilla/layout/svg/base". (we also need to update any Makefiles etc. that reference the old path) jwatt/longsonr, please speak up if there's any reason we shouldn't do this... CC'ing Felipe, in the hopes that he might be interested in fixing this, since it's similar to bug 421473 which he graciously fixed. :)
We should do this. If we're looking at doing cleanup changes I'd say bug 585020 would be more important FWIW.
Created attachment 664209 [details] [diff] [review] Patch fixing bug 596753 Hello, I was browsing layout related bugs and I found this one. I attached a patch which flattens "layout/svg/base/src" into "layout/svg" so that this folder is more consistent now. I also updated all the Makefiles which involved the previous "layout/svg/base/src" path. Daniel Holbert, could you please review this short patch ? I guess it won't take too much time since it mainly consists in file renaming.
I think we should put the source files in layout/svg/src personally.
That doesn't match our existing convention/style elsewhere, though (except in places where we have /src vs /public -- but there's been an explicit move to merge those two, where it's easy/possible)
Comment on attachment 664209 [details] [diff] [review] Patch fixing bug 596753 Cool -- I discussed this more with jwatt over IRC -- he dislikes the convention of dropping the /src directory, but he's willing to go along with it. Patch looks good to me -- thanks, Alexandre! I'll give it a TryServer run as a sanity-check, and then we can land it.
This builds fine on my local machine. Testing this on TryServer: https://tbpl.mozilla.org/?tree=Try&rev=35d7cf98dcda
(In reply to Daniel Holbert [:dholbert] from comment #5) > Comment on attachment 664209 [details] [diff] [review] > Patch fixing bug 596753 > > Cool -- I discussed this more with jwatt over IRC -- he dislikes the > convention of dropping the /src directory, but he's willing to go along with > it. > > Patch looks good to me -- thanks, Alexandre! I'll give it a TryServer run > as a sanity-check, and then we can land it. Thank you for reviewing. I should have put it on a TryServer, I thought it was not necessary since it is not a major change in code - just moving files and editing paths. Sorry for that. By the way, I took a look at the TryServer results and linux builds seem to fail. (linux opt and linux debug) However, when I look at the result logs, both say "No errors or warnings found.". I don't get it..
Those failures are almost certainly sporadic infrastructure issues -- not your fault. The purple one absolutely -- that's what purple means -- and the orange one as well. (If you click through and view the actual log, there's a sudden timeout in some Firefox run after the build has completed -- that could hypothetically be an issue with the patch, but in this case that's highly unlikely.) So, this is ready to land on mozilla-inbound! Alexandre, do you have commit access there? If not, I can land it for you.
Oh, nevermind -- I just re-noticed the [first patch] annotation back in Comment 2, which means you probably don't have commit access. :) Thanks very much for the patch! Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/745b5180e4eb
Well this is not my first patch actually (previously worked on bug 716875 with a mate), and I only have lvl 1 commit access, so please land it for me. Thank you Daniel.