Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10536017:
Refactoring CookiesTreeModel to support multiple data sources. (Closed)
|
Created:
8 years, 6 months ago by nasko Modified:
8 years, 3 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRefactoring CookiesTreeModel to support multiple data sources.
BUG=126093
TEST=Unit tests and manual verification.
Patch Set 1 #Patch Set 2 : Almost working, needs fixing the Remove(All) buttons. #Patch Set 3 : Mostly working, just need to refresh properly on Remove All #
Total comments: 8
Patch Set 4 : Remove All now works. #Patch Set 5 : Some cleanup performed. #Patch Set 6 : More cleanup done. #
Total comments: 34
Patch Set 7 : Fixing based on review feedback. #Patch Set 8 : Moving from wstring to string16. #
Total comments: 77
Patch Set 9 : Another wave of fixes to Markus' review. #Patch Set 10 : Refactoring common code and style fixes. #
Total comments: 24
Patch Set 11 : Fixed all comments by Evan. #
Total comments: 55
Patch Set 12 : Fixes for commes by James and Evan. #Messages
Total messages: 22 (0 generated)
Since this CL needed to be so huge I will review in several chucks. I'm a bit to tired now to finish reviewing the cookies_tree_mode* changes. So here are some first comments. https://chromiumcodereview.appspot.com/10536017/diff/2002/chrome/browser/cook... File chrome/browser/cookies_tree_model.cc (right): https://chromiumcodereview.appspot.com/10536017/diff/2002/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:401: /* Please remove commented code. https://chromiumcodereview.appspot.com/10536017/diff/2002/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:940: for (ContainerMap::iterator ait = app_data_map_.begin(); Please replace "ait" with a better name. https://chromiumcodereview.appspot.com/10536017/diff/2002/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1370: : //app_name_(app_name), Uncomment these to lines please. https://chromiumcodereview.appspot.com/10536017/diff/2002/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1372: appcache_helper_(appcache_helper), s/(app_id_)/(app_id) https://chromiumcodereview.appspot.com/10536017/diff/2002/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1381: delegate_(NULL), Please indent. https://chromiumcodereview.appspot.com/10536017/diff/2002/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1382: //use_cookie_source_(use_cookie_source), Same as above. Please remove or uncomment. https://chromiumcodereview.appspot.com/10536017/diff/2002/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1384: app_name_ = app_name; I think it is preferred to initialize these two field using initializers. Please remove these two lines. https://chromiumcodereview.appspot.com/10536017/diff/2002/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1391: void LocalDataContainer::Init(CookiesTreeModelDelegate* d) { s/d/delegate https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... File chrome/browser/cookies_tree_model.h (right): https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:54: class CookieTreeAppNode; nit: It looks like this list of forward declarations used to be alphabetically sorted. Maybe you could add the new lines at the right positions. If you could even restore the order that would be awsome. But at least move line 54 to line 38. https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:60: typedef std::list<net::CookieMonster::CanonicalCookie> CookieList; This typedef is not necessary. The cookie monster has already a CookieList. See net/cookies/cookie_monster.h:966 It's net::CookieList. https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:70: typedef std::list<BrowsingDataQuotaHelper::QuotaInfo> QuotaInfoArray; nit: s/QuotaInfoArray/QuotaInfoList 1) To be consistent with the other names. 2) It is a list and not an Array. Or is there a particular reason why you named it Array? https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:90: TYPE_APP, // This is used for CookieTreeAppNode nodes. nit: Add one more space before the comment in this line. https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:273: // CookieTreeAppNode ------------------------------------------------------- nit: Add some more '-' to make this line as long as the other comments. https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:284: const string16 app_id() const { return app_id_; } I think you can return a const reference here: const string16& https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:288: string16 app_id_; nit: Please add an empty line here. https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:673: class CookiesTreeModelDelegate { Please document this class (interface). Thanks. https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:674: public: All public methods should be documented. Please document the method below. https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:675: virtual void PopulateAppCacheInfoWithFilter(const string16* app, Is |app| the app name of the app id? maybe you could change the name accordingly. Is there a particular reason why you use const pointers here? Please use const refs (const string16& app) if possible. I think the rule is that we don't have const pointers in the parameter list but const refs. same below. https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:676: const std::wstring& filter) {} Here and below: Please don't use std::wstring. Replace std::wstring with string16. Everywhere. You can use the base/utf_string_conversions.h to convert from a wide string to string16 if really necessary. I think there is a presubmit check that check if you introduce new std::wstrings https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:680: const std::wstring& filter) {} Please don't use std::wstring. wstring is replaced with string16. https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:725: void DeleteStoredObjectsForApp(const string16& app); Please document the method and replace app with app_id. I know there is not much documentation in this class, but we can improve this going forward. https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:737: nit: Add "// CookiesTreeModelDelegate methods:" https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:738: virtual void PopulateAppCacheInfoWithFilter( In all method signatures: s/app/app_id https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:789: class LocalDataContainer { Please document the class by adding a comment.
Fixes for most comments, answers on some, wstring->string16 will happen in a separate update. https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... File chrome/browser/cookies_tree_model.h (right): https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:54: class CookieTreeAppNode; On 2012/06/13 23:40:39, markusheintz_ wrote: > nit: It looks like this list of forward declarations used to be alphabetically > sorted. Maybe you could add the new lines at the right positions. If you could > even restore the order that would be awsome. But at least move line 54 to line > 38. Done. https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:60: typedef std::list<net::CookieMonster::CanonicalCookie> CookieList; On 2012/06/13 23:40:39, markusheintz_ wrote: > This typedef is not necessary. The cookie monster has already a CookieList. See > net/cookies/cookie_monster.h:966 > It's net::CookieList. Just moved code around. If I change it to the net::CookieList, it doesn't behave identically, so requires some extra work. I can do it once everything else is complete, assuming you feel strongly about it. https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:70: typedef std::list<BrowsingDataQuotaHelper::QuotaInfo> QuotaInfoArray; On 2012/06/13 23:40:39, markusheintz_ wrote: > nit: s/QuotaInfoArray/QuotaInfoList > 1) To be consistent with the other names. > 2) It is a list and not an Array. > > Or is there a particular reason why you named it Array? All of these were just existing code being moved around in the file. I'm happy to fix it though : ). https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:90: TYPE_APP, // This is used for CookieTreeAppNode nodes. On 2012/06/13 23:40:39, markusheintz_ wrote: > nit: Add one more space before the comment in this line. > Done. https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:273: // CookieTreeAppNode ------------------------------------------------------- On 2012/06/13 23:40:39, markusheintz_ wrote: > nit: Add some more '-' to make this line as long as the other comments. Done. https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:284: const string16 app_id() const { return app_id_; } On 2012/06/13 23:40:39, markusheintz_ wrote: > I think you can return a const reference here: const string16& Done. https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:288: string16 app_id_; On 2012/06/13 23:40:39, markusheintz_ wrote: > nit: Please add an empty line here. Done. https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:673: class CookiesTreeModelDelegate { On 2012/06/13 23:40:39, markusheintz_ wrote: > Please document this class (interface). Thanks. Done. https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:674: public: On 2012/06/13 23:40:39, markusheintz_ wrote: > All public methods should be documented. Please document the method below. As discussed, I'm just moving those around and they seem pretty straighforward methods, so skipping documentation for now. https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:675: virtual void PopulateAppCacheInfoWithFilter(const string16* app, On 2012/06/13 23:40:39, markusheintz_ wrote: > Is |app| the app name of the app id? maybe you could change the name > accordingly. > > Is there a particular reason why you use const pointers here? > Please use const refs (const string16& app) if possible. I think the rule is > that we don't have const pointers in the parameter list but const refs. > > same below. Yes, there is a reason for the pointer. I want to distinguish between the app id of the "browser process" which is the empty string and a case where we don't have an app id. https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:676: const std::wstring& filter) {} On 2012/06/13 23:40:39, markusheintz_ wrote: > Here and below: Please don't use std::wstring. Replace std::wstring with > string16. Everywhere. > > You can use the base/utf_string_conversions.h to convert from a wide string to > string16 if really necessary. > > I think there is a presubmit check that check if you introduce new std::wstrings > I'll move those over to string16. https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:680: const std::wstring& filter) {} On 2012/06/13 23:40:39, markusheintz_ wrote: > Please don't use std::wstring. wstring is replaced with string16. See the above. https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:725: void DeleteStoredObjectsForApp(const string16& app); On 2012/06/13 23:40:39, markusheintz_ wrote: > Please document the method and replace app with app_id. > > I know there is not much documentation in this class, but we can improve this > going forward. Done. https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:737: On 2012/06/13 23:40:39, markusheintz_ wrote: > nit: Add "// CookiesTreeModelDelegate methods:" Done. https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:738: virtual void PopulateAppCacheInfoWithFilter( On 2012/06/13 23:40:39, markusheintz_ wrote: > In all method signatures: > s/app/app_id Done. https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:789: class LocalDataContainer { On 2012/06/13 23:40:39, markusheintz_ wrote: > Please document the class by adding a comment. Done.
Thanks a lot for replacing all the wstrings. Here a some more comments. http://codereview.chromium.org/10536017/diff/5013/chrome/browser/cookies_tree... File chrome/browser/cookies_tree_model.cc (right): http://codereview.chromium.org/10536017/diff/5013/chrome/browser/cookies_tree... chrome/browser/cookies_tree_model.cc:155: Please delete the empty line here. http://codereview.chromium.org/10536017/diff/5013/chrome/browser/cookies_tree... chrome/browser/cookies_tree_model.cc:188: CookieTreeAppNode* app = static_cast<CookieTreeAppNode*>( You repeat these 4 lines again and again in other method below. Could you extract them into a helper function or method? http://codereview.chromium.org/10536017/diff/5013/chrome/browser/cookies_tree... chrome/browser/cookies_tree_model.cc:737: CookiesTreeModel::CookiesTreeModel(ContainerMap& apps_map, Below you copy the apps_map. I think you can avoid this by passing a pointer to a ContainerMap, and taking the ownership of the map. If you prefer to keep it this way, then please make |apps_map| a const ref. http://codereview.chromium.org/10536017/diff/5013/chrome/browser/cookies_tree... chrome/browser/cookies_tree_model.cc:744: it != apps_map.end(); ++it) { Please fix indentation here. Thanks. http://codereview.chromium.org/10536017/diff/5013/chrome/browser/ui/webui/opt... File chrome/browser/ui/webui/options2/cookies_view_handler2.cc (right): http://codereview.chromium.org/10536017/diff/5013/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:119: LOG(ERROR) << "TreeNodesAdded: " << start << "|" << count; Please remove this line. http://codereview.chromium.org/10536017/diff/5013/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:135: LOG(ERROR) << "TreeNodesAdded: " << Please remove these lines http://codereview.chromium.org/10536017/diff/5013/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:150: LOG(ERROR) << "TreeNodesRemoved: " << start << "|" << count; Please rm http://codereview.chromium.org/10536017/diff/5013/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:165: LOG(ERROR) << "TreeModelBeginBatch"; Please rm. http://codereview.chromium.org/10536017/diff/5013/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:172: LOG(ERROR) << "TreeModelEndBatch"; Please rm. http://codereview.chromium.org/10536017/diff/5013/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:179: string16 name = ASCIIToUTF16("Browser-wide"); You use "Site-Data" as name in other places. Please use it here as well. http://codereview.chromium.org/10536017/diff/5013/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:185: container = new LocalDataContainer( nit: I think you can just say: apps_map[browser_id] = new LocalDataContainer( ... http://codereview.chromium.org/10536017/diff/5013/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:200: string16 id; nit: I think you call this app_id every where else. Maybe you can call it app_id here as well. http://codereview.chromium.org/10536017/diff/5013/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:203: it != extensions->end(); ++it) { Indent one space. http://codereview.chromium.org/10536017/diff/5013/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:209: container = new LocalDataContainer( nit: Same as above: I think you can just say: apps_map[browser_id] = new LocalDataContainer( ... http://codereview.chromium.org/10536017/diff/5013/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:237: LOG(ERROR) << "UpdateSearchResults"; Please rm. http://codereview.chromium.org/10536017/diff/5013/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:244: LOG(ERROR) << "RemoveAll"; Please rm. http://codereview.chromium.org/10536017/diff/5013/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:261: LOG(ERROR) << "RemoveChildren: " << node_path << "|" << node; Please rm. http://codereview.chromium.org/10536017/diff/5013/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:277: LOG(ERROR) << "LoadChildren: " << node_path << "|" << node; Please rm. http://codereview.chromium.org/10536017/diff/5013/chrome/browser/ui/webui/opt... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:288: LOG(ERROR) << "SendChildren: " << Please rm.
Fixes based on comments. I've kept the logging for debugging purposes, but I've removed it too. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... File chrome/browser/cookies_tree_model.cc (right): https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:155: On 2012/06/18 16:52:41, markusheintz_ wrote: > Please delete the empty line here. Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:188: CookieTreeAppNode* app = static_cast<CookieTreeAppNode*>( On 2012/06/18 16:52:41, markusheintz_ wrote: > You repeat these 4 lines again and again in other method below. Could you > extract them into a helper function or method? The reason I didn't is that it assumes a level in the tree, which is not consistent. Look for example at the quota node, it has one level less depth. If you feel really strongly about this, I can add it to the inheritance chain of tree nodes, accounting for the edge cases. To me, this was more clear way to communicate the assumptions. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:737: CookiesTreeModel::CookiesTreeModel(ContainerMap& apps_map, On 2012/06/18 16:52:41, markusheintz_ wrote: > Below you copy the apps_map. > > I think you can avoid this by passing a pointer to a ContainerMap, and taking > the ownership of the map. > > If you prefer to keep it this way, then please make |apps_map| a const ref. Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:744: it != apps_map.end(); ++it) { On 2012/06/18 16:52:41, markusheintz_ wrote: > Please fix indentation here. Thanks. Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/ui/w... File chrome/browser/ui/webui/options2/cookies_view_handler2.cc (right): https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/ui/w... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:119: LOG(ERROR) << "TreeNodesAdded: " << start << "|" << count; On 2012/06/18 16:52:41, markusheintz_ wrote: > Please remove this line. Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/ui/w... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:135: LOG(ERROR) << "TreeNodesAdded: " << On 2012/06/18 16:52:41, markusheintz_ wrote: > Please remove these lines Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/ui/w... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:150: LOG(ERROR) << "TreeNodesRemoved: " << start << "|" << count; On 2012/06/18 16:52:41, markusheintz_ wrote: > Please rm Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/ui/w... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:165: LOG(ERROR) << "TreeModelBeginBatch"; On 2012/06/18 16:52:41, markusheintz_ wrote: > Please rm. Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/ui/w... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:172: LOG(ERROR) << "TreeModelEndBatch"; On 2012/06/18 16:52:41, markusheintz_ wrote: > Please rm. Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/ui/w... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:179: string16 name = ASCIIToUTF16("Browser-wide"); On 2012/06/18 16:52:41, markusheintz_ wrote: > You use "Site-Data" as name in other places. Please use it here as well. In the other cases, it is UI only for a single site. In this case, it is the browser wide storage, so the two uses are different. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/ui/w... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:185: container = new LocalDataContainer( On 2012/06/18 16:52:41, markusheintz_ wrote: > nit: I think you can just say: > apps_map[browser_id] = new LocalDataContainer( ... Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/ui/w... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:200: string16 id; On 2012/06/18 16:52:41, markusheintz_ wrote: > nit: I think you call this app_id every where else. Maybe you can call it app_id > here as well. Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/ui/w... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:203: it != extensions->end(); ++it) { On 2012/06/18 16:52:41, markusheintz_ wrote: > Indent one space. Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/ui/w... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:209: container = new LocalDataContainer( On 2012/06/18 16:52:41, markusheintz_ wrote: > nit: Same as above: > > I think you can just say: > apps_map[browser_id] = new LocalDataContainer( ... Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/ui/w... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:237: LOG(ERROR) << "UpdateSearchResults"; On 2012/06/18 16:52:41, markusheintz_ wrote: > Please rm. Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/ui/w... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:244: LOG(ERROR) << "RemoveAll"; On 2012/06/18 16:52:41, markusheintz_ wrote: > Please rm. Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/ui/w... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:261: LOG(ERROR) << "RemoveChildren: " << node_path << "|" << node; On 2012/06/18 16:52:41, markusheintz_ wrote: > Please rm. Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/ui/w... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:277: LOG(ERROR) << "LoadChildren: " << node_path << "|" << node; On 2012/06/18 16:52:41, markusheintz_ wrote: > Please rm. Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/ui/w... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:288: LOG(ERROR) << "SendChildren: " << On 2012/06/18 16:52:41, markusheintz_ wrote: > Please rm. Done.
Thanks for your patience and sorry for the delay. Please check all for loop for the correct indentation. Mostly nits and two questions. Thanks https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... File chrome/browser/cookies_tree_model.h (right): https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:675: virtual void PopulateAppCacheInfoWithFilter(const string16* app, On 2012/06/14 18:46:42, nasko wrote: > On 2012/06/13 23:40:39, markusheintz_ wrote: > > Is |app| the app name of the app id? maybe you could change the name > > accordingly. > > > > Is there a particular reason why you use const pointers here? > > Please use const refs (const string16& app) if possible. I think the rule is > > that we don't have const pointers in the parameter list but const refs. > > > > same below. > > Yes, there is a reason for the pointer. I want to distinguish between the app id > of the "browser process" which is the empty string and a case where we don't > have an app id. Just to refresh my memory when is it again possible to have no app_id? https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... File chrome/browser/cookies_tree_model.cc (right): https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:188: CookieTreeAppNode* app = static_cast<CookieTreeAppNode*>( On 2012/06/18 19:59:14, nasko wrote: > On 2012/06/18 16:52:41, markusheintz_ wrote: > > You repeat these 4 lines again and again in other method below. Could you > > extract them into a helper function or method? > > The reason I didn't is that it assumes a level in the tree, which is not > consistent. Look for example at the quota node, it has one level less depth. If > you feel really strongly about this, I can add it to the inheritance chain of > tree nodes, accounting for the edge cases. To me, this was more clear way to > communicate the assumptions. Looks like the quota node is the only exemption. I think it's worth eliminating duplicated code. It's up to you to extract it to a method or add it to the inheritance chain. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:885: for (ContainerMap::iterator ait = app_data_map_.begin(); Here and in all other populate methods: I probably miss something but why are you iterating over all elements in the map rather than using if (app_id) ... = app_data_map_.find(app_id)? https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:904: origin != container->appcache_info_.end(); ++origin) { indent by 1 https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:914: info != origin->second.end(); ++info) { indent by 1 https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:946: it != container->cookie_list_.end(); ++it) { indent by 1 https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:986: if (container->database_info_list_.empty()) Here and in all other Populate methods: If you keep the other for loop then you may consolidate this if and the one below to a single if statement: if (container->database_info_list_.empty() || app_id && *app_id != container->app_id()) continue; https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1051: container->local_storage_info_list_.begin(); Fix indentation. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1099: container->session_storage_info_list_.begin(); indent by 5 next two lines indet by 1 https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1130: ait != app_data_map_.end(); ++ait ) { indent by one https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1148: container->indexed_db_info_list_.begin(); Indent by 5. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1149: indexed_db_info != container->indexed_db_info_list_.end(); this and the next line indent by 1 https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1178: ait != app_data_map_.end(); ++ait ) { Indentation. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1196: container->server_bound_cert_list_.begin(); Please fix indentation. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1232: ait != app_data_map_.end(); ++ait ) { Please fix indentation here. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1250: container->file_system_info_list_.begin(); Please fix indentation https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1280: ait != app_data_map_.end(); ++ait ) { Pls fix intendation. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1298: container->quota_info_list_.begin(); Please fix indentation here https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1371: void LocalDataContainer::Init(CookiesTreeModelDelegate* d) { nit: s/d/delegate/ https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1483: } nit: Please add an empty line here. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/ui/w... File chrome/browser/ui/webui/options2/cookies_view_handler2.cc (right): https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/ui/w... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:179: string16 name = ASCIIToUTF16("Browser-wide"); On 2012/06/18 19:59:14, nasko wrote: > On 2012/06/18 16:52:41, markusheintz_ wrote: > > You use "Site-Data" as name in other places. Please use it here as well. > I think it is fine to use "Site-data", since we also use "Cookies and site data" on the settings page. > In the other cases, it is UI only for a single site. In this case, it is the > browser wide storage, so the two uses are different.
I've addressed all the comments. https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... File chrome/browser/cookies_tree_model.h (right): https://chromiumcodereview.appspot.com/10536017/diff/14003/chrome/browser/coo... chrome/browser/cookies_tree_model.h:675: virtual void PopulateAppCacheInfoWithFilter(const string16* app, On 2012/06/19 20:01:33, markusheintz_ wrote: > On 2012/06/14 18:46:42, nasko wrote: > > On 2012/06/13 23:40:39, markusheintz_ wrote: > > > Is |app| the app name of the app id? maybe you could change the name > > > accordingly. > > > > > > Is there a particular reason why you use const pointers here? > > > Please use const refs (const string16& app) if possible. I think the rule is > > > that we don't have const pointers in the parameter list but const refs. > > > > > > same below. > > > > Yes, there is a reason for the pointer. I want to distinguish between the app > id > > of the "browser process" which is the empty string and a case where we don't > > have an app id. > > Just to refresh my memory when is it again possible to have no app_id? In the case where we refresh the tree model from WebUI, in which case we want to build the full tree, not just a single app. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... File chrome/browser/cookies_tree_model.cc (right): https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:188: CookieTreeAppNode* app = static_cast<CookieTreeAppNode*>( On 2012/06/19 20:01:34, markusheintz_ wrote: > On 2012/06/18 19:59:14, nasko wrote: > > On 2012/06/18 16:52:41, markusheintz_ wrote: > > > You repeat these 4 lines again and again in other method below. Could you > > > extract them into a helper function or method? > > > > The reason I didn't is that it assumes a level in the tree, which is not > > consistent. Look for example at the quota node, it has one level less depth. > If > > you feel really strongly about this, I can add it to the inheritance chain of > > tree nodes, accounting for the edge cases. To me, this was more clear way to > > communicate the assumptions. > > Looks like the quota node is the only exemption. > > I think it's worth eliminating duplicated code. > > It's up to you to extract it to a method or add it to the inheritance chain. Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:885: for (ContainerMap::iterator ait = app_data_map_.begin(); On 2012/06/19 20:01:34, markusheintz_ wrote: > Here and in all other populate methods: > > I probably miss something but why are you iterating over all elements in the map > rather than using > > if (app_id) > ... = app_data_map_.find(app_id)? The reason is that if app_id is NULL, you want to create a tree model spanning all the apps, including the browser-wide. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:904: origin != container->appcache_info_.end(); ++origin) { On 2012/06/19 20:01:34, markusheintz_ wrote: > indent by 1 Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:914: info != origin->second.end(); ++info) { On 2012/06/19 20:01:34, markusheintz_ wrote: > indent by 1 Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:946: it != container->cookie_list_.end(); ++it) { On 2012/06/19 20:01:34, markusheintz_ wrote: > indent by 1 Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1051: container->local_storage_info_list_.begin(); On 2012/06/19 20:01:34, markusheintz_ wrote: > Fix indentation. Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1099: container->session_storage_info_list_.begin(); On 2012/06/19 20:01:34, markusheintz_ wrote: > indent by 5 next two lines indet by 1 Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1130: ait != app_data_map_.end(); ++ait ) { On 2012/06/19 20:01:34, markusheintz_ wrote: > indent by one Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1148: container->indexed_db_info_list_.begin(); On 2012/06/19 20:01:34, markusheintz_ wrote: > Indent by 5. Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1149: indexed_db_info != container->indexed_db_info_list_.end(); On 2012/06/19 20:01:34, markusheintz_ wrote: > this and the next line indent by 1 Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1149: indexed_db_info != container->indexed_db_info_list_.end(); On 2012/06/19 20:01:34, markusheintz_ wrote: > this and the next line indent by 1 Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1178: ait != app_data_map_.end(); ++ait ) { On 2012/06/19 20:01:34, markusheintz_ wrote: > Indentation. Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1196: container->server_bound_cert_list_.begin(); On 2012/06/19 20:01:34, markusheintz_ wrote: > Please fix indentation. Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1232: ait != app_data_map_.end(); ++ait ) { On 2012/06/19 20:01:34, markusheintz_ wrote: > Please fix indentation here. Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1250: container->file_system_info_list_.begin(); On 2012/06/19 20:01:34, markusheintz_ wrote: > Please fix indentation Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1280: ait != app_data_map_.end(); ++ait ) { On 2012/06/19 20:01:34, markusheintz_ wrote: > Pls fix intendation. Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1298: container->quota_info_list_.begin(); On 2012/06/19 20:01:34, markusheintz_ wrote: > Please fix indentation here Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1371: void LocalDataContainer::Init(CookiesTreeModelDelegate* d) { On 2012/06/19 20:01:34, markusheintz_ wrote: > nit: s/d/delegate/ Done. https://chromiumcodereview.appspot.com/10536017/diff/5013/chrome/browser/cook... chrome/browser/cookies_tree_model.cc:1483: } On 2012/06/19 20:01:34, markusheintz_ wrote: > nit: Please add an empty line here. Done.
Including area owners jeremy@chromium.org for ui/cocoa jeremy@chromium.org for ui/gtk jhawkins@chromium.org for everything else
Wrong reviewer for ui/gtk in the previous email. Adding estade@chromium.org.
https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/coo... File chrome/browser/cookies_tree_model.h (right): https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/coo... chrome/browser/cookies_tree_model.h:797: // LocalDataContainer --------------------------------------------------------- please create a new file for this. https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/coo... chrome/browser/cookies_tree_model.h:841: string16 app_name_; aren't app names encoded in utf8? therefore this should be a std::string https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/coo... chrome/browser/cookies_tree_model.h:842: string16 app_id_; this should probably also be a std::string https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/ui/... File chrome/browser/ui/gtk/collected_cookies_gtk.cc (right): https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/ui/... chrome/browser/ui/gtk/collected_cookies_gtk.cc:307: string16 name = ASCIIToUTF16("Site Data"); shouldn't this be a localized string? https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/ui/... chrome/browser/ui/gtk/collected_cookies_gtk.cc:308: string16 browser_id; don't declare this, just inline string16()
Rubberstamp LGTM for ui/cocoa .
Thanks for the quick review Evan! I've responded to most of the comments and will bundle the rest of the changes with other review fixes in the next patch. https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/coo... File chrome/browser/cookies_tree_model.h (right): https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/coo... chrome/browser/cookies_tree_model.h:797: // LocalDataContainer --------------------------------------------------------- On 2012/06/19 23:43:50, Evan Stade wrote: > please create a new file for this. Considering that all classes related to the cookie tree model are all in the same file and this class is just a split of the existing CookiesTreeModel, I've kept it here. If you feel strongly, I will split it out, but it seems strange to separate only one of many classes out. https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/coo... chrome/browser/cookies_tree_model.h:841: string16 app_name_; On 2012/06/19 23:43:50, Evan Stade wrote: > aren't app names encoded in utf8? therefore this should be a std::string The CookieTreeNode uses string16 for its ctor and the title of the node. Since the app name is used as the title for the app node, I've chosen to have it as string16. https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/coo... chrome/browser/cookies_tree_model.h:842: string16 app_id_; On 2012/06/19 23:43:50, Evan Stade wrote: > this should probably also be a std::string Yes, this can be std::string, though for uniformity with the other parameters, I did it as string16. https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/ui/... File chrome/browser/ui/gtk/collected_cookies_gtk.cc (right): https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/ui/... chrome/browser/ui/gtk/collected_cookies_gtk.cc:307: string16 name = ASCIIToUTF16("Site Data"); On 2012/06/19 23:43:50, Evan Stade wrote: > shouldn't this be a localized string? In general yes, though a subsequent change will make this string not visible in the UI, so I opted not to make it localizable. https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/ui/... chrome/browser/ui/gtk/collected_cookies_gtk.cc:308: string16 browser_id; On 2012/06/19 23:43:50, Evan Stade wrote: > don't declare this, just inline string16() Done.
LGTM On 2012/06/20 15:49:37, nasko wrote: > Thanks for the quick review Evan! I've responded to most of the comments and > will bundle the rest of the changes with other review fixes in the next patch. > > https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/coo... > File chrome/browser/cookies_tree_model.h (right): > > https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/coo... > chrome/browser/cookies_tree_model.h:797: // LocalDataContainer > --------------------------------------------------------- > On 2012/06/19 23:43:50, Evan Stade wrote: > > please create a new file for this. > > Considering that all classes related to the cookie tree model are all in the > same file and this class is just a split of the existing CookiesTreeModel, I've > kept it here. If you feel strongly, I will split it out, but it seems strange to > separate only one of many classes out. > > https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/coo... > chrome/browser/cookies_tree_model.h:841: string16 app_name_; > On 2012/06/19 23:43:50, Evan Stade wrote: > > aren't app names encoded in utf8? therefore this should be a std::string > > The CookieTreeNode uses string16 for its ctor and the title of the node. Since > the app name is used as the title for the app node, I've chosen to have it as > string16. > > https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/coo... > chrome/browser/cookies_tree_model.h:842: string16 app_id_; > On 2012/06/19 23:43:50, Evan Stade wrote: > > this should probably also be a std::string > > Yes, this can be std::string, though for uniformity with the other parameters, I > did it as string16. > > https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/ui/... > File chrome/browser/ui/gtk/collected_cookies_gtk.cc (right): > > https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/ui/... > chrome/browser/ui/gtk/collected_cookies_gtk.cc:307: string16 name = > ASCIIToUTF16("Site Data"); > On 2012/06/19 23:43:50, Evan Stade wrote: > > shouldn't this be a localized string? > > In general yes, though a subsequent change will make this string not visible in > the UI, so I opted not to make it localizable. > > https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/ui/... > chrome/browser/ui/gtk/collected_cookies_gtk.cc:308: string16 browser_id; > On 2012/06/19 23:43:50, Evan Stade wrote: > > don't declare this, just inline string16() > > Done.
https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/coo... File chrome/browser/cookies_tree_model.h (right): https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/coo... chrome/browser/cookies_tree_model.h:797: // LocalDataContainer --------------------------------------------------------- On 2012/06/20 15:49:37, nasko wrote: > On 2012/06/19 23:43:50, Evan Stade wrote: > > please create a new file for this. > > Considering that all classes related to the cookie tree model are all in the > same file and this class is just a split of the existing CookiesTreeModel, I've > kept it here. If you feel strongly, I will split it out, but it seems strange to > separate only one of many classes out. feel free to separate all of them. https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/coo... chrome/browser/cookies_tree_model.h:841: string16 app_name_; On 2012/06/20 15:49:37, nasko wrote: > On 2012/06/19 23:43:50, Evan Stade wrote: > > aren't app names encoded in utf8? therefore this should be a std::string > > The CookieTreeNode uses string16 for its ctor and the title of the node. Since > the app name is used as the title for the app node, I've chosen to have it as > string16. convert as late as possible. https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/coo... chrome/browser/cookies_tree_model.h:842: string16 app_id_; On 2012/06/20 15:49:37, nasko wrote: > On 2012/06/19 23:43:50, Evan Stade wrote: > > this should probably also be a std::string > > Yes, this can be std::string, though for uniformity with the other parameters, I > did it as string16. I don't understand the uniformity argument. You wouldn't encode an integer as a string for uniformity's sake. https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/res... File chrome/browser/resources/options2/cookies_list.js (right): https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/res... chrome/browser/resources/options2/cookies_list.js:610: if ($('cookies-list').rootId) { no curlies https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/ui/... File chrome/browser/ui/gtk/collected_cookies_gtk.cc (right): https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/ui/... chrome/browser/ui/gtk/collected_cookies_gtk.cc:307: string16 name = ASCIIToUTF16("Site Data"); On 2012/06/20 15:49:37, nasko wrote: > On 2012/06/19 23:43:50, Evan Stade wrote: > > shouldn't this be a localized string? > > In general yes, though a subsequent change will make this string not visible in > the UI, so I opted not to make it localizable. if it won't be visible in the UI, there should be a TODO to change it to the empty string. https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/ui/... File chrome/browser/ui/webui/options2/cookies_view_handler2.cc (right): https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/ui/... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:169: string16 browser_id; this is still here https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/ui/... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:186: string16 app_id; why is this declared here
Including jhawkins@ for review. https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/coo... File chrome/browser/cookies_tree_model.h (right): https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/coo... chrome/browser/cookies_tree_model.h:797: // LocalDataContainer --------------------------------------------------------- On 2012/06/20 21:06:30, Evan Stade wrote: > On 2012/06/20 15:49:37, nasko wrote: > > On 2012/06/19 23:43:50, Evan Stade wrote: > > > please create a new file for this. > > > > Considering that all classes related to the cookie tree model are all in the > > same file and this class is just a split of the existing CookiesTreeModel, > I've > > kept it here. If you feel strongly, I will split it out, but it seems strange > to > > separate only one of many classes out. > > feel free to separate all of them. I've split the LocalDataContainer. https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/coo... chrome/browser/cookies_tree_model.h:841: string16 app_name_; On 2012/06/20 21:06:30, Evan Stade wrote: > On 2012/06/20 15:49:37, nasko wrote: > > On 2012/06/19 23:43:50, Evan Stade wrote: > > > aren't app names encoded in utf8? therefore this should be a std::string > > > > The CookieTreeNode uses string16 for its ctor and the title of the node. Since > > the app name is used as the title for the app node, I've chosen to have it as > > string16. > > convert as late as possible. Done. https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/coo... chrome/browser/cookies_tree_model.h:842: string16 app_id_; On 2012/06/20 21:06:30, Evan Stade wrote: > On 2012/06/20 15:49:37, nasko wrote: > > On 2012/06/19 23:43:50, Evan Stade wrote: > > > this should probably also be a std::string > > > > Yes, this can be std::string, though for uniformity with the other parameters, > I > > did it as string16. > > I don't understand the uniformity argument. You wouldn't encode an integer as a > string for uniformity's sake. Done. https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/res... File chrome/browser/resources/options2/cookies_list.js (right): https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/res... chrome/browser/resources/options2/cookies_list.js:610: if ($('cookies-list').rootId) { On 2012/06/20 21:06:30, Evan Stade wrote: > no curlies Done. https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/ui/... File chrome/browser/ui/gtk/collected_cookies_gtk.cc (right): https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/ui/... chrome/browser/ui/gtk/collected_cookies_gtk.cc:307: string16 name = ASCIIToUTF16("Site Data"); On 2012/06/20 21:06:30, Evan Stade wrote: > On 2012/06/20 15:49:37, nasko wrote: > > On 2012/06/19 23:43:50, Evan Stade wrote: > > > shouldn't this be a localized string? > > > > In general yes, though a subsequent change will make this string not visible > in > > the UI, so I opted not to make it localizable. > > if it won't be visible in the UI, there should be a TODO to change it to the > empty string. Done. https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/ui/... File chrome/browser/ui/webui/options2/cookies_view_handler2.cc (right): https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/ui/... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:169: string16 browser_id; On 2012/06/20 21:06:30, Evan Stade wrote: > this is still here Done. https://chromiumcodereview.appspot.com/10536017/diff/31001/chrome/browser/ui/... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:186: string16 app_id; On 2012/06/20 21:06:30, Evan Stade wrote: > why is this declared here The reason is that in the loop over all extensions, we will actually have valid id that is non-empty. It is a variable to hold that id and pass it to the LocalDataContainer constructor. It will disappear as I migrate the app_id and app_name to std::strings.
https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... File chrome/browser/cookies_tree_model.cc (right): https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... chrome/browser/cookies_tree_model.cc:33: LocalDataContainer* GetLocalDataContainerForNode(const CookieTreeNode* node) { nit: Document function. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... chrome/browser/cookies_tree_model.cc:34: DCHECK(node); nit: Internal methods should not DCHECK parameters for non-NULLness. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... chrome/browser/cookies_tree_model.cc:40: } nit: } // namespace https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... chrome/browser/cookies_tree_model.cc:52: if (parent()) { Optional nit: Save a level of indentation by reversing the logic and returning early. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... chrome/browser/cookies_tree_model.cc:870: for (ContainerMap::iterator ait = app_data_map_.begin(); nit: |ait| is not a good variable name. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... chrome/browser/cookies_tree_model.cc:891: if (filter.empty() || Optional nit: Save indentation by reversing the logic and continuing. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... chrome/browser/cookies_tree_model.cc:916: // mmargh mmargh mmargh! delicious! nit: Remove comment? https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... File chrome/browser/cookies_tree_model.h (right): https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... chrome/browser/cookies_tree_model.h:219: class NodeTitleComparator { nit: Document class. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... chrome/browser/cookies_tree_model.h:224: class AppNodeComparator { nit: Document class. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... chrome/browser/cookies_tree_model.h:243: CookieTreeAppNode* GetOrCreateAppNode(const std::string& app_name, nit: Document method. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... chrome/browser/cookies_tree_model.h:257: class CookieTreeAppNode : public CookieTreeNode { Why not move the definition of this class above its use so you don't have to forward declare? https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... chrome/browser/cookies_tree_model.h:667: const string16& filter) {} Why not make the interface abstract? https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... File chrome/browser/cookies_tree_model_unittest.cc (right): https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... chrome/browser/cookies_tree_model_unittest.cc:77: new MockBrowsingDataCookieHelper(profile_->GetRequestContext()); nit: Indentation is off by two. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... chrome/browser/cookies_tree_model_unittest.cc:119: NULL, Optional nit: Condense parameters to save lines. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/loc... File chrome/browser/local_data_container.h (right): https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/loc... chrome/browser/local_data_container.h:43: } nit: } // namespace https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/loc... chrome/browser/local_data_container.h:102: AppCacheInfoMap appcache_info_; nit: This class is sorely lacking comments. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/loc... chrome/browser/local_data_container.h:118: friend class CookiesTreeModel; nit: friends go at the top of the access section. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/ui/... File chrome/browser/ui/webui/options2/cookies_view_handler2.cc (right): https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/ui/... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:170: apps_map[""] = new LocalDataContainer( s/""/string16/ (or std::string() where appropriate) everywhere. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/ui/... File chrome/browser/ui/webui/options2/cookies_view_handler2.h (right): https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/ui/... chrome/browser/ui/webui/options2/cookies_view_handler2.h:12: #include "chrome/browser/local_data_container.h" Unused?
https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/loc... File chrome/browser/local_data_container.h (right): https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/loc... chrome/browser/local_data_container.h:28: typedef std::map<std::string, LocalDataContainer*> ContainerMap; don't indent inside namespaces. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/res... File chrome/browser/resources/options2/cookies_list.js (right): https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/res... chrome/browser/resources/options2/cookies_list.js:837: if (parentId) { I think parentId has to be truthy at this point, because if it were falsey then |parent| would already equal |this|. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/res... chrome/browser/resources/options2/cookies_list.js:865: if (parentId) { ditto https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/res... File chrome/browser/resources/options2/cookies_view.js (right): https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/res... chrome/browser/resources/options2/cookies_view.js:131: if (args[1][i]) { combine these two if conditions into one with && https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/res... chrome/browser/resources/options2/cookies_view.js:132: if (args[1][i].appId == '') { === https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/ui/... File chrome/browser/ui/webui/options2/cookies_view_handler2.cc (right): https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/ui/... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:225: cookies_tree_model_->DeleteStoredObjectsForApp(""); nit: pass std::string()
Including ben@chromium.org, as ui/views is not covered yet. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... File chrome/browser/cookies_tree_model.cc (right): https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... chrome/browser/cookies_tree_model.cc:33: LocalDataContainer* GetLocalDataContainerForNode(const CookieTreeNode* node) { On 2012/06/21 00:04:12, James Hawkins wrote: > nit: Document function. Done. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... chrome/browser/cookies_tree_model.cc:34: DCHECK(node); On 2012/06/21 00:04:12, James Hawkins wrote: > nit: Internal methods should not DCHECK parameters for non-NULLness. Done. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... chrome/browser/cookies_tree_model.cc:40: } On 2012/06/21 00:04:12, James Hawkins wrote: > nit: } // namespace Done. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... chrome/browser/cookies_tree_model.cc:52: if (parent()) { On 2012/06/21 00:04:12, James Hawkins wrote: > Optional nit: Save a level of indentation by reversing the logic and returning > early. Done. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... chrome/browser/cookies_tree_model.cc:870: for (ContainerMap::iterator ait = app_data_map_.begin(); On 2012/06/21 00:04:12, James Hawkins wrote: > nit: |ait| is not a good variable name. Done. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... chrome/browser/cookies_tree_model.cc:891: if (filter.empty() || On 2012/06/21 00:04:12, James Hawkins wrote: > Optional nit: Save indentation by reversing the logic and continuing. I will leave it as is, since Markus has planned code cleanup/refactor of this area of the code. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... chrome/browser/cookies_tree_model.cc:916: // mmargh mmargh mmargh! delicious! On 2012/06/21 00:04:12, James Hawkins wrote: > nit: Remove comment? Done. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... File chrome/browser/cookies_tree_model.h (right): https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... chrome/browser/cookies_tree_model.h:219: class NodeTitleComparator { On 2012/06/21 00:04:12, James Hawkins wrote: > nit: Document class. Done. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... chrome/browser/cookies_tree_model.h:224: class AppNodeComparator { On 2012/06/21 00:04:12, James Hawkins wrote: > nit: Document class. Done. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... chrome/browser/cookies_tree_model.h:243: CookieTreeAppNode* GetOrCreateAppNode(const std::string& app_name, On 2012/06/21 00:04:12, James Hawkins wrote: > nit: Document method. Done. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... chrome/browser/cookies_tree_model.h:257: class CookieTreeAppNode : public CookieTreeNode { On 2012/06/21 00:04:12, James Hawkins wrote: > Why not move the definition of this class above its use so you don't have to > forward declare? Done. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... chrome/browser/cookies_tree_model.h:667: const string16& filter) {} On 2012/06/21 00:04:12, James Hawkins wrote: > Why not make the interface abstract? Done. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... File chrome/browser/cookies_tree_model_unittest.cc (right): https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... chrome/browser/cookies_tree_model_unittest.cc:77: new MockBrowsingDataCookieHelper(profile_->GetRequestContext()); On 2012/06/21 00:04:12, James Hawkins wrote: > nit: Indentation is off by two. Done. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/coo... chrome/browser/cookies_tree_model_unittest.cc:119: NULL, On 2012/06/21 00:04:12, James Hawkins wrote: > Optional nit: Condense parameters to save lines. Done. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/loc... File chrome/browser/local_data_container.h (right): https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/loc... chrome/browser/local_data_container.h:28: typedef std::map<std::string, LocalDataContainer*> ContainerMap; On 2012/06/21 04:01:41, Evan Stade wrote: > don't indent inside namespaces. Done. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/loc... chrome/browser/local_data_container.h:43: } On 2012/06/21 00:04:12, James Hawkins wrote: > nit: } // namespace Done. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/loc... chrome/browser/local_data_container.h:102: AppCacheInfoMap appcache_info_; On 2012/06/21 00:04:12, James Hawkins wrote: > nit: This class is sorely lacking comments. Done. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/loc... chrome/browser/local_data_container.h:118: friend class CookiesTreeModel; On 2012/06/21 00:04:12, James Hawkins wrote: > nit: friends go at the top of the access section. Done. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/res... File chrome/browser/resources/options2/cookies_list.js (right): https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/res... chrome/browser/resources/options2/cookies_list.js:837: if (parentId) { On 2012/06/21 04:01:41, Evan Stade wrote: > I think parentId has to be truthy at this point, because if it were falsey then > |parent| would already equal |this|. Since I've added a new level in the tree, it no longer starts at the root of the tree, rather at the app level. In this case, the parent ID is valid (not the root), but also we haven't inserted the parent in the parentLookup. There will be a follow up CL to display app stored data, so this is one step of a bigger change. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/res... chrome/browser/resources/options2/cookies_list.js:865: if (parentId) { On 2012/06/21 04:01:41, Evan Stade wrote: > ditto See the above. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/res... File chrome/browser/resources/options2/cookies_view.js (right): https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/res... chrome/browser/resources/options2/cookies_view.js:131: if (args[1][i]) { On 2012/06/21 04:01:41, Evan Stade wrote: > combine these two if conditions into one with && Done. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/res... chrome/browser/resources/options2/cookies_view.js:132: if (args[1][i].appId == '') { On 2012/06/21 04:01:41, Evan Stade wrote: > === Done. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/ui/... File chrome/browser/ui/webui/options2/cookies_view_handler2.cc (right): https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/ui/... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:170: apps_map[""] = new LocalDataContainer( On 2012/06/21 00:04:12, James Hawkins wrote: > s/""/string16/ (or std::string() where appropriate) everywhere. Done. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/ui/... chrome/browser/ui/webui/options2/cookies_view_handler2.cc:225: cookies_tree_model_->DeleteStoredObjectsForApp(""); On 2012/06/21 04:01:41, Evan Stade wrote: > nit: pass std::string() Done. https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/ui/... File chrome/browser/ui/webui/options2/cookies_view_handler2.h (right): https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/ui/... chrome/browser/ui/webui/options2/cookies_view_handler2.h:12: #include "chrome/browser/local_data_container.h" On 2012/06/21 00:04:12, James Hawkins wrote: > Unused? Done.
https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/res... File chrome/browser/resources/options2/cookies_list.js (right): https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/res... chrome/browser/resources/options2/cookies_list.js:837: if (parentId) { On 2012/06/21 16:22:12, nasko wrote: > On 2012/06/21 04:01:41, Evan Stade wrote: > > I think parentId has to be truthy at this point, because if it were falsey > then > > |parent| would already equal |this|. > > Since I've added a new level in the tree, it no longer starts at the root of the > tree, rather at the app level. In this case, the parent ID is valid (not the > root), but also we haven't inserted the parent in the parentLookup. if the parentId is valid then why are you checking it?
https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/res... File chrome/browser/resources/options2/cookies_list.js (right): https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/res... chrome/browser/resources/options2/cookies_list.js:837: if (parentId) { On 2012/06/21 18:40:02, Evan Stade wrote: > On 2012/06/21 16:22:12, nasko wrote: > > On 2012/06/21 04:01:41, Evan Stade wrote: > > > I think parentId has to be truthy at this point, because if it were falsey > > then > > > |parent| would already equal |this|. > > > > Since I've added a new level in the tree, it no longer starts at the root of > the > > tree, rather at the app level. In this case, the parent ID is valid (not the > > root), but also we haven't inserted the parent in the parentLookup. > > if the parentId is valid then why are you checking it? Because, while there is a parent node (the root node), it is not in the lookup table. So in the case where we have valid parentId (the app node has parent of root), but it is not in the lookup table, we want to use the cookie view as the parent to add items to.
ui/views lgtm
bear with me here... https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/res... File chrome/browser/resources/options2/cookies_list.js (right): https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/res... chrome/browser/resources/options2/cookies_list.js:837: if (parentId) { On 2012/06/21 18:49:47, nasko wrote: > On 2012/06/21 18:40:02, Evan Stade wrote: > > On 2012/06/21 16:22:12, nasko wrote: > > > On 2012/06/21 04:01:41, Evan Stade wrote: > > > > I think parentId has to be truthy at this point, because if it were falsey > > > then > > > > |parent| would already equal |this|. > > > > > > Since I've added a new level in the tree, it no longer starts at the root of > > the > > > tree, rather at the app level. In this case, the parent ID is valid (not the > > > root), but also we haven't inserted the parent in the parentLookup. > > > > if the parentId is valid then why are you checking it? > > Because, while there is a parent node (the root node), it is not in the lookup > table. So in the case where we have valid parentId (the app node has parent of > root), but it is not in the lookup table, we want to use the cookie view as the > parent to add items to. I'm not really following you any more. And I don't think you're following me. Explain to me how the code you have here is any different from: var parent = parentLookup[parentId] || this; https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/res... chrome/browser/resources/options2/cookies_list.js:840: return; my point is that this line cannot be reached. It is a logical impossibility.
Yes, I understood what you mean now and I agree. I will update the code later, since there is a chance this CL will be abandoned :( https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/res... File chrome/browser/resources/options2/cookies_list.js (right): https://chromiumcodereview.appspot.com/10536017/diff/36001/chrome/browser/res... chrome/browser/resources/options2/cookies_list.js:837: if (parentId) { On 2012/06/22 00:59:47, Evan Stade wrote: > On 2012/06/21 18:49:47, nasko wrote: > > On 2012/06/21 18:40:02, Evan Stade wrote: > > > On 2012/06/21 16:22:12, nasko wrote: > > > > On 2012/06/21 04:01:41, Evan Stade wrote: > > > > > I think parentId has to be truthy at this point, because if it were > falsey > > > > then > > > > > |parent| would already equal |this|. > > > > > > > > Since I've added a new level in the tree, it no longer starts at the root > of > > > the > > > > tree, rather at the app level. In this case, the parent ID is valid (not > the > > > > root), but also we haven't inserted the parent in the parentLookup. > > > > > > if the parentId is valid then why are you checking it? > > > > Because, while there is a parent node (the root node), it is not in the lookup > > table. So in the case where we have valid parentId (the app node has parent of > > root), but it is not in the lookup table, we want to use the cookie view as > the > > parent to add items to. > > I'm not really following you any more. And I don't think you're following me. > Explain to me how the code you have here is any different from: > > var parent = parentLookup[parentId] || this; I see what your point is. You are indeed correct. |
