Propagate the result of AmountOfFreeSpace to JS and show it in chrome://drive-internals.

Propagate the result of AmountOfFreeSpace to JS and show it in chrome://drive-internals.

BUG=139444
TEST=manually open the page and check if it shows the amount of local free space.


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

[satorux1, 2012-08-13 07:26:20]

http://codereview.chromium.org/10827292/diff/1/chrome/browser/resources/chrom...
File chrome/browser/resources/chromeos/drive_internals.html (right):

http://codereview.chromium.org/10827292/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/drive_internals.html:58: <div
id='local-storage-usage'></div>
I guess it'd be better to do:

<div>Free space: <span id='local-storage-usage'></span> bytes</div>

http://codereview.chromium.org/10827292/diff/1/chrome/browser/resources/chrom...
File chrome/browser/resources/chromeos/drive_internals.js (right):

http://codereview.chromium.org/10827292/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/drive_internals.js:77: var place_holder =
$('local-storage-usage');
placeHolder

http://codereview.chromium.org/10827292/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/drive_internals.js:81: 'Unable to check local
free space.';
chrome://drive-internals is for developers. The message doesn't have to be
friendly. It's ok to print -1 in case of an error.

Besides, it's desirable to have text messages in HTML than JS.

http://codereview.chromium.org/10827292/diff/1/chrome/browser/ui/webui/chrome...
File chrome/browser/ui/webui/chromeos/drive_internals_ui.cc (right):

http://codereview.chromium.org/10827292/diff/1/chrome/browser/ui/webui/chrome...
chrome/browser/ui/webui/chromeos/drive_internals_ui.cc:202: void
DriveInternalsWebUIHandler::OnPageLoaded(const base::ListValue* args) {
Please add the following here followed by a blank line, and other functions too

DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

http://codereview.chromium.org/10827292/diff/1/chrome/browser/ui/webui/chrome...
chrome/browser/ui/webui/chromeos/drive_internals_ui.cc:224: free_space =
base::SysInfo::AmountOfFreeDiskSpace(path);
I think we shouldn't do this on UI thread. We should post a task to a blocking
pool. Please see some example in chrome/browser/chromeos/gdata.

Looking at the code, AmountOfFreeDiskSpace does not have 

  base::ThreadRestrictions::AssertIOAllowed();

I think we should add it, as it'll likely access the disk. Could you add it in a
separate patch?

[Haruki Sato, 2012-08-17 05:46:34]

http://codereview.chromium.org/10827292/diff/1/chrome/browser/resources/chrom...
File chrome/browser/resources/chromeos/drive_internals.html (right):

http://codereview.chromium.org/10827292/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/drive_internals.html:58: <div
id='local-storage-usage'></div>
On 2012/08/13 07:26:20, satorux1 wrote:
> I guess it'd be better to do:
> 
> <div>Free space: <span id='local-storage-usage'></span> bytes</div>

Done.
Thank you for the great advice.

http://codereview.chromium.org/10827292/diff/1/chrome/browser/resources/chrom...
File chrome/browser/resources/chromeos/drive_internals.js (right):

http://codereview.chromium.org/10827292/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/drive_internals.js:77: var place_holder =
$('local-storage-usage');
On 2012/08/13 07:26:20, satorux1 wrote:
> placeHolder

Done. Actually, I ended up removing this temporal variable.

http://codereview.chromium.org/10827292/diff/1/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/drive_internals.js:81: 'Unable to check local
free space.';
On 2012/08/13 07:26:20, satorux1 wrote:
> chrome://drive-internals is for developers. The message doesn't have to be
> friendly. It's ok to print -1 in case of an error.
> 
> Besides, it's desirable to have text messages in HTML than JS.

Done.
and now it's so simple!

http://codereview.chromium.org/10827292/diff/1/chrome/browser/ui/webui/chrome...
File chrome/browser/ui/webui/chromeos/drive_internals_ui.cc (right):

http://codereview.chromium.org/10827292/diff/1/chrome/browser/ui/webui/chrome...
chrome/browser/ui/webui/chromeos/drive_internals_ui.cc:202: void
DriveInternalsWebUIHandler::OnPageLoaded(const base::ListValue* args) {
On 2012/08/13 07:26:20, satorux1 wrote:
> Please add the following here followed by a blank line, and other functions
too
> 
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

Done.

http://codereview.chromium.org/10827292/diff/1/chrome/browser/ui/webui/chrome...
chrome/browser/ui/webui/chromeos/drive_internals_ui.cc:224: free_space =
base::SysInfo::AmountOfFreeDiskSpace(path);
On 2012/08/13 07:26:20, satorux1 wrote:
> I think we shouldn't do this on UI thread. We should post a task to a blocking
> pool. Please see some example in chrome/browser/chromeos/gdata.
> 
> Looking at the code, AmountOfFreeDiskSpace does not have 
> 
>   base::ThreadRestrictions::AssertIOAllowed();
> 
> I think we should add it, as it'll likely access the disk. Could you add it in
a
> separate patch?

Done.
and moved the code to have more cleaner structure.
PTAL.

Also I filed http://crbug.com/143335 and will work on it.

[satorux1, 2012-08-17 10:50:49]

http://codereview.chromium.org/10827292/diff/9/chrome/browser/resources/chrom...
File chrome/browser/resources/chromeos/drive_internals.html (right):

http://codereview.chromium.org/10827292/diff/9/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/drive_internals.html:58: <div>Free space:
<span id='local-storage-freespace'></span> bytes</div>
bytes -> MB. see my comment in .js file.

http://codereview.chromium.org/10827292/diff/9/chrome/browser/resources/chrom...
File chrome/browser/resources/chromeos/drive_internals.js (right):

http://codereview.chromium.org/10827292/diff/9/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/drive_internals.js:77:
$('local-storage-freespace').innerText = localStorageSummary.free_space;
I guess we want to convert it to MB. Otherwise, the number is hard to read.

  var freeSpaceInMB = localStorageSummary.free_space / (1<<20);

Please change the HTML file too.

http://codereview.chromium.org/10827292/diff/9/chrome/browser/ui/webui/chrome...
File chrome/browser/ui/webui/chromeos/drive_internals_ui.cc (right):

http://codereview.chromium.org/10827292/diff/9/chrome/browser/ui/webui/chrome...
chrome/browser/ui/webui/chromeos/drive_internals_ui.cc:90: void
GetFreeDiskSpace(const FilePath& path,
function comment is missing.

http://codereview.chromium.org/10827292/diff/9/chrome/browser/ui/webui/chrome...
chrome/browser/ui/webui/chromeos/drive_internals_ui.cc:92:
DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
let's add 

DCHECK(local_storage_summary);

http://codereview.chromium.org/10827292/diff/9/chrome/browser/ui/webui/chrome...
chrome/browser/ui/webui/chromeos/drive_internals_ui.cc:94: int64 free_space =
base::SysInfo::AmountOfFreeDiskSpace(path);
matter of taste, but you might want to add 'const' as you won't change. it's a
habit.

http://codereview.chromium.org/10827292/diff/9/chrome/browser/ui/webui/chrome...
chrome/browser/ui/webui/chromeos/drive_internals_ui.cc:253: FilePath path;
let's make it a bit more descriptive.

path -> home_path

http://codereview.chromium.org/10827292/diff/9/chrome/browser/ui/webui/chrome...
chrome/browser/ui/webui/chromeos/drive_internals_ui.cc:372:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
nit: reverse the two DCHECKs

[Haruki Sato, 2012-08-17 15:08:45]

http://codereview.chromium.org/10827292/diff/9/chrome/browser/resources/chrom...
File chrome/browser/resources/chromeos/drive_internals.html (right):

http://codereview.chromium.org/10827292/diff/9/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/drive_internals.html:58: <div>Free space:
<span id='local-storage-freespace'></span> bytes</div>
On 2012/08/17 10:50:49, satorux1 wrote:
> bytes -> MB. see my comment in .js file.

Done.

http://codereview.chromium.org/10827292/diff/9/chrome/browser/resources/chrom...
File chrome/browser/resources/chromeos/drive_internals.js (right):

http://codereview.chromium.org/10827292/diff/9/chrome/browser/resources/chrom...
chrome/browser/resources/chromeos/drive_internals.js:77:
$('local-storage-freespace').innerText = localStorageSummary.free_space;
On 2012/08/17 10:50:49, satorux1 wrote:
> I guess we want to convert it to MB. Otherwise, the number is hard to read.
> 
>   var freeSpaceInMB = localStorageSummary.free_space / (1<<20);
> 
> Please change the HTML file too.

Done.

http://codereview.chromium.org/10827292/diff/9/chrome/browser/ui/webui/chrome...
File chrome/browser/ui/webui/chromeos/drive_internals_ui.cc (right):

http://codereview.chromium.org/10827292/diff/9/chrome/browser/ui/webui/chrome...
chrome/browser/ui/webui/chromeos/drive_internals_ui.cc:90: void
GetFreeDiskSpace(const FilePath& path,
On 2012/08/17 10:50:49, satorux1 wrote:
> function comment is missing.

Done.
Thanks.

http://codereview.chromium.org/10827292/diff/9/chrome/browser/ui/webui/chrome...
chrome/browser/ui/webui/chromeos/drive_internals_ui.cc:92:
DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2012/08/17 10:50:49, satorux1 wrote:
> let's add 
> 
> DCHECK(local_storage_summary);

Done.

http://codereview.chromium.org/10827292/diff/9/chrome/browser/ui/webui/chrome...
chrome/browser/ui/webui/chromeos/drive_internals_ui.cc:94: int64 free_space =
base::SysInfo::AmountOfFreeDiskSpace(path);
On 2012/08/17 10:50:49, satorux1 wrote:
> matter of taste, but you might want to add 'const' as you won't change. it's a
> habit.

Done.

http://codereview.chromium.org/10827292/diff/9/chrome/browser/ui/webui/chrome...
chrome/browser/ui/webui/chromeos/drive_internals_ui.cc:253: FilePath path;
On 2012/08/17 10:50:49, satorux1 wrote:
> let's make it a bit more descriptive.
> 
> path -> home_path

Done.

http://codereview.chromium.org/10827292/diff/9/chrome/browser/ui/webui/chrome...
chrome/browser/ui/webui/chromeos/drive_internals_ui.cc:372:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2012/08/17 10:50:49, satorux1 wrote:
> nit: reverse the two DCHECKs

Done.

[satorux1, 2012-08-17 15:13:03]

LGTM!

[commit-bot: I haz the power, 2012-08-17 15:14:06]

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

[commit-bot: I haz the power, 2012-08-17 18:36:10]

Change committed as 152117