105561 - Editor gives stupid message when started on nonexistant file

Status: VERIFIED FIXED

(SeaMonkey :: Composer, defect)

Description

[Akkana Peck]

mozilla -edit dumdum.html gives me a dialog saying "This page can't be edited
for an unknown reason", and when I dismiss it, I get another copy of the same
dialog; then the editor window goes away and the app exits.

The error message is unenlightening, and we should only show the dialog once.

Comment 1

[Charles Manske]

Hmmm. I fixed this recently. On my Windows build, I see the new message:
"The file [filename] cannot be found. Please check the location and try again."
which seems like a decent message.
Network code is supposed to generate the error NS_FILE_NOT_FOUND and it is putting 
up the above message. We detect that in nsEditorShell::EndPageLoad and simply 
close the editor window.
This seems to work when we load a non-existing file from the Open Location dialog,
but when we open on the commandline using "-edit", the errorcode from network
is not NS_FILE_NOT_FOUND, so we don't detect that situation.
Akkana: Do you see the first dialog (with proper) message first? I do on windows.

Comment 2

[Akkana Peck]

You're right, the latest build looks much better.  The first dialog says the
file can't be found, and the second gives the "unknown reason" message.  So this
bug only applies to the second dialog, and the repetition of the message is no
longer a problem.

Comment 3

[Charles Manske]

This now seems to be working; tracing into nsEditorShell::EndPageLoad() shows 
that the aStatus error message is NS_ERROR_FILE_NOT_FOUND and we no longer 
display the second dialog before closing the window.
Akkana: Please confirm if it is working for you; if yes, just close this as
"WORKSFORME"

Comment 4

[Charles Manske]

Attachment: Simplify error checking during end of page loading.

Bug still existed on Linux. Simplified code should work much better.

Comment 5

[Charles Manske]

Note that patch looks complicated, but all I really did is test for 
  if (aStatus == NS_ERROR_FILE_NOT_FOUND)
first. Then all the other code for testing mime type etc. follows without change
except for not having to test for (mCantEditReason != eCantEditFileNotFound)
when displaying the error dialog. 
And this block was moved:
+  nsAutoString doneText;
+  GetBundleString(NS_LITERAL_STRING("LoadingDone"), doneText);
+  SetChromeAttribute(mDocShell, "statusText", NS_LITERAL_STRING("label"),
doneText);
+
lower in codepath since the status line text doesn't need to be set when
when had an error and closed the window.

Comment 6

[Akkana Peck]

Yes, that fixes it, and looks like a better way to do things.

One comment on the patch: you have:
if (mCloseWindowWhenLoaded)
{ 
  ... 
}
if (mCloseWindowWhenLoaded)
{
  ... 
}

That doesn't look intentional ... should probably be coalesced into one clause
since mCloseWindowWhenLoaded isn't modified in the first clause (or am I missing
something?)  Fix that (or explain why it's right) and r=akkana.

Comment 7

[Charles Manske]

I knew you would ask that!
The double "if (mCloseWindowWhenLoaded)" is intentional. One is inside the the 
"else" block (when aStatus != NS_FILE_NOT_FOUND). It may *not* be set true in
that block -- only if there's an error found. That test is done to decide if we
should put up our error dialog. (Remember, we don't want to do that if the error 
was NS_FILE_NOT_FOUND.)
Thus the second "if (mCloseWindowWhenLoaded)" is outside all the error diagnostics
and it's in that block that we actually close the window because we can't edit
the file.

Comment 8

[Akkana Peck]

Comment on attachment 57630 [details] [diff] [review]
Simplify error checking during end of page loading.

Oops!  You're right, I missed the alignment in tkdiff.	Looks fine, then,
r=akkana.

Comment 9

[Kathleen :Brade]

I'd like to see Charley's explanation (or something similar) added as a comment
in the code so no one else gets confused and tries to "fix" it for us.

Comment 10

[Charles Manske]

If you read the complete method, not the diff, it is very logical. I don't think 
extra comments are really necessary.

Comment 11

[kinmoz]

Comment on attachment 57630 [details] [diff] [review]
Simplify error checking during end of page loading.

sr=[e-mail]

Comment 12

[Charles Manske]

checked in

Comment 13

[rubydoo123]

gratuitous verify, been around a long time