484616 - Unfork contentAreaUtils.js between FF and SM

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Serge Gautherie (:sgautherie)]

From some bug 471962 comments:
{
Boris Zbarsky: "I'm somewhat sad that that's still forked.  :("

Justin Wood: "I agree though that contentAreaUtils.js still being forked is painful though."
}

(No idea on the amount of work needed.)

Comment 1

[:Paolo Amadini]

From some bug 483959 comments:
{

Paolo Amadini:

The strange thing is that the mozilla-1.9.1 test suite, located in
"toolkit/content/tests/browser", when run from a comm-central build is
NOT testing the code in "toolkit/content/"!

Robert Kaiser:

This is because it's testing whatever is _loaded_ into the browser window, and
SeaMonkey is loading the suite/ version (right now) in its browser window while
Firefox is loading the toolkit version. The reason might be that SeaMonkey
might want to do some things differently than Firefox, but I think we probably
should load the toolkit version and only change/override those things we want
to have differently in SeaMonkey, if possible - but that's bug 484616 stuff and
needs the input of Neil Rashbrook, who probably knows all this code best.

Note that Thunderbird is not running those tests at all, as anything that
requires a browser window cannot be tested well from a non-browser application
and all mochitest-based test suites fall under that category.

}

Comment 2

[[e-mail]]

Well I think the most obvious reason that it's still forked is because SeaMonkey isn't quite ready to switch to the Toolkit download manager yet.

Comment 3

[:Paolo Amadini]

