Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TIMOB-2849 Android: Set-Cookie Response Headers Only Returns Max of One Cookie #474

Merged
merged 11 commits into from Oct 5, 2011

Conversation

ayeung
Copy link
Contributor

@ayeung ayeung commented Sep 19, 2011

Added a shared cookie store for TiHTTPClient.

For testing: use the original app.js snippet from the bug description. To test the persistent cookiestore functionality do the following:

  1. Launch the app.js in the bug description
  2. Click on Get or Set Cookies (it should list 2 cookies that were set)
  3. Close and relaunch the app
  4. Click on clear cookies
  5. Verify that it says something like "Set 2 cookies to expire a year ago"

@@ -0,0 +1,194 @@
/**
* Appcelerator Titanium Mobile
* Copyright (c) 2009-2010 by Appcelerator, Inc. All Rights Reserved.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update Copyright to 2011

@donthorp
Copy link
Contributor

Code Reviewed. Request Accepted

@billdawson
Copy link
Contributor

I think this needs to be simplified. TiCookieStore is only needed once in the application. It should be a singleton with a .getInstance() like TiApplication.getInstance(). Nothing needs to go into the NetworkModule.

When the singleton TiCookieStore needs a context for .getSharedPreferences(), it can just use the TiApplication's context.

@billdawson
Copy link
Contributor

The manner in which the SharedPreferences store is being used here seems inefficient to me (or I'm missing something.) SharedPreferences is already a key-value store, yet you're forcing all cookie keys to be concatenated together into one preferences key. You already have a store ("TiCookiePreferences") dedicated to just cookies, so why not let the keys of that store be cookie keys, and the values of that store be cookie values?

@ayeung
Copy link
Contributor Author

ayeung commented Oct 3, 2011

I currently have one preference that contains the list of all the cookies keys that are stored in preferences and then for each cookie, I use the cookie key for the preference key, and the cookie value for the value. I need to store the list of all the cookie keys in one preference key, since I don't have a way of knowing the which cookies are in preferences at startup. At startup, I iterate through the list of all cookie keys to populate the cookiestore.

@billdawson
Copy link
Contributor

I'm pretty sure there's a way to get the cookie keys (the keys in the preferences) at startup without storing them all in one. Check the SharedPreferences documentation -- there's some method that gives you everything back as a Map, and then from the Map object you can get keys. (If I remember correctly.) That way we can avoid these string operations like splitting by comma, concatenating, etc.

@billdawson
Copy link
Contributor

Functional test and code review accepted. w00t!

Don needs to re-code-rev.

@donthorp
Copy link
Contributor

donthorp commented Oct 5, 2011

Code Reviewed. Request Accepted.

donthorp added a commit that referenced this pull request Oct 5, 2011
TIMOB-2849 Android: Set-Cookie Response Headers Only Returns Max of One Cookie
@donthorp donthorp merged commit 249020e into tidev:master Oct 5, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants