Issue 7033042: Update the appengine code to use OAuth2 and break the gdata dependency.

Issue 7033042: Update the appengine code to use OAuth2 and break the gdata dependency. (Closed)

Created:
9 years, 7 months ago by awong

Modified:
9 years, 7 months ago

Reviewers:
Jamie

CC:
chromium-reviews, jamiewalch+watch_chromium.org, hclam+watch_chromium.org, simonmorris+watch_chromium.org, wez+watch_chromium.org, Erik does not do reviews, dmaclach+watch_chromium.org, garykac+watch_chromium.org, Aaron Boodman, lambroslambrou+watch_chromium.org, pam+watch_chromium.org, ajwong+watch_chromium.org, sergeyu+watch_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

Description

Update the appengine code to use OAuth2 and break the gdata dependency. BUG=none TEST=Connect still works. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86183

Patch Set 1 #

Patch Set 2 : more fixed. #

Total comments: 9

Patch Set 3 : fun fun, fun fun. It's friday, friday. #

Patch Set 4 : remove the hardcoded localhost #

Created: 9 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+205 lines, -279 lines)			Patch
M	remoting/client/appengine/api.py	View	1 2 3	2 chunks	+16 lines, -17 lines	0 comments	Download
M	remoting/client/appengine/app.yaml	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	remoting/client/appengine/auth.py	View	1 2 3	4 chunks	+132 lines, -205 lines	0 comments	Download
D	remoting/client/appengine/chromoting_oauth_setup.html	View		1 chunk	+0 lines, -27 lines	0 comments	Download
M	remoting/client/appengine/chromoting_session.html	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	remoting/client/appengine/hostlist.html	View		2 chunks	+12 lines, -6 lines	0 comments	Download
M	remoting/client/appengine/main.py	View	1	3 chunks	+2 lines, -5 lines	0 comments	Download
M	remoting/client/appengine/static_files/client.js	View	1	3 chunks	+41 lines, -2 lines	0 comments	Download
D	remoting/client/extension/manifest.json	View		1 chunk	+0 lines, -15 lines	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

Jamie

LGTM, with a few questions. Also, since I'm neither a Python nor an OAuth expert, ...

9 years, 7 months ago (2011-05-20 16:46:47 UTC) #2

awong

http://codereview.chromium.org/7033042/diff/2001/remoting/client/appengine/api.py File remoting/client/appengine/api.py (right): http://codereview.chromium.org/7033042/diff/2001/remoting/client/appengine/api.py#newcode41 remoting/client/appengine/api.py:41: self.response.headers['Content-Type'] = 'application/json' On 2011/05/20 16:46:47, Jamie wrote: > ...

9 years, 7 months ago (2011-05-20 20:37:38 UTC) #3

Jamie

9 years, 7 months ago (2011-05-20 20:43:46 UTC) #4

On 2011/05/20 20:37:38, awong wrote:
>
http://codereview.chromium.org/7033042/diff/2001/remoting/client/appengine/ap...
> File remoting/client/appengine/api.py (right):
> 
>
http://codereview.chromium.org/7033042/diff/2001/remoting/client/appengine/ap...
> remoting/client/appengine/api.py:41: self.response.headers['Content-Type'] =
> 'application/json'
> On 2011/05/20 16:46:47, Jamie wrote:
> > In the previous code, the content-type was only set on success. Does this
> > matter?
> 
> Good catch.  Actually, I'm just relaying the full set of headers from the
> googleapi's response below, so this is redundant since they should be setting
> the Content-Type, and we should just be a passthru.
> 
>
http://codereview.chromium.org/7033042/diff/2001/remoting/client/appengine/ap...
> remoting/client/appengine/api.py:42: self.response.out.write(result.content)
> On 2011/05/20 16:46:47, Jamie wrote:
> > Does |write| write the data to the client immediately? If so then don't we
> need
> > to set the status and headers before doing that?
> 
> I'm pretty sure it's not a streaming response.  But just to be sure, I
> reordered.
> 
>
http://codereview.chromium.org/7033042/diff/2001/remoting/client/appengine/au...
> File remoting/client/appengine/auth.py (right):
> 
>
http://codereview.chromium.org/7033042/diff/2001/remoting/client/appengine/au...
> remoting/client/appengine/auth.py:100: 
> On 2011/05/20 16:46:47, Jamie wrote:
> > Nit: No need for this blank line.
> 
> Done.
> 
>
http://codereview.chromium.org/7033042/diff/2001/remoting/client/appengine/ho...
> File remoting/client/appengine/hostlist.html (right):
> 
>
http://codereview.chromium.org/7033042/diff/2001/remoting/client/appengine/ho...
> remoting/client/appengine/hostlist.html:29:
> value="https://chromoting-httpxmpp-dev.corp.google.com"
> On 2011/05/20 16:46:47, Jamie wrote:
> > This has always been needed, hasn't it? But previously it wasn't editable.
> > What's changed?
> 
> We decided to publish it, so now I'm hardcoding it into a form with a default
> value rather than requiring someone stuffing it into the datastore using some
> arcane form.

All good then; unconditional LGTM.

Expand Messages | Collapse Messages