Extract some logic from SuggestAppsDialog to a separate class

Extract some logic from SuggestAppsDialog to a separate class

This extracts logic that handles loading, initializing and handling
requests from web store widget out of SuggestAppsDialog.
What's left in SuggestAppsDialog is logic for showing/hiding the dialog
modal to file manager, and handling results from the widget.

The goal is to eventually remove the newly created class out of
ui/file_manager so it can be used to show Web Store widget for purposes
not related to file manager.

BUG=439448
Committed: https://crrev.com/7ec37f259f276209cad16730050b6db849ea7824
Cr-Commit-Position: refs/heads/master@{#324064}

[tbarzic, 2015-04-07 00:51:29]

Patchset #2 (id:20001) has been deleted

[tbarzic, 2015-04-07 01:32:37]

tbarzic@chromium.org changed reviewers:
+ mtomasz@chromium.org, yoshiki@chromium.org

[tbarzic, 2015-04-07 01:32:38]

Please take a look.
Note that in Patchset 2 cws_widget_container.js is identical to
suggest_apps_dialog.js at ToT (so it may be easier to review changes to that
file relative to patchset 2)

[mtomasz, 2015-04-07 02:27:49]

In general lgtm with nits. Please check if everything compiles with Closure
compiler.

GYP_GENERATORS=ninja tools/gyp/gyp --depth .
ui/file_manager/file_manager/foreground/js/compiled_resources.gyp && ninja -C
out/Default
GYP_GENERATORS=ninja tools/gyp/gyp --depth .
ui/file_manager/file_manager/background/js/compiled_resources.gyp && ninja -C
out/Default
GYP_GENERATORS=ninja tools/gyp/gyp --depth .
ui/file_manager/gallery/js/compiled_resources.gyp && ninja -C out/Default
GYP_GENERATORS=ninja tools/gyp/gyp --depth .
ui/file_manager/video_player/js/compiled_resources.gyp && ninja -C out/Default
GYP_GENERATORS=ninja tools/gyp/gyp --depth .
ui/file_manager/image_loader/compiled_resources.gyp && ninja -C out/Default

https://codereview.chromium.org/1068653002/diff/100001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/js/cws_widget_container.js (right):

https://codereview.chromium.org/1068653002/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/cws_widget_container.js:15: * The
width of the widget (in pixel).
nit: pixels

https://codereview.chromium.org/1068653002/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/cws_widget_container.js:19: var
WEBVIEW_WIDTH = 735;
(I think we should move those dimensions to the CWSWidgetContainer constructor
when moving the file outside of file_manager dir).

https://codereview.chromium.org/1068653002/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/cws_widget_container.js:99: *
@type{!Element}
nit: \s

https://codereview.chromium.org/1068653002/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/cws_widget_container.js:297: if
(this.state_ != CWSWidgetContainer.State.UNINITIALIZED) {
nit: !==

https://codereview.chromium.org/1068653002/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/cws_widget_container.js:328: if
(this.state_ != CWSWidgetContainer.State.ACCESS_TOKEN_READY) {
nit: !==

https://codereview.chromium.org/1068653002/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/cws_widget_container.js:350: /**
@type {WebView} */(this.document_.createElement('webview'));
nit: !WebView

https://codereview.chromium.org/1068653002/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/cws_widget_container.js:410: if
(e.keyCode != 13 /* Enter */)
nit: !==

https://codereview.chromium.org/1068653002/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/cws_widget_container.js:599: if
(this.state_ != CWSWidgetContainer.State.UNINITIALIZED)
nit: !==

https://codereview.chromium.org/1068653002/diff/100001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js
(right):

https://codereview.chromium.org/1068653002/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js:134:
.then(function() {
nit: I think we may need to add jsdoc to those closures. Please check if this CL
compiles.

GYP_GENERATORS=ninja tools/gyp/gyp --depth .
ui/file_manager/file_manager/foreground/js/compiled_resources.gyp && ninja -C
out/Default

https://codereview.chromium.org/1068653002/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js:142: if
(reason != CWSWidgetContainer.ResolveReason.RESET)
nit: !==

https://codereview.chromium.org/1068653002/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js:162:
SuggestAppsDialog.prototype.showAsFileManagerDialog_ = function(title) {
I think this file is not going to be moved outside of Files app. In such case
name of this method is confusing. Correct me if I'm wrong.

[tbarzic, 2015-04-07 02:51:30]

https://codereview.chromium.org/1068653002/diff/100001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/js/cws_widget_container.js (right):

https://codereview.chromium.org/1068653002/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/cws_widget_container.js:15: * The
width of the widget (in pixel).
On 2015/04/07 02:27:49, mtomasz wrote:
> nit: pixels

Done.

https://codereview.chromium.org/1068653002/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/cws_widget_container.js:19: var
WEBVIEW_WIDTH = 735;
On 2015/04/07 02:27:49, mtomasz wrote:
> (I think we should move those dimensions to the CWSWidgetContainer constructor
> when moving the file outside of file_manager dir).

Acknowledged.

https://codereview.chromium.org/1068653002/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/cws_widget_container.js:99: *
@type{!Element}
On 2015/04/07 02:27:49, mtomasz wrote:
> nit: \s

Done.

https://codereview.chromium.org/1068653002/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/cws_widget_container.js:297: if
(this.state_ != CWSWidgetContainer.State.UNINITIALIZED) {
On 2015/04/07 02:27:49, mtomasz wrote:
> nit: !==

Done.

https://codereview.chromium.org/1068653002/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/cws_widget_container.js:328: if
(this.state_ != CWSWidgetContainer.State.ACCESS_TOKEN_READY) {
On 2015/04/07 02:27:49, mtomasz wrote:
> nit: !==

Done.

https://codereview.chromium.org/1068653002/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/cws_widget_container.js:350: /**
@type {WebView} */(this.document_.createElement('webview'));
On 2015/04/07 02:27:49, mtomasz wrote:
> nit: !WebView

Done.

https://codereview.chromium.org/1068653002/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/cws_widget_container.js:410: if
(e.keyCode != 13 /* Enter */)
On 2015/04/07 02:27:49, mtomasz wrote:
> nit: !==

Done.

https://codereview.chromium.org/1068653002/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/cws_widget_container.js:599: if
(this.state_ != CWSWidgetContainer.State.UNINITIALIZED)
On 2015/04/07 02:27:49, mtomasz wrote:
> nit: !==

Done.

https://codereview.chromium.org/1068653002/diff/100001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js
(right):

https://codereview.chromium.org/1068653002/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js:134:
.then(function() {
On 2015/04/07 02:27:49, mtomasz wrote:
> nit: I think we may need to add jsdoc to those closures. Please check if this
CL
> compiles.
> 
> GYP_GENERATORS=ninja tools/gyp/gyp --depth .
> ui/file_manager/file_manager/foreground/js/compiled_resources.gyp && ninja -C
> out/Default

It did compile, but I added jsdocs regardless.
The formatting is a bit more awkward, though. Let me know if you have a better
suggestion.

https://codereview.chromium.org/1068653002/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js:142: if
(reason != CWSWidgetContainer.ResolveReason.RESET)
On 2015/04/07 02:27:49, mtomasz wrote:
> nit: !==

Done.

https://codereview.chromium.org/1068653002/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js:162:
SuggestAppsDialog.prototype.showAsFileManagerDialog_ = function(title) {
On 2015/04/07 02:27:49, mtomasz wrote:
> I think this file is not going to be moved outside of Files app. In such case
> name of this method is confusing. Correct me if I'm wrong.

dropped 'AsFileManager' from the name

[mtomasz, 2015-04-07 02:59:49]

thanks, lgtm. But please wait for @yoshiki's final lgtm, as he wrote most of the
code.

https://codereview.chromium.org/1068653002/diff/100001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js
(right):

https://codereview.chromium.org/1068653002/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js:134:
.then(function() {
On 2015/04/07 02:51:30, tbarzic wrote:
> On 2015/04/07 02:27:49, mtomasz wrote:
> > nit: I think we may need to add jsdoc to those closures. Please check if
this
> CL
> > compiles.
> > 
> > GYP_GENERATORS=ninja tools/gyp/gyp --depth .
> > ui/file_manager/file_manager/foreground/js/compiled_resources.gyp && ninja
-C
> > out/Default
> 
> It did compile, but I added jsdocs regardless.
> The formatting is a bit more awkward, though. Let me know if you have a better
> suggestion.

The formatting is OK. We do it exactly this way.

https://codereview.chromium.org/1068653002/diff/120001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js
(right):

https://codereview.chromium.org/1068653002/diff/120001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js:140: /**
@return {Promise.<CWSWidgetContainer.ResolveReason>} */
nit: !Promise

https://codereview.chromium.org/1068653002/diff/120001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js:155: /**
@param {string} error The error message */
FYI: We skip @param descriptions if they are obvious.

[yoshiki, 2015-04-07 03:33:52]

Thank you for doing a large cleanup! The patch LGTM with some tiny nits!

https://codereview.chromium.org/1068653002/diff/120001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/js/cws_widget_container.js (right):

https://codereview.chromium.org/1068653002/diff/120001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/cws_widget_container.js:112:
this.webstoreButton_.appendChild(webstoreButtonLabel);;
nit: double semicollons at the end

https://codereview.chromium.org/1068653002/diff/120001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/cws_widget_container.js:219: //
Install is done. The install app should be opened.
nit: Could you use /** some comment */ style comment? Since these are
annotation.

https://codereview.chromium.org/1068653002/diff/120001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/cws_widget_container.js:320: * Must
not be called before {@code this.ready()} is resolved.
nit: add a blank line since both the description and the annotation have
multiple lines.

https://codereview.chromium.org/1068653002/diff/120001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js
(right):

https://codereview.chromium.org/1068653002/diff/120001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js:29:
this.widgetRoot_ = this.document_.createElement('div');
nit: This property looks not used in other place. Can we change it to a normal
variable instead of property?

https://codereview.chromium.org/1068653002/diff/120001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js:119: *
@param {function(SuggestAppsDialog.Result, ?string)} onDialogClosed Called
nit: Could you make this callback param non-null?

https://codereview.chromium.org/1068653002/diff/120001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js:144:
}.bind(this))
nit: indent looks wrong

https://codereview.chromium.org/1068653002/diff/120001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js:161: } else
{
nit: indent looks wrong

[tbarzic, 2015-04-07 16:52:55]

https://codereview.chromium.org/1068653002/diff/120001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/js/cws_widget_container.js (right):

https://codereview.chromium.org/1068653002/diff/120001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/cws_widget_container.js:112:
this.webstoreButton_.appendChild(webstoreButtonLabel);;
On 2015/04/07 03:33:52, yoshiki wrote:
> nit: double semicollons at the end

Done.

https://codereview.chromium.org/1068653002/diff/120001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/cws_widget_container.js:219: //
Install is done. The install app should be opened.
On 2015/04/07 03:33:51, yoshiki wrote:
> nit: Could you use /** some comment */ style comment? Since these are
> annotation.

Done.

https://codereview.chromium.org/1068653002/diff/120001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/cws_widget_container.js:320: * Must
not be called before {@code this.ready()} is resolved.
On 2015/04/07 03:33:51, yoshiki wrote:
> nit: add a blank line since both the description and the annotation have
> multiple lines.

Done.

https://codereview.chromium.org/1068653002/diff/120001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js
(right):

https://codereview.chromium.org/1068653002/diff/120001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js:29:
this.widgetRoot_ = this.document_.createElement('div');
On 2015/04/07 03:33:52, yoshiki wrote:
> nit: This property looks not used in other place. Can we change it to a normal
> variable instead of property?

Done.

https://codereview.chromium.org/1068653002/diff/120001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js:119: *
@param {function(SuggestAppsDialog.Result, ?string)} onDialogClosed Called
On 2015/04/07 03:33:52, yoshiki wrote:
> nit: Could you make this callback param non-null?

If you mean to make this !function, that's not really needed since function is
not nullable (so !function is implied)

https://codereview.chromium.org/1068653002/diff/120001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js:140: /**
@return {Promise.<CWSWidgetContainer.ResolveReason>} */
On 2015/04/07 02:59:49, mtomasz wrote:
> nit: !Promise

Done.

https://codereview.chromium.org/1068653002/diff/120001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js:144:
}.bind(this))
On 2015/04/07 03:33:52, yoshiki wrote:
> nit: indent looks wrong

Done.

https://codereview.chromium.org/1068653002/diff/120001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js:155: /**
@param {string} error The error message */
On 2015/04/07 02:59:49, mtomasz wrote:
> FYI: We skip @param descriptions if they are obvious.

Done.

https://codereview.chromium.org/1068653002/diff/120001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js:161: } else
{
On 2015/04/07 03:33:52, yoshiki wrote:
> nit: indent looks wrong

Done.

[tbarzic, 2015-04-07 16:53:02]

The CQ bit was checked by tbarzic@chromium.org

[tbarzic, 2015-04-07 16:53:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
mtomasz@chromium.org, yoshiki@chromium.org
Link to the patchset: https://codereview.chromium.org/1068653002/#ps140001
(title: ".")

[commit-bot: I haz the power, 2015-04-07 16:53:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1068653002/140001

[commit-bot: I haz the power, 2015-04-07 17:32:07]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2015-04-07 17:33:02]

Patchset 7 (id:??) landed as
https://crrev.com/7ec37f259f276209cad16730050b6db849ea7824
Cr-Commit-Position: refs/heads/master@{#324064}