Issue 1695022: NTP - Refactor the most visited code to uncouple it from the rest of the NTP....

Issue 1695022: NTP - Refactor the most visited code to uncouple it from the rest of the NTP.... (Closed)

Created:
10 years, 8 months ago by arv (Not doing code reviews)

Modified:
9 years, 4 months ago

Reviewers:
feldstein

CC:
chromium-reviews, arv (Not doing code reviews), pam+watch_chromium.org, ben+cc_chromium.org

Base URL:
svn://chrome-svn/chrome/trunk/src/

Visibility:
Public.

Description

NTP - Refactor the most visited code to uncouple it from the rest of the NTP. The goal of this refactoring is to allow splitting the different parts of the NTP into different reusable components. BUG=None TEST=Manually Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=46021

Patch Set 1 #

Patch Set 2 : Fix ui_test #

Patch Set 3 : '' #

Total comments: 6

Patch Set 4 : '' #

Created: 10 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1177 lines, -1019 lines)			Patch
M	chrome/browser/dom_ui/new_tab_ui_uitest.cc	View	2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/new_new_tab.css	View	1 2 3	8 chunks	+8 lines, -325 lines	0 comments	Download
M	chrome/browser/resources/new_new_tab.html	View	1 2 3	7 chunks	+36 lines, -130 lines	0 comments	Download
M	chrome/browser/resources/new_new_tab.js	View	1 2 3	17 chunks	+71 lines, -142 lines	0 comments	Download
A	chrome/browser/resources/ntp/most_visited.css	View		1 chunk	+259 lines, -0 lines	0 comments	Download
M	chrome/browser/resources/ntp/most_visited.js	View	1 2 3	1 chunk	+493 lines, -406 lines	0 comments	Download
A	chrome/browser/resources/ntp/util.js	View		1 chunk	+86 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/shared/js/class_list.js	View		1 chunk	+94 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/shared/js/class_list_test.html	View		1 chunk	+91 lines, -0 lines	0 comments	Download
M	tools/grit/grit/format/html_inline.py	View	1 2 3	2 chunks	+38 lines, -15 lines	0 comments	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

feldstein

On 2010/04/29 22:29:05, arv wrote: > Did this fall off your radar? it did sorry, ...

10 years, 7 months ago (2010-04-29 22:36:53 UTC) #3

feldstein

LGTM with a couple questions. I don't really know much python so it might be ...

10 years, 7 months ago (2010-04-29 23:18:18 UTC) #4

arv (Not doing code reviews)

10 years, 7 months ago (2010-04-29 23:24:49 UTC) #5

http://codereview.chromium.org/1695022/diff/13001/14003
File chrome/browser/resources/new_new_tab.html (right):

http://codereview.chromium.org/1695022/diff/13001/14003#newcode97
chrome/browser/resources/new_new_tab.html:97:
document.getElementById(id).className += ' hidden';
On 2010/04/29 23:18:18, feldstein wrote:
> Do we want to use classList.add('hidden') here?  Not a big deal either way.

I wanted this to work before loading classlist.js

http://codereview.chromium.org/1695022/diff/13001/14007
File chrome/browser/resources/new_new_tab.js (right):

http://codereview.chromium.org/1695022/diff/13001/14007#newcode1020
chrome/browser/resources/new_new_tab.js:1020: window.addEventListener('load',
onDataLoaded);
On 2010/04/29 23:18:18, feldstein wrote:
> Why do we call this on document load?  It seems like it will only do anything
> after the most visited is loaded, and that calls it itself.

This is old code but it does look like it could be cleaned up.

http://codereview.chromium.org/1695022/diff/13001/14005
File chrome/browser/resources/ntp/most_visited.js (right):

http://codereview.chromium.org/1695022/diff/13001/14005#newcode122
chrome/browser/resources/ntp/most_visited.js:122: // Send 'getMostVisitedPages'
with a callback since we want to find the new
On 2010/04/29 23:18:18, feldstein wrote:
> long line

Done.

Expand Messages | Collapse Messages