Closed
Bug 718500
Opened 12 years ago
Closed 12 years ago
View > Character Encoding > Auto-Detect menupopup is broken
Categories
(Core :: Internationalization, defect)
Tracking
()
RESOLVED
FIXED
mozilla12
Tracking | Status | |
---|---|---|
firefox12 | - | --- |
People
(Reporter: alice0775, Assigned: m_kato)
References
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
7.06 KB,
patch
|
smontagu
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
Err,
>1. Open Firefox with clean profile and Google Earth Plugin
1. Open Firefox with clean profile
Updated•12 years ago
|
tracking-firefox12:
--- → ?
Assignee | ||
Comment 2•12 years ago
|
||
nsCharsetConverterManager::GetList should add item even if getCharsetAlias with "chardet." prefix is failed.
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #589111 -
Flags: review?(smontagu)
Comment 4•12 years ago
|
||
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•12 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.
Comment 6•12 years ago
|
||
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•12 years ago
|
Attachment #589111 -
Flags: review?(smontagu)
Comment 7•12 years ago
|
||
Thanks for taking care of this, Makoto!
Assignee | ||
Updated•12 years ago
|
Assignee: smontagu → m_kato
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #589111 -
Attachment is obsolete: true
Assignee | ||
Comment 9•12 years ago
|
||
Comment on attachment 589144 [details] [diff] [review] fix v2 same result before landing bug 713519.
Attachment #589144 -
Flags: review?(smontagu)
Comment 10•12 years ago
|
||
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•12 years ago
|
||
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+
Comment 12•12 years ago
|
||
(Might be better as NS_ENSURE_TRUE(array->AppendElement(fullName), NS_ERROR_OUT_OF_MEMORY) or a boolean temporary)
Assignee | ||
Comment 13•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/715e4b7e3b51
Whiteboard: [inbound]
Comment 14•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/715e4b7e3b51
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla12
Updated•12 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•