Store the value of the per-site last file upload directories inside the memory while private browsing is active

RESOLVED FIXED in mozilla1.9.3a2

Status

()

Core
HTML: Form Submission
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Ehsan, Assigned: darktrojan)

Tracking

({privacy})

Trunk
mozilla1.9.3a2
privacy
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Reporter)

Description

9 years ago
We need to support the following use case in a sane way:

1. User enters private browsing mode.
2. She opens a web site with an <input type=file>, browses to a directory and selects a file.
3. She tries to submit another file, but when the file picker shows up, they are moved back to the directory selected at the beginning of step 2, whereas filepicker should be inited to the directory selected at the end of step 2.
4. When she leaves the private browsing mode, on the same website, the filepicker control should be inited to the directory selected at the beginning of step 2.

Please note that the contentprefs service does not store anything inside the private browsing mode, and note that on some platforms, the filepicker's initial directory is specified by the OS if we don't specify one, which is not what we want.

<http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/src/DownloadLastDir.jsm> can be used as a sample of an implementation which achieves these goals.
Keywords: privacy
(Assignee)

Comment 1

9 years ago
Created attachment 419224 [details] [diff] [review]
tests
Assignee: nobody → geoff
Status: NEW → ASSIGNED
(Assignee)

Comment 2

9 years ago
I'll wait to see what happens with bug 536503 before I try to do the same here.
(Assignee)

Comment 3

9 years ago
Created attachment 420877 [details] [diff] [review]
patch v1

This probably isn't review-worthy but I'd appreciate someone having a good look through it.
Attachment #419224 - Attachment is obsolete: true
(Assignee)

Comment 4

9 years ago
Created attachment 422307 [details] [diff] [review]
patch v2
Attachment #420877 - Attachment is obsolete: true
Attachment #422307 - Flags: review?(bzbarsky)
Comment on attachment 422307 [details] [diff] [review]
patch v2

ehsan, do you mind reviewing this?  If you really think I should look too just let me know.
Attachment #422307 - Flags: review?(bzbarsky) → review?(ehsan.akhgari)
(Reporter)

Comment 6

8 years ago
Comment on attachment 422307 [details] [diff] [review]
patch v2

>+NS_IMPL_ISUPPORTS1(nsFileControlFrame::UploadLastDir, nsIObserver)
>+
>+nsFileControlFrame::UploadLastDir* nsFileControlFrame::gUploadLastDir
>+  = new nsFileControlFrame::UploadLastDir();

You should do this from an initializer function not in the global scope.  Also, you need to call NS_IF_ADDREF on the pointer after you create the object, so that other objects releasing it would not cause it to be destroyed sooner than we expect.

Also, you're not deleting the object, so this code will leak (well, not exactly, because there's only one object for the lifetime of the application) but our leak reporting code should probably catch this.  Which brings me to the question that did you try this in a build with leak reporting enabled?

