815202 - Add Pause and Cancel actions to download notifications

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Download Manager, defect)

Description

[Wesley Johnston (:wesj)]

Starting with JB, we can (easily) add actions to notifications. Right now if you tap a download notification we automatically cancel it. It might be nice to add an explicit button for that, along with an explicit Pause option.

Would provide some nice platform integration.

Comment 1

[Aaron Nguyen]

Hello, I'm looking for an issue to work on for my Software Engineering class at school and was wondering if my partner and I can help with this one?  We are very proficient in java and this looks like a perfect issue for us. Thank you for the consideration!

Comment 2

[Aaron Train [:aaronmt]]

Feel free to take it as it's currently unassigned. I suggest joining #mobile on irc.mozilla.org (IRC) to ask us any questions. Wes  is currently the mentor for this bug.

Comment 3

[Wesley Johnston (:wesj)]

To do this guy, we currently use the nsIAlertsService for these notifications. I don't really want to overload it with more of this, so instead I'd like to implement a new NativeWindow api. Something like:

var id = NativeWindow.notifications.show(aIcon, aTitle, aMsg, aOptions);
NativeWindow.notifications.hide(id);

aOptions could include things like progress, buttons, and callbacks for when the notification is clicked. i.e. This will be a bit of work to get up and running.

Comment 4

[Aaron Nguyen]

I just got the source and will be looking into it. I'll be sure to join the irc channel as well.

Comment 5

[Aaron Nguyen]

Attachment: Partial Patch Setting up the Notification with Buttons (WIP)

Takes place of the original Toast:show and sets up a 4.1+ android notification with buttons (function not implemented yet). Work In Progress. 

Build failed with errors.
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
11 errors
make[6]: *** [jars/gecko-browser.jar] Error 1
make[6]: Leaving directory `/home/fire/src/objdir-droid/mobile/android/base'
make[5]: *** [libs] Error 2
make[5]: Leaving directory `/home/fire/src/objdir-droid/mobile/android'
make[4]: *** [libs_tier_app] Error 2
make[4]: Leaving directory `/home/fire/src/objdir-droid'
make[3]: *** [tier_app] Error 2
make[3]: Leaving directory `/home/fire/src/objdir-droid'
make[2]: *** [default] Error 2
make[2]: Leaving directory `/home/fire/src/objdir-droid'
make[1]: *** [realbuild] Error 2
make[1]: Leaving directory `/home/fire/src'
make: *** [build] Error 2

We believe its the version of android that we are using for fennec and the items that aren't found are the default ICS button objects, and other objects. Would you like me to post the builderrors log that we have? Just let me know.

Thanks

Comment 6

[Josh Matthews [:jdm]]

Comment on attachment 718228 [details] [diff] [review]
Partial Patch Setting up the Notification with Buttons (WIP)

This apparently got lost in the shuffle.

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 718228 [details] [diff] [review]
Partial Patch Setting up the Notification with Buttons (WIP)

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

Seems like a good first start, but this bug is a bit more complex than I thought maybe. I wrote out some thoughts and general ideas below. I'd be happy to help write some of this to get things moving, but it'd be good to see what mfinkle thinks about having something like this around... Would you rather move this into the XPCOM AlertsService (and keep expanding it like we did for progress). Lets ping him.

