1173858 - Silence remaining ~43,000 instances of verbose layout warnings when testing

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Eric Rahm [:erahm]]

There are about 43,000 remaining overly verbose warning instances when testing, lets go ahead and silence those.

Comment 1

[Eric Rahm [:erahm]]

Attachment: Silence verbose layout warnings

Comment 2

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=16a9ad51bf6c

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8621075 [details] [diff] [review]
Silence verbose layout warnings

These warnings should probably remain, but be opt-in (with MOZ_LOG) IMO.

These are all reasonably important, I think; they're aiming to catch cases where layout code might accidentally pass in the sentinel-value NS_UNCONSTRAINEDSIZE (2^30) where it should not. Of course, that testcases which use huge sizes will end up working with sizes that happen to be that huge, and will trip this warning as a false positive; that's why we trip these warnings in our crashtest suite.  These warnings are still useful for hinting at problems when we're debugging real-world (non-huge) content, though.

(Sorry for difference in goal from Bug 1171528 -- that bug ended up being a special case because its warning had been cribbed from elsewhere, and the "elsewhere" code had removed the warnings. So there, it most sense to remove the warning. Not here, though.)

Comment 4

[Eric Rahm [:erahm]]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> Comment on attachment 8621075 [details] [diff] [review]
> Silence verbose layout warnings
> 
> These warnings should probably remain, but be opt-in (with MOZ_LOG) IMO.

Sure as long as you're convinced all 7 are worth keeping. Which log are you thinking of using? Should we just do something similar to the second attempt at bug 1171528? You seemed to have an opinion on naming etc so I'd just like to get that cleared up now before generating another patch.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Eric Rahm [:erahm] from comment #4)
> Sure as long as you're convinced all 7 are worth keeping.

That's a high bar. :) I'd rather say that I'm unconvinced that any of these clearly merit removing.

