api: subscriptions -> notifications & http issue 216: DataStore

api: subscriptions -> notifications & http issue 216: DataStore

depends on:
https://codereview.appspot.com/9881043/

closely related changeset to deprecate all of the classes in the subscriptions package (but not quite ready for review):
https://codereview.appspot.com/9535043/

Nick: careful review of everything
Eyal: design review

[yanivi, 2013-06-07 17:48:44]

friendly ping

[ngmiceli, 2013-06-07 17:51:10]

On 2013/06/07 17:48:44, yanivi wrote:
> friendly ping

Waiting on your responses to our comments on the HTTP and OAuth layers, as if
you make major changes there they will have to propagate up here first.

[ngmiceli, 2013-06-10 19:41:45]

Hi,
There is a lack in testing for this package, can we add unit test for the
package?

https://codereview.appspot.com/9045044/diff/41001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notify/ResourceStates.java
(right):

https://codereview.appspot.com/9045044/diff/41001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/ResourceStates.java:33:
public static final String EXISTS = "EXISTS";
it will be easier if the documentation will include all the possibilities of
this keyword - create \ update ???

https://codereview.appspot.com/9045044/diff/41001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/ResourceStates.java:36:
public static final String NOT_EXISTS = "NOT_EXISTS";
ditto. delete?

https://codereview.appspot.com/9045044/diff/44001/google-api-client-servlet/s...
File
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notify/AbstractNotificationServlet.java
(right):

https://codereview.appspot.com/9045044/diff/44001/google-api-client-servlet/s...
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notify/AbstractNotificationServlet.java:38:
* WebHook Servlet to receive notifications.
we should have an issue to update all the documentation of the notify package
when the official spec will be released

