Closed Bug 463772 Opened 11 years ago Closed 11 years ago

Make choice of skipping prompt for getTargetFile obvious

Categories

(Toolkit :: Downloads API, defect)

x86
Windows XP
defect
Not set

Tracking

()

VERIFIED FIXED

People

(Reporter: Callek, Assigned: Callek)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
This was brought about in my audit of the call-tree for "internalSave". Many functions were passing null (undefined) to it by never actually giving an argument up the ladder...

This turns those call sites into "false". I also fixed a potential future headache by having the context menu use saveFrame directly from gContextMenu instead of having two code-paths (that are synced).
Attachment #347032 - Flags: review?(sdwilsh)
its an optional param, this is fine.  what are you really trying to fix here?
I just found the code easier to read and understand when we made this param explicit. If its best as WONTFIX in your opinion, no love lost ;-)

The one other thing I did do in this patch (which I can seperate out to a new bug if you feel it is worth it) is the contextMenu saveFrame to use gContextMenu instead.
Comment on attachment 347032 [details] [diff] [review]
v1

So, I'd rather see us document the methods better and leave the false out (if it's documented as an optional argument that is).
Attachment #347032 - Flags: review?(sdwilsh)
Attachment #347032 - Attachment is obsolete: true
Attachment #347684 - Flags: review?(sdwilsh)
Attachment #347684 - Flags: review?(sdwilsh) → review+
Comment on attachment 347684 [details] [diff] [review]
v2 -- doc update, and fix context menu path

I hate that version of javadoc comments, but I'm not going to ask you to change it.

r=sdwilsh
Attached patch v2.1Splinter Review
address sdwilsh's super-nit (as discussed in irc). Mostly comment-only but needs approval as there is a minor unrelated code-change involved (to context menu). Definitely trivial.
Attachment #347684 - Attachment is obsolete: true
Attachment #347907 - Flags: review+
Attachment #347907 - Flags: approval1.9.1b2?
Comment on attachment 347907 [details] [diff] [review]
v2.1

a191b2=beltzner
Attachment #347907 - Flags: approval1.9.1b2? → approval1.9.1b2+
$ hg push
pushing to ssh://hg.mozilla.org/mozilla-central/
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 2 changes to 2 files

$ hg tip -v
changeset:   21713:6b19e8291f15
tag:         tip
user:        Justin Wood <Callek@gmail.com>
date:        Sat Nov 15 18:19:10 2008 -0500
files:       browser/base/content/browser-context.inc toolkit/content/contentAreaUtils.js
description:
Bug 463772, Make choice of skipping prompt for getTargetFile obvious (comment changes)
Also include minor context-menu fix.
r=sdwilsh, a1.9.1b2=beltzner
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.