Closed
Bug 1002229
Opened 10 years ago
Closed 10 years ago
Add anonymous parameter to Request API
Categories
(Add-on SDK Graveyard :: General, enhancement, P2)
Add-on SDK Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: bruce.bujon, Assigned: bruce.bujon)
Details
(Keywords: dev-doc-needed)
Attachments
(1 file)
Hi, I propose an update of the Request module. I would like to add an anon parameter bound on the mozAnon parameter of the XMLHttpRequest object. It could be use to enable anonymous mode to prevent exposing user crendentials and sending cookies. Thanks.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8413413 -
Flags: review?(tomica+amo)
Is this something we want to do, Irakli, or should this just use xhr?
Flags: needinfo?(rFobic)
Comment 3•10 years ago
|
||
In SDK we do not usually abbreviate names. I'm ok with exposing this feature, but Bruce could you please rename option to `anonymous` instead ? Thanks
Flags: needinfo?(rFobic)
Assignee | ||
Comment 4•10 years ago
|
||
Absolutely ! I will rename the option to « anonymous » as proposed.
Priority: -- → P2
Comment 5•10 years ago
|
||
Comment on attachment 8413413 [details] [review] Pull request this looks good, but i'm gonna r- because i would like some functional tests. you are changing how every XHR object is created (line 70), so i would like a test that ensures this doesn't break existing functionality, plus a test that this param actually works, and that XHR doesn't send (previously existing) cookies when anonymous is set. i see there are other cookie-related tests, so i'm assuming this is possible with our current test harness. it could go something like this: a) mock up (or find) a test-url that sends a cookie in a response b) issue a first request that should receive (and store) the cookie c) issue a second request and verify that XHR is sending the cookie d) issue an anonymous third request and verify that XHR is not sending it
Attachment #8413413 -
Flags: review?(tomica+amo) → review-
Updated•10 years ago
|
Assignee: nobody → bruce.bujon
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 6•10 years ago
|
||
I agree with the need of functional tests. But the fact is I never succeed to pass any test from github master. I tried on my Win7 and Debian desktops (even on a Win8.1..) Never the cfx testxxx commands end. I really would like to contribute but I already spent many hours without having a working test environment. And MDN and Google did not help me until now..
Comment 7•10 years ago
|
||
> I already spent many hours without having a working test environment. sorry to hear that. you should feel free to ask whenever you get stuck, or for a quicker response, you can join us in the #jetpack channel on irc.mozilla.org. > Never the cfx testxxx commands end. first of all, you should run tests with Nightly version of firefox, which is controlled with the cfx -b argument. second, you can limit to running only unit tests for the module you are changing if your changes are unlikely to break other parts of the SDK (which is the case here). and third, -v flag turns on verbose mode, which makes it easier to tell what's going on.. so try with this command line: cfx testpkgs -f request -v -b /path/to/nightly/firefox.exe and again, thanks, and ask if i didn't explain something clearly! ;)
Assignee | ||
Comment 8•10 years ago
|
||
A big thanks for your reply ! I fix remarks made on GitHub and add new functional tests using the scenario described above. I hope it matches what you expected.
Updated•10 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Updated•10 years ago
|
Summary: Add anon parameter to Request API → Add anonymous parameter to Request API
Updated•10 years ago
|
Attachment #8413413 -
Flags: review- → review?(tomica+amo)
Comment 9•10 years ago
|
||
Comment on attachment 8413413 [details] [review] Pull request great work Bruce! r+ thank you for this, and sorry for the delayed review. i left some minor comments in the PR, it would be nice if you could address them (plus one left from the first review, about the mozAnon property). after that, please squash all your commits into one (using rebase), we prefer to have PRs correspond to single commits, to make it easier to track them. just post a comment here when you are done, and i'll merge it!
Attachment #8413413 -
Flags: review?(tomica+amo) → review+
Assignee | ||
Comment 10•10 years ago
|
||
I stuck about getting the mozAnon property from the XHR object. See my reply about the inline comment (https://github.com/mozilla/addon-sdk/pull/1477#discussion_r13260959). Otherwise, I fixed all other remarks and squashed my commits.
Assignee | ||
Comment 11•10 years ago
|
||
The last remark was fixed. Thanks ! Feel free to merge it ;)
Comment 12•10 years ago
|
||
Commit pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/d347cb5b97f54aae4e68c3ee6d718bf72f2a02b1 Merge pull request #1477 from PerfectSlayer/anon-request Bug 1002229 - Add anon parameter to Request API, r=@zombie
Comment 13•10 years ago
|
||
good perseverance Bruce! ;) if you are interested in contributing some more, here is a list of other good starting bugs: https://bugzilla.mozilla.org/buglist.cgi?status_whiteboard=[good+first+bug]&product=Add-on+SDK
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 14•10 years ago
|
||
You're welcome ! My first contribution to a Mozilla project ;) I went one step further =D (I only "use" SDK until now) Thanks for the link. I will have a look at it if I found spare time.
You need to log in
before you can comment on or make changes to this bug.
Description
•