Add a list of recently-opened files to the file menu of the Scratchpad

RESOLVED FIXED in Firefox 16

Status

defect
RESOLVED FIXED
8 years ago
a year ago

People

(Reporter: rcampbell, Assigned: johan.charlez)

Tracking

unspecified
Firefox 16
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 10 obsolete attachments)

Reporter

Description

8 years ago
The workspace should maintain a persistent list of 5 (to 10 or so) recently-opened files in the File menu. Accessing these items will reload the given file from its location on disk.
Reporter

Updated

8 years ago
Whiteboard: [workspace]
Reporter

Updated

8 years ago
Summary: Add a list of recently-opened files to the file menu of the workspace → Add a list of recently-opened files to the file menu of the Scratchpad
Whiteboard: [workspace] → [scratchpad][good-first-bugs]
Assignee

Updated

7 years ago
Assignee: nobody → johan.charlez
Assignee

Comment 1

7 years ago
Posted patch patch 0.1 (obsolete) — Splinter Review
* Added a 'Clear Items' menuitem for clearing the list.
* Added a pref for controlling how many recently opened files should be stored.
Attachment #602946 - Flags: feedback?(rcampbell)
Assignee

Updated

7 years ago
Status: NEW → ASSIGNED
Component: Developer Tools → Developer Tools: Scratchpad
QA Contact: developer.tools → developer.tools.scratchpad
Whiteboard: [scratchpad][good-first-bugs] → [good first bug]
Rob, this patch has been waiting for feedback for some time now.
Reporter

Comment 3

7 years ago
yes. I've been traveling and have talked to Johan about it. Thanks for your help.
Reporter

Comment 4

7 years ago
Comment on attachment 602946 [details] [diff] [review]
patch 0.1

+pref("devtools.scratchpad.recentlyOpened.max-recent", 10);

that preference name is a little ungainly. How about changing that to:

devtools.scratchpad.recentfiles ?

+  setRecentFiles: function SP_setRecentFiles(aFile) {
+    let maxRecent = Services.prefs.getIntPref(PREF_RECENTLY_OPENED_MAX);
+    let aFiles = this.getRecentFiles();
+    let aFilesLength = aFiles.length;

we usually use the aParameter naming conventions for function/method parameters. I'd probably lose the "a" prefix on these variable names.

+    let branch = Services.prefs.getBranch("devtools.scratchpad.recent-files.list.");

recentfileslist.?

+    if (aFilesLength < maxRecent) {
+      branch.setComplexValue("file"+aFilesLength, Ci.nsILocalFile, aFile);
+    }

trailing space after that brace. Careful with those!

"file" + aFilesLength should have spacing around the + operator.

+    else {
+      // moves every file down, overwriting the oldest recent.
+      for (let i = 1; i < aFilesLength; i++)
+        branch.setComplexValue("file"+(i-1), Ci.nsILocalFile, aFiles[i]);
+
+      branch.setComplexValue("file"+(maxRecent-1), Ci.nsILocalFile, aFile);
+    }
+  },

I think I'd rather see you retrieve the full list of files first and then write them out in one shot rather than doing the reading and writing at the same time. A personal preference but I think it'll look cleaner.

You also get to reuse getRecentFiles()!

in +  getRecentFiles: function SP_getRecentFiles() {

+    while (!error) {
+      try {
+        let file = branch.getComplexValue("file"+i, Ci.nsILocalFile);
+        files.push(file);
+        i++;
+      }
+      catch (err) {
+        error = true;
+      }
+    }

move your try/catch outside of the while loop.

+        menuitem.setAttribute("oncommand", "Scratchpad.openRecentFile("+i+");");

spaces around operators, please.

I think the overall approach is reasonable. Just a few stylistic nits.

Writing a unittest for this is going to be a Blast! ;)
Attachment #602946 - Flags: feedback?(rcampbell) → feedback+
Assignee

Comment 5

