Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(2160)

Issue 2144093002: Adding checks for V8 functions returning Maybe<bool> (Closed)

Created:
4 years, 5 months ago by danbev
Modified:
3 years, 9 months ago
CC:
blink-reviews, blink-reviews-bindings_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Adding checks for V8 functions returning Maybe<bool> While working on an V8 issue [1], involving adding V8_WARN_UNUSED_RESULT to functions that were missing it, we discovered that a few calls to these functions were not using the returned value. This commit allows compliation to succeed, but in most cases only adds a comment that something should be done. I'm opening this CL to get feedback on what the appropriate actions should be. To try this out I used a .gclient with a custom_deps like this: u'custom_deps': { "src/v8": "git@github.com:danbev/v8.git@064718a8921608eaf9b5eadbb7d734ec04068a87" }, [1] https://codereview.chromium.org/2135973002/ BUG=

Patch Set 1 #

Total comments: 1

Patch Set 2 : Adding NOTREACHED if Delete result IsNothing for LazyFieldGetterInner #

Total comments: 3

Patch Set 3 : Using v8CallOrCrash for V8 calls in binding sources #

Patch Set 4 : Replacing TODOs in module_system.cc with checks. #

Patch Set 5 : Add checks for V8 functions returning Maybe<bool> #

Patch Set 6 : Adding checks for V8 functions returning Maybe<bool> #

Unified diffs Side-by-side diffs Delta from patch set Stats (+27 lines, -17 lines) Patch
M AUTHORS View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M extensions/renderer/api_last_error.cc View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M extensions/renderer/module_system.cc View 1 2 3 4 5 3 chunks +10 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/ScriptPromise.cpp View 1 2 3 4 5 3 chunks +4 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/ScriptPromisePropertyBase.cpp View 1 2 3 4 1 chunk +4 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/V8Binding.cpp View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/V8PagePopupControllerBinding.cpp View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp View 1 2 3 4 5 1 chunk +4 lines, -2 lines 0 comments Download

Messages

Total messages: 42 (18 generated)
danbev
4 years, 5 months ago (2016-07-14 04:54:22 UTC) #2
jochen (gone - plz use gerrit)
+haraken for blink bindings changes https://codereview.chromium.org/2144093002/diff/1/extensions/renderer/module_system.cc File extensions/renderer/module_system.cc (right): https://codereview.chromium.org/2144093002/diff/1/extensions/renderer/module_system.cc#newcode460 extensions/renderer/module_system.cc:460: if (object->Delete(context, property).IsNothing()) { ...
4 years, 5 months ago (2016-07-14 15:03:23 UTC) #4
haraken
https://codereview.chromium.org/2144093002/diff/20001/extensions/renderer/module_system.cc File extensions/renderer/module_system.cc (right): https://codereview.chromium.org/2144093002/diff/20001/extensions/renderer/module_system.cc#newcode584 extensions/renderer/module_system.cc:584: // TODO: What action, if any, should be taken ...
4 years, 5 months ago (2016-07-15 02:38:41 UTC) #5
danbev
On 2016/07/15 02:38:41, haraken wrote: > https://codereview.chromium.org/2144093002/diff/20001/extensions/renderer/module_system.cc > File extensions/renderer/module_system.cc (right): > > https://codereview.chromium.org/2144093002/diff/20001/extensions/renderer/module_system.cc#newcode584 > ...
4 years, 5 months ago (2016-07-15 11:07:55 UTC) #6
danbev
4 years, 5 months ago (2016-07-18 06:35:41 UTC) #7
jochen (gone - plz use gerrit)
On 2016/07/15 at 11:07:55, daniel.bevenius wrote: > On 2016/07/15 02:38:41, haraken wrote: > > https://codereview.chromium.org/2144093002/diff/20001/extensions/renderer/module_system.cc ...
4 years, 5 months ago (2016-07-18 11:29:30 UTC) #8
danbev
On 2016/07/18 11:29:30, jochen wrote: > On 2016/07/15 at 11:07:55, daniel.bevenius wrote: > > On ...
4 years, 5 months ago (2016-07-18 12:14:56 UTC) #9
danbev
Updating extensions/renderer/module_system.cc with this commit.
4 years, 5 months ago (2016-07-18 14:29:58 UTC) #10
jochen (gone - plz use gerrit)
extensions/ look good
4 years, 5 months ago (2016-07-19 12:18:40 UTC) #11
haraken
WebKit LGTM
4 years, 5 months ago (2016-07-19 19:57:17 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2144093002/60001
4 years, 5 months ago (2016-07-20 05:13:51 UTC) #14
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/221243)
4 years, 5 months ago (2016-07-20 05:22:40 UTC) #16
danbev
On 2016/07/20 05:22:40, commit-bot: I haz the power wrote: > Try jobs failed on following ...
4 years, 5 months ago (2016-07-20 16:04:01 UTC) #17
jochen (gone - plz use gerrit)
also, you still need my approval :) lgtm
4 years, 5 months ago (2016-07-21 08:28:05 UTC) #18
danbev
On 2016/07/21 08:28:05, jochen wrote: > also, you still need my approval :) > > ...
4 years, 5 months ago (2016-07-21 16:52:10 UTC) #19
danbev
I've rebased this with upstream master and reapplied the changes as it has been so ...
3 years, 11 months ago (2017-01-25 06:33:57 UTC) #20
danbev
Rebased with master and tested (chrome/test:unit_tests) using .gclient: solutions = [{u'managed': False, u'name': u'src', u'url': ...
3 years, 9 months ago (2017-03-19 10:25:32 UTC) #25
jochen (gone - plz use gerrit)
I triggered a dry run. CL still lgtm, so feel free to hit the commit ...
3 years, 9 months ago (2017-03-20 07:12:50 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2144093002/100001
3 years, 9 months ago (2017-03-20 08:55:06 UTC) #33
commit-bot: I haz the power
Prior attempt to commit was detected, but we were not able to check whether the ...
3 years, 9 months ago (2017-03-20 09:06:22 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2144093002/100001
3 years, 9 months ago (2017-03-20 09:07:32 UTC) #38
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/389091)
3 years, 9 months ago (2017-03-20 09:15:28 UTC) #40
jochen (gone - plz use gerrit)
ok, this landed as https://chromium.googlesource.com/chromium/src/+/33ac37fdb8da49817a14b12205afa06d928476cb ... closing
3 years, 9 months ago (2017-03-22 09:56:33 UTC) #41
danbev
3 years, 9 months ago (2017-03-22 10:44:30 UTC) #42
Message was sent while issue was closed.
On 2017/03/22 09:56:33, jochen wrote:
> ok, this landed as
>
https://chromium.googlesource.com/chromium/src/+/33ac37fdb8da49817a14b12205af...
> ... closing

Thanks for all your help and patience on this!

Powered by Google App Engine
This is Rietveld 408576698