Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2905833004:
[OverlayWindow] Add platform-independent window and views implementation. (Closed)
Description[OverlayWindow] Add platform-independent window and views implementation.
This change adds a general window that overlays other windows, initially for the use case of picture in picture. This includes a partially stubbed views implementation. Cocoa work will be done in a separate CL.
BUG=726621
Review-Url: https://codereview.chromium.org/2905833004
Cr-Commit-Position: refs/heads/master@{#488150}
Committed: https://chromium.googlesource.com/chromium/src/+/36debc58c81094f47e733cf951afbd96c1ef4070
Patch Set 1 #
Total comments: 10
Patch Set 2 : Rebase. #Patch Set 3 : Changes per mlamouri@'s comments. #Patch Set 4 : Rebase. #Patch Set 5 : Rename PIP to Overlay. #
Total comments: 9
Patch Set 6 : Changes per sky@'s comments. #
Total comments: 2
Patch Set 7 : Rebase. #Patch Set 8 : Remove BaseWindow subclassing per conversation with sky@. #
Total comments: 10
Patch Set 9 : Rebase. #Patch Set 10 : Changes per sky@ and mlamouri@'s comments. #
Total comments: 6
Patch Set 11 : Changes per sky@'s comments. #Patch Set 12 : Rebase. #
Messages
Total messages: 75 (51 generated)
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...
Description was changed from ========== views ========== to ========== [Picture in Picture] Add window and views implementation. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== [Picture in Picture] Add window and views implementation. ========== to ========== [Picture in Picture] Add platform-independent window and views implementation. BUG=726621 ==========
Description was changed from ========== [Picture in Picture] Add platform-independent window and views implementation. BUG=726621 ========== to ========== [PIP] Add platform-independent window and views implementation. BUG=726621 ==========
apacible@chromium.org changed reviewers: + mlamouri@chromium.org, pkasting@chromium.org
Description was changed from ========== [PIP] Add platform-independent window and views implementation. BUG=726621 ========== to ========== [PIP] Add platform-independent window and views implementation. This change adds a basic window for picture and picture. This includes a partially stubbed views implementation. Cocoa work will be done in a separate CL. BUG=726621 ==========
PTAL, thanks! pkasting: Please advise on an appropriate directory for the PiP window files; we discussed not adding more to c/b/ui, but given the other contents of c/b/ui, this still feels like the most natural place to put the contents. I have also renamed the files/directories to shorten picture-in-picture to pip. This change is meant to be a simple, initial change that will be expanded upon / shelled out once the directory groundwork is completed.
Note: I'm out until monday, so I'll reply then.
Left a few comments but it sounds that a file might be missing. https://codereview.chromium.org/2905833004/diff/1/chrome/browser/ui/BUILD.gn File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2905833004/diff/1/chrome/browser/ui/BUILD.gn#... chrome/browser/ui/BUILD.gn:819: "pip/pip_window.h", Same as below, what about "picture_in_picture/picture_in_picture_window.h"? FWIW, I don't expect us to have much stuff in that directory as chrome/browser/ui/ isn't used by Android so it will only implement the desktop window. The cross-platform implementation will have to be at a different layer. https://codereview.chromium.org/2905833004/diff/1/chrome/browser/ui/pip/pip_w... File chrome/browser/ui/pip/pip_window.h (right): https://codereview.chromium.org/2905833004/diff/1/chrome/browser/ui/pip/pip_w... chrome/browser/ui/pip/pip_window.h:11: class PipWindow : public ui::BaseWindow { Name: what about PictureInPictureWindow? It's a mouthful but I'm afraid "Pip" might be unclear for most. https://codereview.chromium.org/2905833004/diff/1/chrome/browser/ui/pip/pip_w... chrome/browser/ui/pip/pip_window.h:14: virtual ~PipWindow() {} Aren't you missing the implementation file? Also, would be better to move definition there. https://codereview.chromium.org/2905833004/diff/1/chrome/browser/ui/pip/pip_w... chrome/browser/ui/pip/pip_window.h:18: static std::unique_ptr<PipWindow> Create(); What's the difference between ::Create() and the ctor if both are public? https://codereview.chromium.org/2905833004/diff/1/chrome/browser/ui/views/pip... File chrome/browser/ui/views/pip/pip_window_views.cc (right): https://codereview.chromium.org/2905833004/diff/1/chrome/browser/ui/views/pip... chrome/browser/ui/views/pip/pip_window_views.cc:14: PipWindowViews::~PipWindowViews() {} instead of {} you can do `= default;`
On 2017/06/05 at 13:09:34, mlamouri wrote: > Left a few comments but it sounds that a file might be missing. Hmm, BUILD.gn doesn't mention the file. I was wondering how it could compile but I see that pip_window is never used. Should this not be included in this CL then?
Description was changed from ========== [PIP] Add platform-independent window and views implementation. This change adds a basic window for picture and picture. This includes a partially stubbed views implementation. Cocoa work will be done in a separate CL. BUG=726621 ========== to ========== [PIP] Add platform-independent window and views implementation. This change adds a basic window for picture in picture. This includes a partially stubbed views implementation. Cocoa work will be done in a separate CL. BUG=726621 ==========
pkasting@chromium.org changed reviewers: + sky@chromium.org
I think sky is a better reviewer than me here. Questions: * Since this is meant to be able to float atop other system apps and in general not be confined to the Chrome window, I wonder whether "picture in picture" is even an appropriate name for the functionality. This seems sorta like OverlayWindow or DedicatedVideoWindow or something to me. I don't mean to bikeshed, it's just that PiP implies something (to me) about the relationship of this window to the rest of the UI that isn't actually true. * I feel like this belongs in ui/ instead of c/b/ui/ because it's not actually part of the browser window/chrome. I'd like Scott's opinion.
On 2017/06/05 19:23:50, Peter Kasting wrote: > I think sky is a better reviewer than me here. > > Questions: > * Since this is meant to be able to float atop other system apps and in general > not be confined to the Chrome window, I wonder whether "picture in picture" is > even an appropriate name for the functionality. This seems sorta like > OverlayWindow or DedicatedVideoWindow or something to me. I don't mean to > bikeshed, it's just that PiP implies something (to me) about the relationship of > this window to the rest of the UI that isn't actually true. > > * I feel like this belongs in ui/ instead of c/b/ui/ because it's not actually > part of the browser window/chrome. I'd like Scott's opinion. I agree with Peter's suggestion on naming. Either of his suggestions much better convey what this window is as well as what it is for. As to the location, I don't know what the dependencies of this code are going to be. Is it going to have dependencies on chrome code? Either way I'm inclined to leave it in chrome until we have a need to use it else where.
pkasting, sky -- Thanks for the feedback. I'm inclined to OverlayWindow, as we may want to expand this to something other than video down the line (not soon). How does that sound? mlamouri? As for dependencies -- that's a little handwavy right now. Would you be opposed to putting it in chrome/ initially, and relocating if we see fit in the future? https://codereview.chromium.org/2905833004/diff/1/chrome/browser/ui/BUILD.gn File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2905833004/diff/1/chrome/browser/ui/BUILD.gn#... chrome/browser/ui/BUILD.gn:819: "pip/pip_window.h", On 2017/06/05 13:09:33, mlamouri wrote: > Same as below, what about "picture_in_picture/picture_in_picture_window.h"? > > FWIW, I don't expect us to have much stuff in that directory as > chrome/browser/ui/ isn't used by Android so it will only implement the desktop > window. The cross-platform implementation will have to be at a different layer. From a previous conversation with pkasting, spelling out picture_in_picture was considered too wordy. https://codereview.chromium.org/2905833004/diff/1/chrome/browser/ui/pip/pip_w... File chrome/browser/ui/pip/pip_window.h (right): https://codereview.chromium.org/2905833004/diff/1/chrome/browser/ui/pip/pip_w... chrome/browser/ui/pip/pip_window.h:11: class PipWindow : public ui::BaseWindow { On 2017/06/05 13:09:33, mlamouri wrote: > Name: what about PictureInPictureWindow? It's a mouthful but I'm afraid "Pip" > might be unclear for most. See other comment on naming. https://codereview.chromium.org/2905833004/diff/1/chrome/browser/ui/pip/pip_w... chrome/browser/ui/pip/pip_window.h:14: virtual ~PipWindow() {} On 2017/06/05 13:09:33, mlamouri wrote: > Aren't you missing the implementation file? > > Also, would be better to move definition there. It's implemented in pip_window_views.cc/h. The Mac version would be a *cocoa.h/mm file. https://codereview.chromium.org/2905833004/diff/1/chrome/browser/ui/pip/pip_w... chrome/browser/ui/pip/pip_window.h:18: static std::unique_ptr<PipWindow> Create(); On 2017/06/05 13:09:33, mlamouri wrote: > What's the difference between ::Create() and the ctor if both are public? This is implemented on the platform-class level, which creates the corresponding views or cocoa Window. This follows the pattern currently used for other UI elements with different platform implementations. https://codereview.chromium.org/2905833004/diff/1/chrome/browser/ui/views/pip... File chrome/browser/ui/views/pip/pip_window_views.cc (right): https://codereview.chromium.org/2905833004/diff/1/chrome/browser/ui/views/pip... chrome/browser/ui/views/pip/pip_window_views.cc:14: PipWindowViews::~PipWindowViews() {} On 2017/06/05 13:09:33, mlamouri wrote: > instead of {} you can do `= default;` Done.
On 2017/06/13 at 07:58:09, apacible wrote: > pkasting, sky -- Thanks for the feedback. I'm inclined to OverlayWindow, as we may want to expand this to something other than video down the line (not soon). How does that sound? mlamouri? SGTM. I am generally not a fan of designing based on possible future unclear requirements but I'm outnumbered here and I don't have a strong opinion on the name, really. I just thought "pip" probably meant nothing for an average chrome developer. > As for dependencies -- that's a little handwavy right now. Would you be opposed to putting it in chrome/ initially, and relocating if we see fit in the future? sgtm too.
pkasting@chromium.org changed reviewers: - pkasting@chromium.org
I'm fine with the idea of leaving this in chrome for now; that can be revisited later. Sounds like maybe OverlayWindow is seen as better than PipWindow. With that my feedback is exhausted, so I will remove myself :)
Description was changed from ========== [PIP] Add platform-independent window and views implementation. This change adds a basic window for picture in picture. This includes a partially stubbed views implementation. Cocoa work will be done in a separate CL. BUG=726621 ========== to ========== [PIP] Add platform-independent window and views implementation. This change adds a general window that overlays other windows, initially for the use case of picture in picture. This includes a partially stubbed views implementation. Cocoa work will be done in a separate CL. BUG=726621 ==========
Patchset #5 (id:80001) has been deleted
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...
Description was changed from ========== [PIP] Add platform-independent window and views implementation. This change adds a general window that overlays other windows, initially for the use case of picture in picture. This includes a partially stubbed views implementation. Cocoa work will be done in a separate CL. BUG=726621 ========== to ========== [OverlayWindow] Add platform-independent window and views implementation. This change adds a general window that overlays other windows, initially for the use case of picture in picture. This includes a partially stubbed views implementation. Cocoa work will be done in a separate CL. BUG=726621 ==========
Rebased and renamed. PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2905833004/diff/100001/chrome/browser/ui/over... File chrome/browser/ui/overlay/overlay_window.h (right): https://codereview.chromium.org/2905833004/diff/100001/chrome/browser/ui/over... chrome/browser/ui/overlay/overlay_window.h:11: class OverlayWindow : public ui::BaseWindow { Please add a description. https://codereview.chromium.org/2905833004/diff/100001/chrome/browser/ui/over... chrome/browser/ui/overlay/overlay_window.h:24: bool IsActive() const override; Why do you have these declarations with no definitions? https://codereview.chromium.org/2905833004/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/overlay/overlay_window_views.cc (right): https://codereview.chromium.org/2905833004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/overlay/overlay_window_views.cc:14: OverlayWindowViews::~OverlayWindowViews() = default; It's not clear what the ownership model is you're going for here. The way you have it now the widget_ may outlive OverlayWindowViews, e.g. the following leaks the widget_: { OverlayWindow::Create(); } https://codereview.chromium.org/2905833004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/overlay/overlay_window_views.cc:95: NOTREACHED(); Why do you have a bunch of NOTREACHEDs? If they are going to be NOTREACHED in the long term, what are you gaining by subclassing BaseWindow? In other words, why is OverlayWindow a BaseWindow rather than only containing the methods you care about?
https://codereview.chromium.org/2905833004/diff/100001/chrome/browser/ui/over... File chrome/browser/ui/overlay/overlay_window.h (right): https://codereview.chromium.org/2905833004/diff/100001/chrome/browser/ui/over... chrome/browser/ui/overlay/overlay_window.h:11: class OverlayWindow : public ui::BaseWindow { On 2017/06/20 16:51:39, sky wrote: > Please add a description. Done. https://codereview.chromium.org/2905833004/diff/100001/chrome/browser/ui/over... chrome/browser/ui/overlay/overlay_window.h:24: bool IsActive() const override; On 2017/06/20 16:51:39, sky wrote: > Why do you have these declarations with no definitions? These are defined in the platform-specific classes. https://codereview.chromium.org/2905833004/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/overlay/overlay_window_views.cc (right): https://codereview.chromium.org/2905833004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/overlay/overlay_window_views.cc:14: OverlayWindowViews::~OverlayWindowViews() = default; On 2017/06/20 16:51:39, sky wrote: > It's not clear what the ownership model is you're going for here. The way you > have it now the widget_ may outlive OverlayWindowViews, e.g. the following leaks > the widget_: > > { > OverlayWindow::Create(); > } > Updated. https://codereview.chromium.org/2905833004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/overlay/overlay_window_views.cc:95: NOTREACHED(); On 2017/06/20 16:51:39, sky wrote: > Why do you have a bunch of NOTREACHEDs? If they are going to be NOTREACHED in > the long term, what are you gaining by subclassing BaseWindow? In other words, > why is OverlayWindow a BaseWindow rather than only containing the methods you > care about? These will be filled out later. A couple functions, like minimize(), will likely not do anything (need to iterate with UX) if we always want the window to appear.
https://codereview.chromium.org/2905833004/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/overlay/overlay_window_views.cc (right): https://codereview.chromium.org/2905833004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/overlay/overlay_window_views.cc:95: NOTREACHED(); On 2017/06/24 00:42:03, apacible wrote: > On 2017/06/20 16:51:39, sky wrote: > > Why do you have a bunch of NOTREACHEDs? If they are going to be NOTREACHED in > > the long term, what are you gaining by subclassing BaseWindow? In other words, > > why is OverlayWindow a BaseWindow rather than only containing the methods you > > care about? > > These will be filled out later. A couple functions, like minimize(), will likely > not do anything (need to iterate with UX) if we always want the window to > appear. It's hard for me to readily tell from this cl what are you gaining by subclassing BaseWindow?
Maybe it would be beneficial to have the follow-up CLs uploaded to resolve sky@'s concerns? https://codereview.chromium.org/2905833004/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/overlay/overlay_window_views.cc (right): https://codereview.chromium.org/2905833004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/overlay/overlay_window_views.cc:30: // TODO(apacible): Set the WidgetDelegate for more control over behavior. Maybe open bugs and link to them?
On 2017/06/27 18:35:47, mlamouri (slow) wrote: > Maybe it would be beneficial to have the follow-up CLs uploaded to resolve > sky@'s concerns? Talked to sky@ offline yesterday -- I'm removing the subclassing and if there's critical mass for usage later, we'll re-subclass. Currently finishing something before the changes here.
Patchset #7 (id:140001) has been deleted
Patchset #8 (id:180001) has been deleted
Updated to remove subclassing. https://codereview.chromium.org/2905833004/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/overlay/overlay_window_views.cc (right): https://codereview.chromium.org/2905833004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/overlay/overlay_window_views.cc:30: // TODO(apacible): Set the WidgetDelegate for more control over behavior. On 2017/06/27 18:35:47, mlamouri (slow) wrote: > Maybe open bugs and link to them? Updated general window bug (crbug/726621) with CL plans.
lgtm if sky@ is happy https://codereview.chromium.org/2905833004/diff/200001/chrome/browser/ui/over... File chrome/browser/ui/overlay/overlay_window.h (right): https://codereview.chromium.org/2905833004/diff/200001/chrome/browser/ui/over... chrome/browser/ui/overlay/overlay_window.h:16: virtual ~OverlayWindow() {} = default for both https://codereview.chromium.org/2905833004/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/overlay/overlay_window_views.cc (right): https://codereview.chromium.org/2905833004/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/overlay/overlay_window_views.cc:30: // TODO(apacible): Set the WidgetDelegate for more control over behavior. could you have this TODO and the above point to the bug number? https://codereview.chromium.org/2905833004/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/overlay/overlay_window_views.cc:65: // Retrieves the window's current bounds, including its window. Is this comment needed?
https://codereview.chromium.org/2905833004/diff/200001/chrome/browser/ui/over... File chrome/browser/ui/overlay/overlay_window.h (right): https://codereview.chromium.org/2905833004/diff/200001/chrome/browser/ui/over... chrome/browser/ui/overlay/overlay_window.h:8: #include "ui/compositor/layer.h" forward declare this (assuming you need Layer, which I'm not sure you do). https://codereview.chromium.org/2905833004/diff/200001/chrome/browser/ui/over... chrome/browser/ui/overlay/overlay_window.h:28: virtual bool IsAlwaysOnTop() const = 0; Is this really needed? Same question for GetLayer. https://codereview.chromium.org/2905833004/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/overlay/overlay_window_views.cc (right): https://codereview.chromium.org/2905833004/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/overlay/overlay_window_views.cc:65: // Retrieves the window's current bounds, including its window. On 2017/07/07 10:34:38, mlamouri (slow - plz ping) wrote: > Is this comment needed? This comment should be in overlay_window.h
Patchset #9 (id:220001) has been deleted
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/2905833004/diff/200001/chrome/browser/ui/over... File chrome/browser/ui/overlay/overlay_window.h (right): https://codereview.chromium.org/2905833004/diff/200001/chrome/browser/ui/over... chrome/browser/ui/overlay/overlay_window.h:8: #include "ui/compositor/layer.h" On 2017/07/11 18:02:25, sky wrote: > forward declare this (assuming you need Layer, which I'm not sure you do). Done. Included ui/gfx/native_widget_types.h because NativeWindow gets redefined. https://codereview.chromium.org/2905833004/diff/200001/chrome/browser/ui/over... chrome/browser/ui/overlay/overlay_window.h:16: virtual ~OverlayWindow() {} On 2017/07/07 10:34:38, mlamouri (slow - plz ping) wrote: > = default for both Done. https://codereview.chromium.org/2905833004/diff/200001/chrome/browser/ui/over... chrome/browser/ui/overlay/overlay_window.h:28: virtual bool IsAlwaysOnTop() const = 0; On 2017/07/11 18:02:25, sky wrote: > Is this really needed? Same question for GetLayer. Yes, we want these both to be exposed. https://codereview.chromium.org/2905833004/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/overlay/overlay_window_views.cc (right): https://codereview.chromium.org/2905833004/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/overlay/overlay_window_views.cc:30: // TODO(apacible): Set the WidgetDelegate for more control over behavior. On 2017/07/07 10:34:38, mlamouri (slow - plz ping) wrote: > could you have this TODO and the above point to the bug number? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 #10 (id:260001) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by apacible@chromium.org to run a CQ dry run
Patchset #10 (id:280001) 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2905833004/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/overlay/overlay_window_views.h (right): https://codereview.chromium.org/2905833004/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/overlay/overlay_window_views.h:9: #include "ui/views/widget/widget.h" Forward declare Widget. https://codereview.chromium.org/2905833004/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/overlay/overlay_window_views.h:17: void Init() override; Generally we prefix overrides with a comment as to what class the overrides is from. For example: // OverlayWindow: https://codereview.chromium.org/2905833004/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/overlay/overlay_window_views.h:29: std::unique_ptr<views::Widget> widget_; If you're going to own the widget then you should use WIDGET_OWNS_NATIVE_WIDGET in the InitParams.
The CQ bit was checked by apacible@chromium.org to run a CQ dry run
https://codereview.chromium.org/2905833004/diff/300001/chrome/browser/ui/view... File chrome/browser/ui/views/overlay/overlay_window_views.h (right): https://codereview.chromium.org/2905833004/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/overlay/overlay_window_views.h:9: #include "ui/views/widget/widget.h" On 2017/07/18 16:48:07, sky wrote: > Forward declare Widget. Done. https://codereview.chromium.org/2905833004/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/overlay/overlay_window_views.h:17: void Init() override; On 2017/07/18 16:48:07, sky wrote: > Generally we prefix overrides with a comment as to what class the overrides is > from. For example: > // OverlayWindow: Done. https://codereview.chromium.org/2905833004/diff/300001/chrome/browser/ui/view... chrome/browser/ui/views/overlay/overlay_window_views.h:29: std::unique_ptr<views::Widget> widget_; On 2017/07/18 16:48:07, sky wrote: > If you're going to own the widget then you should use WIDGET_OWNS_NATIVE_WIDGET > in the InitParams. 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: This issue passed the CQ dry run.
LGTM
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: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by apacible@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2905833004/#ps340001 (title: "Rebase.")
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": 340001, "attempt_start_ts": 1500529304963900,
"parent_rev": "663e84042e53db232a5f1cb7db77a62aea9b16ed", "commit_rev":
"1ae155b800028c93af04b22aadbba8ea81adf921"}
CQ is committing da patch.
Bot data: {"patchset_id": 340001, "attempt_start_ts": 1500529304963900,
"parent_rev": "0627c931ad4bfb9e7ccad49b75b0060d40710ea9", "commit_rev":
"36debc58c81094f47e733cf951afbd96c1ef4070"}
Message was sent while issue was closed.
Description was changed from ========== [OverlayWindow] Add platform-independent window and views implementation. This change adds a general window that overlays other windows, initially for the use case of picture in picture. This includes a partially stubbed views implementation. Cocoa work will be done in a separate CL. BUG=726621 ========== to ========== [OverlayWindow] Add platform-independent window and views implementation. This change adds a general window that overlays other windows, initially for the use case of picture in picture. This includes a partially stubbed views implementation. Cocoa work will be done in a separate CL. BUG=726621 Review-Url: https://codereview.chromium.org/2905833004 Cr-Commit-Position: refs/heads/master@{#488150} Committed: https://chromium.googlesource.com/chromium/src/+/36debc58c81094f47e733cf951af... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:340001) as https://chromium.googlesource.com/chromium/src/+/36debc58c81094f47e733cf951af... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
