using 2/18 build of netscape 1) launch netscape 2) launch composer 3) new blank page 4) add some text 5) File | Publish 6) enter invalid publish URL ( "dhkjhlsdhsd" ) 7) Publish nothing happens....expecting a dialog/panel to come up telling you that the URL is invalid or something like this.

-->cmanske

Yes, we don't have any error feedback yet. I think fixing bug 27609 will take care of most of the standard error conditions.

Generalizing the summary to cover all error checking feedback.

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

We should see alerts for all errors occurring while reading data or writing to disk. I'm not sure about the "publishing" aspect. But the mechanism is in place now, I think. If there is an issue in the publishing scenario, then this can be reopened (and assigned to somebody else :-).

Since we use nsIWebBrowserPersist for publishing, notifying via the nsIWebProgressListener::alert's should work just fine! Look forward to testing this.

still not working, here is an example: 1) launch netscape 2) launch composer 3) new blank page 4) enter some text 5) File | Publish 6) give it an invalid publish URL(for example, "lksjf" ) 7) click OK on publish panel nothing happens, I'm expecting an error panel to pop up telling me that the URL is invalid

additional error checking: check for page title

I don't understand "check for page title." We are not planning to prompt the user for a page title (the presence of the field should be sufficient). We will output a title tag (unlike 4.x which only wrote the title if there was text for it) so the document won't be invalid due to that issue.

okay I understand the page title field not needing a title.

nominating EDITORBASE.....already marked nsbeta1+

We pass through the error messages given to use via nsIPrompt::Alert notificaions. I'm not sure if there's any other errors we should trap. The major problem in this area is bug 132320: we don't get error status when FTP login fails. We get an alert, but no status value indicating an error

@@ -852,6 +859,40 @@ : Added dump of aStatus values during onStateChange method of the progress listener (copied from existin "onStatus" dumps). This is done only if debug flag is set. @@ -883,48 +924,70 @@ : Less change than it looks: Do processing of non-zero aStatus value during any STATE_STOP notification instead of just when STATE_IS_NETWORK is also set. This is intended to abort publishing quicker when there's an error instead of piling up repeated errors such as when trying to publish > 1 images to the same bad directory. Also adds specific error handling when aStatus indicates the "Directory or file doesn't exist" error, which primarily happens when trying to FTP publish to a directory that doesn't exist on the server. @@ -1074,23 +1142,9 @@ and following blocks : Carefull cleanup of all the nsIPrompt an nsIAuthPrompt methods to clarify when params are "inout" (use "...Obj" notation to indicate that). Make sure appropriate methods are called (e.g., UpdateUsernamePasswordFromPrompt() after prompting for a username and/or password) after prompt dialog is called.

Charley, can you list the user-visible error message text strings? I'd like to review them. Thanks.

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

This bug is critical to Publishing feature

Comment on attachment 77000 [details] [diff] [review] Patch v1 comments: 1) should the error codes be in a .properties file ? 2) why the need for a setTimeout for the cancelpublishing() call? 3) be sure to get a test build for qa to try out. 4) not sure why we need all those dump statements (aStatus)- arent they just for debugging?

Answers to your questions: 1) should the error codes be in a .properties file ? They SHOULD be defined in an IDL file. But because of the way we generate them from macros, that is very difficult. I talked to Bill Law about this and there's a plan to fix this problem in the future. I think putting them in a properties file would only confuse I18N. They are numbers, not strings, anyway. 2) why the need for a setTimeout for the cancelpublishing() call? I must return to the network code before calling it to stop the network activities by: gPersistObj.cancelSave() in CancelPublishing() 3) be sure to get a test build for qa to try out. yes, of course! Doing that right now 4) not sure why we need all those dump statements (aStatus)- arent they just for debugging? Yes they are. There's a const global set so they show up only when we need them. Before, they were only in onStatusChange. The place we really need them is in onStateChange

Comment on attachment 77000 [details] [diff] [review] Patch v1 ok r=andreww

