[NTP4] Make TilePage and CardSlider evented to simplify code and fix page switcher bug

[NTP4] Make TilePage and CardSlider evented to simplify code and fix page                                                                         
switcher bug (as well as make my life easier for downstream bugs that depend on
this one).

R=estade@chromium.org
TEST=NTP still works exactly as it does today without page switcher bug.
BUG=104564, 97762
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=118266

[Dan Beam, 2012-01-17 18:10:13]

ping

[Evan Stade, 2012-01-17 19:45:28]

http://codereview.chromium.org/9116037/diff/10001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/card_slider.js (right):

http://codereview.chromium.org/9116037/diff/10001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/card_slider.js:85: CARD_CHANGE_ENDED:
'cardSlider:card_change_ended',
as long as you're changing this, I'm unsure of its utility. Putting event name
strings in this struct won't generate compile time errors; will it even generate
runtime errors if you type, say, CARD_CHANGE_END instead of _ENDED? does
AddEventListener complaing about undefined input?

http://codereview.chromium.org/9116037/diff/10001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/card_slider.js:313: e.propertyName !=
'-webkit-transform') {
why !== then !=

http://codereview.chromium.org/9116037/diff/10001/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

http://codereview.chromium.org/9116037/diff/10001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:626: // We don't use an
AutoReset<bool> here as the uninstall dialog runs in a
the uninstall dialog runs in the UI thread, same as this code. It's not
synchronous because no dialog is synchronous.

http://codereview.chromium.org/9116037/diff/10001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:628: uninstall_from_page_ =
true;
I don't see why you need this new boolean. What is wrong with checking whether
extension_id_prompting_ is empty?

http://codereview.chromium.org/9116037/diff/10001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:853: extension_id_prompting_
= "";
.clear()

[Dan Beam, 2012-01-17 21:33:08]

http://codereview.chromium.org/9116037/diff/10001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/card_slider.js (right):

http://codereview.chromium.org/9116037/diff/10001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/card_slider.js:85: CARD_CHANGE_ENDED:
'cardSlider:card_change_ended',
On 2012/01/17 19:45:29, Evan Stade wrote:
> as long as you're changing this, I'm unsure of its utility. Putting event name
> strings in this struct won't generate compile time errors; will it even
generate
> runtime errors if you type, say, CARD_CHANGE_END instead of _ENDED?

Nope.

> does
> AddEventListener complaing about undefined input?

Nope.

It would allow us to change the strange_use_of_underscores_in_js without any
changes other than here and would minify better (force of habit I guess...we
don't need to in Chrome).  I can remove if you want, I just don't see the
*problem* with doing this, so I just followed the precedent.

http://codereview.chromium.org/9116037/diff/10001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/card_slider.js:313: e.propertyName !=
'-webkit-transform') {
On 2012/01/17 19:45:29, Evan Stade wrote:
> why !== then !=

I tend to use strict equal/does-not-equal operator when you have 2 unknown types
(the second comparison is to string literal) and when comparing to something on
this.  In general I don't like possibly accidentally comparing references as
null == undefined (or undefined == false), as you could possibly get into
something like:

  var shouldHide = false;
  window.setTimeout(function() {
    // If this node belongs to us.
    if ($('blah') == this.blah_) {
      // $('blah') might be null, this.blah_ probably undefined
      this.doBlah_();
    }
    // Only show if we're not already.
    if (shouldHide == this.isHidden_) {
      // false != undefined, true, this gets called
      return;
    } else {
      // Would throw, but never get here.
      this.hide_();
    }
  }, 100); // forget to .bind()

In this case e.target is always non-null (I believe) if you want me to change.

http://codereview.chromium.org/9116037/diff/10001/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

http://codereview.chromium.org/9116037/diff/10001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:628: uninstall_from_page_ =
true;
On 2012/01/17 19:45:29, Evan Stade wrote:
> I don't see why you need this new boolean. What is wrong with checking whether
> extension_id_prompting_ is empty?

Done.

[Evan Stade, 2012-01-17 22:33:03]

http://codereview.chromium.org/9116037/diff/10001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/card_slider.js (right):

http://codereview.chromium.org/9116037/diff/10001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/card_slider.js:85: CARD_CHANGE_ENDED:
'cardSlider:card_change_ended',
On 2012/01/17 21:33:09, Dan Beam wrote:
> On 2012/01/17 19:45:29, Evan Stade wrote:
> > as long as you're changing this, I'm unsure of its utility. Putting event
name
> > strings in this struct won't generate compile time errors; will it even
> generate
> > runtime errors if you type, say, CARD_CHANGE_END instead of _ENDED?
> 
> Nope.
> 
> > does
> > AddEventListener complaing about undefined input?
> 
> Nope.
> 
> It would allow us to change the strange_use_of_underscores_in_js without any
> changes other than here and would minify better (force of habit I guess...we
> don't need to in Chrome).  I can remove if you want, I just don't see the
> *problem* with doing this, so I just followed the precedent.

I believe we've had this conversation before, and concluded no it wouldn't allow
us to make most changes only here.

[Dan Beam, 2012-01-17 23:33:17]

http://codereview.chromium.org/9116037/diff/10001/chrome/browser/resources/sh...
File chrome/browser/resources/shared/js/cr/ui/card_slider.js (right):

http://codereview.chromium.org/9116037/diff/10001/chrome/browser/resources/sh...
chrome/browser/resources/shared/js/cr/ui/card_slider.js:85: CARD_CHANGE_ENDED:
'cardSlider:card_change_ended',
On 2012/01/17 22:33:05, Evan Stade wrote:
> On 2012/01/17 21:33:09, Dan Beam wrote:
> > On 2012/01/17 19:45:29, Evan Stade wrote:
> > > as long as you're changing this, I'm unsure of its utility. Putting event
> name
> > > strings in this struct won't generate compile time errors; will it even
> > generate
> > > runtime errors if you type, say, CARD_CHANGE_END instead of _ENDED?
> > 
> > Nope.
> > 
> > > does
> > > AddEventListener complaing about undefined input?
> > 
> > Nope.
> > 
> > It would allow us to change the strange_use_of_underscores_in_js without any
> > changes other than here and would minify better (force of habit I guess...we
> > don't need to in Chrome).  I can remove if you want, I just don't see the
> > *problem* with doing this, so I just followed the precedent.
> 
> I believe we've had this conversation before, and concluded no it wouldn't
allow
> us to make most changes only here.

Done. (I know you'd rather change the name, *however* in this case of [quite
feasibly] changing from under_scores to camelCase it'd actually work.)

[Dan Beam, 2012-01-18 00:03:29]

ptal

[Dan Beam, 2012-01-18 00:04:27]

missed this one

http://codereview.chromium.org/9116037/diff/10001/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

http://codereview.chromium.org/9116037/diff/10001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:853: extension_id_prompting_
= "";
On 2012/01/17 19:45:29, Evan Stade wrote:
> .clear()

Done.

[Dan Beam, 2012-01-18 00:07:23]

http://codereview.chromium.org/9116037/diff/10001/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/app_launcher_handler.cc (right):

http://codereview.chromium.org/9116037/diff/10001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/app_launcher_handler.cc:626: // We don't use an
AutoReset<bool> here as the uninstall dialog runs in a
On 2012/01/17 19:45:29, Evan Stade wrote:
> the uninstall dialog runs in the UI thread, same as this code. It's not
> synchronous because no dialog is synchronous.

Done. (removed because of following review comment change, thanks for info, btw)

[Evan Stade, 2012-01-18 00:19:51]

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

http://codereview.chromium.org/9116037/diff/11010/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:622: * Happens when a tile is
removed from a tile page.
at first glance this looks like a tile is being removed completely, but really
it's just being removed between pages. Probably good to clarify that.

http://codereview.chromium.org/9116037/diff/11010/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/tile_page.js (right):

http://codereview.chromium.org/9116037/diff/11010/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/tile_page.js:611: e.tilePage = this;
redundant?

[Dan Beam, 2012-01-18 01:20:07]

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

http://codereview.chromium.org/9116037/diff/11010/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/page_list_view.js:622: * Happens when a tile is
removed from a tile page.
On 2012/01/18 00:19:51, Evan Stade wrote:
> at first glance this looks like a tile is being removed completely, but really
> it's just being removed between pages. Probably good to clarify that.

Done.  (It's both.  I didn't make an explicit event called tilePage:tile_moved
as there doesn't seem to be a use case for it yet, so that action is essentially
just made up of a tilePage:tile_removed (from original page) followed by
tilePage:tile_added (from the new page). This seems logical as when a tile has
moved, it is still removed from it's original page.  Do you disagree?)

http://codereview.chromium.org/9116037/diff/11010/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/tile_page.js (right):

http://codereview.chromium.org/9116037/diff/11010/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/tile_page.js:611: e.tilePage = this;
On 2012/01/18 00:19:51, Evan Stade wrote:
> redundant?

Done. (changed to e.currentTarget)

[Evan Stade, 2012-01-19 01:03:17]

lgtm

http://codereview.chromium.org/9116037/diff/20010/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/tile_page.js (right):

http://codereview.chromium.org/9116037/diff/20010/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/tile_page.js:604: * @param {number} index Where
the tile was positioned before removal.
call it oldIndex

[Dan Beam, 2012-01-19 02:00:05]

https://chromiumcodereview.appspot.com/9116037/diff/20010/chrome/browser/reso...
File chrome/browser/resources/ntp4/tile_page.js (right):

https://chromiumcodereview.appspot.com/9116037/diff/20010/chrome/browser/reso...
chrome/browser/resources/ntp4/tile_page.js:604: * @param {number} index Where
the tile was positioned before removal.
On 2012/01/19 01:03:17, Evan Stade wrote:
> call it oldIndex

Done.

[Evan Stade, 2012-01-19 02:13:05]

still lgtm

https://chromiumcodereview.appspot.com/9116037/diff/20012/chrome/browser/reso...
File chrome/browser/resources/ntp4/page_list_view.js (right):

https://chromiumcodereview.appspot.com/9116037/diff/20012/chrome/browser/reso...
chrome/browser/resources/ntp4/page_list_view.js:272: // HACK(dbeam): Make
removal of pages and dots as quick as possible with
not really a hack, I'd just remove the HACK(dbeam) part

[commit-bot: I haz the power, 2012-01-19 06:12:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/9116037/30001

[commit-bot: I haz the power, 2012-01-19 07:30:19]

Change committed as 118266