299730 - Chrome registry needs to be able to enumerate locales (for locale-switcher extension), and cleanup of manifest modifier parsing

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, defect, P1)

Description

[Benjamin Smedberg]

This bug covers two different things:

1) in order to support the locale-switcher extension in 1.1 I need to be able to
enumerate available locales for a particular package (trivial)

2) I've been looking through uses of the .manifest modifiers, and future
potential uses, and I want to change the parsing strategy a little bit

  a) add application={app-id}
  b) add version= version<= version< version> version>=
  c) if we encounter an expected modifier, treat the entire line as invalid

Comment 1

[Benjamin Smedberg]

Attachment: Add getLocalesForPackage and parse modifiers more strictly, rev. 1

I used "appversion" instead of "version" to be more clear, but otherwise it
does what I said.

Comment 2

[Darin Fisher]

Comment on attachment 188337 [details] [diff] [review]
Add getLocalesForPackage and parse modifiers more strictly, rev. 1

>Index: chrome/src/nsChromeRegistry.cpp

> NS_IMETHODIMP
>+nsChromeRegistry::GetLocalesForPackage(const nsACString& aPackage,
>+                                       nsIUTF8StringEnumerator* *aResult)
>+{
>+  nsCStringArray *a = new nsCStringArray;
>+  if (!a)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+
>+  PackageEntry* entry =
>+    NS_STATIC_CAST(PackageEntry*, PL_DHashTableOperate(&mPackagesHash,
>+                                                       & aPackage,
>+                                                       PL_DHASH_LOOKUP));
>+
>+  if (PL_DHASH_ENTRY_IS_BUSY(entry)) {
>+    entry->locales.EnumerateToArray(a);
>+  }
>+
>+  return NS_NewAdoptingUTF8StringEnumerator(aResult, a);
>+}

If NS_NewAdoptingUTF8StringEnumerator is unable to allocate the 
string enumerator object, then the nsCStringArray will be leaked.


>+enum TriState {
>+  unspecified,
>+  bad,
>+  ok
>+};

I think you should namespace these values in some way to avoid
conflicting with system headers or other headers that may be 
pulled in on some platform that you haven't tested or that doesn't
exist yet.

I think it would also be better to use a more descriptive type
name.  TriState isn't very descriptive.



>+static PRBool
>+CheckStringFlag(const nsSubstring& aFlag, const nsSubstring& aData,
>+                const nsSubstring& aValue, TriState& aResult)

This function and CheckVersionFlag are somewhat complex.  I think it
might be simpler if you turned things around so that these functions
simply test the given flag and report a boolean value indicating true
if the flag is found in aData and matches the given value.  Otherwise,
return false.  Then initialize all flags to false, and only call the
Check functions if the flag is false.  At the end, check to see if any
flags are false.  Then you know some flag wasn't satisfied.

It might make sense to make the code ignore 'unrecognized chrome
registration modifiers' so that an older build fails gracefully if
processing input designed to work with a newer build.  This argues
against setting and checking badFlag.

What do you think?  Am I missing some reason why we need to be more
strict in our checking of these flags?

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

>+enum TriState {
>+  unspecified,
>+  bad,
>+  ok
>+};

I think we often use eUnspecified eBad eOK as names for enum values, maybe
that'd also help avoid conflicts here.

Comment 4

[Benjamin Smedberg]

> This function and CheckVersionFlag are somewhat complex.  I think it
> might be simpler if you turned things around so that these functions
> simply test the given flag and report a boolean value indicating true
> if the flag is found in aData and matches the given value.  Otherwise,
> return false.  Then initialize all flags to false, and only call the

This will not work, because there are three cases being tested:
  correct flag and is true
  correct flag and is false
  incorrect flag

I could have all three Check*Flag functions return a tristate and munge it
later, but that is in fact harder to code since I can't use a simple series of
|| tests.


> It might make sense to make the code ignore 'unrecognized chrome
> registration modifiers' so that an older build fails gracefully if
> processing input designed to work with a newer build.  This argues
> against setting and checking badFlag.

Heh, that was the most important part of the patch. In terms of forwards and
backwards compatibility, I think it is safer to ignore lines with unrecognized
modifiers because it will be much easier to construct a manifest that is
multi-version compatible.

Here are some of the modifiers that I am considering for 1.9:

content global-platform <URI> toolkit=gtk2
override <URI> <URI> toolkit=gtk2

If you were going to design a theme that was backwards compatible, you could use
the following syntax:

# provide the three basic subdirs "win/mac/unix" (compatible with 1.1)
content global-platform jar:theme.jar!/global-platform/ platform

# override the "unix" subdir with QT-specific information
content global-platform jar:theme.jar!/global-platform-qt/ toolkit=qt

# There's a browser icon that should be different for QT also
override chrome://browser/skin/icons/something.png
jar:theme.jar!/skin/qt/something.png

Does this explain why I think we should ignore lines with unexpected modifiers
for forward compatibility?

Comment 5

[Darin Fisher]

OK, I misunderstood how badFlag was being used.  I didn't realize that it was
what gives us the backwards and forwards compatibility.  Good work.  So, can you
just fix the potential memory leak and namespace the TriState values (perhaps
with the "e" prefix suggested above)?

Comment 6

[Benjamin Smedberg]

Comment on attachment 188337 [details] [diff] [review]
Add getLocalesForPackage and parse modifiers more strictly, rev. 1

yeah, I meant to attach a new patch

Comment 7

[Benjamin Smedberg]

Attachment: Add getLocalesForPackage and parse modifiers more strictly, rev. 1.1

Comment 8

[Darin Fisher]

Comment on attachment 188463 [details] [diff] [review]
Add getLocalesForPackage and parse modifiers more strictly, rev. 1.1

r=darin

Comment 9

[Benjamin Smedberg]

Fixed on trunk