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

Issue 2188143002: Move generated Mojo C++ error reporting code out of line (Closed)

Created:
4 years, 4 months ago by tibell
Modified:
4 years, 4 months ago
Reviewers:
yzshen1
CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@before-sharing
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Move generated Mojo C++ error reporting code out of line Also switch from scoped_refptr to unique_ptr, as we only have one owner. Savings: SafeJsonParserStub::AcceptWithResponder: 42 bytes SafeJsonParser_Parse_ForwardToCallback: 44 bytes We lose a few bytes (but come out positive in the end) due to fixing the incorrect Ninja template expansions in the error messages. BUG=597125 Committed: https://crrev.com/029b671b558428d07b0054e83451253e3a5db225 Cr-Commit-Position: refs/heads/master@{#408901}

Patch Set 1 #

Patch Set 2 : Add copyright statement #

Patch Set 3 : Don't factor out ProxyToResponder #

Patch Set 4 : Fix compilation #

Patch Set 5 : unique_ptr needs to be a friend class #

Patch Set 6 : Use public destructor instead of friend class #

Total comments: 6

Patch Set 7 : Address review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+47 lines, -35 lines) Patch
M mojo/public/cpp/bindings/lib/validation_errors.h View 1 2 3 4 5 6 2 chunks +8 lines, -0 lines 0 comments Download
M mojo/public/cpp/bindings/lib/validation_errors.cc View 1 2 3 4 5 6 1 chunk +11 lines, -0 lines 0 comments Download
M mojo/public/tools/bindings/generators/cpp_templates/interface_definition.tmpl View 1 2 3 4 5 6 8 chunks +28 lines, -35 lines 0 comments Download

Messages

Total messages: 41 (31 generated)
tibell
Fix compilation
4 years, 4 months ago (2016-07-28 07:21:51 UTC) #9
tibell
unique_ptr needs to be a friend class
4 years, 4 months ago (2016-07-29 00:17:22 UTC) #14
tibell
Use public destructor instead of friend class
4 years, 4 months ago (2016-07-29 01:16:06 UTC) #20
tibell
I'm still reading through the assembly for the generated code so hopefully I'll find more ...
4 years, 4 months ago (2016-07-29 01:26:15 UTC) #25
yzshen1
LGTM with a few nits. Thanks! https://codereview.chromium.org/2188143002/diff/100001/mojo/public/cpp/bindings/lib/validation_errors.h File mojo/public/cpp/bindings/lib/validation_errors.h (right): https://codereview.chromium.org/2188143002/diff/100001/mojo/public/cpp/bindings/lib/validation_errors.h#newcode84 mojo/public/cpp/bindings/lib/validation_errors.h:84: const base::StringPiece& description ...
4 years, 4 months ago (2016-07-31 07:16:32 UTC) #28
tibell
Address review comments
4 years, 4 months ago (2016-07-31 23:49:49 UTC) #29
tibell
https://codereview.chromium.org/2188143002/diff/100001/mojo/public/cpp/bindings/lib/validation_errors.h File mojo/public/cpp/bindings/lib/validation_errors.h (right): https://codereview.chromium.org/2188143002/diff/100001/mojo/public/cpp/bindings/lib/validation_errors.h#newcode84 mojo/public/cpp/bindings/lib/validation_errors.h:84: const base::StringPiece& description = ""); On 2016/07/31 07:16:32, yzshen1 ...
4 years, 4 months ago (2016-07-31 23:50:29 UTC) #32
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/2188143002/120001
4 years, 4 months ago (2016-08-01 02:54:39 UTC) #37
commit-bot: I haz the power
Committed patchset #7 (id:120001)
4 years, 4 months ago (2016-08-01 02:57:32 UTC) #39
commit-bot: I haz the power
4 years, 4 months ago (2016-08-01 02:59:32 UTC) #41
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/029b671b558428d07b0054e83451253e3a5db225
Cr-Commit-Position: refs/heads/master@{#408901}

Powered by Google App Engine
This is Rietveld 408576698