Add message and image to NTP to promote extensions and bookmark sync....

Add message and image to NTP to promote extensions and bookmark sync.

BUG=27393, 26744
TEST= promotional messages should show up on NTP.


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

[Miranda Callahan, 2009-11-13 20:54:54]

Arv, I am handing this off to review, but my Javascript-fu is still breaking on
the issue I discussed with you earlier: None of the steps I'm taking to center +
shrink-wrap the promo line are working; I'm wondering if I'm missing something
obvious.

I have the feeling that the code I used to let the footer float on the bottom of
the page messed up the edge calculations in a strange way.

I am going to continue working on this problem, but I thought that you might see
something I'm missing. Other than that problem, it should be ready for review.

[arv (Not doing code reviews), 2009-11-13 22:08:52]

Where are the images?

http://codereview.chromium.org/394005/diff/6033/5018
File chrome/browser/dom_ui/new_tab_ui.cc (right):

http://codereview.chromium.org/394005/diff/6033/5018#newcode219
Line 219: dom_ui_->RegisterMessageCallback("zeroPromoMessageCounter",
I don't like the name of this message. How about something like
"stopPromoMessages" or "preventPromo"

http://codereview.chromium.org/394005/diff/6033/5016
File chrome/browser/resources/i18n_template.js (right):

http://codereview.chromium.org/394005/diff/6033/5016#newcode40
Line 40: 'i18n-html-content': function(element, attributeValue, obj) {
No. Just use

i18n-values=".innerHTML:key"

The i18n-values has special handling for nested .innerHTML already

http://codereview.chromium.org/394005/diff/6033/5013
File chrome/browser/resources/new_new_tab.css (right):

http://codereview.chromium.org/394005/diff/6033/5013#newcode13
Line 13: height: auto;
height: auto is the default

http://codereview.chromium.org/394005/diff/6033/5013#newcode14
Line 14: min-height: 100%;
this forces the height to be at least 100%.

http://codereview.chromium.org/394005/diff/6033/5013#newcode18
Line 18: * html #container {
Useless selector. Nothing matches '* html' since html is the root of the tree.

http://codereview.chromium.org/394005/diff/6033/5013#newcode744
Line 744: color: rgb(6, 45, 117);
don't forget the underline color

http://codereview.chromium.org/394005/diff/6033/5013#newcode747
Line 747: #promonew {
promo-new

http://codereview.chromium.org/394005/diff/6033/5012
File chrome/browser/resources/new_new_tab.html (right):

http://codereview.chromium.org/394005/diff/6033/5012#newcode181
Line 181: <div id="container">
I don't like this. Why are you moving main into container?

container seems mostly useless to me

http://codereview.chromium.org/394005/diff/6033/5012#newcode285
Line 285: <div id="promo-line">
Is there a reason why we don't want to limit the width of the promo-line by
putting it inside #main?

http://codereview.chromium.org/394005/diff/6033/5012#newcode285
Line 285: <div id="promo-line">
To get the centering just see how it is done for the #notification element.

http://codereview.chromium.org/394005/diff/6033/5012#newcode288
Line 288: <span id="promo-close"></span>
Make #promo-close a button

http://codereview.chromium.org/394005/diff/6033/5015
File chrome/browser/resources/new_new_tab.js (right):

http://codereview.chromium.org/394005/diff/6033/5015#newcode1591
Line 1591: function setPromoCloseButton () {
rename, the current name implies a setter but it does not take an argument

http://codereview.chromium.org/394005/diff/6033/5015#newcode1591
Line 1591: function setPromoCloseButton () {
See other comments

http://codereview.chromium.org/394005/diff/6033/5015#newcode1592
Line 1592: function closePromo() {
Inline this below

http://codereview.chromium.org/394005/diff/6033/5015#newcode1598
Line 1598: $('promo-close').onclick=closePromo;
$('...').onclick = function(e) {
  ...
  e.preventDefault();
};

http://codereview.chromium.org/394005/diff/6033/5015#newcode1601
Line 1601: setPromoCloseButton();
No need to create a function. just put this on the global level.

$('...').onclick = function(e) {
  ...
  e.preventDefault();
};

http://codereview.chromium.org/394005/diff/6033/5014
File chrome/browser/resources/new_tab_theme.css (right):

http://codereview.chromium.org/394005/diff/6033/5014#newcode110
Line 110: }
What about the color of the new things you added. Shouldn't those also be
themed?

[Miranda Callahan, 2009-11-14 02:55:55]

Responded to comments; thanks for your great help.

The images were uploaded earlier today in a separate commit
(http://src.chromium.org/viewvc/chrome?view=rev&revision=31912).

http://codereview.chromium.org/394005/diff/6033/5018
File chrome/browser/dom_ui/new_tab_ui.cc (right):

http://codereview.chromium.org/394005/diff/6033/5018#newcode219
Line 219: dom_ui_->RegisterMessageCallback("zeroPromoMessageCounter",
On 2009/11/13 22:08:52, arv wrote:
> I don't like the name of this message. How about something like
> "stopPromoMessages" or "preventPromo"

Done.

http://codereview.chromium.org/394005/diff/6033/5016
File chrome/browser/resources/i18n_template.js (right):

http://codereview.chromium.org/394005/diff/6033/5016#newcode40
Line 40: 'i18n-html-content': function(element, attributeValue, obj) {
On 2009/11/13 22:08:52, arv wrote:
> No. Just use
> 
> i18n-values=".innerHTML:key"
> 
> The i18n-values has special handling for nested .innerHTML already

Done.

http://codereview.chromium.org/394005/diff/6033/5013
File chrome/browser/resources/new_new_tab.css (right):

http://codereview.chromium.org/394005/diff/6033/5013#newcode13
Line 13: height: auto;
On 2009/11/13 22:08:52, arv wrote:
> height: auto is the default

Done.

http://codereview.chromium.org/394005/diff/6033/5013#newcode14
Line 14: min-height: 100%;
On 2009/11/13 22:08:52, arv wrote:
> this forces the height to be at least 100%.

Removed all this code and used position: fixed to force a footer.

http://codereview.chromium.org/394005/diff/6033/5013#newcode18
Line 18: * html #container {
On 2009/11/13 22:08:52, arv wrote:
> Useless selector. Nothing matches '* html' since html is the root of the tree.

Done.

http://codereview.chromium.org/394005/diff/6033/5013#newcode744
Line 744: color: rgb(6, 45, 117);
On 2009/11/13 22:08:52, arv wrote:
> don't forget the underline color

following Glen's mock, using same color as link.  see
http://mocks/glen/chrome/spec/80_updatepromo/1/#centered.png

http://codereview.chromium.org/394005/diff/6033/5013#newcode747
Line 747: #promonew {
On 2009/11/13 22:08:52, arv wrote:
> promo-new

Done.

http://codereview.chromium.org/394005/diff/6033/5012
File chrome/browser/resources/new_new_tab.html (right):

http://codereview.chromium.org/394005/diff/6033/5012#newcode181
Line 181: <div id="container">
On 2009/11/13 22:08:52, arv wrote:
> I don't like this. Why are you moving main into container?
> 
> container seems mostly useless to me

Removed.

http://codereview.chromium.org/394005/diff/6033/5012#newcode285
Line 285: <div id="promo-line">
On 2009/11/13 22:08:52, arv wrote:
> Is there a reason why we don't want to limit the width of the promo-line by
> putting it inside #main?

Done.

http://codereview.chromium.org/394005/diff/6033/5012#newcode285
Line 285: <div id="promo-line">
On 2009/11/13 22:08:52, arv wrote:
> To get the centering just see how it is done for the #notification element.

Done.

http://codereview.chromium.org/394005/diff/6033/5012#newcode288
Line 288: <span id="promo-close"></span>
On 2009/11/13 22:08:52, arv wrote:
> Make #promo-close a button

Done.

http://codereview.chromium.org/394005/diff/6033/5015
File chrome/browser/resources/new_new_tab.js (right):

http://codereview.chromium.org/394005/diff/6033/5015#newcode1591
Line 1591: function setPromoCloseButton () {
On 2009/11/13 22:08:52, arv wrote:
> rename, the current name implies a setter but it does not take an argument

Done.

http://codereview.chromium.org/394005/diff/6033/5015#newcode1592
Line 1592: function closePromo() {
On 2009/11/13 22:08:52, arv wrote:
> Inline this below

Done.

http://codereview.chromium.org/394005/diff/6033/5015#newcode1598
Line 1598: $('promo-close').onclick=closePromo;
On 2009/11/13 22:08:52, arv wrote:
> $('...').onclick = function(e) {
>   ...
>   e.preventDefault();
> };

Done.

http://codereview.chromium.org/394005/diff/6033/5015#newcode1601
Line 1601: setPromoCloseButton();
On 2009/11/13 22:08:52, arv wrote:
> No need to create a function. just put this on the global level.
> 
> $('...').onclick = function(e) {
>   ...
>   e.preventDefault();
> };

Done.

http://codereview.chromium.org/394005/diff/6033/5014
File chrome/browser/resources/new_tab_theme.css (right):

http://codereview.chromium.org/394005/diff/6033/5014#newcode110
Line 110: }
On 2009/11/13 22:08:52, arv wrote:
> What about the color of the new things you added. Shouldn't those also be
> themed?

No, this is a standard yellow info bar that should always stand out from any
theme.

[Ben Goodger (Google), 2009-11-15 18:48:07]

OK, let's get this in now for the next dev channel.

Arv can follow up monday with further comments as necessary.

http://codereview.chromium.org/394005/diff/8013/24
File chrome/browser/dom_ui/new_tab_ui.cc (right):

http://codereview.chromium.org/394005/diff/8013/24#newcode215
Line 215: DISALLOW_COPY_AND_ASSIGN(PromotionalMessageHandler);
should be in private:

[Miranda Callahan, 2009-11-15 18:55:29]

http://codereview.chromium.org/394005/diff/8013/24
File chrome/browser/dom_ui/new_tab_ui.cc (right):

http://codereview.chromium.org/394005/diff/8013/24#newcode215
Line 215: DISALLOW_COPY_AND_ASSIGN(PromotionalMessageHandler);
On 2009/11/15 18:48:07, Ben Goodger wrote:
> should be in private:

Done.

[arv (Not doing code reviews), 2009-11-16 17:47:48]

http://codereview.chromium.org/394005/diff/5064/6078
File chrome/browser/resources/new_new_tab.css (right):

http://codereview.chromium.org/394005/diff/5064/6078#newcode693
Line 693: display: block;
Do you really need display block here?

http://codereview.chromium.org/394005/diff/5064/6078#newcode697
Line 697: bottom: 0px;
bottom: 0;

Don't use units for zero

http://codereview.chromium.org/394005/diff/5064/6078#newcode707
Line 707: display: block;
Do you really need display block here?

http://codereview.chromium.org/394005/diff/5064/6077
File chrome/browser/resources/new_new_tab.html (right):

http://codereview.chromium.org/394005/diff/5064/6077#newcode175
Line 175: <div id="bottom-right-promo">
can you put this after main like before?

http://codereview.chromium.org/394005/diff/5064/6080
File chrome/browser/resources/new_new_tab.js (right):

http://codereview.chromium.org/394005/diff/5064/6080#newcode1591
Line 1591: /* Close the promo notices when close button on the promo-line is
clicked. */
// Comment in JS.

/**
 * JSDoc comment
 */

In this case you should use a comment.

[Miranda Callahan, 2009-11-16 22:10:54]

Addressed all comments. Because this bug was committed, and because I have
another tweak to this code in another open bug, I would like to move this
discussion to http://codereview.chromium.org/385135/show.  This patch addresses
a few tweaks to the extensions promo, and also adds something Glen requested
that I would like you to take a look at -- when the extension promo line is
removed, it uses webkit transitions to zing off the bottom of the screen instead
of just disappearing.

http://codereview.chromium.org/394005/diff/5064/6078
File chrome/browser/resources/new_new_tab.css (right):

http://codereview.chromium.org/394005/diff/5064/6078#newcode693
Line 693: display: block;
On 2009/11/16 17:47:48, arv wrote:
> Do you really need display block here?

No; removed.

http://codereview.chromium.org/394005/diff/5064/6078#newcode697
Line 697: bottom: 0px;
On 2009/11/16 17:47:48, arv wrote:
> bottom: 0;
> 
> Don't use units for zero

Done.

http://codereview.chromium.org/394005/diff/5064/6078#newcode707
Line 707: display: block;
On 2009/11/16 17:47:48, arv wrote:
> Do you really need display block here?

No; removed.

http://codereview.chromium.org/394005/diff/5064/6077
File chrome/browser/resources/new_new_tab.html (right):

http://codereview.chromium.org/394005/diff/5064/6077#newcode175
Line 175: <div id="bottom-right-promo">
On 2009/11/16 17:47:48, arv wrote:
> can you put this after main like before?

I put it up here so it will be at the bottom of the z-axis, and allow the rest
of the page to float over it (at a request from Glen).

http://codereview.chromium.org/394005/diff/5064/6080
File chrome/browser/resources/new_new_tab.js (right):

http://codereview.chromium.org/394005/diff/5064/6080#newcode1591
Line 1591: /* Close the promo notices when close button on the promo-line is
clicked. */
On 2009/11/16 17:47:48, arv wrote:
> // Comment in JS.
> 
> /**
>  * JSDoc comment
>  */
> 
> In this case you should use a comment.

Done.

[arv (Not doing code reviews), 2009-11-16 22:15:05]

On Mon, Nov 16, 2009 at 14:10, <mirandac@chromium.org> wrote:
>
>  http://codereview.chromium.org/394005/diff/5064/6077#newcode175
> Line 175: <div id="bottom-right-promo">
> On 2009/11/16 17:47:48, arv wrote:
>
>> can you put this after main like before?
>>
>
> I put it up here so it will be at the bottom of the z-axis, and allow
> the rest of the page to float over it (at a request from Glen).


z-index is your friend.

Looking at the other CL now.


-- 
erik