Open Bug 882846 Opened 6 years ago Updated Last year

Move -marionette CLI-handling code into the browser

Categories

(Testing :: Marionette, defect, P3)

defect

Tracking

(Not tracked)

People

(Reporter: jgriffin, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: pi-marionette-server)

Attachments

(1 file)

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.
@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
Whiteboard: [runner]
Whiteboard: [runner]
Assignee: jgriffin → nobody
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?
Flags: needinfo?(mhammond)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.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).
Flags: needinfo?(jgriffin)
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?
Flags: needinfo?(gavin.sharp)
Priority: -- → P5
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)
Flags: needinfo?(gavin.sharp)
Increasing the priority of this because -marionette has now disappeared
from -h altogether.  See
https://bugzilla.mozilla.org/show_bug.cgi?id=1467453.
Priority: P5 → P3
You need to log in before you can comment on or make changes to this bug.