95088 - Send Page from Composer should not force saving page

Status: NEW

(SeaMonkey :: Composer, enhancement, P5)

Description

[[e-mail]]

AFAIK this should be possible using a data: URL

Comment 1

[rubydoo123]

you don't need to save to preview, you do need to save to browse -- is that the
option you are refering to?

Comment 2

[[e-mail]]

Yes, sorry, I was thinking of 4.x

Comment 3

[Kathleen :Brade]

In 4.x, you needed a saved document to browse.  This is true in 6.x also.

The question we need to investigate for this bug is whether we can use a data url 
instead of forcing the user to save.  This isn't a high priority bug right now 
but we would welcome a patch.

Comment 4

[rubydoo123]

future

Comment 5

[[e-mail]]

Attachment: Proposed patch

Comment 6

[Charles Manske]

Very interesting trick! First, we would want this as a separate method so it
can be shared by Print and Send Page commands, which also have the same problem
of requiring saving before the command.
Note that these lines in each of those commands:
 if (!CheckAndSaveDocument(window.editorShell.GetString("BeforePreview"),
DocumentHasBeenSaved()))
    return;
should be removed if we use this, since that is what prompts for the user to 
save and the purpose of the fix is to not require that. 
I wonder if there's a problem with the number of characters in the output: Do 
you know if there's a limit on the length of a URL? I noticed some problems in
the Browser when testing this: Since the entire contents of the page is being
placed in the URL string, it tries to display that URL in the Browser's 
location textfield. It seems to have problems with a doc that was 63k long.
Maybe we should modify the Browser's code to test for "data:" urls and not show
the contents in the location field?
Thanks for the help on this, Neil!

Comment 7

[Charles Manske]

Modifying Summary to cover all cases where we want to avoid saving the page.

Comment 8

[[e-mail]]

The purpose of my fix was to show the current version of the document even if it
hadn't been saved, rather than showing the last saved version. I wasn't sure
whether it was a good idea to remove the prompt altogether so I left it in.

Comment 9

[Charles Manske]

Right, the goal is to show the *current* version in Browser, which is why
we don't want to bother the user to save the file!

Comment 10

[[e-mail]]

My fault for not asking MPT first :-)

I see no problems with print but I seem to remember that Send Page also sends
the URL of the sent page. If I am right then it wouldn't like a data: URL...

Comment 11

[[e-mail]]

Attachment: Patch to handle browse and save (print is in C++)

Comment 12

[[e-mail]]

Any chance you can check in this bug just for preview and send (why did I say
save?) and then open a new bug for print?

Comment 13

[Charles Manske]

I'm still worried about the problems of putting the entire document in the
url, as I noted above, e.g., the bad effects of a long doc in Browser's 
location field. Did you investigate that at all?

Comment 14

[[e-mail]]

Attachment: fix pre; workaround location

Comment 15

[Charles Manske]

Latest fix for previewing in browser is better.
Testing "Send Page" causes various asserts in the network code, and sometimes, 
but not always, sending fails.
Unfortunately this still needs more testing, especially to investigate the "Send 
Page" problem.

Comment 16

[Syd Logan]

spam composer change

Comment 17

[Charles Manske]

Tested code again today: 
1. Load the www.mozilla.org page in browser,
2. Use File | Edit Page  to load into Composer
3. Use File | Send Page  to email it.

