Module: testCookies/testDisableCookies.js Test-page: test-files/cookies/cookie_single.html

cleanup + converts test to make use of local-data

Comment on attachment 467024 [details] [diff] [review] Patch v1 - (default) >+ cm = Cc["@mozilla.org/cookiemanager;1"] >+ .getService(Ci.nsICookieManager2); > cm.removeAll(); Please move the . to the end of the first line and indent getService 2 spaces from Cc. Adding feedback? for Henrik. Henrik, I'm thinking we shouldn't have any code like this in tests. We should move this to an API (testCookiesAPI in this case). Even if the API file is only a few lines, it can be built out as we go. I don't think we should burden our community with having to know this stuff. Could/should we add this to the refactor work?

(In reply to comment #2) > >+ cm = Cc["@mozilla.org/cookiemanager;1"] > >+ .getService(Ci.nsICookieManager2); > > cm.removeAll(); > > Please move the . to the end of the first line and indent getService 2 spaces > from Cc. Please don't indent. Should start at the same column. > Adding feedback? for Henrik. Henrik, I'm thinking we shouldn't have any code > like this in tests. We should move this to an API (testCookiesAPI in this > case). Even if the API file is only a few lines, it can be built out as we go. > I don't think we should burden our community with having to know this stuff. > Could/should we add this to the refactor work? Makes sense. I wouldn't wait for the refactoring but go ahead. File a new bug which adds the CookieAPI and the functions we need. Further all tests which make use of it should be updated too.

fixes indentation Takes Anthony's comment about the . at the end, and Henrik's about two spaces from the margin. If this is still wrong, you guys need to come to agreement

With no indentation I meant no 2 more blanks. Sorry. See the following code: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#267

(In reply to comment #5) > With no indentation I meant no 2 more blanks. Sorry. See the following code: > http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#267 Yes, Aaron, follow this example. FYI, if you ever have any formatting questions, always assume the Mozilla Style guidelines. You can refer to source code on MXR for examples.

Sold. Using the example.

Comment on attachment 471557 [details] [diff] [review] Patch v1.2 - (default) This test will always pass due to the missing update of the following line: controller.assertJS("subject.cookieCount == 0", {cookieCount : cm.countCookiesFromHost(".mozilla.org")});

Good catch. The cookie preferences manager for the cookie we create in cookie_single.html, has no specified domain nor a comparable path, i.e., (/Users/Aaron/). Can we use cookieExists to check for a blank domain and none specific path for this local cookie we create?

Scratch that above comment, was thinking locally instead of localhost. Patch changes the assert to check the count of the cookies from host localhost

Comment on attachment 474823 [details] [diff] [review] Patch v1.3 - [localhost] - (default) >- // Search for a cookie from mozilla.org and verify cookies are not saved >+ // Search for a cookie from localhost and verify cookies are not saved > controller.assertJS("subject.cookieCount == 0", >- {cookieCount : cm.countCookiesFromHost(".mozilla.org")}); >+ {cookieCount : cm.countCookiesFromHost("localhost")}); I don't like that we hard-code the value. Please grab the hostname directly from the url in the locationbar after the test page has been loaded. There is an API function you can use. It's fairly straight forward.

You talking about nsIURI.host or simply window.content.location.hostname?

Whatever makes it easier.

Comment on attachment 477124 [details] [diff] [review] Patch v1.4 - [fix from comment 11] - (default) Same as for the other cookie test. Please use persisted to store values which are shared between functions. In the future only update one of the tests which need the same updates. It will lower the amount of review work.

Please let this be the last one :D

Comment on attachment 477504 [details] [diff] [review] Patch v1.5 - (default) >+ persisted.hostName = {}; The last part you will have to update is that line. Please assign 'undefined' otherwise we have an empty object lingering around for the remaining tests. With that change r=me.

(In reply to comment #19) > Created attachment 477953 [details] [diff] [review] > Patch v1.5 - (default + teardown change) Landed: http://hg.mozilla.org/qa/mozmill-tests/rev/9a2c4c472264 [default] http://hg.mozilla.org/qa/mozmill-tests/rev/d1e39d7472f8 [mozilla1.9.2] http://hg.mozilla.org/qa/mozmill-tests/rev/c1f94b26ce1c [mozilla1.9.1]

Move of Mozmill Test related project bugs to newly created components. You can filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.