Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11359174:
Convert HTML5 notification pyauto tests to browser_tests. (Closed)
|
Created:
8 years, 1 month ago by chrisgao (Use stgao instead) Modified:
7 years, 9 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionConvert HTML5 notification pyauto tests to browser_tests.
BUG=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170623
Patch Set 1 #Patch Set 2 : Merge new codes and delete notifications.py #
Total comments: 62
Patch Set 3 : Addressed comments from Ken. #
Total comments: 24
Patch Set 4 : Fix build errors in Mac&Win #
Total comments: 34
Patch Set 5 : . #Patch Set 6 : . #Patch Set 7 : . #
Total comments: 14
Patch Set 8 : . #
Total comments: 30
Patch Set 9 : . #
Total comments: 2
Patch Set 10 : . #
Messages
Total messages: 20 (0 generated)
Merged new codes in and deleted notifications.py No changes to pyauto C++ backend because the methods are used in other pyauto testcases(chrome/test/functional/wifi_notification.py, chrome/test/functional/chromeos_txt_msg_functional.py).
https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... File chrome/browser/notifications/notification_browsertest.cc (right): https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:48: // InProcessBrowserTest The preferred style is: Overriden from InProcessBrowserTest: https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:51: // content::NotificationObserver same here https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:54: const content::NotificationDetails& details) OVERRIDE; for chromium, we usually encourage you to include headers for what you use. Here you are using the OVERRIDE macro, which means you should include the appropriate header defining this macro. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:61: const size_t count, don't put const for primitives https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:71: void Debug(); remove? https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:88: const bool waiting, indent is wrong here https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:115: static const std::string kReference; http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Static_and_Glo... https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:122: class BrowserOpenedNotificationObserver : move this class out. put it in an unnamed namespace https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:125: BrowserOpenedNotificationObserver(); indent is wrong https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:313: BrowserOpenedNotificationObserver signal; We should be able to do something like: ui_test_utils::BrowserAddedObserver browser_added_observer; chrome::NewEmptyWindow(browser()->profile()); Browser* second_window = browser_added_observer.WaitForSingleNewBrowser(); I think you said something about crashing if you tried this. What is the crash/race? https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:323: content::WindowedNotificationObserver signal( rename all instances of signal to observer; that fits better with the name of the class. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:330: void NotificationsPermissionTest::CloseTab(Browser* browser, const int index) { You only use this func in one place, so remove this func and move the code where it is used https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:359: bool NotificationsPermissionTest::checkOriginInSetting( Check https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:447: chrome::GetWebContentsAt(browser, index)); indent https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:483: std::string script("createNotification("); use base::StringPrintf https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:503: LOG(ERROR) << "Create and Wait"; remove all logs https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:562: void NotificationsPermissionTest::RequestPermission(Browser* browser) { return bool here and ASSERT_TRUE in the tests, so that we don't continue the test if this fails for some reason https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:575: std::string script("cancelNotification("); base::StringPrintf https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:580: UTF8ToWide(script.c_str(), script.length(), &tmp); I think there's a version of UTF8ToWide that takes a string and returns a wstring. Use that instead and just put the conversion inline when calling ExecuteJavaScript... https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:591: bool NotificationsPermissionTest::PerformActionOnInfobar( Change this to separate allow, deny, and dismiss funcs, or change from strings to an enum. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:698: "files/notifications/no_such_file.png"); indent 4 from left. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:700: EXPECT_EQ(string16(ASCIIToUTF16("My Title")), notification.title()); string16 constructor is not needed https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:701: EXPECT_EQ(string16(ASCIIToUTF16("My Body")), notification.body()); here too https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:708: // Note: webkitNotifications.createHTMLNotification is deprecated . at end of comments https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:713: std::string result = CreateHTMLNotification( std::string result = CreateHTMLNotification( "../empty.html", browser(), "", true); https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:718: EXPECT_EQ(result, "-1"); the standard is EXPECT_EQ(expected, actual); You should switch these two https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:718: EXPECT_EQ(result, "-1"); why is this supposed to be -1? Does the createHTMLNotification function not work? If so, just delete this whole test. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:733: ASSERT_TRUE(CheckNotificationCount(0)); change this function to GetNotificationCount(), so you can do ASSERT_EQ(0, GetNotificationCount()) instead; here and all places https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:884: GURL EXPECTED_ICON_URL = test_server()->GetURL( kExpectedIconUrl https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:887: EXPECT_EQ(string16(ASCIIToUTF16("My Title")), notification.title()); string16 constructor is not needed https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:1147: return; // crbug.com/66072 this bug says the test is currently disabled on mac/linux; any idea what's up here?
Have finished all comments. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... File chrome/browser/notifications/notification_browsertest.cc (right): https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:48: // InProcessBrowserTest On 2012/11/13 23:38:21, kkania wrote: > The preferred style is: > Overriden from InProcessBrowserTest: Done. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:51: // content::NotificationObserver On 2012/11/13 23:38:21, kkania wrote: > same here Done. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:54: const content::NotificationDetails& details) OVERRIDE; On 2012/11/13 23:38:21, kkania wrote: > for chromium, we usually encourage you to include headers for what you use. Here > you are using the OVERRIDE macro, which means you should include the appropriate > header defining this macro. Done. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:61: const size_t count, On 2012/11/13 23:38:21, kkania wrote: > don't put const for primitives Done. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:71: void Debug(); On 2012/11/13 23:38:21, kkania wrote: > remove? Done. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:88: const bool waiting, On 2012/11/13 23:38:21, kkania wrote: > indent is wrong here Done. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:115: static const std::string kReference; On 2012/11/13 23:38:21, kkania wrote: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Static_and_Glo... Done. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:122: class BrowserOpenedNotificationObserver : On 2012/11/13 23:38:21, kkania wrote: > move this class out. put it in an unnamed namespace Done. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:125: BrowserOpenedNotificationObserver(); On 2012/11/13 23:38:21, kkania wrote: > indent is wrong Done. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:313: BrowserOpenedNotificationObserver signal; I have tried the following three ways to create an incognito window: A) InProcessBrowserTest::CreateIncognitoBrowser() B) BrowserOpenedNotificationObserver observer; if (is_incognito) chrome::ExecuteCommand(source_browser, IDC_NEW_INCOGNITO_WINDOW); else chrome::ExecuteCommand(source_browser, IDC_NEW_WINDOW); observer.Wait(); return observer.new_browser(); C) ui_test_utils::BrowserAddedObserver browser_added_observer; Profile* profile = ProfileManager::GetDefaultProfileOrOffTheRecord(); LOG(ERROR) << profile << "," << source_browser->profile(); chrome::NewEmptyWindow( is_incognito ? profile->GetOffTheRecordProfile() : profile); return browser_added_observer.WaitForSingleNewBrowser(); All these three solutions will hit the bug http://crbug.com/160657 with the following error: [660:660:1114/093014:2422471610353:FATAL:profile_destroyer.cc(153)] Check failed: registrar_.IsEmpty(). Some render process hosts where not destroyed early enough! But solution C also crashed occasionally with the following error: [448:489:1114/092824:2422361520442:FATAL:resource_dispatcher_host_impl.cc(896)] Check failed: ContainsKey(active_resource_contexts_, resource_context). 2)[660:660:1114/093014:2422471610353:FATAL:profile_destroyer.cc(153)] Check failed: registrar_.IsEmpty(). Some render process hosts where not destroyed early enough! Solution A also crashed occasionally with another error, but I forgot to take down the error stack. Thus, I prefer solution B for the moment. On 2012/11/13 23:38:21, kkania wrote: > We should be able to do something like: > > ui_test_utils::BrowserAddedObserver browser_added_observer; > chrome::NewEmptyWindow(browser()->profile()); > Browser* second_window = browser_added_observer.WaitForSingleNewBrowser(); > > I think you said something about crashing if you tried this. What is the > crash/race? https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:323: content::WindowedNotificationObserver signal( On 2012/11/13 23:38:21, kkania wrote: > rename all instances of signal to observer; that fits better with the name of > the class. Done. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:330: void NotificationsPermissionTest::CloseTab(Browser* browser, const int index) { On 2012/11/13 23:38:21, kkania wrote: > You only use this func in one place, so remove this func and move the code where > it is used Done. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:359: bool NotificationsPermissionTest::checkOriginInSetting( On 2012/11/13 23:38:21, kkania wrote: > Check Done. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:447: chrome::GetWebContentsAt(browser, index)); On 2012/11/13 23:38:21, kkania wrote: > indent Done. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:483: std::string script("createNotification("); On 2012/11/13 23:38:21, kkania wrote: > use base::StringPrintf Done. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:503: LOG(ERROR) << "Create and Wait"; On 2012/11/13 23:38:21, kkania wrote: > remove all logs Done. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:562: void NotificationsPermissionTest::RequestPermission(Browser* browser) { On 2012/11/13 23:38:21, kkania wrote: > return bool here and ASSERT_TRUE in the tests, so that we don't continue the > test if this fails for some reason Done. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:575: std::string script("cancelNotification("); On 2012/11/13 23:38:21, kkania wrote: > base::StringPrintf Done. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:580: UTF8ToWide(script.c_str(), script.length(), &tmp); On 2012/11/13 23:38:21, kkania wrote: > I think there's a version of UTF8ToWide that takes a string and returns a > wstring. Use that instead and just put the conversion inline when calling > ExecuteJavaScript... Done. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:591: bool NotificationsPermissionTest::PerformActionOnInfobar( Changed to enum. On 2012/11/13 23:38:21, kkania wrote: > Change this to separate allow, deny, and dismiss funcs, or change from strings > to an enum. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:698: "files/notifications/no_such_file.png"); On 2012/11/13 23:38:21, kkania wrote: > indent 4 from left. Done. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:700: EXPECT_EQ(string16(ASCIIToUTF16("My Title")), notification.title()); On 2012/11/13 23:38:21, kkania wrote: > string16 constructor is not needed Done. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:701: EXPECT_EQ(string16(ASCIIToUTF16("My Body")), notification.body()); On 2012/11/13 23:38:21, kkania wrote: > here too Done. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:708: // Note: webkitNotifications.createHTMLNotification is deprecated On 2012/11/13 23:38:21, kkania wrote: > . at end of comments Done. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:713: std::string result = CreateHTMLNotification( On 2012/11/13 23:38:21, kkania wrote: > std::string result = CreateHTMLNotification( > "../empty.html", > browser(), > "", > true); Done. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:718: EXPECT_EQ(result, "-1"); On 2012/11/13 23:38:21, kkania wrote: > the standard is EXPECT_EQ(expected, actual); > You should switch these two Done. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:718: EXPECT_EQ(result, "-1"); Accoring to http://www.w3.org/TR/notifications/, http://crbug.com/156905, and https://plus.sandbox.google.com/u/0/+GoogleChromeDevelopers/posts/8vWo8hq4pDm, I think that webkitNotifications.createHTMLNotification is going away. And when it is called, I got the following exception: TypeError: Object #<NotificationCenter> has no method 'createHTMLNotification' Thus, the whole testcase is removed. So is testcase TestSpecialURLNotification, because it relies on HTML notification. On 2012/11/13 23:38:21, kkania wrote: > why is this supposed to be -1? Does the createHTMLNotification function not > work? If so, just delete this whole test. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:733: ASSERT_TRUE(CheckNotificationCount(0)); On 2012/11/13 23:38:21, kkania wrote: > change this function to GetNotificationCount(), so you can do ASSERT_EQ(0, > GetNotificationCount()) instead; here and all places Done. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:884: GURL EXPECTED_ICON_URL = test_server()->GetURL( On 2012/11/13 23:38:21, kkania wrote: > kExpectedIconUrl Done. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:887: EXPECT_EQ(string16(ASCIIToUTF16("My Title")), notification.title()); On 2012/11/13 23:38:21, kkania wrote: > string16 constructor is not needed Done. https://codereview.chromium.org/11359174/diff/3001/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:1147: return; // crbug.com/66072 Not sure about that. Should we disable this testcase? On 2012/11/13 23:38:21, kkania wrote: > this bug says the test is currently disabled on mac/linux; any idea what's up > here?
https://codereview.chromium.org/11359174/diff/1002/chrome/browser/notificatio... File chrome/browser/notifications/notification_browsertest.cc (right): https://codereview.chromium.org/11359174/diff/1002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:46: const std::string kReference = "preference"; don't indent stuff inside namespace, but leave newline before and after stuff, and include // namespace at end. E.g: namespace { void SomeFunction() { ... } } // namespace https://codereview.chromium.org/11359174/diff/1002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:46: const std::string kReference = "preference"; http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static... https://codereview.chromium.org/11359174/diff/1002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:47: const std::string kExpectedIconUrl = "files/notifications/no_such_file.png"; http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static... https://codereview.chromium.org/11359174/diff/1002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:137: GURL EMPTY_PAGE_URL; constants are kEmptyPageUrl style. There is no consistent rule yet about whether enums are UPPER_STYLE or kThisStyle; however technically these aren't constants, so i think i'd rename them to empty_page_url_ https://codereview.chromium.org/11359174/diff/1002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:278: bool increasing) { instead of passing in increasing arg, how about we just determine it by comparing current count vs desired count? https://codereview.chromium.org/11359174/diff/1002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:280: chrome::GetWebContentsAt(browser, 0))->GetInfoBarCount(); indent https://codereview.chromium.org/11359174/diff/1002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:283: ui_test_utils::RegisterAndWait( how about usr for loop and use content::WindowedObserver. THen we can get rid of the custom observer logic https://codereview.chromium.org/11359174/diff/1002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:291: chrome::GetWebContentsAt(browser, 0))->GetInfoBarCount(); fix all indents https://codereview.chromium.org/11359174/diff/1002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:298: BrowserOpenedNotificationObserver observer; You should use option A and find the underlying problem https://codereview.chromium.org/11359174/diff/1002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:677: IN_PROC_BROWSER_TEST_F( I'm thinking we should perhaps remove this function. It doesn't seem to test notifications at all, just that our test code can modify the preferences correctly. https://codereview.chromium.org/11359174/diff/1002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:1073: browser(), 4, not 5 https://codereview.chromium.org/11359174/diff/1002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:1083: result = CreateNotification( are we guaranteed that this func waits until the replacement is complete? Or is it possible that the icon_url hasn't been changed when we return?
All comments are resolved. https://codereview.chromium.org/11359174/diff/1002/chrome/browser/notificatio... File chrome/browser/notifications/notification_browsertest.cc (right): https://codereview.chromium.org/11359174/diff/1002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:46: const std::string kReference = "preference"; On 2012/11/15 16:29:07, kkania wrote: > don't indent stuff inside namespace, but leave newline before and after stuff, > and include // namespace at end. E.g: > > namespace { > > void SomeFunction() { > ... > } > > } // namespace Done. https://codereview.chromium.org/11359174/diff/1002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:46: const std::string kReference = "preference"; On 2012/11/15 16:29:07, kkania wrote: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static... Done. https://codereview.chromium.org/11359174/diff/1002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:47: const std::string kExpectedIconUrl = "files/notifications/no_such_file.png"; On 2012/11/15 16:29:07, kkania wrote: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static... Done. https://codereview.chromium.org/11359174/diff/1002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:137: GURL EMPTY_PAGE_URL; On 2012/11/15 16:29:07, kkania wrote: > constants are kEmptyPageUrl style. There is no consistent rule yet about whether > enums are UPPER_STYLE or kThisStyle; however technically these aren't constants, > so i think i'd rename them to empty_page_url_ Done. https://codereview.chromium.org/11359174/diff/1002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:278: bool increasing) { On 2012/11/15 16:29:07, kkania wrote: > instead of passing in increasing arg, how about we just determine it by > comparing current count vs desired count? Function removed. https://codereview.chromium.org/11359174/diff/1002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:280: chrome::GetWebContentsAt(browser, 0))->GetInfoBarCount(); On 2012/11/15 16:29:07, kkania wrote: > indent Done. https://codereview.chromium.org/11359174/diff/1002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:283: ui_test_utils::RegisterAndWait( On 2012/11/15 16:29:07, kkania wrote: > how about usr for loop and use content::WindowedObserver. THen we can get rid of > the custom observer logic Move logic of waiting to function RequestPermission. This custom observer logic is removed. https://codereview.chromium.org/11359174/diff/1002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:291: chrome::GetWebContentsAt(browser, 0))->GetInfoBarCount(); On 2012/11/15 16:29:07, kkania wrote: > fix all indents Done. https://codereview.chromium.org/11359174/diff/1002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:298: BrowserOpenedNotificationObserver observer; On 2012/11/15 16:29:07, kkania wrote: > You should use option A and find the underlying problem Use solution A. https://codereview.chromium.org/11359174/diff/1002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:677: IN_PROC_BROWSER_TEST_F( Agree. It is more like a unit test. Removed. On 2012/11/15 16:29:07, kkania wrote: > I'm thinking we should perhaps remove this function. It doesn't seem to test > notifications at all, just that our test code can modify the preferences > correctly. https://codereview.chromium.org/11359174/diff/1002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:1073: browser(), On 2012/11/15 16:29:07, kkania wrote: > 4, not 5 Done. https://codereview.chromium.org/11359174/diff/1002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:1083: result = CreateNotification( On 2012/11/15 16:29:07, kkania wrote: > are we guaranteed that this func waits until the replacement is complete? Or is > it possible that the icon_url hasn't been changed when we return? According to my test, Chrome supports notification replacement, although it is not documented in http://www.chromium.org/developers/design-documents/desktop-notifications/api.... And since there is only one notification, the old one should not be in the list of pending notifications after it is shown. So according to step 4 in the spec http://www.w3.org/TR/notifications/#replacing-a-notification, the event handler ondisplay is called when the replacement is complete. As there is no notification removal or addition, waiting for NOTIFICATION_NOTIFY_BALLOON_CONNECTED or NOTIFICATION_NOTIFY_BALLOON_DISCONNECTED causes timeout error. Thus, content::ExecuteJavaScriptAndExtractString returns after replacement is completed.
http://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificatio... File chrome/browser/notifications/notification_browsertest.cc (right): http://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:5: #include "base/compiler_specific.h" #include <deque> #include <string> #include "base/compiler_specific.h" ... http://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:33: #include "net/test/test_server.h" #include "testing/gtest/include/gtest/gtest.h" http://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:49: ALLOW = 1, typically style is to just set the value of the first item in the enum, and let the other ones be blank http://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:77: void DropOriginPreference(const GURL& origin); private? http://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:84: bool waiting, wait_for_new_balloon http://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:89: std::string CreateSimpleNotification(Browser* browser, bool waiting); wait_for_new_balloon http://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:111: empty_page_url_ = test_server()->GetURL(kEmptyPageUrl); remove kEmptyPageUrl and put value directly here http://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:112: test_page_url_ = test_server()->GetURL(kTestPageUrl); same for this http://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:180: void NotificationsTest::GetAllowedOrigins(ContentSettingsForOneType* settings) { get rid of this func and just move the body to where it is used. http://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:241: std::string result; move this closer to where it is used http://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:251: if (!waiting) { you can simplify this code by creating the observer always, but doing: if (wait_for_new_balloon) observer.Wait() http://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:298: if (!flag || result != "1") check this before observer.Wait(), so that we don't wait forever when we know that the script failed http://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:300: int new_count = InfoBarTabHelper::FromWebContents( instead of checking this, in the observer, instead of using AllSources(), just use the InfoBarHelper as the source http://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:318: return flag ? (result == "1") : false; return flag && result == "1" http://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:359: ASSERT_TRUE(settings != NULL); remove this statement; ASSERT should be used to verify the code under test, not to check that the test is written properly http://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:364: if (it->setting != setting || it->source.compare(kReference) != 0) { remove kReference and just use "preference" here http://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificatio... chrome/browser/notifications/notification_browsertest.cc:738: #if defined(OS_WIN) I thought you said this test doesn't work on mac/win?
https://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificati... File chrome/browser/notifications/notification_browsertest.cc (right): https://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:5: #include "base/compiler_specific.h" On 2012/11/19 17:08:56, kkania wrote: > #include <deque> > #include <string> > > #include "base/compiler_specific.h" > ... Done. https://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:33: #include "net/test/test_server.h" On 2012/11/19 17:08:56, kkania wrote: > #include "testing/gtest/include/gtest/gtest.h" Done. https://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:49: ALLOW = 1, On 2012/11/19 17:08:56, kkania wrote: > typically style is to just set the value of the first item in the enum, and let > the other ones be blank Done. https://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:77: void DropOriginPreference(const GURL& origin); On 2012/11/19 17:08:56, kkania wrote: > private? Done. https://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:84: bool waiting, On 2012/11/19 17:08:56, kkania wrote: > wait_for_new_balloon Done. https://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:89: std::string CreateSimpleNotification(Browser* browser, bool waiting); On 2012/11/19 17:08:56, kkania wrote: > wait_for_new_balloon Done. https://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:111: empty_page_url_ = test_server()->GetURL(kEmptyPageUrl); On 2012/11/19 17:08:56, kkania wrote: > remove kEmptyPageUrl and put value directly here Done. https://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:112: test_page_url_ = test_server()->GetURL(kTestPageUrl); On 2012/11/19 17:08:56, kkania wrote: > same for this Done. https://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:180: void NotificationsTest::GetAllowedOrigins(ContentSettingsForOneType* settings) { On 2012/11/19 17:08:56, kkania wrote: > get rid of this func and just move the body to where it is used. Done. https://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:241: std::string result; On 2012/11/19 17:08:56, kkania wrote: > move this closer to where it is used Done. https://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:251: if (!waiting) { On 2012/11/19 17:08:56, kkania wrote: > you can simplify this code by creating the observer always, but doing: > if (wait_for_new_balloon) > observer.Wait() Changed as you commented. But can the compiler optimize the code? https://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:298: if (!flag || result != "1") On 2012/11/19 17:08:56, kkania wrote: > check this before observer.Wait(), so that we don't wait forever when we know > that the script failed Done. https://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:300: int new_count = InfoBarTabHelper::FromWebContents( On 2012/11/19 17:08:56, kkania wrote: > instead of checking this, in the observer, instead of using AllSources(), just > use the InfoBarHelper as the source Done. https://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:318: return flag ? (result == "1") : false; On 2012/11/19 17:08:56, kkania wrote: > return flag && result == "1" Done. And also pull checking ahead before waiting in other functions. https://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:359: ASSERT_TRUE(settings != NULL); On 2012/11/19 17:08:56, kkania wrote: > remove this statement; ASSERT should be used to verify the code under test, not > to check that the test is written properly Done. https://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:364: if (it->setting != setting || it->source.compare(kReference) != 0) { On 2012/11/19 17:08:56, kkania wrote: > remove kReference and just use "preference" here Done. https://codereview.chromium.org/11359174/diff/17002/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:738: #if defined(OS_WIN) On 2012/11/19 17:08:56, kkania wrote: > I thought you said this test doesn't work on mac/win? In bot mac, it failed due to small screen. In bot linux, it succeeded. But it failed in my desktop machine through NoMachine Client with a smaller screen as I have shown you. In bot win, the last run (http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/82727/...) succeeded.
https://codereview.chromium.org/11359174/diff/24004/chrome/browser/notificati... File chrome/browser/notifications/notification_browsertest.cc (right): https://codereview.chromium.org/11359174/diff/24004/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:46: #if !defined(OS_CHROMEOS) it looks like the two original tests worked on chromeos. just exclude the new tests that fail, and give a quick explanation https://codereview.chromium.org/11359174/diff/24004/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:100: bool CheckOriginInSetting(const ContentSettingsForOneType & settings, & should be next to type https://codereview.chromium.org/11359174/diff/24004/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:101: const GURL & origin); same here https://codereview.chromium.org/11359174/diff/24004/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:231: bool flag = content::ExecuteJavaScriptAndExtractString( 'flag' is a bit unclear. perhaps success or script_success would be better. same for other uses of 'flag' https://codereview.chromium.org/11359174/diff/24004/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:238: content::RunAllPendingInMessageLoop(); Perhaps add a comment here why this is needed https://codereview.chromium.org/11359174/diff/24004/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:294: content::RunAllPendingInMessageLoop(); Perhaps add a comment here why this is needed https://codereview.chromium.org/11359174/diff/24004/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:734: IN_PROC_BROWSER_TEST_F( just delete this test for now; some bots may have different resolutions that could cause failures elsewhere.
https://codereview.chromium.org/11359174/diff/24004/chrome/browser/notificati... File chrome/browser/notifications/notification_browsertest.cc (right): https://codereview.chromium.org/11359174/diff/24004/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:46: #if !defined(OS_CHROMEOS) On 2012/11/26 21:08:50, kkania wrote: > it looks like the two original tests worked on chromeos. just exclude the new > tests that fail, and give a quick explanation Disable all new testcases. https://codereview.chromium.org/11359174/diff/24004/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:100: bool CheckOriginInSetting(const ContentSettingsForOneType & settings, On 2012/11/26 21:08:50, kkania wrote: > & should be next to type Done. https://codereview.chromium.org/11359174/diff/24004/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:101: const GURL & origin); On 2012/11/26 21:08:50, kkania wrote: > same here Done. https://codereview.chromium.org/11359174/diff/24004/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:231: bool flag = content::ExecuteJavaScriptAndExtractString( On 2012/11/26 21:08:50, kkania wrote: > 'flag' is a bit unclear. perhaps success or script_success would be better. same > for other uses of 'flag' Done. https://codereview.chromium.org/11359174/diff/24004/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:238: content::RunAllPendingInMessageLoop(); On 2012/11/26 21:08:50, kkania wrote: > Perhaps add a comment here why this is needed Use set_on_collection_changed_callback instead. https://codereview.chromium.org/11359174/diff/24004/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:294: content::RunAllPendingInMessageLoop(); On 2012/11/26 21:08:50, kkania wrote: > Perhaps add a comment here why this is needed Use set_on_collection_changed_callback instead. https://codereview.chromium.org/11359174/diff/24004/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:734: IN_PROC_BROWSER_TEST_F( On 2012/11/26 21:08:50, kkania wrote: > just delete this test for now; some bots may have different resolutions that > could cause failures elsewhere. Done.
Hi Steven, I'm from chrome browser QA team. Would you please help review my browser_tests for the HTML5 notification? Best wishes, Shuotao Gao
https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... File chrome/browser/notifications/notification_browsertest.cc (right): https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:91: bool Check() { nit: 'if (done_) return true;' might be slightly more readable instead of 'if (!done_ &&' https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:102: void onCollectionChanged() { OnCollectionChanged() https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:110: const content::NotificationDetails& details) OVERRIDE { test that type == BALLOON_CONNECTED or BALLOON_DISCONNECTED https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:192: return manager->balloon_collection()->GetActiveBalloons(); nit: No need for local manager variable https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:197: return manager->balloon_collection()->GetActiveBalloons().size(); nit: No need for local manager variable https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:204: bool success = manager->CancelById(notification.notification_id()); nit: No need for local manager variable https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:256: ASSERT_TRUE(infobar->AsConfirmInfoBarDelegate()); Move below as ASSERT_TRUE(confirm_infobar) https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:264: EXPECT_TRUE(false); This seems weird, I'd ask about a better way to do this. Maybe just: EXPECT_TRUE(buttons & ConfirmInfoBarDelegate::BUTTON_OK); before the if() https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:271: EXPECT_TRUE(false); same here https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:275: origin.c_str()); Is this translation dependent? Not sure how we do this normally, but seems fragile, e.g. if the text is changed? https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:291: replace_id); nit: no need for separate lines for the args? https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:397: it != settings->end();) { nit: ' ' between ; and ) https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:402: } nit: {} unnecessary for single line ifs. https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:415: } nit: no {} https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:805: "chat"); nit: one or two lines for args, here and below
https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... File chrome/browser/notifications/notification_browsertest.cc (right): https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:91: bool Check() { On 2012/11/29 03:32:16, stevenjb (chromium) wrote: > nit: 'if (done_) return true;' might be slightly more readable instead of 'if > (!done_ &&' Done. https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:102: void onCollectionChanged() { On 2012/11/29 03:32:16, stevenjb (chromium) wrote: > OnCollectionChanged() Done. https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:110: const content::NotificationDetails& details) OVERRIDE { On 2012/11/29 03:32:16, stevenjb (chromium) wrote: > test that type == BALLOON_CONNECTED or BALLOON_DISCONNECTED Done. https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:192: return manager->balloon_collection()->GetActiveBalloons(); On 2012/11/29 03:32:16, stevenjb (chromium) wrote: > nit: No need for local manager variable Done. https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:197: return manager->balloon_collection()->GetActiveBalloons().size(); On 2012/11/29 03:32:16, stevenjb (chromium) wrote: > nit: No need for local manager variable Done. https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:204: bool success = manager->CancelById(notification.notification_id()); On 2012/11/29 03:32:16, stevenjb (chromium) wrote: > nit: No need for local manager variable Done. https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:256: ASSERT_TRUE(infobar->AsConfirmInfoBarDelegate()); On 2012/11/29 03:32:16, stevenjb (chromium) wrote: > Move below as ASSERT_TRUE(confirm_infobar) Done. https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:264: EXPECT_TRUE(false); On 2012/11/29 03:32:16, stevenjb (chromium) wrote: > This seems weird, I'd ask about a better way to do this. Maybe just: > EXPECT_TRUE(buttons & ConfirmInfoBarDelegate::BUTTON_OK); > before the if() Removed the if-else block, just check the OK button shows up. https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:271: EXPECT_TRUE(false); On 2012/11/29 03:32:16, stevenjb (chromium) wrote: > same here Removed the if-else block, just check the Cancel button shows up. https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:275: origin.c_str()); On 2012/11/29 03:32:16, stevenjb (chromium) wrote: > Is this translation dependent? Not sure how we do this normally, but seems > fragile, e.g. if the text is changed? After discussion with Ken, decided to remove the validation of all texts here. https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:291: replace_id); On 2012/11/29 03:32:16, stevenjb (chromium) wrote: > nit: no need for separate lines for the args? Done. https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:397: it != settings->end();) { On 2012/11/29 03:32:16, stevenjb (chromium) wrote: > nit: ' ' between ; and ) Done. https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:402: } On 2012/11/29 03:32:16, stevenjb (chromium) wrote: > nit: {} unnecessary for single line ifs. Done. https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:415: } On 2012/11/29 03:32:16, stevenjb (chromium) wrote: > nit: no {} Done. https://codereview.chromium.org/11359174/diff/16015/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:805: "chat"); On 2012/11/29 03:32:16, stevenjb (chromium) wrote: > nit: one or two lines for args, here and below Done.
lgtm https://codereview.chromium.org/11359174/diff/19005/chrome/browser/notificati... File chrome/browser/notifications/notification_browsertest.cc (right): https://codereview.chromium.org/11359174/diff/19005/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:260: (buttons & ConfirmInfoBarDelegate::BUTTON_CANCEL)); Should be two separate EXPECT statements so that if either fails we know immediately which.
https://codereview.chromium.org/11359174/diff/19005/chrome/browser/notificati... File chrome/browser/notifications/notification_browsertest.cc (right): https://codereview.chromium.org/11359174/diff/19005/chrome/browser/notificati... chrome/browser/notifications/notification_browsertest.cc:260: (buttons & ConfirmInfoBarDelegate::BUTTON_CANCEL)); On 2012/11/29 23:50:01, stevenjb (chromium) wrote: > Should be two separate EXPECT statements so that if either fails we know > immediately which. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chrisgao@chromium.org/11359174/26004
Presubmit check for 11359174-26004 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome/test/functional
Hi Ken, I have got clean try job. But I need your LGTM for change on directory chrome/test/functional. Best wishes, Shuotao Gao
chrome/test/functional LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chrisgao@chromium.org/11359174/26004
Message was sent while issue was closed.
Change committed as 170623 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
