660771 - parser should use mozilla::Preferences

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 536257 [details] [diff] [review]
Patch v1.0

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

r+ with that fixed

::: parser/htmlparser/src/nsViewSourceHTML.cpp
@@ +204,2 @@
>  
> +  mWrapLongLines = Preferences::GetBool("view_source.wrap_long_lines", PR_TRUE);

default should be PR_FALSE

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.1

Roc:

Absolutely.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/mozilla-central/rev/a4107cfe6b21

Comment 4

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #3)
> http://hg.mozilla.org/mozilla-central/rev/a4107cfe6b21

    1.50  CViewSourceHTML::CViewSourceHTML()
    1.51  {
    1.52    mSyntaxHighlight = PR_FALSE;
    1.53    mWrapLongLines = PR_FALSE;
    1.54    mTabSize = -1;

There is no need to set initial values anymore.

Just out of curiosity, the default value of mSyntaxHighlight should be PR_FALSE or PR_TRUE? In original code, the value is PR_FALSE if failing to get nsIPrefBranch but the value is PR_TRUE if GetBoolPref is failed.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #4)
> (In reply to comment #3)
> > http://hg.mozilla.org/mozilla-central/rev/a4107cfe6b21
> 
>     1.50  CViewSourceHTML::CViewSourceHTML()
>     1.51  {
>     1.52    mSyntaxHighlight = PR_FALSE;
>     1.53    mWrapLongLines = PR_FALSE;
>     1.54    mTabSize = -1;
> 
> There is no need to set initial values anymore.
> 
> Just out of curiosity, the default value of mSyntaxHighlight should be
> PR_FALSE or PR_TRUE? In original code, the value is PR_FALSE if failing to
> get nsIPrefBranch but the value is PR_TRUE if GetBoolPref is failed.

I guess that it should be PR_TRUE because prefBranch is never NULL until shutting down.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: remove unnecessary code

followup patch for comment 4. the initialized values are always overwritten.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: remove unnecessary code

oops, removed also mTabSize.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 538152 [details] [diff] [review]
remove unnecessary code

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

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/bf50cdd9ba99

Comment 10

[Matt Brubeck (:mbrubeck)]

"remove unnecessary code" merged from m-i to m-c:
http://hg.mozilla.org/mozilla-central/rev/bf50cdd9ba99