Alternative GDrive promo for the File Manager

Alternative GDrive promo for the File Manager

BUG=138954
TEST=


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

[Vladislav Kaznacheev, 2012-07-24 16:54:28]

Please review

[Dmitry Zvorygin, 2012-07-25 15:15:46]

LGTM, just take a look at my notes.

http://codereview.chromium.org/10818031/diff/3001/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system/statistics_provider.cc (right):

http://codereview.chromium.org/10818031/diff/3001/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system/statistics_provider.cc:215: if (name ==
"CHROMEOS_RELEASE_BOARD") {
I don't like that you use this sting in two places, and with little different
meaning. Why not to make a constant at least?

http://codereview.chromium.org/10818031/diff/3001/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/css/gdrive_welcome.css (right):

http://codereview.chromium.org/10818031/diff/3001/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/css/gdrive_welcome.css:68:
background-color: rgb(53, 122, 232);
I'm not a css pro, but are you sure you need in all these styles both background
color and background-image, filling all the background?

http://codereview.chromium.org/10818031/diff/3001/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/10818031/diff/3001/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/file_manager.js:4096: more =
links.ownerDocument.createElement('a');
Why not to use this.document(or self.document, although I prefer bind() over
self-closures)? Isn't it clearer than links.ownerDocument ?

http://codereview.chromium.org/10818031/diff/3001/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/file_manager.js:4174: if
(result.totalSizeKB < 100 * 1024 * 1024)
Consider using util.SCALE.

http://codereview.chromium.org/10818031/diff/3001/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/js/mock_chrome.js (right):

http://codereview.chromium.org/10818031/diff/3001/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/mock_chrome.js:325: var kB_inGb = 1024
* 1024;
Consider using util.SCALE.

[Vladislav Kaznacheev, 2012-07-25 16:45:13]

https://chromiumcodereview.appspot.com/10818031/diff/3001/chrome/browser/chro...
File chrome/browser/chromeos/system/statistics_provider.cc (right):

https://chromiumcodereview.appspot.com/10818031/diff/3001/chrome/browser/chro...
chrome/browser/chromeos/system/statistics_provider.cc:215: if (name ==
"CHROMEOS_RELEASE_BOARD") {
The meaning in C++ is exactly the same. This name is also used in JS but there
is no way to declare a constant across the two languages.
On 2012/07/25 15:15:46, Dmitry Zvorygin wrote:
> I don't like that you use this sting in two places, and with little different
> meaning. Why not to make a constant at least?

https://chromiumcodereview.appspot.com/10818031/diff/3001/chrome/browser/reso...
File chrome/browser/resources/file_manager/css/gdrive_welcome.css (right):

https://chromiumcodereview.appspot.com/10818031/diff/3001/chrome/browser/reso...
chrome/browser/resources/file_manager/css/gdrive_welcome.css:68:
background-color: rgb(53, 122, 232);
Good catch! Removed.
On 2012/07/25 15:15:46, Dmitry Zvorygin wrote:
> I'm not a css pro, but are you sure you need in all these styles both
background
> color and background-image, filling all the background?

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

https://chromiumcodereview.appspot.com/10818031/diff/3001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/file_manager.js:4096: more =
links.ownerDocument.createElement('a');
Changed to self.document_
On 2012/07/25 15:15:46, Dmitry Zvorygin wrote:
> Why not to use this.document(or self.document, although I prefer bind() over
> self-closures)? Isn't it clearer than links.ownerDocument ?

https://chromiumcodereview.appspot.com/10818031/diff/3001/chrome/browser/reso...
chrome/browser/resources/file_manager/js/file_manager.js:4174: if
(result.totalSizeKB < 100 * 1024 * 1024)
I tried and it looks even less readable. Added a comment instead.
On 2012/07/25 15:15:46, Dmitry Zvorygin wrote:
> Consider using util.SCALE.

[commit-bot: I haz the power, 2012-07-25 18:29:03]

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

[commit-bot: I haz the power, 2012-07-25 19:58:48]

Change committed as 148388