Bug 80760 - WinLauncher should show loading errors
Summary: WinLauncher should show loading errors
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 80749
  Show dependency treegraph
 
Reported: 2012-03-10 10:35 PST by Ashod Nakashian
Modified: 2012-06-12 15:48 PDT (History)
6 users (show)

See Also:


Attachments
Patch (8.88 KB, patch)
2012-03-10 11:22 PST, Ashod Nakashian
no flags Details | Formatted Diff | Diff
Patch (2.34 KB, patch)
2012-03-11 03:07 PDT, Ashod Nakashian
no flags Details | Formatted Diff | Diff
Patch (Functional) (2.34 KB, patch)
2012-03-11 03:30 PDT, Ashod Nakashian
no flags Details | Formatted Diff | Diff
Patch (Whitespaces) (8.70 KB, patch)
2012-03-11 03:33 PDT, Ashod Nakashian
no flags Details | Formatted Diff | Diff
Patch (2.44 KB, patch)
2012-03-11 10:39 PDT, Ashod Nakashian
no flags Details | Formatted Diff | Diff
Patch (2.41 KB, patch)
2012-03-11 11:32 PDT, Ashod Nakashian
no flags Details | Formatted Diff | Diff
Patch (2.30 KB, patch)
2012-03-14 21:40 PDT, Ashod Nakashian
no flags Details | Formatted Diff | Diff
Patch (2.29 KB, patch)
2012-03-15 00:37 PDT, Ashod Nakashian
no flags Details | Formatted Diff | Diff
Patch (2.28 KB, patch)
2012-03-15 08:21 PDT, Ashod Nakashian
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ashod Nakashian 2012-03-10 10:35:04 PST
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.
Comment 1 Ashod Nakashian 2012-03-10 11:22:23 PST
Created attachment 131180 [details]
Patch
Comment 2 Ashod Nakashian 2012-03-10 11:24:47 PST
Added CC list based on previous commits.
Comment 3 Brent Fulgham 2012-03-10 21:45:33 PST
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.
Comment 4 Brent Fulgham 2012-03-10 21:47:31 PST
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!
Comment 5 Ashod Nakashian 2012-03-11 03:07:10 PDT
Created attachment 131212 [details]
Patch
Comment 6 Ashod Nakashian 2012-03-11 03:10:16 PDT
(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).
Comment 7 Ashod Nakashian 2012-03-11 03:30:06 PDT
Created attachment 131214 [details]
Patch (Functional)
Comment 8 Ashod Nakashian 2012-03-11 03:33:15 PDT
Created attachment 131215 [details]
Patch (Whitespaces)
Comment 9 Patrick R. Gansterer 2012-03-11 09:03:32 PDT
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
Comment 10 Patrick R. Gansterer 2012-03-11 09:09:09 PDT
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)
Comment 11 Ashod Nakashian 2012-03-11 09:39:49 PDT
(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.
Comment 12 Patrick R. Gansterer 2012-03-11 09:42:19 PDT
(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.
Comment 13 Ashod Nakashian 2012-03-11 09:45:23 PDT
(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.
Comment 14 Ashod Nakashian 2012-03-11 10:39:15 PDT
Created attachment 131244 [details]
Patch
Comment 15 Patrick R. Gansterer 2012-03-11 10:43:41 PDT
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, ...)
Comment 16 Ashod Nakashian 2012-03-11 10:58:43 PDT
(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.
Comment 17 Ashod Nakashian 2012-03-11 11:32:31 PDT
Created attachment 131250 [details]
Patch
Comment 18 Patrick R. Gansterer 2012-03-11 11:42:55 PDT
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?
Comment 19 Ashod Nakashian 2012-03-11 11:48:46 PDT
(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.
Comment 20 Adam Roben (:aroben) 2012-03-12 11:03:08 PDT
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.
Comment 21 Ashod Nakashian 2012-03-12 12:19:28 PDT
(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.
Comment 22 Adam Roben (:aroben) 2012-03-13 10:16:25 PDT
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.
Comment 23 Ashod Nakashian 2012-03-14 21:40:39 PDT
Created attachment 131984 [details]
Patch
Comment 24 WebKit Review Bot 2012-03-14 21:44:06 PDT
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.
Comment 25 Patrick R. Gansterer 2012-03-14 23:27:39 PDT
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
Comment 26 Ashod Nakashian 2012-03-15 00:37:57 PDT
Created attachment 131998 [details]
Patch
Comment 27 WebKit Review Bot 2012-03-15 00:39:33 PDT
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.
Comment 28 Ashod Nakashian 2012-03-15 08:21:06 PDT
Created attachment 132055 [details]
Patch

Why parameters unused in the implementation should be removed from the declaration is inexplicable to me.
Comment 29 Brent Fulgham 2012-03-15 09:56:12 PDT
(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, ...").
Comment 30 Ashod Nakashian 2012-03-15 23:57:14 PDT
(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.
Comment 31 Adam Roben (:aroben) 2012-03-20 12:18:41 PDT
(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>.
Comment 32 Ashod Nakashian 2012-03-20 20:56:39 PDT
(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.
Comment 33 Brent Fulgham 2012-06-12 15:37:54 PDT
Comment on attachment 132055 [details]
Patch

Looks good to me.
Comment 34 WebKit Review Bot 2012-06-12 15:48:13 PDT
Comment on attachment 132055 [details]
Patch

Clearing flags on attachment: 132055

Committed r120133: <http://trac.webkit.org/changeset/120133>
Comment 35 WebKit Review Bot 2012-06-12 15:48:19 PDT
All reviewed patches have been landed.  Closing bug.