[Webkit-unassigned] [Bug 80760] WinLauncher should show loading errors
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Mar 13 10:16:26 PDT 2012
https://bugs.webkit.org/show_bug.cgi?id=80760
--- Comment #22 from Adam Roben (:aroben) <aroben at webkit.org> 2012-03-13 10:16:25 PST ---
(From update of attachment 131250)
View in context: https://bugs.webkit.org/attachment.cgi?id=131250&action=review
> Tools/WinLauncher/WinLauncher.cpp:147
> + BSTR errorDescription = 0;
You're leaking this string. That at least definitely needs to be fixed before this patch lands.
>>> Tools/WinLauncher/WinLauncher.cpp:152
>>> + ::MessageBoxW(0, static_cast<LPCWSTR>(errorDescription), L"Error", MB_APPLMODAL | MB_OK);
>>
>> Using a message box seems awfully heavy-handed. Is there some less disruptive way of notifying the user?
>
> Well, as it stands page load errors don't even bother to notify the user at all. So between silent failure and a message box, I think the latter is a more welcome response (I know because I scratched my head until I figured that loading was failing - especially that during loading there is no indication of loading progress.)
>
> Now regarding alternatives I'm thinking a status bar would be good to have. But until we add a status bar (or some other notification method,) considering that page load failures are dead-ends, I find message boxes to be simple and very informative. Notice also that this is a test/demo project, it's not supposed to be elegant (not that it's a bad thing, but complexity defeats both goals).
OK, you've convinced me.
>>> Tools/WinLauncher/WinLauncher.h:53
>>> + /* [in] */ IWebFrame *frame) { return didFailProvisionalLoadWithError(webView, error); }
>>
>> I don't think the extra overload of didFailProvisionalLoadWithError is needed. I'd just move the implementation of this overload to the .cpp file and put the MessageBox code there.
>
> That sounds reasonable. However I didn't want to change existing code/conventions. I tend to be conservative and conformist when contributing to existing code. If there are no objections to my response of the previous issue, I suggest improving this (and the other similar cases) in a separate patch.
I don't think there's really a convention here. Only one other delegate function (didCommitLoadForFrame) actually does anything, and it calls a differently-named function (updateAddressBar). Moving the function definition to the .cpp file and getting rid of the extra overload would match the conventions used throughout the WebKit codebase.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list