(They're all testing approximately the same thing, so I think they're all equally sane. And the thing they're testing is a useful thing to test.  I've tripped over some of these & it's helped me catch bugs in my local patches the past.)

> Which log are you
> thinking of using? Should we just do something similar to the second attempt
> at bug 1171528?

Similar to that, yeah. Maybe a new header called layout/base/LayoutLogging.h?

(CC'ing dbaron; I'd suggest getting his feedback or at least tacit-approval on this, as he's module owner and may want to chime in on naming here before we've gone too far rewriting warnings.)

Also: note that these are currently all NS_WARN_IF -- i.e. they're evaluating a condition before logging, and they're *only* evaluating the condition (and only logging) in debug builds. We should preserve that, to avoid this causing a (minor) perf hit in opt builds from evaluating these conditions for no reason. Not sure if that means we need MOZ_LOG_IF, or perhaps #ifdef DEBUG\n MOZ_LOG_IF(...)\n #endif, or maybe something else.

Comment 6

[Daniel Holbert [:dholbert]]

(Also, one thought I had about attachment 8620432 [details] [diff] [review] (second attempt on bug 1171528): is it problematic that we're creating a static PRLogModuleInfo, in function-defn that lives in a header-file? That means each .cpp file that uses this API will get its own distinct PRLogModuleInfo, I think. Does that matter? i.e. maybe we should define this hypothetical "GetLayoutLog" function in a .cpp file, so there's only one shared PRLogModuleInfo?)

Comment 7

[Eric Rahm [:erahm]]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> (In reply to Eric Rahm [:erahm] from comment #4)
> > Sure as long as you're convinced all 7 are worth keeping.
> 
> That's a high bar. :) I'd rather say that I'm unconvinced that any of these
> clearly merit removing.
> 
> (They're all testing approximately the same thing, so I think they're all
> equally sane. And the thing they're testing is a useful thing to test.  I've
> tripped over some of these & it's helped me catch bugs in my local patches
> the past.)
> 
> > Which log are you
> > thinking of using? Should we just do something similar to the second attempt
> > at bug 1171528?
> 
> Similar to that, yeah. Maybe a new header called layout/base/LayoutLogging.h?

Sounds good.

> (CC'ing dbaron; I'd suggest getting his feedback or at least tacit-approval
> on this, as he's module owner and may want to chime in on naming here before
> we've gone too far rewriting warnings.)

I'll f? on the patch.

> Also: note that these are currently all NS_WARN_IF -- i.e. they're
> evaluating a condition before logging, and they're *only* evaluating the
> condition (and only logging) in debug builds. We should preserve that, to
> avoid this causing a (minor) perf hit in opt builds from evaluating these
> conditions for no reason.

The condition is always evaluated [1], although the message is only output in debug builds. I *would* hope that this gets optimized out in opt builds if the value isn't used.

I don't think the debug only part matters if we're switching to MOZ_LOG (the message will only be created if the log level is enabled), but I can wrap in #ifdef DEBUG if you'd prefer.

> Not sure if that means we need MOZ_LOG_IF, or
> perhaps #ifdef DEBUG\n MOZ_LOG_IF(...)\n #endif, or maybe something else.

MOZ_LOG_IF (or some variant) would be a good future improvement. For now I think we can do something specific for layout like the following psuedo-code:

> #ifdef DEBUG
>   #define LAYOUT_DWARN_IF(cond, msg) if (cond) MOZ_LOG(layoutLog, Warning, (msg)); 
> #else
>   #define LAYOUT_DWARN_IF(cond, msg)
> #endif

And then your messages become:
>  LAYOUT_DWARN_IF(value == overflow, "There was an overflow");

Or we could remove the debug-only restriction and go with |LAYOUT_WARN_IF|

[1] https://hg.mozilla.org/mozilla-central/annotate/bfd82015df48/xpcom/glue/nsDebug.h#l48

Comment 8

[Eric Rahm [:erahm]]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> (Also, one thought I had about attachment 8620432 [details] [diff] [review]
> (second attempt on bug 1171528): is it problematic that we're creating a
> static PRLogModuleInfo, in function-defn that lives in a header-file? That
> means each .cpp file that uses this API will get its own distinct
> PRLogModuleInfo, I think. Does that matter? i.e. maybe we should define this
> hypothetical "GetLayoutLog" function in a .cpp file, so there's only one
> shared PRLogModuleInfo?)

I can never quite remember the rules on this, regardless it should probably be in a cpp file.

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Eric Rahm [:erahm] from comment #7)
> > Also: note that these are currently all NS_WARN_IF -- i.e. they're
> > evaluating a condition before logging, and they're *only* evaluating the
> > condition (and only logging) in debug builds.
[...]
> The condition is always evaluated [1], although the message is only output
> in debug builds.

Sorry, I mis-spoke -- these are using NS_WARN_IF_FALSE, which behaves as I described (it doesn't evaluate the condition in debug builds).[1]

(NS_WARN_IF is completely different and is meant to be used inside of an "if(...)" statement, which is why it's functional in opt builds)

> I *would* hope that this gets optimized out in opt builds if the value isn't used.

If there's a virtual function-call in the condition (e.g. frame->IsFrameOfType(nsIFrame::eReplaced) in one of these cases), I don't think it can be optimized out, regardless of whether its value is used.

> LAYOUT_DWARN_IF

This seems reasonable, though we shouldn't make it sound so similar to NS_WARN_IF (since NS_WARN_IF is special & meant to be used inside of if() statements, as I've now remembered).

Maybe LAYOUT_WARN_IF_FALSE? (so, identical in behavior to the currently-used NS_WARN_IF_FALSE, except that it's also restricted to require layout logging to be on) 

[1] https://hg.mozilla.org/mozilla-central/annotate/bfd82015df48/xpcom/glue/nsDebug.h#l68

Comment 10

[Daniel Holbert [:dholbert]]

(or LAYOUT_DWARN_IF_FALSE, if you like. I'm not sure the "D" is necessary, since the analogous NS_WARN_IF_FALSE is already debug-only; and I doubt we'll create a non-debug version of this macro, but I don't have strong feelings on DWARN vs WARN)

Comment 11

[Eric Rahm [:erahm]]

(In reply to Daniel Holbert [:dholbert] from comment #9)
> (NS_WARN_IF is completely different and is meant to be used inside of an
> "if(...)" statement, which is why it's functional in opt builds)

Oh wow that difference in behavior is gloriously confusing :(

> > LAYOUT_DWARN_IF
> 
> This seems reasonable, though we shouldn't make it sound so similar to
> NS_WARN_IF (since NS_WARN_IF is special & meant to be used inside of if()
> statements, as I've now remembered).
> 
> Maybe LAYOUT_WARN_IF_FALSE? (so, identical in behavior to the currently-used
> NS_WARN_IF_FALSE, except that it's also restricted to require layout logging
> to be on) 

Yes, LAYOUT_WARN_IF_FALSE sounds good.

> (or LAYOUT_DWARN_IF_FALSE, if you like. I'm not sure the "D" is necessary,
> since the analogous NS_WARN_IF_FALSE is already debug-only; and I doubt
> we'll create a non-debug version of this macro, but I don't have strong
> feelings on DWARN vs WARN)

Since this will behave the same as WARN_IF_FALSE I think it's fine to omit the 'D'.

Comment 12

[Eric Rahm [:erahm]]

Attachment: Part 1: Add log module for layout

Comment 13

[Eric Rahm [:erahm]]

Attachment: Part 2: Silence verbose layout warnings

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8621181 [details] [diff] [review]
Part 1: Add log module for layout

>+++ b/layout/base/LayoutLogging.cpp
>+PRLogModuleInfo* GetLayoutLog()
>+{
>+  static PRLogModuleInfo* log = nullptr;
>+  if (!log) {
>+    log = PR_NewLogModule("layout");
>+  }
>+
>+  return log;
>+}
>+
>diff --git a/layout/base/LayoutLogging.h b/layout/base/LayoutLogging.h

Nit: Both of your new files currently end with an extra blank line (as shown above for the end of the .cpp file).  Drop that blank line.

(Files *should* end with a newline *character* at the end of the last line -- and your editor may handle that automatically. But there shouldn't be a blank line beyond that.)

>+++ b/layout/base/LayoutLogging.h
>+#ifndef LayoutLogging_h__
>+#define LayoutLogging_h__

Nit: the coding style guide says (implicitly) that #include-guards shouldn't actually have trailing underscores. (I confirmed this with Ms2ger yesterday; he wrote this section of the coding style guide several years ago)

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#C.2FC.2B.2B_practices

So, I think this should just be:
#ifndef LayoutLogging_h
etc.

>+#define LAYOUT_WARN_IF_FALSE(_cond, _msg)                          \
>+  PR_BEGIN_MACRO                                                   \
>+    if (!(_cond)) {                                                \
>+      MOZ_LOG(GetLayoutLog(), mozilla::LogLevel::Warning, (_msg)); \
>+    }                                                              \
>+  PR_END_MACRO
>+#else
>+#define LAYOUT_WARN_IF_FALSE(_cond, _msg)
>+#endif

The "else" version needs to define it to something like:
  do { /* nothing */ } while(0)
as we do for e.g. NS_WARN_IF_FALSE:
 http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsDebug.h#68
and MOZ_ASSERT:
 http://mxr.mozilla.org/mozilla-central/source/mfbt/Assertions.h#385

Otherwise, in a non-debug build, this code:
  LAYOUT_WARN_IF_FALSE(condition, blah);
...would end up as just a semicolon:
  ;

...which I think can be problematic in some pathological cases. (E.g. if you forgot to add a semicolon at the end of the previous line, then the current formulation would make your code still mysteriously compile, only in non-debug builds, because this macro would end up accidentally providing the semicolon for the previous line).

>+#endif // LayoutLogging_h__
>+

(Drop trailing underscores and trailing blank line)

r=me with the above, as long as dbaron is OK with this too.

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8621183 [details] [diff] [review]
Part 2: Silence verbose layout warnings

r=me with the following:

>Bug 1173858 - Part 2: Silence verbose layout warnings. r=dholbert

This commit message is a bit hand-wavy. Better:

"Use LAYOUT_WARN_IF_FALSE to silence some verbose layout warnings by default"

>+++ b/layout/generic/nsBlockReflowState.cpp
> /* state used in reflow of block frames */
> 
> #include "nsBlockReflowState.h"
> 
>+#include "LayoutLogging.h"
>+
> #include "mozilla/DebugOnly.h"
> 
> #include "nsBlockFrame.h"
> #include "nsLineLayout.h"
> #include "nsPresContext.h

This should just join the existing main block of #includes here; it doesn't need its own blank-line-padded section.  So, it probably should be inserted just before the nsBlockFrame.h include.

(I think the DebugOnly #include-line here seems out of place -- there are a bunch of mozilla/*.h includes lower down which it should join. Feel free to bump that down as well while you're here, but no pressure.)

>-  NS_WARN_IF_FALSE(NS_UNCONSTRAINEDSIZE != aReflowState.ComputedISize(),
>+  LAYOUT_WARN_IF_FALSE(NS_UNCONSTRAINEDSIZE != aReflowState.ComputedISize(),
>                    "have unconstrained width; this should only result from "
>                    "very large sizes, not attempts at intrinsic width "
>                    "calculation");

Please reindent the body of these macros. (rewrapping the warning-text if necessary, to keep things under 80 characters)

(If you want to do the reindenting [/rewrapping] as a "part 3", so that this patch is just a mechanical S/NS/LAYOUT/, that's fine with me too.)

Comment 16

[Eric Rahm [:erahm]]

(In reply to Daniel Holbert [:dholbert] from comment #14)

> Nit: Both of your new files currently end with an extra blank line (as shown
> above for the end of the .cpp file).  Drop that blank line.

I'll clean that up.

> >+++ b/layout/base/LayoutLogging.h
> >+#ifndef LayoutLogging_h__
> >+#define LayoutLogging_h__
> 
> Nit: the coding style guide says (implicitly) that #include-guards shouldn't
> actually have trailing underscores. (I confirmed this with Ms2ger yesterday;
> he wrote this section of the coding style guide several years ago)

I usually just do what the other files in the directory are doing (I chose the wrong file of course), it's good to know we have a standard! I'll remove the trailing underscores.

> 
> >+#define LAYOUT_WARN_IF_FALSE(_cond, _msg)                          \
> >+  PR_BEGIN_MACRO                                                   \
> >+    if (!(_cond)) {                                                \
> >+      MOZ_LOG(GetLayoutLog(), mozilla::LogLevel::Warning, (_msg)); \
> >+    }                                                              \
> >+  PR_END_MACRO
> >+#else
> >+#define LAYOUT_WARN_IF_FALSE(_cond, _msg)
> >+#endif
> 
> The "else" version needs to define it to something like:
>   do { /* nothing */ } while(0)

I'll update w/ an empty PR_BEGIN/END block.

Comment 17

[Eric Rahm [:erahm]]

(In reply to Daniel Holbert [:dholbert] from comment #15)
> This commit message is a bit hand-wavy. Better:
> 
> "Use LAYOUT_WARN_IF_FALSE to silence some verbose layout warnings by default"

Sure, this seems more explicit.

> This should just join the existing main block of #includes here; it doesn't
> need its own blank-line-padded section.  So, it probably should be inserted
> just before the nsBlockFrame.h include.
> 
> (I think the DebugOnly #include-line here seems out of place -- there are a
> bunch of mozilla/*.h includes lower down which it should join. Feel free to
> bump that down as well while you're here, but no pressure.)

I was going for alphabetical, clearly that's a lost cause here :) I'll move those two down.

> Please reindent the body of these macros. (rewrapping the warning-text if
> necessary, to keep things under 80 characters)
> 
> (If you want to do the reindenting [/rewrapping] as a "part 3", so that this
> patch is just a mechanical S/NS/LAYOUT/, that's fine with me too.)

I'll add a 3rd patch, the whitespace changes from reindenting made this one too noisy.

Comment 18

[Eric Rahm [:erahm]]

Attachment: Part 3 - Reindent blocks convert to use LAYOUT_WARN_IF

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8621249 [details] [diff] [review]
Part 3 - Reindent blocks convert to use LAYOUT_WARN_IF

># HG changeset patch
># User Eric Rahm <[e-mail]>
># Date 1434058800 25200
>#      Thu Jun 11 14:40:00 2015 -0700
># Node ID d38707dd2d934c3ce8fb521eab39083c31b8ddfb
># Parent  3ce89aa5559e4ca344adb6b065c7b27d83d8931f
>Bug 1173858 - Part 3 - Reindent blocks convert to use LAYOUT_WARN_IF. r=dholber

s/convert/that we've converted/

s/dholber/dholbert/

r=me

Comment 20

[Eric Rahm [:erahm]]

There was discussion on mozilla.dev.tech.layout [1] about this, I isolated most of the warnings down to one crash test, |layout/generic/crashtests/421671.html| [2].

[1] https://groups.google.com/forum/#!topic/mozilla.dev.tech.layout/YXauN50HDhI
[2] https://hg.mozilla.org/mozilla-central/annotate/0093691d3715/layout/generic/crashtests/421671.html

Comment 21

[Eric Rahm [:erahm]]

Note: the new way to see these logs is to turn on the 'layout' log module:
> NSPR_LOG_MODULES='layout:2' ./mach run
will enable Error and Warning level log output. Specifying a higher number will enable more verbose logging.

Comment 22

[Daniel Holbert [:dholbert]]

I think this might not be quite ready actually; it looks like this makes our logging go from this form...

[Child 649] WARNING: have unconstrained width; this should only result from very large sizes, not attempts at intrinsic width calculation: 'aISize != NS_UNCONSTRAINEDSIZE', file /scratch/work/builds/mozilla-inbound/mozilla/layout/generic/nsLineLayout.cpp, line 160

...to this form:
-524822016[7f9dcd85e140]: have unconstrained width; this should only result from very large sizes, not attempts at intrinsic width calculation

So we're losing:
 (a) Process ID and child/parent label.
 (b) Source file & line number
 (c) the condition being tested
and:
 (d) we're gaining a cryptic numeric prefix.

I think we really need to preserve (a) and (b).  (If necessary, maybe we can preserve (b) via editing the logged messages to include the name of the method they're inside, or something similar.)  I'm less concerned about (c), though it'd be nice to keep as well. And (d) is just confusing; it'd be nice to lose it.

Also, if dbaron doesn't have cycles to take a look at this (sounds like he might not), perhaps mats can offer some feedback/thoughts.

Comment 23

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #22)
> So we're losing:

Forgot to mention: we're also losing "WARNING". I think that's valuable to have in the logging. (to indicate the severity).  Maybe the MOZ_LOG infrastructure should include that (a textual representation of the log-level) by default?

Comment 24

[Eric Rahm [:erahm]]

(In reply to Daniel Holbert [:dholbert] from comment #22)
> I think this might not be quite ready actually; it looks like this makes our
> logging go from this form...
> 
> [Child 649] WARNING: have unconstrained width; this should only result from
> very large sizes, not attempts at intrinsic width calculation: 'aISize !=
> NS_UNCONSTRAINEDSIZE', file
> /scratch/work/builds/mozilla-inbound/mozilla/layout/generic/nsLineLayout.cpp,
> line 160
> 
> ...to this form:
> -524822016[7f9dcd85e140]: have unconstrained width; this should only result
> from very large sizes, not attempts at intrinsic width calculation
> 
> So we're losing:
>  (a) Process ID and child/parent label.
>  (b) Source file & line number
>  (c) the condition being tested
> and:
>  (d) we're gaining a cryptic numeric prefix.
> 
> I think we really need to preserve (a) and (b).  (If necessary, maybe we can
> preserve (b) via editing the logged messages to include the name of the
> method they're inside, or something similar.)  I'm less concerned about (c),
> though it'd be nice to keep as well. And (d) is just confusing; it'd be nice
> to lose it.

Unfortunately I can't do anything about (d) (it's an NSPR thing). (a), (b), and (c) are certainly doable.

> Forgot to mention: we're also losing "WARNING". I think that's valuable to
> have in the logging. (to indicate the severity).  

I'll add this as well.

> Maybe the MOZ_LOG infrastructure should include that (a textual representation of the
> log-level) by default?

Yes that would be a nice improvment, it's on my list of things to do as we gradually switch over from NSPR logging.

Comment 25

[Daniel Holbert [:dholbert]]

(In reply to Eric Rahm [:erahm] from comment #24)
> >  (d) we're gaining a cryptic numeric prefix.
[...]
> 
> Unfortunately I can't do anything about (d) (it's an NSPR thing).

Hmm... That is unfortunate.  This 26-character gibberish prefix makes MOZ_LOG a good bit less attractive as the new hot way to do opt-in logging.  Has this come up as an issue anywhere else where we're using MOZ_LOG?

Could you make sure there's a bug filed about this, at least?

Comment 26

[Eric Rahm [:erahm]]

Attachment: Part 1: Add log module for layout

Add a log module for use by layout. An analog to NS_WARN_IF_FALSE is provided
that has the same behavior as NS_WARN_IF_FALSE: it's debug only and emits a
message prefixed with '[pid] WARNING', includes the condition being checked,
file name and line number.

Comment 27

[Eric Rahm [:erahm]]

Attachment: Part 2: Use LAYOUT_WARN_IF to silence some verbose layout warnings by default

Comment 28

[Eric Rahm [:erahm]]

Attachment: Part 3 - Reindent blocks that we've converted to use LAYOUT_WARN_IF

Comment 29

[Eric Rahm [:erahm]]

Comment on attachment 8622654 [details] [diff] [review]
Part 2: Use LAYOUT_WARN_IF to silence some verbose layout warnings by default

Updated per dholbert's recommendations. Carrying forward r+

Comment 30

[Eric Rahm [:erahm]]

Comment on attachment 8622656 [details] [diff] [review]
Part 3 - Reindent blocks that we've converted to use LAYOUT_WARN_IF

Updated commit message, carrying forward r+.

Comment 31

[Daniel Holbert [:dholbert]]

Could you post a sample of what the warning output looks like, with the new patch?

Comment 32

[Eric Rahm [:erahm]]

(In reply to Daniel Holbert [:dholbert] from comment #31)
> Could you post a sample of what the warning output looks like, with the new
> patch?

Example from running a crashtest:

> [erahm@mozilla21268 mozilla-central]$ NSPR_LOG_MODULES='layout:2' ./mach crashtest layout/generic/crashtests/421671.html
> ...snip...
> 0:13.16 -857837952[2b09ce13a5c0]: [24969] WARNING: have unconstrained inline-size; this should only result from very large sizes, not attempts at intrinsic inline-size calculation: '(mFrameType == NS_CSS_FRAME_TYPE_INLINE && !frame->IsFrameOfType(nsIFrame::eReplaced)) || type == nsGkAtoms::textFrame || ComputedISize() != NS_UNCONSTRAINEDSIZE', file /home/erahm/dev/mozilla-central/layout/generic/nsHTMLReflowState.cpp, line 455

Comment 33

[Eric Rahm [:erahm]]

Sorry missed this.

(In reply to Daniel Holbert [:dholbert] from comment #25)
> (In reply to Eric Rahm [:erahm] from comment #24)
> > >  (d) we're gaining a cryptic numeric prefix.
> [...]
> > 
> > Unfortunately I can't do anything about (d) (it's an NSPR thing).
> 
> Hmm... That is unfortunate.  This 26-character gibberish prefix makes
> MOZ_LOG a good bit less attractive as the new hot way to do opt-in logging. 
> Has this come up as an issue anywhere else where we're using MOZ_LOG?

Not that I know of, my guess is that historically users of PR_LOG (MOZ_LOG is just a wrapper around this at the moment) are used to it (or even want it).

> Could you make sure there's a bug filed about this, at least?

MOZ_LOG is moving away from using NSPR internally; I'm not sure it's worth filing a bug. Feel free to file one of course, but I doubt they'd accept a change as this is intended behavior.

Comment 34

[Daniel Holbert [:dholbert]]

Comment on attachment 8622652 [details] [diff] [review]
Part 1: Add log module for layout

Thanks, that's way better than comment 22.

Stylistic nits, RE namespaces:
>+++ b/layout/base/LayoutLogging.h
>+
>+namespace mozilla
>+{
>+namespace detail
>+{
>+
>+void layout_warning(const char* aStr, const char* aExpr,
>+                    const char* aFile, int32_t aLine);
>+
>+}
>+}

Per https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes , put brace on same line as namespace, and mention the namespace name on the close-brace.

namespace mozilla {
namespace detail {
...
} // namespace detail
} // namespace mozilla

>+++ b/layout/base/LayoutLogging.cpp
>+void mozilla::detail::layout_warning(const char* aStr, const char* aExpr,
>+                                     const char* aFile, int32_t aLine)
>+{
>+  MOZ_LOG(GetLayoutLog(),
>+          mozilla::LogLevel::Warning,
>+          ("[%d] WARNING: %s: '%s', file %s, line %d",
>+           base::GetCurrentProcId(),
>+           aStr, aExpr, aFile, aLine));
>+}

I think this should just be wrapped in...
 namespace mozilla {
 namespace detail {
...just like the .h file is.

(Also: it looks like we did lose the "Child"/"Parent" process labels. For existing NS_WARNING/NS_ASSERTION output, it looks like that label comes from nsDebugImpl::SetMultiprocessMode, which I guess we can't easily use here. Oh well.)

Comment 35

[Daniel Holbert [:dholbert]]

(In reply to Eric Rahm [:erahm] from comment #33)
> MOZ_LOG is moving away from using NSPR internally; I'm not sure it's worth
> filing a bug. Feel free to file one of course, but I doubt they'd accept a
> change as this is intended behavior.

Do you know if there's a bug filed on MOZ_LOG switching away from NSPR logging, then? Mainly, I'm curious how long we have to live with these prefixes. If that'll be fixed tangentially by switching the logging backend, I want to keep track of when that might happen. :)

Comment 36

[Eric Rahm [:erahm]]

(In reply to Daniel Holbert [:dholbert] from comment #35)
> (In reply to Eric Rahm [:erahm] from comment #33)
> > MOZ_LOG is moving away from using NSPR internally; I'm not sure it's worth
> > filing a bug. Feel free to file one of course, but I doubt they'd accept a
> > change as this is intended behavior.
> 
> Do you know if there's a bug filed on MOZ_LOG switching away from NSPR
> logging, then? Mainly, I'm curious how long we have to live with these
> prefixes. If that'll be fixed tangentially by switching the logging backend,
> I want to keep track of when that might happen. :)

There's a pretty extensive discussion of potential logging improvements in bug 881389 and it's dependent/see also bugs. There isn't a specific bug for switching backends yet, but I can file one and CC you on it. I imagine there will be a bit of back and forth on the format of the output.

Comment 37

[Mats Palmgren (inactive)]

Comment on attachment 8622652 [details] [diff] [review]
Part 1: Add log module for layout

>+void mozilla::detail::layout_warning(...

Is there a reason to not use standard CamelCase naming here?
Maybe include the word "log" as well?  (LayoutLogWarning)

Comment 38

[Daniel Holbert [:dholbert]]

Comment on attachment 8622652 [details] [diff] [review]
Part 1: Add log module for layout

Three more nits:

>+++ b/layout/base/LayoutLogging.h
>+/**
>+ * Use the layout log to warn if a given condition is false.
>+ *
>+ * This is only enabled in debug builds and LogLevel::Warning must be enabled
>+ * to see these messages.

(1) This "LogLevel::Warning must be enabled" documentation is a bit abstract. (maybe that's my fault for not having read up on MOZ_LOG enough)

Perhaps this should say something like:
 [...]and the logging is only displayed if the environmental variable
 NSPR_LOG_MODULES includes "layout:2" (or higher).

(This may become wrong if & when we do switch logging backends. But for now, it's more helpful than "LogLevel::Warning must be enabled" I think, for someone looking at this file & wondering how to make the warnings show up.)

Anyway, feel free to disregard this nit, if this is sufficiently-documented somewhere else.

>+#ifdef DEBUG
>+#define LAYOUT_WARN_IF_FALSE(_cond, _msg)                                \
>+  PR_BEGIN_MACRO                                                         \
>+    if (!(_cond) &&                                                      \
>+        MOZ_LOG_TEST(GetLayoutLog(), mozilla::LogLevel::Warning)) {      \
>+      mozilla::detail::layout_warning(_msg, #_cond, __FILE__, __LINE__); \

(2) I think we should reorder the conditions here.  First, test if logging is enabled; second, test !(_cond).  (If the condition is semi-expensive, and logging is disabled, then the current patch will make us spin our wheels evaluating it for no good reason.)

(3) Why are we using the "detail" namespace here? We probably should just put this function in "namespace mozilla {".

(and as mats observed, camelcase naming is preferred)

Comment 39

[Eric Rahm [:erahm]]

(In reply to Daniel Holbert [:dholbert] from comment #34)
> Comment on attachment 8622652 [details] [diff] [review]
> Part 1: Add log module for layout
> 
> Thanks, that's way better than comment 22.
> 
> Stylistic nits, RE namespaces:
> >+++ b/layout/base/LayoutLogging.h
> >+
> >+namespace mozilla
> >+{
> >+namespace detail
> >+{
> >+
> >+void layout_warning(const char* aStr, const char* aExpr,
> >+                    const char* aFile, int32_t aLine);
> >+
> >+}
> >+}
> 
> Per
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Coding_Style#Classes , put brace on same line as namespace, and mention the
> namespace name on the close-brace.
> 
> namespace mozilla {
> namespace detail {
> ...
> } // namespace detail
> } // namespace mozilla

I'll update the formatting and add comments.

> >+++ b/layout/base/LayoutLogging.cpp
> >+void mozilla::detail::layout_warning(const char* aStr, const char* aExpr,
> >+                                     const char* aFile, int32_t aLine)
> >+{
> >+  MOZ_LOG(GetLayoutLog(),
> >+          mozilla::LogLevel::Warning,
> >+          ("[%d] WARNING: %s: '%s', file %s, line %d",
> >+           base::GetCurrentProcId(),
> >+           aStr, aExpr, aFile, aLine));
> >+}
> 
> I think this should just be wrapped in...
>  namespace mozilla {
>  namespace detail {
> ...just like the .h file is.

I'll update to use nested namespaces as above.

> (Also: it looks like we did lose the "Child"/"Parent" process labels. For
> existing NS_WARNING/NS_ASSERTION output, it looks like that label comes from
> nsDebugImpl::SetMultiprocessMode, which I guess we can't easily use here. Oh
> well.)

Yes that was my conclusion as well :( I think we can address this when switching logging backends, I'll make a note of it.

(In reply to Mats Palmgren (:mats) from comment #37)
> Comment on attachment 8622652 [details] [diff] [review]
> Part 1: Add log module for layout
> 
> >+void mozilla::detail::layout_warning(...
> 
> Is there a reason to not use standard CamelCase naming here?
> Maybe include the word "log" as well?  (LayoutLogWarning)

Thanks for the quick feedback! No good reason, I'll switch to LayoutLogWarning.

(In reply to Daniel Holbert [:dholbert] from comment #38)
> Comment on attachment 8622652 [details] [diff] [review]
> Part 1: Add log module for layout
> 
> Three more nits:
> 
> >+++ b/layout/base/LayoutLogging.h
> >+/**
> >+ * Use the layout log to warn if a given condition is false.
> >+ *
> >+ * This is only enabled in debug builds and LogLevel::Warning must be enabled
> >+ * to see these messages.
> 
> (1) This "LogLevel::Warning must be enabled" documentation is a bit
> abstract. (maybe that's my fault for not having read up on MOZ_LOG enough)
> 
> Perhaps this should say something like:
>  [...]and the logging is only displayed if the environmental variable
>  NSPR_LOG_MODULES includes "layout:2" (or higher).
> 
> (This may become wrong if & when we do switch logging backends. But for now,
> it's more helpful than "LogLevel::Warning must be enabled" I think, for
> someone looking at this file & wondering how to make the warnings show up.)
> 
> Anyway, feel free to disregard this nit, if this is sufficiently-documented
> somewhere else.

Good point, a comment here on how to actually enable the log level would be useful. For the time being I plan on remaining backwards compatible with how we turn on NSPR logging, so the comment should remain useful.

> 
> >+#ifdef DEBUG
> >+#define LAYOUT_WARN_IF_FALSE(_cond, _msg)                                \
> >+  PR_BEGIN_MACRO                                                         \
> >+    if (!(_cond) &&                                                      \
> >+        MOZ_LOG_TEST(GetLayoutLog(), mozilla::LogLevel::Warning)) {      \
> >+      mozilla::detail::layout_warning(_msg, #_cond, __FILE__, __LINE__); \
> 
> (2) I think we should reorder the conditions here.  First, test if logging
> is enabled; second, test !(_cond).  (If the condition is semi-expensive, and
> logging is disabled, then the current patch will make us spin our wheels
> evaluating it for no good reason.)

I'll swap them.

> (3) Why are we using the "detail" namespace here? We probably should just
> put this function in "namespace mozilla {".
> 
> (and as mats observed, camelcase naming is preferred)

Generally things that shouldn't be called directly are placed in a detail namespace (I was encouraged to do this in mozilla/Logging.h by :froydnj). In this case we definitely do not want to encourage calling |LayoutLogWarning| directly as this misses the debug only aspect and the log level test.

Comment 40

[Daniel Holbert [:dholbert]]

(In reply to Eric Rahm [:erahm] from comment #39)
> Generally things that shouldn't be called directly are placed in a detail
> namespace (I was encouraged to do this in mozilla/Logging.h by :froydnj). In
> this case we definitely do not want to encourage calling |LayoutLogWarning|
> directly as this misses the debug only aspect and the log level test.

Interesting, ok. I was unaware of this convention, but it seems like a good idea. (Kind of like anonymous namespaces, but with a (somewhat) Well Known Name instead of no name, I guess.)

Thanks for the explanation.

Comment 41

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e50081be9bb

Comment 42

[Daniel Holbert [:dholbert]]

Just noticed from that try run that the commit messages need updating -- they say LAYOUT_WARN_IF, but they should say LAYOUT_WARN_IF_FALSE.

Comment 43

[Eric Rahm [:erahm]]

(In reply to Daniel Holbert [:dholbert] from comment #42)
> Just noticed from that try run that the commit messages need updating --
> they say LAYOUT_WARN_IF, but they should say LAYOUT_WARN_IF_FALSE.

Good catch! I'll update it.

Comment 44

[Eric Rahm [:erahm]]

Try looks happy, lets land this.

Comment 45

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e1ebe4e8755
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b8b27c82201
https://hg.mozilla.org/integration/mozilla-inbound/rev/a748653e5358

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5e1ebe4e8755
https://hg.mozilla.org/mozilla-central/rev/2b8b27c82201
https://hg.mozilla.org/mozilla-central/rev/a748653e5358