735594 - --enable-jsd=no fails to build

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[prabindh]

Attachment: Patch for fix

With --enable-jsd=no, jsdIDebuggerService should not be used. Since there is no check for MOZ_JSDEBUGGER, compilation fails.

mozilla-central/js/xpconnect/src/nsXPConnect.cpp:2464: error: 'jsdIDebuggerService' was not declared in this scope

Patch added for this at below location - also attached

https://github.com/prabindh/mozilla-embedded/blob/master/patches/13Mar/07_qws_compile_nsXPConnect.cpp.diff

Comment 1

[Bobby Holley (:bholley)]

Comment on attachment 605683 [details] [diff] [review]
Patch for fix

This works, but is a bit confusing. Can we just have 2 separate versions of nsXPConnect::CheckForDebugMode, one for each version of the ifdef? I'm thinking:

#ifndef MOZ_JSDEBUGGER

void
nsXPConnect::CheckForDebugMode(JSRuntime *rt)
{
gDesiredDebugMode = gDebugMode = false;
}

#else

.... other function

#endif


Also, if you wouldn't mind correcting the open-brace on the existing nsXPConnect::CheckForDebugMode to be on its own line, I'd much appreciate it. ;-)

FWIW jsd is going away sometime soon. :-)

Comment 2

[prabindh]

Thanks for the comments - uploaded new patch at below location. Can you please confirm ?

https://github.com/prabindh/mozilla-embedded/blob/master/patches/13Mar/08_qws_compile_nsXPConnect.cpp.diff

Comment 3

[prabindh]

Attachment: Modified per comments from Bobby

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 605891 [details] [diff] [review]
Modified per comments from Bobby

Looks good. r=bholley

Do you need someone to land the patch to mozilla-central for you?

Comment 5

[prabindh]

Thanks, I have no idea how to "land the patch to mozilla-central" :) If there is some documentation on how to do that please point me, or if someone else doing it is simpler, please do so.

BTW - what is going to replace jsd ? Any roadmap publicly available ?

Comment 6

[Bobby Holley (:bholley)]

(In reply to prabindh from comment #5)
> Thanks, I have no idea how to "land the patch to mozilla-central" :) If
> there is some documentation on how to do that please point me, or if someone
> else doing it is simpler, please do so.

Yeah, you need commit access. I set the checkin-needed keyword, which should cause somebody to do it in the next few days. If you don't see any activity on this bug within a week, let me know and I'll make something happen. :-)

> BTW - what is going to replace jsd ? Any roadmap publicly available ?

https://developer.mozilla.org/en/SpiderMonkey/JS_Debugger_API_Guide

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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

Thanks for the patch! To make life easier for those of us checking in your patches for you, please follow the directions below in the future. Thanks!
https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Backed out due to mochitest-4 orange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5fb2d7b5108

Comment 9

[Bobby Holley (:bholley)]

prabindh - So, this task broke the jsd tests, so it was backed out. :-(

It looks like that ifdef isn't actually available to the source file, so it's never defined. Apparently, you have to manually add it to DEFINES in the Makefile, like the other code here:

http://mxr.mozilla.org/mozilla-central/search?string=MOZ_JSDEBUGGER

Can you do something like that for Makefile.in, and verify that the flag is actually defined for a vanilla browser build?

Comment 10

[prabindh]

The flag gets defined in the generated autoconf.mk. Will update again with more information. This was validated on Linux ARM builds.

Comment 12

[Phil Ringnalda (:philor)]

Right, it gets defined in autoconf.mk, and autoconf.mk gets included in generated Makefiles, and that's why the ifdef in a Makefile like http://mxr.mozilla.org/mozilla-central/source/dom/base/Makefile.in#151 works, but then to have it work for the preprocessor for an #ifdef in .cpp, it has to be in DEFINES. Your patch would build with both --enable-jsd and --disable-jsd, it just wouldn't ever work for --enable-jsd because MOZ_JSDEBUGGER would never be DEFINEd, so CheckForDebugMode would always return false.

Comment 13

[Phil Ringnalda (:philor)]

Attachment: With more build goop

The interdiff is all build goop, so that's what I want a second review on.

https://tbpl.mozilla.org/?tree=Try&rev=fb3b77c4c486 is tryserver doing a Linux32 opt --disable-jsd and everything else --enable, passing the tests that failed before.

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/41143e1cae85

Comment 15

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/41143e1cae85

Comment 16

[prabindh]

Thanks for adding this.