Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7461089:
First implementation of web intents table. (Closed)
|
Created:
9 years, 4 months ago by groby-ooo-7-16 Modified:
9 years, 4 months ago CC:
chromium-reviews, dhollowa Visibility:
Public. |
DescriptionFirst implementation of web intents table.
BUG=none
TEST=none
R=dhollowa@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=94355
Patch Set 1 #
Total comments: 44
Patch Set 2 : Fixed review issues. #
Total comments: 22
Patch Set 3 : More review fixes, small test refactor. #
Total comments: 32
Patch Set 4 : More review fixes. #
Total comments: 4
Patch Set 5 : Final nit fixes #
Messages
Total messages: 18 (0 generated)
http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... File chrome/browser/webdata/web_intents_table.cc (right): http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.cc:19: bool WebIntentsTable::InitWebIntentsTable() { I'd prefer to remove this level of indirection and move this code to Init(). http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.cc:20: if (!db_->DoesTableExist("web_intents")) { Save indentation by reversing the logic and returning early: if (db->DoesTableExist()) return true; ... http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.cc:72: s.BindString(0, service_url.spec()); Blank line for consistency and readability. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.cc:87: } Blank line, as above. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... File chrome/browser/webdata/web_intents_table.h (right): http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.h:9: #include "base/string16.h" #include "base/basictypes.h" for DISALLOW_COPY_AND_ASSIGN http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.h:15: // Describes the relevant elements of a WebIntent. Add a TODO noting this should likely be moved once we have more of the infrastructure in place. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.h:16: class WebIntent { Use a struct. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.h:18: string16 action; Document the members. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.h:20: GURL service_url; Would be nice to align the ordering of the members with the ordering of the columns in the table. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.h:23: // This class manages the WebIntents tables within the SQLite database passed For now, s/tables/table/. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.h:34: : WebDatabaseTable(db, meta_table) {} Move implementation to source file, here and elsewhere. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.h:37: virtual bool Init(); // WebDatabaseTable implementation. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.h:40: bool SetWebIntent(const string16& action, Document these methods. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.h:45: std::vector<WebIntent>& intents); Hmm, if we're modifying |intents|, this should be a pointer, not a non-const ref. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... File chrome/browser/webdata/web_intents_table_unittest.cc (right): http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table_unittest.cc:22: virtual ~WebIntentsTableTest() {} Can likely leave off the destructor. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table_unittest.cc:43: Remove extra blank line. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table_unittest.cc:45: TEST_F(WebIntentsTableTest, SetGetDeleteIntent) { Document the test case. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table_unittest.cc:50: string16 action=ASCIIToUTF16("http://webintents.org/intents/share"); Spaces around '=', here and elsewhere. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table_unittest.cc:63: EXPECT_EQ(1U, intents.size()); When checking size of a vector you're going to access at some point, use ASSERT.
Drive-by with a test comment. Please let me do another review after submitting the updated version. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... File chrome/browser/webdata/web_intents_table_unittest.cc (right): http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table_unittest.cc:31: file_util::Delete(file_, false); I think ScopedTempDir would be better. It handles tricky cases well especially on Windows. Could you use it? http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table_unittest.cc:41: DISALLOW_COPY_AND_ASSIGN(WebIntentsTableTest); nit: No need for this for a test fixture.
http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... File chrome/browser/webdata/web_intents_table.h (right): http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.h:16: class WebIntent { s/WebIntent/WebIntentData/
http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... File chrome/browser/webdata/web_intents_table.cc (right): http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.cc:19: bool WebIntentsTable::InitWebIntentsTable() { On 2011/07/26 20:21:53, James Hawkins wrote: > I'd prefer to remove this level of indirection and move this code to Init(). Done. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.cc:20: if (!db_->DoesTableExist("web_intents")) { On 2011/07/26 20:21:53, James Hawkins wrote: > Save indentation by reversing the logic and returning early: > > if (db->DoesTableExist()) > return true; > > ... Done. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.cc:72: s.BindString(0, service_url.spec()); On 2011/07/26 20:21:53, James Hawkins wrote: > Blank line for consistency and readability. Done. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.cc:87: } On 2011/07/26 20:21:53, James Hawkins wrote: > Blank line, as above. Done. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... File chrome/browser/webdata/web_intents_table.h (right): http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.h:9: #include "base/string16.h" On 2011/07/26 20:21:53, James Hawkins wrote: > #include "base/basictypes.h" for DISALLOW_COPY_AND_ASSIGN Done. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.h:15: // Describes the relevant elements of a WebIntent. On 2011/07/26 20:21:53, James Hawkins wrote: > Add a TODO noting this should likely be moved once we have more of the > infrastructure in place. Done. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.h:16: class WebIntent { On 2011/07/26 20:21:53, James Hawkins wrote: > Use a struct. Done. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.h:16: class WebIntent { On 2011/07/26 20:46:18, James Hawkins wrote: > s/WebIntent/WebIntentData/ Done. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.h:18: string16 action; On 2011/07/26 20:21:53, James Hawkins wrote: > Document the members. Done. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.h:20: GURL service_url; On 2011/07/26 20:21:53, James Hawkins wrote: > Would be nice to align the ordering of the members with the ordering of the > columns in the table. Done. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.h:23: // This class manages the WebIntents tables within the SQLite database passed On 2011/07/26 20:21:53, James Hawkins wrote: > For now, s/tables/table/. Done. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.h:34: : WebDatabaseTable(db, meta_table) {} On 2011/07/26 20:21:53, James Hawkins wrote: > Move implementation to source file, here and elsewhere. Done. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.h:37: virtual bool Init(); On 2011/07/26 20:21:53, James Hawkins wrote: > // WebDatabaseTable implementation. Done. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.h:40: bool SetWebIntent(const string16& action, On 2011/07/26 20:21:53, James Hawkins wrote: > Document these methods. Done. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table.h:45: std::vector<WebIntent>& intents); On 2011/07/26 20:21:53, James Hawkins wrote: > Hmm, if we're modifying |intents|, this should be a pointer, not a non-const > ref. Done. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... File chrome/browser/webdata/web_intents_table_unittest.cc (right): http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table_unittest.cc:22: virtual ~WebIntentsTableTest() {} On 2011/07/26 20:21:53, James Hawkins wrote: > Can likely leave off the destructor. Done. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table_unittest.cc:31: file_util::Delete(file_, false); On 2011/07/26 20:33:48, Paweł Hajdan Jr. wrote: > I think ScopedTempDir would be better. It handles tricky cases well especially > on Windows. Could you use it? Done. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table_unittest.cc:41: DISALLOW_COPY_AND_ASSIGN(WebIntentsTableTest); On 2011/07/26 20:33:48, Paweł Hajdan Jr. wrote: > nit: No need for this for a test fixture. Done. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table_unittest.cc:43: On 2011/07/26 20:21:53, James Hawkins wrote: > Remove extra blank line. Done. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table_unittest.cc:45: TEST_F(WebIntentsTableTest, SetGetDeleteIntent) { On 2011/07/26 20:21:53, James Hawkins wrote: > Document the test case. Done. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table_unittest.cc:50: string16 action=ASCIIToUTF16("http://webintents.org/intents/share"); On 2011/07/26 20:21:53, James Hawkins wrote: > Spaces around '=', here and elsewhere. Done. http://codereview.chromium.org/7461089/diff/1/chrome/browser/webdata/web_inte... chrome/browser/webdata/web_intents_table_unittest.cc:63: EXPECT_EQ(1U, intents.size()); On 2011/07/26 20:21:53, James Hawkins wrote: > When checking size of a vector you're going to access at some point, use ASSERT. Done.
http://codereview.chromium.org/7461089/diff/5001/chrome/browser/webdata/web_i... File chrome/browser/webdata/web_intents_table_unittest.cc (right): http://codereview.chromium.org/7461089/diff/5001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:25: file_util::Delete(file_, false); You don't need it now. http://codereview.chromium.org/7461089/diff/5001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:29: file_util::Delete(file_, false); You don't need it now either (please remove TearDown completely).
http://codereview.chromium.org/7461089/diff/5001/chrome/browser/webdata/web_i... File chrome/browser/webdata/web_intents_table.cc (right): http://codereview.chromium.org/7461089/diff/5001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.cc:38: bool WebIntentsTable::IsSyncable() { Add a TODO(jhawkins) to figure out the Sync story for Web Intents. http://codereview.chromium.org/7461089/diff/5001/chrome/browser/webdata/web_i... File chrome/browser/webdata/web_intents_table.h (right): http://codereview.chromium.org/7461089/diff/5001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.h:14: #include <vector> C++ includes go before chrome includes. http://codereview.chromium.org/7461089/diff/5001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.h:17: // TODO: Will need to be moved to different place once more infrastructure Always add an ldap to a TODO. Feel free to use mine here. TODO(jhawkins): http://codereview.chromium.org/7461089/diff/5001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.h:34: // Remove this last empty comment line. http://codereview.chromium.org/7461089/diff/5001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.h:38: virtual ~WebIntentsTable() {} Move this implementation to the source file as well. http://codereview.chromium.org/7461089/diff/5001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.h:46: Remove blank line. http://codereview.chromium.org/7461089/diff/5001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.h:55: bool RemoveWebIntent(const string16& action, Document this method (yes, I know it's somewhat obvious, but there should be documentation anyway). http://codereview.chromium.org/7461089/diff/5001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.h:60: bool InitWebIntentsTable(); This method is no longer used; remove. http://codereview.chromium.org/7461089/diff/5001/chrome/browser/webdata/web_i... File chrome/browser/webdata/web_intents_table_unittest.cc (right): http://codereview.chromium.org/7461089/diff/5001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:19: WebIntentsTableTest() {} Can remove this constructor.
http://codereview.chromium.org/7461089/diff/5001/chrome/browser/webdata/web_i... File chrome/browser/webdata/web_intents_table.cc (right): http://codereview.chromium.org/7461089/diff/5001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.cc:38: bool WebIntentsTable::IsSyncable() { On 2011/07/26 21:48:41, James Hawkins wrote: > Add a TODO(jhawkins) to figure out the Sync story for Web Intents. Done. http://codereview.chromium.org/7461089/diff/5001/chrome/browser/webdata/web_i... File chrome/browser/webdata/web_intents_table.h (right): http://codereview.chromium.org/7461089/diff/5001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.h:14: #include <vector> On 2011/07/26 21:48:41, James Hawkins wrote: > C++ includes go before chrome includes. Done. http://codereview.chromium.org/7461089/diff/5001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.h:17: // TODO: Will need to be moved to different place once more infrastructure On 2011/07/26 21:48:41, James Hawkins wrote: > Always add an ldap to a TODO. Feel free to use mine here. > > TODO(jhawkins): Keeping it mine for now :) http://codereview.chromium.org/7461089/diff/5001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.h:34: // On 2011/07/26 21:48:41, James Hawkins wrote: > Remove this last empty comment line. Done. http://codereview.chromium.org/7461089/diff/5001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.h:38: virtual ~WebIntentsTable() {} On 2011/07/26 21:48:41, James Hawkins wrote: > Move this implementation to the source file as well. Done. http://codereview.chromium.org/7461089/diff/5001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.h:46: On 2011/07/26 21:48:41, James Hawkins wrote: > Remove blank line. Done. http://codereview.chromium.org/7461089/diff/5001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.h:55: bool RemoveWebIntent(const string16& action, On 2011/07/26 21:48:41, James Hawkins wrote: > Document this method (yes, I know it's somewhat obvious, but there should be > documentation anyway). Done. http://codereview.chromium.org/7461089/diff/5001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.h:60: bool InitWebIntentsTable(); On 2011/07/26 21:48:41, James Hawkins wrote: > This method is no longer used; remove. Done. http://codereview.chromium.org/7461089/diff/5001/chrome/browser/webdata/web_i... File chrome/browser/webdata/web_intents_table_unittest.cc (right): http://codereview.chromium.org/7461089/diff/5001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:19: WebIntentsTableTest() {} On 2011/07/26 21:48:41, James Hawkins wrote: > Can remove this constructor. Done. http://codereview.chromium.org/7461089/diff/5001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:25: file_util::Delete(file_, false); On 2011/07/26 21:46:25, Paweł Hajdan Jr. wrote: > You don't need it now. Done. http://codereview.chromium.org/7461089/diff/5001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:29: file_util::Delete(file_, false); On 2011/07/26 21:46:25, Paweł Hajdan Jr. wrote: > You don't need it now either (please remove TearDown completely). Done.
http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... File chrome/browser/webdata/web_intents_table_unittest.cc (right): http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:24: CHECK(db_ != NULL); Could you remove this CHECK? It's going to crash anyway right on the next line, and NULL dereference is trivial to debug, *and* CHECKs are generally discouraged in unit tests because they crash the entire binary, causing following tests not to run. http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:32: ASSERT_TRUE(db_ != NULL); nit: Not needed. http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:38: delete db_; Either use scoped_ptr or just do WebDatabase db_; which is simpler, and works: the entire test fixture class is created fresh and destroyed for each test case.
http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... File chrome/browser/webdata/web_intents_table.cc (right): http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.cc:33: "ON web_intents (action,type)")) { nit: Space after comma. http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... File chrome/browser/webdata/web_intents_table.h (right): http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.h:20: GURL service_url; // URL for service invocation. nit: Align using two spaces after longest member var (to conserve horizontal space). http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... File chrome/browser/webdata/web_intents_table_unittest.cc (right): http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:21: WebIntentsTableTest() : db_(NULL) {} Move these methods to the protected section. http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:24: CHECK(db_ != NULL); Don't use CHECK (or DCHECK) in testing code. Use ASSERT_NE(...) http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:32: ASSERT_TRUE(db_ != NULL); ASSERT_NE(...) http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:33: ASSERT_EQ(sql::INIT_OK, Params should line up in the same column if wrapping is required, here and elsewhere. http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:37: virtual void TearDown() { Use a scoped_ptr for |db_| and get rid of TearDown(). http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:45: namespace { Wrap the entire test implementation (including the test fixture definition) inside the unnamed namespace. http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:46: static GURL test_url("http://google.com/"); Don't use static inside an unnamed namespace. http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:47: static string16 test_action = ASCIIToUTF16("http://webintents.org/intents/share"); nit: 80 cols http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:88: // Recover stored intents from DB Period at end of sentence, here and elsewhere. http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:94: std::swap(intents[0],intents[1]); Space after comma. http://codereview.chromium.org/7461089/diff/1011/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/7461089/diff/1011/chrome/chrome_browser.gypi#n... chrome/chrome_browser.gypi:3686: 'browser/webdata/web_intents_table_table.h', web_intents_table.h
http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... File chrome/browser/webdata/web_intents_table.cc (right): http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.cc:33: "ON web_intents (action,type)")) { On 2011/07/26 23:35:06, James Hawkins wrote: > nit: Space after comma. Done. http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... File chrome/browser/webdata/web_intents_table.h (right): http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.h:20: GURL service_url; // URL for service invocation. On 2011/07/26 23:35:06, James Hawkins wrote: > nit: Align using two spaces after longest member var (to conserve horizontal > space). Done. http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... File chrome/browser/webdata/web_intents_table_unittest.cc (right): http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:21: WebIntentsTableTest() : db_(NULL) {} On 2011/07/26 23:35:06, James Hawkins wrote: > Move these methods to the protected section. Done. http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:24: CHECK(db_ != NULL); On 2011/07/26 23:35:06, James Hawkins wrote: > Don't use CHECK (or DCHECK) in testing code. > > Use ASSERT_NE(...) Done. http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:24: CHECK(db_ != NULL); On 2011/07/26 23:30:53, Paweł Hajdan Jr. wrote: > Could you remove this CHECK? It's going to crash anyway right on the next line, > and NULL dereference is trivial to debug, *and* CHECKs are generally discouraged > in unit tests because they crash the entire binary, causing following tests not > to run. Done. http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:32: ASSERT_TRUE(db_ != NULL); On 2011/07/26 23:30:53, Paweł Hajdan Jr. wrote: > nit: Not needed. Done. http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:32: ASSERT_TRUE(db_ != NULL); On 2011/07/26 23:35:06, James Hawkins wrote: > ASSERT_NE(...) Done. http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:33: ASSERT_EQ(sql::INIT_OK, On 2011/07/26 23:35:06, James Hawkins wrote: > Params should line up in the same column if wrapping is required, here and > elsewhere. Done. http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:37: virtual void TearDown() { On 2011/07/26 23:35:06, James Hawkins wrote: > Use a scoped_ptr for |db_| and get rid of TearDown(). Done. http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:38: delete db_; On 2011/07/26 23:30:53, Paweł Hajdan Jr. wrote: > Either use scoped_ptr or just do WebDatabase db_; which is simpler, and works: > the entire test fixture class is created fresh and destroyed for each test case. Done. http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:45: namespace { On 2011/07/26 23:35:06, James Hawkins wrote: > Wrap the entire test implementation (including the test fixture definition) > inside the unnamed namespace. Done. http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:46: static GURL test_url("http://google.com/"); On 2011/07/26 23:35:06, James Hawkins wrote: > Don't use static inside an unnamed namespace. Done. http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:47: static string16 test_action = ASCIIToUTF16("http://webintents.org/intents/share"); On 2011/07/26 23:35:06, James Hawkins wrote: > nit: 80 cols Done. http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:88: // Recover stored intents from DB On 2011/07/26 23:35:06, James Hawkins wrote: > Period at end of sentence, here and elsewhere. Done. http://codereview.chromium.org/7461089/diff/1011/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:94: std::swap(intents[0],intents[1]); On 2011/07/26 23:35:06, James Hawkins wrote: > Space after comma. Done. http://codereview.chromium.org/7461089/diff/1011/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/7461089/diff/1011/chrome/chrome_browser.gypi#n... chrome/chrome_browser.gypi:3686: 'browser/webdata/web_intents_table_table.h', On 2011/07/26 23:35:06, James Hawkins wrote: > web_intents_table.h Done.
Code I commented in the drive-by LGTM, thank you!
LTGM with two nits. http://codereview.chromium.org/7461089/diff/7008/chrome/browser/webdata/web_i... File chrome/browser/webdata/web_intents_table.h (right): http://codereview.chromium.org/7461089/diff/7008/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.h:20: GURL service_url; // URL for service invocation. Sorry I didn't catch this before. Don't line up member vars in a struct. http://codereview.chromium.org/7461089/diff/7008/chrome/browser/webdata/web_i... File chrome/browser/webdata/web_intents_table_unittest.cc (right): http://codereview.chromium.org/7461089/diff/7008/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:38: string16 test_action = ASCIIToUTF16("http://webintents.org/intents/share"); nit: Move the vars above the class.
On 2011/07/27 00:19:53, James Hawkins wrote: > LTGM with two nits. > LGTM that is.
Final nits fixed. Will pull trigger tomorrow. http://codereview.chromium.org/7461089/diff/7008/chrome/browser/webdata/web_i... File chrome/browser/webdata/web_intents_table.h (right): http://codereview.chromium.org/7461089/diff/7008/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.h:20: GURL service_url; // URL for service invocation. On 2011/07/27 00:19:53, James Hawkins wrote: > Sorry I didn't catch this before. Don't line up member vars in a struct. Done. http://codereview.chromium.org/7461089/diff/7008/chrome/browser/webdata/web_i... File chrome/browser/webdata/web_intents_table_unittest.cc (right): http://codereview.chromium.org/7461089/diff/7008/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table_unittest.cc:38: string16 test_action = ASCIIToUTF16("http://webintents.org/intents/share"); On 2011/07/27 00:19:53, James Hawkins wrote: > nit: Move the vars above the class. Done.
Change committed as FAKE
Sorry, error on my part. I reset the commit flag.
Change committed as 94355 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
