Last Comment Bug 768927 - Improve error message strings for problems hit when launching an app in the web runtime
: Improve error message strings for problems hit when launching an app in the w...
Status: VERIFIED FIXED
[qa!]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Web Apps (show other bugs)
: unspecified
: All Mac OS X
: P2 normal
: Firefox 16
Assigned To: Bill Walker [:bwalker] [@wfwalker]
: Jason Smith [:jsmith]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-27 09:14 PDT by Jason Smith [:jsmith]
Modified: 2016-02-04 15:00 PST (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed patch (9.00 KB, patch)
2012-07-03 17:03 PDT, Bill Walker [:bwalker] [@wfwalker]
myk: feedback+
Details | Diff | Review
proposed patch, v2 (9.00 KB, patch)
2012-07-05 10:28 PDT, walker
myk: review+
Details | Diff | Review
proposed patch v3 (9.21 KB, patch)
2012-07-07 10:04 PDT, Bill Walker [:bwalker] [@wfwalker]
no flags Details | Diff | Review

Description Jason Smith [:jsmith] 2012-06-27 09:14:24 PDT
Steps:

1. Install the latest nightly
2. Install a web application
3. Uninstall nightly and ensure no version of firefox installed supports web apps on desktop
4. Launch the web application

Expected:

An error message similar to what is seen on Windows should be shown - "You must have FF 15 installed or higher to run web apps."

Actual:

A not-so-useful error message is shown below:

WebappRT Not Found

Failure to locate specified override runtime with signature 'org.mozilla.nightly'
Comment 1 Bill Walker [:bwalker] [@wfwalker] 2012-07-03 12:05:52 PDT
The message string is in mozilla-central/webapprt/mac/webapprt.mm

@throw MakeException(@"WebappRT Not Found", [NSString stringWithFormat:@"Failed to locate specified override runtime with signature '%@'", alternateBinaryID]);
Comment 2 Bill Walker [:bwalker] [@wfwalker] 2012-07-03 17:03:02 PDT
Created attachment 638915 [details] [diff] [review]
proposed patch

first cut at fixing error messages for both mac and win
Comment 3 Myk Melez [:myk] [@mykmelez] 2012-07-03 19:51:49 PDT
Comment on attachment 638915 [details] [diff] [review]
proposed patch

Review of attachment 638915 [details] [diff] [review]:
-----------------------------------------------------------------

A few grammar/spelling nits, but otherwise this looks good!

Note: I tend to avoid the term "web runtime", which devolves into "WebRT", which people confuse for "WebRTC", and instead use "webapp runtime" these days.  But it's not that big a deal.

::: webapprt/mac/webapprt.mm
@@ +114,5 @@
>      NSLog(@"USING FIREFOX : %@", firefoxPath);
>  
>      NSString *myWebRTPath = [myBundle pathForAuxiliaryExecutable: @"webapprt"];
>      if (!myWebRTPath) {
> +      @throw MakeException(@"Missing Web Runtime Files", @"Cannot locate binary for this application");

Nit: elsewhere you use "app" in place of "application".  It seems like this message should do so as well.

@@ +221,5 @@
>  
>          bool exists;
>          nsresult rv = rtINI->Exists(&exists);
>          if (NS_FAILED(rv) || !exists) {
> +          NSString* msg = [NSString stringWithFormat: @"This copy of Firefox (%@) cannot run web applications, because it is missing Web Runtime application.ini", firefoxVersion];

Perhaps replace "web applications" with "apps" here?

Also, while here: Web Runtime application.ini -> the Web Runtime's application.ini file.

@@ +350,5 @@
>      }
>    }
>  
>    NSLog(@"unable to find a valid webrt path");
> +  @throw MakeException(@"This app requires that Firefox version 15 or above is installed.", @"Firefox 15+ has not been detected.");

Nit: "app" elsewhere is capitalized, but here it's lowercase.  Seems like we should be consistent.
Comment 4 walker 2012-07-05 10:23:56 PDT
(In reply to Myk Melez [:myk] [@mykmelez] from comment #3)
> Comment on attachment 638915 [details] [diff] [review]
> proposed patch
> 
> Review of attachment 638915 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A few grammar/spelling nits, but otherwise this looks good!
> 
> Note: I tend to avoid the term "web runtime", which devolves into "WebRT",
> which people confuse for "WebRTC", and instead use "webapp runtime" these
> days.  But it's not that big a deal.

I checked with product management; Ragavan prefers to stick with "Web Runtime" as we've used it publicly in other places (cc'ing him)

> 
> ::: webapprt/mac/webapprt.mm
> @@ +114,5 @@
> >      NSLog(@"USING FIREFOX : %@", firefoxPath);
> >  
> >      NSString *myWebRTPath = [myBundle pathForAuxiliaryExecutable: @"webapprt"];
> >      if (!myWebRTPath) {
> > +      @throw MakeException(@"Missing Web Runtime Files", @"Cannot locate binary for this application");
> 
> Nit: elsewhere you use "app" in place of "application".  It seems like this
> message should do so as well.

Agreed. Will fix.



> 
> @@ +221,5 @@
> >  
> >          bool exists;
> >          nsresult rv = rtINI->Exists(&exists);
> >          if (NS_FAILED(rv) || !exists) {
> > +          NSString* msg = [NSString stringWithFormat: @"This copy of Firefox (%@) cannot run web applications, because it is missing Web Runtime application.ini", firefoxVersion];
> 
> Perhaps replace "web applications" with "apps" here?
> 
> Also, while here: Web Runtime application.ini -> the Web Runtime's
> application.ini file.

Agreed. Will fix.



> 
> @@ +350,5 @@
> >      }
> >    }
> >  
> >    NSLog(@"unable to find a valid webrt path");
> > +  @throw MakeException(@"This app requires that Firefox version 15 or above is installed.", @"Firefox 15+ has not been detected.");
> 
> Nit: "app" elsewhere is capitalized, but here it's lowercase.  Seems like we
> should be consistent.

Agreed. Will fix.
Comment 5 walker 2012-07-05 10:28:52 PDT
Created attachment 639389 [details] [diff] [review]
proposed patch, v2

new patch addressing concerns from myk's feedback
Comment 6 Myk Melez [:myk] [@mykmelez] 2012-07-05 13:48:02 PDT
Comment on attachment 639389 [details] [diff] [review]
proposed patch, v2

Nit: "app" is used as a common noun in these messages (unlike Web Runtime, which is mostly used as a proper noun), and thus it would be more grammatically correct to spell it all lowercase.
Comment 7 Myk Melez [:myk] [@mykmelez] 2012-07-06 18:26:29 PDT
Bill and I didn't expect this patch to cause failures, but he pushed it to TryServer just in case, where it came up roses (modulo the usual suspects):

https://tbpl.mozilla.org/?tree=Try&rev=88d6af2e5579

So this is ready to commit.  But Bill has used two email addresses in this bug, and this'll be his first patch committed, so I want to make sure I get the proper address into the commit record.

Bill: by which address would you like to be known in mozilla-central?
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-07-07 06:43:25 PDT
(In reply to Myk Melez [:myk] [@mykmelez] from comment #7)
> Bill: by which address would you like to be known in mozilla-central?

Probably shouldn't have checkin-needed on here if it's not ready for checkin yet.
Comment 9 Bill Walker [:bwalker] [@wfwalker] 2012-07-07 07:50:37 PDT
> Bill: by which address would you like to be known in mozilla-central?

I do want to be bwalker@mozilla.com in mozilla-central; references to walker@shout.net were a mistake.
Comment 10 Bill Walker [:bwalker] [@wfwalker] 2012-07-07 10:04:56 PDT
Created attachment 639967 [details] [diff] [review]
proposed patch v3

should be identical to "proposed patch v2" except now with proper user identifier
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-07-07 11:59:12 PDT
https://hg.mozilla.org/mozilla-central/rev/9533b40ff28b
Comment 13 Jason Smith [:jsmith] 2012-07-12 12:03:45 PDT
Tested on OS X 10.7. I'm getting the same error as I see in comment 0. Checking the diff of changes, it looks like the error messages were improved, but there might be another problem going on here that leads the error message in comment 0 to occur, rather than the nice error message about FF 15+ needed to be installed to occur. Do we want a different bug opened for this or should this bug be reopened?
Comment 14 Myk Melez [:myk] [@mykmelez] 2012-07-13 01:05:50 PDT
(In reply to Jason Smith [:jsmith] from comment #13)
> Tested on OS X 10.7. I'm getting the same error as I see in comment 0.

Those strings no longer exist in the codebase, so the errors you see are being generated by a stub executable from a nightly build before the changes landed.  To test this, you'll need to first install the app using the latest nightly.


> Do we want a different bug opened for this or should this bug be reopened?

If you really still see the old messages after installing an app with the latest nightly build, then the bug should be reopened.  But I'd do more testing first, as I don't see how it's possible for that to happen.
Comment 15 Jason Smith [:jsmith] 2012-07-13 07:41:52 PDT
(In reply to Myk Melez [:myk] [@mykmelez] from comment #14)
> (In reply to Jason Smith [:jsmith] from comment #13)
> > Tested on OS X 10.7. I'm getting the same error as I see in comment 0.
> 
> Those strings no longer exist in the codebase, so the errors you see are
> being generated by a stub executable from a nightly build before the changes
> landed.  To test this, you'll need to first install the app using the latest
> nightly.

What about this? This sounds like the error I'm getting, and it's part of this patch (named differently yes as you've said, but I think this what I'm getting):

Line 338 - 339 of webapprt.mm:
@throw MakeException(@"Web Runtime Not Found",
[NSString stringWithFormat:@"Failed to locate specified override Web Runtime with signature '%@'", alternateBinaryID]);

> 
> 
> > Do we want a different bug opened for this or should this bug be reopened?
> 
> If you really still see the old messages after installing an app with the
> latest nightly build, then the bug should be reopened.  But I'd do more
> testing first, as I don't see how it's possible for that to happen.

I'll take another look, but I'm expecting to get the same error, given that the patch is just making string changes, but the exception with the unuseful error message still exists on line 338 - 339 of webapprt.mm.
Comment 16 Myk Melez [:myk] [@mykmelez] 2012-07-13 09:48:16 PDT
(In reply to Jason Smith [:jsmith] from comment #15)
> I'll take another look, but I'm expecting to get the same error, given that
> the patch is just making string changes, but the exception with the unuseful
> error message still exists on line 338 - 339 of webapprt.mm.

If you're seeing the new message, then the behavior is as expected!  Nevertheless, it's certainly possible that the new message remains unuseful.  But in that case, we should file a new bug on the issue.
Comment 17 Jason Smith [:jsmith] 2012-07-13 13:24:44 PDT
(In reply to Myk Melez [:myk] [@mykmelez] from comment #16)
> (In reply to Jason Smith [:jsmith] from comment #15)
> > I'll take another look, but I'm expecting to get the same error, given that
> > the patch is just making string changes, but the exception with the unuseful
> > error message still exists on line 338 - 339 of webapprt.mm.
> 
> If you're seeing the new message, then the behavior is as expected! 
> Nevertheless, it's certainly possible that the new message remains unuseful.
> But in that case, we should file a new bug on the issue.

Tested and reproduces the behavior with Web Runtime Not Found error as I suspected.

I'll file it, morph this bug, and mark this as verified. I'd suggest in the future though to make sure that the patches for fixing bugs are truly focusing on fixing the problem stated in the bug. This patch though provides value in improving the strings for error messages I don't believe addressed the problem stated in the original bug (which was in regards to the WebRT Not Found issue, which still occurs).

Note You need to log in before you can comment on or make changes to this bug.