Put "make this my home page" link into the tip section....

Put "make this my home page" link into the tip section.

BUG=28196
TEST= "make this my home page" should show up as a tip.


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

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

[arv (Not doing code reviews), 2009-12-01 22:27:31]

http://codereview.chromium.org/449073/diff/10/3002
File chrome/browser/resources/new_new_tab.js (right):

http://codereview.chromium.org/449073/diff/10/3002#newcode106
chrome/browser/resources/new_new_tab.js:106: var homepage_button =
document.createElement('button');
no underscores in js variable names. variableNamesLikeThis.

http://codereview.chromium.org/449073/diff/10/3002#newcode132
chrome/browser/resources/new_new_tab.js:132:
tipElement.removeEventListener('click', setAsHomePageLinkClicked);
How about moving this to clearTip?

http://codereview.chromium.org/449073/diff/10/3002#newcode1290
chrome/browser/resources/new_new_tab.js:1290: clearTipText();
The good part with the old code was that it did not remove the link in case of a
failure.

Can you move this down to onHomePageSet?

[tony, 2009-12-01 22:37:16]

http://codereview.chromium.org/449073/diff/10/3004
File chrome/browser/dom_ui/ntp_resource_cache.cc (left):

http://codereview.chromium.org/449073/diff/10/3004#oldcode362
chrome/browser/dom_ui/ntp_resource_cache.cc:362: if
(!profile_->GetPrefs()->GetBoolean(prefs::kHomePageIsNewTabPage))
Can you remove the pref observer for this?  It shouldn't cause the cache to
invalidate anymore.

[Miranda Callahan, 2009-12-01 23:29:41]

Addressed concerns; PTAL.

http://codereview.chromium.org/449073/diff/10/3004
File chrome/browser/dom_ui/ntp_resource_cache.cc (left):

http://codereview.chromium.org/449073/diff/10/3004#oldcode362
chrome/browser/dom_ui/ntp_resource_cache.cc:362: if
(!profile_->GetPrefs()->GetBoolean(prefs::kHomePageIsNewTabPage))
On 2009/12/01 22:37:16, tony wrote:
> Can you remove the pref observer for this?  It shouldn't cause the cache to
> invalidate anymore.

Done.

http://codereview.chromium.org/449073/diff/10/3002
File chrome/browser/resources/new_new_tab.js (right):

http://codereview.chromium.org/449073/diff/10/3002#newcode106
chrome/browser/resources/new_new_tab.js:106: var homepage_button =
document.createElement('button');
On 2009/12/01 22:27:31, arv wrote:
> no underscores in js variable names. variableNamesLikeThis.

Done.

http://codereview.chromium.org/449073/diff/10/3002#newcode132
chrome/browser/resources/new_new_tab.js:132:
tipElement.removeEventListener('click', setAsHomePageLinkClicked);
On 2009/12/01 22:27:31, arv wrote:
> How about moving this to clearTip?

Done.

http://codereview.chromium.org/449073/diff/10/3002#newcode1290
chrome/browser/resources/new_new_tab.js:1290: clearTipText();
On 2009/12/01 22:27:31, arv wrote:
> The good part with the old code was that it did not remove the link in case of
a
> failure.
> 
> Can you move this down to onHomePageSet?

I can; my worry about this was that it might have a strange effect if the user
clicked on "Make this my home page" and then immediately refreshed the page --
before the "onHomePageSet" callback had occurred.  If "onHomePageSet" then gets
called, the tip which will be cleared will be an entirely different tip.  This
is probably less likely than the (also unlikely, but still) failure case,
though, so I'll make the move.

[arv (Not doing code reviews), 2009-12-02 00:05:44]

LGTM

I would prefer if the code was extracted but do as you wish.

http://codereview.chromium.org/449073/diff/8001/6002
File chrome/browser/dom_ui/tips_handler.cc (right):

http://codereview.chromium.org/449073/diff/8001/6002#newcode65
chrome/browser/dom_ui/tips_handler.cc:65: // to set home page before resetting
tip index to 0.
Can we move this code to its own function? It seems like it is a bit too hidden
in here.

[Miranda Callahan, 2009-12-02 00:39:09]

Extracted the code to its own function.  Thanks!

http://codereview.chromium.org/449073/diff/8001/6002
File chrome/browser/dom_ui/tips_handler.cc (right):

http://codereview.chromium.org/449073/diff/8001/6002#newcode65
chrome/browser/dom_ui/tips_handler.cc:65: // to set home page before resetting
tip index to 0.
On 2009/12/02 00:05:45, arv wrote:
> Can we move this code to its own function? It seems like it is a bit too
hidden
> in here.

Done.

[Miranda Callahan, 2009-12-02 19:26:53]

Fixed a ui test that this change caused to hang; could you take a look at the
new test, please?  Thanks!

[arv (Not doing code reviews), 2009-12-02 19:32:37]

http://codereview.chromium.org/449073/diff/9014/11015
File chrome/browser/dom_ui/new_tab_ui_uitest.cc (right):

http://codereview.chromium.org/449073/diff/9014/11015#newcode163
chrome/browser/dom_ui/new_tab_ui_uitest.cc:163: ASSERT_EQ(L"", style_display);
This does not seem to test what we care about.

Are we hiding the tip when it is empty? Does it take up space?

[Miranda Callahan, 2009-12-02 21:41:50]

PTAL.

http://codereview.chromium.org/449073/diff/9014/11015
File chrome/browser/dom_ui/new_tab_ui_uitest.cc (right):

http://codereview.chromium.org/449073/diff/9014/11015#newcode163
chrome/browser/dom_ui/new_tab_ui_uitest.cc:163: ASSERT_EQ(L"", style_display);
On 2009/12/02 19:32:37, arv wrote:
> This does not seem to test what we care about.
> 
> Are we hiding the tip when it is empty? Does it take up space?

D'oh, you are right.  We remove the textContent in clearTipLine when
onHomePageSet is called.  Fixed.

[arv (Not doing code reviews), 2009-12-02 22:09:33]

LGTM