::: mobile/android/base/GeckoApp.java
@@ +755,5 @@
>              if (event.equals("Toast:Show")) {
>                  final String msg = message.getString("message");
>                  final String duration = message.getString("duration");
>                  handleShowToast(msg, duration);
> +            } else if (event.equals("ToastFennec:Show")) {

Hmm... So what we really want here are notifications, not toasts, but that's nomenclature here.

@@ +757,5 @@
>                  final String duration = message.getString("duration");
>                  handleShowToast(msg, duration);
> +            } else if (event.equals("ToastFennec:Show")) {
> +                //Figure what information the downloader needs to execute the download
> +                //Activate the download

This code should probably not know anything about what the toast is (i.e. downloads).

@@ +1096,5 @@
>          });
>      }
>  
> +    void handleShowToastFennec() {
> +        //Notification code goes here.

I'm going to recommend we move this into the NotificationClient class to save having so much code in GeckoApp. I wonder if we can just add an addFromJSON(JSONObject obj) method there that would pull apart the message. You can use a GeckoAppShell.sNotificationCLient from this code (tbh, that object should be lazily created instead of created in onCreate...)

That, in turn, can push the data to NotificationHandler, along with a String array for buttons.

@@ +1107,5 @@
> +                .setContentText("Download Bar? ")
> +                //set icon to firefox
> +                .setSmallIcon(R.drawable.icon)
> +                .setContentIntent(pendingIntent)
> +                .addAction(R.drawable.ic_media_pause, "Pause", pendingIntent)

We'll want to add some data to each pendingIntent we pass here (probably pendingIntent.clone().putExtra("Button", 1) so that we can tell what happened when you click. We should also sue the NotificationCompat stuff here. It will fall back gracefully on older versions for us:

http://developer.android.com/reference/android/support/v4/app/NotificationCompat.Builder.html

::: mobile/android/chrome/content/browser.js
@@ +1346,5 @@
> +        message: aMessage,
> +        duration: aDuration
> +      });
> +    }
> +  },

I'd recommend we pull all this out into its own js file too. Maybe named NotificationBuilder.js? Then we can do something Androidy like:

var guid = NotificationBuilder.create()
                              .setTitle("title")
                              .setMessage("message")
                              .
                              .addAction("Pause")
                              .addAction("Cancel")
                              .show();

NotificationBuilder.get(guid)
                   .setProgress(cur/total)
                   .update();

Android has a crazy rich set of stuff we can do here (and would save us some of our current work with progressbars too) I'd love to expose it to JS/Addons without having to write an XPCOM service for it. But as a start, I'd just make sure we can show a title, large and small icons, a message, and a progressbar.

And then we can add these buttons!

Comment 8

[Marius Cristian Solomon]

If work on this issue has stalled, I'd like to start working on this as my first contribution to Fennec.

Comment 9

[:Margaret Leibovic]

(In reply to Marius Cristian Solomon from comment #8)
> If work on this issue has stalled, I'd like to start working on this as my
> first contribution to Fennec.

There hasn't been activity on this patch for a while, so it's probably safe to pick it up and start working on it. However, it looks like it may be a tricky first bug, so it may require a lot of work :)

Aaron, do you intend to finish this patch, or do you want to let Marius take over here?

We should also hear what mfinkle has to say in response to wesj's comment. I haven't spent much time in this code, but I think it would be nice to start with something simple, then decide whether or not to make it part of the AlertsService later.

Comment 10

[Aaron Nguyen]

Hi Margaret, it's fine by me to hand it over to Marius. I have had little time to work on this patch and would probably be for the best.

Hey Marius, here is a link to a conversation with wesj about how he wanted to handle this feature: http://pastebin.mozilla.org/2369623 . some of the links to the code are off due to changes but hope this helps.

Comment 11

[Marius Cristian Solomon]

Thanks Margaret and Aaron. I'll post updates as I make progress.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

I'd be OK with creating a NativeWindow.Notification system

Comment 13

[Federico Paolinelli]

Hi there, can I grab this one or is Marius still working on it?

Comment 14

[:Margaret Leibovic]

