Closed Bug 1034114 Opened 10 years ago Closed 10 years ago

WebApp startup slowed significantly by debug server

Categories

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P2)

All
Android
defect

Tracking

(fennec+)

RESOLVED FIXED
Firefox 35
Tracking Status
fennec + ---

People

(Reporter: snorp, Assigned: esawin)

References

Details

(Whiteboard: [WebRuntime])

Attachments

(1 file, 3 obsolete files)

It is known that starting the remote debugging server is slow (bug 1020053). Right now we start that for webapps when it's running in a debuggable build of Firefox. It makes webapp startup suck.

Instead of starting the debugger every time, I suggest showing a notification that debugging is available. Swiping it away would simply dismiss the notification, and activating it would start the debug server. We could get an immediate 1s (!) improvement in startup time on non-release builds.
tracking-fennec: --- → ?
tracking-fennec: ? → +
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #0)
> Instead of starting the debugger every time, I suggest showing a
> notification that debugging is available. Swiping it away would simply
> dismiss the notification, and activating it would start the debug server. We
> could get an immediate 1s (!) improvement in startup time on non-release
> builds.

That sounds reasonable to me!
Priority: -- → P2
Whiteboard: [WebRuntime]
Assignee: nobody → esawin
Having a debug Fennec build does not start the remote debugger for web apps with ApplicationInfo.FLAG_DEBUGGABLE set to false (see http://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/WebappRT.js#69).

So essentially that means that we don't get the debugger startup delay (400ms on Nexus 7) for regular web apps installed via the marketplace. The remote debugger would automatically start for web apps installed directly via an APK, which is most likely what the developer wants in this case anyways and adding another step to active the debugger after each APK update could be bothersome.

What do you think?
Flags: needinfo?(snorp)
As discussed on IRC, we're still going to provide a notification to start the remote debugger as planned, even though only apps that are directly installed via APK (or enabled for debugging) are currently affected by the startup delay.
Flags: needinfo?(snorp)
Attached patch WIP (obsolete) — Splinter Review
It looks like we have a fundamental problem with notifications handling from webapps. Instead of triggering the registered onClick or onCancel handlers, we launch/switch to Fennec and lose the notifications mapping.
Wes, do you have any hints on the issue described in my previous comment, before I start diving into it?
Flags: needinfo?(wjohnston)
I've been trying to mark notifications using implicit intents.

i.e. intent.setClass(context, WebApp0.class);

Shouldn't be too hard. Thanks for looking at :)
Flags: needinfo?(wjohnston)
Attached patch Remote debugger notification (obsolete) — Splinter Review
Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=241d35170036
Attachment #8492441 - Attachment is obsolete: true
Attachment #8494575 - Flags: review?(wjohnston)
Attached patch Webapp notification context (obsolete) — Splinter Review
I split the patch since this is a general issue not related to the bug, we probably should put that under a new bug number.
Attachment #8494576 - Flags: review?(wjohnston)
Attachment #8494576 - Flags: review?(wjohnston) → feedback?(wjohnston)
Comment on attachment 8494576 [details] [diff] [review]
Webapp notification context

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

::: mobile/android/base/GeckoAppShell.java
@@ +1073,5 @@
>          return true;
>      }
>  
> +    public static void setWebappIndex(int index) {
> +        sWebappIndex = index; 

whitespace

::: mobile/android/base/NotificationHelper.java
@@ +197,5 @@
>          notificationIntent.putExtra(COOKIE_ATTR, message.optString(COOKIE_ATTR));
> +        
> +        int index = GeckoAppShell.getWebappIndex();
> +        String id = "org.mozilla.gecko.webapp.Webapps$Webapp" + index;
> +        Log.e(LOGTAG, "esawin # id " + id);

I would add a getWebAppClass[Name]() maybe?
Attachment #8494575 - Flags: review?(wjohnston) → review?(snorp)
Attachment #8494576 - Flags: feedback?(wjohnston)
Comment on attachment 8494575 [details] [diff] [review]
Remote debugger notification

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

Looks good, not sure about the warning icon though.

::: mobile/android/chrome/content/WebappRT.js
@@ +71,5 @@
>        if (response.isDebuggable) {
> +        Notifications.create({
> +          title: Strings.browser.formatStringFromName("remoteStartNotificationTitle", [name], 1),
> +          message: Strings.browser.GetStringFromName("remoteStartNotificationMessage"),
> +          icon: "drawable://warning_doorhanger",

Do we really want a warning?
Attachment #8494575 - Flags: review?(snorp) → review+
Depends on: 1072639
Status: NEW → ASSIGNED
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #10)
> Comment on attachment 8494575 [details] [diff] [review]
> Remote debugger notification
> 
> Review of attachment 8494575 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, not sure about the warning icon though.
> 
> ::: mobile/android/chrome/content/WebappRT.js
> @@ +71,5 @@
> >        if (response.isDebuggable) {
> > +        Notifications.create({
> > +          title: Strings.browser.formatStringFromName("remoteStartNotificationTitle", [name], 1),
> > +          message: Strings.browser.GetStringFromName("remoteStartNotificationMessage"),
> > +          icon: "drawable://warning_doorhanger",
> 
> Do we really want a warning?

That's what we currently show for the "remote debugging activated" notification. The general info icon is the Firefox symbol, so that wouldn't work well here. A dedicated debugger icon would be nice.
Comment on attachment 8494576 [details] [diff] [review]
Webapp notification context

Moved to bug 1072639.
Attachment #8494576 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/88968ae38b60
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [WebRuntime][fixed-in-fx-team] → [WebRuntime]
Target Milestone: --- → Firefox 35
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.