$cookies service refactoring

- remove obsolete code in tests
- add warning logs when maximum cookie limits (as specified via RFC 2965) were reached
- non-string values will now get dropped
- after each update $cookies hash will reflect the actual state of browser cookies
  this means that if browser drops some cookies due to cookie overflow, $cookies will reflect that
- $sessionStore got renamed to $cookieStore to avoid name conflicts with html5's sessionStore

6 files changed, 147 insertions, 37 deletions

diff --git a/src/AngularPublic.js b/src/AngularPublic.js
index 40425b8d..617a7e2e 100644
--- a/src/AngularPublic.js
+++ b/src/AngularPublic.js
@@ -1,16 +1,17 @@
 var browserSingleton;
-angularService('$browser', function browserFactory(){
+angularService('$browser', function($log){
   if (!browserSingleton) {
     browserSingleton = new Browser(
         window.location,
         jqLite(window.document),
         jqLite(window.document.getElementsByTagName('head')[0]),
-        XHR);
+        XHR,
+        $log);
     browserSingleton.startPoller(50, function(delay, fn){setTimeout(delay,fn);});
     browserSingleton.bind();
   }
   return browserSingleton;
-});
+}, {inject:['$log']});
 
 extend(angular, {
   'element': jqLite,
diff --git a/src/Browser.js b/src/Browser.js
index e21e419b..c7e61e97 100644
--- a/src/Browser.js
+++ b/src/Browser.js
@@ -8,7 +8,7 @@ var XHR = window.XMLHttpRequest || function () {
   throw new Error("This browser does not support XMLHttpRequest.");
 };
 
-function Browser(location, document, head, XHR) {
+function Browser(location, document, head, XHR, $log) {
   var self = this;
   self.isMock = false;
 
@@ -106,28 +106,50 @@ function Browser(location, document, head, XHR) {
   var rawDocument = document[0];
   var lastCookies = {};
   var lastCookieString = '';
+
   /**
-   * cookies() -> hash of all cookies
-   * cookies(name, value) -> set name to value
-   *   if value is undefined delete it
-   * cookies(name) -> should get value, but deletes (no one calls it right now that way)
+   * The cookies method provides a 'private' low level access to browser cookies. It is not meant to
+   * be used directly, use the $cookie service instead.
+   *
+   * The return values vary depending on the arguments that the method was called with as follows:
+   * <ul><li>
+   * cookies() -> hash of all cookies, this is NOT a copy of the internal state, so do not modify it
+   * </li><li>
+   * cookies(name, value) -> set name to value, if value is undefined delete the cookie
+   * </li><li>
+   * cookies(name) -> the same as (name, undefined) == DELETES (no one calls it right now that way)
+   * </li></ul>
    */
-  self.cookies = function (name, value){
+  self.cookies = function (/**string*/name, /**string*/value){
+    var cookieLength, cookieArray, i, keyValue;
+
     if (name) {
       if (value === _undefined) {
         delete lastCookies[name];
         rawDocument.cookie = escape(name) + "=;expires=Thu, 01 Jan 1970 00:00:00 GMT";
       } else {
-        rawDocument.cookie = escape(name) + '=' + escape(lastCookies[name] = ''+value);
+        if (isString(value)) {
+          rawDocument.cookie = escape(name) + '=' + escape(lastCookies[name] = value);
+
+          cookieLength = name.length + value.length + 1;
+          if (cookieLength > 4096) {
+            $log.warn("Cookie '"+ name +"' possibly not set or overflowed because it was too large ("+
+              cookieLength + " > 4096 bytes)!");
+          }
+          if (lastCookies.length > 20) {
+            $log.warn("Cookie '"+ name +"' possibly not set or overflowed because too many cookies " +
+              "were already set (" + lastCookies.length + " > 20 )");
+          }
+        }
       }
     } else {
       if (rawDocument.cookie !== lastCookieString) {
         lastCookieString = rawDocument.cookie;
-        var cookieArray = lastCookieString.split("; ");
+        cookieArray = lastCookieString.split("; ");
         lastCookies = {};
 
-        for (var i = 0; i < cookieArray.length; i++) {
-          var keyValue = cookieArray[i].split("=");
+        for (i = 0; i < cookieArray.length; i++) {
+          keyValue = cookieArray[i].split("=");
           if (keyValue.length === 2) { //ignore nameless cookies
             lastCookies[unescape(keyValue[0])] = unescape(keyValue[1]);
           }
diff --git a/src/services.js b/src/services.js
index f6dc931d..ddeebe1f 100644
--- a/src/services.js
+++ b/src/services.js
@@ -397,8 +397,18 @@ angularService('$resource', function($xhr){
 }, {inject: ['$xhr.cache']});
 
 
+/**
+ * $cookies service provides read/write access to the browser cookies. Currently only session
+ * cookies are supported.
+ *
+ * Only a simple Object is exposed and by adding or removing properties to/from this object, new
+ * cookies are created or deleted from the browser at the end of the current eval.
+ */
 angularService('$cookies', function($browser) {
-  var cookies = {}, rootScope = this, lastCookies;
+  var cookies = {},
+      rootScope = this,
+      lastCookies;
+
   $browser.addPollFn(function(){
     var currentCookies = $browser.cookies();
     if (lastCookies != currentCookies) {
@@ -407,38 +417,61 @@ angularService('$cookies', function($browser) {
       rootScope.$eval();
     }
   });
+
   this.$onEval(PRIORITY_FIRST, update);
   this.$onEval(PRIORITY_LAST, update);
+
   return cookies;
 
   function update(){
-    var name, browserCookies = $browser.cookies();
+    var name,
+        browserCookies = $browser.cookies();
+
+    //$cookies -> $browser
     for(name in cookies) {
-      if (browserCookies[name] !== cookies[name]) {
-        $browser.cookies(name, browserCookies[name] = cookies[name]);
+      if (cookies[name] !== browserCookies[name]) {
+        $browser.cookies(name, cookies[name]);
       }
     }
+
+    //get what was actually stored in the browser
+    browserCookies = $browser.cookies();
+
+    //$browser -> $cookies
     for(name in browserCookies) {
-      if (browserCookies[name] !== cookies[name]) {
+      if (isUndefined(cookies[name])) {
         $browser.cookies(name, _undefined);
+      } else {
+        cookies[name] = browserCookies[name];
+      }
+    }
+
+    //drop cookies in $cookies for cookies that $browser or real browser dropped
+    for (name in cookies) {
+      if (isUndefined(browserCookies[name])) {
+        delete cookies[name];
       }
     }
   }
 }, {inject: ['$browser']});
 
 
-angularService('$sessionStore', function($store) {
+/**
+ * $cookieStore provides a key-value (string-object) storage that is backed by session cookies.
+ * Objects put or retrieved from this storage are automatically serialized or deserialized.
+ */
+angularService('$cookieStore', function($store) {
 
   return {
-    get: function(key) {
+    get: function(/**string*/key) {
       return fromJson($store[key]);
     },
 
-    put: function(key, value) {
+    put: function(/**string*/key, /**Object*/value) {
       $store[key] = toJson(value);
     },
 
-    remove: function(key) {
+    remove: function(/**string*/key) {
       delete $store[key];
     }
   };
diff --git a/test/BrowserSpecs.js b/test/BrowserSpecs.js
index abd761eb..148fbca4 100644
--- a/test/BrowserSpecs.js
+++ b/test/BrowserSpecs.js
@@ -72,11 +72,16 @@ describe('browser', function(){
       }
     }
 
-    var browser;
+    var browser, log, logs;
 
     beforeEach(function() {
       deleteAllCookies();
-      browser = new Browser({}, jqLite(document));
+      logs = {log:[], warn:[], info:[], error:[]}
+      log = {log: function() {logs.log.push(Array.prototype.slice.call(arguments))},
+             warn: function() {logs.warn.push(Array.prototype.slice.call(arguments))},
+             info: function() {logs.info.push(Array.prototype.slice.call(arguments))},
+             error: function() {logs.error.push(Array.prototype.slice.call(arguments))}};
+      browser = new Browser({}, jqLite(document), undefined, XHR, log);
       expect(document.cookie).toEqual('');
     });
 
@@ -121,7 +126,7 @@ describe('browser', function(){
 
       it('should create and store a cookie', function() {
         browser.cookies('cookieName', 'cookieValue');
-        expect(document.cookie).toEqual('cookieName=cookieValue');
+        expect(document.cookie).toMatch(/cookieName=cookieValue;? ?/);
         expect(browser.cookies()).toEqual({'cookieName':'cookieValue'});
       });
 
@@ -145,6 +150,47 @@ describe('browser', function(){
         expect(rawCookies).toContain('cookie1%3D=val%3Bue');
         expect(rawCookies).toContain('cookie2%3Dbar%3Bbaz=val%3Due');
       });
+
+      it('should log warnings when 4kb per cookie storage limit is reached', function() {
+        var i, longVal = '', cookieString;
+
+        for(i=0; i<4092; i++) {
+          longVal += '+';
+        }
+
+        cookieString = document.cookie;
+        browser.cookies('x', longVal); //total size 4094-4096, so it should go through
+        expect(document.cookie).not.toEqual(cookieString);
+        expect(browser.cookies()['x']).toEqual(longVal);
+        expect(logs.warn).toEqual([]);
+
+        browser.cookies('x', longVal + 'xxx') //total size 4097-4099, a warning should be logged
+        //browser behavior is undefined, so we test for existance of warning logs only
+        expect(logs.warn).toEqual(
+          [[ "Cookie 'x' possibly not set or overflowed because it was too large (4097 > 4096 " +
+             "bytes)!" ]]);
+      });
+
+      it('should log warnings when 20 cookies per domain storage limit is reached', function() {
+        var i, str;
+
+        for (i=0; i<20; i++) {
+          str = '' + i;
+          browser.cookies(str, str);
+        }
+
+        i=0;
+        for (str in browser.cookies()) {
+          i++;
+        }
+        expect(i).toEqual(20);
+        expect(logs.warn).toEqual([]);
+
+        browser.cookies('one', 'more');
+        //browser behavior is undefined, so we test for existance of warning logs only
+        expect(logs.warn).toEqual([]);
+      });
+
     });
 
 
@@ -157,14 +203,12 @@ describe('browser', function(){
 
       it ('should return a value for an existing cookie', function() {
         document.cookie = "foo=bar";
-        browser.cookies(true);
         expect(browser.cookies().foo).toEqual('bar');
       });
 
 
       it ('should unescape cookie values that were escaped by puts', function() {
         document.cookie = "cookie2%3Dbar%3Bbaz=val%3Due";
-        browser.cookies(true);
         expect(browser.cookies()['cookie2=bar;baz']).toEqual('val=ue');
       });
 
@@ -197,7 +241,6 @@ describe('browser', function(){
       expect(browser.cookies()).toEqual({'oatmealCookie':'drool'});
 
       document.cookie = 'oatmealCookie=changed';
-      browser.cookies(true);
       expect(browser.cookies().oatmealCookie).toEqual('changed');
     });
 
@@ -233,4 +276,3 @@ describe('browser', function(){
     });
   });
 });
-
diff --git a/test/angular-mocks.js b/test/angular-mocks.js
index a0d25042..5b5c9863 100644
--- a/test/angular-mocks.js
+++ b/test/angular-mocks.js
@@ -102,7 +102,9 @@ MockBrowser.prototype = {
       if (value == undefined) {
         delete this.cookieHash[name];
       } else {
-        this.cookieHash[name] = ""+value;
+        if (isString(value)) {
+          this.cookieHash[name] = value;
+        }
       }
     } else {
       return copy(this.cookieHash);
diff --git a/test/servicesSpec.js b/test/servicesSpec.js
index 28bde598..3416f0ea 100644
--- a/test/servicesSpec.js
+++ b/test/servicesSpec.js
@@ -373,13 +373,22 @@ describe("service", function(){
 
   describe('$cookies', function() {
 
-    it('should provide access to existing cookies via object properties', function(){
+    it('should provide access to existing cookies via object properties and keep them in sync',
+        function(){
       expect(scope.$cookies).toEqual({});
 
       scope.$browser.cookies('brandNew', 'cookie');
       scope.$browser.poll();
 
       expect(scope.$cookies).toEqual({'brandNew':'cookie'});
+
+      scope.$browser.cookies('brandNew', 'cookie2');
+      scope.$browser.poll();
+      expect(scope.$cookies).toEqual({'brandNew':'cookie2'});
+
+      scope.$browser.cookies('brandNew', undefined);
+      scope.$browser.poll();
+      expect(scope.$cookies).toEqual({});
     });
 
 
@@ -396,10 +405,11 @@ describe("service", function(){
     });
 
 
-    it('should turn non-string into String when creating a cookie', function() {
+    it('should ignore non-string values when asked to create a cookie', function() {
       scope.$cookies.nonString = [1, 2, 3];
       scope.$eval();
-      expect(scope.$browser.cookies()).toEqual({'nonString':'1,2,3'});
+      expect(scope.$browser.cookies()).toEqual({});
+      expect(scope.$cookies).toEqual({});
     });
 
 
@@ -425,10 +435,10 @@ describe("service", function(){
   });
 
 
-  describe('$sessionStore', function() {
+  describe('$cookieStore', function() {
 
     it('should serialize objects to json', function() {
-      scope.$sessionStore.put('objectCookie', {id: 123, name: 'blah'});
+      scope.$cookieStore.put('objectCookie', {id: 123, name: 'blah'});
       scope.$eval(); //force eval in test
       expect(scope.$browser.cookies()).toEqual({'objectCookie': '{"id":123,"name":"blah"}'});
     });
@@ -437,12 +447,12 @@ describe("service", function(){
     it('should deserialize json to object', function() {
       scope.$browser.cookies('objectCookie', '{"id":123,"name":"blah"}');
       scope.$browser.poll();
-      expect(scope.$sessionStore.get('objectCookie')).toEqual({id: 123, name: 'blah'});
+      expect(scope.$cookieStore.get('objectCookie')).toEqual({id: 123, name: 'blah'});
     });
 
 
     it('should delete objects from the store when remove is called', function() {
-      scope.$sessionStore.put('gonner', { "I'll":"Be Back"});
+      scope.$cookieStore.put('gonner', { "I'll":"Be Back"});
       scope.$eval(); //force eval in test
       expect(scope.$browser.cookies()).toEqual({'gonner': '{"I\'ll":"Be Back"}'});
     });