Files.app: Fix errors when launching Files.app via "chrome://files" (2)

Files.app: Fix errors when launching Files.app via "chrome://files" (2)

The previous patch (r203795) is not a good way because chrome.app.window.contentWindow is always undefined. This patch introduced another method to check if it is running as apps instead.

BUG=243414
TEST=Confirm a close button works correctly on normal Files.app and doesn't work on chrome://files.
R=mtomasz@chromium.org

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

[yoshiki, 2013-06-04 06:15:43]

@mtomasz: PTAL

[mtomasz, 2013-06-04 06:38:44]

Thanks for fixing this! Some minor comments.

https://codereview.chromium.org/16207004/diff/1/chrome/browser/resources/file...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://codereview.chromium.org/16207004/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/file_manager.js:895:
console.info('Files.app window is not created yet. Maybe launched ' +
Please update the comment or remove it.

https://codereview.chromium.org/16207004/diff/1/chrome/browser/resources/file...
File chrome/browser/resources/file_manager/js/util.js (right):

https://codereview.chromium.org/16207004/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/util.js:807: * @return {boolean} True
if the files.app is running as AppsV2. False if
nit: the files.app -> Files.app (for consistency)

https://codereview.chromium.org/16207004/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/util.js:810: runningAsApps: function()
{
This naming is confusing. Files.app in chrome://files is also an App V2. How
about renaming to 'runningInBrowser' ?

https://codereview.chromium.org/16207004/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/util.js:811: if (!util.platform.v2() ||
!util.platform.newUI())
Remove util.platform.newUI() check?

https://codereview.chromium.org/16207004/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/util.js:813: return (window.appID) ?
true : false;
... -> return !!window.appID ?

[yoshiki, 2013-06-04 06:57:34]

@mtomasz: PTAL

https://codereview.chromium.org/16207004/diff/1/chrome/browser/resources/file...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://codereview.chromium.org/16207004/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/file_manager.js:895:
console.info('Files.app window is not created yet. Maybe launched ' +
On 2013/06/04 06:38:44, mtomasz wrote:
> Please update the comment or remove it.

Removed.

https://codereview.chromium.org/16207004/diff/1/chrome/browser/resources/file...
File chrome/browser/resources/file_manager/js/util.js (right):

https://codereview.chromium.org/16207004/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/util.js:807: * @return {boolean} True
if the files.app is running as AppsV2. False if
On 2013/06/04 06:38:44, mtomasz wrote:
> nit: the files.app -> Files.app (for consistency)

Done.

https://codereview.chromium.org/16207004/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/util.js:810: runningAsApps: function()
{
On 2013/06/04 06:38:44, mtomasz wrote:
> This naming is confusing. Files.app in chrome://files is also an App V2. How
> about renaming to 'runningInBrowser' ?

Done.

https://codereview.chromium.org/16207004/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/util.js:811: if (!util.platform.v2() ||
!util.platform.newUI())
On 2013/06/04 06:38:44, mtomasz wrote:
> Remove util.platform.newUI() check?

Done.

https://codereview.chromium.org/16207004/diff/1/chrome/browser/resources/file...
chrome/browser/resources/file_manager/js/util.js:813: return (window.appID) ?
true : false;
On 2013/06/04 06:38:44, mtomasz wrote:
> ... -> return !!window.appID ?

Refactored.

[mtomasz, 2013-06-04 07:02:17]

lgtm!

[yoshiki, 2013-06-04 07:05:12]

Thanks!

[commit-bot: I haz the power, 2013-06-04 07:05:28]

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

[commit-bot: I haz the power, 2013-06-04 15:17:17]

Change committed as 203969