https://codereview.appspot.com/9045044/diff/44001/google-api-client-servlet/s...
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notify/AbstractNotificationServlet.java:52:
public class NotificationServlet extends AbstractWebHookServlet {
extends AbstractNotificationServlet?

https://codereview.appspot.com/9045044/diff/44001/google-api-client-servlet/s...
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notify/AbstractNotificationServlet.java:57:
protected SubscriptionStore createSubscriptionStore() {
SubscriptionStore -> StoredChannel?

Please update this doc if there are more classes to update

https://codereview.appspot.com/9045044/diff/44001/google-api-client-servlet/s...
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notify/AbstractNotificationServlet.java:123:
try {
try to minimize the try block to as minimum as required. Do that by:
move the contentStream declaration to where it first use.

https://codereview.appspot.com/9045044/diff/44001/google-api-client-servlet/s...
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notify/AbstractNotificationServlet.java:157:
|| resourceUri == null || channelId == null) {
you should also check here that messageNumber can be cast to long

https://codereview.appspot.com/9045044/diff/44001/google-api-client-servlet/s...
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notify/AbstractNotificationServlet.java:168:
if (storedChannel == null) {
can you add more documentation, we don't get how can storeChannel be null here,
or what it means?

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/GoogleAuthorizationCodeFlow.java
(right):

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/GoogleAuthorizationCodeFlow.java:67:
* @since 1.7
change since to be after author

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notify/Notification.java
(right):

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/Notification.java:23:
* Notification meta-data sent to this client about a watched resource.
meta-data -> metadata

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/Notification.java:26:
* Implementation is thread-safe.
?????? No. It isn't, you have setters (wihtout lock) so it isn't a thread safe
instance

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/Notification.java:73:
public Notification(long messageNumber, String resourceState, String resourceId,
Eyal: I hate a constructor which gets more than 3-4 parameters. I prefer a
constructor which gets one parameter (lets call it NotificationArgs) and then,
when later on you will need to another variable to the constructor, you won't
need to change the signature of the method, you will only need to add another
property to args.

Nick: Use the builder pattern here

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notify/NotificationCallback.java
(right):

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/NotificationCallback.java:72:
void onNotification(StoredChannel storedChannel, Notification notification,
String contentType,
Eyal:

Think about creating two subclasses to Notification:
ParsedNotifiction \ TypedNotification which contains T (the parsed object)
UnparsedNotifiation \ RawNotification which contains contentType and stream

Then:

public interface UnparsedNotificationCallback extends Serializable {
  void onNotification(StoredChannel storedChannel, UnparsedNotification
notification);
}

and

public interface ParsedNotificationCallback<T> extends Serializable {
  void onNotification(StoredChannel storedChannel, ParsedNotification<T>
notification);
}

It is similar to the old design. for me it's weird that contentType and stream
here is not part of the notification itself. Lets talk about it and try to
figure out was is the best implementation here.

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notify/NotificationUtils.java
(right):

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/NotificationUtils.java:25:
public class NotificationUtils {
Nick: add final

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notify/ResourceStates.java
(right):

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/ResourceStates.java:2:
* Copyright (c) 2012 Google Inc.
2013.
and also all the rest files

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/ResourceStates.java:33:
public static final String EXISTS = "EXISTS";
Please also add explanation that this resource state is used for Create and
Update

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/ResourceStates.java:36:
public static final String NOT_EXISTS = "NOT_EXISTS";
ditto here Delete

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notify/StoredChannel.java
(right):

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/StoredChannel.java:88:
public StoredChannel store(DataStore dataStore) throws IOException {
why do we need that constructor? who is going to use it?

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notify/TypedNotificationCallback.java
(right):

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/TypedNotificationCallback.java:109:
public void onNotification(StoredChannel subscription, Notification
notification,
Eyal: shouldn't the method be final?

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notify/json/JsonNotificationCallback.java
(right):

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/json/JsonNotificationCallback.java:90:
protected abstract JsonFactory getJsonFactory() throws IOException;
why not passing the JSonFactory in the constructor for this callback rather than
this abstract method

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notify/package-info.java
(right):

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/package-info.java:2:
* Copyright (c) 2012 Google Inc.
2013

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/package-info.java:22:

consistent, in the other file (notify.json package) there wasn't a break line
(line 22)

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/testing/notify/MockNotificationCallback.java
(right):

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/testing/notify/MockNotificationCallback.java:38:
/** True if this handler was called. */
{@code true}

[yanivi, 2013-06-13 20:36:36]

https://codereview.appspot.com/9045044/diff/41001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notify/ResourceStates.java
(right):

https://codereview.appspot.com/9045044/diff/41001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/ResourceStates.java:33:
public static final String EXISTS = "EXISTS";
On 2013/06/10 19:41:45, ngmiceli wrote:
> it will be easier if the documentation will include all the possibilities of
> this keyword - create \ update ???

Create and update are not possible.  Only sync, exists, and not_exists are
possible.

https://codereview.appspot.com/9045044/diff/41001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/ResourceStates.java:36:
public static final String NOT_EXISTS = "NOT_EXISTS";
On 2013/06/10 19:41:45, ngmiceli wrote:
> ditto. delete?

ditto.

https://codereview.appspot.com/9045044/diff/44001/google-api-client-servlet/s...
File
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notify/AbstractNotificationServlet.java
(right):

https://codereview.appspot.com/9045044/diff/44001/google-api-client-servlet/s...
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notify/AbstractNotificationServlet.java:38:
* WebHook Servlet to receive notifications.
On 2013/06/10 19:41:45, ngmiceli wrote:
> we should have an issue to update all the documentation of the notify package
> when the official spec will be released

I'd rather wait for the spec to be released before opening the issue.

https://codereview.appspot.com/9045044/diff/44001/google-api-client-servlet/s...
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notify/AbstractNotificationServlet.java:52:
public class NotificationServlet extends AbstractWebHookServlet {
On 2013/06/10 19:41:45, ngmiceli wrote:
> extends AbstractNotificationServlet?

Done.

https://codereview.appspot.com/9045044/diff/44001/google-api-client-servlet/s...
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notify/AbstractNotificationServlet.java:57:
protected SubscriptionStore createSubscriptionStore() {
On 2013/06/10 19:41:45, ngmiceli wrote:
> SubscriptionStore -> StoredChannel?
> 
> Please update this doc if there are more classes to update

Done.

https://codereview.appspot.com/9045044/diff/44001/google-api-client-servlet/s...
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notify/AbstractNotificationServlet.java:123:
try {
On 2013/06/10 19:41:45, ngmiceli wrote:
> try to minimize the try block to as minimum as required. Do that by:
> move the contentStream declaration to where it first use.

I think it already is.

https://codereview.appspot.com/9045044/diff/44001/google-api-client-servlet/s...
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notify/AbstractNotificationServlet.java:157:
|| resourceUri == null || channelId == null) {
On 2013/06/10 19:41:45, ngmiceli wrote:
> you should also check here that messageNumber can be cast to long

Done.

https://codereview.appspot.com/9045044/diff/44001/google-api-client-servlet/s...
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notify/AbstractNotificationServlet.java:168:
if (storedChannel == null) {
On 2013/06/10 19:41:45, ngmiceli wrote:
> can you add more documentation, we don't get how can storeChannel be null
here,
> or what it means?

Done.

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/GoogleAuthorizationCodeFlow.java
(right):

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/auth/oauth2/GoogleAuthorizationCodeFlow.java:67:
* @since 1.7
On 2013/06/10 19:41:45, ngmiceli wrote:
> change since to be after author 

Done.

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notify/Notification.java
(right):

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/Notification.java:23:
* Notification meta-data sent to this client about a watched resource.
On 2013/06/10 19:41:45, ngmiceli wrote:
> meta-data -> metadata

Done.

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/Notification.java:26:
* Implementation is thread-safe.
On 2013/06/10 19:41:45, ngmiceli wrote:
> ?????? No. It isn't, you have setters (wihtout lock) so it isn't a thread safe
> instance

Done.

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/Notification.java:73:
public Notification(long messageNumber, String resourceState, String resourceId,
On 2013/06/10 19:41:45, ngmiceli wrote:
> Eyal: I hate a constructor which gets more than 3-4 parameters. I prefer a
> constructor which gets one parameter (lets call it NotificationArgs) and then,
> when later on you will need to another variable to the constructor, you won't
> need to change the signature of the method, you will only need to add another
> property to args.
> 
> Nick: Use the builder pattern here

We are using the established pattern for the library.  required parameters go
into the constructor.  This is a topic that was hotly debated years ago in the
library, and this is the consensus we arrived at that I think works pretty well.
 To summarize the reason: we want to make sure that if you don't specify
required parameters then you get a compile error.

Builder pattern doesn't make sense because this class isn't intended to be
thread-safe.  By the way, the same pattern of putting required parameters into
the constructor applies to Builder as well.  So that's an orthogonal issue.

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notify/NotificationCallback.java
(right):

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/NotificationCallback.java:72:
void onNotification(StoredChannel storedChannel, Notification notification,
String contentType,
On 2013/06/10 19:41:45, ngmiceli wrote:
> Eyal:
> 
> Think about creating two subclasses to Notification:
> ParsedNotifiction \ TypedNotification which contains T (the parsed object)
> UnparsedNotifiation \ RawNotification which contains contentType and stream
> 
> Then:
> 
> public interface UnparsedNotificationCallback extends Serializable {
>   void onNotification(StoredChannel storedChannel, UnparsedNotification
> notification);
> }
> 
> and
> 
> public interface ParsedNotificationCallback<T> extends Serializable {
>   void onNotification(StoredChannel storedChannel, ParsedNotification<T>
> notification);
> }
> 
> It is similar to the old design. for me it's weird that contentType and stream
> here is not part of the notification itself. Lets talk about it and try to
> figure out was is the best implementation here.

Done-ish.  Let me know if it looks right to you.

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notify/NotificationUtils.java
(right):

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/NotificationUtils.java:25:
public class NotificationUtils {
On 2013/06/10 19:41:45, ngmiceli wrote:
> Nick: add final

Done.

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notify/ResourceStates.java
(right):

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/ResourceStates.java:2:
* Copyright (c) 2012 Google Inc.
On 2013/06/10 19:41:45, ngmiceli wrote:
> 2013.
> and also all the rest files

Done.

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/ResourceStates.java:33:
public static final String EXISTS = "EXISTS";
On 2013/06/10 19:41:45, ngmiceli wrote:
> Please also add explanation that this resource state is used for Create and
> Update

Done.

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/ResourceStates.java:36:
public static final String NOT_EXISTS = "NOT_EXISTS";
On 2013/06/10 19:41:45, ngmiceli wrote:
> ditto here Delete

Done.

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notify/StoredChannel.java
(right):

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/StoredChannel.java:88:
public StoredChannel store(DataStore dataStore) throws IOException {
On 2013/06/10 19:41:45, ngmiceli wrote:
> why do we need that constructor? who is going to use it?

I don't understand the question.  I added JavaDoc to the store methods.

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notify/TypedNotificationCallback.java
(right):

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/TypedNotificationCallback.java:109:
public void onNotification(StoredChannel subscription, Notification
notification,
On 2013/06/10 19:41:45, ngmiceli wrote:
> Eyal: shouldn't the method be final?

Done.

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notify/json/JsonNotificationCallback.java
(right):

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/json/JsonNotificationCallback.java:90:
protected abstract JsonFactory getJsonFactory() throws IOException;
On 2013/06/10 19:41:45, ngmiceli wrote:
> why not passing the JSonFactory in the constructor for this callback rather
than
> this abstract method

because it has to be Serializable and have a default constructor with no
parameters for deserialization.

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notify/package-info.java
(right):

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/package-info.java:2:
* Copyright (c) 2012 Google Inc.
On 2013/06/10 19:41:45, ngmiceli wrote:
> 2013

Done.

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notify/package-info.java:22:

On 2013/06/10 19:41:45, ngmiceli wrote:
> consistent, in the other file (notify.json package) there wasn't a break line
> (line 22)

Done.

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/testing/notify/MockNotificationCallback.java
(right):

https://codereview.appspot.com/9045044/diff/44001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/testing/notify/MockNotificationCallback.java:38:
/** True if this handler was called. */
On 2013/06/10 19:41:45, ngmiceli wrote:
> {@code true}

Changed to "whether".

[yanivi, 2013-06-17 12:05:25]

Ping.  Let's get this done today.

[peleyal, 2013-06-17 20:09:48]

https://codereview.appspot.com/9045044/diff/60001/google-api-client-appengine...
File
google-api-client-appengine/src/main/java/com/google/api/client/googleapis/extensions/appengine/notifications/package-info.java
(right):

https://codereview.appspot.com/9045044/diff/60001/google-api-client-appengine...
google-api-client-appengine/src/main/java/com/google/api/client/googleapis/extensions/appengine/notifications/package-info.java:20:
* @author Yaniv Inbar
[Optional]
Please consider add here (and in other files) Matthias.
He originally wrote the notification and I know that you changed a lot of stuff
here, but I think it's nice to keep his name 
on the files that were exist before on the deprecated subscription package

https://codereview.appspot.com/9045044/diff/60001/google-api-client-assembly/...
File
google-api-client-assembly/android-properties/google-api-client-gson-1.15.0-rc-SNAPSHOT.jar.properties
(right):

https://codereview.appspot.com/9045044/diff/60001/google-api-client-assembly/...
google-api-client-assembly/android-properties/google-api-client-gson-1.15.0-rc-SNAPSHOT.jar.properties:1:
src=../libs-sources/google-api-client-gson-${project.api.version}-sources.jar
I need explanation... what are you trying to do here?

https://codereview.appspot.com/9045044/diff/60001/google-api-client-assembly/...
File
google-api-client-assembly/dependencies/google-api-client-gson-dependencies.html
(right):

https://codereview.appspot.com/9045044/diff/60001/google-api-client-assembly/...
google-api-client-assembly/dependencies/google-api-client-gson-dependencies.html:1:
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
"http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
and here.. explanation too.
1. Is it something that we need to generated only in the first time we are
adding a new jar?
2. What is the purpose? Is it a just generated page that contains all the
dependencies?
3. If you change dependencies of this jar (gson) - should we regenerate this
html file?

https://codereview.appspot.com/9045044/diff/60001/google-api-client-gson/src/...
File
google-api-client-gson/src/main/java/com/google/api/client/googleapis/notifications/json/gson/GsonNotificationCallback.java
(right):

https://codereview.appspot.com/9045044/diff/60001/google-api-client-gson/src/...
google-api-client-gson/src/main/java/com/google/api/client/googleapis/notifications/json/gson/GsonNotificationCallback.java:29:
* Must not be implemented in form of an anonymous class as this will break
serialization.
[optional] must not --> must NOT

https://codereview.appspot.com/9045044/diff/60001/google-api-client-gson/src/...
google-api-client-gson/src/main/java/com/google/api/client/googleapis/notifications/json/gson/GsonNotificationCallback.java:38:
* during the {@code .handleNotification(..)} call will be lost.
{@code .handleNotification(...)
what is it?
can you change to {@link instead?

https://codereview.appspot.com/9045044/diff/60001/google-api-client-jackson2/...
File
google-api-client-jackson2/src/main/java/com/google/api/client/googleapis/notifications/json/jackson2/JacksonNotificationCallback.java
(right):

https://codereview.appspot.com/9045044/diff/60001/google-api-client-jackson2/...
google-api-client-jackson2/src/main/java/com/google/api/client/googleapis/notifications/json/jackson2/JacksonNotificationCallback.java:29:
* Must not be implemented in form of an anonymous class as this will break
serialization.
ditto (NOT)

https://codereview.appspot.com/9045044/diff/60001/google-api-client-jackson2/...
google-api-client-jackson2/src/main/java/com/google/api/client/googleapis/notifications/json/jackson2/JacksonNotificationCallback.java:38:
* during the {@code .handleNotification(..)} call will be lost.
ditto

https://codereview.appspot.com/9045044/diff/60001/google-api-client-jackson2/...
File
google-api-client-jackson2/src/main/java/com/google/api/client/googleapis/notifications/json/jackson2/package-info.java
(right):

https://codereview.appspot.com/9045044/diff/60001/google-api-client-jackson2/...
google-api-client-jackson2/src/main/java/com/google/api/client/googleapis/notifications/json/jackson2/package-info.java:26:

remove line 26

https://codereview.appspot.com/9045044/diff/60001/google-api-client-servlet/s...
File
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notifications/NotificationServlet.java
(right):

https://codereview.appspot.com/9045044/diff/60001/google-api-client-servlet/s...
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notifications/NotificationServlet.java:75:
* By default it uses {@link MemoryDataStoreFactory#getDefaultInstance()} which
means it will NOT
why not using JdoDataStoreFactory here?
It is weird that it uses memory data store and in the same sentence it also says
that it's BAD PRACTICE

https://codereview.appspot.com/9045044/diff/60001/google-api-client-servlet/s...
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notifications/NotificationServlet.java:107:
private transient DataStore<StoredChannel> channelDataStore;
[optional]
why not creating it in the constructor and avoid the lock?
Lazy initialization? In my opinion it's better to change the constructor.

https://codereview.appspot.com/9045044/diff/60001/google-api-client-servlet/s...
File
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notifications/WebhookUtils.java
(right):

https://codereview.appspot.com/9045044/diff/60001/google-api-client-servlet/s...
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notifications/WebhookUtils.java:59:
* {@link #processWebhookNotification(HttpServletRequest, HttpServletResponse,
DataStore)} that
that derived -> that uses (here and in other places as well)
Or maybe it's my bad English...

https://codereview.appspot.com/9045044/diff/60001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/AbstractNotification.java
(right):

https://codereview.appspot.com/9045044/diff/60001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/AbstractNotification.java:101:
.add("channelExpiration", channelExpiration).add("channelToken", channelToken);
don't you want to add "changed" as well?

https://codereview.appspot.com/9045044/diff/60001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/NotificationUtils.java
(right):

https://codereview.appspot.com/9045044/diff/60001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/NotificationUtils.java:29:
return UUID.randomUUID().toString();
do we really need it as a utils class?
Right now it looks like an additional file. Unless you think that we are going
to add more context here in the future

https://codereview.appspot.com/9045044/diff/60001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/StoredChannel.java
(right):

https://codereview.appspot.com/9045044/diff/60001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/StoredChannel.java:102:
public StoredChannel store(DataStoreFactory dataStoreFactory) throws IOException
{
should this method (and the one below) be synchronized as well?

https://codereview.appspot.com/9045044/diff/60001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/json/JsonNotificationCallback.java
(right):

https://codereview.appspot.com/9045044/diff/60001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/json/JsonNotificationCallback.java:29:
* Must not be implemented in form of an anonymous class as this will break
serialization.
not --> NOT

[yanivi, 2013-06-18 16:07:50]

https://codereview.appspot.com/9045044/diff/60001/google-api-client-appengine...
File
google-api-client-appengine/src/main/java/com/google/api/client/googleapis/extensions/appengine/notifications/package-info.java
(right):

https://codereview.appspot.com/9045044/diff/60001/google-api-client-appengine...
google-api-client-appengine/src/main/java/com/google/api/client/googleapis/extensions/appengine/notifications/package-info.java:20:
* @author Yaniv Inbar
On 2013/06/17 20:09:48, peleyal wrote:
> [Optional]
> Please consider add here (and in other files) Matthias.
> He originally wrote the notification and I know that you changed a lot of
stuff
> here, but I think it's nice to keep his name 
> on the files that were exist before on the deprecated subscription package

Done.

https://codereview.appspot.com/9045044/diff/60001/google-api-client-assembly/...
File
google-api-client-assembly/android-properties/google-api-client-gson-1.15.0-rc-SNAPSHOT.jar.properties
(right):

https://codereview.appspot.com/9045044/diff/60001/google-api-client-assembly/...
google-api-client-assembly/android-properties/google-api-client-gson-1.15.0-rc-SNAPSHOT.jar.properties:1:
src=../libs-sources/google-api-client-gson-${project.api.version}-sources.jar
On 2013/06/17 20:09:48, peleyal wrote:
> I need explanation... what are you trying to do here?

This is a properties file used by the Eclipse Android support.  See the other
files in this directory.  We are just being consistent.  I'd like to eventually
get rid of this and switch to recommending Gradle support.

https://codereview.appspot.com/9045044/diff/60001/google-api-client-assembly/...
File
google-api-client-assembly/dependencies/google-api-client-gson-dependencies.html
(right):

https://codereview.appspot.com/9045044/diff/60001/google-api-client-assembly/...
google-api-client-assembly/dependencies/google-api-client-gson-dependencies.html:1:
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
"http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
On 2013/06/17 20:09:48, peleyal wrote:
> and here.. explanation too.
> 1. Is it something that we need to generated only in the first time we are
> adding a new jar?

first time and every time we change the version

> 2. What is the purpose? Is it a just generated page that contains all the
> dependencies?

yes.  Lawyers like it :)

> 3. If you change dependencies of this jar (gson) - should we regenerate this
> html file?

yes, that case too.

https://codereview.appspot.com/9045044/diff/60001/google-api-client-gson/src/...
File
google-api-client-gson/src/main/java/com/google/api/client/googleapis/notifications/json/gson/GsonNotificationCallback.java
(right):

https://codereview.appspot.com/9045044/diff/60001/google-api-client-gson/src/...
google-api-client-gson/src/main/java/com/google/api/client/googleapis/notifications/json/gson/GsonNotificationCallback.java:29:
* Must not be implemented in form of an anonymous class as this will break
serialization.
On 2013/06/17 20:09:48, peleyal wrote:
> [optional] must not --> must NOT

Done.

https://codereview.appspot.com/9045044/diff/60001/google-api-client-gson/src/...
google-api-client-gson/src/main/java/com/google/api/client/googleapis/notifications/json/gson/GsonNotificationCallback.java:38:
* during the {@code .handleNotification(..)} call will be lost.
On 2013/06/17 20:09:48, peleyal wrote:
> {@code .handleNotification(...)
> what is it?
> can you change to {@link instead?

Completely removed.  Instead, put a more clear JavaDoc comment on the
onNotification methods.

https://codereview.appspot.com/9045044/diff/60001/google-api-client-jackson2/...
File
google-api-client-jackson2/src/main/java/com/google/api/client/googleapis/notifications/json/jackson2/JacksonNotificationCallback.java
(right):

https://codereview.appspot.com/9045044/diff/60001/google-api-client-jackson2/...
google-api-client-jackson2/src/main/java/com/google/api/client/googleapis/notifications/json/jackson2/JacksonNotificationCallback.java:29:
* Must not be implemented in form of an anonymous class as this will break
serialization.
On 2013/06/17 20:09:48, peleyal wrote:
> ditto (NOT)

Done.

https://codereview.appspot.com/9045044/diff/60001/google-api-client-jackson2/...
google-api-client-jackson2/src/main/java/com/google/api/client/googleapis/notifications/json/jackson2/JacksonNotificationCallback.java:38:
* during the {@code .handleNotification(..)} call will be lost.
On 2013/06/17 20:09:48, peleyal wrote:
> ditto

Done.

https://codereview.appspot.com/9045044/diff/60001/google-api-client-jackson2/...
File
google-api-client-jackson2/src/main/java/com/google/api/client/googleapis/notifications/json/jackson2/package-info.java
(right):

https://codereview.appspot.com/9045044/diff/60001/google-api-client-jackson2/...
google-api-client-jackson2/src/main/java/com/google/api/client/googleapis/notifications/json/jackson2/package-info.java:26:

On 2013/06/17 20:09:48, peleyal wrote:
> remove line 26

Done.

https://codereview.appspot.com/9045044/diff/60001/google-api-client-servlet/s...
File
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notifications/NotificationServlet.java
(right):

https://codereview.appspot.com/9045044/diff/60001/google-api-client-servlet/s...
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notifications/NotificationServlet.java:75:
* By default it uses {@link MemoryDataStoreFactory#getDefaultInstance()} which
means it will NOT
On 2013/06/17 20:09:48, peleyal wrote:
> why not using JdoDataStoreFactory here?
> It is weird that it uses memory data store and in the same sentence it also
says
> that it's BAD PRACTICE

MemoryDataStoreFactory is free and simple.  It makes it easy to start quickly,
and makes for nice demos.  JdoDataStoreFactory would immediately means you have
to write your own subclass, so that would default the purpose of having a
default constructor, not to mention the hassles of setting up JDO.

https://codereview.appspot.com/9045044/diff/60001/google-api-client-servlet/s...
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notifications/NotificationServlet.java:107:
private transient DataStore<StoredChannel> channelDataStore;
On 2013/06/17 20:09:48, peleyal wrote:
> [optional]
> why not creating it in the constructor and avoid the lock?
> Lazy initialization? In my opinion it's better to change the constructor.

Done.

https://codereview.appspot.com/9045044/diff/60001/google-api-client-servlet/s...
File
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notifications/WebhookUtils.java
(right):

https://codereview.appspot.com/9045044/diff/60001/google-api-client-servlet/s...
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notifications/WebhookUtils.java:59:
* {@link #processWebhookNotification(HttpServletRequest, HttpServletResponse,
DataStore)} that
On 2013/06/17 20:09:48, peleyal wrote:
> that derived -> that uses (here and in other places as well)
> Or maybe it's my bad English...

Done.

https://codereview.appspot.com/9045044/diff/60001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/AbstractNotification.java
(right):

https://codereview.appspot.com/9045044/diff/60001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/AbstractNotification.java:101:
.add("channelExpiration", channelExpiration).add("channelToken", channelToken);
On 2013/06/17 20:09:48, peleyal wrote:
> don't you want to add "changed" as well?

Done.

https://codereview.appspot.com/9045044/diff/60001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/NotificationUtils.java
(right):

https://codereview.appspot.com/9045044/diff/60001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/NotificationUtils.java:29:
return UUID.randomUUID().toString();
On 2013/06/17 20:09:48, peleyal wrote:
> do we really need it as a utils class?
> Right now it looks like an additional file. Unless you think that we are going
> to add more context here in the future

Yep, that's the idea.  But even if not, doesn't seem like a big deal?

https://codereview.appspot.com/9045044/diff/60001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/StoredChannel.java
(right):

https://codereview.appspot.com/9045044/diff/60001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/StoredChannel.java:102:
public StoredChannel store(DataStoreFactory dataStoreFactory) throws IOException
{
On 2013/06/17 20:09:48, peleyal wrote:
> should this method (and the one below) be synchronized as well?

why?  it's not directly accessing internal data.

https://codereview.appspot.com/9045044/diff/60001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/json/JsonNotificationCallback.java
(right):

https://codereview.appspot.com/9045044/diff/60001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/json/JsonNotificationCallback.java:29:
* Must not be implemented in form of an anonymous class as this will break
serialization.
On 2013/06/17 20:09:48, peleyal wrote:
> not --> NOT

Done.

[peleyal, 2013-06-18 21:27:42]

Only small stuff. Let's close them ASAP, and you will be able to push it really
really soon :)

https://codereview.appspot.com/9045044/diff/60001/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/NotificationUtils.java
(right):

https://codereview.appspot.com/9045044/diff/60001/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/NotificationUtils.java:29:
return UUID.randomUUID().toString();
Not a big deal.
On 2013/06/18 16:07:51, yanivi wrote:
> On 2013/06/17 20:09:48, peleyal wrote:
> > do we really need it as a utils class?
> > Right now it looks like an additional file. Unless you think that we are
going
> > to add more context here in the future
> 
> Yep, that's the idea.  But even if not, doesn't seem like a big deal?
>

https://codereview.appspot.com/9045044/diff/91002/google-api-client-servlet/s...
File
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notifications/NotificationServlet.java
(right):

https://codereview.appspot.com/9045044/diff/91002/google-api-client-servlet/s...
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notifications/NotificationServlet.java:72:
* By default it uses {@link MemoryDataStoreFactory#getDefaultInstance()} which
means it will NOT
please add WARNING at the beginning of this <p>

https://codereview.appspot.com/9045044/diff/91002/google-api-client-servlet/s...
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notifications/NotificationServlet.java:73:
* persist the notification channels when the servlet process dies, so it is a
bad choice for a
BAD CHOICE

https://codereview.appspot.com/9045044/diff/91002/google-api-client-servlet/s...
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notifications/NotificationServlet.java:105:
* application.
cool :)
I prefer as many explanations as possible

https://codereview.appspot.com/9045044/diff/91002/google-api-client-servlet/s...
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notifications/NotificationServlet.java:117:
protected NotificationServlet(DataStoreFactory dataStoreFactory) throws
IOException {
[optional]
I think that you can remove this constructor and only use directly the one
below. so the constructor above will look like this:

 public NotificationServlet() throws IOException {
 
this(StoredChannel.getDefaultDataStore(MemoryDataStoreFactory.getDefaultInstance()));
}

https://codereview.appspot.com/9045044/diff/91002/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/TypedNotificationCallback.java
(right):

https://codereview.appspot.com/9045044/diff/91002/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/TypedNotificationCallback.java:85:
* Any state changes occurring during this notification will not be persisted.
I tried to understand what you mean here and I didn't succeeded, so maybe it's
better to improve that one

https://codereview.appspot.com/9045044/diff/91002/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/UnparsedNotificationCallback.java
(right):

https://codereview.appspot.com/9045044/diff/91002/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/UnparsedNotificationCallback.java:68:
* Any state changes occurring during this notification will not be persisted.
ditto

https://codereview.appspot.com/9045044/diff/91002/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/testing/notifications/MockUnparsedNotificationCallback.java
(right):

https://codereview.appspot.com/9045044/diff/91002/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/testing/notifications/MockUnparsedNotificationCallback.java:28:
* @author Yaniv Inbar
@author Matthias Linder (mlinder)

https://codereview.appspot.com/9045044/diff/91002/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/testing/notifications/package-info.java
(right):

https://codereview.appspot.com/9045044/diff/91002/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/testing/notifications/package-info.java:20:
* @since 1.16
@author Matthias Linder (mlinder)

[yanivi, 2013-06-19 00:15:44]

https://codereview.appspot.com/9045044/diff/91002/google-api-client-servlet/s...
File
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notifications/NotificationServlet.java
(right):

https://codereview.appspot.com/9045044/diff/91002/google-api-client-servlet/s...
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notifications/NotificationServlet.java:72:
* By default it uses {@link MemoryDataStoreFactory#getDefaultInstance()} which
means it will NOT
On 2013/06/18 21:27:43, peleyal wrote:
> please add WARNING at the beginning of this <p>

Done.

https://codereview.appspot.com/9045044/diff/91002/google-api-client-servlet/s...
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notifications/NotificationServlet.java:73:
* persist the notification channels when the servlet process dies, so it is a
bad choice for a
On 2013/06/18 21:27:43, peleyal wrote:
> BAD CHOICE

Done.

https://codereview.appspot.com/9045044/diff/91002/google-api-client-servlet/s...
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notifications/NotificationServlet.java:105:
* application.
On 2013/06/18 21:27:43, peleyal wrote:
> cool :)
> I prefer as many explanations as possible

ACK

https://codereview.appspot.com/9045044/diff/91002/google-api-client-servlet/s...
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notifications/NotificationServlet.java:117:
protected NotificationServlet(DataStoreFactory dataStoreFactory) throws
IOException {
On 2013/06/18 21:27:43, peleyal wrote:
> [optional]
> I think that you can remove this constructor and only use directly the one
> below. so the constructor above will look like this:
> 
>  public NotificationServlet() throws IOException {
>  
>
this(StoredChannel.getDefaultDataStore(MemoryDataStoreFactory.getDefaultInstance()));
> }

No, actually the whole point is that this is the recommended constructor to use
that takes DataStoreFactory.  You normally shouldn't need to use the
DataStore<StoredChannel> constructor unless you need to specify which
notification data store to use other than the default one.

https://codereview.appspot.com/9045044/diff/91002/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/TypedNotificationCallback.java
(right):

https://codereview.appspot.com/9045044/diff/91002/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/TypedNotificationCallback.java:85:
* Any state changes occurring during this notification will not be persisted.
On 2013/06/18 21:27:43, peleyal wrote:
> I tried to understand what you mean here and I didn't succeeded, so maybe it's
> better to improve that one

Removed it.  It doesn't make sense.  Persistence of this object isn't done here,
so it doesn't make sense to document it here.

https://codereview.appspot.com/9045044/diff/91002/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/UnparsedNotificationCallback.java
(right):

https://codereview.appspot.com/9045044/diff/91002/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/UnparsedNotificationCallback.java:68:
* Any state changes occurring during this notification will not be persisted.
On 2013/06/18 21:27:43, peleyal wrote:
> ditto

ditto

https://codereview.appspot.com/9045044/diff/91002/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/testing/notifications/MockUnparsedNotificationCallback.java
(right):

https://codereview.appspot.com/9045044/diff/91002/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/testing/notifications/MockUnparsedNotificationCallback.java:28:
* @author Yaniv Inbar
On 2013/06/18 21:27:43, peleyal wrote:
> @author Matthias Linder (mlinder)

Done.

https://codereview.appspot.com/9045044/diff/91002/google-api-client/src/main/...
File
google-api-client/src/main/java/com/google/api/client/googleapis/testing/notifications/package-info.java
(right):

https://codereview.appspot.com/9045044/diff/91002/google-api-client/src/main/...
google-api-client/src/main/java/com/google/api/client/googleapis/testing/notifications/package-info.java:20:
* @since 1.16
On 2013/06/18 21:27:43, peleyal wrote:
> @author Matthias Linder (mlinder)

Done.

[peleyal, 2013-06-19 00:17:47]

LGTM :)

[jinny7845, 2015-04-25 12:38:43]

On 2013/06/19 00:15:44, yanivi wrote:
>
https://codereview.appspot.com/9045044/diff/91002/google-api-client-servlet/s...
> File
>
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notifications/NotificationServlet.java
> (right):
> 
>
https://codereview.appspot.com/9045044/diff/91002/google-api-client-servlet/s...
>
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notifications/NotificationServlet.java:72:
> * By default it uses {@link MemoryDataStoreFactory#getDefaultInstance()} which
> means it will NOT
> On 2013/06/18 21:27:43, peleyal wrote:
> > please add WARNING at the beginning of this <p>
> 
> Done.
> 
>
https://codereview.appspot.com/9045044/diff/91002/google-api-client-servlet/s...
>
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notifications/NotificationServlet.java:73:
> * persist the notification channels when the servlet process dies, so it is a
> bad choice for a
> On 2013/06/18 21:27:43, peleyal wrote:
> > BAD CHOICE
> 
> Done.
> 
>
https://codereview.appspot.com/9045044/diff/91002/google-api-client-servlet/s...
>
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notifications/NotificationServlet.java:105:
> * application.
> On 2013/06/18 21:27:43, peleyal wrote:
> > cool :)
> > I prefer as many explanations as possible
> 
> ACK
> 
>
https://codereview.appspot.com/9045044/diff/91002/google-api-client-servlet/s...
>
google-api-client-servlet/src/main/java/com/google/api/client/googleapis/extensions/servlet/notifications/NotificationServlet.java:117:
> protected NotificationServlet(DataStoreFactory dataStoreFactory) throws
> IOException {
> On 2013/06/18 21:27:43, peleyal wrote:
> > [optional]
> > I think that you can remove this constructor and only use directly the one
> > below. so the constructor above will look like this:
> > 
> >  public NotificationServlet() throws IOException {
> >  
> >
>
this(StoredChannel.getDefaultDataStore(MemoryDataStoreFactory.getDefaultInstance()));
> > }
> 
> No, actually the whole point is that this is the recommended constructor to
use
> that takes DataStoreFactory.  You normally shouldn't need to use the
> DataStore<StoredChannel> constructor unless you need to specify which
> notification data store to use other than the default one.
> 
>
https://codereview.appspot.com/9045044/diff/91002/google-api-client/src/main/...
> File
>
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/TypedNotificationCallback.java
> (right):
> 
>
https://codereview.appspot.com/9045044/diff/91002/google-api-client/src/main/...
>
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/TypedNotificationCallback.java:85:
> * Any state changes occurring during this notification will not be persisted.
> On 2013/06/18 21:27:43, peleyal wrote:
> > I tried to understand what you mean here and I didn't succeeded, so maybe
it's
> > better to improve that one
> 
> Removed it.  It doesn't make sense.  Persistence of this object isn't done
here,
> so it doesn't make sense to document it here.
> 
>
https://codereview.appspot.com/9045044/diff/91002/google-api-client/src/main/...
> File
>
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/UnparsedNotificationCallback.java
> (right):
> 
>
https://codereview.appspot.com/9045044/diff/91002/google-api-client/src/main/...
>
google-api-client/src/main/java/com/google/api/client/googleapis/notifications/UnparsedNotificationCallback.java:68:
> * Any state changes occurring during this notification will not be persisted.
> On 2013/06/18 21:27:43, peleyal wrote:
> > ditto
> 
> ditto
> 
>
https://codereview.appspot.com/9045044/diff/91002/google-api-client/src/main/...
> File
>
google-api-client/src/main/java/com/google/api/client/googleapis/testing/notifications/MockUnparsedNotificationCallback.java
> (right):
> 
>
https://codereview.appspot.com/9045044/diff/91002/google-api-client/src/main/...
>
google-api-client/src/main/java/com/google/api/client/googleapis/testing/notifications/MockUnparsedNotificationCallback.java:28:
> * @author Yaniv Inbar
> On 2013/06/18 21:27:43, peleyal wrote:
> > @author Matthias Linder (mlinder)
> 
> Done.
> 
>
https://codereview.appspot.com/9045044/diff/91002/google-api-client/src/main/...
> File
>
google-api-client/src/main/java/com/google/api/client/googleapis/testing/notifications/package-info.java
> (right):
> 
>
https://codereview.appspot.com/9045044/diff/91002/google-api-client/src/main/...
>
google-api-client/src/main/java/com/google/api/client/googleapis/testing/notifications/package-info.java:20:
> * @since 1.16
> On 2013/06/18 21:27:43, peleyal wrote:
> > @author Matthias Linder (mlinder)
> 
> Done.