718500 - View > Character Encoding > Auto-Detect menupopup is broken

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Alice0775 White]

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/047c8ba7d2e4
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120116 Firefox/12.0a1 ID:20120116031100

View > Character Encoding > Auto-Detect menupopup is broken

Reproducible: Always

Steps to Reproduce:
1. Open Firefox with clean profile and Google Earth Plugin
2. Open http://maps.google.com/
3. Alt > View > Character Encoding > Auto-Detect


Actual Results:
  No menu pops up

Expected Results:
  Auto-Detect menu should pop up

Regression window(m-c)
Works:
http://hg.mozilla.org/mozilla-central/rev/964b118ac852
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120113 Firefox/12.0a1 ID:20120113031050
Fails:
http://hg.mozilla.org/mozilla-central/rev/8d4638feec54
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120113 Firefox/12.0a1 ID:20120113005738
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=964b118ac852&tochange=8d4638feec54


Regression window(m-i)
Works:
http://hg.mozilla.org/integration/mozilla-inbound/rev/3c371703da3c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120112 Firefox/12.0a1 ID:20120112080524
Fails:
http://hg.mozilla.org/integration/mozilla-inbound/rev/d7abb1f2efc8
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120112 Firefox/12.0a1 ID:20120112084225
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3c371703da3c&tochange=d7abb1f2efc8

Suspected:
d76bf407d5c1	Asaf Romano — Bug 713519 - getCharsetAlias should throw for unknown character sets. r=smontagu.

Comment 1

[Alice0775 White]

Err,
>1. Open Firefox with clean profile and Google Earth Plugin
1. Open Firefox with clean profile

Comment 2

[Makoto Kato [:m_kato]]

nsCharsetConverterManager::GetList should add item even if getCharsetAlias with "chardet." prefix is failed.

Comment 3

[Makoto Kato [:m_kato]]

Attachment: fix

Comment 4

[Simon Montagu :smontagu]

Comment on attachment 589111 [details] [diff] [review]
fix

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

::: intl/uconv/src/nsCharsetConverterManager.cpp
@@ -268,5 @@
>      if (NS_FAILED(supStr->GetData(name)))
>        continue;
>  
>      fullName += name;
> -    rv = GetCharsetAlias(fullName.get(), alias);

I wonder do we need the GetCharsetAlias call even in the non-autodetection case? Isn't fullName always going to be a canonical name, since it's returned from the registered encoder/decoder names? Or am I missing something?

Comment 5

[Makoto Kato [:m_kato]]

(In reply to Simon Montagu from comment #4)
> Comment on attachment 589111 [details] [diff] [review]
> fix
> 
> Review of attachment 589111 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: intl/uconv/src/nsCharsetConverterManager.cpp
> @@ -268,5 @@
> >      if (NS_FAILED(supStr->GetData(name)))
> >        continue;
> >  
> >      fullName += name;
> > -    rv = GetCharsetAlias(fullName.get(), alias);
> 
> I wonder do we need the GetCharsetAlias call even in the non-autodetection
> case? Isn't fullName always going to be a canonical name, since it's
> returned from the registered encoder/decoder names? Or am I missing
> something?

By bug 206379 (replacing nsIAtom), GetList seems to call GetCharsetAlias.

When I debug again, tt is sure that it is unnecessary to use GetCharsetAlias...
Should we remove GetCharsetAlias calling on GetList?  If fullname is alias, we should fix charset list into category.

Comment 6

[Simon Montagu :smontagu]

Yes, I think we should remove the call to GetCharsetAlias. If you want to be extra cautious, you could add a test that all the names returned from GetDecoderList/GetEncoderList are not changed by calling GetCharsetAlias on them.

Comment 7

[Asaf Romano (gone)]

Thanks for taking care of this, Makoto!

Comment 8

[Makoto Kato [:m_kato]]

Attachment: fix v2

Comment 9

[Makoto Kato [:m_kato]]

Comment on attachment 589144 [details] [diff] [review]
fix v2

same result before landing bug 713519.

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 589144 [details] [diff] [review]
fix v2

>@@ -257,28 +256,23 @@ nsCharsetConverterManager::GetList(const
>+    nsCAutoString fullName(aPrefix);
>+    fullName.Append(name);
>+    rv = array->AppendElement(fullName) ? NS_OK : NS_ERROR_OUT_OF_MEMORY;

rv seems unused here.

Comment 11

[Simon Montagu :smontagu]

Comment on attachment 589144 [details] [diff] [review]
fix v2

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

::: intl/uconv/src/nsCharsetConverterManager.cpp
@@ +266,5 @@
>        continue;
>  
> +    nsCAutoString fullName(aPrefix);
> +    fullName.Append(name);
> +    rv = array->AppendElement(fullName) ? NS_OK : NS_ERROR_OUT_OF_MEMORY;

Add |NS_ENSURE_SUCCESS(rv, rv)| here and r=me (thanks, Ms2ger!)

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(Might be better as NS_ENSURE_TRUE(array->AppendElement(fullName), NS_ERROR_OUT_OF_MEMORY) or a boolean temporary)

Comment 13

[Makoto Kato [:m_kato]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/715e4b7e3b51

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/715e4b7e3b51