7 years ago
Posted patch patch 0.2 (obsolete) — Splinter Review
No test yet, but I have reworked the patch a bit, could you take another look?

I found a better solution for 'SP_getRecentFiles', the try-catch method required a lot of additional code to fix the following bugs.

Fixed bugs:
* If the "recentfiles"-pref has been set to a number that is less than the number of files currently stored, the oldest files are removed and the list is reordered.
* If a "file-pref" (e.g. "...recentfileslist.3") has been reset and it's not the most recently opened file, the list is reordered.

Added:
* When opening a file from the 'Open Recent' menu, set it as the most recently opened file.
* Mark file currently opened with checked="true".

Fixed a few typos.
Fixed typo: SP_getState -> SP_setState.
Attachment #602946 - Attachment is obsolete: true
Attachment #613654 - Flags: feedback?(rcampbell)
Reporter

Comment 6

7 years ago
Comment on attachment 613654 [details] [diff] [review]
patch 0.2

+  /**
+   * Save a recently opened file as a complexValue.
+   *
+   * @param nsILocalFile file
+   *        The file we want to store as a recently opened file.
+   *
+   * @return bool conditional
+   *         Returns false if the file already exists in the list of
+   *         recently opened files.

nit: don't usually have extra linebreaks between @param and @return

in setRecentFile:

+  setRecentFile: function SP_setRecentFile(file) {

parameter names should have an "a" prefix (for "argument"). It's a silly convention we use.

+    // If the file is already in the list, set it as the most recent.
+    for (let i = 0; i < filesCount; i++) {
+      if (files[i].path === file.path) {
+        for (let j = i + 1; j < filesCount; j++) {
+          branch.setComplexValue(j - 1, Ci.nsILocalFile, files[j]);
+        }
+
+        branch.setComplexValue(filesCount - 1, Ci.nsILocalFile, file);
+        return false;
+      }
+    }

not loving this nested loop.

For one, it's doing more than just "setting this file as the most recent". It's also renumbering the rest of the branch.

what about,

let paths = files.map(function(file) { return file.path; });
let pathIndex = paths.indexOf(aFile.path);

if (pathIndex < 0) { // path not found
  if (files.length == maxRecent)
    files.pop(); // remove last file on the array
} else // path found, remove it
  files.splice(pathIndex, 1);

files.unshift(aFile); // add the file to the front of the array

files.forEach(function(file, index) {
  branch.setComplexValue(index, Ci.nsILocalFile, file);
});

I think that covers the whole thing. What do you think?
Attachment #613654 - Flags: feedback?(rcampbell)
Assignee

Comment 7

7 years ago
Posted patch patch 0.3 tests wip 1 (obsolete) — Splinter Review
Updated per your feedback, thanks!

+ Add the file to our list of recent files on "Save" and "Save as".
+ First version with tests.

What do you think? \o/ tests \o/
Attachment #613654 - Attachment is obsolete: true
Attachment #616645 - Flags: feedback?(rcampbell)
Assignee

Comment 8

7 years ago
Posted patch patch 0.3 tests wip 1 (obsolete) — Splinter Review
Silly me, forgot to add the test file.

Se comment above for notes on patch.
Attachment #616645 - Attachment is obsolete: true
Attachment #616645 - Flags: feedback?(rcampbell)
Attachment #616648 - Flags: feedback?(rcampbell)
Reporter

Comment 9

7 years ago
Comment on attachment 616648 [details] [diff] [review]
patch 0.3 tests wip 1

The test is a good start. It tests the new methods you've created to setRecentFile and queries the contents of the recent files menu. That's good stuff. Do we have any file open and save tests anywhere you could query the contents of the recent files menu as well?

good progress!
Assignee

Comment 10

7 years ago
Posted patch patch 0.4 (obsolete) — Splinter Review
+ Test for menuitems in menu.
Attachment #616648 - Attachment is obsolete: true
Attachment #616648 - Flags: feedback?(rcampbell)
Attachment #617672 - Flags: review?(rcampbell)
Reporter

Comment 11

7 years ago
Comment on attachment 617672 [details] [diff] [review]
patch 0.4

+  /**
+   * Open recently opened file.
+   * 
+   * @param integer index
+   *        Clicked menuitem in the 'Open Recent' menu.
+   */
+  openRecentFile: function SP_openRecentFile(index) {
+    let files = this.getRecentFiles();
+    this.setFilename(files[index].path);
+    this.importFromFile(files[index], false);
+    this.setRecentFile(this.getRecentFiles()[index]);
+  },