>         if (!prefSaved) {
>           // Store the last used directory using the content pref service
>-          StoreLastUsedDirectory(doc->GetDocumentURI(), file);
>+          gUploadLastDir->StoreLastUsedDirectory(doc->GetDocumentURI(), localFile);
>           prefSaved = PR_TRUE;

Shouldn't you be checking the return value of StoreLastUsedDirectory and set prefSaved accordingly?

>+nsFileControlFrame::UploadLastDir::UploadLastDir() {
>+  mUploadLastDirStore.Init();
>+  nsCOMPtr<nsIObserverService> observerService
>+    = do_GetService("@mozilla.org/observer-service;1");
>+  observerService->AddObserver(this, NS_PRIVATE_BROWSING_SWITCH_TOPIC, PR_FALSE);
>+  observerService->AddObserver(this, "browser:purge-session-history", PR_FALSE);
>+  observerService->AddObserver(this, "quit-application", PR_FALSE);
>+}

Null-checking observerService here would be a good idea.

> nsresult
>-nsFileControlFrame::FetchLastUsedDirectory(nsIURI* aURI, nsILocalFile* aFile)
>+nsFileControlFrame::UploadLastDir::FetchLastUsedDirectory(nsIURI* aURI,
>+                                                          nsILocalFile** aFile)
> {
>+  // Retrieve the data from memory if it's present during private browsing mode,
>+  // otherwise fall through to check the CPS
>+  nsCOMPtr<nsIPrivateBrowsingService> pbService
>+    = do_GetService(NS_PRIVATE_BROWSING_CONTRACTID);
>+  if (!pbService)
>+    return NS_ERROR_NOT_AVAILABLE;

This is not correct.  The private browsing service is only available in Firefox, and not in any other Gecko-based application, which means that this code will return NS_ERROR_NOT_AVAILABLE for anything except Firefox.  You can treat the case where this service is not available as though we're not in private browsing mode, like this:

PRBool inPrivateBrowsing = PR_FALSE;
nsCOMPtr<nsIPrivateBrowsingService> pbService =
  do_GetService(NS_PRIVATE_BROWSING_CONTRACTID);
if (pbService) {
  pbService->GetPrivateBrowsingEnabled(&inPrivateBrowisng);
}

>+  PRBool pbServiceEnabled;
>+  pbService->GetPrivateBrowsingEnabled(&pbServiceEnabled);
>+  if (pbServiceEnabled) {
>+    nsCOMPtr<nsIContentURIGrouper> hostnameGrouperService
>+      = do_GetService(NS_HOSTNAME_GROUPER_SERVICE_CONTRACTID);
>+    nsString group;
>+    hostnameGrouperService->Group(aURI, group);

Null-checking hostnameGrouperService would be a good idea here.

>+    if (mUploadLastDirStore.Get(group, (aFile))) {

Nit: s/(aFile)/aFile/.

>+      return NS_OK;
>+    }
>+  }
>+
>   // Attempt to get the CPS, if it's not present we'll just return
>   nsCOMPtr<nsIContentPrefService> contentPrefService =
>     do_GetService(NS_CONTENT_PREF_SERVICE_CONTRACTID);
>   if (!contentPrefService)
>     return NS_ERROR_NOT_AVAILABLE;
>   nsCOMPtr<nsIWritableVariant> uri = do_CreateInstance(NS_VARIANT_CONTRACTID);
>   if (!uri)
>     return NS_ERROR_OUT_OF_MEMORY;
>   uri->SetAsISupports(aURI);
>-  NS_NAMED_LITERAL_STRING(prefName, "lastUploadDirectory");
>+  NS_NAMED_LITERAL_STRING(prefName, "browser.upload.lastDir");

Renaming the site-specific setting here means that nightly users who have already upgraded to this code will see the behavior equivalent to their CPS store being reset.  This is not a big issue, but was there any specific reason why you chose to rename this?

BTW, you would want to create a global literal here at the top of the file and just using it instead of repeating the string all around the code.

>   // Get the last used directory, if it is stored
>   PRBool hasPref;
>   if (NS_SUCCEEDED(contentPrefService->HasPref(uri, prefName, &hasPref)) && hasPref) {
>     nsCOMPtr<nsIVariant> pref;
>     contentPrefService->GetPref(uri, prefName, getter_AddRefs(pref));
>     nsString prefStr;
>     pref->GetAsAString(prefStr);
>-    return aFile->InitWithPath(prefStr);
>+
>+    nsCOMPtr<nsILocalFile> localFile = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID);

Null-checking localFile here would also be a good idea.

>+    localFile->InitWithPath(prefStr);
>+
>+    *aFile = localFile;
>+    NS_ADDREF(*aFile);
>   }
>   return NS_OK;
> }
> 
> void
>-nsFileControlFrame::StoreLastUsedDirectory(nsIURI* aURI, nsIFile* aFile)
>+nsFileControlFrame::UploadLastDir::StoreLastUsedDirectory(nsIURI* aURI,
>+                                                          nsILocalFile* aFile)
> {
>+  nsCOMPtr<nsIFile> parentFile;
>+  aFile->GetParent(getter_AddRefs(parentFile));
>+  nsCOMPtr<nsILocalFile> localFile = do_QueryInterface(parentFile);
>+
>+  // Store the data in memory instead of the CPS during private browsing mode
>+  nsCOMPtr<nsIPrivateBrowsingService> pbService
>+    = do_GetService(NS_PRIVATE_BROWSING_CONTRACTID);
>+  if (!pbService)
>+    return;

The same comment about the existence of the PB service applies here as well.  If this service is not available, you should proceed to store the URL in the CPS store.

>+  PRBool pbServiceEnabled;
>+  pbService->GetPrivateBrowsingEnabled(&pbServiceEnabled);
>+  if (pbServiceEnabled) {
>+    nsCOMPtr<nsIContentURIGrouper> hostnameGrouperService
>+      = do_GetService(NS_HOSTNAME_GROUPER_SERVICE_CONTRACTID);
>+    nsString group;
>+    hostnameGrouperService->Group(aURI, group);

Inject the null-checking comment here as well!

>+NS_IMETHODIMP
>+nsFileControlFrame::UploadLastDir::Observe(nsISupports *aSubject,
>+                                           char const *aTopic,
>+                                           PRUnichar const *aData) {
>+  if (strcmp (aTopic, NS_PRIVATE_BROWSING_SWITCH_TOPIC) == 0 &&

Nit: no space between function name and the opening parenthesis.

>+      NS_LITERAL_STRING(NS_PRIVATE_BROWSING_LEAVE).Equals(aData)) {
>+    if (mUploadLastDirStore.IsInitialized()) {
>+      mUploadLastDirStore.Clear();
>+    }

Since you're already watching NS_PRIVATE_BROWSING_SWITCH_TOPIC, a slightly more efficient implementation is to track the status of the private browsing service as a PRBool member variable such as mInPrivateBrowsing, initialize it using the PB service inside the ctor, and toggle it appropriately here.  This way, {Fetch,Store}LastUsedDirectory can just use this member variable instead of querying the PB service directly.

>+  } else if (strcmp (aTopic, "quit-application") == 0) {
>+    nsCOMPtr<nsIObserverService> observerService
>+      = do_GetService("@mozilla.org/observer-service;1");
>+    observerService->RemoveObserver(this, NS_PRIVATE_BROWSING_SWITCH_TOPIC);
>+    observerService->RemoveObserver(this, "browser:purge-session-history");
>+    observerService->RemoveObserver(this, "quit-application");
>+  }

If you make this interface support weak references, then you wouldn't need this block.  See <https://developer.mozilla.org/en/Weak_reference#How_do_I_make_a_class_support_weak_references.3f> for more information.

>+  class UploadLastDir : public nsIObserver {

It would be a good idea to move the declaration of this class inside nsFileControlFrame.cpp, so that you wouldn't need to pull in all of those headers inside this header, and also not make other translation units which include this depend on the details of UploadLastDir implementation.

>+  public:
>+    NS_DECL_ISUPPORTS
>+    NS_DECL_NSIOBSERVER

Wouldn't you need to have a corresponding NS_IMPL_QUERYINTERFACE?

>+  static UploadLastDir* gUploadLastDir;

You should prefix class statics with s, but it would be a good idea to move this to nsFileControlFrame.cpp as well as a global static.

>diff --git a/layout/forms/test/bug536567_subframe.html b/layout/forms/test/bug536567_subframe.html

I haven't fully reviewed the test yet, but it looked mostly okay at a quick glance.  Have you pushed the test to the try server?  I can do that for you if you don't have the access.

>diff --git a/netwerk/base/public/nsIPrivateBrowsingService.idl b/netwerk/base/public/nsIPrivateBrowsingService.idl
>--- a/netwerk/base/public/nsIPrivateBrowsingService.idl
>+++ b/netwerk/base/public/nsIPrivateBrowsingService.idl
>@@ -58,16 +58,18 @@ interface nsIPrivateBrowsingService : ns
>      *
>      * @param aDomain
>      *        The domain that will have its data removed.
>      */
>     void removeDataFromDomain(in AUTF8String aDomain);
> };
> 
> %{C++
>+// The contractID for the generic implementation built in to xpcom.
>+#define NS_PRIVATE_BROWSING_CONTRACTID "@mozilla.org/privatebrowsing;1"

We already have this: <http://mxr.mozilla.org/mozilla-central/source/netwerk/build/nsNetCID.h#445>.

You should probably ask for super-review here as well for changing the IDL path, just to be on the safe side (though I think it's fine.)
Attachment #422307 - Flags: review?(ehsan.akhgari) → review-
(Assignee)

Comment 7

8 years ago
Created attachment 423127 [details] [diff] [review]
patch v3
Attachment #422307 - Attachment is obsolete: true
(Assignee)

Comment 8

8 years ago
(In reply to comment #6)
> Also, you're not deleting the object, so this code will leak (well, not
> exactly, because there's only one object for the lifetime of the application)
> but our leak reporting code should probably catch this.  Which brings me to the
> question that did you try this in a build with leak reporting enabled?

I hadn't, but I have now, and yes it does now catch this. How should I go about releasing it?
I suppose I could have a reference from every instance of nsFileControlFrame, but that seems a bit nasty.

> >         if (!prefSaved) {
> >           // Store the last used directory using the content pref service
> >-          StoreLastUsedDirectory(doc->GetDocumentURI(), file);
> >+          gUploadLastDir->StoreLastUsedDirectory(doc->GetDocumentURI(), localFile);
> >           prefSaved = PR_TRUE;
> 
> Shouldn't you be checking the return value of StoreLastUsedDirectory and set
> prefSaved accordingly?

I figure that if StoreLastUsedDirectory fails once in the loop, it'd fail every time, so there's no need to call it again.

> >-  NS_NAMED_LITERAL_STRING(prefName, "lastUploadDirectory");
> >+  NS_NAMED_LITERAL_STRING(prefName, "browser.upload.lastDir");
> 
> Renaming the site-specific setting here means that nightly users who have
> already upgraded to this code will see the behavior equivalent to their CPS
> store being reset.  This is not a big issue, but was there any specific reason
> why you chose to rename this?

I should've chosen a better name in the first place, but now I've renamed it to match browser.download.lastDir from bug 536503 (which matches the same in the prefService) and browser.content.full-zoom. Just keeping our DBs tidy :)

> Since you're already watching NS_PRIVATE_BROWSING_SWITCH_TOPIC, a slightly more
> efficient implementation is to track the status of the private browsing service
> as a PRBool member variable such as mInPrivateBrowsing, initialize it using the
> PB service inside the ctor, and toggle it appropriately here.  This way,
> {Fetch,Store}LastUsedDirectory can just use this member variable instead of
> querying the PB service directly.

Good point.

> >+  } else if (strcmp (aTopic, "quit-application") == 0) {
> >+    nsCOMPtr<nsIObserverService> observerService
> >+      = do_GetService("@mozilla.org/observer-service;1");
> >+    observerService->RemoveObserver(this, NS_PRIVATE_BROWSING_SWITCH_TOPIC);
> >+    observerService->RemoveObserver(this, "browser:purge-session-history");
> >+    observerService->RemoveObserver(this, "quit-application");
> >+  }
> 
> If you make this interface support weak references, then you wouldn't need this
> block.

I've tried, but observerService is crashing when I tried to use them. What have I missed?

> >+  static UploadLastDir* gUploadLastDir;
> 
> You should prefix class statics with s, but it would be a good idea to move
> this to nsFileControlFrame.cpp as well as a global static.

Oops, missed this. Fixing now...

> >diff --git a/layout/forms/test/bug536567_subframe.html b/layout/forms/test/bug536567_subframe.html
> 
> I haven't fully reviewed the test yet, but it looked mostly okay at a quick
> glance.  Have you pushed the test to the try server?  I can do that for you if
> you don't have the access.

I don't.

> >+// The contractID for the generic implementation built in to xpcom.
> >+#define NS_PRIVATE_BROWSING_CONTRACTID "@mozilla.org/privatebrowsing;1"
> 
> We already have this:
> <http://mxr.mozilla.org/mozilla-central/source/netwerk/build/nsNetCID.h#445>.

Ack! Reverted the .idl, forgot to rebuild that directory, so the current patch still has NS_PRIVATE_BROWSING_CONTRACTID.
(Reporter)

Comment 9

8 years ago
I somehow missed this in the pile of my bugmail; sorry about that!

(In reply to comment #8)
> (In reply to comment #6)
> > Also, you're not deleting the object, so this code will leak (well, not
> > exactly, because there's only one object for the lifetime of the application)
> > but our leak reporting code should probably catch this.  Which brings me to the
> > question that did you try this in a build with leak reporting enabled?
> 
> I hadn't, but I have now, and yes it does now catch this. How should I go about
> releasing it?
> I suppose I could have a reference from every instance of nsFileControlFrame,
> but that seems a bit nasty.

You can create static methods on the UploadLastDir class to instantiate it as a singleton and destroy it, and then call those methods from nsLayoutStatics::Initialize and nsLayoutStatics::Shutdown.

> > >         if (!prefSaved) {
> > >           // Store the last used directory using the content pref service
> > >-          StoreLastUsedDirectory(doc->GetDocumentURI(), file);
> > >+          gUploadLastDir->StoreLastUsedDirectory(doc->GetDocumentURI(), localFile);
> > >           prefSaved = PR_TRUE;
> > 
> > Shouldn't you be checking the return value of StoreLastUsedDirectory and set
> > prefSaved accordingly?
> 
> I figure that if StoreLastUsedDirectory fails once in the loop, it'd fail every
> time, so there's no need to call it again.

But you're plain ignoring the return value in your current code!  If you want to implement that assumption, you should probably break out of the loop or something if that call fails.

> > >-  NS_NAMED_LITERAL_STRING(prefName, "lastUploadDirectory");
> > >+  NS_NAMED_LITERAL_STRING(prefName, "browser.upload.lastDir");
> > 
> > Renaming the site-specific setting here means that nightly users who have
> > already upgraded to this code will see the behavior equivalent to their CPS
> > store being reset.  This is not a big issue, but was there any specific reason
> > why you chose to rename this?
> 
> I should've chosen a better name in the first place, but now I've renamed it to
> match browser.download.lastDir from bug 536503 (which matches the same in the
> prefService) and browser.content.full-zoom. Just keeping our DBs tidy :)

OK.  I still don't see why renaming this is required, but it's not a big deal.

> > >+  } else if (strcmp (aTopic, "quit-application") == 0) {
> > >+    nsCOMPtr<nsIObserverService> observerService
> > >+      = do_GetService("@mozilla.org/observer-service;1");
> > >+    observerService->RemoveObserver(this, NS_PRIVATE_BROWSING_SWITCH_TOPIC);
> > >+    observerService->RemoveObserver(this, "browser:purge-session-history");
> > >+    observerService->RemoveObserver(this, "quit-application");
> > >+  }
> > 
> > If you make this interface support weak references, then you wouldn't need this
> > block.
> 
> I've tried, but observerService is crashing when I tried to use them. What have
> I missed?

You should pass PR_TRUE as the last argument of AddObserver.  Does that solve the crash?

> > >diff --git a/layout/forms/test/bug536567_subframe.html b/layout/forms/test/bug536567_subframe.html
> > 
> > I haven't fully reviewed the test yet, but it looked mostly okay at a quick
> > glance.  Have you pushed the test to the try server?  I can do that for you if
> > you don't have the access.
> 
> I don't.

I just submitted it for you.  I used your name as the custom identifier string.  You can find the results of the build at <http://tinderbox.mozilla.org/showbuilds.cgi?tree=MozillaTry> in a few hours.  (Search for "geoff" on that page to see all of the boxes and the individual results for each of them.)
(Reporter)

Comment 10

8 years ago
Your latest patch does not apply cleanly on tip, so all of the try server jobs failed.  Example log:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1265418290.1265418715.13688.gz
(Assignee)

Comment 11

8 years ago
Created attachment 426175 [details] [diff] [review]
patch v4
Attachment #423127 - Attachment is obsolete: true
Attachment #426175 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #426175 - Flags: review? → review?(ehsan.akhgari)
(Reporter)

Comment 12

8 years ago
Comment on attachment 426175 [details] [diff] [review]
patch v4

>diff --git a/layout/forms/nsFileControlFrame.cpp b/layout/forms/nsFileControlFrame.cpp
>+UploadLastDir* sUploadLastDir = nsnull;

Nit: rename to gUploadLastDir please.

>+void nsFileControlFrame::InitUploadLastDir() {
>+  sUploadLastDir = new UploadLastDir();
>+  NS_IF_ADDREF(sUploadLastDir);
>+
>+  nsCOMPtr<nsIObserverService> observerService =
>+    do_GetService("@mozilla.org/observer-service;1");
>+  if (observerService) {
>+    observerService->AddObserver(sUploadLastDir, NS_PRIVATE_BROWSING_SWITCH_TOPIC, PR_TRUE);
>+    observerService->AddObserver(sUploadLastDir, "browser:purge-session-history", PR_TRUE);
>+  }

Please add a null check for gUploadLastDir before calling AddObserver on it.  Lke:

  if (observerService && gUploadLastDir) {
    // ...

>+UploadLastDir::UploadLastDir() {
>+  mInPrivateBrowsing = PR_FALSE;

Nit: please use the member initialization syntax.

>-void
>-nsFileControlFrame::StoreLastUsedDirectory(nsIURI* aURI, nsIFile* aFile)
>+nsresult
>+UploadLastDir::StoreLastUsedDirectory(nsIURI* aURI, nsILocalFile* aFile)
> {

It would be good if you can assert that the pointer arguments are not null using NS_PRECONDITION.  And the same for the fetch counterpart as well.

>+  mockFilePickerRegisterer.register();
>+  try {
>+    wu.sendMouseEvent("mousedown", rect.left + 5, rect.top + 5, 0, 1, 0);
>+    wu.sendMouseEvent("mouseup", rect.left + 5, rect.top + 5, 0, 1, 0);
>+  } catch (e) { alert (e);

You don't really want an alert here.  Please using Components.utils.reportError instead.

>diff --git a/layout/forms/test/test_bug536567.html b/layout/forms/test/test_bug536567.html
>+<iframe id="content">></iframe>

Nit: extra ">" character.

>+var pbSvc = Cc["@mozilla.org/privatebrowsing;1"].getService(Ci.nsIPrivateBrowsingService);

This will throw in all applications but Firefox, since the private browsing service is only available in Fitefox, and Cc["@mozilla.org/privatebrowsing;1"] is undefined in all other applications.

You may want something like:

  if ("@mozilla.org/privatebrowsing;1" in Cc) {
    // declare pbSvc and proceed
  } else {
    // bail out of the test
  }

>+var tests = [

I like the exhaustive test strategy here a lot!

>+    if (dirs [i].path == event.data) {

Nit: unneeded space before the square bracket.

r=me with the above comments addressed.
Attachment #426175 - Flags: review?(ehsan.akhgari) → review+
(Reporter)

Comment 13

8 years ago
I have also submitted a try server build with the identifier string "geoff".  Please watch it to make sure that all the unit tests pass with your patch.
(Reporter)

Comment 14

8 years ago
The try server builds failed during compilation.  See an example log:

<http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1265820887.1265821631.13939.gz>

The code looks correct.  I'm thinking that maybe the try web interface can't handle patches with hg moves.  I'll push another try server build from hg to see if it fixes things.
(Reporter)

Comment 15

8 years ago
The new try server identifier is 6746b501885c.
(Assignee)

Comment 16

8 years ago
Created attachment 426329 [details] [diff] [review]
patch v5
Attachment #426175 - Attachment is obsolete: true
(Reporter)

Comment 17

8 years ago
The try server run completed successfully on Linux and Mac, and had some random known failures on Windows <http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1265824911.1265836165.19832.gz>.  So, this patch is safe to land.
(Reporter)

Comment 18

8 years ago
Comment on attachment 426329 [details] [diff] [review]
patch v5

>+  } else if (test == "pb on") {
>+    if (!pbSvc) {
>+      // private browsing service not available, finish test
>+      SimpleTest.finish();
>+      return;
>+    }

This bypasses the cleanups needed at the end of test.  Please add a cleanup function to encapsulate those operations, and call it whenever the test needs to finish.
(Assignee)

Comment 19

8 years ago
Created attachment 426361 [details] [diff] [review]
patch v5.1

So it does.
Attachment #426329 - Attachment is obsolete: true
(Reporter)

Updated

8 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/cbb7edcc6d77
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a2
(Assignee)

Comment 21

8 years ago
Created attachment 427304 [details] [diff] [review]
startup fix

I realised I could make a tiny improvement in startup time by calling InitUploadLastDir only when needed. I know this bug is closed but it's hardly worth creating a new one.
Attachment #427304 - Flags: review?(ehsan.akhgari)
(Reporter)

Updated

8 years ago
Attachment #427304 - Flags: review?(ehsan.akhgari) → review+
(Reporter)

Updated

8 years ago
Keywords: checkin-needed
Created attachment 431910 [details] [diff] [review]
(Dv1-SM) Port it to SeaMonkey
[Moved to bug 506493]
Attachment #431910 - Flags: review?(bugspam.Callek)
Depends on: 546484
(Reporter)

Comment 24

8 years ago
Comment on attachment 431910 [details] [diff] [review]
(Dv1-SM) Port it to SeaMonkey
[Moved to bug 506493]

Could you move this to a separate bug, please?  This bug is already confusing in that multiple patches had landed in it.

Thanks!
Attachment #431910 - Attachment is obsolete: true
Attachment #431910 - Flags: review?(bugspam.Callek)
(Reporter)

Comment 25

8 years ago
Serge, please see comment 24.
Blocks: 506493
Attachment #431910 - Attachment description: (Dv1-SM) Port it to SeaMonkey → (Dv1-SM) Port it to SeaMonkey [Moved to bug 506493]
No longer depends on: 546484
You need to log in before you can comment on or make changes to this bug.