It's failing to send the message in nsMsgComposeAndSend::HackAttachments at the 
line:
int status = m_attachments[i].SnarfAttachment(mCompFields);
status is non-zero; Here's the full stack:
nsDebug::WarnIfFalse(const char * 0x0488b908, const char * 0x0488b8b4, const
char * 0x0488b880, int 368) line 396 + 21 bytes
nsURLFetcher::FireURLRequest(nsURLFetcher * const 0x0469be40, nsIURI *
0x0469dc50, nsILocalFile * 0x0469eb90, nsIFileOutputStream * 0x0469dba0,
unsigned int (unsigned int, const char *, const char *, int, const unsigned
short *, void *)* 0x048614c0 FetcherURLDoneCallback(unsigned int, const char *,
const char *, int, const unsigned short *, void *), void * 0x0469d3c4) line 368 + 11
nsMsgAttachmentHandler::SnarfAttachment(nsMsgCompFields * 0x0469c250) line 922 +
77 bytes
nsMsgComposeAndSend::HackAttachments(const nsMsgAttachmentData * 0x00000000,
const nsMsgAttachedFile * 0x00000000) line 2487 + 37 bytes
nsMsgComposeAndSend::Init(nsIMsgIdentity * 0x04c4bd40, nsMsgCompFields *
0x04ce3b10, nsFileSpec * 0x00000000, int 0, int 0, int 0, nsIMsgDBHdr *
0x00000000, const char * 0x04884f7c, const char * 0x00000000, unsigned int 0,
const nsMsgAttachmentData * 0x00000000, const nsMsgAttachedFile * 0x00000000,
const char * 0x100cbe38 gCommonEmptyBuffer) line 2840 + 16 bytes
nsMsgComposeAndSend::CreateAndSendMessage(nsMsgComposeAndSend * const
0x0469f920, nsIEditorShell * 0x04ba75e0, nsIMsgIdentity * 0x04c4bd40,
nsIMsgCompFields * 0x04ce3b10, int 0, int 0, int 0, nsIMsgDBHdr * 0x00000000,
const char * 0x04884f7c, const char * 0x00000000, unsigned int 0, const
nsMsgAttachmentData * 0x00000000, const nsMsgAttachedFile * 0x00000000, void *
...) line 3592 
nsMsgCompose::_SendMsg(int 0, nsIMsgIdentity * 0x04c4bd40, int 0) line 831 + 231
bytes
nsMsgCompose::SendMsg(nsMsgCompose * const 0x04ce7cb0, int 0, nsIMsgIdentity *
0x04c4bd40, nsIMsgProgress * 0x0580e390) line 937 + 20 bytes
XPTC_InvokeByIndex(nsISupports * 0x04ce7cb0, unsigned int 7, unsigned int 3,
nsXPTCVariant * 0x0012bd4c) line 154
XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode
CALL_METHOD) line 1952 + 42 bytes
XPC_WN_CallMethod(JSContext * 0x046caba0, JSObject * 0x03332240, unsigned int 3,
long * 0x03610210, long * 0x0012bf84) line 1254 + 14 bytes

Comment 18

[Charles Manske]

