Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12840003:
Implement local NTP for fallback. (Closed)
|
Created:
7 years, 9 months ago by jeremycho Modified:
7 years, 9 months ago CC:
chromium-reviews, melevin, sreeram, gideonwald, dominich, David Black, arv+watch_chromium.org, samarth+watch_chromium.org, Jered, Shishir Base URL:
https://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionImplement local NTP for fallback.
BUG=178775
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190589
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebase #Patch Set 3 : Rebase again #
Total comments: 4
Patch Set 4 : Respond to Samarth's comments. #
Total comments: 12
Patch Set 5 : Rebase following instant->search #Patch Set 6 : Respond to Samarth's comments. #
Total comments: 6
Patch Set 7 : Respond to Samarth's comments. #
Total comments: 56
Patch Set 8 : Respond to comments. #Patch Set 9 : Rebase #Patch Set 10 : Fix typo. #Patch Set 11 : Add placeholder title. #Patch Set 12 : Respond to comments #
Total comments: 45
Patch Set 13 : Respond. #
Total comments: 41
Patch Set 14 : Rebase. #Patch Set 15 : Respond to comments. #
Total comments: 14
Patch Set 16 : Respond to comments. #Patch Set 17 : Rebase. #Patch Set 18 : Fix compile. #Messages
Total messages: 37 (0 generated)
This is ready for review. Feel free to stop by for a demo.
On 2013/03/13 22:54:09, jeremycho wrote: > This is ready for review. Feel free to stop by for a demo. Friendly ping.
I am not too familiar with the js,css stuff and since I wrote the C++ part, it wouldn't be appropriate for me to review this.
David - perhaps you could do a quick look at the JS/CSS? It's mostly a simplified port of the search-side code and minus the GWS utility functions.
Just some drive-by comments. Thanks, Samarth https://codereview.chromium.org/12840003/diff/1/chrome/browser/instant/search.cc File chrome/browser/instant/search.cc (right): https://codereview.chromium.org/12840003/diff/1/chrome/browser/instant/search... chrome/browser/instant/search.cc:306: entry->GetVirtualURL() == GURL(chrome::kChromeSearchLocalNTPURL)) && Please add a unit test for the changes in this file. https://codereview.chromium.org/12840003/diff/8001/chrome/browser/resources/l... File chrome/browser/resources/local_ntp/local_ntp.css (right): https://codereview.chromium.org/12840003/diff/8001/chrome/browser/resources/l... chrome/browser/resources/local_ntp/local_ntp.css:99: background: transparent url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAQAAAC1+jfqAAAAiElEQVR42r2RsQrDMAxEBRdl8SDcX8lQPGg1GBI6lvz/h7QyRRXV0qUULwfvwZ1tenw5PxToRPWMC52eA9+WDnlh3HFQ/xBQl86NFYJqeGflkiogrOvVlIFhqURFVho3x1moGAa3deMs+LS30CAhBN5nNxeT5hbJ1zwmji2k+aF6NENIPf/hs54f0sZFUVAMigAAAABJRU5ErkJggg==); This will trip a CSS presubmit that tells you use a local file instead (see page_icon.png, etc in the local-omnibox-popup). You'll need to explicitly serve the files in local_ntp_source.cc. (I'm fixing this for the popup in https://codereview.chromium.org/12720004/.) https://codereview.chromium.org/12840003/diff/8001/chrome/browser/resources/l... File chrome/browser/resources/local_ntp/local_ntp.html (right): https://codereview.chromium.org/12840003/diff/8001/chrome/browser/resources/l... chrome/browser/resources/local_ntp/local_ntp.html:8: <script src="chrome-search://local-ntp/local-ntp.js"></script> Does just //local-ntp/... work?
https://codereview.chromium.org/12840003/diff/1/chrome/browser/instant/search.cc File chrome/browser/instant/search.cc (right): https://codereview.chromium.org/12840003/diff/1/chrome/browser/instant/search... chrome/browser/instant/search.cc:306: entry->GetVirtualURL() == GURL(chrome::kChromeSearchLocalNTPURL)) && On 2013/03/15 22:14:29, samarth wrote: > Please add a unit test for the changes in this file. Done. https://codereview.chromium.org/12840003/diff/8001/chrome/browser/resources/l... File chrome/browser/resources/local_ntp/local_ntp.css (right): https://codereview.chromium.org/12840003/diff/8001/chrome/browser/resources/l... chrome/browser/resources/local_ntp/local_ntp.css:99: background: transparent url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAQAAAC1+jfqAAAAiElEQVR42r2RsQrDMAxEBRdl8SDcX8lQPGg1GBI6lvz/h7QyRRXV0qUULwfvwZ1tenw5PxToRPWMC52eA9+WDnlh3HFQ/xBQl86NFYJqeGflkiogrOvVlIFhqURFVho3x1moGAa3deMs+LS30CAhBN5nNxeT5hbJ1zwmji2k+aF6NENIPf/hs54f0sZFUVAMigAAAABJRU5ErkJggg==); On 2013/03/15 22:14:30, samarth wrote: > This will trip a CSS presubmit that tells you use a local file instead (see > page_icon.png, etc in the local-omnibox-popup). You'll need to explicitly serve > the files in local_ntp_source.cc. (I'm fixing this for the popup in > https://codereview.chromium.org/12720004/.) Done. Thanks for your help in figuring this out! https://codereview.chromium.org/12840003/diff/8001/chrome/browser/resources/l... File chrome/browser/resources/local_ntp/local_ntp.html (right): https://codereview.chromium.org/12840003/diff/8001/chrome/browser/resources/l... chrome/browser/resources/local_ntp/local_ntp.html:8: <script src="chrome-search://local-ntp/local-ntp.js"></script> I get a failed to open exception. On 2013/03/15 22:14:30, samarth wrote: > Does just //local-ntp/... work?
Evan - can you review for chrome/browser/resources? Thanks.
Removing myself as reviewer. Samarth can handle this.
Mostly nits below. Can you also sync and rebase? I'd like to patch it in and give it a whirl. Thanks, Samarth https://codereview.chromium.org/12840003/diff/16002/chrome/browser/instant/lo... File chrome/browser/instant/local_ntp_source.cc (right): https://codereview.chromium.org/12840003/diff/16002/chrome/browser/instant/lo... chrome/browser/instant/local_ntp_source.cc:20: const char kJsFilename[] = "local-ntp.js"; nit: kJSFilename https://codereview.chromium.org/12840003/diff/16002/chrome/browser/instant/lo... File chrome/browser/instant/local_ntp_source.h (right): https://codereview.chromium.org/12840003/diff/16002/chrome/browser/instant/lo... chrome/browser/instant/local_ntp_source.h:12: // Serves HTML and resources for the local omnibox popup i.e. Please update comment. https://codereview.chromium.org/12840003/diff/16002/chrome/browser/instant/lo... chrome/browser/instant/local_ntp_source.h:18: // content::URLDataSource overrides. nit: the Chromium style is "Overriden from content::URLDataSource:" https://codereview.chromium.org/12840003/diff/16002/chrome/browser/instant/lo... chrome/browser/instant/local_ntp_source.h:18: // content::URLDataSource overrides. Also, can you make these private? https://codereview.chromium.org/12840003/diff/16002/chrome/browser/instant/se... File chrome/browser/instant/search_unittest.cc (right): https://codereview.chromium.org/12840003/diff/16002/chrome/browser/instant/se... chrome/browser/instant/search_unittest.cc:190: {chrome::kChromeSearchLocalNTPURL, true, "Local new tab page"}, super nit: keep the columns aligned https://codereview.chromium.org/12840003/diff/16002/chrome/browser/resources/... File chrome/browser/resources/local_ntp/local_ntp.css (right): https://codereview.chromium.org/12840003/diff/16002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:100: background-image: url('images/close_bar.png'); Do these images not have a 2x version for high-DPI displays?
Rebased too. Feel free to also stop by for a demo. https://codereview.chromium.org/12840003/diff/16002/chrome/browser/instant/lo... File chrome/browser/instant/local_ntp_source.cc (right): https://codereview.chromium.org/12840003/diff/16002/chrome/browser/instant/lo... chrome/browser/instant/local_ntp_source.cc:20: const char kJsFilename[] = "local-ntp.js"; On 2013/03/18 22:09:19, samarth wrote: > nit: kJSFilename Done. https://codereview.chromium.org/12840003/diff/16002/chrome/browser/instant/lo... File chrome/browser/instant/local_ntp_source.h (right): https://codereview.chromium.org/12840003/diff/16002/chrome/browser/instant/lo... chrome/browser/instant/local_ntp_source.h:12: // Serves HTML and resources for the local omnibox popup i.e. On 2013/03/18 22:09:19, samarth wrote: > Please update comment. Done. https://codereview.chromium.org/12840003/diff/16002/chrome/browser/instant/lo... chrome/browser/instant/local_ntp_source.h:18: // content::URLDataSource overrides. On 2013/03/18 22:09:19, samarth wrote: > nit: the Chromium style is "Overriden from content::URLDataSource:" Done. https://codereview.chromium.org/12840003/diff/16002/chrome/browser/instant/lo... chrome/browser/instant/local_ntp_source.h:18: // content::URLDataSource overrides. On 2013/03/18 22:09:19, samarth wrote: > Also, can you make these private? Done. Also updated local_omnibox_popup. https://codereview.chromium.org/12840003/diff/16002/chrome/browser/instant/se... File chrome/browser/instant/search_unittest.cc (right): https://codereview.chromium.org/12840003/diff/16002/chrome/browser/instant/se... chrome/browser/instant/search_unittest.cc:190: {chrome::kChromeSearchLocalNTPURL, true, "Local new tab page"}, On 2013/03/18 22:09:19, samarth wrote: > super nit: keep the columns aligned Done. https://codereview.chromium.org/12840003/diff/16002/chrome/browser/resources/... File chrome/browser/resources/local_ntp/local_ntp.css (right): https://codereview.chromium.org/12840003/diff/16002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:100: background-image: url('images/close_bar.png'); On 2013/03/18 22:09:19, samarth wrote: > Do these images not have a 2x version for high-DPI displays? They do, but the close bar IDs select the 1px version. I'll look into this some more, but it seems we'd have to make local copies (like in local-omnibox-popup) if we want 2x.
lgtm for c/b/search/ with some comments. I think you'll need someone from chrome/OWNERS for the grd change. You could try sk@. Samarth https://codereview.chromium.org/12840003/diff/38001/chrome/browser/search/loc... File chrome/browser/search/local_ntp_source.h (right): https://codereview.chromium.org/12840003/diff/38001/chrome/browser/search/loc... chrome/browser/search/local_ntp_source.h:21: // Overriden from content::URLDataSource: nit: Overridden https://codereview.chromium.org/12840003/diff/38001/chrome/browser/search/loc... File chrome/browser/search/local_omnibox_popup_source.h (right): https://codereview.chromium.org/12840003/diff/38001/chrome/browser/search/loc... chrome/browser/search/local_omnibox_popup_source.h:21: // Overriden from content::URLDataSource: nit: Overridden https://codereview.chromium.org/12840003/diff/38001/chrome/browser/search/sea... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/12840003/diff/38001/chrome/browser/search/sea... chrome/browser/search/search.cc:324: (url == GURL(chrome::kChromeSearchLocalNTPURL) || Actually these two checks are now redundant given the check for kChromeSearchScheme above. You can just remove then (and make sure the tests still pass).
Scott - can you review for browser_resources.grd?
Thanks for reviewing. https://codereview.chromium.org/12840003/diff/38001/chrome/browser/search/loc... File chrome/browser/search/local_ntp_source.h (right): https://codereview.chromium.org/12840003/diff/38001/chrome/browser/search/loc... chrome/browser/search/local_ntp_source.h:21: // Overriden from content::URLDataSource: On 2013/03/19 02:35:53, samarth wrote: > nit: Overridden Done. https://codereview.chromium.org/12840003/diff/38001/chrome/browser/search/loc... File chrome/browser/search/local_omnibox_popup_source.h (right): https://codereview.chromium.org/12840003/diff/38001/chrome/browser/search/loc... chrome/browser/search/local_omnibox_popup_source.h:21: // Overriden from content::URLDataSource: On 2013/03/19 02:35:53, samarth wrote: > nit: Overridden Done. https://codereview.chromium.org/12840003/diff/38001/chrome/browser/search/sea... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/12840003/diff/38001/chrome/browser/search/sea... chrome/browser/search/search.cc:324: (url == GURL(chrome::kChromeSearchLocalNTPURL) || On 2013/03/19 02:35:53, samarth wrote: > Actually these two checks are now redundant given the check for > kChromeSearchScheme above. You can just remove then (and make sure the tests > still pass). Done.
Friendly ping - we're hoping to get this by the branch point. The JS/CSS is largely a port from the GWS version of the NTP, which is currently live.
.grd LGTM
On 2013/03/20 14:16:21, sky wrote: > .grd LGTM Looks like Evan is OOO today and the rest of the week. Scott - would you be able to approve for resources/* as well? The GWS-version of the code was already approved by dcblack.
Or James - do you think you'd be able to review the JS/CSS? We're hoping to get this in for the branch point.
Oops, jhawkins is also OOO. Erik - would you be able to review for resources/*? The GWS-version of the code was already approved by dcblack and is live.
I'm not terribly knowledgable about the code in resources/local_ntp. You should get a more local owner. On Wed, Mar 20, 2013 at 2:50 PM, <jeremycho@chromium.org> wrote: > On 2013/03/20 14:16:21, sky wrote: > >> .grd LGTM >> > > Looks like Evan is OOO today and the rest of the week. Scott - would you > be > able to approve for resources/* as well? The GWS-version of the code was > already approved by dcblack. > > https://codereview.chromium.**org/12840003/<https://codereview.chromium.org/1... >
Dan - would you be able to review for resources/local_ntp?
I got part way there https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... File chrome/browser/resources/local_ntp/local_ntp.css (right): https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:5: background-attachment: fixed!important fixed !important https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:11: text-align: -webkit-center; what's different about -webkit-center than center? https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:20: .custom-theme #mv-noti-lks span { uh, what is #mv-noti-lks supposed to mean? https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:26: -webkit-transform: translate3d(0, 0, 0); ^ what's the point of this? hacking GPU acceleration? this isn't advised, if so https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:34: @media only screen and (min-width:666px) { what is the significance of this number? https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:40: @media only screen and (min-width:829px) { and this? https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:44: } where'd all my \n's go? https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:51: -webkit-transition: margin 200ms; -webkit-transition-duration: 200ms; ? https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:52: -webkit-transition-property: margin, opacity, width, -webkit-transform; nit: keep these in order of declarations (i.e. -webkit-transform, margin, opacity, width) https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:62: .mv-tile.mv-bl { mv-bl is cryptic https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:79: cursor: pointer; why does this need a pointer? generally things that should be emulating links should actually be links so magic stuff like ctrl+click Just Works™. https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:95: .mv-x-hide .mv-x { what does mv-x mean? close button? https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:100: background-image: url('images/close_bar.png'); background: transparent url(images/close_bar.png); https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:102: cursor: default; ^ why do you need to do this? https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:112: background-image: url('images/close_bar_active.png'); nit: same `background` shorthand nits unless there's some reason you can't... https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:130: width: 88%; ^ what's the significance of this %? https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:140: font: bold 12px Arial; I don't particularly like this short hand -- if the font ever changes you need to update this (and it's semi-likely people wont notice right away) https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:144: cursor: default; ^ why? https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:149: cursor: pointer; ^ why? https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... File chrome/browser/resources/local_ntp/local_ntp.html (right): https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.html:15: <div id="mv-noti" class="mv-noti-hide"> noti -> notice? https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.html:17: <span id="mv-noti-lks"> lks -> links? https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:78: * Possible background-colors of a non-custom theme. Used to determine whether 1 \s between sentences in comments. https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:83: var WHITE = ['rgba(255,255,255,1)', 'rgba(0,0,0,0)']; uh, the second color here is transparent black? https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:94: * @type {number} @const https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:107: * @type {number} @const @const should be on next line if the comment is multi-line https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:178: var HIDE_NOTIFICATION_CLASS = 'mv-noti-hide'; var CLASSES = { HIDE_NOTIFICATION: 'mv-noti-hide', ... }; https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:195: Tile = function(elem, opt_rid) { s/Tile = function(/function Tile(/
Thanks for the quick feedback! https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... File chrome/browser/resources/local_ntp/local_ntp.css (right): https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:5: background-attachment: fixed!important On 2013/03/21 02:45:08, Dan Beam wrote: > fixed !important Done. https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:11: text-align: -webkit-center; On 2013/03/21 02:45:08, Dan Beam wrote: > what's different about -webkit-center than center? Not sure if there is a difference, though I noticed <center> elements apply this. https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:20: .custom-theme #mv-noti-lks span { On 2013/03/21 02:45:08, Dan Beam wrote: > uh, what is #mv-noti-lks supposed to mean? Most visited notification links. I'd prefer to keep these names to be consistent with the GWS code. Left clarifications throughout. https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:26: -webkit-transform: translate3d(0, 0, 0); On 2013/03/21 02:45:08, Dan Beam wrote: > ^ what's the point of this? hacking GPU acceleration? this isn't advised, if so The animation to show/hide tiles looks janky without this. https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:34: @media only screen and (min-width:666px) { On 2013/03/21 02:45:08, Dan Beam wrote: > what is the significance of this number? We only want to show 4 tiles if there's 100px of padding to the left and right. https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:40: @media only screen and (min-width:829px) { On 2013/03/21 02:45:08, Dan Beam wrote: > and this? See above. https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:44: } On 2013/03/21 02:45:08, Dan Beam wrote: > where'd all my \n's go? Back! https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:51: -webkit-transition: margin 200ms; On 2013/03/21 02:45:08, Dan Beam wrote: > -webkit-transition-duration: 200ms; ? Done. https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:52: -webkit-transition-property: margin, opacity, width, -webkit-transform; On 2013/03/21 02:45:08, Dan Beam wrote: > nit: keep these in order of declarations (i.e. -webkit-transform, margin, > opacity, width) Done. https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:62: .mv-tile.mv-bl { On 2013/03/21 02:45:08, Dan Beam wrote: > mv-bl is cryptic Added comment. https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:79: cursor: pointer; On 2013/03/21 02:45:08, Dan Beam wrote: > why does this need a pointer? generally things that should be emulating links > should actually be links so magic stuff like ctrl+click Just Works™. We don't want these to have link functionality/styling, except the pointer. https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:95: .mv-x-hide .mv-x { On 2013/03/21 02:45:08, Dan Beam wrote: > what does mv-x mean? close button? Added comment. https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:100: background-image: url('images/close_bar.png'); On 2013/03/21 02:45:08, Dan Beam wrote: > background: transparent url(images/close_bar.png); Done. https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:102: cursor: default; On 2013/03/21 02:45:08, Dan Beam wrote: > ^ why do you need to do this? This element is nested inside .mv-page, which has cursor:pointer. https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:112: background-image: url('images/close_bar_active.png'); On 2013/03/21 02:45:08, Dan Beam wrote: > nit: same `background` shorthand nits unless there's some reason you can't... Done. https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:130: width: 88%; On 2013/03/21 02:45:08, Dan Beam wrote: > ^ what's the significance of this %? Ah not needed anymore, now that its child element has a width. https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:140: font: bold 12px Arial; On 2013/03/21 02:45:08, Dan Beam wrote: > I don't particularly like this short hand -- if the font ever changes you need > to update this (and it's semi-likely people wont notice right away) Expanded. https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:144: cursor: default; On 2013/03/21 02:45:08, Dan Beam wrote: > ^ why? This is text, but we don't want the text cursor. https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:149: cursor: pointer; On 2013/03/21 02:45:08, Dan Beam wrote: > ^ why? These should look like links (not using <a> to avoid having to specify href). https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... File chrome/browser/resources/local_ntp/local_ntp.html (right): https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.html:15: <div id="mv-noti" class="mv-noti-hide"> On 2013/03/21 02:45:08, Dan Beam wrote: > noti -> notice? commented. https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.html:17: <span id="mv-noti-lks"> On 2013/03/21 02:45:08, Dan Beam wrote: > lks -> links? commented. https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:78: * Possible background-colors of a non-custom theme. Used to determine whether On 2013/03/21 02:45:08, Dan Beam wrote: > 1 \s between sentences in comments. Done. https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:83: var WHITE = ['rgba(255,255,255,1)', 'rgba(0,0,0,0)']; On 2013/03/21 02:45:08, Dan Beam wrote: > uh, the second color here is transparent black? This gets passed on occasion when a default theme is used. https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:94: * @type {number} On 2013/03/21 02:45:08, Dan Beam wrote: > @const Done. https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:107: * @type {number} @const On 2013/03/21 02:45:08, Dan Beam wrote: > @const should be on next line if the comment is multi-line Done. https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:178: var HIDE_NOTIFICATION_CLASS = 'mv-noti-hide'; On 2013/03/21 02:45:08, Dan Beam wrote: > var CLASSES = { > HIDE_NOTIFICATION: 'mv-noti-hide', > ... > }; Done. https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:195: Tile = function(elem, opt_rid) { On 2013/03/21 02:45:08, Dan Beam wrote: > s/Tile = function(/function Tile(/ Done.
Friendly ping :)
https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... File chrome/browser/resources/local_ntp/local_ntp.css (right): https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:20: .custom-theme #mv-noti-lks span { On 2013/03/21 05:28:42, jeremycho wrote: > On 2013/03/21 02:45:08, Dan Beam wrote: > > uh, what is #mv-noti-lks supposed to mean? > > Most visited notification links. I'd prefer to keep these names to be > consistent with the GWS code. Left clarifications throughout. I'd rather you changed the GWS code.
Thanks, PTAL. https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... File chrome/browser/resources/local_ntp/local_ntp.css (right): https://codereview.chromium.org/12840003/diff/42001/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:20: .custom-theme #mv-noti-lks span { On 2013/03/22 05:37:02, Dan Beam wrote: > On 2013/03/21 05:28:42, jeremycho wrote: > > On 2013/03/21 02:45:08, Dan Beam wrote: > > > uh, what is #mv-noti-lks supposed to mean? > > > > Most visited notification links. I'd prefer to keep these names to be > > consistent with the GWS code. Left clarifications throughout. > > I'd rather you changed the GWS code. Done. Kept mv-x, since it really is an X button used in more than one context.
https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... File chrome/browser/resources/local_ntp/local_ntp.css (right): https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:26: -webkit-transform: translate3d(0, 0, 0); + /* Use GPU compositing if available. */ https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:78: box-shadow: inset 0 2px 3px rgba(0, 0, 0, .09); ^ indent is off https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:157: font-family: Arial; ^ why is this necessary? isn't the whole thing Arial? https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:158: font-size: 12px; ^ why isn't this a %? https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:3: // found in the LICENSE file. can you add a namespace here? this shouldn't all be global, i.e. (function() { }()); or cr.define('local_ntp', function() { return { PublicStuff: PublicStuff, // bound to local_ntp.* }; }); https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:80: * @type {Array.<string>} nit: !Array https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:133: HIDE_NOTIFICATION: 'mv-notice-hide' nit: alpha https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:201: // tile in its old location and trigger the undo animation. ^ indent off https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:214: lastBlacklistedTileElement.classList.remove(CLASSES.BLACKLIST); just putting lastBlacklistedTileElement.scrollTop; should force this to happen synchronously https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:240: * @return {Tile} The new Tile_. why Tile_? https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:278: thumbnailElement.style['background-image'] = thumbnailElement.style.backgroundImage = https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:293: blacklistButton.title = "Don't show on this page"; ^ when will this happen? https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:341: \n\n or \n between methods -- be consistent (closure uses 2, chrome uses 1, your code is right in the middle, style-wise) https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:370: if (lastBlacklistedRID != null) { can rid ever be 0? if not, then just if (lastBlacklistedRID) else, I think typeof lastBlacklistedRID != 'undefined' is more correct assuming this type is actually (number|undefined). https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:403: topMarginElement.style['margin-top'] = topMarginElement.style.marginTop https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:415: \n\n or \n https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:470: function removeChildren(node) { node.innerHTML = ''; https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:484: function enable(element, className, enabled) { element.classList.toggle(className, enabled); https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:497: if (window.navigator && window.navigator.embeddedSearch) when will navigator not be there? https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:499: if (window.chrome && window.chrome.embeddedSearch) when will chrome not be there? https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:509: topMarginElement = document.getElementById(CLASSES.TOP_MARGIN); ^ uh, what? getElementById('class')? also, why aren't you using $() instead of getElementById? https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:530: topLevelHandle.searchBox.onsubmit = function() {}; ^ er, what is this doing?
Thanks Dan. PTAL - if it's at possible to approve this today that'd be awesome! Samarth - I added the placeholder page title. https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... File chrome/browser/resources/local_ntp/local_ntp.css (right): https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:26: -webkit-transform: translate3d(0, 0, 0); On 2013/03/22 19:32:50, Dan Beam wrote: > + /* Use GPU compositing if available. */ Done. https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:78: box-shadow: inset 0 2px 3px rgba(0, 0, 0, .09); On 2013/03/22 19:32:50, Dan Beam wrote: > ^ indent is off Done. https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:157: font-family: Arial; On 2013/03/22 19:32:50, Dan Beam wrote: > ^ why is this necessary? isn't the whole thing Arial? No, it inherits another font. We may want to use a different font eventually. https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:158: font-size: 12px; On 2013/03/22 19:32:50, Dan Beam wrote: > ^ why isn't this a %? Done. https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:3: // found in the LICENSE file. On 2013/03/22 19:32:50, Dan Beam wrote: > can you add a namespace here? this shouldn't all be global, i.e. > > (function() { > > }()); > > or > > cr.define('local_ntp', function() { > > > return { > PublicStuff: PublicStuff, // bound to local_ntp.* > }; > }); Done. https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:80: * @type {Array.<string>} On 2013/03/22 19:32:50, Dan Beam wrote: > nit: !Array Done. https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:133: HIDE_NOTIFICATION: 'mv-notice-hide' On 2013/03/22 19:32:50, Dan Beam wrote: > nit: alpha Sorry, not sure what you mean. https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:201: // tile in its old location and trigger the undo animation. On 2013/03/22 19:32:50, Dan Beam wrote: > ^ indent off Done. https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:201: // tile in its old location and trigger the undo animation. On 2013/03/22 19:32:50, Dan Beam wrote: > ^ indent off Done. https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:214: lastBlacklistedTileElement.classList.remove(CLASSES.BLACKLIST); On 2013/03/22 19:32:50, Dan Beam wrote: > just putting > > lastBlacklistedTileElement.scrollTop; > > should force this to happen synchronously Neat! Done. https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:240: * @return {Tile} The new Tile_. On 2013/03/22 19:32:50, Dan Beam wrote: > why Tile_? Done. https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:278: thumbnailElement.style['background-image'] = On 2013/03/22 19:32:50, Dan Beam wrote: > thumbnailElement.style.backgroundImage = Done. https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:293: blacklistButton.title = "Don't show on this page"; On 2013/03/22 19:32:50, Dan Beam wrote: > ^ when will this happen? Added the tracking bug. https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:341: On 2013/03/22 19:32:50, Dan Beam wrote: > \n\n or \n between methods -- be consistent (closure uses 2, chrome uses 1, your > code is right in the middle, style-wise) Done. https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:370: if (lastBlacklistedRID != null) { Using undefined just to be safe. On 2013/03/22 19:32:50, Dan Beam wrote: > can rid ever be 0? if not, then just > > if (lastBlacklistedRID) > > else, I think > > typeof lastBlacklistedRID != 'undefined' > > is more correct assuming this type is actually (number|undefined). https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:403: topMarginElement.style['margin-top'] = On 2013/03/22 19:32:50, Dan Beam wrote: > topMarginElement.style.marginTop Done. https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:415: On 2013/03/22 19:32:50, Dan Beam wrote: > \n\n or \n Done. https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:470: function removeChildren(node) { On 2013/03/22 19:32:50, Dan Beam wrote: > node.innerHTML = ''; Done. https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:484: function enable(element, className, enabled) { On 2013/03/22 19:32:50, Dan Beam wrote: > element.classList.toggle(className, enabled); Done. https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:497: if (window.navigator && window.navigator.embeddedSearch) On 2013/03/22 19:32:50, Dan Beam wrote: > when will navigator not be there? Isn't currently supported, but hopefully someday. https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:499: if (window.chrome && window.chrome.embeddedSearch) On 2013/03/22 19:32:50, Dan Beam wrote: > when will chrome not be there? This shouldn't happen. If it does, other things will also be borked anyways. https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:509: topMarginElement = document.getElementById(CLASSES.TOP_MARGIN); On 2013/03/22 19:32:50, Dan Beam wrote: > ^ uh, what? getElementById('class')? also, why aren't you using $() instead of > getElementById? AFAIK, because we're in chrome-search://, we can't import things like util.js. Added an ID enum. https://codereview.chromium.org/12840003/diff/63003/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:530: topLevelHandle.searchBox.onsubmit = function() {}; On 2013/03/22 19:32:50, Dan Beam wrote: > ^ er, what is this doing? See comment. Without this, Chrome doesn't fire events like onmostvisitedchange.
ping
https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... File chrome/browser/resources/local_ntp/local_ntp.css (right): https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:145: position: absolute; ^ how does this deal with text wrapping/overflow? https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:27: * @type {?number} @type {number} https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:29: var notificationTimer = null; s/null/0/ https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:215: } else if (isUndoing) { } else if (isUndoing) { // If this was called as a result of an undo, re-insert the last blacklisted // tile in its old location and trigger the undo animation. https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:228: } else { } else { // Otherwise render the tiles using the new data without animation. https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:302: // TODO(jeremycho): i18n. See crbug/190223. 1 \s between sentences, why not use real URL here? https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:310: favicon.style['background-image'] = 'url(' + faviconUrl + ')'; always use style.styleProperty rather than style['style-property'] if you can in this case: favicon.style.backgroundImage or favicon.style.background https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:348: notificationTimer = window.setTimeout( ^ you might be able to use CSS to do this, i.e. #notification-area { -webkit-transition: opacity 500ms; -webkit-transition-delay: 60s; } #notification-area.showing { opacity: 0; } https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:356: notification.classList.add(CLASSES.HIDE_NOTIFICATION); notificationTimer = 0; https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:379: if (lastBlacklistedRID != 'undefined') { anytime I see a comparison to the string 'undefined', something is wrong. it should almost always be if (!variable) or if (typeof variable != 'undefined') https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:471: node && node.parentNode && node.parentNode.removeChild(node); if (node && node.parentNode) node.parentNode.removeChild(node) https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:471: node && node.parentNode && node.parentNode.removeChild(node); why is this being called when disconnected or null nodes? https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:489: function enable(element, className, enabled) { ^ what's the benefit of this method https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:490: element.classList.toggle(className, enabled); ^ if you already have this? https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:500: return window.navigator.embeddedSearch; add this when it works https://codereview.chromium.org/12840003/diff/71002/chrome/browser/search/loc... File chrome/browser/search/local_ntp_source.cc (right): https://codereview.chromium.org/12840003/diff/71002/chrome/browser/search/loc... chrome/browser/search/local_ntp_source.cc:19: const char kHTMLFilename[] = "local-ntp.html"; generally when abbreviations are over 2 chars, we use kHtmlFilename https://codereview.chromium.org/12840003/diff/71002/chrome/browser/search/loc... chrome/browser/search/local_ntp_source.cc:89: filename == kCloseBarActiveFilename; nit: return filename == kHTMLFilename || filename == kJSFilename || filename == kCSSFilename || filename == kCloseBarFilename || https://codereview.chromium.org/12840003/diff/71002/chrome/browser/search/loc... File chrome/browser/search/local_ntp_source.h (right): https://codereview.chromium.org/12840003/diff/71002/chrome/browser/search/loc... chrome/browser/search/local_ntp_source.h:21: // Overridden from content::URLDataSource: ^ why are you putting these in private:? https://codereview.chromium.org/12840003/diff/71002/chrome/browser/search/loc... File chrome/browser/search/local_omnibox_popup_source.cc (right): https://codereview.chromium.org/12840003/diff/71002/chrome/browser/search/loc... chrome/browser/search/local_omnibox_popup_source.cc:19: const char kJSFilename[] = "local-omnibox-popup.js"; I'd rather you just change everything else to kHtml, kCss, etc. https://codereview.chromium.org/12840003/diff/71002/chrome/browser/search/sea... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/12840003/diff/71002/chrome/browser/search/sea... chrome/browser/search/search.cc:314: entry->GetVirtualURL() == GURL(chrome::kChromeSearchLocalNTPURL)) && NtpUrl (you've hit on exactly why we do MULTABBR as MultAbbr)
Addressed comments. PTAL. https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... File chrome/browser/resources/local_ntp/local_ntp.css (right): https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:145: position: absolute; On 2013/03/25 18:13:49, Dan Beam wrote: > ^ how does this deal with text wrapping/overflow? That's handled by the styles applied to the shadow DOM child element. https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:27: * @type {?number} On 2013/03/25 18:13:49, Dan Beam wrote: > @type {number} Done. https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:29: var notificationTimer = null; On 2013/03/25 18:13:49, Dan Beam wrote: > s/null/0/ Done. https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:215: } else if (isUndoing) { On 2013/03/25 18:13:49, Dan Beam wrote: > } else if (isUndoing) { > // If this was called as a result of an undo, re-insert the last blacklisted > // tile in its old location and trigger the undo animation. Done. https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:228: } else { On 2013/03/25 18:13:49, Dan Beam wrote: > } else { > // Otherwise render the tiles using the new data without animation. Done. https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:302: // TODO(jeremycho): i18n. See crbug/190223. On 2013/03/25 18:13:49, Dan Beam wrote: > 1 \s between sentences, why not use real URL here? Done. https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:310: favicon.style['background-image'] = 'url(' + faviconUrl + ')'; On 2013/03/25 18:13:49, Dan Beam wrote: > always use style.styleProperty rather than style['style-property'] if you can > > in this case: favicon.style.backgroundImage or favicon.style.background Done. https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:348: notificationTimer = window.setTimeout( Done. On 2013/03/25 18:13:49, Dan Beam wrote: > ^ you might be able to use CSS to do this, i.e. > > #notification-area { > -webkit-transition: opacity 500ms; > -webkit-transition-delay: 60s; > } > > #notification-area.showing { > opacity: 0; > } https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:356: notification.classList.add(CLASSES.HIDE_NOTIFICATION); Removed timer. On 2013/03/25 18:13:49, Dan Beam wrote: > notificationTimer = 0; https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:379: if (lastBlacklistedRID != 'undefined') { On 2013/03/25 18:13:49, Dan Beam wrote: > anytime I see a comparison to the string 'undefined', something is wrong. it > should almost always be if (!variable) or if (typeof variable != 'undefined') Done, to allow lastBlacklistedRID == 0. https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:471: node && node.parentNode && node.parentNode.removeChild(node); On 2013/03/25 18:13:49, Dan Beam wrote: > why is this being called when disconnected or null nodes? It shouldn't be. Removed check. https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:471: node && node.parentNode && node.parentNode.removeChild(node); On 2013/03/25 18:13:49, Dan Beam wrote: > if (node && node.parentNode) > node.parentNode.removeChild(node) Removed check. https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:489: function enable(element, className, enabled) { On 2013/03/25 18:13:49, Dan Beam wrote: > ^ what's the benefit of this method Removed. https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:490: element.classList.toggle(className, enabled); On 2013/03/25 18:13:49, Dan Beam wrote: > ^ if you already have this? Done. https://codereview.chromium.org/12840003/diff/71002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:500: return window.navigator.embeddedSearch; On 2013/03/25 18:13:49, Dan Beam wrote: > add this when it works Done. https://codereview.chromium.org/12840003/diff/71002/chrome/browser/search/loc... File chrome/browser/search/local_ntp_source.cc (right): https://codereview.chromium.org/12840003/diff/71002/chrome/browser/search/loc... chrome/browser/search/local_ntp_source.cc:19: const char kHTMLFilename[] = "local-ntp.html"; On 2013/03/25 18:13:49, Dan Beam wrote: > generally when abbreviations are over 2 chars, we use > > kHtmlFilename Done. https://codereview.chromium.org/12840003/diff/71002/chrome/browser/search/loc... chrome/browser/search/local_ntp_source.cc:89: filename == kCloseBarActiveFilename; On 2013/03/25 18:13:49, Dan Beam wrote: > nit: > > return filename == kHTMLFilename || filename == kJSFilename || > filename == kCSSFilename || filename == kCloseBarFilename || Done. https://codereview.chromium.org/12840003/diff/71002/chrome/browser/search/loc... File chrome/browser/search/local_ntp_source.h (right): https://codereview.chromium.org/12840003/diff/71002/chrome/browser/search/loc... chrome/browser/search/local_ntp_source.h:21: // Overridden from content::URLDataSource: On 2013/03/25 18:13:49, Dan Beam wrote: > ^ why are you putting these in private:? There didn't seem to be a reason to expose them. https://codereview.chromium.org/12840003/diff/71002/chrome/browser/search/loc... File chrome/browser/search/local_omnibox_popup_source.cc (right): https://codereview.chromium.org/12840003/diff/71002/chrome/browser/search/loc... chrome/browser/search/local_omnibox_popup_source.cc:19: const char kJSFilename[] = "local-omnibox-popup.js"; On 2013/03/25 18:13:49, Dan Beam wrote: > I'd rather you just change everything else to kHtml, kCss, etc. Done. https://codereview.chromium.org/12840003/diff/71002/chrome/browser/search/sea... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/12840003/diff/71002/chrome/browser/search/sea... chrome/browser/search/search.cc:314: entry->GetVirtualURL() == GURL(chrome::kChromeSearchLocalNTPURL)) && On 2013/03/25 18:13:49, Dan Beam wrote: > NtpUrl (you've hit on exactly why we do MULTABBR as MultAbbr) Done.
lgtm https://codereview.chromium.org/12840003/diff/71002/chrome/browser/search/loc... File chrome/browser/search/local_ntp_source.h (right): https://codereview.chromium.org/12840003/diff/71002/chrome/browser/search/loc... chrome/browser/search/local_ntp_source.h:21: // Overridden from content::URLDataSource: On 2013/03/25 21:27:16, jeremycho wrote: > On 2013/03/25 18:13:49, Dan Beam wrote: > > ^ why are you putting these in private:? > > There didn't seem to be a reason to expose them. static_cast<parent*>(class)->DoParentThing() still works, anyways, people are divided on this issue https://codereview.chromium.org/12840003/diff/93002/chrome/browser/resources/... File chrome/browser/resources/local_ntp/local_ntp.css (right): https://codereview.chromium.org/12840003/diff/93002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:113: background: transparent url('images/close_bar.png'); nit: url(images/close_bar.png) (quotes aren't necessary) https://codereview.chromium.org/12840003/diff/93002/chrome/browser/resources/... File chrome/browser/resources/local_ntp/local_ntp.html (right): https://codereview.chromium.org/12840003/diff/93002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.html:5: found in the LICENSE file. super optional nit: found in the LICENSE file. --> https://codereview.chromium.org/12840003/diff/93002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.html:8: <!-- Placeholder title until crbug/196415 is resolved. --> ^ url-ify (i.e. http://crbug.com/196415) https://codereview.chromium.org/12840003/diff/93002/chrome/browser/resources/... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/12840003/diff/93002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:291: // https://code.google.com/p/chromium/issues/detail?id=190223 nit: this is fine, but http://crbug.com/190223 is shorter https://codereview.chromium.org/12840003/diff/93002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:458: function removeNode(node) { I don't really see the benefit of this method, but I guess it's fine to leave here... seems to add more lines to this file for little benefit. https://codereview.chromium.org/12840003/diff/93002/chrome/browser/search/loc... File chrome/browser/search/local_ntp_source.cc (right): https://codereview.chromium.org/12840003/diff/93002/chrome/browser/search/loc... chrome/browser/search/local_ntp_source.cc:74: path == kCloseBarActiveFilename) nit: {curlies} https://codereview.chromium.org/12840003/diff/93002/chrome/browser/search/sea... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/12840003/diff/93002/chrome/browser/search/sea... chrome/browser/search/search.cc:322: IsInstantURL(url, profile)); nit: it'd be nice to eventually change this to IsInstantUrl but it can be done later
Thanks for the review! https://codereview.chromium.org/12840003/diff/93002/chrome/browser/resources/... File chrome/browser/resources/local_ntp/local_ntp.css (right): https://codereview.chromium.org/12840003/diff/93002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.css:113: background: transparent url('images/close_bar.png'); On 2013/03/25 21:37:39, Dan Beam wrote: > nit: url(images/close_bar.png) (quotes aren't necessary) Done. https://codereview.chromium.org/12840003/diff/93002/chrome/browser/resources/... File chrome/browser/resources/local_ntp/local_ntp.html (right): https://codereview.chromium.org/12840003/diff/93002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.html:5: found in the LICENSE file. On 2013/03/25 21:37:39, Dan Beam wrote: > super optional nit: found in the LICENSE file. --> Done. https://codereview.chromium.org/12840003/diff/93002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.html:8: <!-- Placeholder title until crbug/196415 is resolved. --> On 2013/03/25 21:37:39, Dan Beam wrote: > ^ url-ify (i.e. http://crbug.com/196415) Done. https://codereview.chromium.org/12840003/diff/93002/chrome/browser/resources/... File chrome/browser/resources/local_ntp/local_ntp.js (right): https://codereview.chromium.org/12840003/diff/93002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:291: // https://code.google.com/p/chromium/issues/detail?id=190223 On 2013/03/25 21:37:39, Dan Beam wrote: > nit: this is fine, but http://crbug.com/190223 is shorter Done. https://codereview.chromium.org/12840003/diff/93002/chrome/browser/resources/... chrome/browser/resources/local_ntp/local_ntp.js:458: function removeNode(node) { Acknowledged. Thought it was a little cleaner than inlining, since node appears twice. On 2013/03/25 21:37:39, Dan Beam wrote: > I don't really see the benefit of this method, but I guess it's fine to leave > here... seems to add more lines to this file for little benefit. https://codereview.chromium.org/12840003/diff/93002/chrome/browser/search/loc... File chrome/browser/search/local_ntp_source.cc (right): https://codereview.chromium.org/12840003/diff/93002/chrome/browser/search/loc... chrome/browser/search/local_ntp_source.cc:74: path == kCloseBarActiveFilename) On 2013/03/25 21:37:39, Dan Beam wrote: > nit: {curlies} Done. https://codereview.chromium.org/12840003/diff/93002/chrome/browser/search/sea... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/12840003/diff/93002/chrome/browser/search/sea... chrome/browser/search/search.cc:322: IsInstantURL(url, profile)); Acknowledged. On 2013/03/25 21:37:39, Dan Beam wrote: > nit: it'd be nice to eventually change this to IsInstantUrl but it can be done > later
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremycho@chromium.org/12840003/85003
Failed to trigger a try job on win_rel HTTP Error 400: Bad Request
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremycho@chromium.org/12840003/104001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jeremycho@chromium.org/12840003/104001
Message was sent while issue was closed.
Change committed as 190589 |
