Closed
Bug 596753
Opened 14 years ago
Closed 12 years ago
Flatten mozilla/layout/svg/base/src/ into mozilla/layout/svg/
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: dholbert, Assigned: u443890)
Details
Attachments
(1 file)
18.56 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
We should do this. If we're looking at doing cleanup changes I'd say bug 585020 would be more important FWIW.
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.
Assignee: nobody → dmt.alexandre
Attachment #664209 -
Flags: review?(dholbert)
Comment 3•12 years ago
|
||
I think we should put the source files in layout/svg/src personally.
Reporter | ||
Comment 4•12 years ago
|
||
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)
Reporter | ||
Comment 5•12 years ago
|
||
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.
Attachment #664209 -
Flags: review?(dholbert) → review+
Reporter | ||
Comment 6•12 years ago
|
||
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..
Reporter | ||
Comment 8•12 years ago
|
||
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.
Reporter | ||
Comment 9•12 years ago
|
||
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
Status: NEW → ASSIGNED
Assignee | ||
Comment 10•12 years ago
|
||
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•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/745b5180e4eb
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in
before you can comment on or make changes to this bug.
Description
•