Use a specialized new tab page in TOUCH_UI builds

Use a specialized new tab page in TOUCH_UI builds

This has no change on regular builds (except to rename the NTP resource to
be something more generic).

On TOUCH_UI builds, an alternate NTP front-end that is optimized for touch
is embedded into chrome instead of the standard one.  In particular,
I use conditional resource processing to choose the source of the
IDR_NEW_TAB_HTML resource (new_new_tab_page.html for regular builds,
touch_ntp/newtab.html for TOUCH_UI builds).

The same WebUI back-end supports the union of functionality needed by all
front-ends.  Eg. the addition of page-index tracking is used only by this
touch-ntp, and there are many features that the touch-ntp doesn't use (yet
anyway).

The idea with this touch NTP is to focus (for now) on apps, and make it easy
to arrange them into pages.  You can swipe/drag to switch pages, and press
and hold to lift an app and rearrange it.

There is lots of further work to improve the touch NTP (including the addition
of automated tests and various UI improvements).  But it's far enough along
now that we'd like to use it as the only NTP in TOUCH_UI builds.

Note that only the files in the main 'touch_ntp' directory are embedded as
a resource in chrome.  The 'tools' sub-directory provides a script for
type and syntax checking.  The 'standalone' sub-directory is a hack that
is used only if newtab.html is loaded directly as a regular web-page.  It
enables the app to be used (with fake data) directly for rapid prototyping
and testing.

BUG=None
TEST=Existing NTP tests still pass in regular builds.  Tests for Touch_UI builds are forthcoming.

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

[Rick Byers, 2011-03-10 15:53:53]

Rob/Chad: this is the combined upstreamed version of  all the touch-NTP work
you've already reviewed - no changes from the last delta I sent you.

Jon, mainly I'd like you to review how I've hooked this NTP up in the two
modified files (conditional resource selection using the same back end).  Of
course I'm interested in any other feedback you may have, but I don't think you
need to read all my JS if you don't want to (it's been reviewed incrementally by
Rob, Chad and others here in WAT).

Thanks!
   Rick

[rjkroege, 2011-03-10 18:25:24]

Trusting you that this is exactly like the code that I've already reviewed...

LGTM.

[arv (Not doing code reviews), 2011-03-10 19:25:58]

This is pretty awesome. Clear and well documented code.

I wish I got looped in on this earlier because it would have save us all some
time.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
File chrome/browser/resources/touch_ntp/eventtracker.js (right):

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/eventtracker.js:5: /** @fileoverview
@fileoverview EventTracker ...

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/eventtracker.js:17: *  Create an EventTracker
to track a set of events.
/**
 * Description
 * @constructor
 */

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/eventtracker.js:46:
EventTracker.prototype.add = function(node, eventType, listener, capture)
Use the following syntax instead:

EventTracker.prototype = {
  add: function(...) {
    ...
  },
  ...

};