Attachment: Update for bitrot (same as Neil's last, but license region changed)

Comment 19

[Charles Manske]

changing milestone

Comment 20

[Charles Manske]

I suspect that the problems with sending mail messages using data: URLs may be
related to bug 102779.

Comment 21

[Charles Manske]

load balancing

Comment 22

[Charles Manske]

If we still have problems using the "data:" strategy for "Send Page", then I 
suggest we use it only for Preview In Browser. It seems that we can print without
forcing a save, so that's not a problem

Comment 23

[Syd Logan]

keeping EDITORBASE

Comment 24

[Matthew T (active 1999-2002)]

> I wasn't sure whether it was a good idea to remove the prompt altogether so I
> left it in.

I see the prompt is still in the latest patch -- please do remove it. See bug 
62181 comment 3 onwards for why. Marking dependency.

Comment 25

[Charles Manske]

Don't worry! I will remove the prompt in version I checkin.

Comment 26

[Charles Manske]

Attachment: Updated patch: Just the portion for "Preview Page", updated for bitrot

Comment 27

[Charles Manske]

Comment on attachment 61151 [details] [diff] [review]
Updated patch: Just the portion for "Preview Page", updated for bitrot

r=cmanske
(Fix is by Neil)

Comment 28

[Charles Manske]

So the plan with the attachment (from #26) is to do just the "Preview in Browser"
part. We don't need to do anything for Print (normal printing uses DOM and thus 
doesn't force us to save). 
We will hold off using "data:" URL to mail the page.

Comment 29

[kinmoz]

Comment on attachment 61151 [details] [diff] [review]
Updated patch: Just the portion for "Preview Page", updated for bitrot

Looks ok to me, just a couple of questions:

Why do you have to spell out the following:
"chrome,scrollbars,status,menu,toolbar,directories,sidebar" in the call to
OpenDialog? The old code just did a "chrome,all".

Do we need to wrap the GetContentAs() call with a try/catch just in case the
editor throws an error?

sr=[e-mail]

Comment 30

[kinmoz]

I should've asked this earlier, but is there any negative impact this can have 
on session history, in the browser window that it brings up? The document we are 
previewing has the potential to be quite large.

Comment 31

[[e-mail]]

I listed out the chrome I wanted because I didn't want the location bar, since
the location passed to the browser is a data: URL and not a real location.

Comment 32

[[e-mail]]

This seems to work:
window.open(previewURI, 'EditorPreview', 'all,location=no,resizable=yes')
Although I noticed that big data: URLs can get sent as Referrer: headers :-(

Comment 33

[Charles Manske]

Comment on attachment 61151 [details] [diff] [review]
Updated patch: Just the portion for "Preview Page", updated for bitrot

I think the problem with referrer means we shouldn't use "data:" URLs to solve
this bug. Milestone changed to investigate using temp file instead.

Comment 34

[Charles Manske]

moving milestone

Comment 35

[Kathleen :Brade]

removing EDITORBASE per meeting

Comment 36

[Charles Manske]

I've inquired with Neil if he can get this done using temp files.

Comment 37

[Charles Manske]

changing milestone

Comment 38

[Charles Manske]

Changing summary: I think "Browse page" should force saving the file first.

Comment 39

[[e-mail]]

Attachment: New approach

This always previews the saved version of the document.
Then, it promptly overwrites the document with the currently edited version!

Comment 40

[Kathleen :Brade]

Neil:  be sure to add a comment about the flags you are using (I know you can't
grab the actual flags but put them in a comment so people can easily know what
they are)

Comment 41

[[e-mail]]

Attachment: Commented flags

I always wondered about these constants. Other portions of the code manage to
expose constants to JS, although I don't know how it works, it just does :-)

Comment 42

[[e-mail]]

dbradley says "you can declare a list of int constants in the IDL", any help?

Comment 43

[Charles Manske]

Neil: it's normally very easy -- you just define some "const ..." in the 
IDL, inside the interface. E.g.:
interface nsIWebProgressListener : nsISupports
{
   const unsigned long STATE_START = 0x00000001;

But the reason you cant do it for GetContentAs is because the flags 
are defined in nsIDocumentEncoder.h; there's no IDL file :(
I suppose we could add them to an editor IDL interface. Remember, btw, that 
nsIEditorShell is being remove, so we shouldn't add new things there.

Comment 44

[Charles Manske]

Very interesting idea! I tried it and the browser loaded the modified document,
but then reverted to the content from the file!
The patch is somewhat funky! Only the last 'ComposerCommands.js' is supposed
to be used, right?

Comment 45

[Charles Manske]

Not going to happen for 1.0

Comment 46

[Syd Logan]

nsbeta1- per buffy triage

Comment 47

[benc]

You shouldn't use data: URLs for this.

From RFC2397  The "data" URL scheme:

2. Description

   Some applications that use URLs also have a need to embed (small)
   media type data directly inline. This document defines a new URL
   scheme that would work like 'immediate addressing'.

...

   The "data:" URL scheme is only useful for short values. Note that
   some applications that use URLs may impose a length limit; for
   example, URLs embedded within <A> anchors in HTML have a length limit
   determined by the SGML declaration for HTML [RFC1866]. The LITLEN
   (1024) limits the number of characters which can appear in a single

   attribute value literal, the ATTSPLEN (2100) limits the sum of all
   lengths of all attribute value specifications which appear in a tag,
   and the TAGLEN (2100) limits the overall length of a tag.

6. Security

...
   The effect of using long "data" URLs in applications is currently
   unknown; some software packages may exhibit unreasonable behavior
   when confronted with data that exceeds its allocated buffer size.

Comment 48

[Wayne Mery (:wsmwk)]

[mod summary - print no longer requires saving page]

is avoiding "save" before send really useful?
If you're in the process of working on something don't you want it saved to disk to avoid potential lose of your work?

Comment 49

[Matthew T (active 1999-2002)]

Sure, replacing the "Save" menu item with automatic saving would be great. But if you're going to keep saving as a manual process, don't force me to save before seemingly-unrelated operations.

Comment 50

[Wayne Mery (:wsmwk)]

shouldn't bug 62181 be a blocker or dup rather than dependent?