filemanager: Fix GetDriveConnectionStateFunctionChange to return "offline" when DriveSystemService …

filemanager: Fix GetDriveConnectionStateFunctionChange to return "offline" when DriveSystemService is unavailable.

Connection-type "offline" is returned with reason "no_service"
Also renamed related variables in JS.

BUG=175522
TEST=Verfify Files app (including "open file", "save file" dialogs) is working if Drive is disblaed, in Incognito window, in Guest mode.

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

[Haruki Sato, 2013-02-12 05:50:18]

As talked offline, implemented "no_service" case for
GetDriveConnectionStateFunction.

could you take a look?
Thanks.

[yoshiki, 2013-02-12 06:01:24]

lgtm, thanks!

[Haruki Sato, 2013-02-12 06:04:46]

Hashimoto-san, could you OWNER review for chrome/browser/chromeos/extensions/ ?

Thanks.

[hashimoto, 2013-02-12 06:12:29]

lgtm with a nit

https://codereview.chromium.org/12221132/diff/1001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

https://codereview.chromium.org/12221132/diff/1001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:3107:
reasons->AppendString(kDriveConnectionReasonNoService);
nit: Shouldn't we include "no_network" in |reasons| if
net::NetworkChangedNotifier::IsOffline()==true?

[yoshiki, 2013-02-12 06:29:15]

sorry, please fix a nit.

https://codereview.chromium.org/12221132/diff/7001/chrome/browser/resources/f...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://codereview.chromium.org/12221132/diff/7001/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/file_manager.js:131: NO_SERVICE:
'no_service',  // Drive service is unavailable.
nit: Please remove the last comma.

[Haruki Sato, 2013-02-12 06:34:07]

Fixed the commented issues. Thank you for the review!

https://codereview.chromium.org/12221132/diff/1001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

https://codereview.chromium.org/12221132/diff/1001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:3107:
reasons->AppendString(kDriveConnectionReasonNoService);
On 2013/02/12 06:12:29, hashimoto wrote:
> nit: Shouldn't we include "no_network" in |reasons| if
> net::NetworkChangedNotifier::IsOffline()==true?
That would be more consistent. We can and I added.
Although we don't have a code to handle reasons in JS, so it would not have any
effective change in UI.

https://codereview.chromium.org/12221132/diff/7001/chrome/browser/resources/f...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://codereview.chromium.org/12221132/diff/7001/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/file_manager.js:131: NO_SERVICE:
'no_service',  // Drive service is unavailable.
On 2013/02/12 06:29:15, yoshiki wrote:
> nit: Please remove the last comma.

Done with a padding fix.
Thanks.

[hashimoto, 2013-02-12 06:42:05]

One more request, thanks!

https://codereview.chromium.org/12221132/diff/6002/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

https://codereview.chromium.org/12221132/diff/6002/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:3093: if
(net::NetworkChangeNotifier::IsOffline())
nit: It seems redundant to have two IsOffline() checks here and below.
How about modifying |ready|'s value to 'system_service &&
system_service->drive_service()->CanStartOperation()' to avoid code duplication?

[Haruki Sato, 2013-02-12 07:28:49]

https://codereview.chromium.org/12221132/diff/6002/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right):

https://codereview.chromium.org/12221132/diff/6002/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/file_browser_private_api.cc:3093: if
(net::NetworkChangeNotifier::IsOffline())
On 2013/02/12 06:42:05, hashimoto wrote:
> nit: It seems redundant to have two IsOffline() checks here and below.
> How about modifying |ready|'s value to 'system_service &&
> system_service->drive_service()->CanStartOperation()' to avoid code
duplication?

Good point!
Refactored as suggested. and it's now clearer that |reasons| actually explains
why connection is "offline".

[hashimoto, 2013-02-12 07:42:57]

lgtm, thanks

[commit-bot: I haz the power, 2013-02-12 07:44:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haruki@chromium.org/12221132/11001

[commit-bot: I haz the power, 2013-02-12 08:50:18]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-02-12 09:01:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haruki@chromium.org/12221132/11001

[commit-bot: I haz the power, 2013-02-12 11:21:40]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-02-12 13:17:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haruki@chromium.org/12221132/11001

[commit-bot: I haz the power, 2013-02-12 15:43:07]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-02-13 04:36:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haruki@chromium.org/12221132/11001

[commit-bot: I haz the power, 2013-02-13 19:35:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haruki@chromium.org/12221132/11001