[Sync] Split up about:sync html files to be more manageable

[Sync] Split up about:sync html files to be more manageable

Split up sync_index.html into chrome_sync.js and various included
html files.

Added simple Event and AsyncFunction classes to abstract out the grungy
WebUI communication details.

BUG=69500
TEST=

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

[akalin, 2011-02-17 20:11:25]

+johnnyg for review (I heard you know javascript)

[John Gregg, 2011-02-17 20:49:38]

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
File chrome/browser/resources/sync_internals/about.css (right):

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/about.css:6: 
nit: random blank lines between some selectors...

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/about.css:28: padding-top: 0.5em;
consolidate various paddings and margins

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/about.css:39: padding-left: 0;
consolidate

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
File chrome/browser/resources/sync_internals/about.html (right):

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/about.html:17: })();
why is it written this way with the function call?

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/about.html:28: <span class="desc"><h2>
Summary </h2></span>
nit: lots of stray spaces in this file

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/about.html:47: <h2>  </h2>
empty <h2>?

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/about.html:58: style="color:red"
class="err"

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
File chrome/browser/resources/sync_internals/chrome_sync.js (right):

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/chrome_sync.js:88: var callback =
this.callbacks_.shift();
i guess you assume that the DOMUI message handlers are going in order? 
otherwise if this is really async you could have callbacks invoked for the wrong
arguments.

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/chrome_sync.js:97:
chrome.sync.getAboutInfo_ = new AsyncFunction('getAboutInfo');
i think these are unnecessarily generic.  are the arguments really that
uncertain or would this work:
getAboutInfo = function(callback)
getNotificationState = function(callback)
getRootNode = function(callback)
getNodeById = function(id, callback)

then you don't need to worry about slicing arguments.

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
File chrome/browser/resources/sync_internals/notifications.html (right):

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/notifications.html:47: <tr
jsselect='notificationCounts'>
nit: be consistent with naming your jst variables between notifications.html and
about.html.  I'm not actually sure if this_style or thisStyle is right...

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
File chrome/browser/resources/sync_internals/sync_node_browser.html (right):

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/sync_node_browser.html:23: <li>ID: <span
jscontent='id'></span></li>
nit: maybe indent for readability

[akalin, 2011-02-18 00:36:22]

Addressed all comments, please take another look.

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
File chrome/browser/resources/sync_internals/about.css (right):

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/about.css:6: 
On 2011/02/17 20:49:38, John Gregg wrote:
> nit: random blank lines between some selectors...

Removed blank lines.

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/about.css:28: padding-top: 0.5em;
On 2011/02/17 20:49:38, John Gregg wrote:
> consolidate various paddings and margins

Done.

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/about.css:39: padding-left: 0;
On 2011/02/17 20:49:38, John Gregg wrote:
> consolidate

Done.

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
File chrome/browser/resources/sync_internals/about.html (right):

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/about.html:17: })();
On 2011/02/17 20:49:38, John Gregg wrote:
> why is it written this way with the function call?

This file is included by sync_index.html, so I wrap these functions in a closure
to avoid polluting the global namespace.

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/about.html:28: <span class="desc"><h2>
Summary </h2></span>
On 2011/02/17 20:49:38, John Gregg wrote:
> nit: lots of stray spaces in this file

Done.

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/about.html:47: <h2>  </h2>
On 2011/02/17 20:49:38, John Gregg wrote:
> empty <h2>?

Done.

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/about.html:58: style="color:red"
On 2011/02/17 20:49:38, John Gregg wrote:
> class="err"

Done.

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
File chrome/browser/resources/sync_internals/chrome_sync.js (right):

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/chrome_sync.js:88: var callback =
this.callbacks_.shift();
On 2011/02/17 20:49:38, John Gregg wrote:
> i guess you assume that the DOMUI message handlers are going in order? 
> otherwise if this is really async you could have callbacks invoked for the
wrong
> arguments.

