The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla1.8beta3

Status

()

Toolkit
Startup and Profile System
P1
normal
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: bsmedberg, Assigned: bsmedberg)

Tracking

unspecified
mozilla1.8beta3
Points:
---
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: has patch with review, needed for b4 not b3)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

12 years ago
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.
Attachment #188337 - Flags: first-review?(darin)
(Assignee)

Updated

12 years ago
Flags: blocking1.8b4+

Comment 2

12 years ago
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?
>+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.
(Assignee)

Comment 4

12 years ago
> 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

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

Comment 6

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

yeah, I meant to attach a new patch
Attachment #188337 - Attachment is obsolete: true
Attachment #188337 - Flags: first-review?(darin)
(Assignee)

Comment 7

12 years ago
Created attachment 188463 [details] [diff] [review]
Add getLocalesForPackage and parse modifiers more strictly, rev. 1.1
Attachment #188463 - Flags: first-review?(darin)

Comment 8

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

r=darin
Attachment #188463 - Flags: first-review?(darin) → first-review+
(Assignee)

Updated

12 years ago
Priority: -- → P1
Whiteboard: has patch with review, needed for b4 not b3
Target Milestone: --- → mozilla1.8beta3
(Assignee)

Updated

12 years ago
Attachment #188463 - Flags: approval1.8b4?

Updated

12 years ago
Attachment #188463 - Flags: approval1.8b4? → approval1.8b4+
(Assignee)

Comment 9

12 years ago
Fixed on trunk
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

9 years ago
Component: XRE Startup → Startup and Profile System
QA Contact: nobody → startup
You need to log in before you can comment on or make changes to this bug.