828116 - move some toolkit modules to toolkit/modules

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

We have toolkit/modules now (bug 813833), so we should move PopupNotifications there.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

There are a few of these, actually.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

I think all of the jsms in toolkit/content could move to toolkit/modules, actually. Probably best to wait until after bug 784841 :)

Comment 3

[harsh chhabra]

hi,
can you assign this bug to me ?

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Sure thing! Let me know if you need any advice.

Comment 5

[harsh chhabra]

i had a look on toolkit/content there is PopupNotifications.jsm and some other jsm , so which files we suppose to move to toolkit/module, only PopupNotifications.jsm or all other jsm files ?, and as this is my first bug please guide me as you can.
Thanks

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Yes, per comment 2 I think all of the JSM files in toolkit/content should move to toolkit/modules.

Comment 7

[harsh chhabra]

i have moved the all jsm files toolkit/modules from toolkit/content,what to do next in order to submit the bug ?

Comment 8

[:Gavin Sharp [email: [e-mail]]]

You'll need to attach the patch here. See https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch .

Comment 9

[Dave Townsend [:mossop]]

I'd like to add the modules currently living in toolkit/mozapps/shared to this along with the tests that go along with them please.

Comment 10

[harsh chhabra]

okay i have no problem,Gavin what to do ?Although i have created a patch of this bug.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

I'm not sure which part you're unsure about. You'll need to add those changes to your patch, and then attach your patch to this bug and ask for review, as mentioned in comment 8.

Comment 12

[Josh Matthews [:jdm]]

Harsh, could you confirm that you're still working on this?

Comment 13

[Ekanan Ketunuti]

Attachment: move all jsms in toolkit/content to toolkit/modules

Comment 14

[Ekanan Ketunuti]

Attachment: part 2 - mv toolkit/mozapps/shared to toolkit/modules

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Thanks for the patch!

Any of the tests under toolkit/content/tests/ that are associated with these modules will need to move as well. Can you include those in the patch too?

Comment 16

[Ekanan Ketunuti]

Attachment: part 1 take 2 - move all jsms in toolkit/content to toolkit/modules

Comment 17

[Ekanan Ketunuti]

Attachment: part 2 take 2 - move modules in toolkit/mozapps/shared to toolkit/modules

Comment 18

[Dave Townsend [:mossop]]

Comment on attachment 742882 [details] [diff] [review]
part 2 take 2 - move modules in toolkit/mozapps/shared to toolkit/modules

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

Looks good to me!

Comment 19

[Dave Townsend [:mossop]]

Comment on attachment 742880 [details] [diff] [review]
part 1 take 2 - move all jsms in toolkit/content to toolkit/modules

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

Looking good. Make the changes here and make sure that all the tests pass before asking for final review. You might as well combine both the patches into one review request. Me or gavin is fine as reviewer.

::: toolkit/content/Makefile.in
@@ -50,5 @@
>  DEFINES += -DMOZ_TOOLKIT_SEARCH
>  endif
>  
>  EXTRA_JS_MODULES = \
>    debug.js \

Take debug.js along as well I think

::: toolkit/modules/Makefile.in
@@ +25,4 @@
>    TelemetryTimestamps.jsm \
>    Timer.jsm \
> +  PrivateBrowsingUtils.jsm \
> +  Sqlite.jsm \

Can you put these in alphabetical order

::: toolkit/modules/tests/xpcshell/xpcshell.ini
@@ +6,3 @@
>  [test_newtab-migrate-v1.js]
>  [test_Preferences.js]
> +[test_propertyListsUtils.js]

Looks like you haven't moved the toolkit/content/tests/unit/propertyLists files that the test_propertyListsUtils test relies on.

Comment 20

[Ekanan Ketunuti]

Attachment: fold patch

(In reply to Dave Townsend (:Mossop) from comment #19)

> ::: toolkit/content/Makefile.in
> @@ -50,5 @@
> >  DEFINES += -DMOZ_TOOLKIT_SEARCH
> >  endif
> >  
> >  EXTRA_JS_MODULES = \
> >    debug.js \
> 
> Take debug.js along as well I think

Done.

> 
> ::: toolkit/modules/Makefile.in
> @@ +25,4 @@
> >    TelemetryTimestamps.jsm \
> >    Timer.jsm \
> > +  PrivateBrowsingUtils.jsm \
> > +  Sqlite.jsm \
> 
> Can you put these in alphabetical order

Done.

> 
> ::: toolkit/modules/tests/xpcshell/xpcshell.ini
> @@ +6,3 @@
> >  [test_newtab-migrate-v1.js]
> >  [test_Preferences.js]
> > +[test_propertyListsUtils.js]
> 
> Looks like you haven't moved the toolkit/content/tests/unit/propertyLists
> files that the test_propertyListsUtils test relies on.

Done.

Comment 21

[Ekanan Ketunuti]

Try build are burning https://tbpl.mozilla.org/?tree=Try&rev=c07db255caa3

I don't quite understand the log. What should I do next?

Comment 22

[Ekanan Ketunuti]

pushed only path 1 (move all jsms in toolkit/content to toolkit/modules) to try https://tbpl.mozilla.org/?tree=Try&rev=32110953e25e

Comment 23

[Ekanan Ketunuti]

Attachment: fold patch

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

the patch breaks several things. please let me know what to do next.

Comment 24

[Dave Townsend [:mossop]]

(In reply to Ekanan Ketunuti from comment #21)
> Try build are burning https://tbpl.mozilla.org/?tree=Try&rev=c07db255caa3
> 
> I don't quite understand the log. What should I do next?

The error here is: *** Must define relativesrcdir when defining XPCSHELL_TESTS..  Stop.

You need to add relativesrcdir to toolkit/modules/Makefile.in, look elsewhere in the source for examples.

(In reply to Ekanan Ketunuti from comment #23)
> Created attachment 747352 [details] [diff] [review]
> fold patch
> 
> https://tbpl.mozilla.org/?tree=Try&rev=f4bb5cb74c73
> 
> the patch breaks several things. please let me know what to do next.

The errors show lots of references to Services.search being undefined. If you look in Services.jsm you'll see that the search property is dependant on MOZ_TOOLKIT_SEARCH. Searching the source for that shows that it is added as a definition for the preprocessor here: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/Makefile.in#49. So you need to move that block to toolkit/modules/Makefile.in.

Comment 25

[Ekanan Ketunuti]

Attachment: Move modules in toolkit/content and toolkit/mozapps/shared to toolkit/modules

this is now green on try :) 

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

Comment 26

[Dave Townsend [:mossop]]

Comment on attachment 747885 [details] [diff] [review]
Move modules in toolkit/content and toolkit/mozapps/shared to toolkit/modules

Excellent work, thanks!

Comment 27

[Dave Townsend [:mossop]]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/b880a068345e

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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