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.)

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. }

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 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);

(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 ;-)

(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.

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

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 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.

(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.

(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.

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.

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.

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"?

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.

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.

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.

(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.

(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?

(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.

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

(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...

(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.

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.

(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 :-)

> (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?

(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.

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).

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

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 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 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 on attachment 419399 [details] [diff] [review] Reintroduce the validateFileName function Hit Enter instead of Del :-( See the following comment...

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".

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 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.

(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.

(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.

"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 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]

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

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.

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

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

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

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.

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 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.

(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 on attachment 419570 [details] [diff] [review] Finally! Don't nsContextMenu.js and imageContextOverlay.xul still need contentAreaCommandsBundle?

(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 on attachment 419551 [details] [diff] [review] Move FillInHTMLTooltip to utilityOverlay.js Obsoleted by bug 264001.

Comment on attachment 419570 [details] [diff] [review] Finally! This is waiting on the other patches that aren't checked in yet.

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.)

(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 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 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 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 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 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 on attachment 419546 [details] [diff] [review] [Step 2] Reintroduce the validateFileName function [Checkin: Comment 63] http://hg.mozilla.org/comm-central/rev/6f6dd128fa68

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?

(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 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...

(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 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!)

(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.

(In reply to comment #65) I filed bug 539478 and bug 539482.

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 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 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

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 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

(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.

(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.

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

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.

(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?

(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.

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?

(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.

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?

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?

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 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

(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).

(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?

Yeah you'd need a new patch.

(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.

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

(In reply to comment #95) See bug 475283 comment 8.

"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.

(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.

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

Comment on attachment 428771 [details] [diff] [review] [Step 7] Finally! [Checkin: Comment 100] http://hg.mozilla.org/comm-central/rev/b6e234049013