963544 - Removing (DEBUG BUILD) WARNING messages from a few NS_ENSURE_TRUE() in dom/base//Navigator.cpp

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[ISHIKAWA, Chiaki]

Attachment: Proposed Patch. Open code NS_ENSURE_TRUE() and remove warning message.

While wading through the output of |make mozmill| test suite by
running DEBUG BUILD of Thunderbird (C-C) on x86 linux to look for
remaining issues that hinders our efforts to make TB a rock-solid mail
client (tm) , I noticed a few places that produced copious verbose WARNING
lines, but not so useful. 

I am proposing a patch to reduce such occurrences from Navigator.cpp

It produces repetitive and large number of WARNING lines that are
eliminated by patch proposed here: about 1000+ lines in total. It is
about 1% of the whole lines of |make mozmill| test suite output of
DEBUG BUILD of TB. The reader may think 1% is not a big number, but if
you read through the log output actually (with various scripting
filters), these are basically noise (repetitive and do not carry
much information), and should be eliminated (at least they do not mean  
much on x86 linux).

Basically, the attached patch modifies the three places where a
feature is checked for being enabled or not in preference and if not,
further processing inside a function is not done and the function
immediately returns.  

Problem is that the checking & returning currently uses
NS_ENSURE_TRUE(), and it causes WARNING output in DEBUG BUILD as a
side-effect.

The early return is *NORMAL and EXPECTED* when user preference does
not enable the checked feature, and so WARNING message is unnecessary IMHO.

Macros like NS_ENSURE_TRUE() are discouraged now since they
 - hide the flow of program, and
 - output WARNING lines (often without the developer not realizing it!).

See, for example, the discussion on devel-platform mailing list.
https://groups.google.com/forum/#!topic/mozilla.dev.platform/1clMLuuhtWQ
"Please use NS_WARN_IF instead of NS_ENSURE_SUCCESS"
(Karl Tomlinson's post on 7 Jan is to the point regarding the
unnecessary WARNING output.)

My point here is to reduce the clutter in the logging output of |make
mozmill| test suite run of DEBUG BUILD of TB (and presumably running
of DEBUG BUILD of FIREFOX using its test suites as well.)

So due to the reasons above, I suggest that we change the check of
NS_ENSURE_TRUE() to be open coded and no WARNING message is produced
in DEBUG BUILD?

There are other usage of NS_ENSURE_TRUE() in the file, but other
checks do not produce copious WARNING lines under x86 linux |make
mozmill| test suite run of DEBUG BUILD of TB, and not so bothersome.
I left others as is.

TIA

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8365050 [details] [diff] [review]
Proposed Patch. Open code NS_ENSURE_TRUE() and remove warning message.

I am probably not the best reviewer for this code.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8365050 [details] [diff] [review]
Proposed Patch. Open code NS_ENSURE_TRUE() and remove warning message.

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

::: dom/base/Navigator.cpp
@@ +1686,5 @@
>    // First of all, the general pref has to be turned on.
>    bool enabled = false;
>    Preferences::GetBool("dom.sms.enabled", &enabled);
> +  if (!enabled)
> +    return false;

Brace your one line ifs please

if (cond) {
  thing;
}

@@ +1874,5 @@
>  
>    // First of all, the general pref has to be turned on.
>    bool enabled = false;
>    Preferences::GetBool("dom.datastore.enabled", &enabled);
> +  if (!enabled) 

Extra whitespace at the end of the line.

Comment 3

[ISHIKAWA, Chiaki]

I found that second issue I mentioned about 
Fontconfig complaining about C.UTF-8 is a known debian issue
was supposedly fixed in the new version in Nov last year.
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=721275

(64-bit linux installation is for deveopment and I often update it to get the latest development tools, whereas 32-bit version is used for desktop work and I don't update the packages often for stability reasons.)

I will try to see if the updated version fixes the issue on my Debian GNU/Linux 32-bit and if so, then usage of C.UTF-8 with TB (and presumably FF) is possible with the proposed patch.

TIA

Comment 4

[ISHIKAWA, Chiaki]

(In reply to ISHIKAWA, Chiaki from comment #3)
> I found that second issue I mentioned about 
> Fontconfig complaining about C.UTF-8 is a known debian issue
> was supposedly fixed in the new version in Nov last year.
> http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=721275
> 
> (64-bit linux installation is for deveopment and I often update it to get
> the latest development tools, whereas 32-bit version is used for desktop
> work and I don't update the packages often for stability reasons.)
> 
> I will try to see if the updated version fixes the issue on my Debian
> GNU/Linux 32-bit and if so, then usage of C.UTF-8 with TB (and presumably
> FF) is possible with the proposed patch.
> 
> TIA

Sorry WRONG　POST to a different bugzilla! (
Bug 960957 - Remove support for *nix systems whose char* APIs for file names don't use UTF-8　）

Comment 5

[ISHIKAWA, Chiaki]

Attachment: Final Patch. Open code NS_ENSURE_TRUE() and remove warning message. Carrying over khuey: review+

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #2)
> Comment on attachment 8365050 [details] [diff] [review]
> Proposed Patch. Open code NS_ENSURE_TRUE() and remove warning message.
> 
> Review of attachment 8365050 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/Navigator.cpp
> @@ +1686,5 @@
> >    // First of all, the general pref has to be turned on.
> >    bool enabled = false;
> >    Preferences::GetBool("dom.sms.enabled", &enabled);
> > +  if (!enabled)
> > +    return false;
> 
> Brace your one line ifs please
> 
> if (cond) {
>   thing;
> }
> 
> @@ +1874,5 @@
> >  
> >    // First of all, the general pref has to be turned on.
> >    bool enabled = false;
> >    Preferences::GetBool("dom.datastore.enabled", &enabled);
> > +  if (!enabled) 
> 
> Extra whitespace at the end of the line.

Thank you for the review. I am uploading a fixed patch: braces and removal of extra whitespace.
(I will take the liberty of adding checkin-needed keyword.)

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/363517e27efd

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/363517e27efd