WebUI: Allow rejecting a promise that is returned from cr.sendWithPromise().

WebUI: Allow rejecting a promise that is returned from cr.sendWithPromise().

 - Moving PromiseResolver helper class such that it can be used from cr.js
 - Updating cr.webUIResponse to accept an isSuccess parameter.
 - Renaming SettingsPageUIHandler#CallJavascriptCallback to
   ResolveJavascriptCallback, and adding new RejectJavascriptCallback method.
 - Adding test.

BUG=591266
Committed: https://crrev.com/8b4b5952c3259661772fb882053bbe67b9de2a67
Cr-Commit-Position: refs/heads/master@{#379104}

[dpapad, 2016-03-02 19:14:55]

https://codereview.chromium.org/1758013002/diff/1/ui/webui/resources/js/cr.js
File ui/webui/resources/js/cr.js (right):

https://codereview.chromium.org/1758013002/diff/1/ui/webui/resources/js/cr.js...
ui/webui/resources/js/cr.js:16: var cr = cr || function() {
Every time cr.js is evaluated is overwriting anything that has been previously
defined on 'cr' namespace. For example what is happening in my case (left the
console.logs on purpose to showcase the problem).

To reproduce do the following
1) Revert this line here.
2) ./out/Default_gn/browser_tests
--gtest_filter=CrSettingsSearchEnginesTest.SearchEngines

You will see the following in the output
cr.js running
promise_resolver.js running
cr.js running // overwrites the 'cr' namespace and cr.PromiseResolver no longer
exists.
<test is failing because cr.PromiseResolver> does not exist.

With this change, the 2nd time cr.js is evaluated does nothing, and no
overwriting happens, fixing the issue.

[Dan Beam, 2016-03-02 19:17:40]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2016-03-02 19:17:40]

https://codereview.chromium.org/1758013002/diff/1/ui/webui/resources/js/cr/pr...
File ui/webui/resources/js/cr/promise_resolver.js (right):

