[Extensions] Apply WARN_UNUSED_RESULT and final keyword to ExtensionFunctions.

[Extensions] Apply WARN_UNUSED_RESULT and final keyword to ExtensionFunctions.

R=rdevlin.cronin@chromium.org

Committed: https://crrev.com/ccdc96360ea64de68f0b26c4b2e75650c96f632c
Cr-Commit-Position: refs/heads/master@{#318073}

[not at google - send to devlin, 2015-02-24 01:03:22]

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

[Devlin, 2015-02-24 17:04:42]

lgtm

https://codereview.chromium.org/950023005/diff/1/chrome/browser/extensions/ch...
File chrome/browser/extensions/chrome_extension_function.h (right):

https://codereview.chromium.org/950023005/diff/1/chrome/browser/extensions/ch...
chrome/browser/extensions/chrome_extension_function.h:90: ResponseAction Run()
final;
Even though there's about a half dozen of these, I think a basic comment like
"subclasses should implement RunAsync() instead." would be worthwhile here, so
that no one comes along and thinks it was put in errantly/flippantly.

[not at google - send to devlin, 2015-02-25 16:00:31]

https://codereview.chromium.org/950023005/diff/1/chrome/browser/extensions/ch...
File chrome/browser/extensions/chrome_extension_function.h (right):

https://codereview.chromium.org/950023005/diff/1/chrome/browser/extensions/ch...
chrome/browser/extensions/chrome_extension_function.h:90: ResponseAction Run()
final;
On 2015/02/24 17:04:42, Devlin wrote:
> Even though there's about a half dozen of these, I think a basic comment like
> "subclasses should implement RunAsync() instead." would be worthwhile here, so
> that no one comes along and thinks it was put in errantly/flippantly.

"Should implement" contradicts the comment on line 72, so I've written a more
encouraging comment. Also scattered a few more comments around the place.

[not at google - send to devlin, 2015-02-25 16:00:35]

The CQ bit was checked by kalman@chromium.org

[not at google - send to devlin, 2015-02-25 16:00:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/950023005/#ps20001 (title:
"many comments")

[commit-bot: I haz the power, 2015-02-25 16:01:00]

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

[commit-bot: I haz the power, 2015-02-25 16:40:56]

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

[commit-bot: I haz the power, 2015-02-25 16:40:57]

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

[Devlin, 2015-02-25 16:41:58]

(still lgtm, just thinking out loud)

https://codereview.chromium.org/950023005/diff/20001/extensions/browser/exten...
File extensions/browser/extension_function.h (right):

https://codereview.chromium.org/950023005/diff/20001/extensions/browser/exten...
extensions/browser/extension_function.h:327: //
More as a note-to-self than anything, but we should enforce this with a CHECK
somewhere (thinking something as simple as a "said_respond_later_" private bool
of ExtensionFunction that can be set when RespondLater()ing).

https://codereview.chromium.org/950023005/diff/20001/extensions/browser/exten...
extensions/browser/extension_function.h:340: // call Respond() with |result| as
their result.
Same - I kind of wonder if we can CHECK on destruction, but shutdown/other cases
where we destroy this without finishing might make it complicated.

[not at google - send to devlin, 2015-02-25 16:53:57]

And - looks like I caught a bug :D

https://codereview.chromium.org/950023005/diff/20001/extensions/browser/exten...
File extensions/browser/extension_function.h (right):

https://codereview.chromium.org/950023005/diff/20001/extensions/browser/exten...
extensions/browser/extension_function.h:327: //
On 2015/02/25 16:41:58, Devlin wrote:
> More as a note-to-self than anything, but we should enforce this with a CHECK
> somewhere (thinking something as simple as a "said_respond_later_" private
bool
> of ExtensionFunction that can be set when RespondLater()ing).

There is a reason I didn't do this - the most logical place to put this check
was in SendResponse, but the old code uses this too - and the old code has bugs.
I'd need to introduce some extra complexity to bypass sanity checks in some
cases. Not to say it's not worthwhile, I just didn't want the churn at the time.

https://codereview.chromium.org/950023005/diff/20001/extensions/browser/exten...
extensions/browser/extension_function.h:340: // call Respond() with |result| as
their result.
On 2015/02/25 16:41:58, Devlin wrote:
> Same - I kind of wonder if we can CHECK on destruction, but shutdown/other
cases
> where we destroy this without finishing might make it complicated.

Indeed. Even more complex.

[not at google - send to devlin, 2015-02-25 17:03:14]

The CQ bit was checked by kalman@chromium.org

[not at google - send to devlin, 2015-02-25 17:03:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/950023005/#ps40001 (title:
"fix chromeos api")

[commit-bot: I haz the power, 2015-02-25 17:03:54]

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

[commit-bot: I haz the power, 2015-02-25 17:54:15]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-25 17:54:43]

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