Closed
Bug 797890
Opened 12 years ago
Closed 12 years ago
Add button to load memory reports from the clipboard
Categories
(Toolkit :: about:memory, defect)
Toolkit
about:memory
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)
References
Details
Attachments
(1 file)
7.97 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
This is pretty useful to me in practice!
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #668009 -
Flags: review?(n.nethercote)
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → justin.lebar+bug
Assignee | ||
Comment 2•12 years ago
|
||
I didn't add a test because it seems like way more effort than it's worth (particularly because we're re-using most of the existing show-from-files code), but I'm happy to figure out a test if you think it's worthwhile.
Comment 3•12 years ago
|
||
Comment on attachment 668009 [details] [diff] [review] Patch, v1 Review of attachment 668009 [details] [diff] [review]: ----------------------------------------------------------------- I agree a test isn't necessary. What's the specific use case, BTW? I'm having trouble imagining it. ::: toolkit/components/aboutmemory/content/aboutMemory.js @@ +443,4 @@ > reader.readAsText(aFile); > > } catch (ex) { > + let body = clearBody(); Would it be better to move this to the top of the try block (where it was) and then pass |body| to updateAboutMemoryFromJSONString()? Not sure, I'll let you decide. @@ +457,5 @@ > +{ > + // Get the clipboard's contents. > + let cb = Cc["@mozilla.org/widget/clipboard;1"] > + .getService(Components.interfaces.nsIClipboard); > + let xferable = Cc["@mozilla.org/widget/transferable;1"] |xferable| strikes me as a very un-jlebar-ish variable name, but I'll allow it :)
Attachment #668009 -
Flags: review?(n.nethercote) → review+
Assignee | ||
Comment 4•12 years ago
|
||
> What's the specific use case, BTW? I'm having trouble imagining it. e.g. cjones posts a memory report dump in bug 797189. I can very quickly open it up, copy it to my clipboard, and click the button in about:memory. Saving it to a file and then picking the file is much slower. > |xferable| strikes me as a very un-jlebar-ish variable name, but I'll allow it :) Wow, was it that obvious I copied this code??
Comment 5•12 years ago
|
||
Is the input here properly sanitized for realz? The easier we make it to input random stuff into about:memory the closer we are to some kind of XSS thing. Now, admittedly this still requires the user cutting and pasting, but it is better to be cautious.
Comment 6•12 years ago
|
||
> Is the input here properly sanitized for realz?
Yes: it goes through JSON.parse.
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #6) > > Is the input here properly sanitized for realz? > > Yes: it goes through JSON.parse. And we never set innerHTML, so we're good in theory. It strikes me that for the use-case I'm interested in, being able to specify a URL to fetch the about:memory dump from would be particularly convenient. That way, we could fetch a compressed dump, whereas right now, if I want to be able to use this convenient copy/paste thing, I have to post uncompressed dumps to bugzilla, which are obscenely large. But that's for a separate bug.
Assignee | ||
Comment 8•12 years ago
|
||
> Would it be better to move this to the top of the try block (where it was) and then pass |body| to
> updateAboutMemoryFromJSONString()? Not sure, I'll let you decide.
I was on the fence about this, but I think it's maybe better how it is because functions with fewer arguments are better, and functions with fewer assumptions are better. (Otherwise updateAboutMemoryFromJSONString would assume that the |body| it got had just been cleared.)
Comment 9•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/aa211d740d88
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in
before you can comment on or make changes to this bug.
Description
•