if |intl.menuitems.alwaysappendaccesskeys| is true, the accesskey of the menu is always append to the captions. But when an ellipsis exists at non-tail of the caption, the accesskey label is inserted to wrong position if the caption is |Search Google for "..."| and the accesskey is 's': Actual result: |Search Google for "(S)..."| Expected result: |Search Google for "..."(S)| The attached patch is created by Kazuyoshi Kato-san (kzys@8-p.info). Thank you for your work!

Comment on attachment 292311 [details] [diff] [review] Patch I'm not really comfortable reviewing this, and as a result not sure when I would be able to get to it. Perhaps one of the Neils would be a better choice?

Comment on attachment 292311 [details] [diff] [review] Patch > const nsDependentString kEllipsis = nsContentUtils::GetLocalizedEllipsis(); >- PRInt32 offset = mTitle.RFind(kEllipsis); >- if (offset == kNotFound) { >+ PRInt32 offset = kNotFound; >+ if (StringEndsWith(mTitle, kEllipsis)) { >+ offset = mTitle.Length() - kEllipsis.Length(); >+ } else { > // Try to check with our old ellipsis (for old addons) >- if (!kEllipsis.Equals(OLD_ELLIPSIS)) >- offset = mTitle.RFind(OLD_ELLIPSIS); >- if (offset == kNotFound) { >+ if (StringEndsWith(mTitle, OLD_ELLIPSIS)) { >+ offset = mTitle.Length() - OLD_ELLIPSIS.Length(); >+ } else { > // Try to check with our default ellipsis (for non-localized addons) >- nsAutoString defaultEllipsis(PRUnichar(0x2026)); >- if (!kEllipsis.Equals(defaultEllipsis)) >- offset = mTitle.RFind(defaultEllipsis); >+ const nsAutoString kDefaultEllipsis(PRUnichar(0x2026)); >+ if (StringEndsWith(mTitle, kDefaultEllipsis)) { >+ offset = mTitle.Length() - kDefaultEllipsis.Length(); >+ } > } > } >+ > if (offset == kNotFound) { > offset = (PRInt32)mTitle.Length(); > if (mTitle.Last() == PRUnichar(':')) > offset--; > } Just looking at this block shows me that you can improve on the previous test, but in fact you can merge the two blocks because they're both doing the same thing - looking for a trailing substring. Start with offset as the length (and you'll be able to make it a PRUint32 now, because you're not doing a RFind any more), then check each of the four substrings/characters in turn. I'd also suggest using } else if { to cut down on the indentation.

Kato-san's work.

Comment on attachment 292419 [details] [diff] [review] Patch #2 (Sorry if these weren't clear in my previous comment.) >+ PRUint32 offset = mTitle.Length(); >+ offset = mTitle.Length() - You can use -= as you already have the length. >- if (mTitle.Last() == PRUnichar(':')) >- offset--; >+ } else if (StringEndsWith(mTitle, NS_LITERAL_STRING(":"))){ >+ offset = mTitle.Length() - 1; Last() and -- is better than StringEndsWith() for a single character.

Comment on attachment 292730 [details] [diff] [review] Patch #3 Thank you Neil. And let's land this to 1.9. This patch is very low risky.

checked-in, thank you Kato-san!!