UI changes to support syncing foreign sessions. Changes were largely made to ...

UI changes to support syncing foreign sessions. Changes were largely made to the sync options and new tab page.

BUG=30519
TEST=None
 

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

[Nicolas Zea, 2010-10-14 23:31:48]

Arv, Thakis, I'm was hoping someone who knows JS/CSS/HTML better than I can
ensure I'm not doing things all wrong and you both had been making changes. Feel
free to punt over to someone else.

Backend changes to support the UI can be found here:
http://codereview.chromium.org/3825005/show

Note that session sync is not enabled without the --enable-sync-session chromium
switch, so nothing will be visible to the normal user yet.

[Nico, 2010-10-17 23:34:55]

Do you have  a screenshot somewhere?

http://codereview.chromium.org/3767009/diff/1/3
File chrome/browser/resources/new_new_tab.css (right):

http://codereview.chromium.org/3767009/diff/1/3#newcode238
chrome/browser/resources/new_new_tab.css:238: content: '\00bb'; /* raque gets
flipped automatically in rtl */
This needs to be \u00bb

http://codereview.chromium.org/3767009/diff/1/4
File chrome/browser/resources/new_new_tab.html (right):

http://codereview.chromium.org/3767009/diff/1/4#newcode73
chrome/browser/resources/new_new_tab.html:73: APPS: 1 << 6
*mark* (for later)

http://codereview.chromium.org/3767009/diff/1/4#newcode182
chrome/browser/resources/new_new_tab.html:182: <!-- Start disabled until sync is
enabled and foreign sessions are available
. at and of comment

http://codereview.chromium.org/3767009/diff/1/4#newcode287
chrome/browser/resources/new_new_tab.html:287:
updateSimpleSection('most-visited', Section.THUMB);
trailing whitespace

http://codereview.chromium.org/3767009/diff/1/4#newcode288
chrome/browser/resources/new_new_tab.html:288:
updateSimpleSection('foreign-sessions', Section.SYNC);
Section.SYNC isn't defined above (see *mark* above)

Also, can the sync section be minimized like the other sessions can?

http://codereview.chromium.org/3767009/diff/1/5
File chrome/browser/resources/new_new_tab.js (right):

http://codereview.chromium.org/3767009/diff/1/5#newcode129
chrome/browser/resources/new_new_tab.js:129: var parentSessEl =
sessionElement.lastElementChild;
no abbreviations

http://codereview.chromium.org/3767009/diff/1/5#newcode198
chrome/browser/resources/new_new_tab.js:198: // TODO(zea): Should there be a
clickHandler as well? We appear to be break-
no hyphenation in comments

http://codereview.chromium.org/3767009/diff/1/6
File chrome/browser/resources/options/sync_options.html (right):

http://codereview.chromium.org/3767009/diff/1/6#newcode1
chrome/browser/resources/options/sync_options.html:1: <div classs="page hidden"
id="syncPage">
huh?

http://codereview.chromium.org/3767009/diff/1/6#newcode63
chrome/browser/resources/options/sync_options.html:63: <td
class="option-name"><label><input id="sessions-check"
indented too far

http://codereview.chromium.org/3767009/diff/1/7
File chrome/browser/sync/resources/configure.html (right):

http://codereview.chromium.org/3767009/diff/1/7#newcode439
chrome/browser/sync/resources/configure.html:439: <!-- Sessions -->
indented too far

[Aaron Boodman, 2010-10-18 00:59:55]

http://codereview.chromium.org/3767009/diff/1/3
File chrome/browser/resources/new_new_tab.css (right):

http://codereview.chromium.org/3767009/diff/1/3#newcode205
chrome/browser/resources/new_new_tab.css:205: #foreign-sessions {
It seems like .miniview should already cover both of these.

http://codereview.chromium.org/3767009/diff/1/5
File chrome/browser/resources/new_new_tab.js (right):

