Show ExtensionID->OriginSyncStatus in syncfs-internals WebUI

Show ExtensionID->OriginSyncStatus in syncfs-internals WebUI.

BUG=244067
TEST=Manual
Sign into Chrome
Start any SyncFS application
Go to chrome://syncfs-internals, click on Extension Status tab
You should see at lease one extension along with its sync status.

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

[calvinlo, 2013-06-07 05:52:46]

PTAL. It's about the same as before but now uses the landed
DriveFileSyncService.GetOriginStatusMap() function.

Updated screenshot attached to the bug issue.

[nhiroki, 2013-06-07 06:31:25]

Looks good.

https://codereview.chromium.org/16398011/diff/2003/chrome/browser/resources/s...
File chrome/browser/resources/sync_file_system_internals/extension_statuses.html
(right):

https://codereview.chromium.org/16398011/diff/2003/chrome/browser/resources/s...
chrome/browser/resources/sync_file_system_internals/extension_statuses.html:12: 
nit: Can you remove this blank line?

https://codereview.chromium.org/16398011/diff/2003/chrome/browser/ui/webui/sy...
File
chrome/browser/ui/webui/sync_file_system_internals/extension_statuses_handler.cc
(right):

https://codereview.chromium.org/16398011/diff/2003/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_file_system_internals/extension_statuses_handler.cc:37:
const base::ListValue* args) {
Can you add "DCHECK(args);"?

https://codereview.chromium.org/16398011/diff/2003/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_file_system_internals/extension_statuses_handler.cc:46:
base::DictionaryValue* dict = new DictionaryValue;
nit: indent

https://codereview.chromium.org/16398011/diff/2003/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_file_system_internals/extension_statuses_handler.cc:52:
LOG(ERROR) << "GOT TO HERE list.size=" << status_map.size();
Is this for debugging?

[calvinlo, 2013-06-07 06:40:28]

https://codereview.chromium.org/16398011/diff/2003/chrome/browser/resources/s...
File chrome/browser/resources/sync_file_system_internals/extension_statuses.html
(right):

https://codereview.chromium.org/16398011/diff/2003/chrome/browser/resources/s...
chrome/browser/resources/sync_file_system_internals/extension_statuses.html:12: 
On 2013/06/07 06:31:25, nhiroki wrote:
> nit: Can you remove this blank line?

Done.

https://codereview.chromium.org/16398011/diff/2003/chrome/browser/ui/webui/sy...
File
chrome/browser/ui/webui/sync_file_system_internals/extension_statuses_handler.cc
(right):

https://codereview.chromium.org/16398011/diff/2003/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_file_system_internals/extension_statuses_handler.cc:37:
const base::ListValue* args) {
On 2013/06/07 06:31:25, nhiroki wrote:
> Can you add "DCHECK(args);"?

Done.

https://codereview.chromium.org/16398011/diff/2003/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_file_system_internals/extension_statuses_handler.cc:46:
base::DictionaryValue* dict = new DictionaryValue;
On 2013/06/07 06:31:25, nhiroki wrote:
> nit: indent

Done.

https://codereview.chromium.org/16398011/diff/2003/chrome/browser/ui/webui/sy...
chrome/browser/ui/webui/sync_file_system_internals/extension_statuses_handler.cc:52:
LOG(ERROR) << "GOT TO HERE list.size=" << status_map.size();
On 2013/06/07 06:31:25, nhiroki wrote:
> Is this for debugging?

Done. Sorry, forgot to take this out.

[calvinlo, 2013-06-07 06:48:13]

+bauerb for chrome/browser/resources/sync_file_system_internals_resources.grd
OWNERS review.

Please take a look at actual code too if you have time. Our team has OWNERS file
ownership for the /sync_file_system_internals subdirectories now but an extra
review is also welcome.

Thanks!

[nhiroki, 2013-06-07 10:34:03]

+tzik@ for double checking.

[Bernhard Bauer, 2013-06-07 10:44:46]

https://codereview.chromium.org/16398011/diff/21001/chrome/browser/resources/...
File chrome/browser/resources/sync_file_system_internals/extension_statuses.js
(right):

https://codereview.chromium.org/16398011/diff/21001/chrome/browser/resources/...
chrome/browser/resources/sync_file_system_internals/extension_statuses.js:11:
function extensionStatuses() {
Class names should start upper-case.

https://codereview.chromium.org/16398011/diff/21001/chrome/browser/resources/...
chrome/browser/resources/sync_file_system_internals/extension_statuses.js:55:
document.addEventListener('DOMContentLoaded', main);
You could directly add getExtensionStatuses here.

https://codereview.chromium.org/16398011/diff/21001/chrome/browser/resources/...
chrome/browser/resources/sync_file_system_internals/extension_statuses.js:56:
return new extensionStatuses;
Do you actually need an instance of this class here? You could directly declare
a static method ExtensionStatuses.onGetExtensionStatuses and call that.

https://codereview.chromium.org/16398011/diff/21001/chrome/browser/resources/...
File chrome/browser/resources/sync_file_system_internals_resources.grd (right):

https://codereview.chromium.org/16398011/diff/21001/chrome/browser/resources/...
chrome/browser/resources/sync_file_system_internals_resources.grd:15: <include
name="IDR_SYNC_FILE_SYSTEM_INTERNALS_EXTENSION_STATUSES_JS"
file="sync_file_system_internals/extension_statuses.js" type="BINDATA" />
It probably would make sense to add the same OWNERS as
chrome/browser/resources/sync_file_system_internals/ for this file.

https://codereview.chromium.org/16398011/diff/21001/chrome/browser/ui/webui/s...
File
chrome/browser/ui/webui/sync_file_system_internals/extension_statuses_handler.h
(right):

https://codereview.chromium.org/16398011/diff/21001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_file_system_internals/extension_statuses_handler.h:23:
virtual void RegisterMessages() OVERRIDE;
Can you add a comment that states what interface this implements?

[tzik, 2013-06-10 02:41:17]

lgtm

[calvinlo, 2013-06-10 05:15:45]

https://codereview.chromium.org/16398011/diff/21001/chrome/browser/resources/...
File chrome/browser/resources/sync_file_system_internals/extension_statuses.js
(right):

https://codereview.chromium.org/16398011/diff/21001/chrome/browser/resources/...
chrome/browser/resources/sync_file_system_internals/extension_statuses.js:11:
function extensionStatuses() {
On 2013/06/07 10:44:46, Bernhard Bauer wrote:
> Class names should start upper-case.

Done.

https://codereview.chromium.org/16398011/diff/21001/chrome/browser/resources/...
chrome/browser/resources/sync_file_system_internals/extension_statuses.js:55:
document.addEventListener('DOMContentLoaded', main);
On 2013/06/07 10:44:46, Bernhard Bauer wrote:
> You could directly add getExtensionStatuses here.

Done.

https://codereview.chromium.org/16398011/diff/21001/chrome/browser/resources/...
chrome/browser/resources/sync_file_system_internals/extension_statuses.js:56:
return new extensionStatuses;
On 2013/06/07 10:44:46, Bernhard Bauer wrote:
> Do you actually need an instance of this class here? You could directly
declare
> a static method ExtensionStatuses.onGetExtensionStatuses and call that.

You're right, this could just be an exported static function. I originally
wanted to do that but my TL insisted before (for sync_service.js) that a whole
object be returned. Since that sync_service.js already declares functions using
the prototyping syntax and returns whole objects, I think it's good to keep this
the same way so all the. js files have the same style?

https://codereview.chromium.org/16398011/diff/21001/chrome/browser/resources/...
File chrome/browser/resources/sync_file_system_internals_resources.grd (right):

https://codereview.chromium.org/16398011/diff/21001/chrome/browser/resources/...
chrome/browser/resources/sync_file_system_internals_resources.grd:15: <include
name="IDR_SYNC_FILE_SYSTEM_INTERNALS_EXTENSION_STATUSES_JS"
file="sync_file_system_internals/extension_statuses.js" type="BINDATA" />
On 2013/06/07 10:44:46, Bernhard Bauer wrote:
> It probably would make sense to add the same OWNERS as
> chrome/browser/resources/sync_file_system_internals/ for this file.

Done. I've added per-file sync_file_system_internals.*=tzik@chromium.org to give
ownership to my teammate who should be the final owner of
sync_file_system_internals/

https://codereview.chromium.org/16398011/diff/21001/chrome/browser/ui/webui/s...
File
chrome/browser/ui/webui/sync_file_system_internals/extension_statuses_handler.h
(right):

https://codereview.chromium.org/16398011/diff/21001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_file_system_internals/extension_statuses_handler.h:23:
virtual void RegisterMessages() OVERRIDE;
On 2013/06/07 10:44:46, Bernhard Bauer wrote:
> Can you add a comment that states what interface this implements?

Done.

[nhiroki, 2013-06-10 09:31:51]

lgtm

https://codereview.chromium.org/16398011/diff/74001/chrome/browser/resources/...
File chrome/browser/resources/OWNERS (right):

https://codereview.chromium.org/16398011/diff/74001/chrome/browser/resources/...
chrome/browser/resources/OWNERS:12: per-file
sync_file_system_internals.*=tzik@chromium.org
nit: Sorting.

nit: I'd prefer to push this into a separate CL.

https://codereview.chromium.org/16398011/diff/74001/chrome/browser/ui/webui/s...
File
chrome/browser/ui/webui/sync_file_system_internals/extension_statuses_handler.h
(right):

https://codereview.chromium.org/16398011/diff/74001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_file_system_internals/extension_statuses_handler.h:33:
};
nit: Please add a blank line here.

https://codereview.chromium.org/16398011/diff/74001/chrome/browser/ui/webui/s...
File
chrome/browser/ui/webui/sync_file_system_internals/sync_file_system_internals_handler.h
(right):

https://codereview.chromium.org/16398011/diff/74001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_file_system_internals/sync_file_system_internals_handler.h:38:
};
nit: Please add a blank line here.

[calvinlo, 2013-06-10 09:52:38]

https://codereview.chromium.org/16398011/diff/74001/chrome/browser/resources/...
File chrome/browser/resources/OWNERS (right):

https://codereview.chromium.org/16398011/diff/74001/chrome/browser/resources/...
chrome/browser/resources/OWNERS:12: per-file
sync_file_system_internals.*=tzik@chromium.org
On 2013/06/10 09:31:51, nhiroki wrote:
> nit: Sorting.
> 
> nit: I'd prefer to push this into a separate CL.

Done. Sent as part of https://codereview.chromium.org/15703008/ separately.

https://codereview.chromium.org/16398011/diff/74001/chrome/browser/ui/webui/s...
File
chrome/browser/ui/webui/sync_file_system_internals/extension_statuses_handler.h
(right):

https://codereview.chromium.org/16398011/diff/74001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_file_system_internals/extension_statuses_handler.h:33:
};
On 2013/06/10 09:31:51, nhiroki wrote:
> nit: Please add a blank line here.

