Closed Bug 718500 Opened 12 years ago Closed 12 years ago

View > Character Encoding > Auto-Detect menupopup is broken

Categories

(Core :: Internationalization, defect)

12 Branch
x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12
Tracking Status
firefox12 - ---

People

(Reporter: alice0775, Assigned: m_kato)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
Err,
>1. Open Firefox with clean profile and Google Earth Plugin
1. Open Firefox with clean profile
nsCharsetConverterManager::GetList should add item even if getCharsetAlias with "chardet." prefix is failed.
Attached patch fix (obsolete) — Splinter Review
Attachment #589111 - Flags: review?(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?
(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.
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.
Attachment #589111 - Flags: review?(smontagu)
Thanks for taking care of this, Makoto!
Assignee: smontagu → m_kato
Attached patch fix v2Splinter Review
Attachment #589111 - Attachment is obsolete: true
Comment on attachment 589144 [details] [diff] [review]
fix v2

same result before landing bug 713519.
Attachment #589144 - Flags: review?(smontagu)
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 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!)
Attachment #589144 - Flags: review?(smontagu) → review+
(Might be better as NS_ENSURE_TRUE(array->AppendElement(fullName), NS_ERROR_OUT_OF_MEMORY) or a boolean temporary)
https://hg.mozilla.org/mozilla-central/rev/715e4b7e3b51
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.