Closed Bug 828116 Opened 7 years ago Closed 6 years ago

move some toolkit modules to toolkit/modules

Categories

(Toolkit :: General, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: Gavin, Assigned: ananuti)

Details

(Whiteboard: [mentor=gavin])

Attachments

(1 file, 6 obsolete files)

We have toolkit/modules now (bug 813833), so we should move PopupNotifications there.
There are a few of these, actually.
Summary: move PopupNotifications to toolkit/modules → move some toolkit modules to toolkit/modules
I think all of the jsms in toolkit/content could move to toolkit/modules, actually. Probably best to wait until after bug 784841 :)
Whiteboard: [mentor=gavin]
hi,
can you assign this bug to me ?
Sure thing! Let me know if you need any advice.
Assignee: nobody → harshit080
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
Yes, per comment 2 I think all of the JSM files in toolkit/content should move to toolkit/modules.
i have moved the all jsm files toolkit/modules from toolkit/content,what to do next in order to submit the bug ?
Status: NEW → ASSIGNED
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.
okay i have no problem,Gavin what to do ?Although i have created a patch of this bug.
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.
Harsh, could you confirm that you're still working on this?
Flags: needinfo?(harshit080)
Assignee: harshit080 → nobody
Flags: needinfo?(harshit080)
Assignee: nobody → ananuti
Attachment #742670 - Flags: review?(gavin.sharp)
Attachment #742671 - Flags: feedback?(dtownsend+bugmail)
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?
Attachment #742670 - Attachment is obsolete: true
Attachment #742670 - Flags: review?(gavin.sharp)
Attachment #742880 - Flags: feedback?(gavin.sharp)
Attachment #742671 - Attachment is obsolete: true
Attachment #742671 - Flags: feedback?(dtownsend+bugmail)
Attachment #742882 - Flags: feedback?(dtownsend+bugmail)
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!
Attachment #742882 - Flags: feedback?(dtownsend+bugmail) → feedback+
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.
Attachment #742880 - Flags: feedback?(gavin.sharp) → feedback+
Attached patch fold patch (obsolete) — Splinter Review
(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.
Attachment #742880 - Attachment is obsolete: true
Attachment #742882 - Attachment is obsolete: true
Try build are burning https://tbpl.mozilla.org/?tree=Try&rev=c07db255caa3

I don't quite understand the log. What should I do next?
Flags: needinfo?(gavin.sharp)
pushed only path 1 (move all jsms in toolkit/content to toolkit/modules) to try https://tbpl.mozilla.org/?tree=Try&rev=32110953e25e
Attached patch fold patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=f4bb5cb74c73

the patch breaks several things. please let me know what to do next.
Attachment #747244 - Attachment is obsolete: true
(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.
Flags: needinfo?(gavin.sharp)
this is now green on try :) 

https://tbpl.mozilla.org/?tree=Try&rev=8c2d202685bb
Attachment #747352 - Attachment is obsolete: true
Attachment #747885 - Flags: review?(dtownsend+bugmail)
Comment on attachment 747885 [details] [diff] [review]
Move modules in toolkit/content and toolkit/mozapps/shared to toolkit/modules

Excellent work, thanks!
Attachment #747885 - Flags: review?(dtownsend+bugmail) → review+
https://hg.mozilla.org/mozilla-central/rev/b880a068345e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.