Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2345263003:
[inspector] provide more usefull error message for non serializable value (Closed)
Description[inspector] provide more usefull error message for non serializable value
Runtime.evaluate can return result by value. We need to provide more details why method call was failed.
BUG=chromium:645640
R=dgozman@chromium.org,alph@chromium.org
Committed: https://crrev.com/0965b9b5df532d3aa0583966ca60794b54f56943
Committed: https://crrev.com/c0d1afa2d8e8df3d1fe6b25eaca751a96bda1e8d
Cr-Original-Commit-Position: refs/heads/master@{#39574}
Cr-Commit-Position: refs/heads/master@{#39606}
Patch Set 1 #
Total comments: 10
Patch Set 2 : addressed comments #
Total comments: 2
Patch Set 3 : addressed comments #
Messages
Total messages: 34 (17 generated)
Dmitry, please take a look!
https://codereview.chromium.org/2345263003/diff/1/src/inspector/InjectedScrip... File src/inspector/InjectedScript.cpp (right): https://codereview.chromium.org/2345263003/diff/1/src/inspector/InjectedScrip... src/inspector/InjectedScript.cpp:148: std::unique_ptr<protocol::Value> protocolValue = Looks like resultValue could be empty here? https://codereview.chromium.org/2345263003/diff/1/src/inspector/InjectedScrip... src/inspector/InjectedScript.cpp:183: toProtocolValue(errorString, context, wrappedObject).get(), &errors); Should check return value of toProtocolValue and use it's errorString. https://codereview.chromium.org/2345263003/diff/1/src/inspector/InjectedScrip... src/inspector/InjectedScript.cpp:273: v8::Local<v8::Value> r = function.call(hadException); Looks like r could be empty here? https://codereview.chromium.org/2345263003/diff/1/src/inspector/InjectedScrip... src/inspector/InjectedScript.cpp:278: toProtocolValue(&errorString, context, r).get(), &errors); Should check return value of toProtocolValue and use it's errorString. https://codereview.chromium.org/2345263003/diff/1/src/inspector/V8DebuggerAge... File src/inspector/V8DebuggerAgentImpl.cpp (right): https://codereview.chromium.org/2345263003/diff/1/src/inspector/V8DebuggerAge... src/inspector/V8DebuggerAgentImpl.cpp:1017: toProtocolValue(errorString, debuggerContext, objects).get(), Should check the result of toProtocolValue and return it's errorString.
All done. Please take a look! https://codereview.chromium.org/2345263003/diff/1/src/inspector/InjectedScrip... File src/inspector/InjectedScript.cpp (right): https://codereview.chromium.org/2345263003/diff/1/src/inspector/InjectedScrip... src/inspector/InjectedScript.cpp:148: std::unique_ptr<protocol::Value> protocolValue = On 2016/09/19 17:35:05, dgozman wrote: > Looks like resultValue could be empty here? Added check. I think that it should be not empty if exception wasn't thrown. https://codereview.chromium.org/2345263003/diff/1/src/inspector/InjectedScrip... src/inspector/InjectedScript.cpp:183: toProtocolValue(errorString, context, wrappedObject).get(), &errors); On 2016/09/19 17:35:05, dgozman wrote: > Should check return value of toProtocolValue and use it's errorString. Done. https://codereview.chromium.org/2345263003/diff/1/src/inspector/InjectedScrip... src/inspector/InjectedScript.cpp:273: v8::Local<v8::Value> r = function.call(hadException); On 2016/09/19 17:35:05, dgozman wrote: > Looks like r could be empty here? Done. https://codereview.chromium.org/2345263003/diff/1/src/inspector/InjectedScrip... src/inspector/InjectedScript.cpp:278: toProtocolValue(&errorString, context, r).get(), &errors); On 2016/09/19 17:35:05, dgozman wrote: > Should check return value of toProtocolValue and use it's errorString. Done. https://codereview.chromium.org/2345263003/diff/1/src/inspector/V8DebuggerAge... File src/inspector/V8DebuggerAgentImpl.cpp (right): https://codereview.chromium.org/2345263003/diff/1/src/inspector/V8DebuggerAge... src/inspector/V8DebuggerAgentImpl.cpp:1017: toProtocolValue(errorString, debuggerContext, objects).get(), On 2016/09/19 17:35:06, dgozman wrote: > Should check the result of toProtocolValue and return it's errorString. Done.
The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [inspector] provide more usefull error message for non serialable value Runtime.evaluate can return result by value. We need to provide more details why method call was failed. BUG=chromium:645640 R=dgozman@chromium.org ========== to ========== [inspector] provide more usefull error message for non serializable value Runtime.evaluate can return result by value. We need to provide more details why method call was failed. BUG=chromium:645640 R=dgozman@chromium.org ==========
lgtm
The CQ bit was checked by kozyatinskiy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-v8-committers". Note that this has nothing to do with OWNERS files.
Description was changed from ========== [inspector] provide more usefull error message for non serializable value Runtime.evaluate can return result by value. We need to provide more details why method call was failed. BUG=chromium:645640 R=dgozman@chromium.org ========== to ========== [inspector] provide more usefull error message for non serializable value Runtime.evaluate can return result by value. We need to provide more details why method call was failed. BUG=chromium:645640 R=dgozman@chromium.org,alph@chromium.org ==========
kozyatinskiy@chromium.org changed reviewers: + alph@chromium.org
Alexei, please take a look!
lgtm https://codereview.chromium.org/2345263003/diff/20001/src/inspector/StringUti... File src/inspector/StringUtil.cpp (right): https://codereview.chromium.org/2345263003/diff/20001/src/inspector/StringUti... src/inspector/StringUtil.cpp:116: *errorString = "Object has too long reference chain"; nit: Object reference chain is too long.
All done! Thanks. https://codereview.chromium.org/2345263003/diff/20001/src/inspector/StringUti... File src/inspector/StringUtil.cpp (right): https://codereview.chromium.org/2345263003/diff/20001/src/inspector/StringUti... src/inspector/StringUtil.cpp:116: *errorString = "Object has too long reference chain"; On 2016/09/21 02:05:59, alph wrote: > nit: Object reference chain is too long. Done.
The CQ bit was checked by kozyatinskiy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org, alph@chromium.org Link to the patchset: https://codereview.chromium.org/2345263003/#ps40001 (title: "addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [inspector] provide more usefull error message for non serializable value Runtime.evaluate can return result by value. We need to provide more details why method call was failed. BUG=chromium:645640 R=dgozman@chromium.org,alph@chromium.org ========== to ========== [inspector] provide more usefull error message for non serializable value Runtime.evaluate can return result by value. We need to provide more details why method call was failed. BUG=chromium:645640 R=dgozman@chromium.org,alph@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [inspector] provide more usefull error message for non serializable value Runtime.evaluate can return result by value. We need to provide more details why method call was failed. BUG=chromium:645640 R=dgozman@chromium.org,alph@chromium.org ========== to ========== [inspector] provide more usefull error message for non serializable value Runtime.evaluate can return result by value. We need to provide more details why method call was failed. BUG=chromium:645640 R=dgozman@chromium.org,alph@chromium.org Committed: https://crrev.com/0965b9b5df532d3aa0583966ca60794b54f56943 Cr-Commit-Position: refs/heads/master@{#39574} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0965b9b5df532d3aa0583966ca60794b54f56943 Cr-Commit-Position: refs/heads/master@{#39574}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2352263003/ by machenbach@chromium.org. The reason for reverting is: Breaks https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/bui... See also https://github.com/v8/v8/wiki/Blink-layout-tests.
Message was sent while issue was closed.
Description was changed from ========== [inspector] provide more usefull error message for non serializable value Runtime.evaluate can return result by value. We need to provide more details why method call was failed. BUG=chromium:645640 R=dgozman@chromium.org,alph@chromium.org Committed: https://crrev.com/0965b9b5df532d3aa0583966ca60794b54f56943 Cr-Commit-Position: refs/heads/master@{#39574} ========== to ========== [inspector] provide more usefull error message for non serializable value Runtime.evaluate can return result by value. We need to provide more details why method call was failed. BUG=chromium:645640 R=dgozman@chromium.org,alph@chromium.org Committed: https://crrev.com/0965b9b5df532d3aa0583966ca60794b54f56943 Cr-Commit-Position: refs/heads/master@{#39574} ==========
The CQ bit was checked by kozyatinskiy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Failed test as marked as needed manual rebaseline in https://codereview.chromium.org/2356233002/ .
Message was sent while issue was closed.
Description was changed from ========== [inspector] provide more usefull error message for non serializable value Runtime.evaluate can return result by value. We need to provide more details why method call was failed. BUG=chromium:645640 R=dgozman@chromium.org,alph@chromium.org Committed: https://crrev.com/0965b9b5df532d3aa0583966ca60794b54f56943 Cr-Commit-Position: refs/heads/master@{#39574} ========== to ========== [inspector] provide more usefull error message for non serializable value Runtime.evaluate can return result by value. We need to provide more details why method call was failed. BUG=chromium:645640 R=dgozman@chromium.org,alph@chromium.org Committed: https://crrev.com/0965b9b5df532d3aa0583966ca60794b54f56943 Cr-Commit-Position: refs/heads/master@{#39574} ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [inspector] provide more usefull error message for non serializable value Runtime.evaluate can return result by value. We need to provide more details why method call was failed. BUG=chromium:645640 R=dgozman@chromium.org,alph@chromium.org Committed: https://crrev.com/0965b9b5df532d3aa0583966ca60794b54f56943 Cr-Commit-Position: refs/heads/master@{#39574} ========== to ========== [inspector] provide more usefull error message for non serializable value Runtime.evaluate can return result by value. We need to provide more details why method call was failed. BUG=chromium:645640 R=dgozman@chromium.org,alph@chromium.org Committed: https://crrev.com/0965b9b5df532d3aa0583966ca60794b54f56943 Committed: https://crrev.com/c0d1afa2d8e8df3d1fe6b25eaca751a96bda1e8d Cr-Original-Commit-Position: refs/heads/master@{#39574} Cr-Commit-Position: refs/heads/master@{#39606} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c0d1afa2d8e8df3d1fe6b25eaca751a96bda1e8d Cr-Commit-Position: refs/heads/master@{#39606} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
