[Extensions] Isolate ExtensionFunction results_ and error_

[Extensions] Isolate ExtensionFunction results_ and error_

ExtensionFunction results and errors should be set exclusively through
the responses of the function implementations in order to reduce the
likelihood of malformed or inconsistent responses.

Move results_ and error_ to be private members of ExtensionFunction. For
backwards compatibility, surface new members on legacy function
implementations (AsyncExtensionFunction, Chrome*ExtensionFunction) that
are then used in the responses of the functions.

As we continue to migrate away from these legacy implementations, we can
continue to lock down the response values.

BUG=648275
Committed: https://crrev.com/756d84a652255bad6feee508c1f9f6b8b9e1c4c5
Cr-Commit-Position: refs/heads/master@{#422270}

[Devlin, 2016-09-21 21:10:42]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 21:11:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 21:18:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-21 21:18:44]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Devlin, 2016-09-21 23:02:36]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 23:03:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 23:35:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-21 23:35:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Devlin, 2016-09-27 19:50:01]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 19:50:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 20:30:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-27 20:30:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Devlin, 2016-09-27 21:51:47]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 21:52:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 22:05:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-27 22:05:40]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Devlin, 2016-09-29 17:16:38]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 17:16:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 18:46:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 18:46:15]

Dry run: Exceeded global retry quota

[Devlin, 2016-09-29 20:46:14]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 20:46:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 22:17:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 22:17:38]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-09-29 22:19:18]

Description was changed from

==========
maybe

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

to

==========
[Extensions] Isolate ExtensionFunction results_ and error_

ExtensionFunction results and errors should be set exclusively through
the responses of the function implementations in order to reduce the
likelihood of malformed or inconsistent responses.

Move results_ and error_ to be private members of ExtensionFunction. For
backwards compatibility, surface new members on legacy function
implementations (AsyncExtensionFunction, Chrome*ExtensionFunction) that
are then used in the responses of the functions.

As we continue to migrate away from these legacy implementations, we can
continue to lock down the response values.

BUG=648275
==========

[Devlin, 2016-09-29 22:19:26]

Patchset #1 (id:1) has been deleted

[Devlin, 2016-09-29 22:19:34]

Patchset #1 (id:20001) has been deleted

[Devlin, 2016-09-29 22:19:43]

Patchset #1 (id:40001) has been deleted

[Devlin, 2016-09-29 22:19:49]

Patchset #1 (id:60001) has been deleted

[Devlin, 2016-09-29 22:19:56]

Patchset #1 (id:80001) has been deleted

[Devlin, 2016-09-29 22:20:03]

Patchset #1 (id:100001) has been deleted

[Devlin, 2016-09-29 22:20:10]

Patchset #1 (id:120001) has been deleted

[Devlin, 2016-09-29 22:20:21]

rdevlin.cronin@chromium.org changed reviewers:
+ lazyboy@chromium.org

[lazyboy, 2016-09-30 01:16:05]

https://codereview.chromium.org/2360073002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.cc (right):

https://codereview.chromium.org/2360073002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.cc:375:
SetError(error);
if (!success) SetError(), unless I'm missing something.

https://codereview.chromium.org/2360073002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.cc:495:
SetError(error);
Same here and in other places.
Unless we implicitly depend on errors to be always set, in that case, add
comments.

https://codereview.chromium.org/2360073002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_extension_function.h (right):

https://codereview.chromium.org/2360073002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/chrome_extension_function.h:76: std::string error_;
I'm finding this very hard to grok, can we use different name for these?
Probably an ugly name that suggests that this is temporary workaround (also
citing the bug# in comment) until RespondFoo only exclusively sets them.

https://codereview.chromium.org/2360073002/diff/140001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_api.cc (left):

https://codereview.chromium.org/2360073002/diff/140001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:1982: error_.clear();
I see this was added in https://codereview.chromium.org/6880219, could there be
any side effect removing this?

[Devlin, 2016-09-30 16:10:46]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 16:11:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 18:03:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 18:03:09]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Devlin, 2016-09-30 18:29:09]

https://codereview.chromium.org/2360073002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.cc (right):

https://codereview.chromium.org/2360073002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.cc:375:
SetError(error);
On 2016/09/30 01:16:04, lazyboy wrote:
> if (!success) SetError(), unless I'm missing something.

In this CL, I was trying to keep things as close to the original implementation
as possible, which for the IME api, is kind of unfortunate (see
crbug.com/641425).  But it looks like this doesn't break tests, so done.

https://codereview.chromium.org/2360073002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.cc:495:
SetError(error);
On 2016/09/30 01:16:04, lazyboy wrote:
> Same here and in other places.
> Unless we implicitly depend on errors to be always set, in that case, add
> comments.

Done.

https://codereview.chromium.org/2360073002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/chrome_extension_function.h (right):