Yeah, forgot to document that assumption.  Added comment.

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/chrome_sync.js:97:
chrome.sync.getAboutInfo_ = new AsyncFunction('getAboutInfo');
On 2011/02/17 20:49:38, John Gregg wrote:
> i think these are unnecessarily generic.  are the arguments really that
> uncertain or would this work:
> getAboutInfo = function(callback)
> getNotificationState = function(callback)
> getRootNode = function(callback)
> getNodeById = function(id, callback)
> 
> then you don't need to worry about slicing arguments.  

Done.

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
File chrome/browser/resources/sync_internals/sync_node_browser.html (right):

http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
chrome/browser/resources/sync_internals/sync_node_browser.html:23: <li>ID: <span
jscontent='id'></span></li>
On 2011/02/17 20:49:38, John Gregg wrote:
> nit: maybe indent for readability

Done.

[John Gregg, 2011-02-18 00:41:51]

LGTM

On 2011/02/18 00:36:22, akalin wrote:
> Addressed all comments, please take another look.
> 
>
http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
> File chrome/browser/resources/sync_internals/about.css (right):
> 
>
http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
> chrome/browser/resources/sync_internals/about.css:6: 
> On 2011/02/17 20:49:38, John Gregg wrote:
> > nit: random blank lines between some selectors...
> 
> Removed blank lines.
> 
>
http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
> chrome/browser/resources/sync_internals/about.css:28: padding-top: 0.5em;
> On 2011/02/17 20:49:38, John Gregg wrote:
> > consolidate various paddings and margins
> 
> Done.
> 
>
http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
> chrome/browser/resources/sync_internals/about.css:39: padding-left: 0;
> On 2011/02/17 20:49:38, John Gregg wrote:
> > consolidate
> 
> Done.
> 
>
http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
> File chrome/browser/resources/sync_internals/about.html (right):
> 
>
http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
> chrome/browser/resources/sync_internals/about.html:17: })();
> On 2011/02/17 20:49:38, John Gregg wrote:
> > why is it written this way with the function call?
> 
> This file is included by sync_index.html, so I wrap these functions in a
closure
> to avoid polluting the global namespace.
> 
>
http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
> chrome/browser/resources/sync_internals/about.html:28: <span class="desc"><h2>
> Summary </h2></span>
> On 2011/02/17 20:49:38, John Gregg wrote:
> > nit: lots of stray spaces in this file
> 
> Done.
> 
>
http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
> chrome/browser/resources/sync_internals/about.html:47: <h2>  </h2>
> On 2011/02/17 20:49:38, John Gregg wrote:
> > empty <h2>?
> 
> Done.
> 
>
http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
> chrome/browser/resources/sync_internals/about.html:58: style="color:red"
> On 2011/02/17 20:49:38, John Gregg wrote:
> > class="err"
> 
> Done.
> 
>
http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
> File chrome/browser/resources/sync_internals/chrome_sync.js (right):
> 
>
http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
> chrome/browser/resources/sync_internals/chrome_sync.js:88: var callback =
> this.callbacks_.shift();
> On 2011/02/17 20:49:38, John Gregg wrote:
> > i guess you assume that the DOMUI message handlers are going in order? 
> > otherwise if this is really async you could have callbacks invoked for the
> wrong
> > arguments.
> 
> Yeah, forgot to document that assumption.  Added comment.
> 
>
http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
> chrome/browser/resources/sync_internals/chrome_sync.js:97:
> chrome.sync.getAboutInfo_ = new AsyncFunction('getAboutInfo');
> On 2011/02/17 20:49:38, John Gregg wrote:
> > i think these are unnecessarily generic.  are the arguments really that
> > uncertain or would this work:
> > getAboutInfo = function(callback)
> > getNotificationState = function(callback)
> > getRootNode = function(callback)
> > getNodeById = function(id, callback)
> > 
> > then you don't need to worry about slicing arguments.  
> 
> Done.
> 
>
http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
> File chrome/browser/resources/sync_internals/sync_node_browser.html (right):
> 
>
http://codereview.chromium.org/6531038/diff/1/chrome/browser/resources/sync_i...
> chrome/browser/resources/sync_internals/sync_node_browser.html:23: <li>ID:
<span
> jscontent='id'></span></li>
> On 2011/02/17 20:49:38, John Gregg wrote:
> > nit: maybe indent for readability
> 
> Done.