Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2651793007:
Implemented StructTraits for cc::TextureMailbox (Closed)
|
Created:
3 years, 11 months ago by Saman Sami Modified:
3 years, 10 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, cc-bugs_chromium.org, darin (slow to review) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplemented StructTraits for cc::TextureMailbox
cc::CopyOutputRequest, which we soon need to send over mojo, has a
cc::TextureMailbox. This CL creates a mojo struct for cc::TextureMailbox
to be used in the mojo struct of cc::CopyOutputRequest in a future CL.
We don't send shared_bitmap_ over IPC, and it's not a priority because
cc::CopyOutputRequest doesn't need it.
BUG=672071
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2651793007
Cr-Commit-Position: refs/heads/master@{#446811}
Committed: https://chromium.googlesource.com/chromium/src/+/bf8eacc94d36e3718e5b74d5915f50422932e04d
Patch Set 1 #
Total comments: 6
Patch Set 2 : c #Patch Set 3 : c #Patch Set 4 : c #
Total comments: 2
Patch Set 5 : c #
Total comments: 4
Patch Set 6 : c #
Messages
Total messages: 55 (40 generated)
Description was changed from ========== Implemented StructTraits for cc::TextureMailbox BUG=672071 ========== to ========== Implemented StructTraits for cc::TextureMailbox BUG=672071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by samans@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...
fsamuel@chromium.org changed reviewers: + fsamuel@chromium.org
Minor things. Also, this needs a test. Please add a description of why you're adding this StructTraits. https://codereview.chromium.org/2651793007/diff/1/cc/ipc/texture_mailbox_stru... File cc/ipc/texture_mailbox_struct_traits.h (right): https://codereview.chromium.org/2651793007/diff/1/cc/ipc/texture_mailbox_stru... cc/ipc/texture_mailbox_struct_traits.h:15: static const gpu::MailboxHolder mailbox_holder( pass by const ref instead. const gpu::MailboxHolder& to avoid an extra copy. https://codereview.chromium.org/2651793007/diff/1/cc/ipc/texture_mailbox_stru... cc/ipc/texture_mailbox_struct_traits.h:20: static const gfx::Size size_in_pixels(const cc::TextureMailbox& input) { const gfx::Size& https://codereview.chromium.org/2651793007/diff/1/cc/ipc/texture_mailbox_stru... cc/ipc/texture_mailbox_struct_traits.h:50: static const gfx::ColorSpace color_space(const cc::TextureMailbox& input) { gfx::ColorSpace. No need for const
Description was changed from ========== Implemented StructTraits for cc::TextureMailbox BUG=672071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Implemented StructTraits for cc::TextureMailbox cc::CopyOutputRequest, which we soon need to send over mojo, has a cc::TextureMailbox. This CL creates a mojo struct for cc::TextureMailbox to be used in the mojo struct of cc::CopyOutputRequest in a future CL. We don't send shared_bitmap_ over IPC, and it's not a priority because cc::CopyOutputRequest doesn't need it. BUG=672071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
samans@chromium.org changed reviewers: - fsamuel@chromium.org
The CQ bit was checked by samans@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: 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_presub...)
The CQ bit was checked by samans@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 checked by samans@chromium.org to run a CQ dry run
samans@chromium.org changed reviewers: + fsamuel@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2651793007/diff/1/cc/ipc/texture_mailbox_stru... File cc/ipc/texture_mailbox_struct_traits.h (right): https://codereview.chromium.org/2651793007/diff/1/cc/ipc/texture_mailbox_stru... cc/ipc/texture_mailbox_struct_traits.h:15: static const gpu::MailboxHolder mailbox_holder( On 2017/01/25 22:03:47, Fady Samuel wrote: > pass by const ref instead. > > const gpu::MailboxHolder& to avoid an extra copy. Done. https://codereview.chromium.org/2651793007/diff/1/cc/ipc/texture_mailbox_stru... cc/ipc/texture_mailbox_struct_traits.h:20: static const gfx::Size size_in_pixels(const cc::TextureMailbox& input) { On 2017/01/25 22:03:47, Fady Samuel wrote: > const gfx::Size& Done. https://codereview.chromium.org/2651793007/diff/1/cc/ipc/texture_mailbox_stru... cc/ipc/texture_mailbox_struct_traits.h:50: static const gfx::ColorSpace color_space(const cc::TextureMailbox& input) { On 2017/01/25 22:03:47, Fady Samuel wrote: > gfx::ColorSpace. No need for const const gfx::ColorSpace&
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_presub...)
LGTM. Please pass to danakj@ and tsepez@. You can figure out the bitmap in the next CL.
samans@chromium.org changed reviewers: + danakj@chromium.org, tsepez@chromium.org
tsepez@: Please review IPC. danakj@: Please review cc/
The CQ bit was checked by samans@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: 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_presub...)
samans@chromium.org changed reviewers: + sky@chromium.org
sky@: Please review cc/resources/DEPS
mojom LGTM
The CQ bit was checked by samans@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: 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_presub...)
https://codereview.chromium.org/2651793007/diff/60001/cc/resources/DEPS File cc/resources/DEPS (right): https://codereview.chromium.org/2651793007/diff/60001/cc/resources/DEPS#newcode2 cc/resources/DEPS:2: "+mojo/public/cpp/bindings/struct_traits.h", Don't you need to update deps in a BUILD.gn too?
The CQ bit was checked by samans@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2651793007/diff/60001/cc/resources/DEPS File cc/resources/DEPS (right): https://codereview.chromium.org/2651793007/diff/60001/cc/resources/DEPS#newcode2 cc/resources/DEPS:2: "+mojo/public/cpp/bindings/struct_traits.h", On 2017/01/26 18:42:38, sky wrote: > Don't you need to update deps in a BUILD.gn too? Done.
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: 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_presub...)
LGTM
samans@chromium.org changed reviewers: + vmpstr@chromium.org
vmpstr@: Please review cc/
https://codereview.chromium.org/2651793007/diff/80001/cc/ipc/texture_mailbox.... File cc/ipc/texture_mailbox.mojom (right): https://codereview.chromium.org/2651793007/diff/80001/cc/ipc/texture_mailbox.... cc/ipc/texture_mailbox.mojom:11: // TODO (samans): Add shared_bitmap. Typically no space between TODO and (. Can you file a bug for this if it doesn't exist yet, and reference it in the comment please https://codereview.chromium.org/2651793007/diff/80001/cc/ipc/texture_mailbox_... File cc/ipc/texture_mailbox_struct_traits.h (right): https://codereview.chromium.org/2651793007/diff/80001/cc/ipc/texture_mailbox_... cc/ipc/texture_mailbox_struct_traits.h:29: #if defined(OS_ANDROID) I assume the android compiler is OK with return a; return b;? Some compilers would warn about unreachable code
The CQ bit was checked by samans@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...
PTAL https://codereview.chromium.org/2651793007/diff/80001/cc/ipc/texture_mailbox.... File cc/ipc/texture_mailbox.mojom (right): https://codereview.chromium.org/2651793007/diff/80001/cc/ipc/texture_mailbox.... cc/ipc/texture_mailbox.mojom:11: // TODO (samans): Add shared_bitmap. On 2017/01/27 21:08:05, vmpstr wrote: > Typically no space between TODO and (. > > Can you file a bug for this if it doesn't exist yet, and reference it in the > comment please Done. https://codereview.chromium.org/2651793007/diff/80001/cc/ipc/texture_mailbox_... File cc/ipc/texture_mailbox_struct_traits.h (right): https://codereview.chromium.org/2651793007/diff/80001/cc/ipc/texture_mailbox_... cc/ipc/texture_mailbox_struct_traits.h:29: #if defined(OS_ANDROID) On 2017/01/27 21:08:05, vmpstr wrote: > I assume the android compiler is OK with > > return a; > return b;? > > Some compilers would warn about unreachable code It doesn't seem to mind, but to avoid possible problems in the future. I'll fix this.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by samans@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, fsamuel@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2651793007/#ps100001 (title: "c")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1485557595259660,
"parent_rev": "a05a1702da44842fed11d76a18a1452c06421473", "commit_rev":
"bf8eacc94d36e3718e5b74d5915f50422932e04d"}
Message was sent while issue was closed.
Description was changed from ========== Implemented StructTraits for cc::TextureMailbox cc::CopyOutputRequest, which we soon need to send over mojo, has a cc::TextureMailbox. This CL creates a mojo struct for cc::TextureMailbox to be used in the mojo struct of cc::CopyOutputRequest in a future CL. We don't send shared_bitmap_ over IPC, and it's not a priority because cc::CopyOutputRequest doesn't need it. BUG=672071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Implemented StructTraits for cc::TextureMailbox cc::CopyOutputRequest, which we soon need to send over mojo, has a cc::TextureMailbox. This CL creates a mojo struct for cc::TextureMailbox to be used in the mojo struct of cc::CopyOutputRequest in a future CL. We don't send shared_bitmap_ over IPC, and it's not a priority because cc::CopyOutputRequest doesn't need it. BUG=672071 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2651793007 Cr-Commit-Position: refs/heads/master@{#446811} Committed: https://chromium.googlesource.com/chromium/src/+/bf8eacc94d36e3718e5b74d5915f... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/bf8eacc94d36e3718e5b74d5915f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