Done.

[Bernhard Bauer, 2013-06-10 12:46:35]

https://codereview.chromium.org/16398011/diff/51004/chrome/browser/ui/webui/s...
File
chrome/browser/ui/webui/sync_file_system_internals/extension_statuses_handler.cc
(right):

https://codereview.chromium.org/16398011/diff/51004/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_file_system_internals/extension_statuses_handler.cc:53:
web_ui()->CallJavascriptFunction("ExtensionStatuses.onGetExtensionStatuses",
Wait, you're still exporting a variable called "extensionStatuses" (lower case).
Does this work?

[kinuko, 2013-06-10 13:42:37]

On 2013/06/10 05:15:45, calvinlo wrote:
>
https://codereview.chromium.org/16398011/diff/21001/chrome/browser/resources/...
> File chrome/browser/resources/sync_file_system_internals/extension_statuses.js
> (right):
> 
>
https://codereview.chromium.org/16398011/diff/21001/chrome/browser/resources/...
> chrome/browser/resources/sync_file_system_internals/extension_statuses.js:11:
> function extensionStatuses() {
> On 2013/06/07 10:44:46, Bernhard Bauer wrote:
> > Class names should start upper-case.
> 
> Done.
> 
>
https://codereview.chromium.org/16398011/diff/21001/chrome/browser/resources/...
> chrome/browser/resources/sync_file_system_internals/extension_statuses.js:55:
> document.addEventListener('DOMContentLoaded', main);
> On 2013/06/07 10:44:46, Bernhard Bauer wrote:
> > You could directly add getExtensionStatuses here.
> 
> Done.
> 
>
https://codereview.chromium.org/16398011/diff/21001/chrome/browser/resources/...
> chrome/browser/resources/sync_file_system_internals/extension_statuses.js:56:
> return new extensionStatuses;
> On 2013/06/07 10:44:46, Bernhard Bauer wrote:
> > Do you actually need an instance of this class here? You could directly
> declare
> > a static method ExtensionStatuses.onGetExtensionStatuses and call that.
> 
> You're right, this could just be an exported static function. I originally
> wanted to do that but my TL insisted before (for sync_service.js) that a whole
> object be returned. Since that sync_service.js already declares functions
using
> the prototyping syntax and returns whole objects, I think it's good to keep
this
> the same way so all the. js files have the same style?

I think what he's suggesting is doing something like following, which is
slightly different from your previous code.
(@bauerb please let me know if I'm wrong)

ExtensionStatuses.onGetExtensionStatuses = function(...) { ... };

document.addEventListener('DOMContentLoaded', getExtensionStatuses);
 return ExtensionStatuses;
})();

And I agree that not creating an instance is better than what I suggested.
Anyway either way (keeping it as is or revising it to use static methods) works
for me.

>
https://codereview.chromium.org/16398011/diff/21001/chrome/browser/resources/...
> File chrome/browser/resources/sync_file_system_internals_resources.grd
(right):
> 
>
https://codereview.chromium.org/16398011/diff/21001/chrome/browser/resources/...
> chrome/browser/resources/sync_file_system_internals_resources.grd:15: <include
> name="IDR_SYNC_FILE_SYSTEM_INTERNALS_EXTENSION_STATUSES_JS"
> file="sync_file_system_internals/extension_statuses.js" type="BINDATA" />
> On 2013/06/07 10:44:46, Bernhard Bauer wrote:
> > It probably would make sense to add the same OWNERS as
> > chrome/browser/resources/sync_file_system_internals/ for this file.
> 
> Done. I've added per-file
mailto:sync_file_system_internals.*=tzik@chromium.org to give
> ownership to my teammate who should be the final owner of
> sync_file_system_internals/
> 
>
https://codereview.chromium.org/16398011/diff/21001/chrome/browser/ui/webui/s...
> File
>
chrome/browser/ui/webui/sync_file_system_internals/extension_statuses_handler.h
> (right):
> 
>
https://codereview.chromium.org/16398011/diff/21001/chrome/browser/ui/webui/s...
>
chrome/browser/ui/webui/sync_file_system_internals/extension_statuses_handler.h:23:
> virtual void RegisterMessages() OVERRIDE;
> On 2013/06/07 10:44:46, Bernhard Bauer wrote:
> > Can you add a comment that states what interface this implements?
> 
> Done.

[calvinlo, 2013-06-11 14:34:15]

I rebased and then fixed up the JavaScript based on the given suggestion. Thanks
for that. Please also note that I added a refresh button to make it easier for
me to manually test. I manually tested the latest version just to make sure this
works, sorry about the casing typo before.

https://codereview.chromium.org/16398011/diff/51004/chrome/browser/ui/webui/s...
File
chrome/browser/ui/webui/sync_file_system_internals/extension_statuses_handler.cc
(right):

https://codereview.chromium.org/16398011/diff/51004/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/sync_file_system_internals/extension_statuses_handler.cc:53:
web_ui()->CallJavascriptFunction("ExtensionStatuses.onGetExtensionStatuses",
On 2013/06/10 12:46:35, Bernhard Bauer wrote:
> Wait, you're still exporting a variable called "extensionStatuses" (lower
case).
> Does this work?

Sorry typo. Fixed.

[calvinlo, 2013-06-12 10:04:15]

Sorry to bug you again Bernhard, just wondering if you wanted to take a look at
this again before I try it in the CQ. I have enough owners to cover all the
patch files but I wanted to respect your original review and get your LGTM
before putting in the final version.

Thanks.

[Bernhard Bauer, 2013-06-12 10:53:31]

https://codereview.chromium.org/16398011/diff/100001/chrome/browser/resources...
File chrome/browser/resources/sync_file_system_internals/extension_statuses.js
(right):

https://codereview.chromium.org/16398011/diff/100001/chrome/browser/resources...
chrome/browser/resources/sync_file_system_internals/extension_statuses.js:8: var
extensionStatuses = (function() {
Sorry if I'm being nitpicky here, but you're conceptually exporting a class with
a static method, so it should be exported in upper case, i.e. ExtensionStatuses.

[calvinlo, 2013-06-12 11:40:22]

https://codereview.chromium.org/16398011/diff/100001/chrome/browser/resources...
File chrome/browser/resources/sync_file_system_internals/extension_statuses.js
(right):

https://codereview.chromium.org/16398011/diff/100001/chrome/browser/resources...
chrome/browser/resources/sync_file_system_internals/extension_statuses.js:8: var
extensionStatuses = (function() {
On 2013/06/12 10:53:32, Bernhard Bauer wrote:
> Sorry if I'm being nitpicky here, but you're conceptually exporting a class
with
> a static method, so it should be exported in upper case, i.e.
ExtensionStatuses.

Per offline conversation, I was just initially following what Kinuko had done in
this patch
https://codereview.chromium.org/16404008/diff/19001/chrome/browser/resources/....
Per offline chat, that was missed in the review so I've now upper cased it in
both files and also updated the handlers to call the class in uppercase.

[Bernhard Bauer, 2013-06-13 07:47:31]

LGTM, thanks!

[commit-bot: I haz the power, 2013-06-13 07:50:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calvinlo@chromium.org/16398011/110001

[commit-bot: I haz the power, 2013-06-13 08:12:05]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-13 09:53:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calvinlo@chromium.org/16398011/144001

[commit-bot: I haz the power, 2013-06-13 10:07:31]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-14 05:11:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calvinlo@chromium.org/16398011/159001

[commit-bot: I haz the power, 2013-06-14 07:18:33]

Change committed as 206351