833879 - Move layout/xul/base/src/tree/ to layout/xul/tree, layout/xul/base/src/grid to layout/xul/grid

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Alex Vincent [:WeirdAl]]

I'll have a patch for this tonight.  Basically, the grid and tree code is buried deep, and tree has its own public, src subdirectories.

I'd like to move them up in the hierarchy for two reasons:
* Grids and trees are special cases for XUL layout; it's an inconvenience to go so deep in the source tree for changes.
* I intend to add a TreeViews.jsm module with lots of (reftests/chrome Mochitests) somewhere under layout/xul/tree to help developers with custom tree views.

The patch I have now is 1.1 MB, but it's almost all hg moves... it shouldn't be that bad.

Comment 1

[Alex Vincent [:WeirdAl]]

cc'ing roc as module owner

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Please!

Comment 3

[Alex Vincent [:WeirdAl]]

Attachment: patch

hg diff --git to the rescue.  Much smaller now.

I'd love it if someone could run this through the tryserver!

Comment 4

[Alex Vincent [:WeirdAl]]

Comment on attachment 705441 [details] [diff] [review]
patch

https://tbpl.mozilla.org/?tree=Try&rev=8a92049aad8e

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This is great... except I think it would have been nice to flatten the public/src pair within the tree/ directory at the same time (fewer file moves to track, and I think squashing that pair all into tree/ is preferable, though I'd be interested in the opinions of others).

Comment 6

[Alex Vincent [:WeirdAl]]

Comment on attachment 705441 [details] [diff] [review]
patch

based on comment 5, canceling the checkin flag for now.  I personally don't care much either way.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to David Baron [:dbaron] from comment #5)
> This is great... except I think it would have been nice to flatten the
> public/src pair within the tree/ directory at the same time (fewer file
> moves to track, and I think squashing that pair all into tree/ is
> preferable, though I'd be interested in the opinions of others).

I think that's a good idea, but I think it would be fine to land the current patch and then do that as a followup. It would also be fine to fold that into this patch. Up to Alex.

Comment 8

[Alex Vincent [:WeirdAl]]

I'll try to do the flattening over the next couple days - it won't take long, but I have limited time.  If we get down to a week before train departure, I'd prefer we take the current patch as-is than wait for me to get the flattening done.

Comment 9

[Alex Vincent [:WeirdAl]]

Attachment: patch with tree flattened

Once again, a tryserver run (this time with crashtests and reftests) is recommended.  I've built it locally on my Mac, but I've not run the tests.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think a test run is needed as long as it builds. You're not actually changing any code here.

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 705746 [details] [diff] [review]
patch with tree flattened

Review of attachment 705746 [details] [diff] [review]:
-----------------------------------------------------------------

Note that this conflicts with bug 784841; I wouldn't mind if you waited a little before getting this pushed :)

::: content/events/src/Makefile.in
@@ +80,5 @@
>               -I$(srcdir)/../../../dom/settings \
>               -I$(srcdir)/../../../dom/src/storage \
>               -I$(srcdir)/../../../layout/generic \
>               -I$(srcdir)/../../../layout/xul/base/src \
> +             -I$(srcdir)/../../../layout/xul/tree/ \

Probably don't want the trailing slash here

::: content/xul/templates/src/Makefile.in
@@ +50,5 @@
>  include $(topsrcdir)/config/rules.mk
>  
>  LOCAL_INCLUDES	= -I$(srcdir)/../../../base/src \
>  		  -I$(srcdir)/../../content/src \
> +		  -I$(srcdir)/../../../../layout/xul/tree/ \

or here

::: layout/base/Makefile.in
@@ +131,5 @@
>  		-I$(srcdir)/../forms \
>  		-I$(srcdir)/../tables \
>  		-I$(srcdir)/../printing \
>  		-I$(srcdir)/../xul/base/src \
> +		-I$(srcdir)/../xul/tree/ \

or here

Comment 12

[Alex Vincent [:WeirdAl]]

Comment on attachment 705746 [details] [diff] [review]
patch with tree flattened

*sigh* I forgot about the DIRS logic patch... how soon is that expected to land?

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I hope in less than a week; depending on when ted reviews :)

Comment 14

[bhavana bajaj [:bajaj]]

This does not look like a regression or has a user facing impact & not a release blocker, hence no need to track. Please feel free to renominate with reasoning if this is not the case.

If this is not fixed within FX21 on nightly,will consider uplifts if necessary based on the risk & where we are in the cycle .

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Land it, this is taking longer than I expected.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb45ceb51e82

Alex, can you please make sure that your hg is configured to generate all the needed checkin metadata for future patches? It makes life easier for those checking in on your behalf. Thanks!
https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bb45ceb51e82