The more verbose syntax you are currently using is not necessary for modern JS
engines.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/eventtracker.js:47: {
{ before new line

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/eventtracker.js:48: var h = {node: node,
var h = {
  eventType: ...
  ...
};

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/eventtracker.js:92: 
remove empty lines

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
File chrome/browser/resources/touch_ntp/grabber.js (right):

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:18: * @constructor
Missing description

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:52: this.touchHandler_.enable(/*
opt_capture */ false);
this.touchHandler_.enabled = false;

seems like a better API

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:78: * Events are fired at the
element affected (not the element being dragged).
@enum {string}

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:141: if (this.dragging_) {
Chrome style is to not use curly braces for single line if statements

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:149: * @param {!CustomEvent} e The
TouchHandler event.
CustomEvent is a useless DOM interface. Just use Event instead?

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:171:
this.dispatchEvent_(Grabber.EventType.RELEASE);
cr.dispatchSimpleEvent(this, Grabber.EventType.RELEASE);

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:217: this.baseTransform_ =
window.getComputedStyle(this.element_, null).
is this the right window?

this.ownerDocument.defaultView.getComputedStyle(this.element_);

Who designed these aweful DOM APIs? ;)

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:217: this.baseTransform_ =
window.getComputedStyle(this.element_, null).
skip null here?

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:270: y + 'px, 0) ' +
this.baseTransform_;
+2 indentation

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:276: *        indicating the
position to check.
indent 4 spaces

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:288: this.element_.style.display =
'none';
I would prefer pointerEvents = 'none' since that does not require layout but I'm
not 100% sure that elementFromPoint is aware of pointer events

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:335: Grabber.prototype.getElement
= function() {
Use getters and setters for getters and setters

Grabber.prototype = {
  ...
  get element() {
    return this.element_;
  },
  ...
};

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:346: var event =
document.createEvent('Event');
cr.dispatchSimpleEvent

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
File chrome/browser/resources/touch_ntp/newtab.css (right):

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/newtab.css:1: /** Copyright (c) 2011 The
Chromium Authors. All rights reserved.
Not JSDoc/JavaDoc

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/newtab.css:17: background:
-webkit-gradient(linear, left top, left bottom, from(#252c39),
-webkit-linear-gradient FTW

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/newtab.css:23: margin: 0px;
s/0px/0/

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/newtab.css:72: }
empty line between each rule

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/newtab.css:141: {
{ before newline

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
File chrome/browser/resources/touch_ntp/newtab.html (right):

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/newtab.html:13: <meta name="viewport"
content="user-scalable=no, width=device-width, maximum-scale=1.0" />
No /> in HTML

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/newtab.html:21: validate, I need to supply
and img src attribute, but to avoid
validation is over rated. Don't let it get to you

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/newtab.html:48: <script
type="text/javascript">
skip type

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/newtab.html:49: if(!window.chrome ||
!window.chrome.send) {
ws after if

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/newtab.html:51: // is intentionally
obfuscated so that grit doesn't try to embed this
This should be easy to fix in a cleaner way. Take a look at html_inline.py

http://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/tools/grit/grit/form...

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
File chrome/browser/resources/touch_ntp/standalone/standalone_hack.js (right):

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/standalone/standalone_hack.js:416: /*  Hack
to add Element.classList to older browsers that don't yet support it.
Why do we care about old browsers?

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/standalone/standalone_hack.js:527: /* Hack to
add Function.bind to older browsers that don't yet support it. From:
Why do we care about old browsers?

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
File chrome/browser/resources/touch_ntp/touchhandler.js (right):

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/touchhandler.js:52: this.totalMoveY_ = 0;
move value types to prototype

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/touchhandler.js:833:
event.initCustomEvent(eventType, bubbles, /*cancelable*/ true, detail);
var event = new cr.Event(eventType, bubbles, true);
event.foobar = baz
this.element_.dispatchEvent(event);

[Rick Byers, 2011-03-11 02:44:33]

Thanks for your detailed feedback Erik!  Sorry I didn't think to loop you in
earlier during the development of this code - it didn't occur to me that you
might be interested in it.

I've responded to all of your feedback and added a couple questions about a few
outstanding points (eg. I stopped converting classes to your prototype style
when I found the gjslint limitation - let me know what you think and I'll either
finish them all, or put them back).

Thanks!
   Rick

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
File chrome/browser/resources/touch_ntp/eventtracker.js (right):

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/eventtracker.js:5: /** @fileoverview
On 2011/03/10 19:25:59, arv wrote:
> @fileoverview EventTracker ...

Done.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/eventtracker.js:17: *  Create an EventTracker
to track a set of events.
On 2011/03/10 19:25:59, arv wrote:
> /**
>  * Description
>  * @constructor
>  */

Done.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/eventtracker.js:46:
EventTracker.prototype.add = function(node, eventType, listener, capture)
On 2011/03/10 19:25:59, arv wrote:
> Use the following syntax instead:
> 
> EventTracker.prototype = {
>   add: function(...) {
>     ...
>   },
>   ...
> 
> };
> 
> The more verbose syntax you are currently using is not necessary for modern JS
> engines.

I like it, thanks! Done.  Does it not matter that this style omits the
'constructor' property that would otherwise exist?

Note that gjslint isn't smart enough to detect the type of 'this' using this
style, so since I don't want to loose good linting, I've added @this
annotations.  Kind of defeats the simplification though.  Do you think this is
still worth it?  Let me know and I'll convert all my other classes to this style
too.

Maybe we need a Chrome JavaScript style guide for rules like this (that
presumably can't go in the Google JavaScript style guide due to older browsers)?

What is your preference for how to write static methods (like the one below)?

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/eventtracker.js:47: {
On 2011/03/10 19:25:59, arv wrote:
> { before new line

Done.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/eventtracker.js:48: var h = {node: node,
On 2011/03/10 19:25:59, arv wrote:
> var h = {
>   eventType: ...
>   ...
> };

Done.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/eventtracker.js:85: * @private
Note that this is a static method (doesn't access any instance members).  I'm
following the Google style guide here, does it look OK?

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/eventtracker.js:92: 
On 2011/03/10 19:25:59, arv wrote:
> remove empty lines

Done.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
File chrome/browser/resources/touch_ntp/grabber.js (right):

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:18: * @constructor
On 2011/03/10 19:25:59, arv wrote:
> Missing description

Done.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:52: this.touchHandler_.enable(/*
opt_capture */ false);
On 2011/03/10 19:25:59, arv wrote:
> this.touchHandler_.enabled = false;
> 
> seems like a better API

enabled=false would suggest to me that the touchHandler was being disabled. 
That's not what this does - it enables but without capture.  I've added a
comment to help clarify.  Let me know if I've misunderstood your suggestion.

Note that I did also add a 'disable' method.  In this code I don't ever use a
TouchHandler that does capturing, so I could replace enable(bool)/disable with a
boolean property as you suggest with some loss of flexibility in other scenarios
if you think it's better.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:78: * Events are fired at the
element affected (not the element being dragged).
On 2011/03/10 19:25:59, arv wrote:
> @enum {string}

Done.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:141: if (this.dragging_) {
On 2011/03/10 19:25:59, arv wrote:
> Chrome style is to not use curly braces for single line if statements

Done.
Does this apply to if/else blocks to?  Eg. I've got some code in touchhandler.js
like:

if (c) {
  a();
} else {
  b();
}

Should it be this instead?

if (c)
  a();
else
  b();

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:149: * @param {!CustomEvent} e The
TouchHandler event.
On 2011/03/10 19:25:59, arv wrote:
> CustomEvent is a useless DOM interface. Just use Event instead?

The only reason I used CustomEvent is that it adds the 'detail' property where I
can stash extra stuff.  Are you saying that it's better to just rely on expando
properties on the JS Event object?  That would certainly be simpler if it's OK.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:171:
this.dispatchEvent_(Grabber.EventType.RELEASE);
On 2011/03/10 19:25:59, arv wrote:
> cr.dispatchSimpleEvent(this, Grabber.EventType.RELEASE);

I should have looked more closely at the utilities in cr.js to see what I could
use there - thanks for pointing that out.  But in this case, dispatchSimpleEvent
doesn't set 'sender' and my clients rely on being able to get the Grabber
instance that raised the event (eg. so a single function can handle the grab of
any app - rather than needing a closure per app).

Should I expand dispatchSimpleEvent to add an optional sender argument at the
end perhaps?

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:217: this.baseTransform_ =
window.getComputedStyle(this.element_, null).
On 2011/03/10 19:25:59, arv wrote:
> is this the right window?
> 
> this.ownerDocument.defaultView.getComputedStyle(this.element_);
> 
> Who designed these aweful DOM APIs? ;)

Interesting - thanks.  I had to read up on this a bit - I'm still new to
HTML/JavaScript and didn't appreciate the impact of IFrames.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:217: this.baseTransform_ =
window.getComputedStyle(this.element_, null).
On 2011/03/10 19:25:59, arv wrote:
> skip null here?

Done.  I thought JSCompiler was complaining if I omitted the optional 2nd param,
but it seems happy now.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:270: y + 'px, 0) ' +
this.baseTransform_;
On 2011/03/10 19:25:59, arv wrote:
> +2 indentation

Done.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:276: *        indicating the
position to check.
On 2011/03/10 19:25:59, arv wrote:
> indent 4 spaces

Done.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:288: this.element_.style.display =
'none';
On 2011/03/10 19:25:59, arv wrote:
> I would prefer pointerEvents = 'none' since that does not require layout but
I'm
> not 100% sure that elementFromPoint is aware of pointer events

elementFromPoint does appear to be aware of pointerEvents - thanks!

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:335: Grabber.prototype.getElement
= function() {
On 2011/03/10 19:25:59, arv wrote:
> Use getters and setters for getters and setters
> 
> Grabber.prototype = {
>   ...
>   get element() {
>     return this.element_;
>   },
>   ...
> };
> 

Thanks, but JSCompiler doesn't yet support this ES5 syntax.  Instead I've used
the Object.defineProperty syntax for now.  I discovered that an option was
recently added to the JSCompiler sources to enable support for this, but there
hasn't been an official release yet - so I'll leave a TODO saying to upgrade.  I
could create a new jscompiler build for myself, but I wouldn't want that to be a
pre-req for anyone else looking to modify the code here... Agree?

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:346: var event =
document.createEvent('Event');
On 2011/03/10 19:25:59, arv wrote:
> cr.dispatchSimpleEvent

See above.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
File chrome/browser/resources/touch_ntp/newtab.css (right):

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/newtab.css:1: /** Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2011/03/10 19:25:59, arv wrote:
> Not JSDoc/JavaDoc

Done.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/newtab.css:17: background:
-webkit-gradient(linear, left top, left bottom, from(#252c39),
On 2011/03/10 19:25:59, arv wrote:
> -webkit-linear-gradient FTW

I was happy for a second thinking that the just-released iOS 4.3 probably fixed
this, but I just tested and can you believe they didn't update the WebKit build
in mobile safari?  They dropped a new JS engine in, but no new webkit?  Sheesh. 
I guess I need to leave this for now to enable me to keep testing on iPad.  

Or, if you think it's worth it, I could put some code in standalone_hack.js that
overrides the style when running on Safari.  It would be a shame to have the
background definition essentially duplicated though.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/newtab.css:23: margin: 0px;
On 2011/03/10 19:25:59, arv wrote:
> s/0px/0/

Done.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/newtab.css:72: }
On 2011/03/10 19:25:59, arv wrote:
> empty line between each rule

Done.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/newtab.css:141: {
On 2011/03/10 19:25:59, arv wrote:
> { before newline

Done.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
File chrome/browser/resources/touch_ntp/newtab.html (right):

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/newtab.html:13: <meta name="viewport"
content="user-scalable=no, width=device-width, maximum-scale=1.0" />
On 2011/03/10 19:25:59, arv wrote:
> No /> in HTML

Done.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/newtab.html:21: validate, I need to supply
and img src attribute, but to avoid
On 2011/03/10 19:25:59, arv wrote:
> validation is over rated. Don't let it get to you

Thanks!  

I think this comment is stale anyway - I now remove the template from the DOM
entirely when the page is loaded, so the qascore extension I was using to
validate my HTML shouldn't see it.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/newtab.html:48: <script
type="text/javascript">
On 2011/03/10 19:25:59, arv wrote:
> skip type

Done.  But I'm curious why - isn't it better to be explicit?

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/newtab.html:49: if(!window.chrome ||
!window.chrome.send) {
On 2011/03/10 19:25:59, arv wrote:
> ws after if

Done.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/newtab.html:51: // is intentionally
obfuscated so that grit doesn't try to embed this
On 2011/03/10 19:25:59, arv wrote:
> This should be easy to fix in a cleaner way. Take a look at html_inline.py
> 
>
http://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/tools/grit/grit/form...

Perfect, thanks!  I was never really happy with this hack - much better now.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
File chrome/browser/resources/touch_ntp/standalone/standalone_hack.js (right):

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/standalone/standalone_hack.js:416: /*  Hack
to add Element.classList to older browsers that don't yet support it.
On 2011/03/10 19:25:59, arv wrote:
> Why do we care about old browsers?

See the comment at the top of this file - which I've expanded on now.

Since we don't quite yet have a stable touch-screen based platform to run Chrome
on, I've been doing some of my testing on an iPad.  I've tried to keep any need
for support of older browsers contained to standalone_hack.js (which is just a
tool in the source tree - not built into the product).

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/standalone/standalone_hack.js:527: /* Hack to
add Function.bind to older browsers that don't yet support it. From:
On 2011/03/10 19:25:59, arv wrote:
> Why do we care about old browsers?

See above.

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
File chrome/browser/resources/touch_ntp/grabber.js (right):

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/grabber.js:126: 
Darn - reitveld did a poor job of matching this diff up (probably due to the
indentation change combined with rearranging a few lines).  I've put a diff
ignoring whitespace here if you're interested:
http://www.corp.google.com/~rbyers/grabber.diff.txt

It's all responding to the comments here .

[jstritar, 2011-03-11 14:47:51]

The changes to the browser resources and ntp_resource_cache LGTM. Also, nice
JavaScript code. I was just fixing a memory leak caused by event listeners in
some extension code- your EventTracker looks very convenient.

[Rick Byers, 2011-03-11 14:57:44]

Thanks Jon!
I tried to write some of these classes with the idea of potentially re-using
them elsewhere - so feel free to move them into 'shared/js' if you ever have
a scenario where they would be useful.

Rick

On Fri, Mar 11, 2011 at 9:47 AM, <jstritar@chromium.org> wrote:

> The changes to the browser resources and ntp_resource_cache LGTM. Also,
> nice
> JavaScript code. I was just fixing a memory leak caused by event listeners
> in
> some extension code- your EventTracker looks very convenient.
>
>
> http://codereview.chromium.org/6661024/
>

[arv (Not doing code reviews), 2011-03-11 20:08:46]

Once again. This is pretty solid. I played around with the demo page and I
actually found a bug:

1. Drag and drop an icon to a new page at the end
2. Now drag the same one and I can create a new page at the end
3. There is now a blank page between. Should we prevent this from happening?

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
File chrome/browser/resources/touch_ntp/eventtracker.js (right):

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/eventtracker.js:46:
EventTracker.prototype.add = function(node, eventType, listener, capture)
On 2011/03/11 02:44:33, rbyers wrote:
> On 2011/03/10 19:25:59, arv wrote:
> > Use the following syntax instead:
> > 
> > EventTracker.prototype = {
> >   add: function(...) {
> >     ...
> >   },
> >   ...
> > 
> > };
> > 
> > The more verbose syntax you are currently using is not necessary for modern
JS
> > engines.
> 
> I like it, thanks! Done.  Does it not matter that this style omits the
> 'constructor' property that would otherwise exist?

No. constructor is not reliable anyway.

function A() {}
A.prototype = {...};
var a = new A;
a.constructor === Object;

but

function A() {}
var a = new A;
a.constructor === A;

> Note that gjslint isn't smart enough to detect the type of 'this' using this
> style, so since I don't want to loose good linting, I've added @this
> annotations.  Kind of defeats the simplification though.  Do you think this is
> still worth it?  Let me know and I'll convert all my other classes to this
style
> too.

We can fix gjslint :-)

> Maybe we need a Chrome JavaScript style guide for rules like this (that
> presumably can't go in the Google JavaScript style guide due to older
browsers)?

Yeah

> What is your preference for how to write static methods (like the one below)?

ClassName.staticMethodName = function() { ... };

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/eventtracker.js:85: * @private
On 2011/03/11 02:44:33, rbyers wrote:
> Note that this is a static method (doesn't access any instance members).  I'm
> following the Google style guide here, does it look OK?

Yes, looks perfect

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
File chrome/browser/resources/touch_ntp/grabber.js (right):

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:52: this.touchHandler_.enable(/*
opt_capture */ false);
On 2011/03/11 02:44:33, rbyers wrote:
> On 2011/03/10 19:25:59, arv wrote:
> > this.touchHandler_.enabled = false;
> > 
> > seems like a better API
> 
> enabled=false would suggest to me that the touchHandler was being disabled. 
> That's not what this does - it enables but without capture.  I've added a
> comment to help clarify.  Let me know if I've misunderstood your suggestion.
> 
> Note that I did also add a 'disable' method.  In this code I don't ever use a
> TouchHandler that does capturing, so I could replace enable(bool)/disable with
a
> boolean property as you suggest with some loss of flexibility in other
scenarios
> if you think it's better.

OK, then it makes sense.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:141: if (this.dragging_) {
On 2011/03/11 02:44:33, rbyers wrote:
> On 2011/03/10 19:25:59, arv wrote:
> > Chrome style is to not use curly braces for single line if statements
> 
> Done.
> Does this apply to if/else blocks to?  Eg. I've got some code in
touchhandler.js
> like:
> 
> if (c) {
>   a();
> } else {
>   b();
> }
> 
> Should it be this instead?
> 
> if (c)
>   a();
> else
>   b();

Yes.

If either the if or else part is more than one line both should have curly
braces. For example:

if (long_expression_that ==
    wraps) {
  singleStatement();
} else {
  singleStatement();
}

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:171:
this.dispatchEvent_(Grabber.EventType.RELEASE);
On 2011/03/11 02:44:33, rbyers wrote:
> On 2011/03/10 19:25:59, arv wrote:
> > cr.dispatchSimpleEvent(this, Grabber.EventType.RELEASE);
> 
> I should have looked more closely at the utilities in cr.js to see what I
could
> use there - thanks for pointing that out.  But in this case,
dispatchSimpleEvent
> doesn't set 'sender' and my clients rely on being able to get the Grabber
> instance that raised the event (eg. so a single function can handle the grab
of
> any app - rather than needing a closure per app).
> 
> Should I expand dispatchSimpleEvent to add an optional sender argument at the
> end perhaps?

(I misunderstood this. See more in next comment)

The sender is called 'target' in DOM. You can also use 'this' and
'currentTarget'. 'currentTarget' is the node that the current listener was
attached to.

<a>
  <b>
     <c/>
  </b>
</a>

b.addEventListener(type, function(e) {
  assertEquals(b, e.currentTarget);
  assertEquals(c, e.target);
  assertEquals(b, this)
});

c.addEventListener(type, function(e) {
  assertEquals(c, e.currentTarget);
  assertEquals(c, e.target);
  assertEquals(c, this)
});

var e = document.createEvent('Event');
e.initEvent(type, true, true);
c.dispatchEvent(e);

To play around with this you can use:

http://plexode.com/eval3/#ht=%3Cdiv%20id%3Da%3E%0A%20%20%3Cdiv%20id%3Db%3E%0A...


With this info do you still need to have a sender?

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:217: this.baseTransform_ =
window.getComputedStyle(this.element_, null).
On 2011/03/11 02:44:33, rbyers wrote:
> On 2011/03/10 19:25:59, arv wrote:
> > is this the right window?
> > 
> > this.ownerDocument.defaultView.getComputedStyle(this.element_);
> > 
> > Who designed these aweful DOM APIs? ;)
> 
> Interesting - thanks.  I had to read up on this a bit - I'm still new to
> HTML/JavaScript and didn't appreciate the impact of IFrames.

I assume most of the Chrome JS would fail in the presence of frames but
sometimes it is easy to get the right document and window so it might be worth
remembering that for the future.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:335: Grabber.prototype.getElement
= function() {
On 2011/03/11 02:44:33, rbyers wrote:
> On 2011/03/10 19:25:59, arv wrote:
> > Use getters and setters for getters and setters
> > 
> > Grabber.prototype = {
> >   ...
> >   get element() {
> >     return this.element_;
> >   },
> >   ...
> > };
> > 
> 
> Thanks, but JSCompiler doesn't yet support this ES5 syntax.  Instead I've used
> the Object.defineProperty syntax for now.  I discovered that an option was
> recently added to the JSCompiler sources to enable support for this, but there
> hasn't been an official release yet - so I'll leave a TODO saying to upgrade. 
I
> could create a new jscompiler build for myself, but I wouldn't want that to be
a
> pre-req for anyone else looking to modify the code here... Agree?   
> 

It is a such a relief to not have to be hamstrung by old browser limitations and
bugs that I think letting JSCompiler get in our way is not an option.

I'm fine with requiring tip of tree Closure Compiler too if that is what it
takes. We should make changes to GJSLint (Closure Linter) and JSCompiler instead
of changes to best practices coding practices for ToT Chrome.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:346: var event =
document.createEvent('Event');
On 2011/03/11 02:44:33, rbyers wrote:
> On 2011/03/10 19:25:59, arv wrote:
> > cr.dispatchSimpleEvent
> 
> See above.

Ah, I was confused by the overloaded term, sender. How about renaming this
grabber?

var e = new cr.Event(eventType, true, true);
e.grabber = this;
target.dispatchEvent(e);

but then the cr utilities are not that useful any more.

I think another approach to this is to make your elements into grabbers (no
wrapper object) much like how the rest of cr.ui is done. Then the sender and the
target would always be the same.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
File chrome/browser/resources/touch_ntp/newtab.css (right):

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/newtab.css:17: background:
-webkit-gradient(linear, left top, left bottom, from(#252c39),
On 2011/03/11 02:44:33, rbyers wrote:
> On 2011/03/10 19:25:59, arv wrote:
> > -webkit-linear-gradient FTW
> 
> I was happy for a second thinking that the just-released iOS 4.3 probably
fixed
> this, but I just tested and can you believe they didn't update the WebKit
build
> in mobile safari?  They dropped a new JS engine in, but no new webkit? 
Sheesh. 
> I guess I need to leave this for now to enable me to keep testing on iPad.  
> 
> Or, if you think it's worth it, I could put some code in standalone_hack.js
that
> overrides the style when running on Safari.  It would be a shame to have the
> background definition essentially duplicated though.

I'm fine leaving the more verbose syntax in here.

How important is it to be able to test this on non Chrome browsers?

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
File chrome/browser/resources/touch_ntp/newtab.html (right):

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/newtab.html:48: <script
type="text/javascript">
On 2011/03/11 02:44:33, rbyers wrote:
> On 2011/03/10 19:25:59, arv wrote:
> > skip type
> 
> Done.  But I'm curious why - isn't it better to be explicit?

type is optional and defaults to text/javascript in HTML5.

Less cruft lets us focus on the important things.

...also, no other script type is supported.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
File chrome/browser/resources/touch_ntp/standalone/standalone_hack.js (right):

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/standalone/standalone_hack.js:527: /* Hack to
add Function.bind to older browsers that don't yet support it. From:
On 2011/03/11 02:44:33, rbyers wrote:
> On 2011/03/10 19:25:59, arv wrote:
> > Why do we care about old browsers?
> 
> See above.

Mobile Safari does not have classList or bind?

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
File chrome/browser/resources/touch_ntp/eventtracker.js (right):

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/eventtracker.js:42: * @this {EventTracker}
I would just remove this and ignore those warnings in the linter... the linter
is wrong anyway

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
File chrome/browser/resources/touch_ntp/grabber.js (right):

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/grabber.js:45: // TODO: Switch to getter
syntax for properties once there is an official
Sad

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/grabber.js:52: 
We can skip this.element_ now and replace all usages with .element

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/grabber.js:170: {
} else {

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/grabber.js:269:
this.element_.style.webkitTransform = 'translate3d(' + x + 'px, ' +
WebkitTransform for consistency with the name conversion rule; "replace -c with
C"

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/grabber.js:286: // should ensure that we
don't trigger a relayout.
Actually, why don't you set "pointer-events: none" on the dragged element (the
whole time). Then you can just use event.target in mouseover, mousemove etc to
get the element currently over.

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
File chrome/browser/resources/touch_ntp/newtab.html (right):

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.html:43: <if expr="False">
nice

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
File chrome/browser/resources/touch_ntp/newtab.js (right):

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:102:
appTemplate.removeAttribute('id');
I don't really see a need to remove the attribute node here

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:144:
document.body.addEventListener(Grabber.EventType.DRAG_ENTER, appDragEnter,
document.addEventListener also works

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:145: false);
you can skip optional false

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:162:
e.sender.element.classList.add('trashing');
Yeah, this is what I hate with working with non Chrome browsers. All these
wrappers makes things harder to reason about. If you made the target a Grabber
this would have been:

e.target.classList.add

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:172: chrome.send('getApps');
Call this earlier for faster NTP

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:199: {
{ after new line

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:200: var element = /** @type
!Element */ (document.getElementById(id));
Oh noes... type casting is a plague that I loathed a lot when I was a TL on
Closure. Don't let it spread please.

...also, this type cast is incorrect since getElementById can return null. That
is the whole point of this function, isn't it?

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:201: assert(Boolean(element),
'Missing required element: ' + id);
no need to convert to boolean here

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:235: // Note that was have to
explicitly clean up the grabber objects so they stop
typo

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:282:
dots[slider.getCurrentCard()].classList.add('selected');
slider.currentCard... "use getters for getters" rule

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:310: if (typeof(themeUrlMapper) ==
'function') {
typeof is a unary operator and not a function

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:359: var span =
appElement.getElementsByTagName('span')[0];
appElement.querySelector('span')

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:366: var appImg =
appElement.getElementsByTagName('img')[0];
querySelector

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:369: appImg.style.webkitMaskImage =
'url(' + app.icon_big + ')';
We ran into a URL bug in WebKit for the NTP. I guess these URLs are not generic
but you might want to reuse the url function in shared/js/util.js

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:392: var grabber = new Grabber(/**
@type {!Element} */ (appElement));
sigh

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:443: var switchPage = function(e) {
function switchPage(e) {

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:446: }
missing semicolon

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:471: // Don't propagate the click
(in case we have a link or something)
This comment explains why we preventDefault. It does not explain why we do not
want propagation.

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:487: e = e.parentNode) {
e.parentElement allows you to skip the nodeTyoe check

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:498: var draggingAppContainer =
null;
Do you really need to init these to null? Most of the time null and undefined
are interchangeable

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:615: for (var c =
draggingAppContainer; c; c = c.nextSibling) {
nextElementSibling maybe to skip non element nodes?

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:625: if (followingContainer) {
This if is not needed. insertBefore(node, null) is exactly the same as
appendChild(node)

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:643: e.stopPropagation();
I don't understand what propagation has to do with the default action?

[Rick Byers, 2011-03-15 21:47:54]

Thanks again for your feedback Erik.  I think I've changed everything to account
for it.  In particular, I've adapted to your preferred prototype style
everywhere and I've abandoned the use of JSCompiler for type checking.

I've got one issue to investigate on iOS (pointer-events doesn't seem to be
working) so there will be at least one further tweak.  But I figured you'd
probably have more feedback anyway and so wanted to get the last couple days
worth of changes to you tonight before I go home.

Regarding:
>  I played around with the demo page and I actually found a bug:
> 1. Drag and drop an icon to a new page at the end
> 2. Now drag the same one and I can create a new page at the end
> 3. There is now a blank page between. Should we prevent this from happening?

That behavior is actually by-design - only empty pages at the end are to be
removed.  I figured a page may be temporarily empty while moving some apps out
to move others in, and since we don't support inserting new pages in the middle
I didn't want to just remove it.  But there's definitely room for debate here
and certainly future feature requests (like explicitly adding/removing pages in
the middle).

Thanks,
   Rick

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
File chrome/browser/resources/touch_ntp/grabber.js (right):

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:141: if (this.dragging_) {
On 2011/03/11 20:08:46, arv wrote:
> On 2011/03/11 02:44:33, rbyers wrote:
> > On 2011/03/10 19:25:59, arv wrote:
> > > Chrome style is to not use curly braces for single line if statements
> > 
> > Done.
> > Does this apply to if/else blocks to?  Eg. I've got some code in
> touchhandler.js
> > like:
> > 
> > if (c) {
> >   a();
> > } else {
> >   b();
> > }
> > 
> > Should it be this instead?
> > 
> > if (c)
> >   a();
> > else
> >   b();
> 
> Yes.
> 
> If either the if or else part is more than one line both should have curly
> braces. For example:
> 
> if (long_expression_that ==
>     wraps) {
>   singleStatement();
> } else {
>   singleStatement();
> }

Ok, thanks.  I believe I've updated for this bracing style everywhere.  Let me
know if you see anywhere I've missed though.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:171:
this.dispatchEvent_(Grabber.EventType.RELEASE);
On 2011/03/11 20:08:46, arv wrote:
> On 2011/03/11 02:44:33, rbyers wrote:
> > On 2011/03/10 19:25:59, arv wrote:
> > > cr.dispatchSimpleEvent(this, Grabber.EventType.RELEASE);
> > 
> > I should have looked more closely at the utilities in cr.js to see what I
> could
> > use there - thanks for pointing that out.  But in this case,
> dispatchSimpleEvent
> > doesn't set 'sender' and my clients rely on being able to get the Grabber
> > instance that raised the event (eg. so a single function can handle the grab
> of
> > any app - rather than needing a closure per app).
> > 
> > Should I expand dispatchSimpleEvent to add an optional sender argument at
the
> > end perhaps?
> 
> (I misunderstood this. See more in next comment)
> 
> The sender is called 'target' in DOM. You can also use 'this' and
> 'currentTarget'. 'currentTarget' is the node that the current listener was
> attached to.
> 
> <a>
>   <b>
>      <c/>
>   </b>
> </a>
> 
> b.addEventListener(type, function(e) {
>   assertEquals(b, e.currentTarget);
>   assertEquals(c, e.target);
>   assertEquals(b, this)
> });
> 
> c.addEventListener(type, function(e) {
>   assertEquals(c, e.currentTarget);
>   assertEquals(c, e.target);
>   assertEquals(c, this)
> });
> 
> var e = document.createEvent('Event');
> e.initEvent(type, true, true);
> c.dispatchEvent(e);
> 
> To play around with this you can use:
> 
>
http://plexode.com/eval3/#ht=%253Cdiv%2520id%253Da%253E%250A%2520%2520%253Cdi...
> 
> 
> With this info do you still need to have a sender?

See below.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:335: Grabber.prototype.getElement
= function() {
On 2011/03/11 20:08:46, arv wrote:
> On 2011/03/11 02:44:33, rbyers wrote:
> > On 2011/03/10 19:25:59, arv wrote:
> > > Use getters and setters for getters and setters
> > > 
> > > Grabber.prototype = {
> > >   ...
> > >   get element() {
> > >     return this.element_;
> > >   },
> > >   ...
> > > };
> > > 
> > 
> > Thanks, but JSCompiler doesn't yet support this ES5 syntax.  Instead I've
used
> > the Object.defineProperty syntax for now.  I discovered that an option was
> > recently added to the JSCompiler sources to enable support for this, but
there
> > hasn't been an official release yet - so I'll leave a TODO saying to
upgrade. 
> I
> > could create a new jscompiler build for myself, but I wouldn't want that to
be
> a
> > pre-req for anyone else looking to modify the code here... Agree?   
> > 
> 
> It is a such a relief to not have to be hamstrung by old browser limitations
and
> bugs that I think letting JSCompiler get in our way is not an option.
> 
> I'm fine with requiring tip of tree Closure Compiler too if that is what it
> takes. We should make changes to GJSLint (Closure Linter) and JSCompiler
instead
> of changes to best practices coding practices for ToT Chrome.

Ok.  I agree we shouldn't be sacrificing good style for these tools.  I'll
switch to using ToT jscompiler to get around this for now.  We can discuss the
bigger question of using JSCompiler at all off-line.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/grabber.js:346: var event =
document.createEvent('Event');
On 2011/03/11 20:08:46, arv wrote:
> On 2011/03/11 02:44:33, rbyers wrote:
> > On 2011/03/10 19:25:59, arv wrote:
> > > cr.dispatchSimpleEvent
> > 
> > See above.
> 
> Ah, I was confused by the overloaded term, sender. How about renaming this
> grabber?
> 
> var e = new cr.Event(eventType, true, true);
> e.grabber = this;
> target.dispatchEvent(e);
> 
> but then the cr utilities are not that useful any more.
> 
> I think another approach to this is to make your elements into grabbers (no
> wrapper object) much like how the rest of cr.ui is done. Then the sender and
the
> target would always be the same.

Darn - I forgot that I added a 'sender' property myself, I was thinking it was a
built-in property of the DOM event model (like it is in other eventing patterns)
that I was just setting.  Sorry for the confusion.  I'm having trouble getting
used to the fact that adding a new property is syntactically identical to
changing the value of an existing property - seems like a flaw in the language
to me, but maybe I just need to get into the dynamic mindset more.

The cr.ui approach sounds interesting.  Is there a design doc or anything
anywhere for it?  I've looked over the code a bit and have the general idea, but
I don't have a clear picture of the pros/cons relative to normal wrapper objects
(a pattern I got from google3 code).

Note however that I don't think this would make 'the sender and the target
always be the same'.  The whole reason I need something in addition to target is
that in the HTML5 DND event model (which Grabber copies), some events
(enter/leave/drop) have two elements of interest - the source and the target.

So for now I've made my own custom event type that adds a property for the
source (potentially distinct from the target).  Since at this point the cr
utilities don't seem to buy me anything, I didn't bother taking the dependency. 
Let me know if you have any better ideas.  Also to help make this source/target
distinction more clear, I now always specify the target explicitly in calls to
'sendEvent'.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
File chrome/browser/resources/touch_ntp/newtab.css (right):

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/newtab.css:17: background:
-webkit-gradient(linear, left top, left bottom, from(#252c39),
On 2011/03/11 20:08:46, arv wrote:
> On 2011/03/11 02:44:33, rbyers wrote:
> > On 2011/03/10 19:25:59, arv wrote:
> > > -webkit-linear-gradient FTW
> > 
> > I was happy for a second thinking that the just-released iOS 4.3 probably
> fixed
> > this, but I just tested and can you believe they didn't update the WebKit
> build
> > in mobile safari?  They dropped a new JS engine in, but no new webkit? 
> Sheesh. 
> > I guess I need to leave this for now to enable me to keep testing on iPad.  
> > 
> > Or, if you think it's worth it, I could put some code in standalone_hack.js
> that
> > overrides the style when running on Safari.  It would be a shame to have the
> > background definition essentially duplicated though.
> 
> I'm fine leaving the more verbose syntax in here.

Thanks.
 
> How important is it to be able to test this on non Chrome browsers?

It's definitely essential until we have a touch-based platform reliably running
Chrome.  Ideally I'd also like to keep it working until we have GPU acceleration
on such a device (since I can't reliably demonstrate the 'feel' of the NTP when
dragging/slidding is really janky).  We can talk off-line about what the time
frame for that might be if you like.

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
File chrome/browser/resources/touch_ntp/standalone/standalone_hack.js (right):

http://codereview.chromium.org/6661024/diff/1/chrome/browser/resources/touch_...
chrome/browser/resources/touch_ntp/standalone/standalone_hack.js:527: /* Hack to
add Function.bind to older browsers that don't yet support it. From:
On 2011/03/11 20:08:46, arv wrote:
> On 2011/03/11 02:44:33, rbyers wrote:
> > On 2011/03/10 19:25:59, arv wrote:
> > > Why do we care about old browsers?
> > 
> > See above.
> 
> Mobile Safari does not have classList or bind?

Right.  I just double-checked that the recent iOS 4.3 update doesn't help.  At
least this ugliness is contained to something that's not part of the product -
just a testing tool.

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
File chrome/browser/resources/touch_ntp/eventtracker.js (right):

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/eventtracker.js:42: * @this {EventTracker}
On 2011/03/11 20:08:47, arv wrote:
> I would just remove this and ignore those warnings in the linter... the linter
> is wrong anyway

Done.

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
File chrome/browser/resources/touch_ntp/grabber.js (right):

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/grabber.js:45: // TODO: Switch to getter
syntax for properties once there is an official
On 2011/03/11 20:08:47, arv wrote:
> Sad

Fixed

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/grabber.js:52: 
On 2011/03/11 20:08:47, arv wrote:
> We can skip this.element_ now and replace all usages with .element

Done.

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/grabber.js:170: {
On 2011/03/11 20:08:47, arv wrote:
> } else {

Done.

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/grabber.js:269:
this.element_.style.webkitTransform = 'translate3d(' + x + 'px, ' +
On 2011/03/11 20:08:47, arv wrote:
> WebkitTransform for consistency with the name conversion rule; "replace -c
with
> C"

Done.

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/grabber.js:286: // should ensure that we
don't trigger a relayout.
On 2011/03/11 20:08:47, arv wrote:
> Actually, why don't you set "pointer-events: none" on the dragged element (the
> whole time). Then you can just use event.target in mouseover, mousemove etc to
> get the element currently over.

Nice, I like it.  It complicates my TouchHandler abstraction a bit though.  Now
TouchHandler has to capture the original target of the UI event and include it
in the event details.  TouchHandler events are always fired at the element it is
tracking (so that clients can listen for movement events in one deterministic
place).  

I can imagine a number of possible different ways to structure the abstractions
here though (eg. maybe TouchHandler should be a singleton rather than created
per element of interest).  Let me know if you have a particular design you'd
prefer and I can always look into a redesign in a future update, etc.

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
File chrome/browser/resources/touch_ntp/newtab.js (right):

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:102:
appTemplate.removeAttribute('id');
On 2011/03/11 20:08:47, arv wrote:
> I don't really see a need to remove the attribute node here

I just noticed that it left empty 'id' attributes on all the elements in the
DOM.  It looks like assigning id to null is the right way to do this.

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:144:
document.body.addEventListener(Grabber.EventType.DRAG_ENTER, appDragEnter,
On 2011/03/11 20:08:47, arv wrote:
> document.addEventListener also works

Done.

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:145: false);
On 2011/03/11 20:08:47, arv wrote:
> you can skip optional false

Cool.  As an aside, is the only way for me to know that it's optional to check
the webkit source?  Neither the W3C spec nor MDC documentation say that it's
optional.  Since we don't have our own DOM API docs (AFAIK), I generally just
assume it matches Mozilla.

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:162:
e.sender.element.classList.add('trashing');
On 2011/03/11 20:08:47, arv wrote:
> Yeah, this is what I hate with working with non Chrome browsers. All these
> wrappers makes things harder to reason about. If you made the target a Grabber
> this would have been:
> 
> e.target.classList.add

Nope, that would be wrong (counfuses the source and target of the 'enter'
event).  I need the APP to get the class 'trashing' but the target of the drag
is the trash can.  Regardless hopefully this change addresses your concern?

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:172: chrome.send('getApps');
On 2011/03/11 20:08:47, arv wrote:
> Call this earlier for faster NTP

Done.

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:199: {
On 2011/03/11 20:08:47, arv wrote:
> { after new line

Done.

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:200: var element = /** @type
!Element */ (document.getElementById(id));
On 2011/03/11 20:08:47, arv wrote:
> Oh noes... type casting is a plague that I loathed a lot when I was a TL on
> Closure. Don't let it spread please.
> 
> ...also, this type cast is incorrect since getElementById can return null.
That
> is the whole point of this function, isn't it?

You were right that I was being sloppy - I fixed it to be correct now. 

As we've discussed off-line, I've stopped using jscompiler for type checking -
so I can remove all of these casts.

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:201: assert(Boolean(element),
'Missing required element: ' + id);
On 2011/03/11 20:08:47, arv wrote:
> no need to convert to boolean here

Done.

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:235: // Note that was have to
explicitly clean up the grabber objects so they stop
On 2011/03/11 20:08:47, arv wrote:
> typo

Done.

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:282:
dots[slider.getCurrentCard()].classList.add('selected');
On 2011/03/11 20:08:47, arv wrote:
> slider.currentCard... "use getters for getters" rule

Done.

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:310: if (typeof(themeUrlMapper) ==
'function') {
On 2011/03/11 20:08:47, arv wrote:
> typeof is a unary operator and not a function

Done.

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:359: var span =
appElement.getElementsByTagName('span')[0];
On 2011/03/11 20:08:47, arv wrote:
> appElement.querySelector('span')

Done.

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:366: var appImg =
appElement.getElementsByTagName('img')[0];
On 2011/03/11 20:08:47, arv wrote:
> querySelector

Done.

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:369: appImg.style.webkitMaskImage =
'url(' + app.icon_big + ')';
On 2011/03/11 20:08:47, arv wrote:
> We ran into a URL bug in WebKit for the NTP. I guess these URLs are not
generic
> but you might want to reuse the url function in shared/js/util.js

Thanks! I'm not sure what the restrictions are on URLs in the app database, so I
might as well use it.  Done.

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:443: var switchPage = function(e) {
On 2011/03/11 20:08:47, arv wrote:
> function switchPage(e) {

Done.

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:446: }
On 2011/03/11 20:08:47, arv wrote:
> missing semicolon

But not when using the syntax you prefer, right?

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:471: // Don't propagate the click
(in case we have a link or something)
On 2011/03/11 20:08:47, arv wrote:
> This comment explains why we preventDefault. It does not explain why we do not
> want propagation.

What is your preferred style for when to call stopPropagation?  The reason I was
calling both was it felt like I had handled the event and so no-one else should
be made aware of it.  But if it's normal for already-handled events to continue
propagating then I'll just use preventDefault.

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:487: e = e.parentNode) {
On 2011/03/11 20:08:47, arv wrote:
> e.parentElement allows you to skip the nodeTyoe check

Done.

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:498: var draggingAppContainer =
null;
On 2011/03/11 20:08:47, arv wrote:
> Do you really need to init these to null? Most of the time null and undefined
> are interchangeable

Nope, I originally used undefined and switched to null because the type grammar
was simpler and so I thought it might be the preferred pattern.  I'll switch it
back to {!Element|undefined} instead of {Element}

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:615: for (var c =
draggingAppContainer; c; c = c.nextSibling) {
On 2011/03/11 20:08:47, arv wrote:
> nextElementSibling maybe to skip non element nodes?

Done.

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:625: if (followingContainer) {
On 2011/03/11 20:08:47, arv wrote:
> This if is not needed. insertBefore(node, null) is exactly the same as
> appendChild(node)

Nice

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:643: e.stopPropagation();
On 2011/03/11 20:08:47, arv wrote:
> I don't understand what propagation has to do with the default action?

Does the expanded comment help?  I want to treat all drops the same EXCEPT a
drop on the trash can.  Is there another style you'd prefer?

[arv (Not doing code reviews), 2011-03-15 23:53:43]

I think using strict mode is good but you need to wrap each file in an anonymous
function and export the globals then.

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
File chrome/browser/resources/touch_ntp/newtab.js (right):

http://codereview.chromium.org/6661024/diff/9001/chrome/browser/resources/tou...
chrome/browser/resources/touch_ntp/newtab.js:145: false);
On 2011/03/15 21:47:54, rbyers wrote:
> On 2011/03/11 20:08:47, arv wrote:
> > you can skip optional false
> 
> Cool.  As an aside, is the only way for me to know that it's optional to check
> the webkit source?  Neither the W3C spec nor MDC documentation say that it's
> optional.  Since we don't have our own DOM API docs (AFAIK), I generally just
> assume it matches Mozilla.

The WebKit DOM bindings does type coercion. In JS, missing params at the call
site are treated as undefined at the callee which is coerced to false.

http://codereview.chromium.org/6661024/diff/20001/chrome/browser/resources/to...
File chrome/browser/resources/touch_ntp/eventtracker.js (right):

http://codereview.chromium.org/6661024/diff/20001/chrome/browser/resources/to...
chrome/browser/resources/touch_ntp/eventtracker.js:14: 'use strict';
Please do not use 'use strict' at the top level. It will break code that is in
other files

http://codereview.chromium.org/6661024/diff/20001/chrome/browser/resources/to...
File chrome/browser/resources/touch_ntp/standalone/standalone_hack.js (right):

http://codereview.chromium.org/6661024/diff/20001/chrome/browser/resources/to...
chrome/browser/resources/touch_ntp/standalone/standalone_hack.js:567: /* iOS
appears to have a bug where once an element is touched, if it later has
Use // for comments

[Rick Byers, 2011-03-16 22:06:02]

Thanks again Erik.
I've put everything inside anonymous functions so that 'use strict' will be
scoped.  As usual, Reitveld did a poor job aligning the diff due to all the
whitespace changes.

I've also tracked down the issue I was seeing on iPad.  Apparently touch events
(unlike mouse events) are always fired at the START of the touch - they don't
change to fire at other elements while the touch is moving.  This isn't iOS
specific - comes from WebKit and so affects Chrome too (and  after some
discussion, appears to be something we won't be able to change).  This means I
can't rely on the immediate target like we had hoped.  So I'm back to using
document.elementFromPoint to determine the element at the point of the touch,
but I've done this at a low-level in TouchHandler so keep this dirty detail
contained.

I've got a bit of final testing to do after all these changes, but again wanted
to get the latest to you before I leave for the night in case you had any more
feedback.

Thanks,
   Rick

http://codereview.chromium.org/6661024/diff/20001/chrome/browser/resources/to...
File chrome/browser/resources/touch_ntp/eventtracker.js (right):

http://codereview.chromium.org/6661024/diff/20001/chrome/browser/resources/to...
chrome/browser/resources/touch_ntp/eventtracker.js:14: 'use strict';
On 2011/03/15 23:53:43, arv wrote:
> Please do not use 'use strict' at the top level. It will break code that is in
> other files

Oh, because we concatenate all the files together when embedding as a resource,
right?  Ok - that's no problem for all my classes (which define a single global
each).  For standalone_hack.js I don't need to worry because we don't embed it. 
I'll see what I can come up with for newtabpage.js - I was feeling a little bad
about polluting the global namespace there anyway.

Has anyone ever looked into changing how we embed HTML resources to embed each
script file as a separate resource?  That would have the additional benefit of
avoiding duplication for all the shared JS files (which I assume are being
embedded into MANY different resources).

[arv (Not doing code reviews), 2011-03-16 22:55:05]

LGTM

http://codereview.chromium.org/6661024/diff/20001/chrome/browser/resources/to...
File chrome/browser/resources/touch_ntp/eventtracker.js (right):

http://codereview.chromium.org/6661024/diff/20001/chrome/browser/resources/to...
chrome/browser/resources/touch_ntp/eventtracker.js:14: 'use strict';
On 2011/03/16 22:06:02, rbyers wrote:
> On 2011/03/15 23:53:43, arv wrote:
> > Please do not use 'use strict' at the top level. It will break code that is
in
> > other files
> 
> Oh, because we concatenate all the files together when embedding as a
resource,
> right? 

Yup

> Ok - that's no problem for all my classes (which define a single global
> each).  For standalone_hack.js I don't need to worry because we don't embed
it. 
> I'll see what I can come up with for newtabpage.js - I was feeling a little
bad
> about polluting the global namespace there anyway.
> 
> Has anyone ever looked into changing how we embed HTML resources to embed each
> script file as a separate resource?  That would have the additional benefit of
> avoiding duplication for all the shared JS files (which I assume are being
> embedded into MANY different resources).

We have a way to do this already using chrome://resources/ URLs

http://codereview.chromium.org/6661024/diff/13005/chrome/browser/resources/to...
chrome/browser/resources/touch_ntp/eventtracker.js:94: 
{ before line break

[Rick Byers, 2011-03-17 16:18:22]

Thanks Erik.
I've fixed that one indentation issue and enabled --strict mode for gjslint to
catch these automatically.  I also had to fix a bunch of other indentation
issues gjslint found, but there are no other changes so I'll go ahead and submit
to the commit queue now.

Thanks again!
   Rick

http://codereview.chromium.org/6661024/diff/13005/chrome/browser/resources/to...
File chrome/browser/resources/touch_ntp/eventtracker.js (right):

http://codereview.chromium.org/6661024/diff/13005/chrome/browser/resources/to...
chrome/browser/resources/touch_ntp/eventtracker.js:94: {
On 2011/03/16 22:55:05, arv wrote:
> { before line break

Dammit - I keep making this mistake, sorry - old habits die hard!

Looks like gjslint --strict can catch these.  I've changed my 'check' script to
use --strict and fixed the other indentation problems it found.

[commit-bot: I haz the power, 2011-03-17 19:03:39]

Can't apply patch for file
chrome/browser/resources/touch_ntp/standalone/webstore_icon.png.
svn: 'chrome/browser/resources/touch_ntp/standalone' is not a working copy

[Rick Byers, 2011-03-17 21:10:12]

FYI, The Commit queue and try server doesn't support adding new binary
files.  Sadrul has submitted this for me (r78596) - thanks Sadrul!

Rick

On Thu, Mar 17, 2011 at 3:03 PM, <commit-bot@chromium.org> wrote:

> Can't apply patch for file
> chrome/browser/resources/touch_ntp/standalone/webstore_icon.png.
> svn: 'chrome/browser/resources/touch_ntp/standalone' is not a working copy
>
>
>
> http://codereview.chromium.org/6661024/
>