Add finch flag for rendering a recently closed link on the local ntp.

Add finch flag for rendering a recently closed link on the local ntp.

BUG=237541
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208374

[jeremycho, 2013-05-02 21:14:48]

samarth for code review
dcblack for UX review

Thanks!

[jeremycho, 2013-05-02 21:26:03]

On 2013/05/02 21:14:48, jeremycho wrote:
> samarth for code review
> dcblack for UX review
> 
> Thanks!

You can demo with:
--force-fieldtrials='InstantExtended/Group1 recently_closed_ntp_link:1/'

[samarth, 2013-05-03 04:52:44]

+Jered to look at the ntp_source changes.

https://codereview.chromium.org/14685004/diff/1/chrome/browser/search/local_n...
File chrome/browser/search/local_ntp_source.cc (right):

https://codereview.chromium.org/14685004/diff/1/chrome/browser/search/local_n...
chrome/browser/search/local_ntp_source.cc:85: if
(strcmp(kResources[i].mime_type, "text/html") == 0) {
Check the IDR or filename instead of relying on a single html mime type.

https://codereview.chromium.org/14685004/diff/1/chrome/browser/search/local_n...
chrome/browser/search/local_ntp_source.cc:85: if
(strcmp(kResources[i].mime_type, "text/html") == 0) {
Some sort of test for the new code could be nice.

https://codereview.chromium.org/14685004/diff/1/chrome/browser/search/local_n...
chrome/browser/search/local_ntp_source.cc:91: static const base::StringPiece
html(ResourceBundle::GetSharedInstance()
Does this (static stringpiece) work?  Is the memory returned by
GetRawDataResource guaranteed to be valid across calls?

https://codereview.chromium.org/14685004/diff/1/chrome/browser/search/local_n...
chrome/browser/search/local_ntp_source.cc:133: bool
LocalNtpSource::ShouldAddContentSecurityPolicy() const {
Umm, this is almost certainly a bad idea.  I assume this is because otherwise
you can't link to chrome:// pages.

Instead of doing this, try using NavigateContentWindow.

https://codereview.chromium.org/14685004/diff/1/chrome/browser/search/search.cc
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/14685004/diff/1/chrome/browser/search/search....
chrome/browser/search/search.cc:66: const char kRecentlyClosedNTPLinkFlagName[]
= "recently_closed_ntp_link";
show_recently_closed_ntp_link or just show_recently_closed

https://codereview.chromium.org/14685004/diff/1/chrome/browser/search/search.h
File chrome/browser/search/search.h (right):

https://codereview.chromium.org/14685004/diff/1/chrome/browser/search/search....
chrome/browser/search/search.h:125: bool IsRecentlyClosedNTPLinkEnabled();
How about ShouldShowRecentlyClosedOnNTP?

[jeremycho, 2013-05-03 06:30:29]

https://codereview.chromium.org/14685004/diff/1/chrome/browser/search/local_n...
File chrome/browser/search/local_ntp_source.cc (right):

https://codereview.chromium.org/14685004/diff/1/chrome/browser/search/local_n...
chrome/browser/search/local_ntp_source.cc:85: if
(strcmp(kResources[i].mime_type, "text/html") == 0) {
On 2013/05/03 04:52:45, samarth wrote:
> Check the IDR or filename instead of relying on a single html mime type.

Done.

https://codereview.chromium.org/14685004/diff/1/chrome/browser/search/local_n...
chrome/browser/search/local_ntp_source.cc:85: if
(strcmp(kResources[i].mime_type, "text/html") == 0) {
On 2013/05/03 04:52:45, samarth wrote:
> Some sort of test for the new code could be nice.

Will work on this.

https://codereview.chromium.org/14685004/diff/1/chrome/browser/search/local_n...
chrome/browser/search/local_ntp_source.cc:91: static const base::StringPiece
html(ResourceBundle::GetSharedInstance()
This idiom is used in other StartDataRequest functions, so I believe so.

On 2013/05/03 04:52:45, samarth wrote:
> Does this (static stringpiece) work?  Is the memory returned by
> GetRawDataResource guaranteed to be valid across calls?

https://codereview.chromium.org/14685004/diff/1/chrome/browser/search/local_n...
chrome/browser/search/local_ntp_source.cc:133: bool
LocalNtpSource::ShouldAddContentSecurityPolicy() const {
This is needed for webui::GetI18nTemplateHtml, which injects inline JS into the
html.  It's done elsewhere too, e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pri...


On 2013/05/03 04:52:45, samarth wrote:
> Umm, this is almost certainly a bad idea.  I assume this is because otherwise
> you can't link to chrome:// pages.
> 
> Instead of doing this, try using NavigateContentWindow.

https://codereview.chromium.org/14685004/diff/1/chrome/browser/search/search.cc
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/14685004/diff/1/chrome/browser/search/search....
chrome/browser/search/search.cc:66: const char kRecentlyClosedNTPLinkFlagName[]
= "recently_closed_ntp_link";
On 2013/05/03 04:52:45, samarth wrote:
> show_recently_closed_ntp_link or just show_recently_closed

Done.

https://codereview.chromium.org/14685004/diff/1/chrome/browser/search/search.h
File chrome/browser/search/search.h (right):

https://codereview.chromium.org/14685004/diff/1/chrome/browser/search/search....
chrome/browser/search/search.h:125: bool IsRecentlyClosedNTPLinkEnabled();
On 2013/05/03 04:52:45, samarth wrote:
> How about ShouldShowRecentlyClosedOnNTP?

Done.

[Jered, 2013-05-03 14:38:01]

https://codereview.chromium.org/14685004/diff/3002/chrome/browser/search/loca...
File chrome/browser/search/local_ntp_source.cc (right):

https://codereview.chromium.org/14685004/diff/3002/chrome/browser/search/loca...
chrome/browser/search/local_ntp_source.cc:60: const std::string& key,
deindent

https://codereview.chromium.org/14685004/diff/3002/chrome/browser/search/loca...
chrome/browser/search/local_ntp_source.cc:85: if (stripped_path ==
"local-ntp.html") {
Make this a const char* and reference it here and in the other place in this
file where it's used.

https://codereview.chromium.org/14685004/diff/3002/chrome/browser/search/loca...
chrome/browser/search/local_ntp_source.cc:94: webui::GetI18nTemplateHtml(html,
&translated_strings);
Ah, I bet you're running into a restriction against inline-script.

To get around this, you can add this to the local-ntp.html:
<script src="local-ntp-strings.js"></script>

Then here, when you get a request for local-ntp-strings.js, call
AppendJsonJS()
or similar to output an appropriate dictionary.

https://codereview.chromium.org/14685004/diff/3002/chrome/browser/search/loca...
chrome/browser/search/local_ntp_source.cc:134: return false;
Don't do this. We're using CSP as a layer of defense for most
visited/suggestions. What restriction are you running into? We can relax the
policy, or find another way.

[jeremycho, 2013-05-03 21:57:17]

Thanks for the comments - PTAL.  If we could get this in for m28 that'd be
awesome!

https://codereview.chromium.org/14685004/diff/3002/chrome/browser/search/loca...
File chrome/browser/search/local_ntp_source.cc (right):

https://codereview.chromium.org/14685004/diff/3002/chrome/browser/search/loca...
chrome/browser/search/local_ntp_source.cc:60: const std::string& key,
On 2013/05/03 14:38:01, Jered wrote:
> deindent

Done.

https://codereview.chromium.org/14685004/diff/3002/chrome/browser/search/loca...
chrome/browser/search/local_ntp_source.cc:85: if (stripped_path ==
"local-ntp.html") {
On 2013/05/03 14:38:01, Jered wrote:
> Make this a const char* and reference it here and in the other place in this
> file where it's used.

Done.

https://codereview.chromium.org/14685004/diff/3002/chrome/browser/search/loca...
chrome/browser/search/local_ntp_source.cc:94: webui::GetI18nTemplateHtml(html,
&translated_strings);
On 2013/05/03 14:38:01, Jered wrote:
> Ah, I bet you're running into a restriction against inline-script.
> 
> To get around this, you can add this to the local-ntp.html:
> <script src="local-ntp-strings.js"></script>
> 
> Then here, when you get a request for local-ntp-strings.js, call
> AppendJsonJS()
> or similar to output an appropriate dictionary.

Yup, thanks for the pointer.  Done.

https://codereview.chromium.org/14685004/diff/3002/chrome/browser/search/loca...
chrome/browser/search/local_ntp_source.cc:134: return false;
On 2013/05/03 14:38:01, Jered wrote:
> Don't do this. We're using CSP as a layer of defense for most
> visited/suggestions. What restriction are you running into? We can relax the
> policy, or find another way.

Removed

[jeremycho, 2013-05-04 01:40:21]

FYI, I'll make sure to merge the changes in 14569018 to local_ntp_source.cc once
that's submitted.

[jeremycho, 2013-05-04 02:14:30]

Updated text and style of link.

[jeremycho, 2013-05-06 21:29:00]

Rebased - PTAL when you can, but feel free to prioritize below more urgent CLs.

[Jered, 2013-05-06 23:57:33]

On 2013/05/06 21:29:00, jeremycho wrote:
> Rebased - PTAL when you can, but feel free to prioritize below more urgent
CLs.

lgtm for local_ntp_source.cc stuff.

[samarth, 2013-05-15 16:49:17]

Can you also rebase when you get a chance?

Thanks,
Samarth

https://codereview.chromium.org/14685004/diff/24001/chrome/browser/resources/...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/14685004/diff/24001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:1618: var recentTabsLink =
document.createElement('span');
Can this just be an <a href="chrome://history">? Of course the link won't work
but you could hijack the click handler and call navigateContentWindow. That way
things like hovering, etc will work better.

https://codereview.chromium.org/14685004/diff/24001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:1620:
recentTabsLink.addEventListener('click', function() {
Can you also add logic to look for a modifier and handle it appropriately?
Ctrl/command/middle-clicking should really do the right thing. Take a look at
getDispositionFromClickButton for a start.

[jeremycho, 2013-05-16 20:06:47]

Rebased.  PTAL - thanks!

https://codereview.chromium.org/14685004/diff/24001/chrome/browser/resources/...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/14685004/diff/24001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:1618: var recentTabsLink =
document.createElement('span');
On 2013/05/15 16:49:17, samarth wrote:
> Can this just be an <a href="chrome://history">? Of course the link won't work
> but you could hijack the click handler and call navigateContentWindow. That
way
> things like hovering, etc will work better.

It would add a non-functioning "Open link in new tab", etc. to the context menu,
so I'm not sure this is what we want.

https://codereview.chromium.org/14685004/diff/24001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:1620:
recentTabsLink.addEventListener('click', function() {
On 2013/05/15 16:49:17, samarth wrote:
> Can you also add logic to look for a modifier and handle it appropriately?
> Ctrl/command/middle-clicking should really do the right thing. Take a look at
> getDispositionFromClickButton for a start.

Done.

[jeremycho, 2013-06-18 21:45:54]

ping - we'd like to have this in for m29 if possible.  thanks!

[samarth, 2013-06-19 18:38:51]

Two comments:

1) The link is positioned strangely when themes are present. Sending you a
screenshot. (It's also a little too close to the most-visited even without
themes -- I suspect it looks weird because we moved everything down a bit)

2) We should tie the visibility of the link with that of the most-visited.
Otherwise, on initial load, you see it below the searchbox, and then it moves
down when the iframes are painted. (Alternatively, perhaps we can just make the
iframes invisible rather than display:none so they don't move stuff around. That
might still look strange.)

[jeremycho, 2013-06-19 21:43:35]

Thanks for testing.  The weird link positioning is because some attribution
images are tall with bottom whitespace.  To address this, I've added
vertical-align:top, which still looks a little funny, but at least it's
consistent across different attributions.

Re: Comment 2 - I don't think that should be an issue since the footer is
position:absolute.

[jeremycho, 2013-06-21 20:21:34]

friendly ping

[jeremycho, 2013-06-22 19:30:27]

Since Samarth is OOO, Jered would you be able to approve the remaining files? 
Thanks!

[Jered, 2013-06-24 17:42:26]

The position of this link looks really weird. It looks like it's anchored to the
bottom of my window, but it doesn't move when I scroll. It also overlaps most
visited tiles when I make my window narrow, and is on the wrong side for rtl.

How about borrowing some of the footer css from ntp4? I think they got it right.

https://codereview.chromium.org/14685004/diff/54001/chrome/browser/resources/...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/14685004/diff/54001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:368:
attribution.classList.add(CLASSES.HIDE_ATTRIBUTION);
nit: This change seems unnecessary in the context of this CL.

[jeremycho, 2013-06-24 20:11:53]

On 2013/06/24 17:42:26, Jered wrote:
> The position of this link looks really weird. It looks like it's anchored to
the
> bottom of my window, but it doesn't move when I scroll. It also overlaps most
> visited tiles when I make my window narrow, and is on the wrong side for rtl.
> 
> How about borrowing some of the footer css from ntp4? I think they got it
right.
> 
>
https://codereview.chromium.org/14685004/diff/54001/chrome/browser/resources/...
> File chrome/browser/resources/local_ntp/local_ntp.js (right):
> 
>
https://codereview.chromium.org/14685004/diff/54001/chrome/browser/resources/...
> chrome/browser/resources/local_ntp/local_ntp.js:368:
> attribution.classList.add(CLASSES.HIDE_ATTRIBUTION);
> nit: This change seems unnecessary in the context of this CL.

Thanks for trying it out.  Updated so that it remains fixed when scrolling (I'm
assuming that's what you actually meant?) and fixed RTL.  For the overlapping -
it's the current behavior of the attribution and can also happen in ntp4 (it's
just less obvious since the entire footer is a different color).

[jeremycho, 2013-06-24 20:12:09]

https://codereview.chromium.org/14685004/diff/54001/chrome/browser/resources/...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/14685004/diff/54001/chrome/browser/resources/...
chrome/browser/resources/local_ntp/local_ntp.js:368:
attribution.classList.add(CLASSES.HIDE_ATTRIBUTION);
On 2013/06/24 17:42:26, Jered wrote:
> nit: This change seems unnecessary in the context of this CL.

This is for setting display:none when no attribution is present.  With hidden,
it was still taking up space.

[Jered, 2013-06-24 20:51:47]

On 2013/06/24 20:12:09, jeremycho wrote:
>
https://codereview.chromium.org/14685004/diff/54001/chrome/browser/resources/...
> File chrome/browser/resources/local_ntp/local_ntp.js (right):
> 
>
https://codereview.chromium.org/14685004/diff/54001/chrome/browser/resources/...
> chrome/browser/resources/local_ntp/local_ntp.js:368:
> attribution.classList.add(CLASSES.HIDE_ATTRIBUTION);
> On 2013/06/24 17:42:26, Jered wrote:
> > nit: This change seems unnecessary in the context of this CL.
> 
> This is for setting display:none when no attribution is present.  With hidden,
> it was still taking up space.

1) This has trouble with some larger theme attribution images.
The link is floating off in the middle of MV tiles. For example, try
the "goodplanet" theme by Yann Arthus-Bertrand with a bunch of trees.

2) The link looks odd to me floating off in space. How about
something like
#recent-tabs {                                                                  
                                  
  color: #666;                                                                  
                                  
  cursor: pointer;                                                              
                                  
  vertical-align: top;                                                          
                                  
  border: 1px solid #c0c0c0;
  border-radius: 2px;                                                           
                                  
  padding: 3px;                                                                 
                                  
  background: #fff;                                                             
                                  
  opacity: 0.9;                                                                 
                                  
}

With the background/opacity, you wouldn't need to add a dropshadow
for themes, and it'd look less weird in small windows where the link
overlaps MV tiles.

[jeremycho, 2013-06-24 21:29:06]

On 2013/06/24 20:51:47, Jered wrote:
> On 2013/06/24 20:12:09, jeremycho wrote:
> >
>
https://codereview.chromium.org/14685004/diff/54001/chrome/browser/resources/...
> > File chrome/browser/resources/local_ntp/local_ntp.js (right):
> > 
> >
>
https://codereview.chromium.org/14685004/diff/54001/chrome/browser/resources/...
> > chrome/browser/resources/local_ntp/local_ntp.js:368:
> > attribution.classList.add(CLASSES.HIDE_ATTRIBUTION);
> > On 2013/06/24 17:42:26, Jered wrote:
> > > nit: This change seems unnecessary in the context of this CL.
> > 
> > This is for setting display:none when no attribution is present.  With
hidden,
> > it was still taking up space.
> 
> 1) This has trouble with some larger theme attribution images.
> The link is floating off in the middle of MV tiles. For example, try
> the "goodplanet" theme by Yann Arthus-Bertrand with a bunch of trees.

Updated to keep the link in the corner.

> 2) The link looks odd to me floating off in space. How about
> something like
> #recent-tabs {                                                                
 
>                                   
>   color: #666;                                                                
 
>                                   
>   cursor: pointer;                                                            
 
>                                   
>   vertical-align: top;                                                        
 
>                                   
>   border: 1px solid #c0c0c0;
>   border-radius: 2px;                                                         
 
>                                   
>   padding: 3px;                                                               
 
>                                   
>   background: #fff;                                                           
 
>                                   
>   opacity: 0.9;                                                               
 
>                                   
> }
> 
> With the background/opacity, you wouldn't need to add a dropshadow
> for themes, and it'd look less weird in small windows where the link
> overlaps MV tiles.

Done - the overlap does look better now.

[Jered, 2013-06-24 22:40:29]

"Theme created by" is still on top of MV tiles.
Does it have to be giant blue text? I'd prefer just leaving it the default text
color and size.

[jeremycho, 2013-06-24 23:35:51]

On 2013/06/24 22:40:29, Jered wrote:
> "Theme created by" is still on top of MV tiles.
> Does it have to be giant blue text? I'd prefer just leaving it the default
text
> color and size.

Updated to move the theme attribution behind MV tiles.  David requested the
styling to look like a large link.

[Jered, 2013-06-24 23:47:50]

On 2013/06/24 23:35:51, jeremycho wrote:
> On 2013/06/24 22:40:29, Jered wrote:
> > "Theme created by" is still on top of MV tiles.
> > Does it have to be giant blue text? I'd prefer just leaving it the default
> text
> > color and size.
> 
> Updated to move the theme attribution behind MV tiles.  David requested the
> styling to look like a large link.

lgtm

ok still looks cheesy but I think this is as good as we're going to
get here.

[commit-bot: I haz the power, 2013-06-25 02:51:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremycho@chromium.org/14685004/56002

[commit-bot: I haz the power, 2013-06-25 03:07:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremycho@chromium.org/14685004/56002

[commit-bot: I haz the power, 2013-06-25 03:13:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremycho@chromium.org/14685004/56002

[commit-bot: I haz the power, 2013-06-25 03:20:47]

Change committed as 208374