Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(750)

Issue 14295045: Issue 351: Reimplement OAuth2 (Step 5): WP support (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 5 months ago by peleyal
Modified:
10 years, 5 months ago
Reviewers:
shai.raiten
CC:
google-api-dotnet-client_googlegroups.com, jonskeet
Base URL:
https://google-api-dotnet-client.googlecode.com/hg/
Visibility:
Public.

Description

Issue 351: Reimplement OAuth2 (Step 5): WP support Sample code: var credential = await GoogleWebAuthenticationBroker.AuthenticateAsync( new FileStream("client_secrets.json", FileMode.Open, FileAccess.Read), new[] { DriveService.Scope.Drive }, "Eyal", CancellationToken.None); var i = new BaseClientService.Initializer() { HttpClientInitializer = credential, ApplicationName = "eyal application", };

Patch Set 1 #

Total comments: 21

Patch Set 2 : Shai comments #

Patch Set 3 : minor #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+951 lines, -6 lines) Patch
A Src/GoogleApis.Auth.WP/GoogleApis.Auth.WP.csproj View 1 chunk +158 lines, -0 lines 0 comments Download
A Src/GoogleApis.Auth.WP/OAuth2/AuthorizationCodeBroker.cs View 1 chunk +90 lines, -0 lines 0 comments Download
A Src/GoogleApis.Auth.WP/OAuth2/AuthorizationCodeWPInstalledApp.cs View 1 1 chunk +67 lines, -0 lines 0 comments Download
A Src/GoogleApis.Auth.WP/OAuth2/GoogleWebAuthenticationBroker.cs View 1 2 1 chunk +95 lines, -0 lines 0 comments Download
A Src/GoogleApis.Auth.WP/OAuth2/WebAuthenticationBrokerUserControl.xaml View 1 chunk +23 lines, -0 lines 0 comments Download
A Src/GoogleApis.Auth.WP/OAuth2/WebAuthenticationBrokerUserControl.xaml.cs View 1 chunk +90 lines, -0 lines 0 comments Download
A Src/GoogleApis.Auth.WP/Properties/AssemblyInfo.cs View 1 chunk +41 lines, -0 lines 0 comments Download
A Src/GoogleApis.Auth.WP/app.config View 1 chunk +15 lines, -0 lines 0 comments Download
A Src/GoogleApis.Auth.WP/packages.config View 1 chunk +7 lines, -0 lines 0 comments Download
A Src/GoogleApis.WP/Apis/Util/Protect/DataProtection.cs View 1 chunk +38 lines, -0 lines 0 comments Download
A Src/GoogleApis.WP/Apis/Util/Store/StorageDataStore.cs View 1 1 chunk +101 lines, -0 lines 0 comments Download
A Src/GoogleApis.WP/GoogleApis.WP.csproj View 1 chunk +145 lines, -0 lines 0 comments Download
A Src/GoogleApis.WP/Properties/AssemblyInfo.cs View 1 chunk +41 lines, -0 lines 0 comments Download
A Src/GoogleApis.WP/app.config View 1 chunk +15 lines, -0 lines 0 comments Download
A Src/GoogleApis.WP/packages.config View 1 chunk +8 lines, -0 lines 0 comments Download
M Src/GoogleApis/Apis/Json/NewtonsoftJsonSerializer.cs View 1 5 chunks +17 lines, -6 lines 1 comment Download

Messages

Total messages: 6
peleyal
Hi Shai, Need your review. It's the implementation of WP. I also submitted several CLs ...
10 years, 5 months ago (2013-10-02 22:26:09 UTC) #1
Shai.Raiten
Hi all, I've completed my review, looks awesome! You can see I added some questions ...
10 years, 5 months ago (2013-10-06 06:18:53 UTC) #2
peleyal
Thanks for the review Shai! I still didn't get which exact exceptions we should catch ...
10 years, 5 months ago (2013-10-08 14:03:05 UTC) #3
peleyal
ping :)
10 years, 5 months ago (2013-10-09 22:50:12 UTC) #4
Shai.Raiten
Hi Eyal, I've replay to your questions, this is all from my end! AmAzInG WoRk ...
10 years, 5 months ago (2013-10-13 06:57:45 UTC) #5
peleyal
10 years, 5 months ago (2013-10-15 20:04:40 UTC) #6
Hi Shai.
Thanks!

I'm not sure about the exceptions you mentioned. 
I didn't find any documentation on which exceptions can be thrown from the store
API.
Meantime I prefer to leave it without catching any exception than
1) Catch general exception
2) Catch several exceptions that we are not sure in 100% that it's even possible
that they may be thrown.

Looking forward to your reply.

Thanks,
Eyal

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.WP/Apis/Util/St...
File Src/GoogleApis.WP/Apis/Util/Store/StorageDataStore.cs (right):

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.WP/Apis/Util/St...
Src/GoogleApis.WP/Apis/Util/Store/StorageDataStore.cs:41: {
I'm just trying to figure out where is the list of all possible exceptions.
I don't like to catch Exception (general exception), but I don't like to catch
the exceptions you mentioned because I didn't find any documentation that those
exceptions can be thrown.
Can you refer me to some doc like that?

Thanks!

On 2013/10/13 06:57:45, Shai.Raiten wrote:
> Sometimes uses will Kill the app and this might cause file corruption, so it
> good to make sure the following: 
> 
> IOException - The file could not be opened or retrieved as a stream.
> And add Exception (generic), if the value is Null you'll receive an
> ArgumentNullException, if the converter returns a negative buffer value you'll
> receive ArgumentOutOfRangeException and couple more. so just to make sure..
:-)
> 
> 
> On 2013/10/08 14:03:05, peleyal wrote:
> > I didn't find any best practices on this case.
> > Can you please elaborate?
> > 
> > On 2013/10/06 06:18:53, Shai.Raiten wrote:
> > > You should check or catch null reference exception. for all methods here.
> > 
>

https://codereview.appspot.com/14295045/diff/1/Src/GoogleApis.WP/Apis/Util/St...
Src/GoogleApis.WP/Apis/Util/Store/StorageDataStore.cs:83: {
We use FileMode.OpenOrCreate, so it won't throw an exception :)
On 2013/10/13 06:57:45, Shai.Raiten wrote:
> IsolatedStorageFileStream will throw exception if file not found so -
> FileNotFoundException.
> 
> Or make sure the file exists.
> 
> 
> On 2013/10/08 14:03:05, peleyal wrote:
> > ditto.
> > Please elaborate, which exception should I catch and what should I do in
case
> > there is an exception?
> > 
> > Isn't it something that our developers should catch?
> > 
> > On 2013/10/06 06:18:53, Shai.Raiten wrote:
> > > Again, due to phone development you need to consider file corruption
> > > (Application crashed, user closed etc..) so add the proper checks. 
> > 
>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b