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-14224: Add system-wide cookie store for httpclient #4549

Merged
merged 4 commits into from Aug 23, 2013

Conversation

hieupham007
Copy link
Contributor

Testing steps in JIRA.

@ghost ghost assigned pingwang2011 Aug 19, 2013
@@ -475,6 +480,7 @@ public TiHTTPClient(KrollProxy proxy)
{
this.proxy = proxy;
this.client = getClient(false);
this.client.setCookieStore(cookieStore);
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually getClient() is called in two different functions, so it's not enough just set cookie store here.
Since the cookie store is associated with the client, it is better to set it in getClient() when the http client is created.

@@ -1265,11 +1273,10 @@ public void progress(int progress) {
}

Log.d(TAG, "Preparing to execute request", Log.DEBUG_MODE);

Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary white spaces.

@pingwang2011
Copy link
Contributor

Ran the test case. I clicked "Download", then "Abort", and then "Download" again, everything works fine. But when I clicked "Abort" again, it still kept downloading. Seems it can not be aborted the second time.

@pingwang2011
Copy link
Contributor

FR passed. Code reviewed and left a minor comment.

@pingwang2011
Copy link
Contributor

CR & FR passed. Accepted

pingwang2011 added a commit that referenced this pull request Aug 23, 2013
timob-14224: Add system-wide cookie store for httpclient
@pingwang2011 pingwang2011 merged commit c9a03d8 into tidev:master Aug 23, 2013
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

2 participants