http://codereview.chromium.org/3767009/diff/1/5#newcode150
chrome/browser/resources/new_new_tab.js:150: // Now go backwards and hide as
many elements as needed.
There should be no need to manually hide overflowing elements.
updateAllMiniviewClippings() should be doing that already.

[Nicolas Zea, 2010-10-18 20:06:39]

Addressed various comments. In addition, I've uploaded screenshots:
http://www.corp.google.com/~zea/session_sync.png
http://www.corp.google.com/~zea/session_sync_mouseover.png

http://codereview.chromium.org/3767009/diff/1/3
File chrome/browser/resources/new_new_tab.css (right):

http://codereview.chromium.org/3767009/diff/1/3#newcode205
chrome/browser/resources/new_new_tab.css:205: #foreign-sessions {
On 2010/10/18 00:59:55, Aaron Boodman wrote:
> It seems like .miniview should already cover both of these.

Done.

http://codereview.chromium.org/3767009/diff/1/3#newcode238
chrome/browser/resources/new_new_tab.css:238: content: '\00bb'; /* raque gets
flipped automatically in rtl */
On 2010/10/17 23:34:56, Nico wrote:
> This needs to be \u00bb

Done.

http://codereview.chromium.org/3767009/diff/1/4
File chrome/browser/resources/new_new_tab.html (right):

http://codereview.chromium.org/3767009/diff/1/4#newcode182
chrome/browser/resources/new_new_tab.html:182: <!-- Start disabled until sync is
enabled and foreign sessions are available
On 2010/10/17 23:34:56, Nico wrote:
> . at and of comment

Done.

http://codereview.chromium.org/3767009/diff/1/4#newcode287
chrome/browser/resources/new_new_tab.html:287:
updateSimpleSection('most-visited', Section.THUMB);
On 2010/10/17 23:34:56, Nico wrote:
> trailing whitespace

Done.

http://codereview.chromium.org/3767009/diff/1/4#newcode288
chrome/browser/resources/new_new_tab.html:288:
updateSimpleSection('foreign-sessions', Section.SYNC);
On 2010/10/17 23:34:56, Nico wrote:
> Section.SYNC isn't defined above (see *mark* above)
> 
> Also, can the sync section be minimized like the other sessions can?

