Add back-end for advanced search button that brings up places UI

RESOLVED FIXED in mozilla1.9beta2

Status

()

Toolkit
Downloads API
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

(Blocks: 1 bug)

Trunk
mozilla1.9beta2
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -
wanted1.9 +
in-litmus ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

11 years ago
Follow-up for UI overhaul.  The advanced search button is part of the spec, but not yet added.
Flags: in-litmus?
(Assignee)

Updated

10 years ago
Flags: blocking-firefox3?
Target Milestone: Firefox 3 M8 → Firefox 3 M9
Flags: blocking-firefox3? → blocking-firefox3+

Comment 1

10 years ago
Instead of just "Advanced search", the button could indicate how many total downloads there are in the history. Helps solve the problem of people not realizing there's actually more download history than what's shown in the download manager. -- Especially true if we add a "Clean up" that hides the hides the currently shown completed downloads.
moving out bugs that don't need to block b1
Target Milestone: Firefox 3 M9 → Firefox 3 M10
(Assignee)

Updated

10 years ago
Assignee: nobody → comrade693+bmo
(Assignee)

Updated

10 years ago
Depends on: 399800
(Assignee)

Comment 3

10 years ago
note to self:  since places organizer is browser specific, this needs to be done in browser/ as an overlay to the download manager.

The back-end work that uses places can still be a part of the download manager however.
(Assignee)

Comment 4

10 years ago
Created attachment 284946 [details] [diff] [review]
back-end changes v1.0

I think it may be best to do this in stages.  This patch is just the back-end changes needed to annotate URI's as downloads, and add them if they aren't exthandler downloads.
Attachment #284946 - Flags: review?(mano)
Comment on attachment 284946 [details] [diff] [review]
back-end changes v1.0

>Index: toolkit/components/downloads/src/Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/downloads/src/Makefile.in,v
>retrieving revision 1.15
>diff -u -8 -p -r1.15 Makefile.in
>--- toolkit/components/downloads/src/Makefile.in	28 Aug 2007 00:10:29 -0000	1.15
>+++ toolkit/components/downloads/src/Makefile.in	15 Oct 2007 16:28:09 -0000
>@@ -61,16 +61,20 @@ REQUIRES  = xpcom \
>             windowwatcher \
>             webbrowserpersist \
>             appshell \
>             dom \
>             embed_base \
>             alerts \
>             storage \
>             xulapp \
>+            history \
>+            docshell \
>+            places \
>+            toolkitcomps \
>             $(NULL)
> 
> CPPSRCS   = \
>     nsDownloadManager.cpp \
>     $(NULL)
> 
> ifeq ($(OS_ARCH),WINNT)
> CPPSRCS += nsDownloadScanner.cpp
>Index: toolkit/components/downloads/src/nsDownloadManager.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v
>retrieving revision 1.138
>diff -u -8 -p -r1.138 nsDownloadManager.cpp
>--- toolkit/components/downloads/src/nsDownloadManager.cpp	14 Oct 2007 00:57:34 -0000	1.138
>+++ toolkit/components/downloads/src/nsDownloadManager.cpp	15 Oct 2007 16:28:10 -0000
>@@ -70,16 +70,22 @@
> #include "nsIMutableArray.h"
> #include "nsIAlertsService.h"
> #include "nsIPropertyBag2.h"
> #include "nsIHttpChannel.h"
> #include "nsIDownloadManagerUI.h"
> #include "nsTArray.h"
> #include "nsIResumableChannel.h"
> 
>+#include "nsINavHistoryService.h"
>+#include "nsIBrowserHistory.h"
>+#include "nsDocShellCID.h"
>+#include "nsToolkitCompsCID.h"
>+#include "nsIAnnotationService.h"
>+
> #ifdef XP_WIN
> #include <shlobj.h>
> #include "nsDownloadScanner.h"
> #endif
> 
> static PRBool gStoppingDownloads = PR_FALSE;
> 
> #define DOWNLOAD_MANAGER_BUNDLE "chrome://mozapps/locale/downloads/downloads.properties"
>@@ -1095,18 +1101,48 @@ nsDownloadManager::AddDownload(DownloadT
>                                nsIDownloadManager::DOWNLOAD_NOTSTARTED);
>   NS_ENSURE_TRUE(id, NS_ERROR_FAILURE);
>   dl->mID = id;
> 
>   rv = AddToCurrentDownloads(dl);
>   (void)dl->SetState(nsIDownloadManager::DOWNLOAD_QUEUED);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>+  // After this, we should not return an error because we've addref'd our
>+  // download object!
>   NS_ADDREF(*aDownload = dl);
> 
>+  nsCOMPtr<nsIBrowserHistory> bh(do_GetService(NS_GLOBALHISTORY2_CONTRACTID));
>+  NS_ENSURE_TRUE(bh, NS_OK);
>+
>+  // If we have a MIME info, we know that exthandler has already added this to
>+  // the history, but if we do not, we'll have to add it ourselves.
>+  if (!aMIMEInfo) {
>+    nsAutoString title;
>+    nsCOMPtr<nsINavHistoryService> nhs =
>+      do_GetService(NS_NAVHISTORYSERVICE_CONTRACTID);

bh === nhs, you could just QI.

>+    if (nhs)
>+      (void)nhs->GetPageTitle(aSource, title);
>+
>+    rv = bh->AddPageWithDetails(aSource, title.get(), aStartTime);

but actually, i think you should rather use nhs->AddVisit here.

>+    NS_ENSURE_SUCCESS(rv, NS_OK);
>+  }
>+
>+  // We want to hide the URI so it doesn't show up normally in history.
>+  (void)bh->HidePage(aSource);
>+
>+  // Now, let's annotate the given URI to indicate that it is a download.
>+  nsCOMPtr<nsIAnnotationService> as =
>+    do_GetService(NS_ANNOTATIONSERVICE_CONTRACTID);
>+  NS_ENSURE_TRUE(as, NS_OK);
>+
>+  rv = as->SetPageAnnotationInt32(aSource, NS_LITERAL_CSTRING("isDownload"),
>+                                  1, 0, nsIAnnotationService::EXPIRE_WITH_HISTORY);
>+  NS_ENSURE_SUCCESS(rv, NS_OK);
>+

