Last Comment Bug 596753 - Flatten mozilla/layout/svg/base/src/ into mozilla/layout/svg/
: Flatten mozilla/layout/svg/base/src/ into mozilla/layout/svg/
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla18
Assigned To: Alexandre Dumont
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-15 14:46 PDT by Daniel Holbert [:dholbert]
Modified: 2012-09-26 17:13 PDT (History)
4 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch fixing bug 596753 (18.56 KB, patch)
2012-09-24 14:18 PDT, Alexandre Dumont
dholbert: review+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2010-09-15 14:46:03 PDT
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. :)
Comment 1 Robert Longson 2010-09-15 14:52:05 PDT
We should do this. If we're looking at doing cleanup changes I'd say bug 585020 would be more important FWIW.
Comment 2 Alexandre Dumont 2012-09-24 14:18:23 PDT
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.
Comment 3 Jonathan Watt [:jwatt] 2012-09-24 14:53:13 PDT
I think we should put the source files in layout/svg/src personally.
Comment 4 Daniel Holbert [:dholbert] 2012-09-24 14:58:06 PDT
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 5 Daniel Holbert [:dholbert] 2012-09-24 15:37:19 PDT
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.
Comment 6 Daniel Holbert [:dholbert] 2012-09-24 15:56:14 PDT
This builds fine on my local machine. Testing this on TryServer:
  https://tbpl.mozilla.org/?tree=Try&rev=35d7cf98dcda
Comment 7 Alexandre Dumont 2012-09-25 02:41:45 PDT
(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..
Comment 8 Daniel Holbert [:dholbert] 2012-09-26 08:13:13 PDT
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.
Comment 9 Daniel Holbert [:dholbert] 2012-09-26 08:19:00 PDT
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
Comment 10 Alexandre Dumont 2012-09-26 08:24:28 PDT
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.
Comment 11 Ryan VanderMeulen [:RyanVM] 2012-09-26 17:13:26 PDT
https://hg.mozilla.org/mozilla-central/rev/745b5180e4eb

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