Bug 80760 – WinLauncher should show loading errors

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Ashod Nakashian]

WinLauncher doesn't report loading errors which doesn't help trouble-shoot loading problems. Case in point, HTTPS loading fails if an appropriate certificate isn't available and WinLauncher fails to load the target page silently.

[Ashod Nakashian]

Created attachment 131180 [details]
Patch

[Ashod Nakashian]

Added CC list based on previous commits.

[Brent Fulgham]

Comment on attachment 131180 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131180&action=review

This change includes a lot of white-space modifications. While I agree with these changes, they cause the actual patch to be hard to review due to clutter in the ChangeLog and other areas.

Please generate a patch with the functional change, and a second patch (if you are so inclined), and I'll be happy to approve them separately.

> Tools/WinLauncher/WinLauncher.cpp:150
> +    ::MessageBoxW(0, (LPCWSTR)errorDescription, L"Error", MB_APPLMODAL | MB_OK);

This should be a C++ cast, even though much of the rest of the file still uses C-style casts.

[Brent Fulgham]

Thanks for suggesting these improvements and, more importantly, proposing a patch! I have a few minor nit-picks that I hope you'll consider.

Thanks!

[Ashod Nakashian]

Created attachment 131212 [details]
Patch

[Ashod Nakashian]

(In reply to comment #3)
> (From update of attachment 131180 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131180&action=review
> 
> This change includes a lot of white-space modifications. While I agree with these changes, they cause the actual patch to be hard to review due to clutter in the ChangeLog and other areas.

That makes total sense. I don't like to squash multiple unrelated changes in a single patch as well. It's my trusted merge tool that does the whitespace cleanup. Please see the functional changes patch, I'll send in another with the whitespaces only - cleanup is Good(tm).

[Ashod Nakashian]

Created attachment 131214 [details]
Patch (Functional)

[Ashod Nakashian]

Created attachment 131215 [details]
Patch (Whitespaces)

[Patrick R. Gansterer]

Comment on attachment 131214 [details]
Patch (Functional)

View in context: https://bugs.webkit.org/attachment.cgi?id=131214&action=review

> Tools/WinLauncher/WinLauncher.cpp:145
> +HRESULT WinLauncherWebHost::didFailProvisionalLoadWithError(IWebView *webView, IWebError *error)

unuesed parameter webView

> Tools/WinLauncher/WinLauncher.cpp:147
> +    BSTR frameURL = 0;

unused variable frameURL

> Tools/WinLauncher/WinLauncher.cpp:149
> +    HRESULT hr = error->localizedDescription(&errorDescription);

unused variable hr -> please handle a possible error

[Patrick R. Gansterer]

Comment on attachment 131215 [details]
Patch (Whitespaces)

View in context: https://bugs.webkit.org/attachment.cgi?id=131215&action=review

AFAIK we usually DO NOT commit any whitespace only changes -> unusally we remove it when we touch a line (+/- a few lines) which contains whitespace
whitspace is not nice, but does not hurt anybody during development, but it creates unneded chages which make digging in svn log more complicated

> Tools/WinLauncher/WinLauncher.cpp:145
> +HRESULT WinLauncherWebHost::didFailProvisionalLoadWithError(IWebView *webView, IWebError *error)

why do you still add this change? this and the other patch won't apply as two independed changes (IMHO that's what brent requested)

[Ashod Nakashian]

(In reply to comment #10)
> (From update of attachment 131215 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131215&action=review
> 
> AFAIK we usually DO NOT commit any whitespace only changes -> unusally we remove it when we touch a line (+/- a few lines) which contains whitespace
> whitspace is not nice, but does not hurt anybody during development, but it creates unneded chages which make digging in svn log more complicated

Fair enough. I'll limit changes to the lines I touch and nearby where reasonable.

> 
> > Tools/WinLauncher/WinLauncher.cpp:145
> > +HRESULT WinLauncherWebHost::didFailProvisionalLoadWithError(IWebView *webView, IWebError *error)
> 
> why do you still add this change? this and the other patch won't apply as two independed changes (IMHO that's what brent requested)

Because I assume if these two patches are applied after one another, they should contain incremental changes. May be I messed it up while generating the patch, but that was the rationale.

[Patrick R. Gansterer]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 131215 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=131215&action=review
> > 
> > > Tools/WinLauncher/WinLauncher.cpp:145
> > > +HRESULT WinLauncherWebHost::didFailProvisionalLoadWithError(IWebView *webView, IWebError *error)
> > 
> > why do you still add this change? this and the other patch won't apply as two independed changes (IMHO that's what brent requested)
> 
> Because I assume if these two patches are applied after one another, they should contain incremental changes. May be I messed it up while generating the patch, but that was the rationale.

If the first patch landed the file will contain this line. Then the second patch won't apply, since the line will exist already.

[Ashod Nakashian]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (From update of attachment 131215 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=131215&action=review
> > > 
> > > > Tools/WinLauncher/WinLauncher.cpp:145
> > > > +HRESULT WinLauncherWebHost::didFailProvisionalLoadWithError(IWebView *webView, IWebError *error)
> > > 
> > > why do you still add this change? this and the other patch won't apply as two independed changes (IMHO that's what brent requested)
> > 
> > Because I assume if these two patches are applied after one another, they should contain incremental changes. May be I messed it up while generating the patch, but that was the rationale.
> 
> If the first patch landed the file will contain this line. Then the second patch won't apply, since the line will exist already.

I see that now. Thanks. I'll update the patch with the fixes.

[Ashod Nakashian]

Created attachment 131244 [details]
Patch

[Patrick R. Gansterer]

Comment on attachment 131244 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131244&action=review

> Tools/WinLauncher/WinLauncher.cpp:149
> +    if (FAILED(hr))

is returning S_OK in this case "correct"?

> Tools/WinLauncher/WinLauncher.cpp:152
> +        ::MessageBoxW(0, (LPCWSTR)errorDescription, L"Error", MB_APPLMODAL | MB_OK);

please use static_cast

also can you merge the redundant code? here is it possible to do sth like if(FAILED) errorDescription = ""; MessageBoxW(.., errorDescription, ...)

[Ashod Nakashian]

(In reply to comment #15)
> (From update of attachment 131244 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131244&action=review
> 
> > Tools/WinLauncher/WinLauncher.cpp:149
> > +    if (FAILED(hr))
> 
> is returning S_OK in this case "correct"?

Yes it is, as the handler didn't fail, just an internal operation that doesn't affect the correctness of the handler itself (this may be more complicated in other cases, but in this case we're simply handling the page load failure to inform the user, not to take any further action that is subject to success or failure.)

> 
> > Tools/WinLauncher/WinLauncher.cpp:152
> > +        ::MessageBoxW(0, (LPCWSTR)errorDescription, L"Error", MB_APPLMODAL | MB_OK);
> 
> please use static_cast
> 
> also can you merge the redundant code? here is it possible to do sth like if(FAILED) errorDescription = ""; MessageBoxW(.., errorDescription, ...)

Both done. Updating patch.

[Ashod Nakashian]

Created attachment 131250 [details]
Patch

[Patrick R. Gansterer]

Comment on attachment 131250 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131250&action=review

> Tools/WinLauncher/WinLauncher.cpp:148
> +    HRESULT hr = error->localizedDescription(&errorDescription);

sorry for an additional dump question: is it possible that localizedDescription returns S_OK and sets errorDescription to NULL?

[Ashod Nakashian]

(In reply to comment #18)
> (From update of attachment 131250 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131250&action=review
> 
> > Tools/WinLauncher/WinLauncher.cpp:148
> > +    HRESULT hr = error->localizedDescription(&errorDescription);
> 
> sorry for an additional dump question: is it possible that localizedDescription returns S_OK and sets errorDescription to NULL?

No worries. The current implementation (WebError::localizedDescription) doesn't allow for that, but even if errorDescription is NULL, the user will only get a blank message-box, nothing fatal.

[Adam Roben (:aroben)]

Comment on attachment 131250 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131250&action=review

> 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?

> Tools/WinLauncher/WinLauncher.h:53
>      virtual HRESULT STDMETHODCALLTYPE didFailProvisionalLoadWithError( 
>          /* [in] */ IWebView *webView,
>          /* [in] */ IWebError *error,
> -        /* [in] */ IWebFrame *frame) { return S_OK; }
> +        /* [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.

[Ashod Nakashian]

(In reply to comment #20)
> (From update of attachment 131250 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131250&action=review
> 
> > 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).

> 
> > Tools/WinLauncher/WinLauncher.h:53
> >      virtual HRESULT STDMETHODCALLTYPE didFailProvisionalLoadWithError( 
> >          /* [in] */ IWebView *webView,
> >          /* [in] */ IWebError *error,
> > -        /* [in] */ IWebFrame *frame) { return S_OK; }
> > +        /* [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.

[Adam Roben (:aroben)]

Comment on attachment 131250 [details]
Patch

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.

[Ashod Nakashian]

Created attachment 131984 [details]
Patch

[WebKit Review Bot]

Attachment 131984 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/WinLauncher/WinL..." exit_code: 1
Tools/WinLauncher/WinLauncher.h:53:  The parameter name "frame" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Patrick R. Gansterer]

Comment on attachment 131984 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131984&action=review

> Tools/WinLauncher/WinLauncher.cpp:145
> +HRESULT STDMETHODCALLTYPE WinLauncherWebHost::didFailProvisionalLoadWithError(IWebView *webView, IWebError *error, IWebFrame *frame)

unused parameters

[Ashod Nakashian]

Created attachment 131998 [details]
Patch

[WebKit Review Bot]

Attachment 131998 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/WinLauncher/WinL..." exit_code: 1
Tools/WinLauncher/WinLauncher.h:53:  The parameter name "frame" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ashod Nakashian]

Created attachment 132055 [details]
Patch

Why parameters unused in the implementation should be removed from the declaration is inexplicable to me.

[Brent Fulgham]

(In reply to comment #28)
> Created an attachment (id=132055) [details]
> Patch
> 
> Why parameters unused in the implementation should be removed from the declaration is inexplicable to me.

The pragmatic reason is that it silences a compiler warning, making it easier to notice warnings that have real consequences.

The higher-level reason is that it signals to people reading the sources that the parameter is not used.

If you like, you can comment out the name of the parameter (e.g., "... double /*blah*/, int used, ...").

[Ashod Nakashian]

(In reply to comment #29)
> (In reply to comment #28)
> > Created an attachment (id=132055) [details] [details]
> > Patch
> > 
> > Why parameters unused in the implementation should be removed from the declaration is inexplicable to me.
> 
> The pragmatic reason is that it silences a compiler warning, making it easier to notice warnings that have real consequences.

That's only true for implementations, not for declarations (there is no 'usage' at declaration point).

> 
> The higher-level reason is that it signals to people reading the sources that the parameter is not used.

Reasonable, but I think this promises more than it warrants. For example an update might start using these parameters. I think the correct reference for what's used and what's not is the docs, which defines the contract, not the code, which is only an implementation, not the only possible one.

> 
> If you like, you can comment out the name of the parameter (e.g., "... double /*blah*/, int used, ...").

Thanks.

[Adam Roben (:aroben)]

(In reply to comment #28)
> Created an attachment (id=132055) [details]
> Patch
> 
> Why parameters unused in the implementation should be removed from the declaration is inexplicable to me.

This is explained here: <http://www.webkit.org/coding/coding-style.html#names-variable-name-in-function-decl>.

[Ashod Nakashian]

(In reply to comment #31)
> (In reply to comment #28)
> > Created an attachment (id=132055) [details] [details]
> > Patch
> > 
> > Why parameters unused in the implementation should be removed from the declaration is inexplicable to me.
> 
> This is explained here: <http://www.webkit.org/coding/coding-style.html#names-variable-name-in-function-decl>.

Thanks.

Can we sign-off this one? I think all issues are addressed and it's ready to land.

[Brent Fulgham]

Comment on attachment 132055 [details]
Patch

Looks good to me.

[WebKit Review Bot]

Comment on attachment 132055 [details]
Patch

Clearing flags on attachment: 132055

Committed r120133: <http://trac.webkit.org/changeset/120133>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.