(In reply to comment #2)
> Well I think the most obvious reason that it's still forked is because
> SeaMonkey isn't quite ready to switch to the Toolkit download manager yet.

Isn't the download manager implementation abstracted through the nsITransfer
interface? This code is identical in both versions of contentAreaUtils.js:

var tr = Components.classes["@mozilla.org/transfer;1"]
         .createInstance(Components.interfaces.nsITransfer);

Comment 4

[Robert Kaiser]

(In reply to comment #2)
> Well I think the most obvious reason that it's still forked is because
> SeaMonkey isn't quite ready to switch to the Toolkit download manager yet.

In that case, setting dependency on that bug ;-)

Comment 5

[:Paolo Amadini]

(In reply to comment #4)
> (In reply to comment #2)
> > Well I think the most obvious reason that it's still forked is because
> > SeaMonkey isn't quite ready to switch to the Toolkit download manager yet.
> 
> In that case, setting dependency on that bug ;-)

This is good for informative purposes, however let's keep in mind that if that
bug is the only reason for having the code forked, we might as well unfork it
now, since the DM implementation is decoupled from the saving code, if what I
said in comment #3 is right.

Actually, it seems that the portion of Justin's patch on bug 381157 dealing
with "contentAreaUtils.js" could be separated and addressed as part of this
bug; splitting the patch in smaller blocks may also help with testing and
review.

Alternatively, we may try to merge the two "contentAreaUtils.js" files as
they are, while trying to keep the existing differences, as you suggested
in your previous comment.

Comment 6

[:Paolo Amadini]

Noting related bug 188253, probably should be set as a dependency too.

Comment 7

[:Paolo Amadini]

Attachment: Difference between contentAreaUtils.js in "toolkit" and in "suite"

This is the current difference between the two "contentAreaUtils.js" files,
the one in the "toolkit" folder in mozilla191 and the one in the "suite"
folder in comm-central.

Most of the functions are simply identical. Other functions are present in one
file and not the other, but are fundamentally independent.

From a brief scan, it seems that the differences in the other functions are
mostly refactorings or bugfixes that have not been backported, and do not
affect functionality.

The script reads some strings from a string bundle, whose chrome URL is
different between the two versions. The strings that are read from the
bundles are however identical so those can be unified too.

As a conclusion, it seems that unifying the two would not be very hard.
Referencing the toolkit version from the suite's browser XUL file, in
addition to another file containing the suite-specific functions,
would probably suffice.

Comment 8

[[e-mail]]

Comment on attachment 383097 [details] [diff] [review]
Difference between contentAreaUtils.js in "toolkit" and in "suite"

> function saveURL(aURL, aFileName, aFilePickerTitleKey, aShouldBypassCache,
>-                 aSkipPrompt, aReferrer)
>+                 aReferrer)
aSkipPrompt doesn't mean what it says :-( It's actually aNotForcingPrompt or aUsersPreferenceToSkipPromptNotIgnored or aHonourUsersPreferenceToSkipPrompt.

However SeaMonkey expects to honour the user's preference, which means that all our callers would have to be adjusted too...

>+function getTargetFile(aFpP)
We have different code here because we didn't feel the need to rewrite it just because we were switching download managers. It's mostly the same code as we've always had but just updated to work around toolkit's confusing preferences.

Comment 9

[:Paolo Amadini]

(In reply to comment #8)
> (From update of attachment 383097 [details] [diff] [review])
> >+function getTargetFile(aFpP)
> We have different code here because we didn't feel the need to rewrite it
> just because we were switching download managers. It's mostly the same
> code as we've always had but just updated to work around toolkit's
> confusing preferences.

I cross-checked, and concluded that getTargetFile is functionally equivalent
in the two files. Since this version seems clearer, I filed bug 498695 to
align the toolkit version.

> > function saveURL(aURL, aFileName, aFilePickerTitleKey, aShouldBypassCache,
> >-                 aSkipPrompt, aReferrer)
> >+                 aReferrer)
> aSkipPrompt doesn't mean what it says :-( It's actually aNotForcingPrompt or
> aUsersPreferenceToSkipPromptNotIgnored or aHonourUsersPreferenceToSkipPrompt.

My interpretation:

 * @param aSkipPrompt
 *        If false, don't save the file automatically to the user's
 *        default download directory, even if the associated preference
 *        is set, but ask for the target explicitly.

As you explained, unfortunately the default behavior is overridden if the
variable is "false", while "true" is the default...

> However SeaMonkey expects to honour the user's preference, which means that
> all our callers would have to be adjusted too...

This seems the most sensible solution to me, for all the affected functions.
The alternative would be to treat undefined and false differently, but that
is error-prone and I don't recommend it.

Comment 10

[Robert Kaiser]

(In reply to comment #8)
> aSkipPrompt doesn't mean what it says :-( It's actually aNotForcingPrompt or
> aUsersPreferenceToSkipPromptNotIgnored or aHonourUsersPreferenceToSkipPrompt.

<bikeshed>or aAvoidPrompt or...</bikeshed>

if we really want to, we can rename it in toolkit, it doesn't really change anything for the callers what we name it in the definition and function.
The important point in this bug is that we end up with ideally calling the same global contentAereaUtils in all Mozilla products and have every extension be able to rely on those functions with the same names having the same API and work the same.

Comment 11

[:Paolo Amadini]

Now that bug 498695 is eventually fixed in Toolkit (it was one of the
major differences between the two "contentAreaUtils.js" codes) I'm having
a look at the other interesting things that remain to be done before
unforking and have not been examined yet.

The current version of "suite/common/contentAreaUtils.js" has a dependency on
"suite/common/utilityOverlay.js", because it uses these two functions:

* "validateFileName" 
   http://mxr.mozilla.org/comm-central/search?string=validateFileName
* "GenerateValidFilename"
   http://mxr.mozilla.org/comm-central/search?string=GenerateValidFilename

The only use of the latter function can be converted easily to a call
to "validateFileName". At this point, to align the file with Toolkit's
current version (goal of this bug) and remove the dependency, we only
have to introduce a copy of the "validateFileName" function in
"suite/common/contentAreaUtils.js". Note that there's no choice here: the
function already exists in Toolkit's version and must reappear to allow the
unforking. Fortunately, the two functions are identical.

The result of this operation would be for the "validateFileName" function
to exist with the same exact definition in two scripts that are included in
the same window. This condition already exists elsewhere, in files like
"mail/base/content/messenger.xul" that include both
"suite/common/utilityOverlay.js" and the original
"mozilla/toolkit/content/contentAreaUtils.js".

I think the above should not block the unforking (we'll be introducing just
one more place where utilityOverlay.js and Toolkit's contentAreaUtils.js are
included together, there are already others). Anyway, I'll file a separate
bug to have this fixed in all places.

Comment 12

[:Paolo Amadini]

Here I am with more background information.

Here are the places that include the SeaMonkey version,
"chrome://communicator/content/contentAreaUtils.js":

http://mxr.mozilla.org/comm-central/search?string=chrome%3A%2F%2Fcommunicator%2Fcontent%2FcontentAreaUtils.js

/mozilla/extensions/venkman/resources/content/venkman-scripts.xul line 58
/mozilla/extensions/irc/xul/content/sm/overlay.xul line 6
/mozilla/extensions/venkman-cvs/resources/content/venkman-scripts.xul line 57
/suite/debugQA/content/debugQATextEditorShell.xul line 81 (false positive)
/suite/browser/pageinfo/pageInfo.xul line 63
/suite/browser/viewSourceOverlay.xul line 63
/suite/common/tests/unit/test_contentAreaUtils.js line 45 (test for a function)
/suite/common/contentAreaContextOverlay.xul line 53 (see below)

It seems that every one of these places also includes "utilityOverlay.js",
either directly or through "utilityOverlay.xul" / "navigatorOverlay.xul".
This is true also for the files that include "contentAreaContextOverlay.xul":

http://mxr.mozilla.org/comm-central/search?string=contentAreaContextOverlay.xul

/editor/ui/composer/content/editor.xul line 55
/suite/debugQA/content/debugQATextEditorShell.xul line 48
/suite/mailnews/addrbook/addressbook.xul line 44
/suite/mailnews/mailWindowOverlay.xul line 46 (see below)
/suite/mailnews/compose/messengercompose.xul line 50
/suite/browser/navigator.xul line 47

"mailWindowOverlay.xul", in turn, is included in these files:

http://mxr.mozilla.org/comm-central/search?string=mailWindowOverlay.xul

/mail/base/content/hiddenWindow.xul line 45
/mail/base/content/messenger.xul line 46 (sse below)
/mail/base/content/messageWindow.xul line 45 (sse below)
/suite/mailnews/messenger.xul line 44
/suite/mailnews/messageWindow.xul line 43

All these files also include "utilityOverlay.xul", and that's fine.
The strange thing is that "messenger.xul" and "messageWindow.xul"
also include "chrome://global/content/contentAreaUtils.js".

This means that these two files files are actually indirectly including
(through the chain you see above) *both* versions of "contentAreaUtils.js"!
I think this is possible and would cause no warnings, and went unnoticed
because the SeaMonkey functions with the same name are overriding the
Toolkit ones.

Comment 13

[:Paolo Amadini]

There are some functions that are defined only in the SeaMonkey version
of "contentAreaUtils.js":

openAsExternal
openNewWindowWith
openNewTabWith
openNewTabWindowOrExistingWith
FillInHTMLTooltip

They should be moved elsewhere to allow the unforking. Is any one of
them a candidate for being moved to "utilityOverlay.js"?

Comment 14

[:Paolo Amadini]

Some functions have some minor differences in comments between the two
versions, because they were implemented and reviewed separately (for
example, "urlSecurityCheck" in bug 342485 and bug 421024).

I'll post patches to adjust the comments for both the SeaMonkey version
and the Toolkit version, in order to have a cleaner diff to look at when
comparing the other differences.

Comment 15

[:Paolo Amadini]

Neil, do you have some time to let me know your opinion on comment 11,
comment 12 and comment 13?

I've seen some activity and interest in this area recently, both in
SeaMonkey and Firefox / Toolkit, and I have a few patches to contribute,
but having to do the work twice and the inability to write reliable
automated tests until the code is unforked is really blocking me.

The recent bug 516189 is an example of an issue caused by the tested code
being different between Firefox and SeaMonkey.

I hope you'll also be able to review the series of patches that will
be required to close this bug. I'm eager to get this done, and I already
worked out the steps I think are necessary to do it, but this work always
risks to get stalled waiting for code reviews and comment replies. If
you're busy, I'd appreciate if you can point me to someone who could work
with me to allow my work to get through the review process promptly.

Comment 16

[Robert Kaiser]

Note thjat SeaMonkey is on the way right now to really wrap up the very large 2.0 release that has been in the making for 3 years or so now, and that release is due in October and based on 1.9.1 - once we have really branched comm-central for it and also have the release close enough to be pushed out, people from our team, including Neil, will have more room for looking into things like this.

Comment 17

[:Paolo Amadini]

(In reply to comment #16)
> once we have really branched comm-central for it and also have the release
> close enough to be pushed out, people from our team, including Neil,
> will have more room for looking into things like this.

Thanks for the answer Robert, since there's a release coming soon, I think
that it's better to postpone working on this bug, also considering its nature.
In addition, I'd rather wait for the appropriate time to begin and end the
work rather than spreading it across several months.

Comment 18

[[e-mail]]

(In reply to comment #11)
> * "GenerateValidFilename"
>    http://mxr.mozilla.org/comm-central/search?string=GenerateValidFilename
> 
> The only use of the latter function can be converted easily to a call
> to "validateFileName".
I see three uses...

> At this point, to align the file with Toolkit's
> current version (goal of this bug) and remove the dependency, we only
> have to introduce a copy of the "validateFileName" function in
> "suite/common/contentAreaUtils.js".
Why not just move it?

(In reply to comment #12)
> The strange thing is that "messenger.xul" and "messageWindow.xul"
> also include "chrome://global/content/contentAreaUtils.js".
Only the mail/ version, used by Thunderbird.

(In reply to comment #13)
> There are some functions that are defined only in the SeaMonkey version
> of "contentAreaUtils.js":
> 
> openAsExternal
> openNewWindowWith
> openNewTabWith
> openNewTabWindowOrExistingWith
> FillInHTMLTooltip
> 
> They should be moved elsewhere to allow the unforking. Is any one of
> them a candidate for being moved to "utilityOverlay.js"?
Maybe contentAreaClick would be a better home for the open* functions?

Comment 19

[:Paolo Amadini]

(In reply to comment #18)
> (In reply to comment #11)
> > * "GenerateValidFilename"
> >    http://mxr.mozilla.org/comm-central/search?string=GenerateValidFilename
> > 
> > The only use of the latter function can be converted easily to a call
> > to "validateFileName".
> I see three uses...

Strange, in "/suite/common/contentAreaUtils.js" I see only one use (line 878).

> > At this point, to align the file with Toolkit's
> > current version (goal of this bug) and remove the dependency, we only
> > have to introduce a copy of the "validateFileName" function in
> > "suite/common/contentAreaUtils.js".
> Why not just move it?

Good point. I've determined that "suite/common/contentAreaUtils.js" has a
dependency on "suite/common/utilityOverlay.js" and the latter is always
included when the former is present, but I'm not sure the converse is true.

If we move the function, we reverse the dependency. Do you think it may
be appropriate?

> (In reply to comment #12)
> > The strange thing is that "messenger.xul" and "messageWindow.xul"
> > also include "chrome://global/content/contentAreaUtils.js".
> Only the mail/ version, used by Thunderbird.

Oh, I see, these are different files with the same chrome URL. I'm glad the
condition I noticed doesn't exist.

> (In reply to comment #13)
> > There are some functions that are defined only in the SeaMonkey version
> > of "contentAreaUtils.js":
> > 
> > openAsExternal
> > openNewWindowWith
> > openNewTabWith
> > openNewTabWindowOrExistingWith
> > FillInHTMLTooltip
> > 
> > They should be moved elsewhere to allow the unforking. Is any one of
> > them a candidate for being moved to "utilityOverlay.js"?
> Maybe contentAreaClick would be a better home for the open* functions?

Yes. Most of the places that include "contentAreaUtils.js" also include
"contentAreaClick.js". I still have to check if the remaining places use
the above functions or not. If so, we may just just add the inclusion
of "contentAreaClick.js" to those places.

Thank you for taking the time to look at the comments. Please let me know
when you think it could be a good time for me to start working on this and
get reviews for getting the refactoring in the tree.

Comment 20

[:Paolo Amadini]

Neil, do you think you can find some time to review this refactoring this
week if I find some time to work on it?

Comment 21

[[e-mail]]

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #11)
> > > * "GenerateValidFilename"
> > >    http://mxr.mozilla.org/comm-central/search?string=GenerateValidFilename
> > > The only use of the latter function can be converted easily to a call
> > > to "validateFileName".
> > I see three uses...
> Strange, in "/suite/common/contentAreaUtils.js" I see only one use (line 878).
There's one in suite/mailnews/mailCommands.js and one in editor/ui/composer/content/ComposerCommands.js too.

> > > At this point, to align the file with Toolkit's
> > > current version (goal of this bug) and remove the dependency, we only
> > > have to introduce a copy of the "validateFileName" function in
> > > "suite/common/contentAreaUtils.js".
> > Why not just move it?
> Good point. I've determined that "suite/common/contentAreaUtils.js" has a
> dependency on "suite/common/utilityOverlay.js" and the latter is always
> included when the former is present, but I'm not sure the converse is true.
I notice that the only call to validateFileName in utilityOverlay.js is by GenerateValidFilename, so once its callers are converted then there is no dependency.

> Thank you for taking the time to look at the comments. Please let me know
> when you think it could be a good time for me to start working on this and
> get reviews for getting the refactoring in the tree.
Well 2.0.x is out and although there are still a few bugs left to squish we can probably start thinking about getting refactoring done while we are still in the alpha stage of 2.1; as for reviews, well I'll do my best although my review queue annoyingly seems to trend to get longer rather than shorter...

Comment 22

[:Paolo Amadini]

(In reply to comment #21)
> Well 2.0.x is out and although there are still a few bugs left to squish we can
> probably start thinking about getting refactoring done while we are still in
> the alpha stage of 2.1;

Good!

> as for reviews, well I'll do my best although my review
> queue annoyingly seems to trend to get longer rather than shorter...

I appreciate that you're willing to help, thanks. But I'm not sure if
starting to work on this bug without a commitment from a reviewer to
work with me in a definite time frame is the best way to handle it.

In fact, this bug has some peculiarities for which the ordinary best-effort
multi-month review process might be particularly inefficient. This bug:
 - Is regarded as tightly linked to the release cycles.
 - Is the only bug on comm-central on which I currently plan to work.
 - Does not introduce new code, thus it should be quick to review.
 - May be composed by more than one patch.
 - May have patches that quickly become outdated.

I think we should decide a week in advance, and work together to get the
bug in the tree during that week. If my estimates are correct, this bug
requires from 30 minutes to 3 hours of review time spread across two or
three cycles. I don't expect to spend more than three hours (excluding
build and test time) to finish it, given that we've already worked out
most details, so it would seem strange if review took more.

This mode of operation greatly benefits me in terms of re-build and re-test
time, but only marginally affects the review time. If you can't afford to
work in this way, and no other reviewer does, then it's not really a
problem: I'll just renounce to the potential benefits of this mode of
operation, attach the first small patch and wait :-) After all, the good
thing about this kind of voluntary development is that, after having waited
for nine months, I may as well wait for another nine months or more - there's
no obligation of any kind for developers or reviewers, and eventually
someone else will fix the bugs I hoped to work on after this, if they're
important enough.

Please let me know what you think is the best way to proceed.

Comment 23

[[e-mail]]

Well if you want to aim for a particular week then my guess would be the last week in the year is likely to be my most productive in terms of reviews.

Comment 24

[:Paolo Amadini]

(In reply to comment #23)
> Well if you want to aim for a particular week then my guess would be the last
> week in the year is likely to be my most productive in terms of reviews.

Nice :-) If it's OK for you, I'll build comm-central and check back on Sun,
27 Dec 2009, so that we can work during the following week. Thanks :-)

Comment 25

[:Paolo Amadini]

> (In reply to comment #23)
> > Well if you want to aim for a particular week then my guess would be
> > the last
> > week in the year is likely to be my most productive in terms of reviews.
> 
> Nice :-) If it's OK for you, I'll build comm-central and check back on Sun,
> 27 Dec 2009, so that we can work during the following week. Thanks :-)

comm-central built :-) We're ready to start!

The first task I'd like to do is to move these functions to a new location:
  openAsExternal
  openNewWindowWith
  openNewTabWith
  openNewTabWindowOrExistingWith

Suggested locations were "utilityOverlay.js" or "contentAreaClick.js". To
understand which might be the best location, I'm trying to understand which
files in "suite" are derived from which files in "browser".

This is my current understanding:

  ->  /mozilla/browser/base/content/browser.js
  <-  /suite/common/contentAreaClick.js

  ->  /mozilla/browser/base/content/utilityOverlay.js
  <-  /suite/common/utilityOverlay.js

Since a function named "openNewWindowWith" currently exists in
"/mozilla/browser/base/content/utilityOverlay.js", even if it has
a different signature, I guess the best new location for the "open*"
functions is "utilityOverlay.js", to preserve the parallelism.

This change also doesn't require any caller to be modified.

Should I go on and write the patch?

Comment 26

[[e-mail]]

(In reply to comment #25)
> The first task I'd like to do is to move these functions to a new location:
>   openAsExternal
>   openNewWindowWith
>   openNewTabWith
>   openNewTabWindowOrExistingWith
> 
> Suggested locations were "utilityOverlay.js" or "contentAreaClick.js". To
> understand which might be the best location, I'm trying to understand which
> files in "suite" are derived from which files in "browser".
It's the other way around. Back when it was called xpfe, openNewTabWith was created in contentAreaUtils.js. Firefox originally copied it to its contentAreaUtils.js but then Firefox 3 incidentally moved it to its utilityOverlay.js as part of bug 342485, so it makes sense to follow them.

Comment 27

[:Paolo Amadini]

Attachment: [Step 1] Move the open* functions to utilityOverlay.js [Checkin: Comment 31]

Here is the first patch. I placed the new functions more or less where their
counterparts in "browser" are located. Since I also moved the two global
constants kExistingWindow and kNewWindow to the new file, I removed their
redefinition in one of the functions in the new file, even though the
redefinition didn't hurt (the two files are often loaded in the same context).

Comment 28

[:Paolo Amadini]

Attachment: Reintroduce the validateFileName function

This is one of the possible approaches for reintroducing the validateFileName
function. The advantage is that we keep the GenerateValidFileName function,
that other applications also have in their own "utilityOverlay.js", thus we
preserve the parallelism and don't need to modify all the callers, at the
cost of introducing some minor code duplication.

I think that the long-term approach to avoid this kind of duplication for
the utility functions may be to write a new JavaScript Code Module located
in Toolkit, using the same approach as the other code modules:
https://developer.mozilla.org/en/JavaScript_code_modules#Standard_code_modules

Comment 29

[:Paolo Amadini]

CHECK-IN LOGISTICS:

Since Neil's connection is not particularly stable today and thus we can't
commit ourselves, we came up with this solution: this bug will have a series
of attachments marked with [Step 1], [Step 2], and so on. Every attachment
applies to the tree as it is after the previous attachment has been applied.

So, the attachments that need check-in are those marked with [Step #], but
since this is a work in progress, more attachments will be added to the bug
later.

To achieve this, I'm doing local commits after every approved attachment,
so in theory we could rebase and push them, but the magic with hg bundles
and distributed version control will be for the next time :-)

Comment 30

[[e-mail]]

Comment on attachment 419313 [details] [diff] [review]
Reintroduce the validateFileName function

>+// This function is identical to "validateFileName" defined in
>+// "/mozilla/toolkit/content/contentAreaUtils.js". We don't use the original
>+// function because otherwise all the users of "utilityOverlay.xul" would also
>+// have to include either "contentAreaContextOverlay.xul" or directly Mozilla
>+// Toolkit's "contentAreaUtils.js".
Well, I can only see the following users:
ComposerCommands.js - used by editor, which does include contentAreaUtils.js somehow, although I'm not sure how yet.
mailCommands.js - used by mail, which certainly needs contentAreUtils.js for functions such as FillInHTMLTooltip
contentAreaUtils.js - not a problem, obviously!

Comment 31

[Serge Gautherie (:sgautherie)]

Comment on attachment 419294 [details] [diff] [review]
[Step 1] Move the open* functions to utilityOverlay.js
[Checkin: Comment 31]


http://hg.mozilla.org/comm-central/rev/d274c28e9d26

Comment 32

[:Paolo Amadini]

Attachment: Reintroduce the validateFileName function

Comment 33

[:Paolo Amadini]

Comment on attachment 419399 [details] [diff] [review]
Reintroduce the validateFileName function

Hit Enter instead of Del :-( See the following comment...

Comment 34

[:Paolo Amadini]

This is an alternate approach for reintroducing the validateFileName
function. This is the reverse inclusion graph for the two files that
contain calls to "GenerateValidFileName":

  chrome://editor/content/ComposerCommands.js
    chrome://debugqa/content/debugQATextEditorShell.xul
    chrome://editor/content/editorOverlay.xul
      chrome://editor/content/editor.xul
      chrome://debugqa/content/debugQATextEditorShell.xul
      chrome://messenger/content/messengercompose/messengercompose.xul
  chrome://messenger/content/mailCommands.js
    chrome://messenger/content/SearchDialog.xul
    chrome://messenger/content/msgSelectOffline.xul
    chrome://messenger/content/virtualFolderProperties.xul
    chrome://messenger/content/FilterEditor.xul
    chrome://messenger/content/mailWindowOverlay.xul
      chrome://messenger/content/messenger.xul
      chrome://messenger/content/messageWindow.xul

Even if there are many places where "GenerateValidFileName" might potentially
be called, it seems that the only top-level windows that contain code paths
that lead to that function are "mailWindowOverlay.xul" and "editor.xul",
and both already include "contentAreaContextOverlay.xul".

Comment 35

[:Paolo Amadini]

We should either port bug 468881 (remove Save Frame As option from File menu),
or move the "saveFrameDocument" function to another place, for example
"browser.js". Which option would you recommend?

Comment 36

[[e-mail]]

Comment on attachment 419399 [details] [diff] [review]
Reintroduce the validateFileName function

>+// If you use this function, you must include "contentAreaContextOverlay.xul" or
>+// Mozilla Toolkit's "contentAreaUtils.js" in the same scope as this file.
I wouldn't bother with this comment, instead at some point I'd just fix the calls to GenerateValidFilename to use validateFileName diretly.

Comment 37

[[e-mail]]

(In reply to comment #35)
> We should either port bug 468881 (remove Save Frame As option from File menu),
> or move the "saveFrameDocument" function to another place, for example
> "browser.js". Which option would you recommend?
I'd say navigator.js would be a better place.

Comment 38

[:Paolo Amadini]

Attachment: [Step 2] Reintroduce the validateFileName function [Checkin: Comment 63]

(In reply to comment #36)
> I wouldn't bother with this comment, instead at some point I'd just fix the
> calls to GenerateValidFilename to use validateFileName diretly.

I replaced the comment to indicate this intention.

Comment 39

[:Paolo Amadini]

Attachment: [Step 3] Move saveFrameDocument to navigator.js [Checkin: Comment 68]

Comment 40

[:Paolo Amadini]

Attachment: [Step 4] Move getContentFrameURI near its only user, and remove getContentFrameDocument [Checkin: Comment 74]

Comment 41

[:Paolo Amadini]

Attachment: Move FillInHTMLTooltip to utilityOverlay.js

"FillInHTMLTooltip" is used by "messenger.xul", "messageWindow.xul" and
"navigator.xul", so I thought I'd move it to "utilityOverlay.js". Let me
know if there's a better place.

Comment 42

[[e-mail]]

Comment on attachment 419550 [details] [diff] [review]
[Step 4] Move getContentFrameURI near its only user, and remove getContentFrameDocument
[Checkin: Comment 74]

[I wonder whether it is worth keeping it as a separate function]

Comment 43

[:Paolo Amadini]

Attachment: [Step 5] Temporarily avoid the "SaveAttachmentTitle" file picker title key [Checkin Comment 75]

This file picker title key can be used with SeaMonkey's version of
contentAreaUtils.js, but not with Toolkit's version (bug 537231).

Comment 44

[[e-mail]]

Comment on attachment 419551 [details] [diff] [review]
Move FillInHTMLTooltip to utilityOverlay.js

>+ * NOTE: Any changes to this routine need to be mirrored in ChromeListener::FindTitleText()
>+ *       (located in mozilla/embedding/browser/webBrowser/nsDocShellTreeOwner.cpp)
At least, up until 2001-07-13, when it was renamed by bug 90195 to DefaultTooltipTextProvider::GetNodeText().

>+ *       which performs the same function, but for embedded clients that
>+ *       don't use a XUL/JS layer. It is important that the logic of
>+ *       these two routines be kept more or less in sync.
Interestingly they are not quite in sync! When both attributes are present, the C++ code prefers the non-namespaced version.

>+      direction = defView.getComputedStyle(tipElement, "")
>+        .getPropertyValue("direction");
I could comment on how the indentation is wrong,

>+    if (t && t.search(/\S/) >= 0) {
Or on the fact that test is better than search.

But this function badly needs a rewrite anyway.

Comment 45

[[e-mail]]

And in fact I have such a rewrite in bug 264001; time to resurrect it!

Comment 46

[[e-mail]]

And bug 234638 comment #2 had the right idea all along ;-)

Comment 47

[:Paolo Amadini]

Attachment: Align the signature of saveDocument, saveURL and saveImageURL

Note that this patch does not change the implementation, just the signature.

Comment 48

[:Paolo Amadini]

Attachment: Latest difference between contentAreaUtils.js in "toolkit" and in "suite"

After all the previous patches are applied, the remaining differences between
the two files should be only implementation details. The DownloadLastDir.jsm
module is also available in SeaMonkey, but sticks to the old behavior if the
private browsing component is not present.

Comment 49

[:Paolo Amadini]

Attachment: Finally!

The "Save As" dialog works correctly with this patch! I'll test the other
major uses of "contentAreaUtils.js" before marking this for checkin.

Comment 50

[Philip Chee]

Comment on attachment 419568 [details] [diff] [review]
Latest difference between contentAreaUtils.js in "toolkit" and in "suite"

Hmm. I think using diff -w here might reduce the noise.

Comment 51

[:Paolo Amadini]

(In reply to comment #50)
> (From update of attachment 419568 [details] [diff] [review])
> Hmm. I think using diff -w here might reduce the noise.

Only marginally, as most differences are slight text changes more than
whitespace modifications. The rest of the diff is confusing, in any case;
I think that to better understand the changes one should actually look at
the various patches that have been applied to contentAreaUtils.js in
Toolkit and have not been ported to SeaMonkey:

http://hg.mozilla.org/mozilla-central/log/7845a4bd5274/toolkit/content/contentAreaUtils.js

Comment 52

[[e-mail]]

Comment on attachment 419570 [details] [diff] [review]
Finally!

Don't nsContextMenu.js and imageContextOverlay.xul still need contentAreaCommandsBundle?

Comment 53

[:Paolo Amadini]

(In reply to comment #52)
> (From update of attachment 419570 [details] [diff] [review])
> Don't nsContextMenu.js and imageContextOverlay.xul still need
> contentAreaCommandsBundle?

Yes, the bundle is still defined in contentAreaContextOverlay.xul, which is
the file where both nsContextMenu.js and imageContextOverlay.xul are included.

Comment 54

[[e-mail]]

Comment on attachment 419551 [details] [diff] [review]
Move FillInHTMLTooltip to utilityOverlay.js

Obsoleted by bug 264001.

Comment 55

[[e-mail]]

Comment on attachment 419570 [details] [diff] [review]
Finally!

This is waiting on the other patches that aren't checked in yet.

Comment 56

[Serge Gautherie (:sgautherie)]

Comment on attachment 419546 [details] [diff] [review]
[Step 2] Reintroduce the validateFileName function
[Checkin: Comment 63]


Are you expecting check-in?
(If so, please use keyword and whiteboard as needed.)

Comment 57

[:Paolo Amadini]

(In reply to comment #55)
> (From update of attachment 419570 [details] [diff] [review])
> This is waiting on the other patches that aren't checked in yet.

Sorry, I didn't understand it, on my part I was waiting for the last review
in order to test the final results of the modifications all at once :-)

Now I made several manual tests related to the previous patches and
I'll mark the corresponding attachments for check-in. I'm not sure if
attachment 419567 [details] [diff] [review] should be checked in without attachment 419570 [details] [diff] [review],
as I only tested that sweeping change with attachment 419570 [details] [diff] [review] already
applied.

Comment 58

[:Paolo Amadini]

Comment on attachment 419546 [details] [diff] [review]
[Step 2] Reintroduce the validateFileName function
[Checkin: Comment 63]

Tests done for this patch:

* From the main mail window, select a message, then
   select the menu entry File -> Save As -> File...
* From a new composer window, select the menu entry
   File -> Save As -> File..., enter a subject and
   check that the subject is used in the file name in
   the save dialog, while invalid characters like ":"
   are filtered

Comment 59

[:Paolo Amadini]

Comment on attachment 419550 [details] [diff] [review]
[Step 4] Move getContentFrameURI near its only user, and remove getContentFrameDocument
[Checkin: Comment 74]

Tests done for this patch and for attachment 419547 [details] [diff] [review]:

* From the browser window, select File -> Save Frame As...
* From the browser window, right click on a web page and
   select This Frame -> Save Frame As...

Comment 60

[:Paolo Amadini]

Comment on attachment 419563 [details] [diff] [review]
[Step 5] Temporarily avoid the "SaveAttachmentTitle" file picker title key [Checkin Comment 75]

Tests done for this patch:

* Create a mail message containing a MIME part declared
   with the "X-Mozilla-External-Attachment-URL" header,
   open it in a mail window and save the attachment

Comment 61

[:Paolo Amadini]

Comment on attachment 419567 [details] [diff] [review]
Align the signature of saveDocument, saveURL and saveImageURL

Tests done for this patch:

* Set the global preference to download automatically to
   a folder, and verify that the download starts in the
   following cases:
    - From a web page, click on a link while pressing SHIFT
    - From the browser window, type an address in URL bar,
       then press SHIFT+Enter
    - From the "Page Info" window, select an image and click
       the "Save As..." button
    - From the "View Source" window, select File -> Save Page
    - From the browser window, right click on a web page and
       select Save Page
    - From the browser window, right click on a web page and
       select This Frame -> Save Frame
    - From the browser window, right click on an image in a
       web page and select Save Image
    - From the browser window, right click on an image in a
       web page and select "Save Audio As..."
    - From the browser window, right click on an image in a
       web page and select "Save Video As..."

Tests NOT done for this patch:

* The case where, after clicking on a link to download its
   destination, the header sniffing code times out and the
   Content-Disposition headers are therefore ignored
* From the browser window, right click on a "canvas" in a
   web page and select the option to save it

Notes:

* "Save Audio As..." and "Save Video As..." in the browser
   window and "Save As..." in the "Page Info" window are
   three cases where the download is started automatically
   but the ellipsis are present in the label regardless of
   the global preference.

Comment 62

[:Paolo Amadini]

Comment on attachment 419567 [details] [diff] [review]
Align the signature of saveDocument, saveURL and saveImageURL

Maybe it's better to wait before checking this in. I was also
thinking that running this patch and the following one together
through a tryserver might be a good idea, even if not mandatory.

Comment 63

[Serge Gautherie (:sgautherie)]

Comment on attachment 419546 [details] [diff] [review]
[Step 2] Reintroduce the validateFileName function
[Checkin: Comment 63]


http://hg.mozilla.org/comm-central/rev/6f6dd128fa68

Comment 64

[Serge Gautherie (:sgautherie)]

Comment on attachment 419546 [details] [diff] [review]
[Step 2] Reintroduce the validateFileName function
[Checkin: Comment 63]

>+/**
>+ * @deprecated   Please use validateFileName from contentAreaUtils.js directly.
>+ */
> function GenerateValidFilename(filename, extension)

Should a follow-up bug be filed?

Comment 65

[[e-mail]]

(In reply to comment #64)
>(From update of attachment 419546 [details] [diff] [review])
>>+/**
>>+ * @deprecated   Please use validateFileName from contentAreaUtils.js directly.
>>+ */
>> function GenerateValidFilename(filename, extension)
>Should a follow-up bug be filed?
Yes please.

(In reply to comment #61)
>"Save Audio As..." and "Save Video As..." in the browser
>window and "Save As..." in the "Page Info" window are
>three cases where the download is started automatically
>but the ellipsis are present in the label regardless of
>the global preference.
This is another candidate for a follow-up bug.

Comment 66

[Serge Gautherie (:sgautherie)]

Comment on attachment 419547 [details] [diff] [review]
[Step 3] Move saveFrameDocument to navigator.js
[Checkin: Comment 68]


This doesn't apply to c-c:
no idea where that context comes from...

Comment 67

[:Paolo Amadini]

(In reply to comment #66)
> (From update of attachment 419547 [details] [diff] [review])
> 
> This doesn't apply to c-c:
> no idea where that context comes from...

The function named "contentAreaFrameFocus" has been removed in bug 537155,
on January 3. This function was located EXACTLY where I moved the function
in the patch! :-) I guess that now most parts of the patch queue in this
bug would need to be regenerated.

I'll take my time to learn how to do this properly, but with no hurry: the
reason for concentrating the work in a single week was to avoid this sort
of things, but we couldn't concentrate the work enough and now we have to
rebase all the patches one by one. In fact, the rebase operation can be
done starting from any base, at any point in time, since in any case I'll
have to re-test everything with the state of the tree at the time of
the rebase.

Comment 68

[Serge Gautherie (:sgautherie)]

Comment on attachment 419547 [details] [diff] [review]
[Step 3] Move saveFrameDocument to navigator.js
[Checkin: Comment 68]


http://hg.mozilla.org/comm-central/rev/a589c9a9e41d


(In reply to comment #67)

> The function named "contentAreaFrameFocus" has been removed in bug 537155,
> on January 3.

Indeed. Sorry, I must have been confused when I checked :-<

> now we have to rebase all the patches one by one.

Let's be optimistic: unless there has been major changes, I should be able to unbitrot and push quite easily :-|

(Thanks for working on this!)

Comment 69

[:Paolo Amadini]

(In reply to comment #68)
> Let's be optimistic: unless there has been major changes, I should be able to
> unbitrot and push quite easily :-|

Thanks for your help, I appreciate it. I don't have a lot of experience with
Mercurial yet, and I find it still difficult to do patch management tasks that
are trivial to others.

Feel free to push step 4 and 5, if you think they are still compatible
with the current state of the tree and the tests are still valid.

Comment 70

[Serge Gautherie (:sgautherie)]

(In reply to comment #65)

I filed bug 539478 and bug 539482.

Comment 71

[:Paolo Amadini]

I finally found some time to do some experimentation and gain confidence
with Mercurial patch management and the Mercurial Queues extension. At first
I tried to rebase my local commits, but that failed since one of the
in-between commits was obsoleted by bug 264001 (it seems to me that conflict
solving in Mercurial is a mess, even with a merge tool like kdiff3 configured,
compared with TSVN + kdiff3 to which I'm accustomed). My second attempt was
to manually apply the patches one by one using Mercurial Queues: this
succeeded, even though at one point I had to deal with a nice ".rej" file
manually while I'd have preferred the invocation of kdiff3 for that one-liner
conflict. The good thing about this story is that now I know more about these
patch management tasks, even though I'd have preferred to learn this without
haste instead of while in the middle of a bug.

The patches for steps 4 and 5 turned out to be identical, so they can be
committed right now. I'll provide the patches for the next steps with
updated context.

Comment 72

[:Paolo Amadini]

Attachment: Align the signature of saveDocument, saveURL and saveImageURL

Comment 73

[:Paolo Amadini]

Attachment: Finally!

Comment 74

[Serge Gautherie (:sgautherie)]

Comment on attachment 419550 [details] [diff] [review]
[Step 4] Move getContentFrameURI near its only user, and remove getContentFrameDocument
[Checkin: Comment 74]


http://hg.mozilla.org/comm-central/rev/40a25e269df7

Comment 75

[Justin Wood (:Callek)]

Comment on attachment 419563 [details] [diff] [review]
[Step 5] Temporarily avoid the "SaveAttachmentTitle" file picker title key [Checkin Comment 75]

pushed as: http://hg.mozilla.org/comm-central/rev/9422277f1904

Comment 76

[:Paolo Amadini]

I repeated the manual tests described previously with a recent build, and
everything worked as expected. I also did the following test related to
the "contentAreaCommandsBundle" element:

* In a web page, right click an image coming from an HTTP URL and select
   "Block Images from..."

Neil, do you think the last attachment is ready to be checked in now,
together with the alignment of the signatures of the "save*" functions?

Comment 77

[[e-mail]]

Comment on attachment 422304 [details] [diff] [review]
Align the signature of saveDocument, saveURL and saveImageURL

>diff --git a/suite/browser/navigator.js b/suite/browser/navigator.js
>--- a/suite/browser/navigator.js
>+++ b/suite/browser/navigator.js
>@@ -233,17 +233,17 @@ function getContentAreaFrameCount()
>     saveFrameItem.removeAttribute("hidden");
>   }
> }
> 
> function saveFrameDocument()
> {
>   var focusedWindow = document.commandDispatcher.focusedWindow;
>   if (isContentFrame(focusedWindow))
>-    saveDocument(focusedWindow.document);
>+    saveDocument(focusedWindow.document, true);
This could be a problem: toolkit has its own saveFrameDocument which doesn't pass true, and I suspect that the load order means that switching to toolkit will use its saveFrameDocument, which is not what we want :-(

>+        saveURL(url, "", null, false, true);
Nit: url, null, null to match what we do in utilityOverlay.js

>+function saveDocument(aDocument, aSkipPrompt)
Nit: s/aSkipPrompt/aUsePref/g, to match what it actually does

Comment 78

[:Paolo Amadini]

(In reply to comment #77)
> (From update of attachment 422304 [details] [diff] [review])
> > function saveFrameDocument()
> > {
> >   var focusedWindow = document.commandDispatcher.focusedWindow;
> >   if (isContentFrame(focusedWindow))
> >-    saveDocument(focusedWindow.document);
> >+    saveDocument(focusedWindow.document, true);
> This could be a problem: toolkit has its own saveFrameDocument which doesn't
> pass true, and I suspect that the load order means that switching to toolkit
> will use its saveFrameDocument, which is not what we want :-(

saveFrameDocument doesn't exist anymore in Toolkit, because of bug 468881:
that's why we moved this function to its own separate file.

http://mxr.mozilla.org/comm-central/search?string=saveFrameDocument

> >+        saveURL(url, "", null, false, true);
> Nit: url, null, null to match what we do in utilityOverlay.js

I'll update the attachment with this modification.

> >+function saveDocument(aDocument, aSkipPrompt)
> Nit: s/aSkipPrompt/aUsePref/g, to match what it actually does

This function is going to be removed in the following step: whatever we
write here is irrelevant. Renaming this parameter in Toolkit is already
part of bug 506987.

Comment 79

[:Paolo Amadini]

Attachment: [Step 6] Align the signature of saveDocument, saveURL and saveImageURL [Checkin: Comment 90]

Comment 80

[[e-mail]]

(In reply to comment #78)
> saveFrameDocument doesn't exist anymore in Toolkit
Sorry, I must have looked at an old version of the file my mistake.

Comment 81

[:Paolo Amadini]

Attachment: Finally!

(identical, recreated just to preserve the order in the summary)

Comment 82

[[e-mail]]

Ah, but it's still in 1.9.2 though, so that would be a problem if we wanted to build from mozilla-1.9.2 rather than mozilla-central.

Comment 83

[:Paolo Amadini]

(In reply to comment #82)
> Ah, but it's still in 1.9.2 though, so that would be a problem if we wanted to
> build from mozilla-1.9.2 rather than mozilla-central.

Is the problem here that a branch for comm-1.9.2 doesn't exist yet, and when
it will be created, it will be created from Comm trunk while the dependency on
the Mozilla main repository will be switched back to mozilla1.9.2?

In this case, wouldn't this operation potentially cause a lot of trouble for
functionality developed while depending on Mozilla trunk, apart from the
modifications in this bug?

Comment 84

[[e-mail]]

(In reply to comment #83)
> Is the problem here that a branch for comm-1.9.2 doesn't exist yet, and when
> it will be created, it will be created from Comm trunk while the dependency on
> the Mozilla main repository will be switched back to mozilla1.9.2?
comm-1.9.2 will use mozilla-1.9.2, yes.

> In this case, wouldn't this operation potentially cause a lot of trouble for
> functionality developed while depending on Mozilla trunk, apart from the
> modifications in this bug?
So far we're still mostly catching up with 1.9.2, but a problem has already arisen in that the Places preferences are different on mozilla-central.

Comment 85

[:Paolo Amadini]

This mode of operation makes it extremely difficult to develop functionality
that crosses the boundaries of the two repositories. Let's take this bug
as an example: we're about to make a change that requires mozilla-central,
but won't work with mozilla1.9.2. The only way out (supposing that we don't
modify the patch) is to wait for comm-1.9.2 to branch, then make the change
only on comm-central. At this point we *might* be able to commit the patch.

But hypothetically, in the worst case, mozilla1.9.3 branches before
comm-1.9.2 (I don't know if this is actually possible), or in any case
before we can commit the patch on this bug, and meanwhile the Toolkit API
in "contentAreaUtils.js" has changed in an incompatible way on mozilla-central.

At that point comm-central would have to be compatible with both the new
mozilla-central and mozilla1.9.3, but this is not possible so we return
in the initial situation, waiting for comm-1.9.3 to branch. Each iteration
lasts several months.

In practice, I think this only means that cross-repository modifications
are feasible during a limited time frame, when there are no discrepancies
between the branching status of Mozilla and Comm. And respecting this time
frame can be difficult if the time for code review is not planned in advance.

Now, returning to the problem at hand, do you see some other way we can be
compatible with both trunk and mozilla1.9.2, or do we have to wait for the
next comm-central branch?

Comment 86

[:Paolo Amadini]

(In reply to comment #85)
> Now, returning to the problem at hand, do you see some other way we can be
> compatible with both trunk and mozilla1.9.2, or do we have to wait for the
> next comm-central branch?

I mean, it's quite obvious that for the saveFrameDocument issue in particular
we just have to modify the patch and rename the function... but should the
modification and all the other changes be tested now with both Mozilla
trunk and branch, or is it sufficient that we "expect it to work", and
the final verification will be done when comm-central branches? How are
other bugs that have such issues handled?

Another issue is that we can't reintroduce the "Save Attachment" file picker
title key (bug 537256) and thus complete all the aspects of this bug until
the next comm-central branch is created, unless the Toolkit API enhancement
for bug 537231 is also backported to the mozilla1.9.2 branch. If the API
enhancement is not approved for backport, we might have to wait for the next
comm-central branch in any case for this aspect of the change.

Comment 87

[:Paolo Amadini]

Should I attempt a build with comm-central and the mozilla1.9.2 branch and
see if everything works as expected, or are the tests on trunk sufficient?

Comment 88

[:Paolo Amadini]

I don't have a clear understanding of what's holding the main change in this
bug from being checked in. From my point of view, there are only two minor
non-blocking points still open:

* The possibility that the title of a rarely displayed file picker will
   change from "Save Attachment" to "Save As" for an entire release,
   without us being able to restore it in the 1.9.2 branch, and
* The possibility that we will experience small regressions when the
   dependency on the Mozilla repository is switched from mozilla-central
   to mozilla1.9.2. This is a possibility into which any code developed at
   this time can occur, and in this case it can be minimized just by
   renaming a function, even after the main change is checked in. Some
   explicit testing may also help, but I think we can also do without
   and still be safe enough.

Do you agree with this analysis, or is there something I'm missing?

Comment 89

[:Paolo Amadini]

Talking with Neil in IRC revealed that this bug is just waiting for his
attention. Since this might require some time, in the meanwhile I rebuilt
with only attachment 423790 [details] [diff] [review] applied and repeated the tests in comment 61
both with and without the option to save automatically without prompting.
Since this works and is already reviewed, I'm marking it for check-in.

Comment 90

[Serge Gautherie (:sgautherie)]

Comment on attachment 423790 [details] [diff] [review]
[Step 6] Align the signature of saveDocument, saveURL and saveImageURL
[Checkin: Comment 90]


http://hg.mozilla.org/comm-central/rev/21a3938f98d8

Comment 91

[Robert Kaiser]

(In reply to comment #87)
> Should I attempt a build with comm-central and the mozilla1.9.2 branch and
> see if everything works as expected, or are the tests on trunk sufficient?

In the mean time, we have decided to drop all 1.9.2 support and concentrate on 1.9.3 for SeaMonkey-specific code, so those problems should be gone. Please only test with mozilla-central and don't care about 1.9.2 if the files are SeaMonkey-specific (i.e. everything in suite, many - but not all - files in editor/ui).

Comment 92

[:Paolo Amadini]

(In reply to comment #91)
> In the mean time, we have decided to drop all 1.9.2 support and concentrate on
> 1.9.3 for SeaMonkey-specific code, so those problems should be gone.

That's great news! It really makes things easier. All the tests in
comment 61 should be valid at the current revision too, since from the
logs it looks like the main code in the affected files is unchanged.

I think the patch is ready for review and check-in.

I have only one doubt: the license header in a file I removed
(/suite/common/tests/unit/test_contentAreaUtils.js) has changed,
and I don't know if now the removal would succeed or would generate
a conflict. Do I need to regenerate the patch?

Comment 93

[Philip Chee]

Yeah you'd need a new patch.

Comment 94

[:Paolo Amadini]

Attachment: [Step 7] Finally! [Checkin: Comment 100]

(In reply to comment #93)
> Yeah you'd need a new patch.

Thanks, I generated the new patch, which is identical to the previous one
except for the different comment in the removed file.

Comment 95

[[e-mail]]

I wonder whether toolkit are interested in porting bug 445831 at all.

Comment 96

[Serge Gautherie (:sgautherie)]

(In reply to comment #95)

See bug 475283 comment 8.

Comment 97

[:Paolo Amadini]

"Finally!" :-) Thank you, Neil, Robert and Serge for helping me through
this platform enhancement, that, though minor, now allows me to work on
some related Toolkit improvements and bug fixes together with their test
cases. If everything goes as expected, those fixes will be available
without requiring any modification to the code in SeaMonkey.

Comment 98

[:Paolo Amadini]

(In reply to comment #95)
> I wonder whether toolkit are interested in porting bug 445831 at all.

I think that if caching the string bundle is the goal (bug 445831 comment 0)
a private "smart getter" might achieve the same result; I specified it in
bug 475283, thanks.

Comment 99

[[e-mail]]

The <stringbundle> element also exposes a slightly easier to use API.

Comment 100

[Serge Gautherie (:sgautherie)]

Comment on attachment 428771 [details] [diff] [review]
[Step 7] Finally!
[Checkin: Comment 100]


http://hg.mozilla.org/comm-central/rev/b6e234049013