(In reply to Federico Paolinelli from comment #13)
> Hi there, can I grab this one or is Marius still working on it?

There hasn't been any activity here in a while, so it's probably okay to take it, but let's ask Marius if he still wants to try working on this.

Comment 15

[Federico Paolinelli]

Any news about this? Can I take the bug?

Comment 16

[:Margaret Leibovic]

(In reply to Federico Paolinelli from comment #15)
> Any news about this? Can I take the bug?

Yeah, enough time has passed that you should just go ahead and work on this.

Comment 17

[Wesley Johnston (:wesj)]

I really want to do this by writing an extended notifications api. Something similar to the Android builder interface:

http://developer.android.com/reference/android/support/v4/app/NotificationCompat.Builder.html

maybe with a hint of the chrome extension api thrown in (actually, the Chrome api covers most of what we'd want to do I think...):

http://developer.chrome.com/dev/extensions/notifications.html

I started this a little in WebrtcUI.js and NotificationsHelper.java, but I'd really like to see it made into a real .jsm file like Prompt.jsm:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/Prompt.jsm

and accessible to addons.

Comment 18

[Federico Paolinelli]

Attachment: First prototype

There is some commented out code because I am trying to figure out how to make the pending intents work through broadcasts.

At the moment the pending intents are firing the activity which has the effect to minimize the notification.

It's a decent first prototype anyway, there is enough material to receive feedback..

Comment 19

[Federico Paolinelli]

Attachment: Second version

Moved the pending intent from being an Intent directed to the GeckoApp activity to a broadcast intercepted by a broadcast receiver installed in the helper. This way fennec does not get brought on front when pressing pause / resume

Comment 20

[Wesley Johnston (:wesj)]

Comment on attachment 786372 [details] [diff] [review]
Second version

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

Still looking, but some comments.

::: mobile/android/base/NotificationHelper.java
@@ +34,5 @@
> +
> +    // Attributes that can be used while sending a notification from js.
> +    private static final String NOTIFICATION_ATTRIBUTE_TITLE = "title";
> +    private static final String NOTIFICATION_ATTRIBUTE_TEXT = "text";
> +    private static final String NOTIFICATION_ATTRIBUTE_ID = "id";

I think title, text, id, and smallIcon are all required. I wonder if we should put them in a special section here.

@@ +56,2 @@
>  
>      public NotificationHelper(Context context) {

Not 100% related, but I'd like to make this a singleton, and not require GeckoApp to hold on to it (that should also help if the activity is killed/restored).

@@ +83,5 @@
> +    }
> +
> +    private void registerReceiver(Context context) {
> +        IntentFilter filter = new IntentFilter(HELPER_BROADCAST_ACTION);
> +        filter.addDataScheme("alert");

You should add a comment about why we need this data scheme.

@@ +110,5 @@
> +
> +        // In case the user swiped out the notification, we empty the id
> +        // set.
> +        if ("delete".equals(notificationType)) {
> +            mShowing.remove(id);

We should notify Javascript about this if we can too (maybe only if Gecko is running, although I think this broadcast receiver dies when Gecko dies anyway...)

@@ +125,5 @@
> +            JSONObject args = new JSONObject();
> +            try {
> +                args.put(NOTIFICATION_ID, id);
> +                if (cookie != null) {
> +                    args.put(NOTIFICATION_ATTRIBUTE_COOKIE, data.getQueryParameter(NOTIFICATION_ATTRIBUTE_COOKIE));

You don't need to get this again, right? You can just reuse the string you have.

@@ +166,5 @@
> +            final String id = message.getString(NOTIFICATION_ATTRIBUTE_ID);
> +            final long progress = message.getLong(NOTIFICATION_ATTRIBUTE_PROGRESS_VALUE);
> +            final long progressMax = message.getLong(NOTIFICATION_ATTRIBUTE_PROGRESS_MAX);
> +            final String text = message.getString(NOTIFICATION_ATTRIBUTE_TEXT);
> +            GeckoAppShell.sNotificationClient.update(id.hashCode(), progress, progressMax, text);

Why are we going through the client here? This worries me a bit because I think, unless you pass a Notification directly to NotificationHandler, we'll create an AlertNotification which uses a remote view for progress and things. I created a new version of add for this originally as well:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/NotificationClient.java#93

Then again, I don't see anyone sending Notification:UpdateProgress messages anyway...

@@ +233,5 @@
> +                    JSONObject action = actions.getJSONObject(i);
> +                    final PendingIntent pending = buildNotificationPendingIntent(message, action.getString(NOTIFICATION_ATTRIBUTE_ACTION));
> +                    final String actionTitle = action.getString(NOTIFICATION_ATTRIBUTE_ACTION_TITLE);
> +                    final Uri actionImage = Uri.parse(message.optString(NOTIFICATION_ATTRIBUTE_ACTION_ICON));
> +                    builder.addAction(BitmapUtils.getResource(actionImage, R.drawable.ic_status_logo),

I think we should probably just ignore the icon if its not specified.

::: mobile/android/chrome/content/downloads.js
@@ +12,5 @@
>  const URI_GENERIC_ICON_DOWNLOAD = "drawable://alert_download";
>  
> +const PAUSE_ACTION = {
> +  actionKind : "pause",
> +  title : Strings.browser.GetStringFromName("alertDownloadsPause"),

I wonder if defining these at startup has any startup time implication.

@@ +49,5 @@
>      this._initialized = true;
>  
>      // Monitor downloads and display alerts
>      this._dlmgr = Cc["@mozilla.org/download-manager;1"].getService(Ci.nsIDownloadManager);
> +    this._bridge = Cc["@mozilla.org/android/bridge;1"].getService(Ci.nsIAndroidBridge);

We'd usually make this a lazy loaded thing.

get _bridge() {
  delete this._bridge;
  return this._bridge = stuff...
}

@@ +122,5 @@
> +  handleNotificationEvent: function dl_handleNotificationEvent(aNotifData, aDownload) {
> +    switch (aNotifData.kind) {
> +      case "click": {
> +        // Use this flag to make sure we only show one prompt at a time
> +        let cancelPrompt = false;

I don't think you want to initialize this here....

@@ +204,5 @@
>        let availableSpace = aDownload.targetFile.diskSpaceAvailable;
>      } catch(ex) { }
>      let contentLength = aDownload.size;
>      if (availableSpace > 0 && contentLength > 0 && contentLength > availableSpace) {
> +      Downloads.showAlert(aDownload, strings.GetStringFromName("lertDownloadsNoSpace"),

Dropped a letter here. Also we should probably just rename Downloads.showAlert to Downloads.showNotification. That way English speakers like me can have life even easier.

Comment 21

[Federico Paolinelli]

(In reply to Wesley Johnston (:wesj) from comment #20)

> @@ +233,5 @@
> > +                    JSONObject action = actions.getJSONObject(i);
> > +                    final PendingIntent pending = buildNotificationPendingIntent(message, action.getString(NOTIFICATION_ATTRIBUTE_ACTION));
> > +                    final String actionTitle = action.getString(NOTIFICATION_ATTRIBUTE_ACTION_TITLE);
> > +                    final Uri actionImage = Uri.parse(message.optString(NOTIFICATION_ATTRIBUTE_ACTION_ICON));
> > +                    builder.addAction(BitmapUtils.getResource(actionImage, R.drawable.ic_status_logo),
> 
> I think we should probably just ignore the icon if its not specified.
> 

I guess we have to choose here, between not showing the action or showing it with a default icon. I'd prefer showing the action anyway, but both cases are fallbacks for a wrong usage of the api. I am leaving as it is at the moment.

Comment 22

[Federico Paolinelli]

Attachment: bugfix-815202

Comment 23

[bennymartin]

I do have some knowledge in this language. i really, find confident in fixing this bug.

Comment 24

[Wesley Johnston (:wesj)]

(In reply to bennymartin from comment #23)
> I do have some knowledge in this language. i really, find confident in
> fixing this bug.

Hey, thanks for looking. Federico already has a mostly complete patch for this bug, but we have java things to work on at http://www.joshmatthews.net/bugsahoy/?mobile=1&java=1 (you'll have to manually see if anyone is assigned yet and if they're actively working on it). We have plenty of things that need done and need to add more to that list.

Comment 25

[Wesley Johnston (:wesj)]

Comment on attachment 787179 [details] [diff] [review]
bugfix-815202

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

I like this a lot. These are almost all nits.

I still have problems with this on my S3 (Samsung insists on hiding the buttons by default, and hides them again every time the progress updates). I think that's an S3 bug TBH, and since they're hidden by default, most users probably won't even know. I'm not sure what the right soluion is.

Let's needinfo ian for some button icons (required by Android), and I threw a build up for him at:
http://people.mozilla.com/~wjohnston/notification.apk

::: mobile/android/base/NotificationHelper.java
@@ +49,5 @@
> +    private static final String NOTIFICATION_ATTRIBUTE_EVENT_KIND = "kind";
> +    private static final String NOTIFICATION_ATTRIBUTE_ACTIONS = "actions";
> +    private static final String NOTIFICATION_ATTRIBUTE_ACTION = "actionKind";
> +    private static final String NOTIFICATION_ATTRIBUTE_ACTION_TITLE = "title";
> +    private static final String NOTIFICATION_ATTRIBUTE_ACTION_ICON = "icon";

We never use this many constants for JSON. I'm not sure if I like it or not. Having huge contants written out everywhere feels... like a lot of words. But I'm willing to give it a try in this file. Maybe we can shorten these a little?

PROGRESS_VALUE_MSG/JSON?
PROGRESS_MAX_MSG/JSON?
LIGHT_MSG/JSON?

@@ +68,5 @@
>          mContext = context;
>          mShowing = new HashSet<String>();
>          registerEventListener("Notification:Show");
>          registerEventListener("Notification:Hide");
> +        registerReceiver(context);

Do we need to do this all the time? Or would we be better to wait until we're showing a notification?

@@ +91,5 @@
> +
> +    private static void registerReceiver(Context context) {
> +        IntentFilter filter = new IntentFilter(HELPER_BROADCAST_ACTION);
> +        // Scheme is needed, otherwise only broadcast with no data will be catched.
> +        filter.addDataScheme("alert");

"alert" feels likely to collide at some point. Can we do something a bit more specific? "moz-notification" or something?

@@ +117,5 @@
> +        }
> +
> +        // In case the user swiped out the notification, we empty the id
> +        // set.
> +        if ("cleared".equals(notificationType)) {

Can we make this (and the "click") string constants?

@@ +148,5 @@
> +        final boolean ongoing = message.optBoolean(NOTIFICATION_ATTRIBUTE_ONGOING);
> +        notificationIntent.putExtra(NOTIFICATION_ATTRIBUTE_ONGOING, ongoing);
> +
> +        Uri.Builder b = new Uri.Builder();
> +        b.scheme("alert").appendQueryParameter(NOTIFICATION_ATTRIBUTE_EVENT_KIND, kind);

constant?

::: mobile/android/chrome/content/downloads.js
@@ +87,5 @@
> +        type: "Notification:Show",
> +        id: this.getNotificationIdFromDownload(aDownload),
> +        title: aTitle,
> +        smallIcon: URI_GENERIC_ICON_DOWNLOAD,
> +        text: aMessage,        

whitespace

@@ +118,5 @@
> +    this._bridge.handleGeckoMessage(JSON.stringify(msg));
> +  },
> +
> +  handleNotificationEvent: function dl_handleNotificationEvent(aNotifData, aDownload) {
> +    switch (aNotifData.kind) {

Lets name things something better than "kind". Maybe aNotifData.responseType?

@@ +136,5 @@
> +                                                     null, null, null, null, {});
> +          if (choice == 0)
> +            this.cancelDownload(aDownload);
> +          this._showingPrompt = false;
> +        }

Lets pull all "click" handling this out into its own function if we can, like the other guys below.

@@ +144,5 @@
> +        this.cancelDownload(aDownload);
> +      break;
> +      case "pause":
> +        aDownload.pause();
> +      break;

indent "break;" to match aDownload.pause();. same for others.

@@ +148,5 @@
> +      break;
> +      case "resume":
> +        aDownload.resume();
> +      break;
> +      case "cleared":

I wonder if we should name this "notification-cleared" to avoid confusion with buttons people might add. Same with the "click" message above. Also, maybe we should throw somewhere if people try to use these "reserved" names.

@@ +163,5 @@
> +        let guid = data.cookie;
> +        this._dlmgr.getDownloadByGUID(guid, (function(status, download) {
> +          if (Components.isSuccessCode(status)){
> +            this.handleNotificationEvent(data, download);
> +          }

No need for braces here.

@@ +171,5 @@
> +      case "last-pb-context-exited": {
> +        let download;
> +        while ((download = this._privateDownloads.pop())) {
> +          try {
> +            Downloads.removeNotification(download); 

trailing whitespace

Comment 26

[Wesley Johnston (:wesj)]

Also, there's a huge-ish screenshot of this at:

http://dl.dropboxusercontent.com/u/72157/buttons.png

Needs some nice pause/resume/cancel icons.

Comment 27

[Federico Paolinelli]

Attachment: bugfix-815202

Added the changes requested.

Comment 28

[Federico Paolinelli]

Attachment: bugfix-815202

- Fixed a small bug in icons url passing (I was getting the icon url from the main message instead of from the action sub-object)

- Tried to draw the (very very basic) icons for notifications buttons (pause, resume, cancel). The icons are 32 dip big and I made the three different versions for mdpi, hdpi, xhdpi. If needed, I can also provide the svg.

Since this is the first time I attach a binary to a patch, I hope I am not messing anything up.

Comment 29

[Federico Paolinelli]

Attachment: bugfix-815202

Just realized that I left a couple of "FEDE" debugging traces in the previous patch :P .

Comment 30

[Wesley Johnston (:wesj)]

Comment on attachment 801256 [details] [diff] [review]
bugfix-815202

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

I found a bug here with clicking on the notification while its paused that I think we need to fix, and maybe one with clicking the cancel button. I also tried to get some screenshots so ian can give this at least an "I'm ok with landing this and tweaking it" thumbs up/down.

fedepaol's icons: http://dl.dropboxusercontent.com/u/72157/buttons1.png
some stock icons: http://dl.dropboxusercontent.com/u/72157/buttons3.png

::: mobile/android/base/NotificationHelper.java
@@ +231,5 @@
> +                for (int i = 0; i < actions.length(); i++) {
> +                    JSONObject action = actions.getJSONObject(i);
> +                    final PendingIntent pending = buildNotificationPendingIntent(message, action.getString(ACTION_ATTR));
> +                    final String actionTitle = action.getString(ACTION_TITLE_ATTR);
> +                    final Uri actionImage = Uri.parse(action.optString(ACTION_ICON_ATTR));

Sorry I missed this :(

::: mobile/android/chrome/content/downloads.js
@@ +11,5 @@
>  
>  const URI_GENERIC_ICON_DOWNLOAD = "drawable://alert_download";
> +const URI_PAUSE_ICON = "drawable://ic_notif_button_pause";
> +const URI_CANCEL_ICON = "drawable://ic_notif_button_cancel";
> +const URI_RESUME_ICON = "drawable://ic_notif_button_resume";

I wonder if we're better to just use stock icons here for now?

@@ +119,5 @@
> +        id: this.getNotificationIdFromDownload(aDownload)
> +    };
> +    this._bridge.handleGeckoMessage(JSON.stringify(msg));
> +  },
> +  

ws, just because I can.

@@ +124,5 @@
> +  handleClickEvent: function dl_handleClickEvent(aDownload) {
> +    if (aDownload.state == Ci.nsIDownloadManager.DOWNLOAD_FINISHED) {
> +      // Only open the downloaded file if the download is complete
> +      this.openDownload(aDownload);
> +    } else if (aDownload.state == Ci.nsIDownloadManager.DOWNLOAD_DOWNLOADING && !this._showingPrompt) {

I think we may need to extend this to run if its paused now. Right now, pausing and clicking the notification just hides the download forever.

@@ +145,5 @@
> +      case "notification-clicked":
> +        this.handleClickEvent(aDownload);
> +        break;
> +      case "cancel":
> +        this.cancelDownload(aDownload);

I also think we may need to prompt here. Its easy to accidentally click on the wrong place, and just force killing the download feels wrong...

@@ +241,5 @@
>                              aDownload.displayName);
>          break;
> +      case Ci.nsIDownloadManager.DOWNLOAD_PAUSED:
> +        Downloads.showNotification(aDownload, aDownload.percentComplete + "%", 
> +                            aDownload.displayName, { percentComplete: aDownload.percentComplete, 

ws

Comment 31

[Federico Paolinelli]

Attachment: bugfix-815202

I fixed the click on paused bug. I also tried to show the confirm on cancel prompt while the cancel button was pressed, but the weird effect is that the prompt is being shown *under* the notification "curtain".
From the user's point of view, what happens is that he presses the button, nothing happens because the prompt is hidden, and when he returns to fennec he finds a prompt he might not remember of. In any case, if you prefer this other counter-intuitive approach, it's easy to implement since I moved the show prompt stuff in a separate function.

Comment 32

[Federico Paolinelli]

Attachment: bugfix-815202

Changes to resolve conflicts after update

Comment 33

[Wesley Johnston (:wesj)]

Comment on attachment 808780 [details] [diff] [review]
bugfix-815202

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

I think this looks good now. Anthony sent me some icons. I'm going to land it with them and we can move on from there :)

Comment 34

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/3181933b1780

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3181933b1780

Comment 36

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Is it possible to use the area left of the Pause/Resume button to widen both buttons (or reducing the margin of the icons)? At the moment, the German translation for the "Cancel" button ("Abbrechen") gets cropped.

Comment 37

[Aaron Train [:aaronmt]]

Please file a new bug.