Comment on attachment 77000 [details] [diff] [review] Patch v1 For the next patch please use more context, makes reviewing a lot easier. -9 or -10 is a good default choice for reviews; the default -3 is not enough for any but the most most trivial of fixes. >+ // XXX We don't have error codes in IDL !!! >+ // Give better error messages for cases we know about: >+ if (aStatus == 2152398868) // Bad directory Sure we do, look at the uses of const in nsIPrefBranch.idl, nsIDOMEvent.idl and others. Then you point some var at the iface: var myclass = Components.interfaces.nsIMyClass; if (aStatus == myclass.BAD_DIRECTORY) ... The code would be a lot more readable (at the cost of a slightly larger .xpt file/memory use). Or you could even define the consts locally in this .js file if that made more sense. Either way, 215298868 is way too ugly for words. Defining these in hex might be less error prone, by the way. Fewer characters, easier to eyeball.

Dan: I don't understand the relevance of your examples. Of course I know how to define const values in an idl file. But the values I need are already defined in .h files like this: NS_ERROR_FILE_UNRECOGNIZED_PATH NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 1) NS_ERROR_FILE_UNRESOLVABLE_SYMLINK NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 8) NS_ERROR_FILE_INVALID_PATH NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 9) NS_ERROR_FILE_DISK_FULL NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 10) NS_ERROR_FILE_CORRUPTED NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 11) NS_ERROR_FILE_NOT_DIRECTORY NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 12) NS_ERROR_FILE_IS_DIRECTORY NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 13) NS_ERROR_FILE_IS_LOCKED NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 14) NS_ERROR_FILE_TOO_BIG NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 15) NS_ERROR_FILE_NO_DEVICE_SPACE NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 16) NS_ERROR_FILE_NAME_TOO_LONG NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 17) NS_ERROR_FILE_NOT_FOUND NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 18) NS_ERROR_FILE_READ_ONLY NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 19) NS_ERROR_FILE_DIR_NOT_EMPTY NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 20) NS_ERROR_FILE_ACCESS_DENIED NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, 21) Are you suggesting that I map the current value to some JS const like this: const long ACCESS_DENIED = 2152398868; ? If "NS_ERROR_MODULE_FILES" happens to change, then this value will be wrong. While that is more readable in the JS, doesn't that just obscure the problem of getting at the internal the error codes defined in macros? I felt that by putting the "hard coded" value in JS and commenting on the problem would keep the basic problem more visible. I agree that hex values would be more readable :-) Bill Law: Care to comment?

> Are you suggesting that I map the current value to some JS const like this: > const long ACCESS_DENIED = 2152398868; > ? > If "NS_ERROR_MODULE_FILES" happens to change, then this value will be wrong. w/out the "long" for js, yes. If the real NS_ERROR_MODULES_FILES "happens" to change (which will break an unknown number of 3rd-party components) you're toast whether you define a const or use the raw number; I vote for readability. If those standard file errors are in fact the ones you're using then they may already be available hanging off Components.results, see http://lxr.mozilla.org/mozilla/source/js/src/xpconnect/src/xpc.msg#131 But ok, sr=dveditz either way.

>Bill Law: Care to comment? Yes. The whole mechanism of using C++ macros to define error codes and not being able to define mnemonics for them in JS is badly broken. I'm not sure I quite agree with the claim that "there's a plan to fix this problem in the future." We threw out some vague ideas in passing, but that wouldn't constitute a "plan." Nailing it down (and presuming no fundamental shift in the current mechanism), what we need are to: 1. Make the "module IDs" available to JS somehow. 2. Move the low-order bits of the error code (the last argument on the NS_ERROR_GENERATE_FAILURE macro) to idl constants. I.e., they end up looking like: NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_FILES, nsIFile::BAD_DIRECTORY); That would permit JS code to re-constitute the actual values (the same way NS_ERROR_GENERATE_FAILURE does). Components.errors could assist, perhaps (maybe by exposing the module id values). Probably step 2. is sufficient in the vast majority of the cases, since the module can be safely ignored.

Note to ADT: As discussed at the 4/2 ADT meeting, this bug fix isn't 100% completed, but we'd like to checkin what is there as an interim because the current patch it fixes basic problems in error prompt dialogs and the specific case when the directory we try to publish to doesn't exist. We need to get this part in so we can get more thorough testing started ASAP.

