Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9625020:
Ported NTP4 UI tests to WebUI. (Closed)
|
Created:
8 years, 9 months ago by Danh Nguyen Modified:
8 years, 9 months ago CC:
chromium-reviews, dbeam+watch-ntp_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionPorted NTP4 UI tests to WebUI.
This CL moves UI tests from the patch off issue 9420040 to WebUI testing framework.
TEST=browser_tests --gtest_filter=NTP4*WebUITest*
ui_tests --gtest_filter=NewTabUI*
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=126965
Patch Set 1 #
Total comments: 46
Patch Set 2 : Updated per review comments from scr and dbeam. #Patch Set 3 : Missed changes to the .gypi file when renaming test files. #
Total comments: 8
Patch Set 4 : More updates per review comments. #
Total comments: 18
Patch Set 5 : Some more updates per review comments. #
Total comments: 2
Patch Set 6 : Few last changes. #
Total comments: 4
Patch Set 7 : Fixed failures on ChromeOS. #Patch Set 8 : Fixed a typo. #Patch Set 9 : Excludes one irrelevant test for ChromeOS. #
Total comments: 4
Patch Set 10 : Tests that apps are not shown in ChromeOS. #
Total comments: 13
Patch Set 11 : Fixed nits and an assertion. #
Total comments: 2
Patch Set 12 : Fixed nits on assertion messages. #Patch Set 13 : Removed extra space in copyright header. #
Messages
Total messages: 33 (0 generated)
There are two tests involving actions(navigating to chrome://hang) which block the process. Thus they can't be run in WebUI. I think we'd better leave them as UI tests. Please take a quick look. Thanks, Danh
https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:2863: 'test/data/webui/ntp4_test.cc', s/test/browsertest/ these two lines. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... File chrome/test/data/webui/ntp4.js (right): https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:24: //This tests the ntp4 new tab page which is not used on touch builds. spacing seems off - should be 2 spaces; see javascript/c++ style guides for more info. Should be 1 space after // Come to think of it, though - why are there any changes to this function? https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:27: for (var i = 0; i < cardSlider.cardCount; ++i) { arv@chromium.org prefers i++ in JavaScript. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:52: 'There should be only one selected tile page.'); alignment is off. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:56: assertEquals(null, alignment is off - either move null down or indent next two lines to agree with null's position. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:62: * Test fixture for NTP4 WebUI testing with login. @extends {NTP4WebUITest} @constructor https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:69: // C++ class for login setup. JSDoc style comments for members/methods please. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:72: // Inserts C++ statement to set up login. JSDoc style comments. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:74: print(' SetLoginName("user@gmail.com");'); Please use GEN - print is v8, but node.js uses console.log (in case we ever want to switch implementations :) ) https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:78: GEN('#include "chrome/test/data/webui/ntp4_test.h"'); Move to the top please so it is closer to the desired position in C++ output file. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:81: userName = document.querySelector('#login-status-header .profile-name'); var https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:84: 'The user name should be present on the new tab.'); alignment is off. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... File chrome/test/data/webui/ntp4_test.h (right): https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4_test.h:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. 2012 https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4_test.h:21: void SetLoginName(const std::string& loginName); This can be protected: https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4_test.h:23: DISALLOW_COPY_AND_ASSIGN(NTP4LoggedInWebUITest); needs to be in private: section to do any good ;-)
https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... File chrome/test/data/webui/ntp4.js (right): https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:27: for (var i = 0; i < cardSlider.cardCount; ++i) { On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > mailto:arv@chromium.org prefers i++ in JavaScript. Then why'd you write it this way, scr@? :P http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/webui/ntp4.j... We do this other places, that's not a big deal. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:52: 'There should be only one selected tile page.'); On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > alignment is off. scr: you might want to explain how it's off https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:83: assertEquals('user@gmail.com', userName.firstChild.wholeText, what's wholeText? I've only ever heard of innerText or textContent https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... File chrome/test/data/webui/ntp4_test.cc (right): https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4_test.cc:5: #include "chrome/test/data/webui/ntp4_test.h" you'll want to include those .h files here with 1 line of space above them (and ntp4_test.h). https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4_test.cc:7: NTP4LoggedInWebUITest::NTP4LoggedInWebUITest() {} I'm not sure if the end curlie (}) should be on next line or not, though I prefer this (it's relatively easy to miss this and assume it says NTP4LoggedInWebUITest::NTP4LoggedInWebUITest();, so just personal preference...). https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... File chrome/test/data/webui/ntp4_test.h (right): https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4_test.h:13: #include "chrome/common/pref_names.h" move all except chrome/browser/ui/webui/web_ui_browsertest.h to .cc we also forward classname when possible (i.e. if you don't really need to know much about something other than it's a parameter to a method, you can just say class ClassName; and include the header for it in the .cc). https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4_test.h:23: DISALLOW_COPY_AND_ASSIGN(NTP4LoggedInWebUITest); On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > needs to be in private: section to do any good ;-) to add to what scr@ said, this macro creates copy constructors and it needs to be in the private section so one can't implicitly make copies via = (and possibly leak memory and waste resources).
https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... File chrome/test/data/webui/ntp4.js (right): https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:52: 'There should be only one selected tile page.'); Sho 'nuff. Please either start a new line after the parenthesis indented 4 spaces -or- align new lines with the first parameter. i.e. either assertEquals( 1, selectedTilePage.length, 'There should be only one selected tile page.'); -or- assertEquals(1, selectedTilePage.length, 'There should be only one selected tile page.'); I like the latter, personally, if it fits in 80 chars. On 2012/03/07 22:49:45, Dan Beam wrote: > On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > > alignment is off. > > scr: you might want to explain how it's off https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:84: 'The user name should be present on the new tab.'); On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > alignment is off. (See previous alignment comments for style suggestions)
I've renamed the _test_*.h,cc files to _browsertest_*.h.cc as Sheridan suggested. Please take another look. Thanks, Danh https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/chrome_tests.gyp... chrome/chrome_tests.gypi:2863: 'test/data/webui/ntp4_test.cc', On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > s/test/browsertest/ these two lines. Done. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... File chrome/test/data/webui/ntp4.js (right): https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:24: //This tests the ntp4 new tab page which is not used on touch builds. On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > spacing seems off - should be 2 spaces; see javascript/c++ style guides for more > info. Should be 1 space after // > > Come to think of it, though - why are there any changes to this function? I'm not sure how that happened. I must have messed it up somehow. Fixed. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:27: for (var i = 0; i < cardSlider.cardCount; ++i) { On 2012/03/07 22:49:45, Dan Beam wrote: > On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > > mailto:arv@chromium.org prefers i++ in JavaScript. > > Then why'd you write it this way, scr@? :P > http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/webui/ntp4.j... > > We do this other places, that's not a big deal. Easy fix. :-) Done. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:52: 'There should be only one selected tile page.'); On 2012/03/07 23:00:38, Sheridan Rawlins wrote: > Sho 'nuff. > > Please either start a new line after the parenthesis indented 4 spaces -or- > align new lines with the first parameter. > > i.e. either > assertEquals( > 1, selectedTilePage.length, > 'There should be only one selected tile page.'); > > -or- > > assertEquals(1, selectedTilePage.length, > 'There should be only one selected tile page.'); > > I like the latter, personally, if it fits in 80 chars. > > On 2012/03/07 22:49:45, Dan Beam wrote: > > On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > > > alignment is off. > > > > scr: you might want to explain how it's off > Done. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:56: assertEquals(null, On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > alignment is off - either move null down or indent next two lines to agree with > null's position. Done. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:62: * Test fixture for NTP4 WebUI testing with login. On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > @extends {NTP4WebUITest} > @constructor Done. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:69: // C++ class for login setup. On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > JSDoc style comments for members/methods please. Done. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:72: // Inserts C++ statement to set up login. On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > JSDoc style comments. Done. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:74: print(' SetLoginName("user@gmail.com");'); On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > Please use GEN - print is v8, but node.js uses console.log (in case we ever want > to switch implementations :) ) Done. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:78: GEN('#include "chrome/test/data/webui/ntp4_test.h"'); On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > Move to the top please so it is closer to the desired position in C++ output > file. Done. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:81: userName = document.querySelector('#login-status-header .profile-name'); On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > var Done. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:83: assertEquals('user@gmail.com', userName.firstChild.wholeText, On 2012/03/07 22:49:45, Dan Beam wrote: > what's wholeText? I've only ever heard of innerText or textContent https://developer.mozilla.org/en/DOM/Text.wholeText I changed it to use textContent now. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:84: 'The user name should be present on the new tab.'); On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > alignment is off. Done. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:84: 'The user name should be present on the new tab.'); On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > alignment is off. Done. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4.js:84: 'The user name should be present on the new tab.'); On 2012/03/07 23:00:38, Sheridan Rawlins wrote: > On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > > alignment is off. > > (See previous alignment comments for style suggestions) Done. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... File chrome/test/data/webui/ntp4_test.cc (right): https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4_test.cc:5: #include "chrome/test/data/webui/ntp4_test.h" On 2012/03/07 22:49:45, Dan Beam wrote: > you'll want to include those .h files here with 1 line of space above them (and > ntp4_test.h). Done. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4_test.cc:7: NTP4LoggedInWebUITest::NTP4LoggedInWebUITest() {} On 2012/03/07 22:49:45, Dan Beam wrote: > I'm not sure if the end curlie (}) should be on next line or not, though I > prefer this (it's relatively easy to miss this and assume it says > NTP4LoggedInWebUITest::NTP4LoggedInWebUITest();, so just personal > preference...). Cool. I'll keep it this way then. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... File chrome/test/data/webui/ntp4_test.h (right): https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4_test.h:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > 2012 Done. Thanks. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4_test.h:13: #include "chrome/common/pref_names.h" On 2012/03/07 22:49:45, Dan Beam wrote: > move all except chrome/browser/ui/webui/web_ui_browsertest.h to .cc > > we also forward classname when possible (i.e. if you don't really need to know > much about something other than it's a parameter to a method, you can just say > class ClassName; and include the header for it in the .cc). Done. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4_test.h:21: void SetLoginName(const std::string& loginName); On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > This can be protected: Yes. Thanks. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4_test.h:23: DISALLOW_COPY_AND_ASSIGN(NTP4LoggedInWebUITest); On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > needs to be in private: section to do any good ;-) Doh! I should have checked the macro. I thought it was included. https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... chrome/test/data/webui/ntp4_test.h:23: DISALLOW_COPY_AND_ASSIGN(NTP4LoggedInWebUITest); On 2012/03/07 22:49:45, Dan Beam wrote: > On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > > needs to be in private: section to do any good ;-) > > to add to what scr@ said, this macro creates copy constructors and it needs to > be in the private section so one can't implicitly make copies via = (and > possibly leak memory and waste resources). Thanks, Dan.
Danh, I don't see try jobs. Can you run git try From within your src dir? Thanks, -SCR On 2012/03/08 16:44:18, Danh Nguyen wrote: > I've renamed the _test_*.h,cc files to _browsertest_*.h.cc as Sheridan > suggested. Please take another look. > > Thanks, > Danh > > https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/chrome_tests.gypi > File chrome/chrome_tests.gypi (right): > > https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/chrome_tests.gyp... > chrome/chrome_tests.gypi:2863: 'test/data/webui/ntp4_test.cc', > On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > > s/test/browsertest/ these two lines. > > Done. > > https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... > File chrome/test/data/webui/ntp4.js (right): > > https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... > chrome/test/data/webui/ntp4.js:24: //This tests the ntp4 new tab page which is > not used on touch builds. > On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > > spacing seems off - should be 2 spaces; see javascript/c++ style guides for > more > > info. Should be 1 space after // > > > > Come to think of it, though - why are there any changes to this function? > > I'm not sure how that happened. I must have messed it up somehow. Fixed. > > https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... > chrome/test/data/webui/ntp4.js:27: for (var i = 0; i < cardSlider.cardCount; > ++i) { > On 2012/03/07 22:49:45, Dan Beam wrote: > > On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > > > mailto:arv@chromium.org prefers i++ in JavaScript. > > > > Then why'd you write it this way, scr@? :P > > > http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/webui/ntp4.j... > > > > We do this other places, that's not a big deal. > > Easy fix. :-) Done. > > https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... > chrome/test/data/webui/ntp4.js:52: 'There should be only one selected tile > page.'); > On 2012/03/07 23:00:38, Sheridan Rawlins wrote: > > Sho 'nuff. > > > > Please either start a new line after the parenthesis indented 4 spaces -or- > > align new lines with the first parameter. > > > > i.e. either > > assertEquals( > > 1, selectedTilePage.length, > > 'There should be only one selected tile page.'); > > > > -or- > > > > assertEquals(1, selectedTilePage.length, > > 'There should be only one selected tile page.'); > > > > I like the latter, personally, if it fits in 80 chars. > > > > On 2012/03/07 22:49:45, Dan Beam wrote: > > > On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > > > > alignment is off. > > > > > > scr: you might want to explain how it's off > > > > Done. > > https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... > chrome/test/data/webui/ntp4.js:56: assertEquals(null, > On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > > alignment is off - either move null down or indent next two lines to agree > with > > null's position. > > Done. > > https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... > chrome/test/data/webui/ntp4.js:62: * Test fixture for NTP4 WebUI testing with > login. > On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > > @extends {NTP4WebUITest} > > @constructor > > Done. > > https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... > chrome/test/data/webui/ntp4.js:69: // C++ class for login setup. > On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > > JSDoc style comments for members/methods please. > > Done. > > https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... > chrome/test/data/webui/ntp4.js:72: // Inserts C++ statement to set up login. > On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > > JSDoc style comments. > > Done. > > https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... > chrome/test/data/webui/ntp4.js:74: print(' SetLoginName("user@gmail.com");'); > On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > > Please use GEN - print is v8, but node.js uses console.log (in case we ever > want > > to switch implementations :) ) > > Done. > > https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... > chrome/test/data/webui/ntp4.js:78: GEN('#include > "chrome/test/data/webui/ntp4_test.h"'); > On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > > Move to the top please so it is closer to the desired position in C++ output > > file. > > Done. > > https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... > chrome/test/data/webui/ntp4.js:81: userName = > document.querySelector('#login-status-header .profile-name'); > On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > > var > > Done. > > https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... > chrome/test/data/webui/ntp4.js:83: assertEquals('user@gmail.com', > userName.firstChild.wholeText, > On 2012/03/07 22:49:45, Dan Beam wrote: > > what's wholeText? I've only ever heard of innerText or textContent > > https://developer.mozilla.org/en/DOM/Text.wholeText > > I changed it to use textContent now. > > https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... > chrome/test/data/webui/ntp4.js:84: 'The user name should be present on the new > tab.'); > On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > > alignment is off. > > Done. > > https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... > chrome/test/data/webui/ntp4.js:84: 'The user name should be present on the new > tab.'); > On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > > alignment is off. > > Done. > > https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... > chrome/test/data/webui/ntp4.js:84: 'The user name should be present on the new > tab.'); > On 2012/03/07 23:00:38, Sheridan Rawlins wrote: > > On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > > > alignment is off. > > > > (See previous alignment comments for style suggestions) > > Done. > > https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... > File chrome/test/data/webui/ntp4_test.cc (right): > > https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... > chrome/test/data/webui/ntp4_test.cc:5: #include > "chrome/test/data/webui/ntp4_test.h" > On 2012/03/07 22:49:45, Dan Beam wrote: > > you'll want to include those .h files here with 1 line of space above them > (and > > ntp4_test.h). > > Done. > > https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... > chrome/test/data/webui/ntp4_test.cc:7: > NTP4LoggedInWebUITest::NTP4LoggedInWebUITest() {} > On 2012/03/07 22:49:45, Dan Beam wrote: > > I'm not sure if the end curlie (}) should be on next line or not, though I > > prefer this (it's relatively easy to miss this and assume it says > > NTP4LoggedInWebUITest::NTP4LoggedInWebUITest();, so just personal > > preference...). > > Cool. I'll keep it this way then. > > https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... > File chrome/test/data/webui/ntp4_test.h (right): > > https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... > chrome/test/data/webui/ntp4_test.h:1: // Copyright (c) 2011 The Chromium > Authors. All rights reserved. > On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > > 2012 > > Done. Thanks. > > https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... > chrome/test/data/webui/ntp4_test.h:13: #include "chrome/common/pref_names.h" > On 2012/03/07 22:49:45, Dan Beam wrote: > > move all except chrome/browser/ui/webui/web_ui_browsertest.h to .cc > > > > we also forward classname when possible (i.e. if you don't really need to know > > much about something other than it's a parameter to a method, you can just say > > class ClassName; and include the header for it in the .cc). > > Done. > > https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... > chrome/test/data/webui/ntp4_test.h:21: void SetLoginName(const std::string& > loginName); > On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > > This can be protected: > > Yes. Thanks. > > https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... > chrome/test/data/webui/ntp4_test.h:23: > DISALLOW_COPY_AND_ASSIGN(NTP4LoggedInWebUITest); > On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > > needs to be in private: section to do any good ;-) > > Doh! I should have checked the macro. I thought it was included. > > https://chromiumcodereview.appspot.com/9625020/diff/1/chrome/test/data/webui/... > chrome/test/data/webui/ntp4_test.h:23: > DISALLOW_COPY_AND_ASSIGN(NTP4LoggedInWebUITest); > On 2012/03/07 22:49:45, Dan Beam wrote: > > On 2012/03/07 22:25:36, Sheridan Rawlins wrote: > > > needs to be in private: section to do any good ;-) > > > > to add to what scr@ said, this macro creates copy constructors and it needs to > > be in the private section so one can't implicitly make copies via = (and > > possibly leak memory and waste resources). > > Thanks, Dan.
I think there are still possibly issues with your indentation, but they're hard to explain over review / without seeing more of our code... https://chromiumcodereview.appspot.com/9625020/diff/11001/chrome/test/data/we... File chrome/test/data/webui/ntp4.js (right): https://chromiumcodereview.appspot.com/9625020/diff/11001/chrome/test/data/we... chrome/test/data/webui/ntp4.js:56: }); btw, these do work, but I was doing this rather than checking the text of the username when I rewrote because there was a regression (which is fixed now), you might be better off just checking that the text of the username is what you'd expect after shoving the username into the testing profile (i.e. pretend danh@google.com is signed in and check that document.querySelector('#login-status-header .profile-name').textContent == 'danh@google.com') or something) https://chromiumcodereview.appspot.com/9625020/diff/11001/chrome/test/data/we... chrome/test/data/webui/ntp4.js:74: /* If these are jsDoc comments, you need /** instead of /* https://chromiumcodereview.appspot.com/9625020/diff/11001/chrome/test/data/we... chrome/test/data/webui/ntp4.js:84: } nit: personally I prefer , at the end of object literals, as it makes it easier to copy/pasta without silly missing , errors https://chromiumcodereview.appspot.com/9625020/diff/11001/chrome/test/data/we... File chrome/test/data/webui/ntp4_browsertest.h (right): https://chromiumcodereview.appspot.com/9625020/diff/11001/chrome/test/data/we... chrome/test/data/webui/ntp4_browsertest.h:17: // Sets the user name into the profile as if the user had logged in. in the profile? (vs. into)
Hi Dan, I re-fixed the indentation and other things. Please take a quick look again. Thanks, Danh https://chromiumcodereview.appspot.com/9625020/diff/11001/chrome/test/data/we... File chrome/test/data/webui/ntp4.js (right): https://chromiumcodereview.appspot.com/9625020/diff/11001/chrome/test/data/we... chrome/test/data/webui/ntp4.js:56: }); On 2012/03/08 17:58:33, Dan Beam wrote: > btw, these do work, but I was doing this rather than checking the text of the > username when I rewrote because there was a regression (which is fixed now), you > might be better off just checking that the text of the username is what you'd > expect after shoving the username into the testing profile (i.e. pretend > mailto:danh@google.com is signed in and check that > document.querySelector('#login-status-header .profile-name').textContent == > 'danh@google.com') or something) This is the logged out test. The logged in test is at the bottom of this file. I did check the displayed user name against the expected value there. https://chromiumcodereview.appspot.com/9625020/diff/11001/chrome/test/data/we... chrome/test/data/webui/ntp4.js:74: /* On 2012/03/08 17:58:33, Dan Beam wrote: > If these are jsDoc comments, you need /** instead of /* Done. https://chromiumcodereview.appspot.com/9625020/diff/11001/chrome/test/data/we... chrome/test/data/webui/ntp4.js:84: } On 2012/03/08 17:58:33, Dan Beam wrote: > nit: personally I prefer , at the end of object literals, as it makes it easier > to copy/pasta without silly missing , errors Done. https://chromiumcodereview.appspot.com/9625020/diff/11001/chrome/test/data/we... File chrome/test/data/webui/ntp4_browsertest.h (right): https://chromiumcodereview.appspot.com/9625020/diff/11001/chrome/test/data/we... chrome/test/data/webui/ntp4_browsertest.h:17: // Sets the user name into the profile as if the user had logged in. On 2012/03/08 17:58:33, Dan Beam wrote: > in the profile? (vs. into) Done. Thanks Dan.
http://codereview.chromium.org/9625020/diff/15001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/9625020/diff/15001/chrome/chrome_tests.gypi#ne... chrome/chrome_tests.gypi:2863: 'test/data/webui/ntp4_browsertest.cc', Please sort alphabetically; maybe fix some of the others too while you're at it (such as print_preview.h/cc). http://codereview.chromium.org/9625020/diff/15001/chrome/test/data/webui/ntp4.js File chrome/test/data/webui/ntp4.js (right): http://codereview.chromium.org/9625020/diff/15001/chrome/test/data/webui/ntp4... chrome/test/data/webui/ntp4.js:18: * Browse to the newtab page & call preLoad(). /** @override */ would say it all and be easier to maintain. http://codereview.chromium.org/9625020/diff/15001/chrome/test/data/webui/ntp4... chrome/test/data/webui/ntp4.js:75: * Specifies C++ class for login setup. nit: candidate for /** @override */ http://codereview.chromium.org/9625020/diff/15001/chrome/test/data/webui/ntp4... chrome/test/data/webui/ntp4.js:80: * Inserts C++ statement to set up login. nit: candidate for /** @override */ http://codereview.chromium.org/9625020/diff/15001/chrome/test/data/webui/ntp4... chrome/test/data/webui/ntp4.js:92: 'The user name should be present on the new tab.'); Would this fit in 2 lines without going over the 80 chars? If so, while what you have is stylistically correct, less lines is preferable. assertEquals('user@gmail.com', userName.textContent, 'The user name should be present on the new tab.'); http://codereview.chromium.org/9625020/diff/15001/chrome/test/data/webui/ntp4... File chrome/test/data/webui/ntp4_browsertest.cc (right): http://codereview.chromium.org/9625020/diff/15001/chrome/test/data/webui/ntp4... chrome/test/data/webui/ntp4_browsertest.cc:16: void NTP4LoggedInWebUITest::SetLoginName(const std::string& name) { This should agree with header: s/name/login_name/ http://codereview.chromium.org/9625020/diff/15001/chrome/test/data/webui/ntp4... File chrome/test/data/webui/ntp4_browsertest.h (right): http://codereview.chromium.org/9625020/diff/15001/chrome/test/data/webui/ntp4... chrome/test/data/webui/ntp4_browsertest.h:18: void SetLoginName(const std::string& loginName); C++ style is to use underscores to separate variable words. s/loginName/login_name/
https://chromiumcodereview.appspot.com/9625020/diff/15001/chrome/chrome_tests... File chrome/chrome_tests.gypi (right): https://chromiumcodereview.appspot.com/9625020/diff/15001/chrome/chrome_tests... chrome/chrome_tests.gypi:2863: 'test/data/webui/ntp4_browsertest.cc', On 2012/03/08 19:10:30, Sheridan Rawlins wrote: > Please sort alphabetically; maybe fix some of the others too while you're at it > (such as print_preview.h/cc). in vim for this file: :2842,2867|LC_ALL=C sort https://chromiumcodereview.appspot.com/9625020/diff/15001/chrome/test/data/we... File chrome/test/data/webui/ntp4.js (right): https://chromiumcodereview.appspot.com/9625020/diff/15001/chrome/test/data/we... chrome/test/data/webui/ntp4.js:60: null, document.querySelector('#login-status-header .profile-name'), to make the indent work here just make a var like you did later var profileName = document.querySelector( '#login-status-header .profile-name'); or var profileName = document.querySelector('#login-status-header .profile-name'); to make wrap work, then assertEquals(null, profileName, 'Login name shouldn\'t exist when signed out.'); https://chromiumcodereview.appspot.com/9625020/diff/15001/chrome/test/data/we... chrome/test/data/webui/ntp4.js:92: 'The user name should be present on the new tab.'); On 2012/03/08 19:10:30, Sheridan Rawlins wrote: > Would this fit in 2 lines without going over the 80 chars? If so, while what > you have is stylistically correct, less lines is preferable. > > assertEquals('user@gmail.com', userName.textContent, > 'The user name should be present on the new tab.'); try to follow scr's example everywhere in this case as you're allowed to group related parameters to a function on the same line, so in this case the message is slightly less related, so it can be on the next line.
I hope I didn't miss anything this time. Please take quick look again. Thanks, Danh https://chromiumcodereview.appspot.com/9625020/diff/15001/chrome/chrome_tests... File chrome/chrome_tests.gypi (right): https://chromiumcodereview.appspot.com/9625020/diff/15001/chrome/chrome_tests... chrome/chrome_tests.gypi:2863: 'test/data/webui/ntp4_browsertest.cc', On 2012/03/08 19:23:18, Dan Beam wrote: > On 2012/03/08 19:10:30, Sheridan Rawlins wrote: > > Please sort alphabetically; maybe fix some of the others too while you're at > it > > (such as print_preview.h/cc). > > in vim for this file: > :2842,2867|LC_ALL=C sort Done. Thanks Dan for the tips. https://chromiumcodereview.appspot.com/9625020/diff/15001/chrome/test/data/we... File chrome/test/data/webui/ntp4.js (right): https://chromiumcodereview.appspot.com/9625020/diff/15001/chrome/test/data/we... chrome/test/data/webui/ntp4.js:18: * Browse to the newtab page & call preLoad(). On 2012/03/08 19:10:30, Sheridan Rawlins wrote: > /** @override */ would say it all and be easier to maintain. Done. https://chromiumcodereview.appspot.com/9625020/diff/15001/chrome/test/data/we... chrome/test/data/webui/ntp4.js:60: null, document.querySelector('#login-status-header .profile-name'), On 2012/03/08 19:23:18, Dan Beam wrote: > to make the indent work here just make a var like you did later > > var profileName = document.querySelector( > '#login-status-header .profile-name'); > > or > > var profileName = > document.querySelector('#login-status-header .profile-name'); > > to make wrap work, then > > assertEquals(null, profileName, > 'Login name shouldn\'t exist when signed out.'); Done. https://chromiumcodereview.appspot.com/9625020/diff/15001/chrome/test/data/we... chrome/test/data/webui/ntp4.js:75: * Specifies C++ class for login setup. On 2012/03/08 19:10:30, Sheridan Rawlins wrote: > nit: candidate for /** @override */ Done. https://chromiumcodereview.appspot.com/9625020/diff/15001/chrome/test/data/we... chrome/test/data/webui/ntp4.js:80: * Inserts C++ statement to set up login. On 2012/03/08 19:10:30, Sheridan Rawlins wrote: > nit: candidate for /** @override */ Done. https://chromiumcodereview.appspot.com/9625020/diff/15001/chrome/test/data/we... chrome/test/data/webui/ntp4.js:92: 'The user name should be present on the new tab.'); On 2012/03/08 19:10:30, Sheridan Rawlins wrote: > Would this fit in 2 lines without going over the 80 chars? If so, while what > you have is stylistically correct, less lines is preferable. > > assertEquals('user@gmail.com', userName.textContent, > 'The user name should be present on the new tab.'); Done. https://chromiumcodereview.appspot.com/9625020/diff/15001/chrome/test/data/we... File chrome/test/data/webui/ntp4_browsertest.cc (right): https://chromiumcodereview.appspot.com/9625020/diff/15001/chrome/test/data/we... chrome/test/data/webui/ntp4_browsertest.cc:16: void NTP4LoggedInWebUITest::SetLoginName(const std::string& name) { On 2012/03/08 19:10:30, Sheridan Rawlins wrote: > This should agree with header: s/name/login_name/ Done. https://chromiumcodereview.appspot.com/9625020/diff/15001/chrome/test/data/we... File chrome/test/data/webui/ntp4_browsertest.h (right): https://chromiumcodereview.appspot.com/9625020/diff/15001/chrome/test/data/we... chrome/test/data/webui/ntp4_browsertest.h:18: void SetLoginName(const std::string& loginName); On 2012/03/08 19:10:30, Sheridan Rawlins wrote: > C++ style is to use underscores to separate variable words. > > s/loginName/login_name/ Done.
lgtm, wait for scr@'s LG though (I'm also not an OWNER of this folder if there are any)
LGTM with nits. https://chromiumcodereview.appspot.com/9625020/diff/19002/chrome/test/data/we... File chrome/test/data/webui/ntp4.js (right): https://chromiumcodereview.appspot.com/9625020/diff/19002/chrome/test/data/we... chrome/test/data/webui/ntp4.js:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. Nit: 2012 https://chromiumcodereview.appspot.com/9625020/diff/19002/chrome/test/data/we... chrome/test/data/webui/ntp4.js:74: testGenPreamble: function() { Nit: only 1 space between ': function' please.
Hi Evan, Scott, I need your owner's blessing to check in this CL. Could you take a quick look whenever you have a chance? Thanks, Danh
On 2012/03/08 22:05:38, Danh Nguyen wrote: > Hi Evan, Scott, > > I need your owner's blessing to check in this CL. Could you take a quick look > whenever you have a chance? > > Thanks, > Danh FYI, Scott as OWNER for test dir; Evan for webui.
chrome/test/data/OWNERS is *, so you don't need me.
http://codereview.chromium.org/9625020/diff/22003/chrome/test/data/webui/ntp4.js File chrome/test/data/webui/ntp4.js (right): http://codereview.chromium.org/9625020/diff/22003/chrome/test/data/webui/ntp4... chrome/test/data/webui/ntp4.js:43: assertGE(navDots.length, 2, 'There should be at least two navdots.'); this will fail on chromeos (there are no apps in the NTP with chromeos). You can check for this scenario with templateData.shouldShowApps.
make that templateData.showAppsPage (soon to be changed to templateData.showApps)
http://codereview.chromium.org/9625020/diff/22003/chrome/test/data/webui/ntp4.js File chrome/test/data/webui/ntp4.js (right): http://codereview.chromium.org/9625020/diff/22003/chrome/test/data/webui/ntp4... chrome/test/data/webui/ntp4.js:81: assertTrue(userName != null); assertNotEquals(null, userName)
H Evan, All the try bot results are now green with the latest patch. Could I have your approval for checking this in? Thanks, Danh http://codereview.chromium.org/9625020/diff/22003/chrome/test/data/webui/ntp4.js File chrome/test/data/webui/ntp4.js (right): http://codereview.chromium.org/9625020/diff/22003/chrome/test/data/webui/ntp4... chrome/test/data/webui/ntp4.js:43: assertGE(navDots.length, 2, 'There should be at least two navdots.'); On 2012/03/09 18:28:31, Evan Stade wrote: > this will fail on chromeos (there are no apps in the NTP with chromeos). You can > check for this scenario with templateData.shouldShowApps. Done. I've also fixed similar instance in the test 'NTPHasThumbNails'. Also excluded the test 'NTPHasLoginNameWhenSignedIn', which is irrelevant for Chrome OS. http://codereview.chromium.org/9625020/diff/22003/chrome/test/data/webui/ntp4... chrome/test/data/webui/ntp4.js:81: assertTrue(userName != null); On 2012/03/12 17:57:41, Sheridan Rawlins wrote: > assertNotEquals(null, userName) Done.
http://codereview.chromium.org/9625020/diff/32005/chrome/test/data/webui/ntp4.js File chrome/test/data/webui/ntp4.js (right): http://codereview.chromium.org/9625020/diff/32005/chrome/test/data/webui/ntp4... chrome/test/data/webui/ntp4.js:37: if (window.templateData.showAppsPage) { you should test the opposite of this case as well, i.e. var apps = document.querySelectorAll('.app'); if (window.templateData.showAppsPage) assertGE(apps.length, 1, 'There should be at least one app.'); else assertEquals(apps.length, 0, 'There should be no apps.'); or set up two test cases to flip this on and off in C++ (via mocking this method, http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/ui/webui/ntp...). http://codereview.chromium.org/9625020/diff/32005/chrome/test/data/webui/ntp4... chrome/test/data/webui/ntp4.js:44: if (window.templateData.showAppsPage) { and here, there will be no nav dots in "bare-minimum" mode
Thanks Dan for your suggestions. Tests were added. I also updated showAppsPage => showApps. Danh http://codereview.chromium.org/9625020/diff/32005/chrome/test/data/webui/ntp4.js File chrome/test/data/webui/ntp4.js (right): http://codereview.chromium.org/9625020/diff/32005/chrome/test/data/webui/ntp4... chrome/test/data/webui/ntp4.js:37: if (window.templateData.showAppsPage) { On 2012/03/14 03:48:13, Dan Beam wrote: > you should test the opposite of this case as well, i.e. > > var apps = document.querySelectorAll('.app'); > if (window.templateData.showAppsPage) > assertGE(apps.length, 1, 'There should be at least one app.'); > else > assertEquals(apps.length, 0, 'There should be no apps.'); > > or set up two test cases to flip this on and off in C++ (via mocking this > method, > http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/ui/webui/ntp...). Done. http://codereview.chromium.org/9625020/diff/32005/chrome/test/data/webui/ntp4... chrome/test/data/webui/ntp4.js:44: if (window.templateData.showAppsPage) { On 2012/03/14 03:48:13, Dan Beam wrote: > and here, there will be no nav dots in "bare-minimum" mode Done.
slgtm w/nits http://codereview.chromium.org/9625020/diff/39001/chrome/test/data/webui/ntp4.js File chrome/test/data/webui/ntp4.js (right): http://codereview.chromium.org/9625020/diff/39001/chrome/test/data/webui/ntp4... chrome/test/data/webui/ntp4.js:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. nit: we don't use 2 spaces, only 1 between sentences http://codereview.chromium.org/9625020/diff/39001/chrome/test/data/webui/ntp4... chrome/test/data/webui/ntp4.js:38: if (window.templateData.showApps) { nit: no curlies when both conditional and body of conditional (and all follow up conditionals) are 1 line. Ex: if (blah) blee(); else if (blar) blag(); else blug(); http://codereview.chromium.org/9625020/diff/39001/chrome/test/data/webui/ntp4... chrome/test/data/webui/ntp4.js:41: else { nit: if you do need curlies, we "cuddle" them, i.e. if (cond) { // stuff here } else if (cond) { // more here } else { // some stuff // down here } http://codereview.chromium.org/9625020/diff/39001/chrome/test/data/webui/ntp4... chrome/test/data/webui/ntp4.js:42: assertGE(apps.length, 0, 'There should be no app.'); nit: no apps. http://codereview.chromium.org/9625020/diff/39001/chrome/test/data/webui/ntp4... chrome/test/data/webui/ntp4.js:52: assertGE(navDots.length, 0, 'There should be no navdot.'); nit: no navdots.
http://codereview.chromium.org/9625020/diff/39001/chrome/test/data/webui/ntp4.js File chrome/test/data/webui/ntp4.js (right): http://codereview.chromium.org/9625020/diff/39001/chrome/test/data/webui/ntp4... chrome/test/data/webui/ntp4.js:52: assertGE(navDots.length, 0, 'There should be no navdot.'); On 2012/03/15 00:12:55, Dan Beam wrote: > nit: no navdots. whoops, sorry, this should be assertEquals()
http://codereview.chromium.org/9625020/diff/39001/chrome/test/data/webui/ntp4.js File chrome/test/data/webui/ntp4.js (right): http://codereview.chromium.org/9625020/diff/39001/chrome/test/data/webui/ntp4... chrome/test/data/webui/ntp4.js:56: TEST_F('NTP4WebUITest', 'NTPHasSelectedPageAndDot', function() { and this should also be wrapped with showApps as navdots don't show, you can build chrome as ChromeOS with ./build/gyp_chromium -Dchromeos=1 # until you rerun this or gclient runhooks or export GYP_DEFINES="$GYP_DEFINES chromeos=1" && gclient runhooks and recompile and run tests
I've fixed all the nits and the assertion Dan pointed out. The results from the try bots(including linux_chromeos) are green now. I'm going to try the commit checkbox, but I suspect I still need Evan's blessing though. Thanks, Danh http://codereview.chromium.org/9625020/diff/39001/chrome/test/data/webui/ntp4.js File chrome/test/data/webui/ntp4.js (right): http://codereview.chromium.org/9625020/diff/39001/chrome/test/data/webui/ntp4... chrome/test/data/webui/ntp4.js:38: if (window.templateData.showApps) { On 2012/03/15 00:12:55, Dan Beam wrote: > nit: no curlies when both conditional and body of conditional (and all follow up > conditionals) are 1 line. Ex: > > if (blah) > blee(); > else if (blar) > blag(); > else > blug(); Done. http://codereview.chromium.org/9625020/diff/39001/chrome/test/data/webui/ntp4... chrome/test/data/webui/ntp4.js:41: else { On 2012/03/15 00:12:55, Dan Beam wrote: > nit: if you do need curlies, we "cuddle" them, i.e. > > if (cond) { > // stuff here > } else if (cond) { > // more here > } else { > // some stuff > // down here > } Done. http://codereview.chromium.org/9625020/diff/39001/chrome/test/data/webui/ntp4... chrome/test/data/webui/ntp4.js:42: assertGE(apps.length, 0, 'There should be no app.'); On 2012/03/15 00:12:55, Dan Beam wrote: > nit: no apps. Done. http://codereview.chromium.org/9625020/diff/39001/chrome/test/data/webui/ntp4... chrome/test/data/webui/ntp4.js:52: assertGE(navDots.length, 0, 'There should be no navdot.'); On 2012/03/15 00:12:55, Dan Beam wrote: > nit: no navdots. Fixed. http://codereview.chromium.org/9625020/diff/39001/chrome/test/data/webui/ntp4... chrome/test/data/webui/ntp4.js:52: assertGE(navDots.length, 0, 'There should be no navdot.'); On 2012/03/15 00:13:27, Dan Beam wrote: > On 2012/03/15 00:12:55, Dan Beam wrote: > > nit: no navdots. > > whoops, sorry, this should be assertEquals() Actually on ChromeOS in the browser tests' settings, there is only one navDot. So I changed this into: assertEquals(1, navDots.length, 'There should be only one navdot.') Thanks Dan for catching this. http://codereview.chromium.org/9625020/diff/39001/chrome/test/data/webui/ntp4... chrome/test/data/webui/ntp4.js:56: TEST_F('NTP4WebUITest', 'NTPHasSelectedPageAndDot', function() { On 2012/03/15 00:15:25, Dan Beam wrote: > and this should also be wrapped with showApps as navdots don't show, you can > build chrome as ChromeOS with > > ./build/gyp_chromium -Dchromeos=1 # until you rerun this or gclient runhooks > or > export GYP_DEFINES="$GYP_DEFINES chromeos=1" && gclient runhooks > and recompile and run tests On ChromeOS there is only one navdot and it's selected, although it's not visible. So this test still applies. Thanks for the tips of building Chrome as ChromeOS. I have been doing similar thing except that I have to add: { "name": "cros_deps", "url": http://src.chromium.org/svn/trunk/src/tools/cros.DEPS", }, into my .gclient file (before running gclient sync), and didn't run 'gclient runhooks'. Does 'gclient runhooks' do that for us?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/danhn@chromium.org/9625020/44001
Presubmit check for 9625020-44001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].
** Presubmit Warnings **
License must match:
.*? Copyright \(c\) 2012 The Chromium Authors\. All rights reserved\.\n.*? Use
of this source code is governed by a BSD-style license that can be\n.*? found in
the LICENSE file\.(?: \*/)?\n
Found a bad license header in these files:
chrome/test/data/webui/ntp4.js
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome/browser/ui/webui
Presubmit checks took 1.2s to calculate.
lgtm with nits http://codereview.chromium.org/9625020/diff/44001/chrome/test/data/webui/ntp4.js File chrome/test/data/webui/ntp4.js (right): http://codereview.chromium.org/9625020/diff/44001/chrome/test/data/webui/ntp4... chrome/test/data/webui/ntp4.js:49: assertEquals(1, navDots.length, 'There should be only one navdot.'); this is not quite true, there is an optional suggestions pane (currently behind a flag) which would add another dot. However this is perhaps good enough for now (can be revisited if the suggestions page ever becomes default). http://codereview.chromium.org/9625020/diff/44001/chrome/test/data/webui/ntp4... chrome/test/data/webui/ntp4.js:58: 'There should be only one selected tile page.'); this string doesn't make sense if the length is actually 0. I would change it to "... exactly one selected tile page." (same above)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/danhn@chromium.org/9625020/41004
Presubmit check for 9625020-41004 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** If this change has an associated bug, add BUG=[bug number]. ** Presubmit Warnings ** License must match: .*? Copyright \(c\) 2012 The Chromium Authors\. All rights reserved\.\n.*? Use of this source code is governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?: \*/)?\n Found a bad license header in these files: chrome/test/data/webui/ntp4.js Presubmit checks took 1.3s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/danhn@chromium.org/9625020/40009
Change committed as 126965 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