https://codereview.chromium.org/2360073002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/chrome_extension_function.h:76: std::string error_;
On 2016/09/30 01:16:05, lazyboy wrote:
> I'm finding this very hard to grok, can we use different name for these?
> Probably an ugly name that suggests that this is temporary workaround (also
> citing the bug# in comment) until RespondFoo only exclusively sets them.

We can't name these something different, because existing Chrome* extension
function implementations directly access them (as "error_" and "results_").  The
main goal of this CL is to ensure that, as we convert to and write new extension
functions, we don't add new uses of these, but for now we need to keep the
current ones.

I've added a comment to that effect.

https://codereview.chromium.org/2360073002/diff/140001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_api.cc (left):

https://codereview.chromium.org/2360073002/diff/140001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:1982: error_.clear();
On 2016/09/30 01:16:05, lazyboy wrote:
> I see this was added in https://codereview.chromium.org/6880219, could there
be
> any side effect removing this?

There shouldn't be.  Run() should never be called if there's an existing error.

[lazyboy, 2016-09-30 20:43:32]

lgtm.

https://codereview.chromium.org/2360073002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.cc (right):

https://codereview.chromium.org/2360073002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.cc:375:
SetError(error);
On 2016/09/30 18:29:09, Devlin (catching up) wrote:
> On 2016/09/30 01:16:04, lazyboy wrote:
> > if (!success) SetError(), unless I'm missing something.
> 
> In this CL, I was trying to keep things as close to the original
implementation
> as possible, which for the IME api, is kind of unfortunate (see
> crbug.com/641425).  But it looks like this doesn't break tests, so done.

The only implementation (non-mock) of ClearComposition I found was in:
InputMethodEngineBase::ClearComposition(), which doesn't set error if it's
returning true, so this *should* be equivalent.

Same is true for
SetCandidates/SetCandidateWindowVisible/SetCursorPosition/DeleteSurroundingText.

[Devlin, 2016-10-01 01:03:21]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2016-10-01 01:03:40]

https://codereview.chromium.org/2360073002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.cc (right):

https://codereview.chromium.org/2360073002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/input_ime/input_ime_api_chromeos.cc:375:
SetError(error);
On 2016/09/30 20:43:32, lazyboy wrote:
> On 2016/09/30 18:29:09, Devlin (catching up) wrote:
> > On 2016/09/30 01:16:04, lazyboy wrote:
> > > if (!success) SetError(), unless I'm missing something.
> > 
> > In this CL, I was trying to keep things as close to the original
> implementation
> > as possible, which for the IME api, is kind of unfortunate (see
> > crbug.com/641425).  But it looks like this doesn't break tests, so done.
> 
> The only implementation (non-mock) of ClearComposition I found was in:
> InputMethodEngineBase::ClearComposition(), which doesn't set error if it's
> returning true, so this *should* be equivalent.
> 
> Same is true for
>
SetCandidates/SetCandidateWindowVisible/SetCursorPosition/DeleteSurroundingText.

Acknowledged.

[commit-bot: I haz the power, 2016-10-01 01:03:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-01 01:59:00]

Description was changed from

==========
[Extensions] Isolate ExtensionFunction results_ and error_

ExtensionFunction results and errors should be set exclusively through
the responses of the function implementations in order to reduce the
likelihood of malformed or inconsistent responses.

Move results_ and error_ to be private members of ExtensionFunction. For
backwards compatibility, surface new members on legacy function
implementations (AsyncExtensionFunction, Chrome*ExtensionFunction) that
are then used in the responses of the functions.

As we continue to migrate away from these legacy implementations, we can
continue to lock down the response values.

BUG=648275
==========

to

==========
[Extensions] Isolate ExtensionFunction results_ and error_

ExtensionFunction results and errors should be set exclusively through
the responses of the function implementations in order to reduce the
likelihood of malformed or inconsistent responses.

Move results_ and error_ to be private members of ExtensionFunction. For
backwards compatibility, surface new members on legacy function
implementations (AsyncExtensionFunction, Chrome*ExtensionFunction) that
are then used in the responses of the functions.

As we continue to migrate away from these legacy implementations, we can
continue to lock down the response values.

BUG=648275
==========

[commit-bot: I haz the power, 2016-10-01 01:59:02]

Committed patchset #2 (id:160001)

[commit-bot: I haz the power, 2016-10-01 02:01:28]

Description was changed from

==========
[Extensions] Isolate ExtensionFunction results_ and error_

ExtensionFunction results and errors should be set exclusively through
the responses of the function implementations in order to reduce the
likelihood of malformed or inconsistent responses.

Move results_ and error_ to be private members of ExtensionFunction. For
backwards compatibility, surface new members on legacy function
implementations (AsyncExtensionFunction, Chrome*ExtensionFunction) that
are then used in the responses of the functions.

As we continue to migrate away from these legacy implementations, we can
continue to lock down the response values.

BUG=648275
==========

to

==========
[Extensions] Isolate ExtensionFunction results_ and error_

ExtensionFunction results and errors should be set exclusively through
the responses of the function implementations in order to reduce the
likelihood of malformed or inconsistent responses.

Move results_ and error_ to be private members of ExtensionFunction. For
backwards compatibility, surface new members on legacy function
implementations (AsyncExtensionFunction, Chrome*ExtensionFunction) that
are then used in the responses of the functions.

As we continue to migrate away from these legacy implementations, we can
continue to lock down the response values.

BUG=648275

Committed: https://crrev.com/756d84a652255bad6feee508c1f9f6b8b9e1c4c5
Cr-Commit-Position: refs/heads/master@{#422270}
==========

[commit-bot: I haz the power, 2016-10-01 02:01:29]

Patchset 2 (id:??) landed as
https://crrev.com/756d84a652255bad6feee508c1f9f6b8b9e1c4c5
Cr-Commit-Position: refs/heads/master@{#422270}