The default bug view has changed. See this FAQ.

View > Character Encoding > Auto-Detect menupopup is broken

RESOLVED FIXED in mozilla12

Status

()

Core
Internationalization
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Alice0775 White, Assigned: m_kato)

Tracking

({regression})

12 Branch
mozilla12
x86
Windows 7
regression
Points:
---

Firefox Tracking Flags

(firefox12-)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
Err,
>1. Open Firefox with clean profile and Google Earth Plugin
1. Open Firefox with clean profile
tracking-firefox12: --- → ?
(Assignee)

Comment 2

5 years ago
nsCharsetConverterManager::GetList should add item even if getCharsetAlias with "chardet." prefix is failed.
(Assignee)

Comment 3

5 years ago
Created attachment 589111 [details] [diff] [review]
fix
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?
(Assignee)

Comment 5

5 years ago
(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.
(Assignee)

Updated

5 years ago
Attachment #589111 - Flags: review?(smontagu)
Thanks for taking care of this, Makoto!
(Assignee)

Updated

5 years ago
Assignee: smontagu → m_kato
(Assignee)

Comment 8

5 years ago
Created attachment 589144 [details] [diff] [review]
fix v2
Attachment #589111 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
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)
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/715e4b7e3b51
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/715e4b7e3b51
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla12

Updated

5 years ago
tracking-firefox12: ? → -
You need to log in before you can comment on or make changes to this bug.