https://codereview.chromium.org/1758013002/diff/1/ui/webui/resources/js/cr/pr...
ui/webui/resources/js/cr/promise_resolver.js:19: cr.define('cr', function() {
why does this need to be namespaced to cr.*?

[dpapad, 2016-03-02 20:01:46]

Description was changed from

==========
WebUI: Making PromiseResolver helper available to non-testing code.

BUG=
==========

to

==========
WebUI: Making PromiseResolver helper available to non-testing code.

BUG=591266
==========

[dpapad, 2016-03-02 20:02:09]

In my next CL I will attempt to use PromiseResolver from within cr.js. Is it OK
to have cr.js depend on another file?
See a related TODO (depending on assert.js) you have added at
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources....

If that is not OK, maybe putting PromiseResolver within cr.js would be best?

https://codereview.chromium.org/1758013002/diff/1/ui/webui/resources/js/cr/pr...
File ui/webui/resources/js/cr/promise_resolver.js (right):

https://codereview.chromium.org/1758013002/diff/1/ui/webui/resources/js/cr/pr...
ui/webui/resources/js/cr/promise_resolver.js:19: cr.define('cr', function() {
On 2016/03/02 at 19:17:40, Dan Beam wrote:
> why does this need to be namespaced to cr.*?

Moved one level up.

[Dan Beam, 2016-03-02 20:04:03]

yeah, we can put this in cr.js if you'd like

i'm fine with it being top-level or inside of cr.* if you want, but top-level is
probably fine and easier to reason about within cr.js itself

[dpapad, 2016-03-02 20:12:23]

On 2016/03/02 at 20:04:03, dbeam wrote:
> yeah, we can put this in cr.js if you'd like
> 
> i'm fine with it being top-level or inside of cr.* if you want, but top-level
is probably fine and easier to reason about within cr.js itself

Leaning towards putting it inside cr.js, because it seems easier
 - No need for promise_resolver.html file.
 - No need to import promise_resolver.html from everywhere where cr.html is now
imported.

An alternative would be to keep them separate and have cr.html depend on
promise_resolver.html, but I did not see any example of such dependencies in
/ui/webui/resources/html/*.

[dpapad, 2016-03-02 20:27:46]

On 2016/03/02 at 20:12:23, dpapad wrote:
> On 2016/03/02 at 20:04:03, dbeam wrote:
> > yeah, we can put this in cr.js if you'd like
> > 
> > i'm fine with it being top-level or inside of cr.* if you want, but
top-level is probably fine and easier to reason about within cr.js itself
> 
> Leaning towards putting it inside cr.js, because it seems easier
>  - No need for promise_resolver.html file.
>  - No need to import promise_resolver.html from everywhere where cr.html is
now imported.
> 
> An alternative would be to keep them separate and have cr.html depend on
promise_resolver.html, but I did not see any example of such dependencies in
/ui/webui/resources/html/*.

FYI I attempted the alternative mentioned above and it looks promising, see
latest patch (have not finished running all the tests yet though).

[dpapad, 2016-03-02 21:27:58]

Description was changed from

==========
WebUI: Making PromiseResolver helper available to non-testing code.

BUG=591266
==========

to

==========
WebUI: Allow rejecting a promise that is returned from cr.sendWithPromise().

 - Moving PromiseRelolver helper class such that it can be used from cr.js
 - Updating webUIResponse to accept an isSuccess parameter.
 - Updating CallJavascriptCallback helper.
 - Adding test.

BUG=591266
==========

[dpapad, 2016-03-02 22:39:59]

Description was changed from

==========
WebUI: Allow rejecting a promise that is returned from cr.sendWithPromise().

 - Moving PromiseRelolver helper class such that it can be used from cr.js
 - Updating webUIResponse to accept an isSuccess parameter.
 - Updating CallJavascriptCallback helper.
 - Adding test.

BUG=591266
==========

to

==========
WebUI: Allow rejecting a promise that is returned from cr.sendWithPromise().

 - Moving PromiseRelolver helper class such that it can be used from cr.js
 - Updating cr.webUIResponse to accept an isSuccess parameter.
 - Updating SettingsPageUIHandler#CallJavascriptCallback helper signature and
   callers.
 - Adding test.

BUG=591266
==========

[dpapad, 2016-03-02 22:41:23]

Ready for review, PTAL.
I went with the approach of keeping PromiseResolver on its own file, and
properly depending on it from cr.html.

[Dan Beam, 2016-03-03 19:04:51]

https://codereview.chromium.org/1758013002/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/appearance_handler.cc (left):

https://codereview.chromium.org/1758013002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/appearance_handler.cc:78:
base::FundamentalValue enabled(QueryResetThemeEnabledState());
this was prettier

https://codereview.chromium.org/1758013002/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/md_settings_ui.cc (right):

https://codereview.chromium.org/1758013002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/md_settings_ui.cc:47: void
SettingsPageUIHandler::CallJavascriptCallback(
maybe we could just introduce 2 methods, ResolveCallback() and RejectCallback()?

https://codereview.chromium.org/1758013002/diff/140001/ui/webui/resources/js/...
File ui/webui/resources/js/promise_resolver.js (right):

https://codereview.chromium.org/1758013002/diff/140001/ui/webui/resources/js/...
ui/webui/resources/js/promise_resolver.js:20: function PromiseResolver() {
I think this should be PromiseResult or PromiseWrapper or something, as Resolver
sounds like it'll... resolve (as in: not reject)

[dpapad, 2016-03-03 19:51:22]

https://codereview.chromium.org/1758013002/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/appearance_handler.cc (left):

https://codereview.chromium.org/1758013002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/appearance_handler.cc:78:
base::FundamentalValue enabled(QueryResetThemeEnabledState());
On 2016/03/03 at 19:04:51, Dan Beam wrote:
> this was prettier

Done.

https://codereview.chromium.org/1758013002/diff/140001/ui/webui/resources/js/...
File ui/webui/resources/js/promise_resolver.js (right):

https://codereview.chromium.org/1758013002/diff/140001/ui/webui/resources/js/...
ui/webui/resources/js/promise_resolver.js:20: function PromiseResolver() {
On 2016/03/03 at 19:04:51, Dan Beam wrote:
> I think this should be PromiseResult or PromiseWrapper or something, as
Resolver sounds like it'll... resolve (as in: not reject)

Technically you are right, "resolve" implies success, "reject" is error, and
"fulfill" is both. But PromiseResolver seems to be already used in various
places,
1) in print_preview
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
2) In Closure library, as goog.promise.Resolver,
https://github.com/google/closure-library/blob/master/closure/goog/promise/re...
3) Experimental Firefox API,
https://developer.mozilla.org/en-US/docs/Web/API/PromiseResolver

Given the above, I think it is fine to keep current name, even though the helper
class can also reject, I don't think it is confusing.

If you still feel strongly about this, then how about ProjectFulfiller?
PromiseResult sounds like what is passed as a parameter to the success callback,
is more confusing IMO, and PromiseWrapper just seems too vague.

[dpapad, 2016-03-03 19:51:53]

https://codereview.chromium.org/1758013002/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/md_settings_ui.cc (right):

https://codereview.chromium.org/1758013002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/md_settings_ui.cc:47: void
SettingsPageUIHandler::CallJavascriptCallback(
On 2016/03/03 at 19:04:51, Dan Beam wrote:
> maybe we could just introduce 2 methods, ResolveCallback() and
RejectCallback()?

Done.

[dpapad, 2016-03-03 20:00:57]

On 2016/03/03 at 19:51:22, dpapad wrote:
>
https://codereview.chromium.org/1758013002/diff/140001/chrome/browser/ui/webu...
> File chrome/browser/ui/webui/settings/appearance_handler.cc (left):
> 
>
https://codereview.chromium.org/1758013002/diff/140001/chrome/browser/ui/webu...
> chrome/browser/ui/webui/settings/appearance_handler.cc:78:
base::FundamentalValue enabled(QueryResetThemeEnabledState());
> On 2016/03/03 at 19:04:51, Dan Beam wrote:
> > this was prettier
> 
> Done.
> 
>
https://codereview.chromium.org/1758013002/diff/140001/ui/webui/resources/js/...
> File ui/webui/resources/js/promise_resolver.js (right):
> 
>
https://codereview.chromium.org/1758013002/diff/140001/ui/webui/resources/js/...
> ui/webui/resources/js/promise_resolver.js:20: function PromiseResolver() {
> On 2016/03/03 at 19:04:51, Dan Beam wrote:
> > I think this should be PromiseResult or PromiseWrapper or something, as
Resolver sounds like it'll... resolve (as in: not reject)
> 
> Technically you are right, "resolve" implies success, "reject" is error, and
"fulfill" is both. But PromiseResolver seems to be already used in various
places,
> 1) in print_preview
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> 2) In Closure library, as goog.promise.Resolver,
https://github.com/google/closure-library/blob/master/closure/goog/promise/re...
> 3) Experimental Firefox API,
https://developer.mozilla.org/en-US/docs/Web/API/PromiseResolver
> 
> Given the above, I think it is fine to keep current name, even though the
helper class can also reject, I don't think it is confusing.
> 
> If you still feel strongly about this, then how about ProjectFulfiller?
PromiseResult sounds like what is passed as a parameter to the success callback,
is more confusing IMO, and PromiseWrapper just seems too vague.

Oops, I meant PromiseFulfiller.

[dpapad, 2016-03-03 20:05:57]

Description was changed from

==========
WebUI: Allow rejecting a promise that is returned from cr.sendWithPromise().

 - Moving PromiseRelolver helper class such that it can be used from cr.js
 - Updating cr.webUIResponse to accept an isSuccess parameter.
 - Updating SettingsPageUIHandler#CallJavascriptCallback helper signature and
   callers.
 - Adding test.

BUG=591266
==========

to

==========
WebUI: Allow rejecting a promise that is returned from cr.sendWithPromise().

 - Moving PromiseResolver helper class such that it can be used from cr.js
 - Updating cr.webUIResponse to accept an isSuccess parameter.
 - Updating SettingsPageUIHandler#CallJavascriptCallback helper signature and
   callers.
 - Adding test.

BUG=591266
==========

[dpapad, 2016-03-03 20:06:44]

Description was changed from

==========
WebUI: Allow rejecting a promise that is returned from cr.sendWithPromise().

 - Moving PromiseResolver helper class such that it can be used from cr.js
 - Updating cr.webUIResponse to accept an isSuccess parameter.
 - Updating SettingsPageUIHandler#CallJavascriptCallback helper signature and
   callers.
 - Adding test.

BUG=591266
==========

to

==========
WebUI: Allow rejecting a promise that is returned from cr.sendWithPromise().

 - Moving PromiseResolver helper class such that it can be used from cr.js
 - Updating cr.webUIResponse to accept an isSuccess parameter.
 - Renaming SettingsPageUIHandler#CallJavascriptCallback to ResolveCallback, and
   adding new RejectCallback method.
 - Adding test.

BUG=591266
==========

[Dan Beam, 2016-03-03 20:11:08]

On 2016/03/03 19:51:22, dpapad wrote:
> ui/webui/resources/js/promise_resolver.js:20: function PromiseResolver() {
> On 2016/03/03 at 19:04:51, Dan Beam wrote:
> > I think this should be PromiseResult or PromiseWrapper or something, as
> Resolver sounds like it'll... resolve (as in: not reject)
> 
> Technically you are right, "resolve" implies success, "reject" is error, and
> "fulfill" is both. But PromiseResolver seems to be already used in various
> places,
> 1) in print_preview
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

why aren't we using this class in print preview as well, btw?

> 2) In Closure library, as goog.promise.Resolver,
>
https://github.com/google/closure-library/blob/master/closure/goog/promise/re...

ehhhh, i guess that's not a bad argument

> 3) Experimental Firefox API,
> https://developer.mozilla.org/en-US/docs/Web/API/PromiseResolver

1) in FF, not Chrome
2) removed from the spec, in the process of fixing the page

> 
> Given the above, I think it is fine to keep current name, even though the
helper
> class can also reject, I don't think it is confusing.

ehhhhh

> 
> If you still feel strongly about this, then how about ProjectFulfiller?

fulfillment is also used in MDN to mean "success".

> PromiseResult sounds like what is passed as a parameter to the success
callback,
> is more confusing IMO, and PromiseWrapper just seems too vague.

agree that PromiseWrapper is a little less specific

fwiw: PromiseResult is what I chose for a reason.  PromiseResolver is not the
end of the world, but you know my opinion.

lgtm either way

https://codereview.chromium.org/1758013002/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/md_settings_ui.h (right):

https://codereview.chromium.org/1758013002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/md_settings_ui.h:34: void ResolveCallback(const
base::Value& callback_id,
ResolveJavascriptCallback (maayyyybbe? i guess this isn't really that ambiguous
currently, except maybe with base::Callback)

[dpapad, 2016-03-03 20:18:53]

We could not use this PromiseResolver is print_preview, until after this CL
(sine it was residing in chrome/test/...). WIP
https://codereview.chromium.org/1755423003.

https://codereview.chromium.org/1758013002/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/md_settings_ui.h (right):

https://codereview.chromium.org/1758013002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/md_settings_ui.h:34: void ResolveCallback(const
base::Value& callback_id,
On 2016/03/03 at 20:11:08, Dan Beam wrote:
> ResolveJavascriptCallback (maayyyybbe? i guess this isn't really that
ambiguous currently, except maybe with base::Callback)

Done.

[dpapad, 2016-03-03 20:20:44]

Description was changed from

==========
WebUI: Allow rejecting a promise that is returned from cr.sendWithPromise().

 - Moving PromiseResolver helper class such that it can be used from cr.js
 - Updating cr.webUIResponse to accept an isSuccess parameter.
 - Renaming SettingsPageUIHandler#CallJavascriptCallback to ResolveCallback, and
   adding new RejectCallback method.
 - Adding test.

BUG=591266
==========

to

==========
WebUI: Allow rejecting a promise that is returned from cr.sendWithPromise().

 - Moving PromiseResolver helper class such that it can be used from cr.js
 - Updating cr.webUIResponse to accept an isSuccess parameter.
 - Renaming SettingsPageUIHandler#CallJavascriptCallback to
   ResolveJavascriptCallback, and adding new RejectJavascriptCallback method.
 - Adding test.

BUG=591266
==========

[dpapad, 2016-03-03 20:21:20]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-03-03 20:21:20]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1758013002/#ps180001
(title: "Re-rename")

[commit-bot: I haz the power, 2016-03-03 20:22:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1758013002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1758013002/180001

[commit-bot: I haz the power, 2016-03-03 21:57:00]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-03-03 21:59:07]

Description was changed from

==========
WebUI: Allow rejecting a promise that is returned from cr.sendWithPromise().

 - Moving PromiseResolver helper class such that it can be used from cr.js
 - Updating cr.webUIResponse to accept an isSuccess parameter.
 - Renaming SettingsPageUIHandler#CallJavascriptCallback to
   ResolveJavascriptCallback, and adding new RejectJavascriptCallback method.
 - Adding test.

BUG=591266
==========

to

==========
WebUI: Allow rejecting a promise that is returned from cr.sendWithPromise().

 - Moving PromiseResolver helper class such that it can be used from cr.js
 - Updating cr.webUIResponse to accept an isSuccess parameter.
 - Renaming SettingsPageUIHandler#CallJavascriptCallback to
   ResolveJavascriptCallback, and adding new RejectJavascriptCallback method.
 - Adding test.

BUG=591266

Committed: https://crrev.com/8b4b5952c3259661772fb882053bbe67b9de2a67
Cr-Commit-Position: refs/heads/master@{#379104}
==========

[commit-bot: I haz the power, 2016-03-03 21:59:08]

Patchset 10 (id:??) landed as
https://crrev.com/8b4b5952c3259661772fb882053bbe67b9de2a67
Cr-Commit-Position: refs/heads/master@{#379104}