Last Comment Bug 299730 - Chrome registry needs to be able to enumerate locales (for locale-switcher extension), and cleanup of manifest modifier parsing
: Chrome registry needs to be able to enumerate locales (for locale-switcher ex...
Status: RESOLVED FIXED
has patch with review, needed for b4 ...
:
Product: Toolkit
Classification: Components
Component: Startup and Profile System (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: mozilla1.8beta3
Assigned To: Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-07-05 09:10 PDT by Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg]
Modified: 2008-07-31 03:02 PDT (History)
4 users (show)
benjamin: blocking1.8b5+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add getLocalesForPackage and parse modifiers more strictly, rev. 1 (26.60 KB, patch)
2005-07-05 12:33 PDT, Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg]
no flags Details | Diff | Splinter Review
Add getLocalesForPackage and parse modifiers more strictly, rev. 1.1 (26.69 KB, patch)
2005-07-06 13:50 PDT, Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg]
darin.moz: first‑review+
asa: approval1.8b4+
Details | Diff | Splinter Review

Description Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2005-07-05 09:10:11 PDT
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 AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2005-07-05 12:33:47 PDT
Created attachment 188337 [details] [diff] [review]
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 2005-07-05 14:57:58 PDT
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) 2005-07-05 15:47:50 PDT
>+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 AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2005-07-06 06:59:36 PDT
> 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 2005-07-06 10:26:58 PDT
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 AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2005-07-06 10:28:48 PDT
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 AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2005-07-06 13:50:58 PDT
Created attachment 188463 [details] [diff] [review]
Add getLocalesForPackage and parse modifiers more strictly, rev. 1.1
Comment 8 Darin Fisher 2005-07-06 17:42:34 PDT
Comment on attachment 188463 [details] [diff] [review]
Add getLocalesForPackage and parse modifiers more strictly, rev. 1.1

r=darin
Comment 9 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2005-07-14 08:41:17 PDT
Fixed on trunk

Note You need to log in before you can comment on or make changes to this bug.