Issue 10669009: Add ShellAccessTokenStore to content_shell

Issue 10669009: Add ShellAccessTokenStore to content_shell (Closed)

Created:
8 years, 6 months ago by John Knottenbelt

Modified:
8 years, 5 months ago

Reviewers:
Avi (use Gerrit), bulach, jochen (gone - plz use gerrit), joth

CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, jochen+watch-content_chromium.org

Base URL:
http://git.chromium.org/chromium/src.git@master

Visibility:
Public.

More Reviews

Description

Add ShellAccessTokenStore to content_shell Geolocation on content_shell requires an implementation of AccessTokenStore in order to work. BUG=134116 TEST=None Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=144705

Patch Set 1 #

Total comments: 10

Patch Set 2 : Use dummy access tokens to avoid hitting server. #

Patch Set 3 : Make DidLoadAccessTokens static. #

Total comments: 1

Patch Set 4 : Include geolocation in the path to shell_access_token_store #

Patch Set 5 : Add explicit private destructor. #

Patch Set 6 : Style fix: mark destructor as virtual and OVERRIDE. #

Patch Set 7 : Remove OVERRIDE from destructor. #

Created: 8 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+83 lines, -0 lines)			Patch
M	content/content_shell.gypi	View	1 2 3	1 chunk	+2 lines, -0 lines	0 comments	Download
A	content/shell/geolocation/shell_access_token_store.h	View	1 2 3 4 5 6	1 chunk	+35 lines, -0 lines	0 comments	Download
A	content/shell/geolocation/shell_access_token_store.cc	View	1 2 3 4	1 chunk	+39 lines, -0 lines	0 comments	Download
M	content/shell/shell_content_browser_client.h	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	content/shell/shell_content_browser_client.cc	View	1 2 3	2 chunks	+6 lines, -0 lines	0 comments	Download

Messages

Total messages: 17 (0 generated)

Expand Messages | Collapse Messages

bulach

lgtm (but not an owner in this area).. some comments below: https://chromiumcodereview.appspot.com/10669009/diff/1/content/shell/shell_access_token_store.cc File content/shell/shell_access_token_store.cc (right): ...

8 years, 6 months ago (2012-06-25 15:28:38 UTC) #2

jochen (gone - plz use gerrit)

https://chromiumcodereview.appspot.com/10669009/diff/1/content/shell/shell_access_token_store.cc File content/shell/shell_access_token_store.cc (right): https://chromiumcodereview.appspot.com/10669009/diff/1/content/shell/shell_access_token_store.cc#newcode6 content/shell/shell_access_token_store.cc:6: #include "chrome/browser/browser_process.h" not needed? https://chromiumcodereview.appspot.com/10669009/diff/1/content/shell/shell_access_token_store.h File content/shell/shell_access_token_store.h (right): https://chromiumcodereview.appspot.com/10669009/diff/1/content/shell/shell_access_token_store.h#newcode5 ...

8 years, 6 months ago (2012-06-25 15:36:44 UTC) #3

joth

does content_shell instantiate real network location provider? e.g. if you run it on linux, windows, ...

8 years, 6 months ago (2012-06-25 16:51:32 UTC) #4

does content_shell instantiate real network location provider? e.g. if you
run it on linux, windows, mac, is the expectation we should get a working
location fix?
If so, I'm concerned that using an empty token will cause the location
provider to keep requesting new tokens each times its started, and what
impact this will have on the server side. An alternative would be to just
use some non-empty hard-coded string ('chromium_content_shell')




On 25 June 2012 08:36, <jochen@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/10669009/diff/1/**
>
content/shell/shell_access_**token_store.cc<https://chromiumcodereview.appspot.com/10669009/diff/1/content/shell/shell_access_token_store.cc>
> File content/shell/shell_access_**token_store.cc (right):
>
> https://chromiumcodereview.**appspot.com/10669009/diff/1/**
>
content/shell/shell_access_**token_store.cc#newcode6<https://chromiumcodereview.appspot.com/10669009/diff/1/content/shell/shell_access_token_store.cc#newcode6>
> content/shell/shell_access_**token_store.cc:6: #include
> "chrome/browser/browser_**process.h"
> not needed?
>
>
> https://chromiumcodereview.**appspot.com/10669009/diff/1/**
>
content/shell/shell_access_**token_store.h<https://chromiumcodereview.appspot.com/10669009/diff/1/content/shell/shell_access_token_store.h>
> File content/shell/shell_access_**token_store.h (right):
>
> https://chromiumcodereview.**appspot.com/10669009/diff/1/**
>
content/shell/shell_access_**token_store.h#newcode5<https://chromiumcodereview.appspot.com/10669009/diff/1/content/shell/shell_access_token_store.h#newcode5>
> content/shell/shell_access_**token_store.h:5: #ifndef
> CHROME_BROWSER_GEOLOCATION_**CHROME_ACCESS_TOKEN_STORE_H_
> update plz
>
>
> https://chromiumcodereview.**appspot.com/10669009/diff/1/**
>
content/shell/shell_access_**token_store.h#newcode14<https://chromiumcodereview.appspot.com/10669009/diff/1/content/shell/shell_access_token_store.h#newcode14>
> content/shell/shell_access_**token_store.h:14:
> ShellAccessTokenStore(net::**URLRequestContextGetter*
> system_request_context);
> The name "system request context" doesn't make sense in content/shell.
> What about just request_context?
>
>
https://chromiumcodereview.**appspot.com/10669009/<https://chromiumcodereview...
>

John Knottenbelt

I think that content_shell is supposed to be more of an example embedding of Chromium ...

8 years, 6 months ago (2012-06-26 07:46:15 UTC) #5

joth

lgtm with one suggestion... https://chromiumcodereview.appspot.com/10669009/diff/10001/content/shell/shell_access_token_store.cc File content/shell/shell_access_token_store.cc (right): https://chromiumcodereview.appspot.com/10669009/diff/10001/content/shell/shell_access_token_store.cc#newcode1 content/shell/shell_access_token_store.cc:1: // Copyright (c) 2012 The ...

8 years, 6 months ago (2012-06-26 15:37:09 UTC) #7

John Knottenbelt

Thanks for review so far! Avi, please could you OWNERS review?

8 years, 5 months ago (2012-06-27 15:07:30 UTC) #8