Closed Bug 998500 Opened 6 years ago Closed 6 years ago

Add command line flag for turning on OOP in B2G Desktop

Categories

(Firefox OS Graveyard :: GonkIntegration, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S6 (25apr)

People

(Reporter: qdot, Assigned: qdot)

References

Details

(Whiteboard: [systemsfe] [p=3])

Attachments

(1 file, 2 obsolete files)

Add a compilation flag to turn on OOP in B2G builds. Mostly for creating a tbpl target for b2g desktop with OOP turned on. We'll keep the platform specification that turns it on automatically for all gonk builds too, since there's really no reason to have it off there.
Attachment #8409142 - Flags: review?(fabrice)
Comment on attachment 8409142 [details] [diff] [review]
Patch 1 (v1) - Add compilation flag for turning on OOP in B2G

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

r=me for the b2g/ part, but you need a build peer to review configure.in
Attachment #8409142 - Flags: review?(fabrice) → review+
Comment on attachment 8409142 [details] [diff] [review]
Patch 1 (v1) - Add compilation flag for turning on OOP in B2G

Asking for build peer review.
Attachment #8409142 - Flags: review?(gps)
Can we create a command-line argument for this as well?  It will be much faster getting these turned on in TBPL if we can just pass -oop to the build to get it to enable OOP.
(In reply to Jonathan Griffin (:jgriffin) from comment #3)
> Can we create a command-line argument for this as well?  It will be much
> faster getting these turned on in TBPL if we can just pass -oop to the build
> to get it to enable OOP.

Command-line argument to the b2g executable? I thought you were talking per-build-configuration only, which is why I added it to configure.in
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #4)
> (In reply to Jonathan Griffin (:jgriffin) from comment #3)
> > Can we create a command-line argument for this as well?  It will be much
> > faster getting these turned on in TBPL if we can just pass -oop to the build
> > to get it to enable OOP.
> 
> Command-line argument to the b2g executable? I thought you were talking
> per-build-configuration only, which is why I added it to configure.in

Yes, to the b2g executable.  We could also get this running with a separate build, but it's a bit more complicated that way.
Changing compilation flag to command line flag on request of jgriffin. This does seem a lot easier too.
Attachment #8409142 - Attachment is obsolete: true
Attachment #8409142 - Flags: review?(gps)
Attachment #8410559 - Flags: review?(fabrice)
Summary: Add compilation flag for turning on OOP in B2G → Add command line flag for turning on OOP in B2G Desktop
Whiteboard: [systemsfe]
Target Milestone: --- → 1.4 S6 (25apr)
Comment on attachment 8410559 [details] [diff] [review]
Patch 1 (v2) - Add command line argument for turning on OOP in B2G Desktop

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

I want to take a second look. Also, I'm very sad about the single quotes.

::: b2g/components/OopCommandLine.js
@@ +6,5 @@
> +Cu.import('resource://gre/modules/XPCOMUtils.jsm');
> +XPCOMUtils.defineLazyModuleGetter(this, 'Services', 'resource://gre/modules/Services.jsm');
> +
> +function oopCommandlineHandler() {
> +}

nit: add blank line

@@ +17,5 @@
> +             * to the default branch means the changes we make won't get written
> +             * back to user preferences.
> +             */
> +            var prefs = Components.classes['@mozilla.org/preferences-service;1'].
> +                    getService(Components.interfaces.nsIPrefService);

let prefs = Services.prefs

@@ +42,5 @@
> +    QueryInterface: XPCOMUtils.generateQI([Ci.nsICommandLineHandler]),
> +    _xpcom_categories: [{
> +        category: 'command-line-handler',
> +        entry: 'm-b2goop'
> +    }]

Do we need that *and* the manifest registration?

::: b2g/installer/package-manifest.in
@@ +432,4 @@
>  @BINPATH@/components/amContentHandler.js
>  @BINPATH@/components/amWebInstallListener.js
>  @BINPATH@/components/nsBlocklistService.js
> +@BINPATH@/components/OopCommandLine.js

that should be only in a #ifndef MOZ_WIDGET_GONK
Attachment #8410559 - Flags: review?(fabrice) → review-
(In reply to Fabrice Desré [:fabrice] from comment #7)

> I want to take a second look. Also, I'm very sad about the single quotes.

I had the usual linter I use for gaia going and it hates double quotes. So double quotes in gecko js, single in gaia?

> ::: b2g/installer/package-manifest.in
> @@ +432,4 @@
> >  @BINPATH@/components/amContentHandler.js
> >  @BINPATH@/components/amWebInstallListener.js
> >  @BINPATH@/components/nsBlocklistService.js
> > +@BINPATH@/components/OopCommandLine.js
> 
> that should be only in a #ifndef MOZ_WIDGET_GONK

It is. There's a #endif after it that ends the #ifndef MOZ_WIDGET_GONK that didn't make it into my patch.
Comment on attachment 8411463 [details] [diff] [review]
Patch 1 (v3) - Add command line argument for turning on OOP in B2G Desktop

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

r=me with nit addressed.

::: b2g/components/OopCommandLine.js
@@ +18,5 @@
> +             * to the default branch means the changes we make won"t get written
> +             * back to user preferences.
> +             */
> +            let prefs = Services.prefs
> +            var branch = prefs.getDefaultBranch("");

s/var/let
Attachment #8411463 - Flags: review?(fabrice) → review+
https://hg.mozilla.org/mozilla-central/rev/bb23e8d8d194
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [systemsfe] → [systemsfe] [p=3]
You need to log in before you can comment on or make changes to this bug.