Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2474803002:
[Media Remoting] Add function to create interstitial. (Closed)
|
Created:
4 years, 1 month ago by apacible Modified:
4 years, 1 month ago CC:
chromium-reviews, feature-media-reviews_chromium.org, xjz+watch_chromium.org, miu+watch_chromium.org, chromoting-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Media Remoting] Add function to create interstitial.
This change is the first part of the UX work for the media remoting project. This is currently not integrated with the rest of the remoting work, pending other changes landing.
This change includes a function that takes a Skia canvas (with some image in pixels) and:
- Desaturates the image.
- Blurs the image.
- Draws a Cast icon and text (depending on the remoting state).
BUG=650357
Committed: https://crrev.com/7af38d67cda48d21835998a9ffd009e79e2c4c26
Cr-Commit-Position: refs/heads/master@{#431016}
Patch Set 1 #
Total comments: 24
Patch Set 2 : Changes per xjz@'s comments. #
Total comments: 21
Patch Set 3 : Changes per miu@'s comments. #
Total comments: 3
Patch Set 4 : Fix compiler errors. #
Total comments: 2
Patch Set 5 : Rebase. #
Messages
Total messages: 50 (31 generated)
Description was changed from ========== remoting interstitial BUG= ========== to ========== [Media Remoting] Add function to create interstitial. BUG=650357 ==========
Description was changed from ========== [Media Remoting] Add function to create interstitial. BUG=650357 ========== to ========== [Media Remoting] Add function to create interstitial. This change is the first part of the UX work for the media remoting project. This is currently not integrated with the rest of the remoting work, pending other changes landing. This change includes a function that takes a Skia canvas (with some image in pixels) and: - Desaturates the image. - Blurs the image. - Draws a Cast icon and text (depending on the remoting state). BUG=650357 ==========
apacible@chromium.org changed reviewers: + miu@chromium.org, xjz@chromium.org
PTAL, thanks! Exact color, sizing, and positioning subject to change pending more final redlines from UX. https://screenshot.googleplex.com/XL0W0QNyYMo.png can grab more screenshots if you'd like.
https://codereview.chromium.org/2474803002/diff/1/chrome/common/media/media_r... File chrome/common/media/media_resource_provider.cc (right): https://codereview.chromium.org/2474803002/diff/1/chrome/common/media/media_r... chrome/common/media/media_resource_provider.cc:27: case media::MEDIA_REMOTING_CAST_ERROR_TEXT: Add our build flag? #if BUILDFLAG(ENABLE_MEDIA_REMOTING) https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?rc... https://codereview.chromium.org/2474803002/diff/1/media/base/media_resources.h File media/base/media_resources.h (right): https://codereview.chromium.org/2474803002/diff/1/media/base/media_resources.... media/base/media_resources.h:30: MEDIA_REMOTING_CAST_ERROR_TEXT, ditto: add build flag here. https://codereview.chromium.org/2474803002/diff/1/media/base/media_resources.... media/base/media_resources.h:40: MEDIA_EXPORT void SetLocalizedStringProvider(LocalizedStringProvider func); Do we need MEDIA_EXPORT here? https://codereview.chromium.org/2474803002/diff/1/media/base/media_resources.... media/base/media_resources.h:49: MEDIA_EXPORT std::string GetLocalizedStringUTF8(MessageId message_id); ditto: MEDIA_EXPORT https://codereview.chromium.org/2474803002/diff/1/media/remoting/remoting_int... File media/remoting/remoting_interstitial_ui.cc (right): https://codereview.chromium.org/2474803002/diff/1/media/remoting/remoting_int... media/remoting/remoting_interstitial_ui.cc:52: SkPaint paint_text; nit: naming. This is used by both text and bitmap. Right? Maybe just "paint"? https://codereview.chromium.org/2474803002/diff/1/media/remoting/remoting_int... media/remoting/remoting_interstitial_ui.cc:61: // Center the text in the canvas. We only center the text horizontally, not vertically. right? And please move this comment down to before Line 67. https://codereview.chromium.org/2474803002/diff/1/media/remoting/remoting_int... media/remoting/remoting_interstitial_ui.cc:67: SkScalar sk_vertical_text_margin = (canvas_size.height() / 2.0) + text_size; nit: this is only the top margin. Is this actual the vertical offset to draw the text? Maybe rename it something like |text_offset_y|? I am not good at naming though. Feel free to suggest anything that sounds good to you. :) https://codereview.chromium.org/2474803002/diff/1/media/remoting/remoting_int... media/remoting/remoting_interstitial_ui.cc:70: SkScalar sk_horizontal_text_margin = nit: ditto naming. https://codereview.chromium.org/2474803002/diff/1/media/remoting/remoting_int... media/remoting/remoting_interstitial_ui.cc:84: SkScalar sk_horizontal_image_margin = nit: naming. https://codereview.chromium.org/2474803002/diff/1/media/remoting/remoting_int... media/remoting/remoting_interstitial_ui.cc:86: SkScalar sk_vertical_image_margin = nit: naming.
The CQ bit was checked by apacible@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...
https://codereview.chromium.org/2474803002/diff/1/chrome/common/media/media_r... File chrome/common/media/media_resource_provider.cc (right): https://codereview.chromium.org/2474803002/diff/1/chrome/common/media/media_r... chrome/common/media/media_resource_provider.cc:27: case media::MEDIA_REMOTING_CAST_ERROR_TEXT: On 2016/11/03 23:53:39, xjz wrote: > Add our build flag? #if BUILDFLAG(ENABLE_MEDIA_REMOTING) > https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?rc... Oh nice, added here and elsewhere. Thanks for the heads up! https://codereview.chromium.org/2474803002/diff/1/media/base/media_resources.h File media/base/media_resources.h (right): https://codereview.chromium.org/2474803002/diff/1/media/base/media_resources.... media/base/media_resources.h:30: MEDIA_REMOTING_CAST_ERROR_TEXT, On 2016/11/03 23:53:39, xjz wrote: > ditto: add build flag here. Done. https://codereview.chromium.org/2474803002/diff/1/media/base/media_resources.... media/base/media_resources.h:40: MEDIA_EXPORT void SetLocalizedStringProvider(LocalizedStringProvider func); On 2016/11/03 23:53:39, xjz wrote: > Do we need MEDIA_EXPORT here? Yes. https://codereview.chromium.org/2474803002/diff/1/media/base/media_resources.... media/base/media_resources.h:49: MEDIA_EXPORT std::string GetLocalizedStringUTF8(MessageId message_id); On 2016/11/03 23:53:39, xjz wrote: > ditto: MEDIA_EXPORT Yes, we need MEDIA_EXPORT to use the function. https://codereview.chromium.org/2474803002/diff/1/media/remoting/remoting_int... File media/remoting/remoting_interstitial_ui.cc (right): https://codereview.chromium.org/2474803002/diff/1/media/remoting/remoting_int... media/remoting/remoting_interstitial_ui.cc:52: SkPaint paint_text; On 2016/11/03 23:53:39, xjz wrote: > nit: naming. This is used by both text and bitmap. Right? Maybe just "paint"? Done. https://codereview.chromium.org/2474803002/diff/1/media/remoting/remoting_int... media/remoting/remoting_interstitial_ui.cc:61: // Center the text in the canvas. On 2016/11/03 23:53:40, xjz wrote: > We only center the text horizontally, not vertically. right? > And please move this comment down to before Line 67. Done. https://codereview.chromium.org/2474803002/diff/1/media/remoting/remoting_int... media/remoting/remoting_interstitial_ui.cc:67: SkScalar sk_vertical_text_margin = (canvas_size.height() / 2.0) + text_size; On 2016/11/03 23:53:40, xjz wrote: > nit: this is only the top margin. Is this actual the vertical offset to draw the > text? Maybe rename it something like |text_offset_y|? I am not good at naming > though. Feel free to suggest anything that sounds good to you. :) Done. https://codereview.chromium.org/2474803002/diff/1/media/remoting/remoting_int... media/remoting/remoting_interstitial_ui.cc:70: SkScalar sk_horizontal_text_margin = On 2016/11/03 23:53:40, xjz wrote: > nit: ditto naming. Done. https://codereview.chromium.org/2474803002/diff/1/media/remoting/remoting_int... media/remoting/remoting_interstitial_ui.cc:84: SkScalar sk_horizontal_image_margin = On 2016/11/03 23:53:40, xjz wrote: > nit: naming. Done. https://codereview.chromium.org/2474803002/diff/1/media/remoting/remoting_int... media/remoting/remoting_interstitial_ui.cc:86: SkScalar sk_vertical_image_margin = On 2016/11/03 23:53:39, xjz wrote: > nit: naming. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2474803002/diff/20001/media/remoting/remoting... File media/remoting/remoting_interstitial_ui.cc (right): https://codereview.chromium.org/2474803002/diff/20001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:18: scoped_refptr<VideoFrame> GetInterstitial(gfx::Size canvas_size, Is |canvas_size| ever different from existing_frame_canvas->getBaseLayerSize()? If not, then it would seem we don't need the |canvas_size| argument. If so, then we might have to include a scaling transform (and some stretching or cropping logic). https://codereview.chromium.org/2474803002/diff/20001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:20: bool isRemotingSuccessful) { ditto: (underscore_naming) https://codereview.chromium.org/2474803002/diff/20001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:48: canvas.drawBitmap(modified_bitmap, SkIntToScalar(0), SkIntToScalar(0)); I don't know for sure (maybe you already looked into this?): Is it legal to draw |modified_bitmap| onto a SkCanvas backed by the same bitmap? In other words, should the SkCanvas be backed by a different bitmap instance? https://codereview.chromium.org/2474803002/diff/20001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:66: // Center the text horizontally in the canvas. The comment says "horizontally," but the next line of code is vertical. Should the comment just say "center the text in the canvas" (since it looks like you're centering both ways)? Alternatively, maybe it'd be better to just include a comment further up in this method that explains the layout of the text and icon in the result. https://codereview.chromium.org/2474803002/diff/20001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:68: size_t display_text_width = paint.measureText(remote_playback_message.c_str(), Avoid using c_str() where possible. Since the length of the string is being passed, no NUL terminator append is necessary, and so you can just call data() instead. https://codereview.chromium.org/2474803002/diff/20001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:71: canvas.drawText(remote_playback_message.c_str(), s/c_str/data/ https://codereview.chromium.org/2474803002/diff/20001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:79: // gfx::ImageSkia and SkBitmap have the same width. Was this comment meant to be placed just before line 83 (where |sk_image_offset_x_| is initialized)? https://codereview.chromium.org/2474803002/diff/20001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:89: const gfx::Rect region_in_frame = Is |region_in_frame| a different size than the canvas? Maybe we can just use |canvas_size| everywhere here, and then we don't need this? https://codereview.chromium.org/2474803002/diff/20001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:94: media::CopyRGBToVideoFrame( I think you have to lock the pixels while reading from the bitmap: modified_bitmap.lockPixels(); media::CopyRGBToVideoFrame(...); modified_bitmap.unlockPixels(); https://codereview.chromium.org/2474803002/diff/20001/media/remoting/remoting... File media/remoting/remoting_interstitial_ui.h (right): https://codereview.chromium.org/2474803002/diff/20001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.h:19: bool isRemotingSuccessful); c++ style: is_remoting_successful
https://codereview.chromium.org/2474803002/diff/1/media/base/media_resources.h File media/base/media_resources.h (right): https://codereview.chromium.org/2474803002/diff/1/media/base/media_resources.... media/base/media_resources.h:40: MEDIA_EXPORT void SetLocalizedStringProvider(LocalizedStringProvider func); On 2016/11/04 01:25:38, apacible wrote: > On 2016/11/03 23:53:39, xjz wrote: > > Do we need MEDIA_EXPORT here? > > Yes. I think only component needs export. right? Also, even if export is needed, I don't think we are allowed to use MEDIA_EXPORT, as this is not in the media target. https://codereview.chromium.org/2474803002/diff/1/media/base/media_resources.... media/base/media_resources.h:49: MEDIA_EXPORT std::string GetLocalizedStringUTF8(MessageId message_id); On 2016/11/04 01:25:38, apacible wrote: > On 2016/11/03 23:53:39, xjz wrote: > > ditto: MEDIA_EXPORT > > Yes, we need MEDIA_EXPORT to use the function. ditto. https://codereview.chromium.org/2474803002/diff/20001/media/remoting/remoting... File media/remoting/remoting_interstitial_ui.cc (right): https://codereview.chromium.org/2474803002/diff/20001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:66: // Center the text horizontally in the canvas. On 2016/11/04 22:02:30, miu wrote: > The comment says "horizontally," but the next line of code is vertical. Should > the comment just say "center the text in the canvas" (since it looks like you're > centering both ways)? > > Alternatively, maybe it'd be better to just include a comment further up in this > method that explains the layout of the text and icon in the result. It is only centered horizontally, not vertically. Agree that this comment should be moved to before Line 70.
The CQ bit was checked by apacible@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 apacible@chromium.org to run a CQ dry run
Patchset #3 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2474803002/diff/1/media/base/media_resources.h File media/base/media_resources.h (right): https://codereview.chromium.org/2474803002/diff/1/media/base/media_resources.... media/base/media_resources.h:40: MEDIA_EXPORT void SetLocalizedStringProvider(LocalizedStringProvider func); On 2016/11/04 22:10:23, xjz wrote: > On 2016/11/04 01:25:38, apacible wrote: > > On 2016/11/03 23:53:39, xjz wrote: > > > Do we need MEDIA_EXPORT here? > > > > Yes. > > I think only component needs export. right? Also, even if export is needed, I > don't think we are allowed to use MEDIA_EXPORT, as this is not in the media > target. Ack. https://codereview.chromium.org/2474803002/diff/1/media/base/media_resources.... media/base/media_resources.h:49: MEDIA_EXPORT std::string GetLocalizedStringUTF8(MessageId message_id); On 2016/11/04 22:10:23, xjz wrote: > On 2016/11/04 01:25:38, apacible wrote: > > On 2016/11/03 23:53:39, xjz wrote: > > > ditto: MEDIA_EXPORT > > > > Yes, we need MEDIA_EXPORT to use the function. > > ditto. Removed MEDIA_EXPORT. https://codereview.chromium.org/2474803002/diff/20001/media/remoting/remoting... File media/remoting/remoting_interstitial_ui.cc (right): https://codereview.chromium.org/2474803002/diff/20001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:18: scoped_refptr<VideoFrame> GetInterstitial(gfx::Size canvas_size, On 2016/11/04 22:02:30, miu wrote: > Is |canvas_size| ever different from existing_frame_canvas->getBaseLayerSize()? > If not, then it would seem we don't need the |canvas_size| argument. If so, then > we might have to include a scaling transform (and some stretching or cropping > logic). Updated to use the |existing_frame_canvas| size. https://codereview.chromium.org/2474803002/diff/20001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:20: bool isRemotingSuccessful) { On 2016/11/04 22:02:30, miu wrote: > ditto: (underscore_naming) Done. https://codereview.chromium.org/2474803002/diff/20001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:48: canvas.drawBitmap(modified_bitmap, SkIntToScalar(0), SkIntToScalar(0)); On 2016/11/04 22:02:30, miu wrote: > I don't know for sure (maybe you already looked into this?): Is it legal to draw > |modified_bitmap| onto a SkCanvas backed by the same bitmap? In other words, > should the SkCanvas be backed by a different bitmap instance? This line is actually a no-op, so removed. https://codereview.chromium.org/2474803002/diff/20001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:66: // Center the text horizontally in the canvas. On 2016/11/04 22:10:24, xjz wrote: > On 2016/11/04 22:02:30, miu wrote: > > The comment says "horizontally," but the next line of code is vertical. Should > > the comment just say "center the text in the canvas" (since it looks like > you're > > centering both ways)? > > > > Alternatively, maybe it'd be better to just include a comment further up in > this > > method that explains the layout of the text and icon in the result. > > It is only centered horizontally, not vertically. Agree that this comment should > be moved to before Line 70. I removed this and added a general comment. https://codereview.chromium.org/2474803002/diff/20001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:68: size_t display_text_width = paint.measureText(remote_playback_message.c_str(), On 2016/11/04 22:02:30, miu wrote: > Avoid using c_str() where possible. Since the length of the string is being > passed, no NUL terminator append is necessary, and so you can just call data() > instead. Done. https://codereview.chromium.org/2474803002/diff/20001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:71: canvas.drawText(remote_playback_message.c_str(), On 2016/11/04 22:02:30, miu wrote: > s/c_str/data/ Done. https://codereview.chromium.org/2474803002/diff/20001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:79: // gfx::ImageSkia and SkBitmap have the same width. On 2016/11/04 22:02:30, miu wrote: > Was this comment meant to be placed just before line 83 (where > |sk_image_offset_x_| is initialized)? This was more a note for myself. Removed. https://codereview.chromium.org/2474803002/diff/20001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:89: const gfx::Rect region_in_frame = On 2016/11/04 22:02:30, miu wrote: > Is |region_in_frame| a different size than the canvas? Maybe we can just use > |canvas_size| everywhere here, and then we don't need this? Done. https://codereview.chromium.org/2474803002/diff/20001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:94: media::CopyRGBToVideoFrame( On 2016/11/04 22:02:30, miu wrote: > I think you have to lock the pixels while reading from the bitmap: > > modified_bitmap.lockPixels(); > media::CopyRGBToVideoFrame(...); > modified_bitmap.unlockPixels(); Done. https://codereview.chromium.org/2474803002/diff/20001/media/remoting/remoting... File media/remoting/remoting_interstitial_ui.h (right): https://codereview.chromium.org/2474803002/diff/20001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.h:19: bool isRemotingSuccessful); On 2016/11/04 22:02:30, miu wrote: > c++ style: is_remoting_successful Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)
lgtm % nit: https://codereview.chromium.org/2474803002/diff/60001/media/remoting/remoting... File media/remoting/remoting_interstitial_ui.cc (right): https://codereview.chromium.org/2474803002/diff/60001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:19: scoped_refptr<VideoFrame> GetInterstitial(SkCanvas* existing_frame_canvas, BTW--Is it ever legal to call this function with a nullptr? If not, put the null-checking burden on the caller, and make this a "documentative" argument by using: const SkCanvas& existing_frame_canvas (unless the methods you call on it are not const) https://codereview.chromium.org/2474803002/diff/60001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:63: // Both text and icon are centered horizontally. Together, they are Nice! :)
apacible@chromium.org changed reviewers: + jam@chromium.org
+cpu for media/base and chrome/common/ LGTM PTAL, thanks!
lgtm
Nice work. Since jam@ is ooo until Dec 5, can you add another reviewer for the changes in media/base and chrome/common/media/media_resource_provider.cc?
apacible@chromium.org changed reviewers: + cpu@chromium.org - jam@chromium.org
On 2016/11/07 17:53:49, xjz wrote: > Nice work. Since jam@ is ooo until Dec 5, can you add another reviewer for the > changes in media/base and chrome/common/media/media_resource_provider.cc? Oops, I added jam@ but wrote cpu@ in the message. Trying again... +cpu for media/base and chrome/common/ LGTM PTAL, thanks!
https://codereview.chromium.org/2474803002/diff/60001/media/remoting/remoting... File media/remoting/remoting_interstitial_ui.cc (right): https://codereview.chromium.org/2474803002/diff/60001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:19: scoped_refptr<VideoFrame> GetInterstitial(SkCanvas* existing_frame_canvas, On 2016/11/05 04:09:52, miu wrote: > BTW--Is it ever legal to call this function with a nullptr? If not, put the > null-checking burden on the caller, and make this a "documentative" argument by > using: > > const SkCanvas& existing_frame_canvas > > (unless the methods you call on it are not const) readPixels() isn't const.
The CQ bit was checked by apacible@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.
lgtm https://codereview.chromium.org/2474803002/diff/80001/media/base/media_resour... File media/base/media_resources.h (right): https://codereview.chromium.org/2474803002/diff/80001/media/base/media_resour... media/base/media_resources.h:31: #if BUILDFLAG(ENABLE_MEDIA_REMOTING) is BUILDFLAG() macro defined in one of the above includes? I assume build_config.h?
Thanks! https://codereview.chromium.org/2474803002/diff/80001/media/base/media_resour... File media/base/media_resources.h (right): https://codereview.chromium.org/2474803002/diff/80001/media/base/media_resour... media/base/media_resources.h:31: #if BUILDFLAG(ENABLE_MEDIA_REMOTING) On 2016/11/08 22:23:15, cpu wrote: > is BUILDFLAG() macro defined in one of the above includes? I assume > build_config.h? With media_features.h, the macro is defined with build_config.h. https://cs.chromium.org/chromium/src/out/Debug/gen/media/media_features.h
The CQ bit was checked by apacible@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xjz@chromium.org, miu@chromium.org Link to the patchset: https://codereview.chromium.org/2474803002/#ps80001 (title: "Fix compiler errors.")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by apacible@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.
The CQ bit was checked by apacible@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xjz@chromium.org, cpu@chromium.org, miu@chromium.org Link to the patchset: https://codereview.chromium.org/2474803002/#ps100001 (title: "Rebase.")
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 ========== [Media Remoting] Add function to create interstitial. This change is the first part of the UX work for the media remoting project. This is currently not integrated with the rest of the remoting work, pending other changes landing. This change includes a function that takes a Skia canvas (with some image in pixels) and: - Desaturates the image. - Blurs the image. - Draws a Cast icon and text (depending on the remoting state). BUG=650357 ========== to ========== [Media Remoting] Add function to create interstitial. This change is the first part of the UX work for the media remoting project. This is currently not integrated with the rest of the remoting work, pending other changes landing. This change includes a function that takes a Skia canvas (with some image in pixels) and: - Desaturates the image. - Blurs the image. - Draws a Cast icon and text (depending on the remoting state). BUG=650357 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [Media Remoting] Add function to create interstitial. This change is the first part of the UX work for the media remoting project. This is currently not integrated with the rest of the remoting work, pending other changes landing. This change includes a function that takes a Skia canvas (with some image in pixels) and: - Desaturates the image. - Blurs the image. - Draws a Cast icon and text (depending on the remoting state). BUG=650357 ========== to ========== [Media Remoting] Add function to create interstitial. This change is the first part of the UX work for the media remoting project. This is currently not integrated with the rest of the remoting work, pending other changes landing. This change includes a function that takes a Skia canvas (with some image in pixels) and: - Desaturates the image. - Blurs the image. - Draws a Cast icon and text (depending on the remoting state). BUG=650357 Committed: https://crrev.com/7af38d67cda48d21835998a9ffd009e79e2c4c26 Cr-Commit-Position: refs/heads/master@{#431016} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/7af38d67cda48d21835998a9ffd009e79e2c4c26 Cr-Commit-Position: refs/heads/master@{#431016} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
