[Aura] Initial app list webui.

[Aura] Initial app list webui.

- Initiali app list webui;
- Refactor NTP4 code so that we could share part of the code;
- Use shared NTP4 code for app list contents;

TODO: Add search box support.

BUG=98308
TEST=Verify aura app list looks like the mock.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109648

[Evan Stade, 2011-11-03 16:42:14]

http://codereview.chromium.org/8423055/diff/5001/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/8423055/diff/5001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/new_tab.js:183: if (config.supportsPageSwitchers)
{
this seems like a heck of a lot of if configs, especially since for the NTP
config.supportsFoo is always true, and for the aura homepage it's always false.

I would say you probably want the shared code in one class and then have the NTP
extend that class. That will also solve the problem that you're using a file
called ntp.js for something that's not an NTP.

This file is mostly organized as a collection of static functions and you'd have
to reorganize the shared functionality into a class (like tile_page.js) but
despite that it's a bit of legwork it shouldn't be too hard.

http://codereview.chromium.org/8423055/diff/5001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/new_tab.js:224: if
(localStrings.getString('login_status_message')) {
seems this should also be toggled by supportsSyncLogin

http://codereview.chromium.org/8423055/diff/5001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/new_tab.js:279: var serverpromo =
localStrings.getString('serverpromo');
and I don't think you want this either

[xiyuan, 2011-11-04 20:56:02]

CL updated to address comments.

I have get rid of the config etc and followed suggestion to move shared code
into a PageListView class. Then NTP and app list would extend and customize it.

Most of the code in page_list_view.js is copied from new_tab.js and adapted into
PageViewList class.

[Evan Stade, 2011-11-07 23:59:20]

cool

http://codereview.chromium.org/8423055/diff/8002/chrome/browser/resources/app...
File chrome/browser/resources/app_list.js (right):

http://codereview.chromium.org/8423055/diff/8002/chrome/browser/resources/app...
chrome/browser/resources/app_list.js:19: function AppListView() {
filename should match class name. app_list.* should be in a subfolder (in spite
of the current state of things, almost nothing besides subdirectories should
just be in resources/)

http://codereview.chromium.org/8423055/diff/8002/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/page_list_view.js (right):

http://codereview.chromium.org/8423055/diff/8002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/page_list_view.js:24: var instance;
I don't like the restriction that this class should only be used as a singleton.
The static functions that rely on |instance| are all pass-throughs, so it's not
too bad to duplicate them into each place (Aura, NewTab) that wants the
singleton use case.

http://codereview.chromium.org/8423055/diff/8002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/page_list_view.js:583: function assert(condition,
opt_message) {
some of these functions should go in util.js

http://codereview.chromium.org/8423055/diff/8002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/page_list_view.js:727: var appendTilePage =
ntp4.appendTilePage;
are you planning to use all of this in the aura homepage? If not, this should
all be in new_tab.js.

This file needs to be moved to resources/shared/... and the namespace changed to
cr.ui. (card_slider.js and possibly other files will have to move as well)

[xiyuan, 2011-11-08 21:09:18]

CL updated. Please take another look. Thanks.

Summary of the changes:
- Moved app_list.* to resources/aura/app_list;
- Removed singleton instance in page_list_view.js and move the singleton logic
into new_tab.js and app_list.js;
- Moved isRTL, assert and getRequiredElement into shared/utils.js;

http://codereview.chromium.org/8423055/diff/8002/chrome/browser/resources/app...
File chrome/browser/resources/app_list.js (right):

http://codereview.chromium.org/8423055/diff/8002/chrome/browser/resources/app...
chrome/browser/resources/app_list.js:19: function AppListView() {
On 2011/11/07 23:59:20, Evan Stade wrote:
> filename should match class name. app_list.* should be in a subfolder (in
spite
> of the current state of things, almost nothing besides subdirectories should
> just be in resources/)

Done.
- Moved app_list.* to resources/aura/app_list;
- Rename AppListView to AppsView and move its code into apps_view.js;
- app_list.js will be the master code to instantiate AppsView and future
searchbox code;

http://codereview.chromium.org/8423055/diff/8002/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/page_list_view.js (right):

http://codereview.chromium.org/8423055/diff/8002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/page_list_view.js:24: var instance;
On 2011/11/07 23:59:20, Evan Stade wrote:
> I don't like the restriction that this class should only be used as a
singleton.
> The static functions that rely on |instance| are all pass-throughs, so it's
not
> too bad to duplicate them into each place (Aura, NewTab) that wants the
> singleton use case.

Done.

http://codereview.chromium.org/8423055/diff/8002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/page_list_view.js:583: function assert(condition,
opt_message) {
On 2011/11/07 23:59:20, Evan Stade wrote:
> some of these functions should go in util.js

Done. Moved assert, getRequiredEleemnt and isRTL into util.js

http://codereview.chromium.org/8423055/diff/8002/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/page_list_view.js:727: var appendTilePage =
ntp4.appendTilePage;
On 2011/11/07 23:59:20, Evan Stade wrote:
> are you planning to use all of this in the aura homepage? If not, this should
> all be in new_tab.js.
> 
> This file needs to be moved to resources/shared/... and the namespace changed
to
> cr.ui. (card_slider.js and possibly other files will have to move as well)

I only need appsPrefChangeCallback and getAppsCallback that are used by
app_launcher_handler. I moved those back into new_tab.js and added cooresponding
part in app_list.js.

However, we probably should not move this file into shared because it's very
application specific and depends on your DOM tree.

card_slider.js etc should be moved into shared but not sure if I should do that
in this CL though. I might not be the best guy to do the move especially the
touch handling part.

[Evan Stade, 2011-11-09 00:19:00]

I think it's ok to move PageListView, Trash, and other shared objects into
shared/cr/ui/js/page_list/. The page switchers depend on the dom structure but
that hasn't stopped you from sharing the code with the aura list (you can
gracefully handle when they're not there). If you think the page switchers are
truly NTP-specific you could move that code out of PageListView and into
NewTabView.

http://codereview.chromium.org/8423055/diff/14022/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/8423055/diff/14022/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.js:65: * Creates a NewTabView object.
would be nice to leave a comment about what NewTabView actually is

http://codereview.chromium.org/8423055/diff/14022/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.js:88: function initialize() {
can you rename this now to avoid confusion with PageListView.initialize

http://codereview.chromium.org/8423055/diff/14022/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.js:94: newTabView = new NewTabView();
I don't understand -- doesn't this call the constructor, which calls initialize,
forming an infinite loop?

http://codereview.chromium.org/8423055/diff/14022/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/page_list_view.js (right):

http://codereview.chromium.org/8423055/diff/14022/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:7: * PageListView manages page
list, dot list, switcher buttons and glue apps
glue apps?

also, final period

http://codereview.chromium.org/8423055/diff/14022/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:131: this.pageSwitcherStart =
$('page-switcher-start');
why is this code here rather than in new_tab.js?

http://codereview.chromium.org/8423055/diff/14022/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/util.js (right):

http://codereview.chromium.org/8423055/diff/14022/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/util.js:127: function isRTL() {
hmm, I think this should be isRtl but the similar C++ function is IsRTL(). I
guess we can leave it isRTL for now but it sucks that it's so inconsistent
whether we capitalize blocks of letters in function names.

[Evan Stade, 2011-11-09 00:38:19]

http://codereview.chromium.org/8423055/diff/14022/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/8423055/diff/14022/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.js:94: newTabView = new NewTabView();
On 2011/11/09 00:19:00, Evan Stade wrote:
> I don't understand -- doesn't this call the constructor, which calls
initialize,
> forming an infinite loop?

ignore this comment, I figured it out as you can see from my above request

[xiyuan, 2011-11-09 19:21:53]

I have removed the dependency of specific DOM nodes from PageListView. However,
I feel it is still not generic enough to be moved into shared, e.g. it depends
on AppLauncherHandler as its backend and this is different from all other shared
js code.

Files such as card_slider.js, drag_wrapper.js, grabber.js, touch_handler.js are
good to move into shared. But page_list_view.js, tile_page.js, apps_page.js,
trash.js, nav_dot.js, page_switcher.js are still not good enough to be moved.

http://codereview.chromium.org/8423055/diff/14022/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/8423055/diff/14022/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.js:65: * Creates a NewTabView object.
On 2011/11/09 00:19:00, Evan Stade wrote:
> would be nice to leave a comment about what NewTabView actually is

Done.

http://codereview.chromium.org/8423055/diff/14022/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.js:88: function initialize() {
On 2011/11/09 00:19:00, Evan Stade wrote:
> can you rename this now to avoid confusion with PageListView.initialize

Done. initialize -> load.

http://codereview.chromium.org/8423055/diff/14022/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.js:94: newTabView = new NewTabView();
On 2011/11/09 00:38:19, Evan Stade wrote:
> On 2011/11/09 00:19:00, Evan Stade wrote:
> > I don't understand -- doesn't this call the constructor, which calls
> initialize,
> > forming an infinite loop?
> 
> ignore this comment, I figured it out as you can see from my above request

I probably should give it a better name. :)

http://codereview.chromium.org/8423055/diff/14022/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/page_list_view.js (right):

http://codereview.chromium.org/8423055/diff/14022/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:7: * PageListView manages page
list, dot list, switcher buttons and glue apps
On 2011/11/09 00:19:00, Evan Stade wrote:
> glue apps?
> 
> also, final period

Done.
Comments updated to:

PageListView manages page list, dot list, switcher buttons and handles apps
pages callbacks from backend.

http://codereview.chromium.org/8423055/diff/14022/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:131: this.pageSwitcherStart =
$('page-switcher-start');
On 2011/11/09 00:19:00, Evan Stade wrote:
> why is this code here rather than in new_tab.js?

Switcher buttons might not be NTP specific. I think I will need to show them
somehow even though they do not show up in aura mock.

I'll move those get element calls into new_tab.js and pass them in from
external.

http://codereview.chromium.org/8423055/diff/14022/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/util.js (right):

http://codereview.chromium.org/8423055/diff/14022/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/util.js:127: function isRTL() {
On 2011/11/09 00:19:00, Evan Stade wrote:
> hmm, I think this should be isRtl but the similar C++ function is IsRTL(). I
> guess we can leave it isRTL for now but it sucks that it's so inconsistent
> whether we capitalize blocks of letters in function names.

I don't know what's best here either. :(

[Evan Stade, 2011-11-10 00:02:21]

just nits

Do you plan to move those js files you listed?

http://codereview.chromium.org/8423055/diff/19001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/8423055/diff/19001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.js:75: $('page-switcher-start'),
getRequiredElement?

http://codereview.chromium.org/8423055/diff/19001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.js:466:
document.addEventListener('DOMContentLoaded', ntp4.load);
can you make it onLoad

http://codereview.chromium.org/8423055/diff/19001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/page_list_view.js (right):

http://codereview.chromium.org/8423055/diff/19001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:128: * @param
{!Element|undefined} opt_Trash Optional trash element.
opt_trash

http://codereview.chromium.org/8423055/diff/19001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:185:
window.addEventListener('online', this.updateOfflineEnabledApps);
.bind(this)

http://codereview.chromium.org/8423055/diff/19001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:186:
window.addEventListener('offline', this.updateOfflineEnabledApps);
.bind(this)

http://codereview.chromium.org/8423055/diff/19001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:305: var pageIndex =
(app.page_index || 0);
remove ()

http://codereview.chromium.org/8423055/diff/19001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:578: updateOfflineEnabledApps:
function() {
should be private

http://codereview.chromium.org/8423055/diff/19001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:593: onDocKeyDown: function(e) {
should be private

[xiyuan, 2011-11-10 00:06:41]

On 2011/11/10 00:02:21, Evan Stade wrote:
> just nits
> 
> Do you plan to move those js files you listed?
> 

I'll move those good ones and update the CL. Thinking of leaving other js files
until I have a more clear UI design to decide whether to move them. Sounds good?

[Evan Stade, 2011-11-10 00:08:27]

sure

-- Evan Stade


On Wed, Nov 9, 2011 at 4:06 PM, <xiyuan@chromium.org> wrote:

> On 2011/11/10 00:02:21, Evan Stade wrote:
>
>> just nits
>>
>
>  Do you plan to move those js files you listed?
>>
>
>
> I'll move those good ones and update the CL. Thinking of leaving other js
> files
> until I have a more clear UI design to decide whether to move them. Sounds
> good?
>
>
http://codereview.chromium.**org/8423055/<http://codereview.chromium.org/8423...
>

[xiyuan, 2011-11-10 18:25:07]

CL updated:
- Fixed nits; 
- Moved card_slider.js, drag_wrapper.js, grabber.js and touch_handler.js into
shared and put them into cr.ui namespace;

Note I prepended cr.ui to TouchHandler instead of doing a "var TouchHandler =
cr.ui.TouchHandler" trick because later would require touch_handler.js to be
included before all other js files.

Please take another look. Thanks.

http://codereview.chromium.org/8423055/diff/19001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/8423055/diff/19001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.js:75: $('page-switcher-start'),
On 2011/11/10 00:02:21, Evan Stade wrote:
> getRequiredElement?

Done.

http://codereview.chromium.org/8423055/diff/19001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.js:466:
document.addEventListener('DOMContentLoaded', ntp4.load);
On 2011/11/10 00:02:21, Evan Stade wrote:
> can you make it onLoad

Done.

http://codereview.chromium.org/8423055/diff/19001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/page_list_view.js (right):

http://codereview.chromium.org/8423055/diff/19001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:128: * @param
{!Element|undefined} opt_Trash Optional trash element.
On 2011/11/10 00:02:21, Evan Stade wrote:
> opt_trash

Done.

http://codereview.chromium.org/8423055/diff/19001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:185:
window.addEventListener('online', this.updateOfflineEnabledApps);
On 2011/11/10 00:02:21, Evan Stade wrote:
> .bind(this)

Done.

http://codereview.chromium.org/8423055/diff/19001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:186:
window.addEventListener('offline', this.updateOfflineEnabledApps);
On 2011/11/10 00:02:21, Evan Stade wrote:
> .bind(this)

Done.

http://codereview.chromium.org/8423055/diff/19001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:305: var pageIndex =
(app.page_index || 0);
On 2011/11/10 00:02:21, Evan Stade wrote:
> remove ()

Done.

http://codereview.chromium.org/8423055/diff/19001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:578: updateOfflineEnabledApps:
function() {
On 2011/11/10 00:02:21, Evan Stade wrote:
> should be private

Done.

http://codereview.chromium.org/8423055/diff/19001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:593: onDocKeyDown: function(e) {
On 2011/11/10 00:02:21, Evan Stade wrote:
> should be private

Done.

[Evan Stade, 2011-11-11 00:17:10]

grabber.js and touch_handler.js are not actually used (they are copied from
touch_ntp, where they are used) so it might be best to remove them completely
for now. Aside from that LGTM

[xiyuan, 2011-11-11 00:34:50]

grabber.js removed.
touch_handler.js seems being used by CardSlider.

[commit-bot: I haz the power, 2011-11-11 17:28:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/8423055/28001

[commit-bot: I haz the power, 2011-11-11 17:29:17]

Presubmit check for 8423055-28001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  chrome/browser/resources/shared/js/cr/ui/touch_handler.js, line 9, 136 chars

Presubmit checks took 3.9s to calculate.