+    this.setRecentFile(this.getRecentFiles()[index]);

is there a reason you don't just setRecentFile(files[index])?

You could also dereference the file in question on your first call to getRecentFiles() and get:

+  openRecentFile: function SP_openRecentFile(index) {
+    let file = this.getRecentFiles()[index];
+    this.setFilename(file.path);
+    this.importFromFile(file, false);
+    this.setRecentFile(file);
+  },

in setRecentFile():

could you add some comments around the if... else and before the forEach to explain what you're doing? (if not found, remove the last file, otherwise, remove it from the list, etc)

in getRecentFiles():

+    let diff = (files.length - maxRecent >= 0) ? files.length - maxRecent : 0;

add a comment for this please.

(number of files is greater than max recent files? how does that happen?)

+    for (let i = 0; i < files.length; i++) {

explain what this is for, please.

This seems to be constructing a list of files. Starting with getting the childList from the branch, and then sorting it.

Then iterating through all the files, getting each one, removing it from the branch, but if the index is greater than the diff value, reinserting it and adding it to the sorted list.

Very confusing. I think this method should be clearer and not do any restructuring of the preference branch at all since you'd expect it to retrieve values. It's generally a bad idea to modify storage when you're retrieving something.

+    } else {
+      recentFilesMenu.setAttribute("disabled", true);
+    }

Is this redundant? Menu's disabled by default, right? If there are no items in the recently opened list, it won't be enabled?

+  clearRecent: function SP_clearRecent()

This doesn't update the menu does it?

in scratchpad.xul:

+        <menupopup id="sp-menu-open_recentPopup">
+        </menupopup>

you can just use a /> to close the menupopup and ditch </menupopup>.

canceling review.
Attachment #617672 - Flags: review?(rcampbell)
Assignee

Comment 12

7 years ago
Reworking the patch to populate the menu when Scratchpad is being opened, and to make use of an observer for preference changes.
Assignee

Comment 13

7 years ago
Posted patch patch 0.5 (obsolete) — Splinter Review
Reworked patch according to your feedback. It's now using a preference observer to repopulate the menu and for clearing items from the menu, as well as hiding/unhiding/disabling/enabling the menu. Furthermore, the menu is now populated when Scratchpad is opened.

Modified the tests to work with the reworked methods, and added new tests to further test the observer. I'm not sure about the tests however, setTimeouts are necessary to make sure the preference observer has fired before running the appropriate test, but I'm not sure if this is the best solution.

In test06 for example I have to call the populateRecentFilesMenu method even though I have a 100ms delay between clearing all files and running the tests in test06. Also, for some reason I have to call the removeRecentFilesMenuitem method to hide the 'Open Recent'-menu, even though it should already be hidden, as the menu is empty.

Any feedback is greatly appreciated.
Attachment #617672 - Attachment is obsolete: true
Attachment #624632 - Flags: feedback?(rcampbell)
Assignee

Updated

7 years ago
Blocks: 756501
Reporter

Comment 14

7 years ago
Comment on attachment 624632 [details] [diff] [review]
patch 0.5

I think this is good, Johan. I'm going to ask Heather to have a pass through this.
Attachment #624632 - Flags: feedback?(rcampbell)
Attachment #624632 - Flags: feedback?(fayearthur)
Attachment #624632 - Flags: feedback+
Comment on attachment 624632 [details] [diff] [review]
patch 0.5