Done (removed that line, it wasn't necessary). Currently the foreign sessions
section does not support being minimized. I plan to add that in a later patch.
For now I mainly want the basics working, since I'm sure there'll be further UI
changes.

http://codereview.chromium.org/3767009/diff/1/5
File chrome/browser/resources/new_new_tab.js (right):

http://codereview.chromium.org/3767009/diff/1/5#newcode129
chrome/browser/resources/new_new_tab.js:129: var parentSessEl =
sessionElement.lastElementChild;
On 2010/10/17 23:34:56, Nico wrote:
> no abbreviations

Done.

http://codereview.chromium.org/3767009/diff/1/5#newcode150
chrome/browser/resources/new_new_tab.js:150: // Now go backwards and hide as
many elements as needed.
On 2010/10/18 00:59:55, Aaron Boodman wrote:
> There should be no need to manually hide overflowing elements.
> updateAllMiniviewClippings() should be doing that already.

Done.

http://codereview.chromium.org/3767009/diff/1/5#newcode198
chrome/browser/resources/new_new_tab.js:198: // TODO(zea): Should there be a
clickHandler as well? We appear to be break-
On 2010/10/17 23:34:56, Nico wrote:
> no hyphenation in comments

Done.

http://codereview.chromium.org/3767009/diff/1/6
File chrome/browser/resources/options/sync_options.html (right):

http://codereview.chromium.org/3767009/diff/1/6#newcode1
chrome/browser/resources/options/sync_options.html:1: <div classs="page hidden"
id="syncPage">
On 2010/10/17 23:34:56, Nico wrote:
> huh?

Done.

http://codereview.chromium.org/3767009/diff/1/6#newcode63
chrome/browser/resources/options/sync_options.html:63: <td
class="option-name"><label><input id="sessions-check"
On 2010/10/17 23:34:56, Nico wrote:
> indented too far

Done.

http://codereview.chromium.org/3767009/diff/1/7
File chrome/browser/sync/resources/configure.html (right):

http://codereview.chromium.org/3767009/diff/1/7#newcode439
chrome/browser/sync/resources/configure.html:439: <!-- Sessions -->
On 2010/10/17 23:34:56, Nico wrote:
> indented too far

Done.

[arv (Not doing code reviews), 2010-10-18 22:22:51]

http://codereview.chromium.org/3767009/diff/10001/11001
File chrome/browser/resources/new_new_tab.css (right):

http://codereview.chromium.org/3767009/diff/10001/11001#newcode205
chrome/browser/resources/new_new_tab.css:205: .foreign_session_client {
Use - for class names and ids

http://codereview.chromium.org/3767009/diff/10001/11001#newcode206
chrome/browser/resources/new_new_tab.css:206: float: left;
rtl?

http://codereview.chromium.org/3767009/diff/10001/11001#newcode208
chrome/browser/resources/new_new_tab.css:208: font-family: Helvetica, Arial,
sans-serif;
Is this needed?

http://codereview.chromium.org/3767009/diff/10001/11003
File chrome/browser/resources/new_new_tab.js (right):

http://codereview.chromium.org/3767009/diff/10001/11003#newcode133
chrome/browser/resources/new_new_tab.js:133: var i=0;
whitespace

var i = 0;

http://codereview.chromium.org/3767009/diff/10001/11003#newcode135
chrome/browser/resources/new_new_tab.js:135: // TODO(zea): Get real client
names.
Can this be done in this CL without too much work? If not can you add a link to
the bug here?

http://codereview.chromium.org/3767009/diff/10001/11003#newcode136
chrome/browser/resources/new_new_tab.js:136: name = "Client " + i;
Use single quotes for consistency

http://codereview.chromium.org/3767009/diff/10001/11003#newcode160
chrome/browser/resources/new_new_tab.js:160: stack.setAttribute('class',
'foreign_session_client');
stack.className = ...

http://codereview.chromium.org/3767009/diff/10001/11003#newcode161
chrome/browser/resources/new_new_tab.js:161: client_name =
document.createElement('span');
missing var

http://codereview.chromium.org/3767009/diff/10001/11003#newcode161
chrome/browser/resources/new_new_tab.js:161: client_name =
document.createElement('span');
No underscores in js. variableNamesLikeThis

http://codereview.chromium.org/3767009/diff/10001/11003#newcode168
chrome/browser/resources/new_new_tab.js:168: var el;
var el = document.createElement(...);

http://codereview.chromium.org/3767009/diff/10001/11003#newcode179
chrome/browser/resources/new_new_tab.js:179: // Prepend a line break. TODO(zea):
should this be done differently?
Yes, this should be done using margin and block elements.

http://codereview.chromium.org/3767009/diff/10001/11003#newcode938
chrome/browser/resources/new_new_tab.js:938: return el.sessionId !== undefined;
return el.sessionId;

http://codereview.chromium.org/3767009/diff/10001/11003#newcode942
chrome/browser/resources/new_new_tab.js:942: e.preventDefault();
Can you add a comment what default action you are preventing?

http://codereview.chromium.org/3767009/diff/10001/11003#newcode944
chrome/browser/resources/new_new_tab.js:944: // HACK(arv): After the window
onblur event happens we get a mouseover event
Can you extract this into a function so that we do not have duplicate code?

[Nicolas Zea, 2010-10-19 00:26:50]

http://codereview.chromium.org/3767009/diff/10001/11001
File chrome/browser/resources/new_new_tab.css (right):

http://codereview.chromium.org/3767009/diff/10001/11001#newcode205
chrome/browser/resources/new_new_tab.css:205: .foreign_session_client {
On 2010/10/18 22:22:51, arv wrote:
> Use - for class names and ids

Done.

http://codereview.chromium.org/3767009/diff/10001/11001#newcode206
chrome/browser/resources/new_new_tab.css:206: float: left;
On 2010/10/18 22:22:51, arv wrote:
> rtl?

I'm doing this to ensure the boxes are aligned to the left of the screen.
They're created and added with appendChild, which I believe keeps them in order.
(they're in order, left to right, when I test it)

http://codereview.chromium.org/3767009/diff/10001/11001#newcode208
chrome/browser/resources/new_new_tab.css:208: font-family: Helvetica, Arial,
sans-serif;
On 2010/10/18 22:22:51, arv wrote:
> Is this needed?

Done.

http://codereview.chromium.org/3767009/diff/10001/11003
File chrome/browser/resources/new_new_tab.js (right):

http://codereview.chromium.org/3767009/diff/10001/11003#newcode133
chrome/browser/resources/new_new_tab.js:133: var i=0;
On 2010/10/18 22:22:51, arv wrote:
> whitespace
> 
> var i = 0;

Done.

http://codereview.chromium.org/3767009/diff/10001/11003#newcode135
chrome/browser/resources/new_new_tab.js:135: // TODO(zea): Get real client
names.
On 2010/10/18 22:22:51, arv wrote:
> Can this be done in this CL without too much work? If not can you add a link
to
> the bug here?

This needs further work. I've created a bug and referenced it though.

http://codereview.chromium.org/3767009/diff/10001/11003#newcode136
chrome/browser/resources/new_new_tab.js:136: name = "Client " + i;
On 2010/10/18 22:22:51, arv wrote:
> Use single quotes for consistency

Done.

http://codereview.chromium.org/3767009/diff/10001/11003#newcode160
chrome/browser/resources/new_new_tab.js:160: stack.setAttribute('class',
'foreign_session_client');
On 2010/10/18 22:22:51, arv wrote:
> stack.className = ...

Done.

http://codereview.chromium.org/3767009/diff/10001/11003#newcode161
chrome/browser/resources/new_new_tab.js:161: client_name =
document.createElement('span');
On 2010/10/18 22:22:51, arv wrote:
> missing var

Done.

http://codereview.chromium.org/3767009/diff/10001/11003#newcode161
chrome/browser/resources/new_new_tab.js:161: client_name =
document.createElement('span');
On 2010/10/18 22:22:51, arv wrote:
> No underscores in js. variableNamesLikeThis

Done.

http://codereview.chromium.org/3767009/diff/10001/11003#newcode168
chrome/browser/resources/new_new_tab.js:168: var el;
On 2010/10/18 22:22:51, arv wrote:
> var el = document.createElement(...);

Done.

http://codereview.chromium.org/3767009/diff/10001/11003#newcode179
chrome/browser/resources/new_new_tab.js:179: // Prepend a line break. TODO(zea):
should this be done differently?
On 2010/10/18 22:22:51, arv wrote:
> Yes, this should be done using margin and block elements.

Done.

http://codereview.chromium.org/3767009/diff/10001/11003#newcode938
chrome/browser/resources/new_new_tab.js:938: return el.sessionId !== undefined;
On 2010/10/18 22:22:51, arv wrote:
> return el.sessionId;
> 

Done.

http://codereview.chromium.org/3767009/diff/10001/11003#newcode942
chrome/browser/resources/new_new_tab.js:942: e.preventDefault();
On 2010/10/18 22:22:51, arv wrote:
> Can you add a comment what default action you are preventing?

Removed (I don't believe it was necessary; works without it).

http://codereview.chromium.org/3767009/diff/10001/11003#newcode944
chrome/browser/resources/new_new_tab.js:944: // HACK(arv): After the window
onblur event happens we get a mouseover event
On 2010/10/18 22:22:51, arv wrote:
> Can you extract this into a function so that we do not have duplicate code?

Done.

[arv (Not doing code reviews), 2010-10-19 17:39:43]

http://codereview.chromium.org/3767009/diff/10001/11001
File chrome/browser/resources/new_new_tab.css (right):

http://codereview.chromium.org/3767009/diff/10001/11001#newcode206
chrome/browser/resources/new_new_tab.css:206: float: left;
On 2010/10/19 00:26:50, nzea wrote:
> On 2010/10/18 22:22:51, arv wrote:
> > rtl?
> 
> I'm doing this to ensure the boxes are aligned to the left of the screen.
> They're created and added with appendChild, which I believe keeps them in
order.
> (they're in order, left to right, when I test it)

But if you use Arabic ui the should flow from right to left.

html[dir=rtl] .foreign-session-client {
  float: right;
}

http://codereview.chromium.org/3767009/diff/18001/19003
File chrome/browser/resources/new_new_tab.js (right):

http://codereview.chromium.org/3767009/diff/18001/19003#newcode134
chrome/browser/resources/new_new_tab.js:134: sessionItems.forEach(function(item)
{
oh, forEach passes the index as the second argument so you can get rid of var i
and i++

sessionItems.forEach(function(item, i) {
  var name = 'Client ' + i;
  parentSessionElement.appendChild(createForeignSession(item, name));
}

http://codereview.chromium.org/3767009/diff/18001/19003#newcode136
chrome/browser/resources/new_new_tab.js:136: name = 'Client ' + i;
missing var which means that you keep changing window.name.

http://codereview.chromium.org/3767009/diff/18001/19003#newcode161
chrome/browser/resources/new_new_tab.js:161: var clientName =
document.createElement('span');
Why do you need a span inside the div?

[Nicolas Zea, 2010-10-19 20:06:22]

http://codereview.chromium.org/3767009/diff/10001/11001
File chrome/browser/resources/new_new_tab.css (right):

http://codereview.chromium.org/3767009/diff/10001/11001#newcode206
chrome/browser/resources/new_new_tab.css:206: float: left;
On 2010/10/19 17:39:44, arv wrote:
> On 2010/10/19 00:26:50, nzea wrote:
> > On 2010/10/18 22:22:51, arv wrote:
> > > rtl?
> > 
> > I'm doing this to ensure the boxes are aligned to the left of the screen.
> > They're created and added with appendChild, which I believe keeps them in
> order.
> > (they're in order, left to right, when I test it)
> 
> But if you use Arabic ui the should flow from right to left.
> 
> html[dir=rtl] .foreign-session-client {
>   float: right;
> }

Ah, I see, thanks. Done.

http://codereview.chromium.org/3767009/diff/18001/19003
File chrome/browser/resources/new_new_tab.js (right):

http://codereview.chromium.org/3767009/diff/18001/19003#newcode134
chrome/browser/resources/new_new_tab.js:134: sessionItems.forEach(function(item)
{
On 2010/10/19 17:39:44, arv wrote:
> oh, forEach passes the index as the second argument so you can get rid of var
i
> and i++
> 
> sessionItems.forEach(function(item, i) {
>   var name = 'Client ' + i;
>   parentSessionElement.appendChild(createForeignSession(item, name));
> }
> 
> 

Done.

http://codereview.chromium.org/3767009/diff/18001/19003#newcode136
chrome/browser/resources/new_new_tab.js:136: name = 'Client ' + i;
On 2010/10/19 17:39:44, arv wrote:
> missing var which means that you keep changing window.name.

Done.

http://codereview.chromium.org/3767009/diff/18001/19003#newcode161
chrome/browser/resources/new_new_tab.js:161: var clientName =
document.createElement('span');
On 2010/10/19 17:39:44, arv wrote:
> Why do you need a span inside the div?

You're right, I don't. Removed

[arv (Not doing code reviews), 2010-10-19 20:46:11]

LGTM