Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12843009:
Replace screen capture confirmation infobar with a message box. (Closed)
|
Created:
7 years, 9 months ago by Sergey Ulanov Modified:
7 years, 9 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionReplace screen capture confirmation infobar with a message box.
Currently an infobar is used to get user consent for screen capture API. We
are moving away from infobars for that API. The plan is to use modal prompt
instead of infobars (but eventually they may be replaced with something
better). This CL adds ScreenCaptureConfirmationUI interface, so that it will
be easy to replace infobars. The new ScreenCaptureConfirmationUIInfobar
implements this interface using infobars (will be removed eventually).
BUG=196287
TBR=thakis@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190149
Patch Set 1 #
Total comments: 3
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #Patch Set 5 : #
Total comments: 2
Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #
Total comments: 9
Patch Set 9 : #Patch Set 10 : #
Total comments: 4
Patch Set 11 : #Patch Set 12 : #
Messages
Total messages: 36 (0 generated)
pkasting@ - please review changes in chrome/browser/ui tommi@ - please review changes in chrome/browser/media
https://codereview.chromium.org/12843009/diff/1/chrome/browser/media/media_ca... File chrome/browser/media/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/12843009/diff/1/chrome/browser/media/media_ca... chrome/browser/media/media_capture_devices_dispatcher.cc:125: // Deny request automatically in the following cases: Please note that this is not new code - I just moved it here from screen_capture_infobar_delegate.cc .
pkasting, tommi: ping
On 2013/03/21 06:53:38, sergeyu wrote: > pkasting, tommi: ping I saw the change, unfortunately I'm WebKit sheriff at the moment, so my time is a bit in short supply. I will try to look at this within the next few days.
lgtm for chrome/browser/media
https://codereview.chromium.org/12843009/diff/1/chrome/browser/media/media_ca... File chrome/browser/media/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/12843009/diff/1/chrome/browser/media/media_ca... chrome/browser/media/media_capture_devices_dispatcher.cc:301: LOG(ERROR) << "RESET"; Don't forget to remove this debugging output...
https://codereview.chromium.org/12843009/diff/1/chrome/browser/media/media_ca... File chrome/browser/media/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/12843009/diff/1/chrome/browser/media/media_ca... chrome/browser/media/media_capture_devices_dispatcher.cc:301: LOG(ERROR) << "RESET"; On 2013/03/21 18:53:50, Greg Spencer (Chromium) wrote: > Don't forget to remove this debugging output... Done. Thanks for catching it.
On 2013/03/21 06:57:43, Peter Kasting wrote: > On 2013/03/21 06:53:38, sergeyu wrote: > > pkasting, tommi: ping > > I saw the change, unfortunately I'm WebKit sheriff at the moment, so my time is > a bit in short supply. I will try to look at this within the next few days. Thank you, Peter. We really need this to land in M27 and Greg is working on another change that is blocked by this one, so we would appreciate if you could get it done sooner. It's a mechanical change and doesn't add any new functionality - so it shouldn't take much time to review. Let me know if it's better to find another reviewer who has time for it. Thanks!
https://codereview.chromium.org/12843009/diff/9001/chrome/browser/ui/screen_c... File chrome/browser/ui/screen_capture_confirmation_ui.h (right): https://codereview.chromium.org/12843009/diff/9001/chrome/browser/ui/screen_c... chrome/browser/ui/screen_capture_confirmation_ui.h:30: const string16& title) = 0; Maybe use "application_name" instead of title? If the subclass implements the UI as a dialog, "title" is confusing, since that dialog also has a title.
https://codereview.chromium.org/12843009/diff/9001/chrome/browser/ui/screen_c... File chrome/browser/ui/screen_capture_confirmation_ui.h (right): https://codereview.chromium.org/12843009/diff/9001/chrome/browser/ui/screen_c... chrome/browser/ui/screen_capture_confirmation_ui.h:30: const string16& title) = 0; On 2013/03/21 21:44:24, Greg Spencer (Chromium) wrote: > Maybe use "application_name" instead of title? If the subclass implements the > UI as a dialog, "title" is confusing, since that dialog also has a title. Done.
+thakis
I'm concerned about the short-term hack below. Is it possible to find a general solution for that first? When letting in short-term hacks, there's always a possibility that they don't get cleaned up, so one needs to weigh likelihood of a fast cleanup (you tend to clean up after yourself), cost/benefit (seems like adding an about:flags entry for kEnableUserMediaScreenCapturing would allow you to test this in m27 just as easily), etc. https://codereview.chromium.org/12843009/diff/17001/chrome/browser/media/medi... File chrome/browser/media/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/12843009/diff/17001/chrome/browser/media/medi... chrome/browser/media/media_capture_devices_dispatcher.cc:47: // TODO(sergeyu): Remove this whitelist as soon as possible. ??
https://codereview.chromium.org/12843009/diff/17001/chrome/browser/media/medi... File chrome/browser/media/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/12843009/diff/17001/chrome/browser/media/medi... chrome/browser/media/media_capture_devices_dispatcher.cc:47: // TODO(sergeyu): Remove this whitelist as soon as possible. On 2013/03/22 18:02:52, Nico wrote: > ?? This is not new in this CL - I just moved it here from a different file. See https://codereview.chromium.org/12596011/ for the original discussion of the issue.
On 2013/03/22 18:23:03, sergeyu wrote: > https://codereview.chromium.org/12843009/diff/17001/chrome/browser/media/medi... > File chrome/browser/media/media_capture_devices_dispatcher.cc (right): > > https://codereview.chromium.org/12843009/diff/17001/chrome/browser/media/medi... > chrome/browser/media/media_capture_devices_dispatcher.cc:47: // TODO(sergeyu): > Remove this whitelist as soon as possible. > On 2013/03/22 18:02:52, Nico wrote: > > ?? > > This is not new in this CL - I just moved it here from a different file. See > https://codereview.chromium.org/12596011/ for the original discussion of the > issue. I see Peter had the same concern on that CL. You said: > That's bad wording on my part. The implementation is rather stable and based on the code that has been used for long time in Chromoting. Main problem is that we don't know how well it will perform with WebRTC stack - we need to make sure that it's not downgrade when compared to plugin-based Hangouts. This requires that it's used in real world on actual networks, so we can get feedback from the users and properly prioritize possible performance issues (and this applies to Hangouts as a whole, not only screencast feature). The usual answer to A/B testing in chrome these days is Finch. I hear they will have a feature for m27 to trigger experiments for internal IPs only if you want to whitelist a higher population of internal people (but I'm not sure there's a need for that).
https://codereview.chromium.org/12843009/diff/17001/chrome/browser/ui/screen_... File chrome/browser/ui/screen_capture_confirmation_ui_infobar.cc (right): https://codereview.chromium.org/12843009/diff/17001/chrome/browser/ui/screen_... chrome/browser/ui/screen_capture_confirmation_ui_infobar.cc:13: : infobar_service_(InfoBarService::FromWebContents(web_contents)), nit: indent 2 more
https://codereview.chromium.org/12843009/diff/17001/chrome/browser/ui/screen_... File chrome/browser/ui/screen_capture_confirmation_ui_infobar.cc (right): https://codereview.chromium.org/12843009/diff/17001/chrome/browser/ui/screen_... chrome/browser/ui/screen_capture_confirmation_ui_infobar.cc:13: : infobar_service_(InfoBarService::FromWebContents(web_contents)), On 2013/03/22 18:39:55, Nico wrote: > nit: indent 2 more Done.
On Fri, Mar 22, 2013 at 11:39 AM, <thakis@chromium.org> wrote: > On 2013/03/22 18:23:03, sergeyu wrote: > > https://codereview.chromium.**org/12843009/diff/17001/** > chrome/browser/media/media_**capture_devices_dispatcher.cc<https://codereview.chromium.org/12843009/diff/17001/chrome/browser/media/media_capture_devices_dispatcher.cc> > >> File chrome/browser/media/media_**capture_devices_dispatcher.cc (right): >> > > > https://codereview.chromium.**org/12843009/diff/17001/** > chrome/browser/media/media_**capture_devices_dispatcher.cc#**newcode47<https://codereview.chromium.org/12843009/diff/17001/chrome/browser/media/media_capture_devices_dispatcher.cc#newcode47> > >> chrome/browser/media/media_**capture_devices_dispatcher.cc:**47: // >> TODO(sergeyu): >> Remove this whitelist as soon as possible. >> On 2013/03/22 18:02:52, Nico wrote: >> > ?? >> > > This is not new in this CL - I just moved it here from a different file. >> See >> https://codereview.chromium.**org/12596011/<https://codereview.chromium.org/1... the original discussion of the >> issue. >> > > I see Peter had the same concern on that CL. You said: > > That's bad wording on my part. The implementation is rather stable and >> based >> > on > the code that has been used for long time in Chromoting. Main problem is > that we > don't know how well it will perform with WebRTC stack - we need to make > sure > that it's not downgrade when compared to plugin-based Hangouts. This > requires > that it's used in real world on actual networks, so we can get feedback > from the > users and properly prioritize possible performance issues (and this > applies to > Hangouts as a whole, not only screencast feature). > > The usual answer to A/B testing in chrome these days is Finch. I hear they > will > have a feature for m27 to trigger experiments for internal IPs only if you > want > to whitelist a higher population of internal people (but I'm not sure > there's a > need for that). > Thanks, I did know about it - will keep Finch in mind next time. I'm not sure if it would be useful here, though. This is not just a chrome feature - it would require hangouts to enable it as well. > > https://codereview.chromium.**org/12843009/<https://codereview.chromium.org/1... > -- Sergey Ulanov
> Thanks, I did know about it - will keep Finch in mind next time. I'm not > sure if it would be useful here, though. This is not just a chrome feature > - it would require hangouts to enable it as well. Sorry, you're basically saying "your feedback is fascinating, but I really need to get this in for m27 so I'll ignore it" on both this and the other review you sent me. I'll remove myself from these two reviews.
On 2013/03/22 20:17:41, Nico wrote: > > Thanks, I did know about it - will keep Finch in mind next time. I'm not > > sure if it would be useful here, though. This is not just a chrome feature > > - it would require hangouts to enable it as well. > > Sorry, you're basically saying "your feedback is fascinating, but I really need > to get this in for m27 so I'll ignore it" on both this and the other review you > sent me. I'll remove myself from these two reviews. This particular comment was about a change that is not part of this CL. I agree that you are making good point, but it's a separate issue, that's not related to this change. This specific CL just refactors how the UI is invoked and doesn't change the behavior.
Does this change actually do anything functional? It looks like it's just adding layers of indirection. If so, don't commit this, just drop this change entirely. When you want to change from an infobar to some other UI later, just delete the infobar at that point and change. Don't introduce an extra abstract and concrete class atop the infobar in the name of simplifying the later transition; that doesn't simplify things, it just increases the amount of code in the codebase, and it won't be necessary in the future world either (where you can just directly create the relevant UI then instead of directly creating an infobar now). Besides this, this moves from a static Create() function and private constructor on the infobar (which is how all infobars should be written) to a public constructor, which Infobars should basically never have. Please tell me if I've overlooked some critical factor; otherwise, r-.
On 2013/03/22 21:00:20, Peter Kasting wrote: > Does this change actually do anything functional? It looks like it's just > adding layers of indirection. If so, don't commit this, just drop this change > entirely. When you want to change from an infobar to some other UI later, just > delete the infobar at that point and change. Don't introduce an extra abstract > and concrete class atop the infobar in the name of simplifying the later > transition; that doesn't simplify things, it just increases the amount of code > in the codebase, and it won't be necessary in the future world either (where you > can just directly create the relevant UI then instead of directly creating an > infobar now). > > Besides this, this moves from a static Create() function and private constructor > on the infobar (which is how all infobars should be written) to a public > constructor, which Infobars should basically never have. > > Please tell me if I've overlooked some critical factor; otherwise, r-. Ok, I've removed the infobar now and replaced it with modal message box now. The reason I added the abstract interface initially is because we may need to have platform-specific implementation in the future, so it wasn't there just to simplify transition.
Much improved! https://codereview.chromium.org/12843009/diff/39001/chrome/browser/ui/screen_... File chrome/browser/ui/screen_capture_confirmation_dialog.cc (right): https://codereview.chromium.org/12843009/diff/39001/chrome/browser/ui/screen_... chrome/browser/ui/screen_capture_confirmation_dialog.cc:24: application_name_ = application_name; If you pass these to ShowMessageBox(), you need not keep them as members. At that point, ScreenCaptureConfirmationDialog can disappear entirely and ShowMessageBox() can simply become a private function on MediaCaptureDevicesDispatcher, simplifying this change significantly further.
https://codereview.chromium.org/12843009/diff/39001/chrome/browser/ui/screen_... File chrome/browser/ui/screen_capture_confirmation_dialog.cc (right): https://codereview.chromium.org/12843009/diff/39001/chrome/browser/ui/screen_... chrome/browser/ui/screen_capture_confirmation_dialog.cc:24: application_name_ = application_name; On 2013/03/22 22:48:30, Peter Kasting wrote: > If you pass these to ShowMessageBox(), you need not keep them as members. Done. > > At that point, ScreenCaptureConfirmationDialog can disappear entirely and > ShowMessageBox() can simply become a private function on > MediaCaptureDevicesDispatcher, simplifying this change significantly further. That's not the direction I want this code to move. We will need to modify this UI - I don't think we want to use ShowMessageBox() in long term.
On 2013/03/22 23:26:09, sergeyu wrote: > I don't think we want to use ShowMessageBox() in long term. Then it seems appropriate to make the code do what you want to do in the long term when you know what it is you want to do in the long term. In the meantime, though, there's no win to adding this class when all you need is an 8-line function in MediaCaptureDevicesDispatcher. That ensures that no matter how long it takes to figure out the permanent UI or what that UI looks like when it appears, it will be trivial to implement it without any leftover code or extra boilerplate changes. I think the overall changes to MediaCaptureDevicesDispatcher should be no longer than your changes in this patch set, considering that you already have a callback function that runs on the UI thread within this class, and can basically just insert a ShowMessageBox() call at the beginning of it for now and replace the |screen_capture_confirmation_| member with a weak pointer factory.
On 2013/03/22 23:47:27, Peter Kasting wrote: > On 2013/03/22 23:26:09, sergeyu wrote: > > I don't think we want to use ShowMessageBox() in long term. > > Then it seems appropriate to make the code do what you want to do in the long > term when you know what it is you want to do in the long term. > > In the meantime, though, there's no win to adding this class when all you need > is an 8-line function in MediaCaptureDevicesDispatcher. That ensures that no > matter how long it takes to figure out the permanent UI or what that UI looks > like when it appears, it will be trivial to implement it without any leftover > code or extra boilerplate changes. I think the overall changes to > MediaCaptureDevicesDispatcher should be no longer than your changes in this > patch set, considering that you already have a callback function that runs on > the UI thread within this class, and can basically just insert a > ShowMessageBox() call at the beginning of it for now and replace the > |screen_capture_confirmation_| member with a weak pointer factory. Ok, I removed the new class.
LGTM save seemingly-unnecessary declaration in the indicator .h https://codereview.chromium.org/12843009/diff/57001/chrome/browser/media/medi... File chrome/browser/media/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/12843009/diff/57001/chrome/browser/media/medi... chrome/browser/media/media_capture_devices_dispatcher.cc:123: if (request.video_type == content::MEDIA_SCREEN_VIDEO_CAPTURE) { Nit: Consider reversing this and then early-returning so you can unindent the long remainder of the function https://codereview.chromium.org/12843009/diff/57001/chrome/browser/media/medi... chrome/browser/media/media_capture_devices_dispatcher.cc:139: Nit: Extra blank line https://codereview.chromium.org/12843009/diff/57001/chrome/browser/media/medi... chrome/browser/media/media_capture_devices_dispatcher.cc:148: if (result == chrome::MESSAGE_BOX_RESULT_YES) { Nit: Simpler: content::MediaStreamDevices devices; if (result == chrome::MESSAGE_BOX_RESULT_YES) { devices.push_back(content::MediaStreamDevice( content::MEDIA_SCREEN_VIDEO_CAPTURE, std::string(), "Screen")); } callback.Run(devices); You could optionally go further by declaring |devices| near the top like: content::MediaStreamDevices devices; if (screen_capture_enabled && request.security_origin.SchemeIsSecure() && (request.audio_type == content::MEDIA_NO_SERVICE)) { ... if (chrome::ShowMessageBox(...) == chrome::MESSAGE_BOX_RESULT_YES) devices.push_back(...); } callback.Run(devices); This might make it clearer that you always want the callback run at the end of the block of execution. https://codereview.chromium.org/12843009/diff/57001/chrome/browser/media/medi... File chrome/browser/media/media_stream_capture_indicator.h (right): https://codereview.chromium.org/12843009/diff/57001/chrome/browser/media/medi... chrome/browser/media/media_stream_capture_indicator.h:46: void RequestAccess( This doesn't seem to be called or implemented?
https://codereview.chromium.org/12843009/diff/57001/chrome/browser/media/medi... File chrome/browser/media/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/12843009/diff/57001/chrome/browser/media/medi... chrome/browser/media/media_capture_devices_dispatcher.cc:123: if (request.video_type == content::MEDIA_SCREEN_VIDEO_CAPTURE) { On 2013/03/23 01:25:54, Peter Kasting wrote: > Nit: Consider reversing this and then early-returning so you can unindent the > long remainder of the function Done. https://codereview.chromium.org/12843009/diff/57001/chrome/browser/media/medi... chrome/browser/media/media_capture_devices_dispatcher.cc:139: On 2013/03/23 01:25:54, Peter Kasting wrote: > Nit: Extra blank line Done. https://codereview.chromium.org/12843009/diff/57001/chrome/browser/media/medi... chrome/browser/media/media_capture_devices_dispatcher.cc:148: if (result == chrome::MESSAGE_BOX_RESULT_YES) { On 2013/03/23 01:25:54, Peter Kasting wrote: > Nit: Simpler: > > content::MediaStreamDevices devices; > if (result == chrome::MESSAGE_BOX_RESULT_YES) { > devices.push_back(content::MediaStreamDevice( > content::MEDIA_SCREEN_VIDEO_CAPTURE, std::string(), "Screen")); > } > callback.Run(devices); > > You could optionally go further by declaring |devices| near the top like: > > content::MediaStreamDevices devices; > if (screen_capture_enabled && request.security_origin.SchemeIsSecure() && > (request.audio_type == content::MEDIA_NO_SERVICE)) { > ... > if (chrome::ShowMessageBox(...) == chrome::MESSAGE_BOX_RESULT_YES) > devices.push_back(...); > } > callback.Run(devices); > > This might make it clearer that you always want the callback run at the end of > the block of execution. Done. https://codereview.chromium.org/12843009/diff/57001/chrome/browser/media/medi... File chrome/browser/media/media_stream_capture_indicator.h (right): https://codereview.chromium.org/12843009/diff/57001/chrome/browser/media/medi... chrome/browser/media/media_stream_capture_indicator.h:46: void RequestAccess( On 2013/03/23 01:25:54, Peter Kasting wrote: > This doesn't seem to be called or implemented? Right, it's not supposed to be here. Removed.
https://codereview.chromium.org/12843009/diff/57001/chrome/browser/media/medi... File chrome/browser/media/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/12843009/diff/57001/chrome/browser/media/medi... chrome/browser/media/media_capture_devices_dispatcher.cc:148: if (result == chrome::MESSAGE_BOX_RESULT_YES) { On 2013/03/23 01:43:38, sergeyu wrote: > On 2013/03/23 01:25:54, Peter Kasting wrote: > > Nit: Simpler: > > > > content::MediaStreamDevices devices; > > if (result == chrome::MESSAGE_BOX_RESULT_YES) { > > devices.push_back(content::MediaStreamDevice( > > content::MEDIA_SCREEN_VIDEO_CAPTURE, std::string(), "Screen")); > > } > > callback.Run(devices); > > > > You could optionally go further by declaring |devices| near the top like: > > > > content::MediaStreamDevices devices; > > if (screen_capture_enabled && request.security_origin.SchemeIsSecure() && > > (request.audio_type == content::MEDIA_NO_SERVICE)) { > > ... > > if (chrome::ShowMessageBox(...) == chrome::MESSAGE_BOX_RESULT_YES) > > devices.push_back(...); > > } > > callback.Run(devices); > > > > This might make it clearer that you always want the callback run at the end of > > the block of execution. > > Done. Patch 9 doesn't actually show a change here? I think the first transform is a win, the second one seems much more arguable.
On 2013/03/23 01:46:17, Peter Kasting wrote: > https://codereview.chromium.org/12843009/diff/57001/chrome/browser/media/medi... > File chrome/browser/media/media_capture_devices_dispatcher.cc (right): > > https://codereview.chromium.org/12843009/diff/57001/chrome/browser/media/medi... > chrome/browser/media/media_capture_devices_dispatcher.cc:148: if (result == > chrome::MESSAGE_BOX_RESULT_YES) { > On 2013/03/23 01:43:38, sergeyu wrote: > > On 2013/03/23 01:25:54, Peter Kasting wrote: > > > Nit: Simpler: > > > > > > content::MediaStreamDevices devices; > > > if (result == chrome::MESSAGE_BOX_RESULT_YES) { > > > devices.push_back(content::MediaStreamDevice( > > > content::MEDIA_SCREEN_VIDEO_CAPTURE, std::string(), "Screen")); > > > } > > > callback.Run(devices); > > > > > > You could optionally go further by declaring |devices| near the top like: > > > > > > content::MediaStreamDevices devices; > > > if (screen_capture_enabled && request.security_origin.SchemeIsSecure() && > > > (request.audio_type == content::MEDIA_NO_SERVICE)) { > > > ... > > > if (chrome::ShowMessageBox(...) == chrome::MESSAGE_BOX_RESULT_YES) > > > devices.push_back(...); > > > } > > > callback.Run(devices); > > > > > > This might make it clearer that you always want the callback run at the end > of > > > the block of execution. > > > > Done. > > Patch 9 doesn't actually show a change here? I think the first transform is a > win, the second one seems much more arguable. Uploaded new patchset 10 with the fix. Sorry.
Cool, still LGTM https://codereview.chromium.org/12843009/diff/76001/chrome/browser/media/medi... File chrome/browser/media/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/12843009/diff/76001/chrome/browser/media/medi... chrome/browser/media/media_capture_devices_dispatcher.cc:131: bool screen_capture_enabled = CommandLine::ForCurrentProcess()->HasSwitch( Nit: Just for readability you might want to break after '=', and indent the switch name 4 more spaces, leaving the final line indented as-is. https://codereview.chromium.org/12843009/diff/76001/chrome/browser/media/medi... chrome/browser/media/media_capture_devices_dispatcher.cc:149: Nit: Extra blank line
https://codereview.chromium.org/12843009/diff/76001/chrome/browser/media/medi... File chrome/browser/media/media_capture_devices_dispatcher.cc (right): https://codereview.chromium.org/12843009/diff/76001/chrome/browser/media/medi... chrome/browser/media/media_capture_devices_dispatcher.cc:131: bool screen_capture_enabled = CommandLine::ForCurrentProcess()->HasSwitch( On 2013/03/23 19:08:40, Peter Kasting wrote: > Nit: Just for readability you might want to break after '=', and indent the > switch name 4 more spaces, leaving the final line indented as-is. Done. https://codereview.chromium.org/12843009/diff/76001/chrome/browser/media/medi... chrome/browser/media/media_capture_devices_dispatcher.cc:149: On 2013/03/23 19:08:40, Peter Kasting wrote: > Nit: Extra blank line Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/12843009/7002
Presubmit check for 12843009-7002 failed and returned exit status 1.
INFO:root:Found 5 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
chrome/chrome_browser_ui.gypi
Presubmit checks took 1.7s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyu@chromium.org/12843009/7002
gypi lgtm
Message was sent while issue was closed.
Change committed as 190149 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