Review of attachment 624632 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch for this much-needed feature! It looks good. I just have some nits and a couple suggestions. As for the setTimeout()s in the test, they should be avoided. Maybe you could observe the pref change in the test.

::: browser/devtools/scratchpad/scratchpad-manager.jsm
@@ +205,5 @@
> +    if (aData == "recentFilesMax") {
> +      let enumerator = Services.wm.getEnumerator("devtools:scratchpad");
> +      while (enumerator.hasMoreElements()) {
> +        let win = enumerator.getNext();
> +        win.Scratchpad.handleRecentFileMaxChange();

I'm more for observing the preference change from each scratchpad window, e.g. having the PreferenceObserver live in scratchpad.js. Less micromanaging for the scratchpad manager.

@@ +209,5 @@
> +        win.Scratchpad.handleRecentFileMaxChange();
> +      }
> +    }
> +    else if (aData.match(/^recentfileslist.([0-9]+)$/)) {
> +      let prefName = aData.match(/^recentfileslist.([0-9]+)$/)[1];

put `aData.match(...)` in a var so you don't have to change it in two places, and to be more descriptive.

::: browser/devtools/scratchpad/scratchpad.js
@@ +789,5 @@
> +      let file = branch.getComplexValue(prefNames[i], Ci.nsILocalFile);
> +      files.push(file);
> +    }
> +
> +    return [files, prefNames];

It seems like a lot of this code could be avoided by just storing the list as an JSON-parseable array of file paths in a string pref. 

To update the list you could parse the preference, update the array and write it back to the preference.

::: browser/devtools/scratchpad/test/browser_scratchpad_bug_651942_recent_files.js
@@ +29,5 @@
> +let gFileContent02 = "hello.world.02('bug651942');";
> +let gFileContent03 = "hello.world.03('bug651942');";
> +let gFileContent04 = "hello.world.04('bug651942');";
> +
> +function p(objects) {

Use a more descriptive function name here than just "p". Is this an unused function? If so, take it out.

@@ +41,5 @@
> +function runTest01()
> +{
> +  gScratchpad = gScratchpadWindow.Scratchpad;
> +
> +  gFile01 = createTemporaryFile(gFile01, gFileName01, gFileContent01);

The name "createTemporaryFile" only describes part of what that function does, I think the more important part to describe is that the function loads the file into a scratchpad.

@@ +45,5 @@
> +  gFile01 = createTemporaryFile(gFile01, gFileName01, gFileContent01);
> +  gFile02 = createTemporaryFile(gFile02, gFileName02, gFileContent02);
> +  gFile03 = createTemporaryFile(gFile03, gFileName03, gFileContent03);
> +
> +  setTimeout(runTest02, 100);

Don't use `setTimeout()`. The test could fail on slower systems. Add a callback argument to `createTemporaryFile()` to be called when the file has been saved.

You'll have to chain the opening, so you could either just test opening two files to keep it readable (probably enough anyways), or use a kind of async utility function like the one here: https://github.com/caolan/async#series.

@@ +48,5 @@
> +
> +  setTimeout(runTest02, 100);
> +}
> +
> +function runTest02()

you could call this something more descriptive, like "testOverwriteRecent".

@@ +63,5 @@
> +    runTest03(recentFiles);
> +  }, 100);
> +}
> +
> +function runTest03(recentFiles)

More descriptive function name.

@@ +67,5 @@
> +function runTest03(recentFiles)
> +{
> +  let [recentFiles02, prefNames02] = gScratchpad.getRecentFiles();
> +
> +  is(recentFiles02[0].path, recentFiles[1].path, "File01: Success");

Something more telling than "Success". I can't tell what this is testing, maybe "File01 path is correct".

@@ +75,5 @@
> +  gScratchpad.openRecentFile(prefNames02[0]);
> +
> +  setTimeout(function() {
> +    runTest04(recentFiles02);
> +  }, 100);

Same here, no `setTimeout()`, use callbacks.

@@ +78,5 @@
> +    runTest04(recentFiles02);
> +  }, 100);
> +}
> +
> +function runTest04(recentFiles02)

