Bug 7171 – No description in WebKitErrors.m for WebKitErrorPlugInWillHandleLoad

• Component: WebKit Misc.
• Status: VERIFIED FIXED

[David Kilzer (:ddkilzer)]

While fixing Bug 7145, I noticed that there was no description in WebKitErrors.m for WebKitErrorPlugInWillHandleLoad (as defined in WebKitErrorsPrivate.h).  Per discussion with xenon on IRC, this should have been added.

WebKitErrorPlugInWillHandleLoad was added to WebkitErrorsPrivate.h in r10537 from the anonsvn repository.

[David Kilzer (:ddkilzer)]

Created attachment 6380 [details]
Patch v1

Trivial patch to add the description and dictionary entry to WebKitErrors.m.

I'm assuming there may be a different description that you want to use.  :)

[Darin Adler]

Comment on attachment 6380 [details]
Patch v1

I think John Sullivan should look this one over. Is this really the string we want? When is it displayed? We may want to improve the descriptive messages for localizers for all of those errors to help make it clear how to see them in, say, the Safari UI.

[John Sullivan]

Comment on attachment 6380 [details]
Patch v1

This particular error code is an unusual one in that it doesn't really signify an error, but the rest of the code behaves as if an error occurred. Currently Safari goes out of its way to show no error text at all for this case. But this still isn't quite right, because the status bar and Activity window still count the error, so you see "1 error" when the load is actually successful.

This broader problem was in Radar, and I've just added it to Bugzilla as bug 7176.

These descriptions generally appear in Safari in the Activity window (but this one is special-cased to not appear, as I said). They also appear in Safari in the page that's displayed when a URL can't be reached, if Safari has not supplied a specific message for that error number. In this particular case, I believe that WebKitErrorDescriptionPlugInWillHandleLoad doesn't occur for main pages, so the error description would never appear in the latter context.

So the new error description here would never appear in Safari. However, it might appear in other WebKit clients. Depending on whether and how 7176 is eventually addressed, this error code might be eliminated. However, in the meantime I see no harm in providing a string here, for the benefit of clients other than Safari. Brevity really pays off for these messages since they might be displayed in a context (e.g. Activity window) where space is tight, so I'd suggest "Plug-in handled load" rather than "Plug-in will handle load". I'm not that excited about using "load" here as a noun, but I can't think of anything obviously better.

I see Darin's point that the localizer comments could be improved. Currently they are all of the form "WebKitErrorXXX description". They would be better if they clarified when this type of error occurred, but only if this can be done briefly and without just repeating the words that are already part of the error name. I don't think doing that is necessary for committing this bug fix.

Also, since this modifies localized strings, you need to run update-webkit-localizable-strings and patch Localizable.strings as well as this file. 

So review- for now just to get the shorter string and to run update-webkit-localizable-strings.

[John Sullivan]

Ignore the part about update-webkit-localizable-strings. Until that procedure is documented, the person checking in the fix will have to worry about this step. So as long as you're happy with the shorter string I suggest here, I'll change this to review+ and check it in. Let me know.

[David Kilzer (:ddkilzer)]

Created attachment 6412 [details]
Patch v2

Updates from Patch v1:

- Shortened UI_String for WebKitErrorDescriptionPlugInWillHandleLoad.
- Ran update-webkit-localizable-strings before preparing ChangeLog (although because the file is considered binary, it's not included in the patch).

Don't forget to run update-webkit-localizable-strings before committing! :)

[John Sullivan]

Comment on attachment 6412 [details]
Patch v2

I'll check this in (and update localizable strings).

[David Kilzer (:ddkilzer)]

Verified by visual inspection of the code.