Closed
Bug 1451686
Opened 6 years ago
Closed 6 years ago
Change -MOZ_LOG* arguments to be in the form -MOZ_LOG=modules (and not -MOZ_LOG <space> modules)
Categories
(Core :: XPCOM, enhancement)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: mayhemer, Assigned: mayhemer)
Details
Attachments
(1 file, 2 obsolete files)
18.08 KB,
patch
|
mayhemer
:
review+
|
Details | Diff | Splinter Review |
I caught myself (and not just me) to type `./firefox -MOZ_LOG=something` instead of .`/firefox -MOZ_LOG something` and being curious why it doesn't work. I think it would be better to accept the params rather in the '=' form. The code would be simpler as well.
Assignee | ||
Comment 1•6 years ago
|
||
I think self-explanatory. Let's get this in soon, so that the `-MOZ_LOG modules` notation doesn't get spread.
Comment 2•6 years ago
|
||
Comment on attachment 8965283 [details] [diff] [review] v1 Review of attachment 8965283 [details] [diff] [review]: ----------------------------------------------------------------- This is fine, but I'd prefer if it supported both (the other options don't seem to support the '=' variant, but in general it's pretty common to support both). Also I just noticed your patch mentions '--MOZ_LOG' but you've actually implemented '-MOZ_LOG' (one dash). '--' as a prefix would be more in line with the existing options, although IIRC they support both for historical reasons. If you want to land this and file a follow up that's fine, otherwise maybe we can just rework it here.
Attachment #8965283 -
Flags: review?(erahm) → review+
Assignee | ||
Comment 3•6 years ago
|
||
so, this has changed a bit more: - we accept: --MOZ_LOG=module_list -MOZ_LOG=module_list --MOZ_LOG module_list -MOZ_LOG module_list (similarly for -MOZ_LOG_FILE) - code separated to its own file, using Tokenizer - public API via a static func with a callback (similar to NSPRLogModuleParser) - a gtest written I still would like to refer to this publicly with only one '-' so that it's simpler mainly for Windows users to start logging from start of the session easily (whole purpose of this bug)
Attachment #8965283 -
Attachment is obsolete: true
Attachment #8967417 -
Flags: review?(erahm)
Comment 4•6 years ago
|
||
Comment on attachment 8967417 [details] [diff] [review] v2 Review of attachment 8967417 [details] [diff] [review]: ----------------------------------------------------------------- Thanks separating this out and adding a test, it makes it a lot easier to review :) I have a few nits, but otherwise looks good. ::: xpcom/base/LogCommandLineHandler.cpp @@ +8,5 @@ > + > +#include "mozilla/Tokenizer.h" > +#include "nsDebug.h" > +#include "nsString.h" > +#include "prenv.h" nit: doesn't need prenv.h @@ +13,5 @@ > + > +namespace mozilla { > + > +void LoggingHandleCommandLineArgs(int argc, char * argv[], > + std::function<void(char const*)> consumer) WDYT about just having the callback take a `const nsACString&`? @@ +43,5 @@ > + } > + // We accept `-MOZ_LOG` as well as `--MOZ_LOG`. > + Unused << p.CheckChar('-'); > + > + for (auto name : names) { nit: const auto& ::: xpcom/base/LogCommandLineHandler.h @@ +9,5 @@ > + > +#include <functional> > + > +namespace mozilla { > + Can you add a doc for this? @@ +12,5 @@ > +namespace mozilla { > + > +void > +LoggingHandleCommandLineArgs(int argc, char* argv[], > + std::function<void(char const*)> consumer); nit: const std::function<...>& ::: xpcom/base/Logging.cpp @@ +14,5 @@ > #include "mozilla/StaticPtr.h" > #include "mozilla/Printf.h" > #include "mozilla/Atomics.h" > #include "mozilla/Sprintf.h" > +#include "mozilla/Tokenizer.h" nit: this shouldn't be needed here, right? @@ +215,5 @@ > + // Scripts can pass -MOZ_LOG=$MOZ_LOG,modules as an argument > + // to merge existing settings, if required. > + > + // PR_SetEnv takes ownership of the string. > + PR_SetEnv(PL_strdup(env)); We can just use `strdup` now, right? ::: xpcom/tests/gtest/TestLogCommandLineHandler.cpp @@ +98,5 @@ > + results.Clear(); > + mozilla::LoggingHandleCommandLineArgs(std::size(argv4), argv4, callback); > + EXPECT_TRUE(results.Length() == 2); > + EXPECT_TRUE(NS_LITERAL_CSTRING("MOZ_LOG=modules").EqualsASCII(results[0].get())); > + EXPECT_TRUE(NS_LITERAL_CSTRING("MOZ_LOG_FILE=file").EqualsASCII(results[1].get())); Can you add something like { "", "--MOZ_LOG", "modules", "-P", "foo", "--MOZ_LOG_FILE", "file" }
Attachment #8967417 -
Flags: review?(erahm) → review+
Assignee | ||
Comment 5•6 years ago
|
||
All comments addressed, thanks!
Attachment #8967417 -
Attachment is obsolete: true
Attachment #8967703 -
Flags: review+
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/40352eb9a1cf Allow also -MOZ_LOG/_FILE=value form of the logging arguments. r=erahm
Keywords: checkin-needed
Comment 7•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/40352eb9a1cf
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 8•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/40352eb9a1cf
You need to log in
before you can comment on or make changes to this bug.
Description
•