After using Composer to publish/save, the html source will show a base tag that wasn't present before. nsWebBrowserPersist should remove the base tag if it wasn't there previously.

Persist is putting a BASE tag in to ensure relative links work when you save to file. I am guessing that this is not the correct behaviour when saving remotely.

Actually the issue is that it is inserting it into the actual dom but the other changes are happening on a copy of the DOM so we are a bit out of synch. It seems to me that nsWebBrowserPersist should remove the base node when it is done (if it inserted it) or change it back to the original value (if it was present). I believe it tries to do the latter but not the former.

oops! clarification: I don't think it has to do with local vs remote. I think we see the base tag get added in every case where the base tag wasn't initially there. Also, note that the actual output does not show the base tag so there isn't a problem if the user closes the composer window and reopens it.

I think the output DOES show the base tag that is added by nsIWebBrowserPersist. We realize that it is a good idea to add the base tag, but for Composer it should be added before the file is uploaded and have the correct href for the new location. What I have noticed in debugging is that the newly-added base tag has the href of the source file, not the new destination url. So that is the real problem. It is not a trivial problem, however, since setting the href url to an "ftp://username:password@ftp.foo.com/filename" would also not be correct in the case that the user is actually wants the "http://www.foo.com/filename" version. Composer is supposed to have code to fix that href in its OnLocationChange callback, but that isn't working correctly (bug 125085).

Composer should not always set a BASE tag. What if there isn't ftp or http upload available? I'm editing local html, and using scp to copy the file. Since Composer doesn't know about the remote filename, it will add a base href to a file on my local computer that won't work for anyone else on the web. At least, please make adding a base tag to be configurable. If you can't make it configurable, at least let me edit it in the HTML source, and don't add it back in when I save!!

*** Bug 126783 has been marked as a duplicate of this bug. ***

*** Bug 127566 has been marked as a duplicate of this bug. ***

i filed the same issue in bug 126851, which was resolved dup of bug 122227 Regarding comment 3: >I think we see the base tag get added in every case where the base tag wasn't >initially there. This is wrong. Composer currently *modifies* existing base tag, adding full local path.

Comment on attachment 71354 [details] [diff] [review] add flag to not do base tag manipulations (needed for Composer) r=adamlock

Comment on attachment 71354 [details] [diff] [review] add flag to not do base tag manipulations (needed for Composer) >+ persistObj.persistFlags = persistObj.persistFlags | webPersist.PERSIST_FLAGS_NO_BASE_TAG_MODIFICATIONS Are you missing a ; here? Fix that and sr=sfraser

Comment on attachment 71354 [details] [diff] [review] add flag to not do base tag manipulations (needed for Composer) a=roc+moz for 0.9.9

fix checked in

The fix checked in to ComposerCommands.js for this bug (which doesn't match the patch, BTW), caused a serious regression bug 128538. I've backed out this fix on the branch, and will do likewise on the trunk. Hopefully it will be a simple matter to rectify things.

The reason this caused the regression is that the persistence object starts life in an inconsistent state. Initially we have: mReplaceExisting(PR_TRUE), mPersistFlags(kDefaultPersistFlags), where const PRUint32 kDefaultPersistFlags = nsIWebBrowserPersist::PERSIST_FLAGS_NO_CONVERSION; Now when we execute the js in this patch: persistObj.persistFlags = persistObj.persistFlags | webPersist.PERSIST_FLAGS_NO_BASE_TAG_MODIFICATIONS [sic, without semicolon at the end] WebBrowserPersist does: mReplaceExisting = (mPersistFlags & PERSIST_FLAGS_REPLACE_EXISTING_FILES) ? PR_TRUE : PR_FALSE; And the state of the flags becomes synced with mReplaceExisting. Since this is likely to bite people other than the composer, I suggest that we make the default flags include PERSIST_FLAGS_REPLACE_EXISTING_FILES, since the initial state acts as if that flag is set.

*** Bug 128740 has been marked as a duplicate of this bug. ***

I have filed bug 128900 to track Boris' comments. I will attach a patch (which technically will cover more than the bug described here; it'll cover some other bugs on my list as well). This patch will always set the replace flag for Composer since there really isn't a case when we wouldn't want to replace the file.

Composer should always set replace flag. The only change between what I checked in Friday and this patch always sets the replace flag and has a comment explaining that. Here is a rundown on the whole patch (which covers several bugs): add const for web persist component for readability add flag variable for "isLocalFile" and set appropriately always set crlf for non-local file locations (4.x parity) set persist flags (which is what introduced the regression) user persist flags for encoding instead of hard-coded values add a comment after one line where we are doing bad things (needs its own bug)

Comment on attachment 72457 [details] [diff] [review] patch to fix 125070 and 126672 r=adamlock

Comment on attachment 72457 [details] [diff] [review] patch to fix 125070 and 126672 sr=sfraser. Please file a bug on the parentDir == root issue.

Comment on attachment 72457 [details] [diff] [review] patch to fix 125070 and 126672 a=asa (on behalf of drivers) for checkin to the 1.0 trunk

This patch is reduced (as requested) for the 0.9.9 branch. It does not do the conversion to webbrowser persist constants but simply does the flag setting for persist.

fix checked into trunk; waiting to resolve after determination of whether some of this fix will land on the 0.9.9 branch.

bug 129054 filed on the null parent issue

Comment on attachment 72592 [details] [diff] [review] proposed diff for branch (don't bother with constant cleanup) a=asa (on behalf of drivers) for checkin to the 0.9.9 branch

fix checked into branch too This should be verified in both places.

Verified on 03-06 trunk and branch builds on Win XP and 03-06 trunk build on Mac OSX. If anyone is still seeing this problem, feel free to reopen this bug.

*** Bug 131600 has been marked as a duplicate of this bug. ***