Improve error message strings for problems hit when launching an app in the web runtime

VERIFIED FIXED in Firefox 16

Status

Firefox Graveyard
Web Apps
P2
normal
VERIFIED FIXED
5 years ago
a year ago

People

(Reporter: jsmith, Assigned: bwalker)

Tracking

unspecified
Firefox 16
All
Mac OS X

Details

(Whiteboard: [qa!])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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'
(Reporter)

Updated

5 years ago
Priority: -- → P2
(Assignee)

Updated

5 years ago
Assignee: nobody → bwalker
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 years ago
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]);
(Assignee)

Comment 2

5 years ago
Created attachment 638915 [details] [diff] [review]
proposed patch

first cut at fixing error messages for both mac and win
Attachment #638915 - Flags: feedback?(myk)
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.
Attachment #638915 - Flags: feedback?(myk) → feedback+

Comment 4

5 years ago
(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

5 years ago
Created attachment 639389 [details] [diff] [review]
proposed patch, v2

new patch addressing concerns from myk's feedback
Attachment #639389 - Flags: review?(myk)
Attachment #639389 - Attachment is patch: true
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.
Attachment #639389 - Flags: review?(myk) → review+
(Reporter)

Updated

5 years ago
QA Contact: jsmith
Keywords: checkin-needed
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?
(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.
Keywords: checkin-needed
(Assignee)

Comment 9

5 years ago
> 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.
(Assignee)

Comment 10

5 years ago
Created attachment 639967 [details] [diff] [review]
proposed patch v3

should be identical to "proposed patch v2" except now with proper user identifier
Attachment #638915 - Attachment is obsolete: true
Attachment #639389 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Whiteboard: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/9533b40ff28b
Whiteboard: checkin-needed
(Reporter)

Updated

5 years ago
Whiteboard: [qa+]
https://hg.mozilla.org/mozilla-central/rev/9533b40ff28b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
(Reporter)

Comment 13

5 years ago
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?
(Reporter)

Updated

5 years ago
Whiteboard: [qa+] → [qa verification failed]
(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.
(Reporter)

Comment 15

5 years ago
(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.
(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.
(Reporter)

Comment 17

5 years ago
(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).
Summary: Launching an app with an incompatible version of Firefox installed on Mac produces a very not so useful error message → Improve error message strings for problems hit when launching an app in the web runtime
(Reporter)

Updated

5 years ago
Status: RESOLVED → VERIFIED
Whiteboard: [qa verification failed] → [qa!]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.