A better and more efficient solution would be a new TRANSITION_* type IMO.
Attachment #284946 - Flags: review?(mano) → review-
(Assignee)

Comment 6

10 years ago
(In reply to comment #5)
> bh === nhs, you could just QI.
On the off chance that someone wants to override one, would it not be better to get the service for each?

> but actually, i think you should rather use nhs->AddVisit here.
I thought about using AddVist, but I thought of a few issues, which is why I didn't use it:
1) The referring visit would be nice to have, but we won't have it (might not ever get it) until nsDownload::OnProgressChange64.  I didn't think we would want to wait until then to add the download to history (in the case of save as, it might be queued for some time).  Although, now that I think about it, in that case I don't believe we have a start time so that might be the better place for this code anyway...
Anyway, how can I get the id for a page?  Please don't tell me I have to setup a query just to get it... (and if this is the case, is this a good addition for some new API?)
2) Is there a way to get the session id for the current session?  Something tells me that 0 isn't the one we want, but then I'm not sure either.

> A better and more efficient solution would be a new TRANSITION_* type IMO.
Do you mean a new constant like nsINavHistoryService::TRANSITION_DOWNLOAD?
(Assignee)

Updated

10 years ago
Depends on: 400543
(Assignee)

Updated

10 years ago
Depends on: 400544
(Assignee)

Updated

10 years ago
Depends on: 401174
(Assignee)

Updated

10 years ago
No longer depends on: 400543, 400544
(Assignee)

Updated

10 years ago
Depends on: 401190
(Assignee)

Comment 7

10 years ago
Created attachment 286238 [details] [diff] [review]
back-end changes v1.1

Once all our dependencies are fixed, this is all we need.
Attachment #284946 - Attachment is obsolete: true
Attachment #286238 - Flags: review?(mano)

Updated

10 years ago
Target Milestone: Firefox 3 M10 → Firefox 3 M11
Blocks: 402231

Updated

10 years ago
Priority: -- → P5
(Assignee)

Updated

10 years ago
Whiteboard: [has patch (phase 1)][needs review Mano]
(Assignee)

Updated

10 years ago
Blocks: 249257
(Assignee)

Updated

10 years ago
Attachment #286238 - Attachment is obsolete: true
Attachment #286238 - Flags: review?(mano)
(Assignee)

Comment 8

10 years ago
We also need to use the NS_GetReferrerFromChannel added in Bug 401174.
Whiteboard: [has patch (phase 1)][needs review Mano] → [needs new patch]
(Assignee)

Comment 9

10 years ago
Created attachment 289195 [details] [diff] [review]
v1.2
Attachment #289195 - Flags: review?(mano)
(Assignee)

Updated

10 years ago
Whiteboard: [needs new patch] → [has patch][needs review mano]
Comment on attachment 289195 [details] [diff] [review]
v1.2

r=mano
Attachment #289195 - Flags: review?(mano) → review+
(Assignee)

Updated

10 years ago
Whiteboard: [has patch][needs review mano] → [has patch][has review][needs dependent bug to land]
wanted for the backend pieces only, the UI will wait until we have a clearer plan for integration of all of this data.
Flags: wanted-firefox3+
Flags: blocking-firefox3-
Flags: blocking-firefox3+
Priority: P5 → P4
Target Milestone: Firefox 3 Mx → Firefox 3 M11
(Assignee)

Updated

10 years ago
Attachment #289195 - Flags: approval1.9?
Comment on attachment 289195 [details] [diff] [review]
v1.2

a=beltzner for drivers
Attachment #289195 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 13

10 years ago
Checking in toolkit/components/downloads/src/Makefile.in;
new revision: 1.17; previous revision: 1.16
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
new revision: 1.147; previous revision: 1.146

The back-end is in.  Once places implements the right interface, some add-on can come in and provide the UI for this feature, and we can work it on for FirefoxNext.  Clearing priority, wanted flag, and retargeting...
Priority: P4 → --
Whiteboard: [has patch][has review][needs dependent bug to land] → [back-end landed]
Target Milestone: Firefox 3 M11 → Future
(Assignee)

Updated

10 years ago
Assignee: sdwilsh → nobody
Filed bug 433038 on the front-end, resolving.
Assignee: nobody → sdwilsh
Summary: Add advanced search button that brings up places UI → Add back-end for advanced search button that brings up places UI
Whiteboard: [back-end landed]
Target Milestone: Future → Firefox 3 beta2
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Blocks: 433038
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.