More descriptive function name.

@@ +94,5 @@
> +  gScratchpad.clearRecentFiles(prefNames03[1]);
> +
> +  setTimeout(function() {
> +    runTest05([recentFiles03[0].path, recentFiles03[1]])
> +  }, 100);

no `setTimeout()`

@@ +97,5 @@
> +    runTest05([recentFiles03[0].path, recentFiles03[1]])
> +  }, 100);
> +}
> +
> +function runTest05(pathsForTheFilesWeCleared)

More descriptive function name.

"pathsForTheFilesWeCleared" is too long, maybe "clearedFiles".

@@ +118,5 @@
> +  isnot(matchFound, true,
> +        "The files were successfully removed from the 'Open Recent'-menu");
> +  gScratchpad.clearRecentFiles();
> +
> +  setTimeout(runTest06, 100);

No `setTimeout()`

@@ +121,5 @@
> +
> +  setTimeout(runTest06, 100);
> +}
> +
> +function runTest06()

More descriptive name.

@@ +136,5 @@
> +  ok(menu.hasAttribute("disabled"), "No files in the menu, it was disabled successfully.");
> +
> +  Services.prefs.setIntPref("devtools.scratchpad.recentFilesMax", 0);
> +
> +  setTimeout(runTest07, 100);

No `setTimeout()`.

