Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9567035:
Added MediaGalleryDatabase (Closed)
|
Created:
8 years, 9 months ago by tpayne Modified:
8 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionThe new media gallery API needs to cache metadata about media files. Adds the first part of database support for this.
BUG=110823
TEST=NONE
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=128219
Patch Set 1 #Patch Set 2 : Minor cleanups #
Total comments: 41
Patch Set 3 : Addressed Comments #Patch Set 4 : Addressed comment #Patch Set 5 : Added missing files #Patch Set 6 : Fix compile error #Patch Set 7 : Fixing FilePath instances for Windows #Patch Set 8 : Fix windows filepaths #Patch Set 9 : Standardize on UTF8 for path storage #
Total comments: 2
Patch Set 10 : Add missing include for OS_WIN #Patch Set 11 : Fix windows compile #
Total comments: 43
Patch Set 12 : Address comments #
Total comments: 8
Patch Set 13 : Address comments #Patch Set 14 : Merge #
Total comments: 14
Patch Set 15 : Addressed comments #Patch Set 16 : Renamed types file #Patch Set 17 : Renamed types file #Patch Set 18 : git try win_rel #Patch Set 19 : #
Total comments: 2
Patch Set 20 : Moved to content namespace, fix some windows compile issues #Patch Set 21 : Moved media_gallery code to chrome #Messages
Total messages: 35 (0 generated)
Media Gallery Database. First pass just enables creating the collection table, adding and retrieving a row. http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... File content/browser/media_gallery/media_gallery_database.h (right): http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database.h:32: class CollectionRow { I don't know if this class is simple enough to make a nested class. Should I pull out a media_gallery_types.[h,cc]?
Some initial thoughts. Lei is going to pick up the first round of review from here. http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... File content/browser/media_gallery/media_gallery_database.cc (right): http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database.cc:19: #define COLLECTION_TABLE_NAME "collections" You've hard coded the column creation, what's the idea behind having the table name as a constant? http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database.cc:21: std::ostream& operator<<(std::ostream& out, In anonymous namespace? http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database.cc:38: static const char kMediaGalleryDatabaseName[] = "media_gallery.db"; nit: we might want to use a "hidden" name, i.e. start with a dot. http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database.cc:65: // 6000 * 4MB = 24MB are you calling 4MB the page size here? set_page_size probably takes page size in bytes. http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database.cc:110: ++cur_version; set it to kCurrentVersionNumber. http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database.cc:119: int MediaGalleryDatabase::GetCurrentVersion() { Why a method to return a constant? http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... File content/browser/media_gallery/media_gallery_database.h (right): http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database.h:18: typedef int COLLECTIONID; Types should be initial cap, camel case. http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database.h:32: class CollectionRow { On 2012/03/02 01:05:00, tpayne wrote: > I don't know if this class is simple enough to make a nested class. Should I > pull out a media_gallery_types.[h,cc]? Hard to say at this point, at some level it's a matter of preference. http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database.h:32: class CollectionRow { You may want to make this a struct though, depending if you plan to add any additional functionality: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Struct... http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database.h:34: CollectionRow() { } Classes with non-trivial fields should not have inline constructors or destructors. (put the implementation in the .cc file). http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database.h:43: virtual ~CollectionRow() { } And the destructor. http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database.h:44: int GetId() const { return id_; } Getter/Setters should use unix_hacker_style naming and don't use get. So GetId() -> id()
http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... File content/browser/media_gallery/media_gallery_database.h (right): http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database.h:51: bool operator==(const CollectionRow& row2) const { Please try to keep implementation out of headers. http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database.h:66: }; missed a DISALLOW_COPY_AND_ASSIGN here. http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... File content/browser/media_gallery/media_gallery_database_unittest.cc (right): http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database_unittest.cc:6: #include "base/file_util.h" Not used, remove. Same for path_service.h and string_util.h. http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database_unittest.cc:32: EXPECT_TRUE(db_.Open(db_file)); I would change these to be ASSERT_FOO. If the set up step failed, there's no point in continuing on to the test. http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database_unittest.cc:47: EXPECT_TRUE(GetDB().DoesTableExist("collections")); Maybe this should be in the header file so you can use it in the implementation and test. http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database_unittest.cc:56: ASSERT_TRUE(rowid); These can probably be EXPECT_FOO. http://codereview.chromium.org/9567035/diff/1001/content/content_browser.gypi File content/content_browser.gypi (right): http://codereview.chromium.org/9567035/diff/1001/content/content_browser.gypi... content/content_browser.gypi:379: 'browser/media_gallery/media_gallery_database.h', nit: alphabetical order
http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... File content/browser/media_gallery/media_gallery_database.cc (right): http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database.cc:19: #define COLLECTION_TABLE_NAME "collections" On 2012/03/05 23:16:03, vandebo wrote: > You've hard coded the column creation, what's the idea behind having the table > name as a constant? Done. http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database.cc:21: std::ostream& operator<<(std::ostream& out, On 2012/03/05 23:16:03, vandebo wrote: > In anonymous namespace? Done. http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database.cc:38: static const char kMediaGalleryDatabaseName[] = "media_gallery.db"; On 2012/03/05 23:16:03, vandebo wrote: > nit: we might want to use a "hidden" name, i.e. start with a dot. Done. http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database.cc:65: // 6000 * 4MB = 24MB On 2012/03/05 23:16:03, vandebo wrote: > are you calling 4MB the page size here? set_page_size probably takes page size > in bytes. This is copied verbatim from url_database. Looks like they've got a typo. I'll fix this version. http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database.cc:110: ++cur_version; On 2012/03/05 23:16:03, vandebo wrote: > set it to kCurrentVersionNumber. Once we have migration code, that's not going to be a good idea. The idea is that we update it as we do each step of the migration. If the migration fails partway through, we want the current version to match the current state of the db. http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database.cc:119: int MediaGalleryDatabase::GetCurrentVersion() { On 2012/03/05 23:16:03, vandebo wrote: > Why a method to return a constant? I don't know. I cribbed this from url_database.cc. It may not be necessary in our case. http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... File content/browser/media_gallery/media_gallery_database.h (right): http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database.h:18: typedef int COLLECTIONID; On 2012/03/05 23:16:03, vandebo wrote: > Types should be initial cap, camel case. Done. http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database.h:32: class CollectionRow { On 2012/03/05 23:16:03, vandebo wrote: > You may want to make this a struct though, depending if you plan to add any > additional functionality: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Struct... Done. http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database.h:32: class CollectionRow { On 2012/03/05 23:16:03, vandebo wrote: > On 2012/03/02 01:05:00, tpayne wrote: > > I don't know if this class is simple enough to make a nested class. Should I > > pull out a media_gallery_types.[h,cc]? > > Hard to say at this point, at some level it's a matter of preference. Since I need to pull out part of the implementation, it makes sense to have a separate file, I think. http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database.h:34: CollectionRow() { } On 2012/03/05 23:16:03, vandebo wrote: > Classes with non-trivial fields should not have inline constructors or > destructors. (put the implementation in the .cc file). Done. http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database.h:43: virtual ~CollectionRow() { } On 2012/03/05 23:16:03, vandebo wrote: > And the destructor. Done. http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database.h:44: int GetId() const { return id_; } On 2012/03/05 23:16:03, vandebo wrote: > Getter/Setters should use unix_hacker_style naming and don't use get. So GetId() > -> id() N/A http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database.h:51: bool operator==(const CollectionRow& row2) const { On 2012/03/05 23:38:36, Lei Zhang wrote: > Please try to keep implementation out of headers. Done. http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database.h:66: }; On 2012/03/05 23:38:36, Lei Zhang wrote: > missed a DISALLOW_COPY_AND_ASSIGN here. Done. http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... File content/browser/media_gallery/media_gallery_database_unittest.cc (right): http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database_unittest.cc:6: #include "base/file_util.h" On 2012/03/05 23:38:36, Lei Zhang wrote: > Not used, remove. Same for path_service.h and string_util.h. Done. http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database_unittest.cc:32: EXPECT_TRUE(db_.Open(db_file)); On 2012/03/05 23:38:36, Lei Zhang wrote: > I would change these to be ASSERT_FOO. If the set up step failed, there's no > point in continuing on to the test. Done. http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database_unittest.cc:47: EXPECT_TRUE(GetDB().DoesTableExist("collections")); On 2012/03/05 23:38:36, Lei Zhang wrote: > Maybe this should be in the header file so you can use it in the implementation > and test. Why would the implementation want this? http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database_unittest.cc:56: ASSERT_TRUE(rowid); On 2012/03/05 23:38:36, Lei Zhang wrote: > These can probably be EXPECT_FOO. Done. http://codereview.chromium.org/9567035/diff/1001/content/content_browser.gypi File content/content_browser.gypi (right): http://codereview.chromium.org/9567035/diff/1001/content/content_browser.gypi... content/content_browser.gypi:379: 'browser/media_gallery/media_gallery_database.h', On 2012/03/05 23:38:36, Lei Zhang wrote: > nit: alphabetical order Done.
http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... File content/browser/media_gallery/media_gallery_database_unittest.cc (right): http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database_unittest.cc:47: EXPECT_TRUE(GetDB().DoesTableExist("collections")); On 2012/03/06 01:52:37, tpayne wrote: > On 2012/03/05 23:38:36, Lei Zhang wrote: > > Maybe this should be in the header file so you can use it in the > implementation > > and test. > > Why would the implementation want this? It's used on content/browser/media_gallery/media_gallery_database.cc:157.
http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... File content/browser/media_gallery/media_gallery_database_unittest.cc (right): http://codereview.chromium.org/9567035/diff/1001/content/browser/media_galler... content/browser/media_gallery/media_gallery_database_unittest.cc:47: EXPECT_TRUE(GetDB().DoesTableExist("collections")); On 2012/03/06 01:57:46, Lei Zhang wrote: > On 2012/03/06 01:52:37, tpayne wrote: > > On 2012/03/05 23:38:36, Lei Zhang wrote: > > > Maybe this should be in the header file so you can use it in the > > implementation > > > and test. > > > > Why would the implementation want this? > > It's used on content/browser/media_gallery/media_gallery_database.cc:157. Done. I don't think it helps much, though.
http://codereview.chromium.org/9567035/diff/13001/content/browser/media_galle... File content/browser/media_gallery/media_gallery_database.cc (right): http://codereview.chromium.org/9567035/diff/13001/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:122: #if defined(OS_WIN) Steve, If we want the DB to be portable, we need to make sure the paths we're storing are platform independent. Step 1 is making sure we have a standard encoding (I chose UTF8 since that's what we assume POSIX uses). But, I don't think this will be sufficient to ensure cross-platform compatibility. What are you planning to use path for? Thanks to removable drives, path cannot be unique, so I'm not sure how useful it is.
http://codereview.chromium.org/9567035/diff/13001/content/browser/media_galle... File content/browser/media_gallery/media_gallery_database.cc (right): http://codereview.chromium.org/9567035/diff/13001/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:122: #if defined(OS_WIN) On 2012/03/06 22:56:00, tpayne wrote: > Steve, > > If we want the DB to be portable, we need to make sure the paths we're storing > are platform independent. Step 1 is making sure we have a standard encoding (I > chose UTF8 since that's what we assume POSIX uses). But, I don't think this will > be sufficient to ensure cross-platform compatibility. > > What are you planning to use path for? Thanks to removable drives, path cannot > be unique, so I'm not sure how useful it is. As discussed, path should be the relative path from the root of the gallery. We should convert to UTF8 as well as ensure path components are all '/'
http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... File content/browser/media_gallery/media_gallery_database.cc (right): http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:11: #if defined(OS_WIN) Put this below, even though the linter will complain. http://dev.chromium.org/developers/coding-style#TOC-Platform-defines http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:26: static const int kCurrentVersionNumber = 1; You don't need static inside an anonymous namespace. http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:28: static const FilePath::CharType kMediaGalleryDatabaseName[] = This will create a static initializers. Declare const char kFoo[] = "foo" here, and use FILE_PATH_LITERAL in the code below. http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:79: if (!CreateCollectionsTable(db)) { nit: no need for parenthesis for a one line conditional block. http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:96: // We can't read databases newer than we were designed for. nit: indentation. http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:98: LOG(WARNING) << "Media Gallery database is too new."; VLOG? http://www.chromium.org/developers/coding-style#TOC-Logging http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:158: void MediaGalleryDatabase::FillCollectionRow(const sql::Statement& s, nit: can we not use "s" and "i" as parameter names? They are hard to search for. http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:160: DCHECK(i); No need. If |i| is NULL, we'll immediately crash on the next line. http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... File content/browser/media_gallery/media_gallery_database.h (right): http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.h:28: // Returns the current version that we will generate history databases with. history -> media gallery. http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.h:31: // Sets the id_ field of the input collection_row to the generated unique id_ -> id http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.h:32: // key value and returns the same. On failure, returns zero. What does it return on success? http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.h:47: sql::Connection db_; Methods before data members. http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... File content/browser/media_gallery/media_gallery_database_unittest.cc (right): http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database_unittest.cc:5: #include "base/file_util.h" nit: you only need file_path.h. http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database_unittest.cc:54: EXPECT_TRUE(rowid); Do you need this when you have the test on the next line? http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database_unittest.cc:59: EXPECT_TRUE(row1 == row2); EXPECT_EQ(row1, row2); http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... File content/browser/media_gallery/media_gallery_types.cc (right): http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_types.cc:9: CollectionRow::CollectionRow() { } Do we need to initialize the POD struct members here? http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_types.cc:23: return id == row2.id http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Boolea... http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... File content/browser/media_gallery/media_gallery_types.h (right): http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_types.h:23: virtual ~CollectionRow(); This doesn't need to be virtual. http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_types.h:34: }; // class CollectionRow nit: no need for the comment. http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_types.h:37: nit: extra blank line.
http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... File content/browser/media_gallery/media_gallery_database.cc (right): http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:11: #if defined(OS_WIN) On 2012/03/07 01:41:42, Lei Zhang wrote: > Put this below, even though the linter will complain. > > http://dev.chromium.org/developers/coding-style#TOC-Platform-defines Done. http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:26: static const int kCurrentVersionNumber = 1; On 2012/03/07 01:41:42, Lei Zhang wrote: > You don't need static inside an anonymous namespace. Done. http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:28: static const FilePath::CharType kMediaGalleryDatabaseName[] = On 2012/03/07 01:41:42, Lei Zhang wrote: > This will create a static initializers. Declare const char kFoo[] = "foo" here, > and use FILE_PATH_LITERAL in the code below. I don't understand how it will create a static initializer. The typedef essentially resolves to what you've suggested. http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:79: if (!CreateCollectionsTable(db)) { On 2012/03/07 01:41:42, Lei Zhang wrote: > nit: no need for parenthesis for a one line conditional block. Done. http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:96: // We can't read databases newer than we were designed for. On 2012/03/07 01:41:42, Lei Zhang wrote: > nit: indentation. Done. http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:98: LOG(WARNING) << "Media Gallery database is too new."; On 2012/03/07 01:41:42, Lei Zhang wrote: > VLOG? http://www.chromium.org/developers/coding-style#TOC-Logging Done. http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:158: void MediaGalleryDatabase::FillCollectionRow(const sql::Statement& s, On 2012/03/07 01:41:42, Lei Zhang wrote: > nit: can we not use "s" and "i" as parameter names? They are hard to search for. Done. http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:160: DCHECK(i); On 2012/03/07 01:41:42, Lei Zhang wrote: > No need. If |i| is NULL, we'll immediately crash on the next line. Done. http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... File content/browser/media_gallery/media_gallery_database.h (right): http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.h:28: // Returns the current version that we will generate history databases with. On 2012/03/07 01:41:42, Lei Zhang wrote: > history -> media gallery. Done. http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.h:31: // Sets the id_ field of the input collection_row to the generated unique On 2012/03/07 01:41:42, Lei Zhang wrote: > id_ -> id Done. http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.h:32: // key value and returns the same. On failure, returns zero. On 2012/03/07 01:41:42, Lei Zhang wrote: > What does it return on success? The new value of row->id. See "and returns the same" http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.h:47: sql::Connection db_; On 2012/03/07 01:41:42, Lei Zhang wrote: > Methods before data members. Done. http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... File content/browser/media_gallery/media_gallery_database_unittest.cc (right): http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database_unittest.cc:5: #include "base/file_util.h" On 2012/03/07 01:41:42, Lei Zhang wrote: > nit: you only need file_path.h. Done. http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database_unittest.cc:54: EXPECT_TRUE(rowid); On 2012/03/07 01:41:42, Lei Zhang wrote: > Do you need this when you have the test on the next line? Done. http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database_unittest.cc:59: EXPECT_TRUE(row1 == row2); On 2012/03/07 01:41:42, Lei Zhang wrote: > EXPECT_EQ(row1, row2); This requires defining operator<< for CollectionRow http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... File content/browser/media_gallery/media_gallery_types.cc (right): http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_types.cc:9: CollectionRow::CollectionRow() { } On 2012/03/07 01:41:42, Lei Zhang wrote: > Do we need to initialize the POD struct members here? Done. http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_types.cc:23: return id == row2.id On 2012/03/07 01:41:42, Lei Zhang wrote: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Boolea... Done. http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... File content/browser/media_gallery/media_gallery_types.h (right): http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_types.h:23: virtual ~CollectionRow(); On 2012/03/07 01:41:42, Lei Zhang wrote: > This doesn't need to be virtual. Done. http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_types.h:34: }; // class CollectionRow On 2012/03/07 01:41:42, Lei Zhang wrote: > nit: no need for the comment. Done. http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_types.h:37: On 2012/03/07 01:41:42, Lei Zhang wrote: > nit: extra blank line. Done.
You need to update a new patch set. http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... File content/browser/media_gallery/media_gallery_database.cc (right): http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:28: static const FilePath::CharType kMediaGalleryDatabaseName[] = On 2012/03/07 18:04:03, tpayne wrote: > On 2012/03/07 01:41:42, Lei Zhang wrote: > > This will create a static initializers. Declare const char kFoo[] = "foo" > here, > > and use FILE_PATH_LITERAL in the code below. > > I don't understand how it will create a static initializer. The typedef > essentially resolves to what you've suggested. Whoops, I misread that as FilePath::StringType somehow. Ignore.
On 2012/03/07 20:59:08, Lei Zhang wrote: > You need to update a new patch set. done
http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... File content/browser/media_gallery/media_gallery_database_unittest.cc (right): http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database_unittest.cc:54: EXPECT_TRUE(rowid); On 2012/03/07 18:04:03, tpayne wrote: > On 2012/03/07 01:41:42, Lei Zhang wrote: > > Do you need this when you have the test on the next line? > > Done. You didn't do anything here. http://codereview.chromium.org/9567035/diff/23001/content/browser/media_galle... File content/browser/media_gallery/media_gallery_database.cc (right): http://codereview.chromium.org/9567035/diff/23001/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:136: base::ReplaceChars(path, "\\", "/", &path); IWYU - #include "base/string_util.h" http://codereview.chromium.org/9567035/diff/23001/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:194: const char *sql = "CREATE TABLE collections" nit: "char *" -> "char*" http://codereview.chromium.org/9567035/diff/23001/content/browser/media_galle... File content/browser/media_gallery/media_gallery_database_unittest.cc (right): http://codereview.chromium.org/9567035/diff/23001/content/browser/media_galle... content/browser/media_gallery/media_gallery_database_unittest.cc:65: #if defined(FILE_PATH_USES_WIN_SEPARATORS) You can also write this as: FilePath foo(FILE_PATH_LITERAL("path1")); foo = foo.AppendASCII("path2"); http://codereview.chromium.org/9567035/diff/23001/content/browser/media_galle... File content/browser/media_gallery/media_gallery_types.cc (right): http://codereview.chromium.org/9567035/diff/23001/content/browser/media_galle... content/browser/media_gallery/media_gallery_types.cc:10: : entry_count(0), I think you want to init |id| to 0 in both ctors as well?
http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... File content/browser/media_gallery/media_gallery_database_unittest.cc (right): http://codereview.chromium.org/9567035/diff/16002/content/browser/media_galle... content/browser/media_gallery/media_gallery_database_unittest.cc:54: EXPECT_TRUE(rowid); On 2012/03/07 21:24:37, Lei Zhang wrote: > On 2012/03/07 18:04:03, tpayne wrote: > > On 2012/03/07 01:41:42, Lei Zhang wrote: > > > Do you need this when you have the test on the next line? > > > > Done. > > You didn't do anything here. Done. http://codereview.chromium.org/9567035/diff/23001/content/browser/media_galle... File content/browser/media_gallery/media_gallery_database.cc (right): http://codereview.chromium.org/9567035/diff/23001/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:136: base::ReplaceChars(path, "\\", "/", &path); On 2012/03/07 21:24:37, Lei Zhang wrote: > IWYU - #include "base/string_util.h" Done. http://codereview.chromium.org/9567035/diff/23001/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:194: const char *sql = "CREATE TABLE collections" On 2012/03/07 21:24:37, Lei Zhang wrote: > nit: "char *" -> "char*" Done. http://codereview.chromium.org/9567035/diff/23001/content/browser/media_galle... File content/browser/media_gallery/media_gallery_database_unittest.cc (right): http://codereview.chromium.org/9567035/diff/23001/content/browser/media_galle... content/browser/media_gallery/media_gallery_database_unittest.cc:65: #if defined(FILE_PATH_USES_WIN_SEPARATORS) On 2012/03/07 21:24:37, Lei Zhang wrote: > You can also write this as: > > FilePath foo(FILE_PATH_LITERAL("path1")); > foo = foo.AppendASCII("path2"); Done. http://codereview.chromium.org/9567035/diff/23001/content/browser/media_galle... File content/browser/media_gallery/media_gallery_types.cc (right): http://codereview.chromium.org/9567035/diff/23001/content/browser/media_galle... content/browser/media_gallery/media_gallery_types.cc:10: : entry_count(0), On 2012/03/07 21:24:37, Lei Zhang wrote: > I think you want to init |id| to 0 in both ctors as well? Done.
lgtm http://codereview.chromium.org/9567035/diff/28001/content/browser/media_galle... File content/browser/media_gallery/media_gallery_database.cc (right): http://codereview.chromium.org/9567035/diff/28001/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:137: base::ReplaceChars(path, "\\", "/", &path); Should this just go up to into the #if defined(OS_WIN) conditional? Very few places use FILE_PATH_USES_WIN_SEPARATORS.
http://codereview.chromium.org/9567035/diff/28001/content/browser/media_galle... File content/browser/media_gallery/media_gallery_database.cc (right): http://codereview.chromium.org/9567035/diff/28001/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:137: base::ReplaceChars(path, "\\", "/", &path); On 2012/03/07 21:47:43, Lei Zhang wrote: > Should this just go up to into the #if defined(OS_WIN) conditional? Very few > places use FILE_PATH_USES_WIN_SEPARATORS. The two defines go hand in hand (you'll never find one defined without the other), but semantically I think this one makes more sense here. I am open to changing it, though.
http://codereview.chromium.org/9567035/diff/28001/content/browser/media_galle... File content/browser/media_gallery/media_gallery_database.cc (right): http://codereview.chromium.org/9567035/diff/28001/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:62: if (!db_.Open(database_dir.Append(FilePath(kMediaGalleryDatabaseName)))) Should we detect a readonly file system at this point and return a different error code? http://codereview.chromium.org/9567035/diff/28001/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:146: VLOG(0) << "Failed to add collection " << row->path.value() Should this be a DVLOG(1) ? http://codereview.chromium.org/9567035/diff/28001/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:193: // TODO(tpayne): Add unique constraint on path once we standardize on path has the predicate here been satisfied ? http://codereview.chromium.org/9567035/diff/28001/content/browser/media_galle... File content/browser/media_gallery/media_gallery_database.h (right): http://codereview.chromium.org/9567035/diff/28001/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.h:39: We'll also need something to update a row, but that can come later. http://codereview.chromium.org/9567035/diff/28001/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.h:40: protected: I think these can be private - There are no plans to inherit from MediaGalleryDatabase, right? http://codereview.chromium.org/9567035/diff/28001/content/browser/media_galle... File content/browser/media_gallery/media_gallery_types.h (right): http://codereview.chromium.org/9567035/diff/28001/content/browser/media_galle... content/browser/media_gallery/media_gallery_types.h:5: #ifndef CONTENT_BROWSER_MEDIA_GALLERY_MEDIA_GALLERY_TYPES_H_ May want to make the name more DB specific - There's going to be a lot of "types"/classes for the media gallery in general.
http://codereview.chromium.org/9567035/diff/28001/content/browser/media_galle... File content/browser/media_gallery/media_gallery_database.cc (right): http://codereview.chromium.org/9567035/diff/28001/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:62: if (!db_.Open(database_dir.Append(FilePath(kMediaGalleryDatabaseName)))) On 2012/03/07 23:28:31, vandebo wrote: > Should we detect a readonly file system at this point and return a different > error code? I think that belongs a level above this class. http://codereview.chromium.org/9567035/diff/28001/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:146: VLOG(0) << "Failed to add collection " << row->path.value() On 2012/03/07 23:28:31, vandebo wrote: > Should this be a DVLOG(1) ? I was thinking this may be a useful error message, even in release mode. http://codereview.chromium.org/9567035/diff/28001/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.cc:193: // TODO(tpayne): Add unique constraint on path once we standardize on path On 2012/03/07 23:28:31, vandebo wrote: > has the predicate here been satisfied ? I'd like to leave the TODO to the next CL. In that CL, I've already added the necessary framework for handling constraint violations. http://codereview.chromium.org/9567035/diff/28001/content/browser/media_galle... File content/browser/media_gallery/media_gallery_database.h (right): http://codereview.chromium.org/9567035/diff/28001/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.h:39: On 2012/03/07 23:28:31, vandebo wrote: > We'll also need something to update a row, but that can come later. Yes, that will come later. http://codereview.chromium.org/9567035/diff/28001/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.h:40: protected: On 2012/03/07 23:28:31, vandebo wrote: > I think these can be private - There are no plans to inherit from > MediaGalleryDatabase, right? It is inherited by the test code. http://codereview.chromium.org/9567035/diff/28001/content/browser/media_galle... File content/browser/media_gallery/media_gallery_types.h (right): http://codereview.chromium.org/9567035/diff/28001/content/browser/media_galle... content/browser/media_gallery/media_gallery_types.h:5: #ifndef CONTENT_BROWSER_MEDIA_GALLERY_MEDIA_GALLERY_TYPES_H_ On 2012/03/07 23:28:31, vandebo wrote: > May want to make the name more DB specific - There's going to be a lot of > "types"/classes for the media gallery in general. Done.
I can't tell what this class is used for since there are no comments in the header above the class and no associated bug. as a result i can't figure out if this belongs in content or not.
On 2012/03/08 16:33:22, John Abd-El-Malek wrote: > I can't tell what this class is used for since there are no comments in the > header above the class and no associated bug. as a result i can't figure out if > this belongs in content or not. Class comment added. Sorry about the Patch Set name. On a slow connection and got ahead of myself.
please add some description to the changelist so that people looking at the svn history have some background there's no BUG filed for a feature this big? http://codereview.chromium.org/9567035/diff/34001/content/browser/media_galle... File content/browser/media_gallery/media_gallery_database.h (right): http://codereview.chromium.org/9567035/diff/34001/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.h:17: namespace media_gallery { why is this in a different namespace from "content"? everything in content should be in the "content" namespace which is our convention for modules (other than chrome)
http://codereview.chromium.org/9567035/diff/34001/content/browser/media_galle... File content/browser/media_gallery/media_gallery_database.h (right): http://codereview.chromium.org/9567035/diff/34001/content/browser/media_galle... content/browser/media_gallery/media_gallery_database.h:17: namespace media_gallery { On 2012/03/09 01:48:06, John Abd-El-Malek wrote: > why is this in a different namespace from "content"? everything in content > should be in the "content" namespace which is our convention for modules (other > than chrome) Done.
On 2012/03/09 01:48:06, John Abd-El-Malek wrote: > please add some description to the changelist so that people looking at the svn > history have some background Done > there's no BUG filed for a feature this big? Done http://codereview.chromium.org/9567035/diff/34001/content/browser/media_galle... > File content/browser/media_gallery/media_gallery_database.h (right): > > http://codereview.chromium.org/9567035/diff/34001/content/browser/media_galle... > content/browser/media_gallery/media_gallery_database.h:17: namespace > media_gallery { > why is this in a different namespace from "content"? everything in content > should be in the "content" namespace which is our convention for modules (other > than chrome)
Ping
On 2012/03/14 18:11:58, tpayne wrote: > Ping Steve or Lei, can you recommend another reviewer?
lgtm, sorry I just saw this. in the future feel free to IM me if I don't respond in a few hours
actually, hold on. i just saw the referenced bug. that says this is for extensions. extensions is not part of the web platform, in which case this wouldn't belong in content. instead it should be in chrome. see http://www.chromium.org/developers/content-module
also, if this is why content/browser/system_message_window_win* and content/browser/media_device_notifications_linux* got added, these need to be moved out to chrome/browser. i suggest a media_gallery subdirectory there On Wed, Mar 14, 2012 at 6:43 PM, <jam@chromium.org> wrote: > actually, hold on. i just saw the referenced bug. that says this is for > extensions. > > extensions is not part of the web platform, in which case this wouldn't > belong > in content. instead it should be in chrome. see > http://www.chromium.org/**developers/content-module<http://www.chromium.org/d... > > http://codereview.chromium.**org/9567035/<http://codereview.chromium.org/9567... >
So... the media device notification interface is in base, so the implementation should be in there, except is can't be (windows code requires a graphics context, Linux requires a specific thread). Web intents will use device notifications irrespective of Media gallery, so that's why I thought media attach belonged in content. But thinking about it a bit further, you could say that media attach notification isn't needed for intents to work, it's just how Chrome is using intents. So by that reasoning, it should move up to Chrome. However, the windows code (functionality wise) is shared with the gamepad code, which is part of content. So maybe the windows code should stay in content and the Linux and Mac (currently reverted) code should go to Chrome. What do you think? -- Steve On Wed, Mar 14, 2012 at 9:29 PM, John Abd-El-Malek <jam@chromium.org> wrote: > also, if this is why content/browser/system_message_window_win* and > content/browser/media_device_notifications_linux* got added, these need to > be moved out to chrome/browser. i suggest a media_gallery subdirectory there > > > On Wed, Mar 14, 2012 at 6:43 PM, <jam@chromium.org> wrote: > >> actually, hold on. i just saw the referenced bug. that says this is for >> extensions. >> >> extensions is not part of the web platform, in which case this wouldn't >> belong >> in content. instead it should be in chrome. see >> http://www.chromium.org/**developers/content-module<http://www.chromium.org/d... >> >> http://codereview.chromium.**org/9567035/<http://codereview.chromium.org/9567... >> > >
On 2012/03/15 01:43:41, John Abd-El-Malek wrote: > actually, hold on. i just saw the referenced bug. that says this is for > extensions. > > extensions is not part of the web platform, in which case this wouldn't belong > in content. instead it should be in chrome. see > http://www.chromium.org/developers/content-module Done.
On Thu, Mar 15, 2012 at 1:59 PM, Steve VanDeBogart <vandebo@chromium.org>wrote: > So... the media device notification interface is in base, so the > implementation should be in there, except is can't be (windows code > requires a graphics context, Linux requires a specific thread). Web > intents will use device notifications irrespective of Media gallery, so > that's why I thought media attach belonged in content. But thinking about > it a bit further, you could say that media attach notification isn't needed > for intents to work, it's just how Chrome is using intents. So by that > reasoning, it should move up to Chrome. However, the windows code > (functionality wise) is shared with the gamepad code, which is part of > content. So maybe the windows code should stay in content and the Linux > and Mac (currently reverted) code should go to Chrome. What do you think? > The 3 platforms should stay in the same module. It's confusing, and also hard to share code, otherwise. What is the shared code between this and gamepad? > > -- > Steve > > > On Wed, Mar 14, 2012 at 9:29 PM, John Abd-El-Malek <jam@chromium.org>wrote: > >> also, if this is why content/browser/system_message_window_win* and >> content/browser/media_device_notifications_linux* got added, these need to >> be moved out to chrome/browser. i suggest a media_gallery subdirectory there >> >> >> On Wed, Mar 14, 2012 at 6:43 PM, <jam@chromium.org> wrote: >> >>> actually, hold on. i just saw the referenced bug. that says this is for >>> extensions. >>> >>> extensions is not part of the web platform, in which case this wouldn't >>> belong >>> in content. instead it should be in chrome. see >>> http://www.chromium.org/**developers/content-module<http://www.chromium.org/d... >>> >>> http://codereview.chromium.**org/9567035/<http://codereview.chromium.org/9567... >>> >> >> >
bcc: chromium-reviews@chromium.org, joi+watch-content@chromium.org, darin-cc@chromium.org On Fri, Mar 16, 2012 at 12:42 AM, John Abd-El-Malek <jam@chromium.org>wrote: > > > On Thu, Mar 15, 2012 at 1:59 PM, Steve VanDeBogart <vandebo@chromium.org>wrote: > >> So... the media device notification interface is in base, so the >> implementation should be in there, except is can't be (windows code >> requires a graphics context, Linux requires a specific thread). Web >> intents will use device notifications irrespective of Media gallery, so >> that's why I thought media attach belonged in content. But thinking about >> it a bit further, you could say that media attach notification isn't needed >> for intents to work, it's just how Chrome is using intents. So by that >> reasoning, it should move up to Chrome. However, the windows code >> (functionality wise) is shared with the gamepad code, which is part of >> content. So maybe the windows code should stay in content and the Linux >> and Mac (currently reverted) code should go to Chrome. What do you think? >> > > The 3 platforms should stay in the same module. It's confusing, and also > hard to share code, otherwise. > > What is the shared code between this and gamepad? > content/browser/system_message_window_win.cc is pretty simple if you want to take a look. Both the gamepad code and the media device attach/detach code list for variants of the same notification. I don't know enough about windows to say if it would be ugly/harmful to listen for these event notifications in two different parts of the codebase. -- Steve On Wed, Mar 14, 2012 at 9:29 PM, John Abd-El-Malek <jam@chromium.org> wrote: >> >>> also, if this is why content/browser/system_message_window_win* and >>> content/browser/media_device_notifications_linux* got added, these need to >>> be moved out to chrome/browser. i suggest a media_gallery subdirectory there >>> >>> >>> On Wed, Mar 14, 2012 at 6:43 PM, <jam@chromium.org> wrote: >>> >>>> actually, hold on. i just saw the referenced bug. that says this is for >>>> extensions. >>>> >>>> extensions is not part of the web platform, in which case this wouldn't >>>> belong >>>> in content. instead it should be in chrome. see >>>> http://www.chromium.org/**developers/content-module<http://www.chromium.org/d... >>>> >>>> http://codereview.chromium.**org/9567035/<http://codereview.chromium.org/9567... >>>> >>> >>> >> >
LGTM for the latest patchset with the move to chrome/
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tpayne@chromium.org/9567035/55001
Change committed as 128219 |
