407655 - Reorganize download context menus

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[Ed Lee :Mardak]

I know there's bug 403703, etc for making the menus match the latest spec. This bug just gets things ready to match the latest spec with what's already implemented (i.e., everything but "go to source" bug 403674).

Comment 1

[Ed Lee :Mardak]

Attachment: v1

Menu ordering per attachment 288692 [details] and bug 397655 comment #34. Note, there's a cancel for scanning.. perhaps we should handle that..

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

There are already filed bugs for each of these issues. Have a look at the blocking list of bug 403674. So how to proceed?

Comment 3

[Ed Lee :Mardak]

As mentioned in comment #0, this is just reorganizing the existing menu items before bug 403674. Doing this before bug 403674 will keep the "blame" around for at least bug 403674 instead of reorganizing afterwards.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 292373 [details] [diff] [review]
v1

>diff --git a/toolkit/mozapps/downloads/content/downloads.js > var gContextMenus = [
>   // DOWNLOAD_DOWNLOADING

Bug 403704

>   // DOWNLOAD_FINISHED

Bug 403703

>   // DOWNLOAD_FAILED
>   // DOWNLOAD_CANCELED

Bug 403709

>   // DOWNLOAD_PAUSED

Bug 403707

Only the following states are not covered by an already existing bug:

>   // DOWNLOAD_QUEUED
>   // DOWNLOAD_BLOCKED
>   // DOWNLOAD_SCANNING
>   // DOWNLOAD_DIRTY

Comment 5

[Ed Lee :Mardak]

The main reason why I didn't use those bugs is that those are to make the menus match the latest spec. This bug doesn't make it match the latest spec but gets it closer. stephend mentioned that those bugs are mainly to help keep things organized for testing.

Additionally, those bugs depend on bug 403674 which this bug blocks, but this bug would be blocking each of bug 403704, etc.. and you can't make that cycle in bugzilla.

Comment 6

[Ed Lee :Mardak]

Attachment: v1.1

Removed retry for blocked and dirty downloads (and their separators)

From bug 397655 comment #38..

in progress: 1-2 actions followed by a separator, open containing, separator,
go to download page, copy download link
queued: cancel
downloading: pause, cancel
paused: resume, cancel
scanning: cancel

not in progress: 0-2 actions followed by a separator, go to download page, copy
download link, separator, remove from list, clear list
finished: open, open containing
canceled: retry
failed: retry
blocked: <no action>
dirty: <no action>

Comment 7

[Ed Lee :Mardak]

Bug 392097 fixes inProgress to report scanning as a valid inProgress state.. Without that, the cancel menu item for scanning will just be gray.

Comment 8

[Shawn Wilsher :sdwilsh]

Comment on attachment 292446 [details] [diff] [review]
v1.1

>+  ["menuitem_copyLocation",
>+   "menuseparator",
>+   "menuitem_removeFromList",
>+   "menuitem_clearList"]
nit: let's format the blocks like so:
[
 "menuitem_*"
 ",menu*"
]
this way, adding anything ever won't mess up blame.

>-    <menuseparator id="menuseparator_copy_location"/>
>+    <menuseparator id="menuseparator"/>
why the change?

r=sdwilsh with these addressed.

Comment 9

[Shawn Wilsher :sdwilsh]

Also no cancel for downloads in the scanning state.

Comment 10

[Ed Lee :Mardak]

(In reply to comment #8)
> (From update of attachment 292446 [details] [diff] [review])
> >+  ["menuitem_copyLocation",
> >+   "menuseparator",
> >+   "menuitem_removeFromList",
> >+   "menuitem_clearList"]
> nit: let's format the blocks like so:
> [
>  "menuitem_*"
>  ",menu*"
> ]
> this way, adding anything ever won't mess up blame.
I'm guessing you meant..
var gCont = [
  // DOWN..
  [
    "menu.."
    , "menu.."
  ],
  ..
];
? See spacing and comma.. should the state separator comma be on the next group like..
[
  ""
]
, [
]
?

> >-    <menuseparator id="menuseparator_copy_location"/>
> >+    <menuseparator id="menuseparator"/>
> why the change?
Because otherwise I would have needed to add in more menu separators with different names. The menu items get cloned anyway, so there's no difference between the copy_location separator and other ones like actions and referrer separators.

Comment 11

[Shawn Wilsher :sdwilsh]

(In reply to comment #10)
> ? See spacing and comma.. should the state separator comma be on the next group
> like..
> [
>   ""
> ]
> , [
> ]
> ?
I wouldn't bother with that one.  Blame isn't so important in that case.

Comment 12

[Ed Lee :Mardak]

Attachment: v1.2

(In reply to comment #8)
> [
>  "menuitem_*"
>  ",menu*"
> ]
> this way, adding anything ever won't mess up blame.
Shuffled.. except the first item can still cause blame loss.

(In reply to comment #9)
> Also no cancel for downloads in the scanning state.
Removed (and the menuseparator)

r+ sdwilsh

Comment 13

[Ed Lee :Mardak]

No depends on bug 392097 because we don't have cancel for scanning.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 293093 [details] [diff] [review]
v1.2

a=beltzner for 1.9

Comment 15

[Ed Lee :Mardak]

Checking in toolkit/mozapps/downloads/content/downloads.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.js,v  <--  downloads.js
new revision: 1.111; previous revision: 1.110
done
Checking in toolkit/mozapps/downloads/content/downloads.xul;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.xul,v  <--  downloads.xul
new revision: 1.37; previous revision: 1.36
done

Comment 16

[Stephen Donner [:stephend] Not actively reading bugmail]

Since this bug and all dependent bugs are fixed, verified FIXED with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3pre) Gecko/2007121802
Minefield/3.0b3pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre)
Gecko/2007121801 Minefield/3.0b3pre

and Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2007121802
Minefield/3.0b3pre

I'll be updating Litmus soon to reflect the (final? :-P) revision.