adt1.0.0+ (on ADT's behalf) for approval for checkin for the current patch. Pls ask for approval, by changing adt1.0.0+ to a nomination adt1.0.0.

Comment on attachment 77000 [details] [diff] [review] Patch v1 a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment on attachment 77000 [details] [diff] [review] Patch v1 This patch has been checked in. The issues fixed are better prompt dialog handling and the specific case of publishing to a directory that does not exist. You should see the aler dialog first, then after Ok there, the publish progress dialog should show the message "xxxx/ doesn't exist at this site"

I don't guarantee this fixes all of the issues in this bug but it does fix some regressions I am seeing this week.

Exactly the same fix as brade, with a few addtional changes: 1. Turn off debug flags to show debug dump 2. Add dump of status message after checking for HTTP error 3. In publish progress: change default string to "PublishCompleted". It will be changed to one of 2 error messages whenever a non-zero aStatus value is passed into SetProgressFinished()

Oops! This is the change to showdebug flags I mentioned: @@ -822,9 +822,9 @@ return promptService; } -const gShowDebugOutputStateChange = true; +const gShowDebugOutputStateChange = false; const gShowDebugOutputProgress = false; -const gShowDebugOutputStatusChange = true; +const gShowDebugOutputStatusChange = false; const gShowDebugOutputLocationChange = false; const gShowDebugOutputSecurityChange = false;

Comment on attachment 77918 [details] [diff] [review] Patch v2 previous patch is simpler and actually works fine. Be sure to add changes to turn off debug flags.

Comment on attachment 77916 [details] [diff] [review] proposed patch r=cmanske, with addition of turning off debug flags in ComposerCommands.js

cc kin for sr= of small patch

Comment on attachment 77916 [details] [diff] [review] proposed patch sr=kin@netscape.com

Comment on attachment 77916 [details] [diff] [review] proposed patch a=asa (on behalf of drivers) for checkin to the 1.0 branch

resolving this bug as fixed (checked in on both branch and trunk) please verify this bug and all duplicates

still not fixed in 4/15 trunk build. see original steps.. when I enter invalid URL and hit puiblish, nothing happens. should get error/warning.

I am also still seeing the original problem, both on the 04-15 trunk, and the 04-13 1.0.0 branch build.

-->brade so it's on my radar and doesn't get lost (my apologies to Michael and Sujay for not checking that case as well)

Display error message if publish fails. I have tested these things: * successfully publish ftp html/images in subdirectory (success) * publish to http yahoo (failure) * publish to peoplestage bogus subdirectory (failure) I am noticing that failure to publish (with this patch) is still causing the location to reset when it shouldn't (as well as for document to be not dirty). Should I investigate that in this bug or can we put that issue in a new bug?

Comment on attachment 79368 [details] [diff] [review] patch to fix original problem described in bug I don't think this patch is necessary. The error handling should be confined to the Pulbish dialog "Validate()" method and in the status monitoring during publishing and Publish Progress dialogs.

Comment on attachment 79368 [details] [diff] [review] patch to fix original problem described in bug I don't think this patch is necessary. The error handling should be confined to the Publish dialog "Validate()" method and in the status monitoring during publishing and Publish Progress dialogs.

I disagree, we need to handle the fix in Publish function. I'm testing a new patch...

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

Comment on attachment 79471 [details] [diff] [review] better patch to fix original bug description r=cmanske

kin, alecf: can one of you sr= the latest patch in this bug? It's very small (adding 3 lines of which two lines are just curly braces) :-)

Comment on attachment 79471 [details] [diff] [review] better patch to fix original bug description sr=kin@netscape.com

This is from bug 138157, we need to make sure we chekc this case also: "First time I tried I put "fpt://..." instead of "ftp://.." in the settings. That caused mozilla to simply do nothing - no warning or anything, just closed the dialog like nothing happened."

This has been fixed on the trunk. I'd like to land this tiny (and very safe) patch on the branch if it's not too late.

adt1.0.0+ (on ADT's behalf) for checkin to the 1.0 branch. Pls check this in to the 1.0 branch today, then add the fixed1.0.0 keyword.

verified in 4/19 trunk. Although I think we could have popped up a better dialog than "Publishing Failed!" when the user enters an invalid URL.

checked into branch

verified in 4/23 branch build.