Closed Bug 517889 Opened 10 years ago Closed 9 years ago

"go to page" in download manager results seems to do nothing

Categories

(Firefox for Android Graveyard :: General, defect)

All
MeeGo
defect
Not set

Tracking

(fennec2.0+)

VERIFIED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: jmaher, Assigned: wesj)

References

Details

Attachments

(2 files, 5 obsolete files)

on my n810 with the 1.9.2 nightly build from 20090921, I download some pdf's from google.com and irs.gov.  Viewing the results I have a "Go to Page", "Open", "Remove" option.  The "Go to Page" option is always grey and never seems to be active.

If we are not supporting it, we should remove it as well to clean up the UI.
need to make sure the links I am downloading really have a url with them that we recognize.  Downloading a xulrunner build from the nightly drop will work perfect for the "Go to Page" option.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WORKSFORME
aye, this worksforme as well on winmo and maemo 1.9.2 builds
Status: RESOLVED → VERIFIED
Component: Linux/Maemo → General
QA Contact: maemo-linux → general
This bug is reproduced on the newest fennec(mobile tip:67b8ca6bfaf2).
The "Go to Page" option is always grey and never seems to be active.
So reopen this bug.
reopen the bug due to Junmin's finding.
Status: VERIFIED → REOPENED
Resolution: WORKSFORME → ---
OS: Maemo → MeeGo
Hardware: ARM → All
In addition, it is also reproduced on the newest nightly build(http://ftp.mozilla.org/pub/mozilla.org/mobile/nightly/latest-mozilla-central-linux/).
Wes - are we not setting the referring page URL correctly?
Assignee: nobody → wjohnston
Attached patch Patch (obsolete) — Splinter Review
Fix
Attachment #485757 - Flags: review?(mark.finkle)
Comment on attachment 485757 [details] [diff] [review]
Patch

Removing review request for a minute here. Need to figure out exactly what Firefox is doing.
Attachment #485757 - Flags: review?(mark.finkle)
Attached patch Better fix (obsolete) — Splinter Review
I think this is the problem. We're not attaching a referrer to documents that we open via this DOMActive event.
Attachment #485757 - Attachment is obsolete: true
Attachment #485860 - Flags: review?(mark.finkle)
Comment on attachment 485860 [details] [diff] [review]
Better fix

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

>       case "Browser:OpenURI":
>-        Browser.addTab(json.uri, json.bringFront, Browser.selectedTab);
>+        if(json.params.referrer)

if(

>+          json.params.referrerURI = Services.io.newURI(json.params.referrer, null, null);
>+
>+        Browser.addTab(json.uri, json.bringFront, Browser.selectedTab, json.params);

Let's not just pass the json.params object to Browser.addTab. Let's just make a new literal object:

  let referrerURI = ...
  Browser.addTab( ..., { referrerURI: referrerURI }); 

>diff --git a/chrome/content/content.js b/chrome/content/content.js

>         if (/^http(s?):/.test(href)) {
>-          aEvent.preventDefault();
>-          sendAsyncMessage("Browser:OpenURI", { uri: href, bringFront: true });
>+           aEvent.preventDefault();

Why the extra space indent?

>+           sendAsyncMessage("Browser:OpenURI", { uri: href,
>+                                                 params: {
>+                                                   referrer: target.ownerDocument.documentURIObject.spec
>+                                                 },

No need for a params object, just pass referrer like the other data.
Attachment #485860 - Flags: review?(mark.finkle) → review-
I reapplied this yesterday to clean up and it didn't work. Then I realized that I had accidentally turned off remote tabs the day before to see if this was a e10s problem or not. Turns out, it doesn't work in either local or remote, but for two different reasons. This was the local one, and one that our users will likely never hit.

For remote tabs, we are sending a message from child to parent for the external helper app service, but we throw away the referrer along the way:

http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#723

We need to modify ExternalHelperAppParent so that it can fill in the hashtable entry for "docshell.internalReferrer". I have a patch to do that building right now.
Attached patch Remote tabs patch (obsolete) — Splinter Review
Fix for remote tabs.
Attachment #486483 - Flags: review?
Attachment #486483 - Flags: review? → review?(crowderbt)
Attached patch Patch V2 (obsolete) — Splinter Review
Uploaded the wrong file last time.
Attachment #486483 - Attachment is obsolete: true
Attachment #486487 - Flags: review?(crowderbt)
Attachment #486483 - Flags: review?(crowderbt)
Comment on attachment 486487 [details] [diff] [review]
Patch V2

Looks good to me, asking dwitte for further opinions
Attachment #486487 - Flags: superreview?(dwitte)
Attachment #486487 - Flags: review?(crowderbt)
Attachment #486487 - Flags: review+
Attached patch Local tab fixes (obsolete) — Splinter Review
Not sure if we want this fix or not. I don't know that we will run into it that often.
Attachment #485860 - Attachment is obsolete: true
Attachment #486750 - Flags: review?(mark.finkle)
Attachment #486750 - Flags: review?(mark.finkle) → review+
Comment on attachment 486487 [details] [diff] [review]
Patch V2

>diff --git a/uriloader/exthandler/nsExternalHelperAppService.cpp b/uriloader/exthandler/nsExternalHelperAppService.cpp

>+    nsCOMPtr<nsIURI> referrer;
>+    rv = NS_GetReferrerFromChannel(channel, getter_AddRefs(referrer));

You don't check rv here, I think you can ditch it.

r=me on the IPDL bits. No need for sr here, but you should have someone double-check that the docshell.internalReferrer bits are correct.
Attachment #486487 - Flags: superreview?(dwitte)
Comment on attachment 486487 [details] [diff] [review]
Patch V2

Boris - can you take a look at the docshell.internalReferrer parts ?
Attachment #486487 - Flags: review?(bzbarsky)
Comment on attachment 486487 [details] [diff] [review]
Patch V2

Seems ok.
Attachment #486487 - Flags: review?(bzbarsky) → review+
tracking-fennec: --- → 2.0+
Attached patch UnibitrottedSplinter Review
Just pushed this to try. If all goes well, azakai says he will push it to MC this weekend.
Attachment #486487 - Attachment is obsolete: true
Attachment #492012 - Flags: review+
Pushed the m-c part:

http://hg.mozilla.org/mozilla-central/rev/1002f2452919

Does the m-b part need to be pushed?
Unbitrotted these local tab fixes as well.
Attachment #486750 - Attachment is obsolete: true
Attachment #492382 - Flags: review+
Pushed the m-b part:

http://hg.mozilla.org/mobile-browser/rev/6ccfabc6c832
Status: REOPENED → RESOLVED
Closed: 10 years ago9 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Verified:

Mozilla/5.0 (Android; Linux armv71; rv2.0b8pre) Gecko/20101123 Firefox/4.0b8pre Fennec/4.0b3pre
Mozilla/5.0 (Maemo; Linux armv71; rv:2.0b8pre) Gecko/20101123 Firefox/4.0b8pre Fennec/4.0b3pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.