Postpone connecting to GData even more, provide progress indication.

Postpone connecting to GData even more, provide progress indication.


BUG=chromium-os:28417
TEST=


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

[Vladislav Kaznacheev, 2012-03-26 16:11:38]

Please review.

[dgozman, 2012-03-26 16:30:21]

LGTM with nits

https://chromiumcodereview.appspot.com/9855024/diff/1/chrome/browser/resource...
File chrome/browser/resources/file_manager/css/file_manager.css (right):

https://chromiumcodereview.appspot.com/9855024/diff/1/chrome/browser/resource...
chrome/browser/resources/file_manager/css/file_manager.css:1140: padding-left:
30px;
Fun stuff :)

https://chromiumcodereview.appspot.com/9855024/diff/1/chrome/browser/resource...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://chromiumcodereview.appspot.com/9855024/diff/1/chrome/browser/resource...
chrome/browser/resources/file_manager/js/file_manager.js:807:
FileManager.prototype.initGData_ = function(retry) {
|retry| is not a meaningful name. Does it mean we will retry later, did a retry
before, or just this attempt is a retry?

I think, the better name is |isRetry| or |firstTime|.

https://chromiumcodereview.appspot.com/9855024/diff/1/chrome/browser/resource...
chrome/browser/resources/file_manager/js/file_manager.js:846:
FileManager.prototype.initUnmountedPanel_ = function() {
Consider not so generic |gdataPanel| or |gdataNotMountedPanel|.

[Vladislav Kaznacheev, 2012-03-27 09:25:34]

https://chromiumcodereview.appspot.com/9855024/diff/1/chrome/browser/resource...
File chrome/browser/resources/file_manager/css/file_manager.css (right):

https://chromiumcodereview.appspot.com/9855024/diff/1/chrome/browser/resource...
chrome/browser/resources/file_manager/css/file_manager.css:1140: padding-left:
30px;
Yeah. I added a comment explaining the purpose.
On 2012/03/26 16:30:21, dgozman wrote:
> Fun stuff :)

https://chromiumcodereview.appspot.com/9855024/diff/1/chrome/browser/resource...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://chromiumcodereview.appspot.com/9855024/diff/1/chrome/browser/resource...
chrome/browser/resources/file_manager/js/file_manager.js:807:
FileManager.prototype.initGData_ = function(retry) {
changed to |dirChanged|
On 2012/03/26 16:30:21, dgozman wrote:
> |retry| is not a meaningful name. Does it mean we will retry later, did a
retry
> before, or just this attempt is a retry?
> 
> I think, the better name is |isRetry| or |firstTime|.

https://chromiumcodereview.appspot.com/9855024/diff/1/chrome/browser/resource...
chrome/browser/resources/file_manager/js/file_manager.js:846:
FileManager.prototype.initUnmountedPanel_ = function() {
On 2012/03/26 16:30:21, dgozman wrote:
> Consider not so generic |gdataPanel| or |gdataNotMountedPanel|.

Done.

[commit-bot: I haz the power, 2012-03-27 10:02:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaznacheev@chromium.org/9855024/5001

[commit-bot: I haz the power, 2012-03-27 12:29:46]

Try job failure for 9855024-5001 (retry) on win_rel for steps "ui_tests,
browser_tests".
It's a second try, previously, steps "ui_tests, browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Vladislav Kaznacheev, 2012-03-28 09:44:35]

Please review. This one is pretty tricky.

[dgozman, 2012-03-28 10:28:29]

https://chromiumcodereview.appspot.com/9855024/diff/11001/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/directory_model.js (right):

https://chromiumcodereview.appspot.com/9855024/diff/11001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/directory_model.js:587:
opt_pathResolveCallback(baseName, leafName, exists && !overridden);
Why do we call this in the case of overridden?

https://chromiumcodereview.appspot.com/9855024/diff/11001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/directory_model.js:590: var
changeDirectoryEntry = function (entry, callback, initial, exists) {
space before (

https://chromiumcodereview.appspot.com/9855024/diff/11001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/directory_model.js:609:
changeDirectoryEntry(this.root_, autoSelect, INITIAL, !EXISTS);
resolveCallback was not called here before. Bug?

https://chromiumcodereview.appspot.com/9855024/diff/11001/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://chromiumcodereview.appspot.com/9855024/diff/11001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/file_manager.js:565:
this.setupGDataDirectoryAfterMount_ = function() {
Can we do the same trick as in DirectoryModel: add listener and then remove it?

https://chromiumcodereview.appspot.com/9855024/diff/11001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/file_manager.js:845: if
(this.gdataLoadingTimer_)
How is this possible?

https://chromiumcodereview.appspot.com/9855024/diff/11001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/file_manager.js:1623:
this.directoryModel_.setupDefaultPath();
We ignore blankWhileLoading since default path is never gdata?
This makes generic name blankWhileLoading really pointless.
And a comment would be nice anyway.

https://chromiumcodereview.appspot.com/9855024/diff/11001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/file_manager.js:2606:
self.alert.showHtml(
Callback is bound below.

https://chromiumcodereview.appspot.com/9855024/diff/11001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/file_manager.js:2688: // Not calling
clearGDataLoadingTimer_ here because we want to keep
Is the following possible?
- requesting gdata mount
- it mount successfully
- starting rescan
- timer fires, we show error message (problem here)
- rescan complete, we show contents

https://chromiumcodereview.appspot.com/9855024/diff/11001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/file_manager.js:2690: // the timer
fires after the mount because onDirectoryChanged_will hide
typo: onDirectoryChanged_will

https://chromiumcodereview.appspot.com/9855024/diff/11001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/file_manager.js:2764: if
(setupInitialDirectory && self.setupGDataDirectoryAfterMount_) {
I think, setupInitialDirectory is true if and only if
setupGDataDirectoryAfterMount_ is not null.

[Vladislav Kaznacheev, 2012-03-28 14:00:57]

I rewrote the setupCurrentDirectory handling to be more straightforward. PTAL.

https://chromiumcodereview.appspot.com/9855024/diff/11001/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/directory_model.js (right):

https://chromiumcodereview.appspot.com/9855024/diff/11001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/directory_model.js:587:
opt_pathResolveCallback(baseName, leafName, exists && !overridden);
Because the client might have to do some cleanup (e.g. hiding a shade)
On 2012/03/28 10:28:30, dgozman wrote:
> Why do we call this in the case of overridden?

https://chromiumcodereview.appspot.com/9855024/diff/11001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/directory_model.js:590: var
changeDirectoryEntry = function (entry, callback, initial, exists) {
On 2012/03/28 10:28:30, dgozman wrote:
> space before (

Done.

https://chromiumcodereview.appspot.com/9855024/diff/11001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/directory_model.js:609:
changeDirectoryEntry(this.root_, autoSelect, INITIAL, !EXISTS);
Yes, it was a but (there was another one in onBaseFound.
On 2012/03/28 10:28:30, dgozman wrote:
> resolveCallback was not called here before. Bug?

https://chromiumcodereview.appspot.com/9855024/diff/11001/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://chromiumcodereview.appspot.com/9855024/diff/11001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/file_manager.js:565:
this.setupGDataDirectoryAfterMount_ = function() {
On 2012/03/28 10:28:30, dgozman wrote:
> Can we do the same trick as in DirectoryModel: add listener and then remove
it?

Done.

https://chromiumcodereview.appspot.com/9855024/diff/11001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/file_manager.js:845: if
(this.gdataLoadingTimer_)
Added a comment:
If the user changed to another directory and then back to GData we re-enter this
method while the timer is still active. In this case we only update the UI but
do not request the mount again

On 2012/03/28 10:28:30, dgozman wrote:
> How is this possible?

https://chromiumcodereview.appspot.com/9855024/diff/11001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/file_manager.js:1623:
this.directoryModel_.setupDefaultPath();
It only affects the case when we (think that we) are opening 
 a file, which is never the case with the default path. I renamed the parameter
to blankWhileOpeningAFile. On 2012/03/28 10:28:30, dgozman wrote:
> We ignore blankWhileLoading since default path is never gdata?
> This makes generic name blankWhileLoading really pointless.
> And a comment would be nice anyway.

https://chromiumcodereview.appspot.com/9855024/diff/11001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/file_manager.js:2606:
self.alert.showHtml(
It was not bound enough:) because the parameter to executeIfAvaliable was not
bound. I fixed it properly now.
On 2012/03/28 10:28:30, dgozman wrote:
> Callback is bound below.

https://chromiumcodereview.appspot.com/9855024/diff/11001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/file_manager.js:2688: // Not calling
clearGDataLoadingTimer_ here because we want to keep
Yes. This is intended (see
http://code.google.com/p/chromium-os/issues/detail?id=28298#c1).
Added a comment at the timer creation point.

On 2012/03/28 10:28:30, dgozman wrote:
> Is the following possible?
> - requesting gdata mount
> - it mount successfully
> - starting rescan
> - timer fires, we show error message (problem here)
> - rescan complete, we show contents

https://chromiumcodereview.appspot.com/9855024/diff/11001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/file_manager.js:2690: // the timer
fires after the mount because onDirectoryChanged_will hide
On 2012/03/28 10:28:30, dgozman wrote:
> typo: onDirectoryChanged_will

Done.

https://chromiumcodereview.appspot.com/9855024/diff/11001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/file_manager.js:2764: if
(setupInitialDirectory && self.setupGDataDirectoryAfterMount_) {
I got rid of this variable completely.

On 2012/03/28 10:28:30, dgozman wrote:
> I think, setupInitialDirectory is true if and only if
> setupGDataDirectoryAfterMount_ is not null.

[dgozman, 2012-03-28 14:18:35]

LGTM

https://chromiumcodereview.appspot.com/9855024/diff/11006/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://chromiumcodereview.appspot.com/9855024/diff/11006/chrome/browser/reso...
chrome/browser/resources/file_manager/js/file_manager.js:2714:
this.setupCurrentDirectoryPostponed_(false);
Comment about meaning of this |false|.

[commit-bot: I haz the power, 2012-03-28 14:45:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaznacheev@chromium.org/9855024/12008

[commit-bot: I haz the power, 2012-03-28 16:32:10]

Change committed as 129425