403674 - Implement "Go to Download Page" for the context-menu items

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect, P1)

Description

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

Mockup here: http://people.mozilla.com/~madhava/files/downloadmanager/downloadmanager_2007-09-25_revision.png

Build ID: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2pre) Gecko/2007111304 Minefield/3.0b2pre

Summary: Implement "Go to Source Location" for the context-menu items

For the various states:
* In-progress
* Cancelled / Completed
* Paused

we need to implement "Go to Source Location" in the context-menu items; see the mockup for their intended positioning.

Comment 1

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

New mockup here: https://bugzilla.mozilla.org/attachment.cgi?id=288692

Comment 2

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

Adding dependencies due to all other 4 bugs are relying on the entity for "Go To Source Location".

Comment 3

[Srirang G Doddihal (Brahmana)]

I was thinking of working on this. Now that its a blocker I will take this up, unless somebody has already started.

This is my understanding of the task:
When the "Go to source location" context menu item is clicked, we need to display the referer page of the corresponding download in a new tab.

My questions:

AFAIK, the referrer information of a download was added recently (Mid-August this year). What needs to be done for downloads that do not have this info? Disable this option or display an alert or will it be No-op?

Comment 4

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

I already started with some ideas but feel free to take this. I would hook up with the depending context menu changes.

Shouldn't a download always have a valid URI? There shouldn't be a difference at it is already implemented for CopySourceLocation right?

http://mxr.mozilla.org/seamonkey/source/toolkit/mozapps/downloads/content/downloads.js#392

Comment 5

[Srirang G Doddihal (Brahmana)]