@@ +167,5 @@
> +
> +  NetUtil.asyncCopy(fileContentStream, fout);
> +
> +  gScratchpad.setFilename(aFile.path);
> +  gScratchpad.importFromFile(aFile.QueryInterface(Ci.nsILocalFile),  true,

It looks like we're not waiting for the copying to finish before opening the file here. Pass a callback to asyncCopy and open the file in the callback.

@@ +193,5 @@
> +{
> +  let doc = gScratchpadWindow.document;
> +
> +  is(doc.getElementById("sp-menu-open_recentPopup").children.length,
> +     gScratchpad.getRecentFiles()[0].length + 2,

Might want to make vars for these things you're comparing and explain the "+ 2" part in a comment.

::: browser/locales/en-US/chrome/browser/devtools/scratchpad.dtd
@@ +30,5 @@
>  <!ENTITY openFileCmd.commandkey       "o">
>  
> +<!ENTITY open_recentMenu.label        "Open Recent">
> +<!ENTITY open_recentMenu.accesskey    "R">
> +<!ENTITY clear_recentMenu.label       "Clear Items">

Nit: Looks like the other entities are camel cased without "_", might want to stick to that to be consistent.
Attachment #624632 - Flags: feedback?(fayearthur) → feedback+
Assignee

Comment 16

7 years ago
Posted patch patch 0.6 (obsolete) — Splinter Review
Thanks Heather for the feedback and help.

Changes:
1. Moved the preference observer to scratchpad.js.
2. Removed the 'removeRecentFilesMenuitem'-method.
3. Updated and improved the test, it is now utilising a preference observer.
4. Reworked the patch to save file-paths in a JSON parsable string instead of a complexValue.
5. Simplified the preference observer a bit.
6. Fixed other nits and bits per your feedback.
Attachment #624632 - Attachment is obsolete: true
Attachment #629531 - Flags: feedback?(fayearthur)
Assignee

Comment 17

7 years ago
^ In addition I added my patch for bug 756501.
Comment on attachment 629531 [details] [diff] [review]
patch 0.6

Looks great to me, sorry for the delay!
Attachment #629531 - Flags: feedback?(fayearthur) → feedback+
Assignee

Comment 19

7 years ago
Posted patch patch 0.6 rebased (obsolete) — Splinter Review
(In reply to Heather Arthur [:harth] from comment #18)
> Comment on attachment 629531 [details] [diff] [review]
> patch 0.6
> 
> Looks great to me, sorry for the delay!

No problem, cheers!

Rebased the patch.
Attachment #629531 - Attachment is obsolete: true
Attachment #634429 - Flags: review?(fayearthur)
Comment on attachment 634429 [details] [diff] [review]
patch 0.6 rebased

Review of attachment 634429 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for rebasing, I think you forgot to add the test file though (: Can you upload a new patch with the test and also take out the code for bug 756501, I actually think we should do that functionality separately. Just a couple more small things while we're at it:

::: browser/devtools/scratchpad/scratchpad.js
@@ +662,5 @@
> +        let filePath = this.getRecentFiles()[aIndex];
> +        file.initWithPath(filePath);
> +        this.importFromFile(file, false);
> +        this.setFilename(file.path);
> +        this.setRecentFile(file);

It looks like you might end up duplicating many of these steps in both openRecentFile() and openFile(). Use your judgement, but a function with any shared code would be great.

@@ +738,5 @@
> +    filePaths.push(aFile.path);
> +
> +    let branch = Services.prefs.
> +                 getBranch("devtools.scratchpad.");
> +    branch.setCharPref("recentFilePaths", JSON.stringify(filePaths));

Maybe return true here if you've specified the return is a boolean up there.

@@ +751,5 @@
> +    let recentFilesMenu = document.getElementById("sp-open_recent-menu");
> +
> +    if (maxRecent < 1) {
> +      recentFilesMenu.setAttribute("hidden", true);
> +      return false;

just "return;" here if you won't be using the return value.
(In reply to Heather Arthur [:harth] from comment #20)
> 
> ::: browser/devtools/scratchpad/scratchpad.js
> @@ +662,5 @@
> > +        let filePath = this.getRecentFiles()[aIndex];
> > +        file.initWithPath(filePath);
> > +        this.importFromFile(file, false);
> > +        this.setFilename(file.path);
> > +        this.setRecentFile(file);
> 
> It looks like you might end up duplicating many of these steps in both
> openRecentFile() and openFile(). Use your judgement, but a function with any
> shared code would be great.

Actually, if you do this shared function, you could probably add in the "savePrompt" code that fixes bug 756501 too, since it seems more of a necessity than functionality addition.
Assignee

Comment 22

7 years ago
Posted patch patch 0.6.1 (obsolete) — Splinter Review
(In reply to Heather Arthur [:harth] from comment #20)
> Comment on attachment 634429 [details] [diff] [review]
> patch 0.6 rebased
> 
> Review of attachment 634429 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for rebasing, I think you forgot to add the test file though (:

Sorry, I'm not sure how that happened.

> @@ +738,5 @@
> > +    filePaths.push(aFile.path);
> > +
> > +    let branch = Services.prefs.
> > +                 getBranch("devtools.scratchpad.");
> > +    branch.setCharPref("recentFilePaths", JSON.stringify(filePaths));
> 
> Maybe return true here if you've specified the return is a boolean up there.

I removed the specification and changed all returns in this method to "return;".

> @@ +751,5 @@
> > +    let recentFilesMenu = document.getElementById("sp-open_recent-menu");
> > +
> > +    if (maxRecent < 1) {
> > +      recentFilesMenu.setAttribute("hidden", true);
> > +      return false;
> 
> just "return;" here if you won't be using the return value.

Done.

(In reply to Heather Arthur [:harth] from comment #21)
> (In reply to Heather Arthur [:harth] from comment #20)
> > 
> > ::: browser/devtools/scratchpad/scratchpad.js
> > @@ +662,5 @@
> > > +        let filePath = this.getRecentFiles()[aIndex];
> > > +        file.initWithPath(filePath);
> > > +        this.importFromFile(file, false);
> > > +        this.setFilename(file.path);
> > > +        this.setRecentFile(file);
> > 
> > It looks like you might end up duplicating many of these steps in both
> > openRecentFile() and openFile(). Use your judgement, but a function with any
> > shared code would be great.
> 
> Actually, if you do this shared function, you could probably add in the
> "savePrompt" code that fixes bug 756501 too, since it seems more of a
> necessity than functionality addition.

I went with this. If you could take a closer look at the "openFile"-method, that would be great.

Cheers :)
Attachment #634429 - Attachment is obsolete: true
Attachment #634429 - Flags: review?(fayearthur)
Attachment #635084 - Flags: review?(fayearthur)
Comment on attachment 635084 [details] [diff] [review]
patch 0.6.1

Looks good. This is awesome, thanks for the patch and updates!
Attachment #635084 - Flags: review?(fayearthur) → review+
Comment on attachment 635084 [details] [diff] [review]
patch 0.6.1

>diff --git a/browser/devtools/scratchpad/scratchpad.js b/browser/devtools/scratchpad/scratchpad.js

>+  setRecentFile: function SP_setRecentFile(aFile)

>+    let paths = filePaths.map(function(file) { return file; });

Drive-by super-nit that I just happened to notice: this map call seems to be a no-op?

Also a thought that occurred to me when thinking about the feature in general: should we avoid saving recently opened files while in private browsing mode? Kind of an edge case, but also relatively easy to fix. Can do that in a followup though.
Reporter

Comment 25

7 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #24)
> Comment on attachment 635084 [details] [diff] [review]
> patch 0.6.1
> 
> >diff --git a/browser/devtools/scratchpad/scratchpad.js b/browser/devtools/scratchpad/scratchpad.js
> 
> >+  setRecentFile: function SP_setRecentFile(aFile)
> 
> >+    let paths = filePaths.map(function(file) { return file; });
> 
> Drive-by super-nit that I just happened to notice: this map call seems to be
> a no-op?

good catch! paths == filePaths.

> Also a thought that occurred to me when thinking about the feature in
> general: should we avoid saving recently opened files while in private
> browsing mode? Kind of an edge case, but also relatively easy to fix. Can do
> that in a followup though.

does this really apply? PrivateBrowsing doesn't necessarily imply PrivateScratchpads.
But what if I'm scratchpadding the page where I'm buying a wedding ring?!1

We just generally seem to avoid persisting any state while in PB mode, particularly if it can be site-related, so it seemed like something we may want to do here. But certainly no need to block on it now; can discuss it with private browsing pros in a separate bug.
Assignee

Comment 27

7 years ago
Posted patch patch 0.6.2 (obsolete) — Splinter Review
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #24)
> Comment on attachment 635084 [details] [diff] [review]
> patch 0.6.1
> 
> >diff --git a/browser/devtools/scratchpad/scratchpad.js b/browser/devtools/scratchpad/scratchpad.js
> 
> >+  setRecentFile: function SP_setRecentFile(aFile)
> 
> >+    let paths = filePaths.map(function(file) { return file; });
> 
> Drive-by super-nit that I just happened to notice: this map call seems to be
> a no-op?

Thanks for catching that, fixed.

Rebased and addressed gavin's keen eye, so carrying forward the r+ from the previous attachment.

Thank you Rob, Heather and Gavin for your feedback and patience on this one. :)

This patch includes the fix for bug 756501.
Attachment #635084 - Attachment is obsolete: true
Attachment #636661 - Flags: review+
Assignee

Updated

7 years ago
Whiteboard: [good first bug] → [good first bug][land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/1464fc01cd17
Whiteboard: [good first bug][land-in-fx-team] → [fixed-in-fx-team]
Assignee

Comment 30

7 years ago
Posted patch patch 0.7Splinter Review
* Plugged leaks.
* Fixed some thing I missed.

Thank you again harth and also dcamp for all the help. :)
Attachment #636661 - Attachment is obsolete: true
Attachment #638490 - Flags: review?(fayearthur)
Comment on attachment 638490 [details] [diff] [review]
patch 0.7

looks good, thanks for the fix.
Attachment #638490 - Flags: review?(fayearthur) → review+
https://hg.mozilla.org/mozilla-central/rev/40e232a4acf3
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 16
Assignee

Updated

7 years ago
Depends on: 793031, 777377

Updated

a year ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.