Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9570012:
Implement Linux Media Stream Infobar (Closed)
|
Created:
8 years, 9 months ago by Jeremy Mao Modified:
8 years, 9 months ago CC:
chromium-reviews, mflodman_chromium_OOO, tfarina Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionImplement Linux Media Stream Infobar
BUG=116249
TEST=NONE
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=125780
Patch Set 1 #
Total comments: 7
Patch Set 2 : Fix macourteau's comments #Patch Set 3 : Add a missing file #
Total comments: 7
Patch Set 4 : Rebase and fix several comments #Patch Set 5 : Rebase and fix Evan's comments #
Total comments: 2
Patch Set 6 : Fix tfarina's comments #
Total comments: 1
Patch Set 7 : Fix Peter's comments #
Total comments: 1
Messages
Total messages: 26 (0 generated)
Hi macourteau: Could you please take a look at this change when you free. Thanks & Best regards Jeremy
Hi Jeremy, Thanks for the patch and the quick turnaround! However, I'm not familiar with GTK code and I'm certainly not the right person to review that code. I've suggested a few changes, but I encourage you to get others to review it as well. You will also need approval from an OWNER to be able to submit the code. Regards, Marc-Antoine http://codereview.chromium.org/9570012/diff/1/chrome/browser/ui/gtk/infobars/... File chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.cc (right): http://codereview.chromium.org/9570012/diff/1/chrome/browser/ui/gtk/infobars/... chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.cc:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. 2011 -> 2012 http://codereview.chromium.org/9570012/diff/1/chrome/browser/ui/gtk/infobars/... chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.cc:11: #include "chrome/browser/ui/gtk/gtk_util.h" Includes should be in alphabetical order. http://codereview.chromium.org/9570012/diff/1/chrome/browser/ui/gtk/infobars/... File chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.h (right): http://codereview.chromium.org/9570012/diff/1/chrome/browser/ui/gtk/infobars/... chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.h:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. 2011 -> 2012 http://codereview.chromium.org/9570012/diff/1/chrome/browser/ui/gtk/infobars/... chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.h:10: #include "content/common/media/media_stream_options.h" Includes should be in alphabetical order (i.e. this one needs to be moved down after the "chrome" includes). http://codereview.chromium.org/9570012/diff/1/chrome/browser/ui/media_stream_... File chrome/browser/ui/media_stream_infobar_delegate.cc (right): http://codereview.chromium.org/9570012/diff/1/chrome/browser/ui/media_stream_... chrome/browser/ui/media_stream_infobar_delegate.cc:10: #include "chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.h" Please don't include GTK headers here (see comments below). http://codereview.chromium.org/9570012/diff/1/chrome/browser/ui/media_stream_... chrome/browser/ui/media_stream_infobar_delegate.cc:114: #if !defined(TOOLKIT_VIEWS) Please change this to: #if !defined(TOOLKIT_VIEWS) && !defined(OS_LINUX) and edit the #endif comment accordingly. Then, implement the CreateInfoBar method in your platform-specific file (which would be media_stream_infobar_gtk.cc in this case). You can see how I've done it for the Views implementation here: http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/views/infob... http://codereview.chromium.org/9570012/diff/1/chrome/browser/ui/media_stream_... chrome/browser/ui/media_stream_infobar_delegate.cc:119: #if defined(OS_LINUX) Please remove this chunk of code (see my comment above).
Hi, I was put as reviewer. I'm not familiar with the browser code or gtk at all, so I'm not able to review this CL. /Henrik
On 2012/03/01 14:12:09, Henrik Grunell wrote: > Hi, I was put as reviewer. I'm not familiar with the browser code or gtk at all, > so I'm not able to review this CL. > > /Henrik Ok, I will find another reviewer from OWNERS files. Thanks -Jeremy
On 2012/03/01 14:06:22, macourteau wrote: > Hi Jeremy, > > Thanks for the patch and the quick turnaround! However, I'm not familiar with > GTK code and I'm certainly not the right person to review that code. > > I've suggested a few changes, but I encourage you to get others to review it as > well. You will also need approval from an OWNER to be able to submit the code. > > Regards, > Marc-Antoine > > http://codereview.chromium.org/9570012/diff/1/chrome/browser/ui/gtk/infobars/... > File chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.cc (right): > > http://codereview.chromium.org/9570012/diff/1/chrome/browser/ui/gtk/infobars/... > chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.cc:1: // Copyright (c) > 2011 The Chromium Authors. All rights reserved. > 2011 -> 2012 fixed > > http://codereview.chromium.org/9570012/diff/1/chrome/browser/ui/gtk/infobars/... > chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.cc:11: #include > "chrome/browser/ui/gtk/gtk_util.h" > Includes should be in alphabetical order. fixed > > http://codereview.chromium.org/9570012/diff/1/chrome/browser/ui/gtk/infobars/... > File chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.h (right): > > http://codereview.chromium.org/9570012/diff/1/chrome/browser/ui/gtk/infobars/... > chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.h:1: // Copyright (c) > 2011 The Chromium Authors. All rights reserved. > 2011 -> 2012 fixed > > http://codereview.chromium.org/9570012/diff/1/chrome/browser/ui/gtk/infobars/... > chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.h:10: #include > "content/common/media/media_stream_options.h" > Includes should be in alphabetical order (i.e. this one needs to be moved down > after the "chrome" includes). fixed > > http://codereview.chromium.org/9570012/diff/1/chrome/browser/ui/media_stream_... > File chrome/browser/ui/media_stream_infobar_delegate.cc (right): > > http://codereview.chromium.org/9570012/diff/1/chrome/browser/ui/media_stream_... > chrome/browser/ui/media_stream_infobar_delegate.cc:10: #include > "chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.h" > Please don't include GTK headers here (see comments below). fixed > > http://codereview.chromium.org/9570012/diff/1/chrome/browser/ui/media_stream_... > chrome/browser/ui/media_stream_infobar_delegate.cc:114: #if > !defined(TOOLKIT_VIEWS) > Please change this to: > #if !defined(TOOLKIT_VIEWS) && !defined(OS_LINUX) > and edit the #endif comment accordingly. > > Then, implement the CreateInfoBar method in your platform-specific file (which > would be media_stream_infobar_gtk.cc in this case). You can see how I've done it > for the Views implementation here: > http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/views/infob... > > http://codereview.chromium.org/9570012/diff/1/chrome/browser/ui/media_stream_... > chrome/browser/ui/media_stream_infobar_delegate.cc:119: #if defined(OS_LINUX) > Please remove this chunk of code (see my comment above). fixed
A couple more comments. I see you've added OWNERS from the gtk/ subdirectory to the review. I think you'll also need an OWNER from the ui/ subdirectory for the change to the media_stream_infobar_delegate.cc file. http://codereview.chromium.org/9570012/diff/7002/chrome/browser/chrome_conten... File chrome/browser/chrome_content_browser_client.cc (right): http://codereview.chromium.org/9570012/diff/7002/chrome/browser/chrome_conten... chrome/browser/chrome_content_browser_client.cc:1122: #endif // TOOLKIT_VIEWS Please also update this comment (i.e. "TOOLKIT_VIEWS || OS_LINUX"). http://codereview.chromium.org/9570012/diff/7002/chrome/browser/ui/media_stre... File chrome/browser/ui/media_stream_infobar_delegate.cc (right): http://codereview.chromium.org/9570012/diff/7002/chrome/browser/ui/media_stre... chrome/browser/ui/media_stream_infobar_delegate.cc:114: #if !defined(OS_LINUX) Please merge this with the above line: #if !defined(TOOLKIT_VIEWS) && !defined(OS_LINUX) http://codereview.chromium.org/9570012/diff/7002/chrome/browser/ui/media_stre... chrome/browser/ui/media_stream_infobar_delegate.cc:121: #endif // !OS_LINUX Please merge with the line below: #endif // !TOOLKIT_VIEWS && !OS_LINUX
http://codereview.chromium.org/9570012/diff/7002/chrome/browser/ui/gtk/infoba... File chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.cc (right): http://codereview.chromium.org/9570012/diff/7002/chrome/browser/ui/gtk/infoba... chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.cc:41: if (!delegate_->has_audio()) { no curlies. http://codereview.chromium.org/9570012/diff/7002/chrome/browser/ui/gtk/infoba... chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.cc:65: // The devices button . at end of comment. http://codereview.chromium.org/9570012/diff/7002/chrome/browser/ui/gtk/infoba... chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.cc:68: G_CALLBACK(&OnDevicesClickedThunk), this); indent. http://codereview.chromium.org/9570012/diff/7002/chrome/browser/ui/gtk/infoba... chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.cc:86: GtkWidget* arrow = gtk_arrow_new(GTK_ARROW_DOWN, GTK_SHADOW_NONE); I applaud the GTK integration here, but shouldn't we be using the same image resource that chrome uses on every other platform when we're not in GTK theme mode?
On 2012/03/01 15:32:29, macourteau wrote: > A couple more comments. > > I see you've added OWNERS from the gtk/ subdirectory to the review. I think > you'll also need an OWNER from the ui/ subdirectory for the change to the > media_stream_infobar_delegate.cc file. > > http://codereview.chromium.org/9570012/diff/7002/chrome/browser/chrome_conten... > File chrome/browser/chrome_content_browser_client.cc (right): > > http://codereview.chromium.org/9570012/diff/7002/chrome/browser/chrome_conten... > chrome/browser/chrome_content_browser_client.cc:1122: #endif // TOOLKIT_VIEWS > Please also update this comment (i.e. "TOOLKIT_VIEWS || OS_LINUX"). Fixed > > http://codereview.chromium.org/9570012/diff/7002/chrome/browser/ui/media_stre... > File chrome/browser/ui/media_stream_infobar_delegate.cc (right): > > http://codereview.chromium.org/9570012/diff/7002/chrome/browser/ui/media_stre... > chrome/browser/ui/media_stream_infobar_delegate.cc:114: #if !defined(OS_LINUX) > Please merge this with the above line: > #if !defined(TOOLKIT_VIEWS) && !defined(OS_LINUX) Fixed > > http://codereview.chromium.org/9570012/diff/7002/chrome/browser/ui/media_stre... > chrome/browser/ui/media_stream_infobar_delegate.cc:121: #endif // !OS_LINUX > Please merge with the line below: > #endif // !TOOLKIT_VIEWS && !OS_LINUX Fixed
On 2012/03/01 17:53:46, Evan Stade wrote: > http://codereview.chromium.org/9570012/diff/7002/chrome/browser/ui/gtk/infoba... > File chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.cc (right): > > http://codereview.chromium.org/9570012/diff/7002/chrome/browser/ui/gtk/infoba... > chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.cc:41: if > (!delegate_->has_audio()) { > no curlies. Fixed > > http://codereview.chromium.org/9570012/diff/7002/chrome/browser/ui/gtk/infoba... > chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.cc:65: // The devices > button > . at end of comment. Fixed > > http://codereview.chromium.org/9570012/diff/7002/chrome/browser/ui/gtk/infoba... > chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.cc:68: > G_CALLBACK(&OnDevicesClickedThunk), this); > indent. Fixed > > http://codereview.chromium.org/9570012/diff/7002/chrome/browser/ui/gtk/infoba... > chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.cc:86: GtkWidget* arrow > = gtk_arrow_new(GTK_ARROW_DOWN, GTK_SHADOW_NONE); > I applaud the GTK integration here, but shouldn't we be using the same image > resource that chrome uses on every other platform when we're not in GTK theme > mode? I am not catching the key point of your question here, can you give me more details.
please respond by using comments. It is easier to track the separate threads that way. I was suggesting you use IDR_INFOBARBUTTON_MENU_DROPARROW, but upon further investigation I see we don't use that ever for Linux (e.g. the translate infobar). However, you are copying a lot of code for creating the menu button. Could you instead create a shared function akin to InfoBarView::CreateMenuButton() from views/infobars/infobar_view.cc? On 2012/03/02 01:49:55, Jeremy Mao wrote: > On 2012/03/01 17:53:46, Evan Stade wrote: > > > http://codereview.chromium.org/9570012/diff/7002/chrome/browser/ui/gtk/infoba... > > File chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.cc (right): > > > > > http://codereview.chromium.org/9570012/diff/7002/chrome/browser/ui/gtk/infoba... > > chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.cc:41: if > > (!delegate_->has_audio()) { > > no curlies. > Fixed > > > > > http://codereview.chromium.org/9570012/diff/7002/chrome/browser/ui/gtk/infoba... > > chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.cc:65: // The devices > > button > > . at end of comment. > Fixed > > > > > http://codereview.chromium.org/9570012/diff/7002/chrome/browser/ui/gtk/infoba... > > chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.cc:68: > > G_CALLBACK(&OnDevicesClickedThunk), this); > > indent. > Fixed > > > > > http://codereview.chromium.org/9570012/diff/7002/chrome/browser/ui/gtk/infoba... > > chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.cc:86: GtkWidget* > arrow > > = gtk_arrow_new(GTK_ARROW_DOWN, GTK_SHADOW_NONE); > > I applaud the GTK integration here, but shouldn't we be using the same image > > resource that chrome uses on every other platform when we're not in GTK theme > > mode? > I am not catching the key point of your question here, can you give me more > details.
Hi Evan, Thanks for your explanation, I think it's a good idea to refactor such kind of code, but implement a shared function means I should also modify code of other parts(e.g. the translate infobar) which is not good for this CL. I suggest we can firstly push these code in, and I will upload another patch to fix the bug you mentioned. Thanks -Jeremy On 2012/03/02 21:21:44, Evan Stade wrote: > please respond by using comments. It is easier to track the separate threads > that way. > > I was suggesting you use IDR_INFOBARBUTTON_MENU_DROPARROW, but upon further > investigation I see we don't use that ever for Linux (e.g. the translate > infobar). However, you are copying a lot of code for creating the menu button. > Could you instead create a shared function akin to > InfoBarView::CreateMenuButton() from views/infobars/infobar_view.cc? > > On 2012/03/02 01:49:55, Jeremy Mao wrote: > > On 2012/03/01 17:53:46, Evan Stade wrote: > > > > > > http://codereview.chromium.org/9570012/diff/7002/chrome/browser/ui/gtk/infoba... > > > File chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.cc (right): > > > > > > > > > http://codereview.chromium.org/9570012/diff/7002/chrome/browser/ui/gtk/infoba... > > > chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.cc:41: if > > > (!delegate_->has_audio()) { > > > no curlies. > > Fixed > > > > > > > > > http://codereview.chromium.org/9570012/diff/7002/chrome/browser/ui/gtk/infoba... > > > chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.cc:65: // The > devices > > > button > > > . at end of comment. > > Fixed > > > > > > > > > http://codereview.chromium.org/9570012/diff/7002/chrome/browser/ui/gtk/infoba... > > > chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.cc:68: > > > G_CALLBACK(&OnDevicesClickedThunk), this); > > > indent. > > Fixed > > > > > > > > > http://codereview.chromium.org/9570012/diff/7002/chrome/browser/ui/gtk/infoba... > > > chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.cc:86: GtkWidget* > > arrow > > > = gtk_arrow_new(GTK_ARROW_DOWN, GTK_SHADOW_NONE); > > > I applaud the GTK integration here, but shouldn't we be using the same image > > > resource that chrome uses on every other platform when we're not in GTK > theme > > > mode? > > I am not catching the key point of your question here, can you give me more > > details.
On 2012/03/03 03:46:22, Jeremy Mao wrote: > Hi Evan, > Thanks for your explanation, I think it's a good idea to refactor such kind of > code, but implement a shared function means I should also modify code of other > parts(e.g. the translate infobar) which is not good for this CL. I suggest we > can firstly push these code in, and I will upload another patch to fix the bug > you mentioned. If you don't want to combine the two that's fine, but in that case how about doing the refactoring first, then the new infobar.
On 2012/03/03 06:34:08, Peter Kasting wrote: > If you don't want to combine the two that's fine, but in that case how about > doing the refactoring first, then the new infobar. Ok, I will do this ASAP.
Hi all, could you please take a look at this again when you are free? The latest change is based on http://codereview.chromium.org/9602007/
http://codereview.chromium.org/9570012/diff/13001/chrome/browser/ui/gtk/infob... File chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.h (right): http://codereview.chromium.org/9570012/diff/13001/chrome/browser/ui/gtk/infob... chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.h:10: #include "chrome/browser/media/media_stream_devices_menu_model.h" nit: please, forward declare this instead. http://codereview.chromium.org/9570012/diff/13001/chrome/browser/ui/gtk/infob... chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.h:12: #include "content/common/media/media_stream_options.h" nit: unused, please remove.
lgtm
On 2012/03/06 14:24:06, tfarina wrote: > http://codereview.chromium.org/9570012/diff/13001/chrome/browser/ui/gtk/infob... > File chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.h (right): > > http://codereview.chromium.org/9570012/diff/13001/chrome/browser/ui/gtk/infob... > chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.h:10: #include > "chrome/browser/media/media_stream_devices_menu_model.h" > nit: please, forward declare this instead. Fixed > > http://codereview.chromium.org/9570012/diff/13001/chrome/browser/ui/gtk/infob... > chrome/browser/ui/gtk/infobars/media_stream_infobar_gtk.h:12: #include > "content/common/media/media_stream_options.h" > nit: unused, please remove. Fixed
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yujie.mao@intel.com/9570012/17003
Presubmit check for 9570012-17003 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Missing LGTM from an OWNER for: chrome/browser/ui/media_stream_infobar_delegate.cc Presubmit checks took 1.3s to calculate.
LGTM w/question http://codereview.chromium.org/9570012/diff/17003/chrome/browser/ui/media_str... File chrome/browser/ui/media_stream_infobar_delegate.cc (right): http://codereview.chromium.org/9570012/diff/17003/chrome/browser/ui/media_str... chrome/browser/ui/media_stream_infobar_delegate.cc:113: #if !defined(TOOLKIT_VIEWS) && !defined(OS_LINUX) Maybe this should become #if defined(OS_MAC)?
On 2012/03/08 21:00:11, Peter Kasting wrote: > LGTM w/question > > http://codereview.chromium.org/9570012/diff/17003/chrome/browser/ui/media_str... > File chrome/browser/ui/media_stream_infobar_delegate.cc (right): > > http://codereview.chromium.org/9570012/diff/17003/chrome/browser/ui/media_str... > chrome/browser/ui/media_stream_infobar_delegate.cc:113: #if > !defined(TOOLKIT_VIEWS) && !defined(OS_LINUX) > Maybe this should become #if defined(OS_MAC)? a better and more clear way, fixed.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yujie.mao@intel.com/9570012/24001
Change committed as 125780
http://codereview.chromium.org/9570012/diff/24001/chrome/browser/ui/media_str... File chrome/browser/ui/media_stream_infobar_delegate.cc (right): http://codereview.chromium.org/9570012/diff/24001/chrome/browser/ui/media_str... chrome/browser/ui/media_stream_infobar_delegate.cc:113: #if defined(OS_MAC) Hi, it's OS_MACOSX, not OS_MAC. As is, this code is never active and mostly just works by accident (due to ld not loading object files that aren't referenced). I'll send a CL to fix this. Nico
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