(In reply to comment #4)
> Shouldn't a download always have a valid URI? There shouldn't be a difference
> at it is already implemented for CopySourceLocation right?
> 
CopySourceLocation would copy the URI of the actual download file and thats what is needed. But for "Go to source location" I thought we should display the page that has the link to the actual download -- which is the referrer. Because if we "Go to" download link it will start downloading the file again instead, which essentially becomes repeating the download.

Correct me if I am wrong.
If I am not then my previous question of referrer being absent is still valid.

> http://mxr.mozilla.org/seamonkey/source/toolkit/mozapps/downloads/content/downloads.js#392
> 

Thank you for this link. I was looking for something like this.

Regards,
Brahmana

Comment 6

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

(In reply to comment #5)
> if we "Go to" download link it will start downloading the file again instead,
> which essentially becomes repeating the download.

You are right. So just use the referrer attribut instead?
http://mxr.mozilla.org/seamonkey/source/toolkit/mozapps/downloads/content/downloads.js#134

Comment 7

[Shawn Wilsher :sdwilsh]

Note that the referrer can be null.  Per the UI spec, you should then take the URI up until the file name and go there.

Comment 8

[Srirang G Doddihal (Brahmana)]

Attachment: JS and XUL files patch

This patch contains changes to the XUL and JS files. Please let me know if I have done something extremely stupid and have missed out several obvious things. This is my patch for the UI.

Regards,
Brahmana

Comment 9

[Srirang G Doddihal (Brahmana)]

Attachment: DTD changes

As the name goes this patch has the dtd entries for the new ENTITY for "Go To Source Location" menu item. The access key is "g" (g for Go).

I am not asking for a review for these patches as I am not sure what it still needs. I am just putting these things to make sure that I am on the right path.

Kindly let me know about any changes needed.

Regards,
Brahmana.

Comment 10

[Srirang G Doddihal (Brahmana)]

(In reply to comment #8)
> This patch contains changes to the XUL and JS files. Please let me know if I
> have done something extremely stupid and have missed out several obvious
> things. This is my patch for the UI.
> 
I meant this is my "First" patch for the UI.

And I had to put in two files as I did not how can combine the two patches from entirely different directories. 

> Regards,
> Brahmana
> 

Comment 11

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

(In reply to comment #8)
> Created an attachment (id=289306) [details]
> JS and XUL files patch

What I cannot say if the menuseparator_copy_location should also be changed to menuseparator_goto_location or similar. (In reply to comment #9)

> Created an attachment (id=289307) [details]
> DTD changes
> 
> As the name goes this patch has the dtd entries for the new ENTITY for "Go To
> Source Location" menu item. The access key is "g" (g for Go).

You should change this to the uppercase letter "G" as "_G_o" stated.

> I am not asking for a review for these patches as I am not sure what it still
> needs. I am just putting these things to make sure that I am on the right path.

It's quiet good to ask for review in either way.


> And I had to put in two files as I did not how can combine the two patches from
> entirely different directories. 

Just run following command from withing the mozilla root folder (I expect that you didn't made any further changes):

cvs diff -pu8 toolkit/mozapps/downloads/content/ toolkit/locales/en-US/chrome/mozapps/downloads >patch.diff

Comment 12

[Srirang G Doddihal (Brahmana)]

(In reply to comment #11)
> What I cannot say if the menuseparator_copy_location should also be changed to
> menuseparator_goto_location or similar. (In reply to comment #9)
> 
Is this really required? Because we still have the "Copy Location"  menu item under this separator and moreover this change would mean a bunch of other changes also. (Though its trivial substitution, keeping it as it is is simpler).

> 
> You should change this to the uppercase letter "G" as "_G_o" stated.
> 
Done.
> Just run following command from withing the mozilla root folder (I expect that
> you didn't made any further changes):
> 
> cvs diff -pu8 toolkit/mozapps/downloads/content/
> toolkit/locales/en-US/chrome/mozapps/downloads >patch.diff
> 

Oh thank you. I did not know that I can use the cvs diff utility on multiple directories. I will put up the next patch soon.

Regards,
Brahmana

Comment 13

[Shawn Wilsher :sdwilsh]

Please request review from me when you attach it to get it on my radar.

Comment 14

[Srirang G Doddihal (Brahmana)]

Attachment: full patch v0.1

Since there have been no comments on the entity id of the menu separator I am not changing it. I guess this patch is good to go. I tested it on a couple of downloads and it worked fine.

Regards,
Brahmana.

Comment 15

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

Brahama, applying your patch to head raises some problems:

patching file toolkit/mozapps/downloads/content/downloads.js
Hunk #1 succeeded at 347 (offset -2 lines).
Hunk #2 succeeded at 516 (offset -2 lines).
Hunk #3 succeeded at 639 (offset -2 lines).
Hunk #4 succeeded at 703 with fuzz 2 (offset -2 lines).
patching file toolkit/mozapps/downloads/content/downloads.xul
Hunk #2 succeeded at 154 (offset 3 lines).
patching file toolkit/locales/en-US/chrome/mozapps/downloads/downloads.dtd
Hunk #1 succeeded at 24 with fuzz 1.

Further there are several small errors:

downloads.js:
      case "cmd_goToLocation";

There should be a colon at the end of the line.

After fixing this a click on Go to Download Location does nothing here.

Comment 16

[Srirang G Doddihal (Brahmana)]

Attachment: patch v0.2

(In reply to comment #15)
> Brahama, applying your patch to head raises some problems:
> 
> patching file toolkit/mozapps/downloads/content/downloads.js
> Hunk #1 succeeded at 347 (offset -2 lines).
> Hunk #2 succeeded at 516 (offset -2 lines).
> Hunk #3 succeeded at 639 (offset -2 lines).
> Hunk #4 succeeded at 703 with fuzz 2 (offset -2 lines).
> patching file toolkit/mozapps/downloads/content/downloads.xul
> Hunk #2 succeeded at 154 (offset 3 lines).
> patching file toolkit/locales/en-US/chrome/mozapps/downloads/downloads.dtd
> Hunk #1 succeeded at 24 with fuzz 1.
> 

Ok. I am not good at cvs. I just used the command that you mentioned in an earlier comment of yours. Moreover I am just seeing "succeeded" messages. So I do not understand what problems my patch is causing.

> Further there are several small errors:
> 
> downloads.js:
>       case "cmd_goToLocation";
> 
> There should be a colon at the end of the line.
> 
> After fixing this a click on Go to Download Location does nothing here.
> 
Extremely sorry about this. I do not have a proper build running at my home (from where I submitted the patch). So I could not actually test it. But I had indeed tried the same changes (of course with your above mentioned correction) and it worked properly for me earlier at my work place.

Anyways I did it again now. I tested it for a download from sourceforge, where in downloads start automatically and for a download with explicit download link and finally for an image saved using the "Save Image As.." context menu item.
In all these cases I got the desired behavior of the corresponding webpage being displayed in a new tab.

This new patch is from the machine I have tested with and it is working very fine. So I hope this will work well with others also.

I created this patch by running the following command from the top-src dir.
cvs diff -u8p toolkit/mozapps/downloads/content toolkit/locales/en-US/chrome/mozapps/downloads > /e/patchFile.txt

Let me know if I am doing something wrong.

Regards,
Brahmana

Comment 17

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

Thanks for updating the patch. Now it can be applied. Before you create a diff simply ensure that you have updated to the latest version locally.

Go to Source Location only works if referrers are allowed. This can be controlled with the pref network.http.sendRefererHeader. If you turn it off no referrer attribute exists within the download object. In that case Go to Source Location won't work. Shawn already stated this in comment 7. 

Comment 18

[Srirang G Doddihal (Brahmana)]

(In reply to comment #17)
> Go to Source Location only works if referrers are allowed. This can be
> controlled with the pref network.http.sendRefererHeader. If you turn it off no
> referrer attribute exists within the download object. In that case Go to Source
> Location won't work. Shawn already stated this in comment 7. 
> 

Looks like I have not understood the comment correctly. When shawn said "take the URI up until the file name and go there." I thought I have to take the URI "including the file name" which happens to be the source (pretty dumb, i know). According to that it currently works fine now as I go to the source when referrer is not there.

If it meant that the source URI should be parsed, then the filename stripped and then go to the rest of the URI, I need to do a bit of string parsing, unless there are methods already to strip the leaf name from a URI.

As such this will lead to index of that directory being shown if the user is authorized, else the user might just get an authorization failure error.

I hope I have understood it correct this time. 
If so is that the desired behavior?

Comment 19

[Shawn Wilsher :sdwilsh]

filename stripped please

Comment 20

[Srirang G Doddihal (Brahmana)]

For the case of referrer not being preset I was thinking getting the last index of '/' in the URI and fetch the substring from 0 - to - the last index obtained earlier. 

But then a '/' in the query string would create a problem. So if there is a query string in the referrer should it be left out?

How should the query strings be handled?

Comment 21

[Shawn Wilsher :sdwilsh]

There's no perfect solution if we do not have a referrer.  Testing for / is probably the best thing you can do.

Comment 22

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

I've taken a look at different download pages. Mostly all of them offer dynamic pages with redirects to different FTP servers. But the download links don't have a query string inside. As Shawn said it's not perfect but mostly we will end up within the correct directory.

Comment 23

[Srirang G Doddihal (Brahmana)]

Attachment: patch v0.3

I guess this is good to go as per the earlier discussion. Let me know if I am missing anything.

Regards,
Brahmana

Comment 24

[Shawn Wilsher :sdwilsh]

Comment on attachment 290662 [details] [diff] [review]
patch v0.3

>+function goToLocation(aDownload)
>+{
>+  var hasReferrer = true;
>+
>+  // Fetch the referrer if there is one, else get the source and process it.
>+
>+  if (aDownload.hasAttribute("referrer")) {
>+    var referrerUri = aDownload.getAttribute("referrer");
>+  } else {
>+    var referrerUri = aDownload.getAttribute("source");
>+    hasReferrer = false;
>+  }
>+
>+  if (hasReferrer) {
>+    // Open the referrer, since there is one.
>+    openURL(referrerUri);
>+  } else {
>+    // As no referrer is present, strip the filename from the URL
>+    // and open the rest of the URL. We consider the part of URL after
>+    // the last '/' as the filename and remove it.
>+
>+    var lastSlashIndex = referrerUri.lastIndexOf('/');
>+    var smallReferrer = referrerUri.substring(0, lastSlashIndex+1);
>+
>+    openURL(smallReferrer);
>+  }
>+}
1) you don't need the boolean variable.  All the logic for parsing should be done in the first if block.
2) instead of testing if it has a referrer first, just do this:
var uri = aDownload.getAttribute("referrer")
if (uri) {
  ...
} else {
  ...
}

Comment 25

[Ed Lee :Mardak]

Why couldn't we just reuse openReferrer which is just openURL(getReferrerOrSource())?

The existing code is being used for handling clicking on the referrer from the info box. The info box will be gone soon, but that logic can be reused.

Comment 26

[Ed Lee :Mardak]

(Add a menuitem that has command="cmd_openReferrer")

Comment 27

[Srirang G Doddihal (Brahmana)]

Attachment: patch v0.4

Ok this patch has the behavior as specified by Shawn and Henrik. If the referrer is not available then it parses the URL and strips anything after the last '/' in the URL and will open the rest of the URL. This, as discussed before, can land the user in a directory view of the server location.

@Mardak:
My earlier patch did that. I used the openReferrer() function already present there and there was not much effort for me. Then Shawn and Henrik said that we need to take away the filename from the source URL, which according to us is the part after the last '/', and open the rest of the URL. This patch does that. The behavior that you mentioned is available with the second patch, patch v0.2. 

Actually speaking, I am also for the approach suggested by Mardak as it makes more sense to people who do not want to end up looking at the directory listing of some remote server, specially people without much computer knowledge.

Shawn and Henrik, what do you say? If what I and Mardak are saying is fine then patch v0.2 is the one to go with else it is this patch.

Regards,
Brahmana

Comment 28

[Ed Lee :Mardak]

Oh, well.. if it is a file and we don't have a referrer, going to the file would just cause the file to be downloaded again.

Comment 29

[Srirang G Doddihal (Brahmana)]

we do not go to the file again because the filename will be stripped. That is what I meant by taking away everything after the last '/' in the source URL. 

Eg; www.somewebsite.com/downloads/afile.ext

Here I remove the afile.ext and open the rest of the URL. This will give him the directory listing of the downloads folder on "somewebsite" if the user is allowed to view that,else he gets a "No Authorization" error.

And BTW why are you not replying on #foxymonkies?

Comment 30

[Ed Lee :Mardak]

For both "go to source" and "copy source", they should use the same value.. that being the referrer if we have it or the download source (unmodified).

[12/04/2007 15:02:46] <madhava> well, "source location" should mean the same in both cases

[12/04/2007 15:06:41] <madhava> it depends on what we think the most common task here is -- getting the download again?  going back to the page where you got it (maybe you downloaded the wrong one)?
[12/04/2007 15:07:52] <madhava> going to the page gives people what they want in the latter case and sets them on the road to it if what they want is the former (getting the download again)
[12/04/2007 15:10:35] <madhava> I don't think we want to get ourselves into a scenario where we have to count on people being able to edit URLs in the location bar
[12/04/2007 15:12:26] <madhava> so, in the case where all we had was the full file path (if they entered it directly or clicked on a link in an email), which means they probably never saw a referring page (if one even exists), then it kind of makes sense to me to just take them to the file
[12/04/2007 15:13:00] <madhava> (it's a bit odd, but it seems the least risky... and this is not the standard case, after all)

Comment 31

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

Comment on attachment 291397 [details] [diff] [review]
patch v0.4

>+    referrerUri = aDownload.getAttribute("source");

That's wrong. We don't have an attribute 'source' here. That's why it doesn't work for me. Replace 'source' with 'uri' and you will have the expected behavior. Looks good then with and without a referrer.

Comment 32

[Madhava Enros [:madhava]]

I just wrote this over in bug 397655, but it's really more relevant here:

Alright.  This is getting complicated enough that it's feeling like we're
trying to do the wrong things.  I've gone back to thinking through the
user-goals here.  Mardak's right -- if people are copying the location, it's
probably to get a link directly to the file (unlike when they want to go to the
source page, for which there could be other reasons).

Here's what I think we should do.  The two menu items should be:

Go to Download Page
Copy Download Link

"Go to Download Page" is the equivalent of "Go to Source Location" in that it
takes you to the referrer.  If we don't have a referrer, we grey out this
option.  It doesn't change behavior based on what information we have.

"Copy Download Link" copies the full URL, including the filename, to the
clipboard.  The two options can do different things because we're not using the
same term "Source Location" (which is kind of uninformative anyway) in both
cases.

Sorry to have gone back and forth on this -- I think this time it's right.

Comment 33

[Ed Lee :Mardak]

Comment on attachment 291397 [details] [diff] [review]
patch v0.4

We need a new patch that does "go to download page" and the menu item should only be accessible if the referrer exists.

We could just reuse the existing openReferrer even though it does a referrerOrSource because we can leverage the fact that it'll eventually only be called if the referrer is set.

Comment 34

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

How should we proceed with the 403* bugs? I set them a while back as depends on this one. Meanwhile I'm unsure if this was the right decision. This bug doesn't seems to be fixed soon. So do we want to prefer them to this one? If yes I could come up with patches shortly.

Comment 35

[Srirang G Doddihal (Brahmana)]

(In reply to comment #34)
> How should we proceed with the 403* bugs? I set them a while back as depends on
> this one. Meanwhile I'm unsure if this was the right decision. This bug doesn't
> seems to be fixed soon. So do we want to prefer them to this one? If yes I
> could come up with patches shortly.
> 

I finished the work for this patch the same day when Madhava put in the UX decisions. I have the patch on my machine since few days. But Mardak said he is rearranging all the context menu-items and I better wait for that to happen and then submit a patch that is in accordance with the changes. If need be I can submit a patch right-away now since the priority changed to P1.

Mardak, what say? Is it ok for you to have one more item when rearranging things?

Comment 36

[Shawn Wilsher :sdwilsh]

We have some time still.  Edward's work should land within the next week, which should be plenty of time for you to get your patch up.  No rush.

Comment 37

[Ed Lee :Mardak]

Attachment: v1

Thanks for the patches Brahmana. I've updated your patch against the version of code that I have that will be checked in soon.. hopefully.

Comment 38

[Ed Lee :Mardak]

Attachment: v1.1

Thanks again for the initial patches Brahmana and pointing out the problems when applying the patch. I've updated it to not be below 'remove info button' on my stack, which is probably better anyway.. We don't want to remove the info button without providing a way to get at the data.

sdwilsh: Do we want to fix up openReferrer to just do openURL(getAttr('referrer')) or leave it as openURL(getRefOrSource()) which will always end up giving the referrer because cmd_openReferrer is only available when hasAttri('referrer')

Comment 39

[Shawn Wilsher :sdwilsh]

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

r=sdwilsh

Comment 40

[Ed Lee :Mardak]

Thanks for working on this Brahmana!

Checking in toolkit/locales/en-US/chrome/mozapps/downloads/downloads.dtd;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.dtd,v  <--  downloads.dtd
new revision: 1.20; previous revision: 1.19
done
Checking in toolkit/mozapps/downloads/content/downloads.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.js,v  <--  downloads.js
new revision: 1.112; previous revision: 1.111
done
Checking in toolkit/mozapps/downloads/content/downloads.xul;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.xul,v  <--  downloads.xul
new revision: 1.38; previous revision: 1.37
done

Comment 41

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

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

Using no HTTP referrer grays out the "Go to Download Page" menuitem like it's expected.

Verified.

Comment 42

[Srirang G Doddihal (Brahmana)]

Mardak, you keep thanking me again and again, which we all know that you don't have to do that. In fact I should thank you for making my patches ready to check in, twice.

Anyways I hope of a continued affiliation with the Mozilla community.

Regards,
Brahmana.

Comment 43

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

Completed downloads: https://litmus.mozilla.org/show_test.cgi?id=5065
Failed downloads: https://litmus.mozilla.org/show_test.cgi?id=5067
Active/in-progress downloads: https://litmus.mozilla.org/show_test.cgi?id=5066