Open Bug 882846 Opened 7 years ago Updated 2 years ago
Move -marionette CLI-handling code into the browser
Bug 870445 introduced a -marionette arg to the browser, but according to :gavin, this should live in nsBrowserContent.js and be separately reviewed by a browser peer. This bug will track that work.
Try run in progress: https://tbpl.mozilla.org/?tree=Try&rev=cc54a4afd520
Attachment #763861 - Flags: review?(gavin.sharp)
I think this was addressed by https://bugzilla.mozilla.org/show_bug.cgi?id=870445?
@MDas, we added it in bug 870445 but we did it wrong and it needs to be moved as per Gavin's request. See comment 0
Comment on attachment 763861 [details] [diff] [review] Move -marionette arg handling code into browser, Really sorry this patch sat in my review queue for so long. Mark, can you give this a look at some point? I recall you and I discussing this long ago...
Attachment #763861 - Flags: review?(gavin.sharp) → feedback?(mhammond)
Comment on attachment 763861 [details] [diff] [review] Move -marionette arg handling code into browser, Review of attachment 763861 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine to me, although it seems we can probably remove the xpcom component completely, and that nsBrowserContentHandler.js should still have a check for #ifdef ENABLE_MARIONETTE and possibly for MARIONETTE_ENABLED_PREF (although an enabled pref doesn't sound that important if the cmdline arg is necessary to enable it) ::: testing/marionette/MarionetteLoader.jsm @@ +13,5 @@ > +const MARIONETTE_PORT_PREF = 'marionette.defaultPrefs.port'; > + > +Cu.import("resource://gre/modules/FileUtils.jsm"); > +Cu.import("resource://gre/modules/Services.jsm"); > +Cu.import("resource://gre/modules/services-common/log4moz.js"); Should use Log.jsm now
Attachment #763861 - Flags: feedback?(mhammond) → feedback+
Not reasonable to ask Jonathan to pick this back up after this long - Mark, perhaps you could have a go at addressing your feedback comments and flag me for review again?
(In reply to :Gavin Sharp [email: email@example.com] from comment #6) > Not reasonable to ask Jonathan to pick this back up after this long - Mark, > perhaps you could have a go at addressing your feedback comments and flag me > for review again? Sure. Jonathan, can you please answer the following for me: * Are you aware of any need to keep the xpcom component? * The old patch had a pref to disable marionette, but it seems it marionette was only activated when the cmdline arg was used - which would seem mean the pref isn't actually needed. Is there something I'm missing here?
Assignee: nobody → mhammond
Flags: needinfo?(mhammond) → needinfo?(jgriffin)
We don't specifically need the xpcom component; I added it because I wasn't sure you would want this stuff added to e.g., nsBrowserContentHandler.js. The pref isn't useful for desktop Firefox, but is relevant for B2G (and maybe Android in the future).
Sorry, but I don't think I'm going to get to this anytime soon and leaving it assigned to me is misleading.
Assignee: markh → nobody
I just noticed this. Is it still the case that it is bad practice to handle flags in arbitrary XPCOM components? Should this still be moved to nsBrowserContentHandler.js?
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
(In reply to Andreas Tolfsen ‹:ato› from comment #10) > I just noticed this. Is it still the case that it is bad practice > to handle flags in arbitrary XPCOM components? Should this still > be moved to nsBrowserContentHandler.js? Gavin's no longer involved with Mozilla, but I see no reason to believe anything has changed in that time (but also no reason to believe it's a huge problem now that it has existed for a number of years)
Increasing the priority of this because -marionette has now disappeared from -h altogether. See https://bugzilla.mozilla.org/show_bug.cgi?id=1467453.
You need to log in before you can comment on or make changes to this bug.