Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12035046:
Fix bug causing tab favicon media indicator to not turn off. (Closed)
|
Created:
7 years, 11 months ago by miu Modified:
7 years, 10 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix bug causing tab favicon media indicator to not turn off.
Root cause: MediaStreamCaptureIndicator::CaptureDevicesClosed() is being called with the original render_process_id/render_view_id, rather than the current ones associated with a WebContents instance. This is because media capture was launched by an extension, and MediaStreamManager only knows what the ID pair was at the time the media capture was started.
Changes include:
1. Tore-out the existing "find a tab" code.
2. Added an alias mapping with a LookUp function that finds a WebContents instance by current or any previously-known ID pair.
3. Fixed possible de-refs of WebContents pointers after the instance is destroyed.
4. Clean-ups of comments/naming, and minor code reorganization.
BUG=171077
TEST=Confirmed, by manually running a debug-build browser (on Win7 and Linux), that existing functionality is not broken and that the bug is fixed.
TBR=ben
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179490
Patch Set 1 #
Total comments: 13
Patch Set 2 : Better naming for methods called from tab_utils. #Patch Set 3 : Remove a DCHECK: During browser shutdown, CaptureDevicesClosed() may not have been called and that'… #
Total comments: 14
Patch Set 4 : Fix issue with de-ref'ing WebContents after destruction. #
Total comments: 16
Patch Set 5 : Tweaks. #
Total comments: 2
Patch Set 6 : #Patch Set 7 : Fix issue with browser shutdown race condition. #
Total comments: 2
Messages
Total messages: 18 (0 generated)
Hi all, Please take a look. Thanks, Yuri
Added ben@ for OWNERS approval.
Hmm.. nice cleanup/refactoring, but I think the bug could be fixed with roughly a ~2 line change (just tested that this works, didn't think too deeply about that change): https://codereview.chromium.org/12036057/ A lot of this already does the same thing as your alias lookup. https://codereview.chromium.org/12035046/diff/1/chrome/browser/media/media_st... File chrome/browser/media/media_stream_capture_indicator.cc (left): https://codereview.chromium.org/12035046/diff/1/chrome/browser/media/media_st... chrome/browser/media/media_stream_capture_indicator.cc:159: DCHECK(command_id >= IDC_MEDIA_CONTEXT_MEDIA_STREAM_CAPTURE_LIST_FIRST && nit: This previous DCHECK seems more clear to me? https://codereview.chromium.org/12035046/diff/1/chrome/browser/media/media_st... chrome/browser/media/media_stream_capture_indicator.cc:205: CreateStatusTray(); Did you mean to remove this? https://codereview.chromium.org/12035046/diff/1/chrome/browser/media/media_st... File chrome/browser/media/media_stream_capture_indicator.cc (right): https://codereview.chromium.org/12035046/diff/1/chrome/browser/media/media_st... chrome/browser/media/media_stream_capture_indicator.cc:419: if (it != aliases_.end()) DCHECK this? Seems like all lookups should resolve. https://codereview.chromium.org/12035046/diff/1/chrome/browser/media/media_st... chrome/browser/media/media_stream_capture_indicator.cc:475: int num_audio, num_video, num_mirroring; nit: I think the style guide specifies that they each need to be on a separate line. Also, maybe you need to initialize so that valgrind doesn't complain? https://codereview.chromium.org/12035046/diff/1/chrome/browser/ui/tabs/tab_ut... File chrome/browser/ui/tabs/tab_utils.cc (right): https://codereview.chromium.org/12035046/diff/1/chrome/browser/ui/tabs/tab_ut... chrome/browser/ui/tabs/tab_utils.cc:20: return capture_indicator->IsRenderViewMirroring(render_process_id, Hmm, not sure about this naming change. Isn't it actually whether the render process is capturing that we care about?
Yep, the 2-line change also fixes the bug. ;-) The weirdness in all this is that for tab mirroring, one media stream follows render view swaps, rather than being stopped and restarted for each swap. This is meant to provide an uninterrupted mirroring experience for the user. IMHO, the clean-up work here makes it much more clear how our design choices elsewhere impact the UI indicator toggling in MediaStreamCaptureIndicator. Repsonses and adjustments made, per your comments. PTAL. Thanks, Yuri https://codereview.chromium.org/12035046/diff/1/chrome/browser/media/media_st... File chrome/browser/media/media_stream_capture_indicator.cc (left): https://codereview.chromium.org/12035046/diff/1/chrome/browser/media/media_st... chrome/browser/media/media_stream_capture_indicator.cc:159: DCHECK(command_id >= IDC_MEDIA_CONTEXT_MEDIA_STREAM_CAPTURE_LIST_FIRST && On 2013/01/23 19:30:43, justinlin wrote: > nit: This previous DCHECK seems more clear to me? They're different. The old DCHECK just checks whether command_id is within the range based on the IDC constants. However, the new checks make sure command_id is within the more-narrow range actually supported by command_map_. https://codereview.chromium.org/12035046/diff/1/chrome/browser/media/media_st... chrome/browser/media/media_stream_capture_indicator.cc:205: CreateStatusTray(); On 2013/01/23 19:30:43, justinlin wrote: > Did you mean to remove this? Yes. Two things happening here: 1. I renamed the method to MaybeCreateStatusTrayIcon(). 2. I moved the call to the place where it is first clear that the status tray icon should be created; at the end of UpdateStatusTrayIconContextMenu(). https://codereview.chromium.org/12035046/diff/1/chrome/browser/media/media_st... File chrome/browser/media/media_stream_capture_indicator.cc (right): https://codereview.chromium.org/12035046/diff/1/chrome/browser/media/media_st... chrome/browser/media/media_stream_capture_indicator.cc:419: if (it != aliases_.end()) On 2013/01/23 19:30:43, justinlin wrote: > DCHECK this? Seems like all lookups should resolve. Not necessarily. Example: When this method is called by IsRenderViewCapturing() or IsRenderViewMirroring(). In other words, outside objects are querying whether a tab is capturing/mirroring, and if it's not, the look-up will fail. https://codereview.chromium.org/12035046/diff/1/chrome/browser/media/media_st... chrome/browser/media/media_stream_capture_indicator.cc:475: int num_audio, num_video, num_mirroring; On 2013/01/23 19:30:43, justinlin wrote: > nit: I think the style guide specifies that they each need to be on a separate > line. Also, maybe you need to initialize so that valgrind doesn't complain? Done. https://codereview.chromium.org/12035046/diff/1/chrome/browser/ui/tabs/tab_ut... File chrome/browser/ui/tabs/tab_utils.cc (right): https://codereview.chromium.org/12035046/diff/1/chrome/browser/ui/tabs/tab_ut... chrome/browser/ui/tabs/tab_utils.cc:20: return capture_indicator->IsRenderViewMirroring(render_process_id, On 2013/01/23 19:30:43, justinlin wrote: > Hmm, not sure about this naming change. Isn't it actually whether the render > process is capturing that we care about? Done. I thought a bit more about the naming here. Originally, these two methods were named almost the same way; but the problem I had is that they indicate two different things. The two methods can be described as: 1. Returns true if the render view is capturing user media (e.g., webcam). 2. Returns true if the render view itself is being mirrored. In case #1, the render view is a *consumer* of some external media source. So, I went with naming this method IsCapturingUserMedia(rp_id, rv_id). In case #2, the render view is itself a *source* of media (A/V is being generated from it). So, the method name should help communicate that flow of media is in the opposite direction. I renamed the method to IsSupplyingMirroredMedia(rp_id, rv_id). Do these names sound good to you? BTW--Also did a little tweak here to make things fit nicer within 80-char lines. ;-)
Cool, yea overall this looks fine to me, but I'm not too familiar with a lot of the other functionality in this file (like ExecuteCommand), so Shijing would be a better reviewer for most of this stuff. Do you think it would make sense to just submit the ~2-ish line change separately first for the fix to the original bug, then we can land this cleanup separately (happy to do it too if you'd like). Also, 1 general problem that both approaches have is I'm not sure if the web_contents pointer can become invalid? I *think* it can't assuming we always receive a removed() event before the web_contents is destroyed, but might be worth thinking about that. On 2013/01/23 20:33:28, Yuri wrote: > Yep, the 2-line change also fixes the bug. ;-) > > The weirdness in all this is that for tab mirroring, one media stream follows > render view swaps, rather than being stopped and restarted for each swap. This > is meant to provide an uninterrupted mirroring experience for the user. IMHO, > the clean-up work here makes it much more clear how our design choices elsewhere > impact the UI indicator toggling in MediaStreamCaptureIndicator. > > Repsonses and adjustments made, per your comments. PTAL. > > Thanks, > Yuri > > https://codereview.chromium.org/12035046/diff/1/chrome/browser/media/media_st... > File chrome/browser/media/media_stream_capture_indicator.cc (left): > > https://codereview.chromium.org/12035046/diff/1/chrome/browser/media/media_st... > chrome/browser/media/media_stream_capture_indicator.cc:159: DCHECK(command_id >= > IDC_MEDIA_CONTEXT_MEDIA_STREAM_CAPTURE_LIST_FIRST && > On 2013/01/23 19:30:43, justinlin wrote: > > nit: This previous DCHECK seems more clear to me? > > They're different. The old DCHECK just checks whether command_id is within the > range based on the IDC constants. However, the new checks make sure command_id > is within the more-narrow range actually supported by command_map_. > > https://codereview.chromium.org/12035046/diff/1/chrome/browser/media/media_st... > chrome/browser/media/media_stream_capture_indicator.cc:205: CreateStatusTray(); > On 2013/01/23 19:30:43, justinlin wrote: > > Did you mean to remove this? > > Yes. Two things happening here: > > 1. I renamed the method to MaybeCreateStatusTrayIcon(). > 2. I moved the call to the place where it is first clear that the status tray > icon should be created; at the end of UpdateStatusTrayIconContextMenu(). > > https://codereview.chromium.org/12035046/diff/1/chrome/browser/media/media_st... > File chrome/browser/media/media_stream_capture_indicator.cc (right): > > https://codereview.chromium.org/12035046/diff/1/chrome/browser/media/media_st... > chrome/browser/media/media_stream_capture_indicator.cc:419: if (it != > aliases_.end()) > On 2013/01/23 19:30:43, justinlin wrote: > > DCHECK this? Seems like all lookups should resolve. > > Not necessarily. Example: When this method is called by IsRenderViewCapturing() > or IsRenderViewMirroring(). In other words, outside objects are querying > whether a tab is capturing/mirroring, and if it's not, the look-up will fail. > > https://codereview.chromium.org/12035046/diff/1/chrome/browser/media/media_st... > chrome/browser/media/media_stream_capture_indicator.cc:475: int num_audio, > num_video, num_mirroring; > On 2013/01/23 19:30:43, justinlin wrote: > > nit: I think the style guide specifies that they each need to be on a separate > > line. Also, maybe you need to initialize so that valgrind doesn't complain? > > Done. > > https://codereview.chromium.org/12035046/diff/1/chrome/browser/ui/tabs/tab_ut... > File chrome/browser/ui/tabs/tab_utils.cc (right): > > https://codereview.chromium.org/12035046/diff/1/chrome/browser/ui/tabs/tab_ut... > chrome/browser/ui/tabs/tab_utils.cc:20: return > capture_indicator->IsRenderViewMirroring(render_process_id, > On 2013/01/23 19:30:43, justinlin wrote: > > Hmm, not sure about this naming change. Isn't it actually whether the render > > process is capturing that we care about? > > Done. I thought a bit more about the naming here. Originally, these two > methods were named almost the same way; but the problem I had is that they > indicate two different things. The two methods can be described as: > > 1. Returns true if the render view is capturing user media (e.g., webcam). > 2. Returns true if the render view itself is being mirrored. > > In case #1, the render view is a *consumer* of some external media source. So, > I went with naming this method IsCapturingUserMedia(rp_id, rv_id). > > In case #2, the render view is itself a *source* of media (A/V is being > generated from it). So, the method name should help communicate that flow of > media is in the opposite direction. I renamed the method to > IsSupplyingMirroredMedia(rp_id, rv_id). > > Do these names sound good to you? > > BTW--Also did a little tweak here to make things fit nicer within 80-char lines. > ;-)
On 2013/01/23 21:34:20, justinlin wrote: > Do you think it would make sense to just submit the ~2-ish line change > separately first for the fix to the original bug, then we can land this cleanup > separately (happy to do it too if you'd like). There's no reason to rush this. This change should get in well before the M26 cutoff, and it's not halting our progress on other features (that I'm aware of). This is just a simple usability bug. > Also, 1 general problem that both approaches have is I'm not sure if the > web_contents pointer can become invalid? I *think* it can't assuming we always > receive a removed() event before the web_contents is destroyed, but might be > worth thinking about that. I looked into that, and you're correct: The "removed" event will always occur before web_contents is destroyed, so it is always valid to deref the pointer between "add" and "remove."
Oh, I didn't mean to rush it in. My concern was mostly just to separate the bug fix from the refactoring/cleanup (since this seems like a moderately sized cleanup), which is why I proposed landing 2 cl's. https://codereview.chromium.org/12035046/diff/1/chrome/browser/media/media_st... File chrome/browser/media/media_stream_capture_indicator.cc (left): https://codereview.chromium.org/12035046/diff/1/chrome/browser/media/media_st... chrome/browser/media/media_stream_capture_indicator.cc:205: CreateStatusTray(); On 2013/01/23 20:33:28, Yuri wrote: > On 2013/01/23 19:30:43, justinlin wrote: > > Did you mean to remove this? > > Yes. Two things happening here: > > 1. I renamed the method to MaybeCreateStatusTrayIcon(). > 2. I moved the call to the place where it is first clear that the status tray > icon should be created; at the end of UpdateStatusTrayIconContextMenu(). OK. I'm probably nitpicking too much, but could webcam/mic be requested from an extension background page, which might not have a web contents? It would short circuit in AddCaptureDevices and not show the tray icon in that case? https://codereview.chromium.org/12035046/diff/8001/chrome/browser/media/media... File chrome/browser/media/media_stream_capture_indicator.cc (right): https://codereview.chromium.org/12035046/diff/8001/chrome/browser/media/media... chrome/browser/media/media_stream_capture_indicator.cc:434: CountDevices(devices, &num_audio, &num_video, &num_mirroring); Could the CountDevices function just be moved into CaptureDeviceUsage::TallyUsage? We can expose something like a "ShouldShowDesktopNotifications" method there too. https://codereview.chromium.org/12035046/diff/8001/chrome/browser/ui/tabs/tab... File chrome/browser/ui/tabs/tab_utils.cc (right): https://codereview.chromium.org/12035046/diff/8001/chrome/browser/ui/tabs/tab... chrome/browser/ui/tabs/tab_utils.cc:32: indicator->IsSupplyingMirroredMedia(render_process_id, render_view_id); nit: I think I kind of prefer just something "IsMirroringTab" , but I don't feel strongly about it :). The "supplying" word in the function name is what sounds weird to me.
https://codereview.chromium.org/12035046/diff/8001/chrome/browser/media/media... File chrome/browser/media/media_stream_capture_indicator.cc (left): https://codereview.chromium.org/12035046/diff/8001/chrome/browser/media/media... chrome/browser/media/media_stream_capture_indicator.cc:137: DCHECK(tabs_.empty()); please add back DCHECKs for aliases_ and usage_map. If they are not empty, it might mean the UI is leaking. We prefer having the browser crash rather than showing a wrong UI. https://codereview.chromium.org/12035046/diff/8001/chrome/browser/media/media... File chrome/browser/media/media_stream_capture_indicator.cc (right): https://codereview.chromium.org/12035046/diff/8001/chrome/browser/media/media... chrome/browser/media/media_stream_capture_indicator.cc:186: const int index = nit, you don't need const for int. https://codereview.chromium.org/12035046/diff/8001/chrome/browser/media/media... chrome/browser/media/media_stream_capture_indicator.cc:434: CountDevices(devices, &num_audio, &num_video, &num_mirroring); On 2013/01/24 07:04:44, justinlin wrote: > Could the CountDevices function just be moved into > CaptureDeviceUsage::TallyUsage? We can expose something like a > "ShouldShowDesktopNotifications" method there too. +1 https://codereview.chromium.org/12035046/diff/8001/chrome/browser/media/media... chrome/browser/media/media_stream_capture_indicator.cc:494: web_contents->NotifyNavigationStateChanged(content::INVALIDATE_TYPE_TAB); what if the page has gone and the web_contents is from aliases_, will it be a problem for using the cached web_contents here? Besides, please do a test to verify the UI works in the following case: Open two tabs with apprtc.appspot.com and https://webrtc-demos.appspot.com/html/pc1.html; allow the access; open the third tab, right click and close other tabs.
https://codereview.chromium.org/12035046/diff/8001/chrome/browser/media/media... File chrome/browser/media/media_stream_capture_indicator.cc (right): https://codereview.chromium.org/12035046/diff/8001/chrome/browser/media/media... chrome/browser/media/media_stream_capture_indicator.cc:494: web_contents->NotifyNavigationStateChanged(content::INVALIDATE_TYPE_TAB); Note, you should do the test on Mac or windows 7, since the system tray is not available on the gprecise and windows 8.
Justin/Shijing, Addressed comments and fixed de-ref'ing of WebContents after destruction. PTAL. Thanks, Yuri https://codereview.chromium.org/12035046/diff/1/chrome/browser/media/media_st... File chrome/browser/media/media_stream_capture_indicator.cc (left): https://codereview.chromium.org/12035046/diff/1/chrome/browser/media/media_st... chrome/browser/media/media_stream_capture_indicator.cc:205: CreateStatusTray(); On 2013/01/24 07:04:44, justinlin wrote: > OK. I'm probably nitpicking too much, but could webcam/mic be requested from an > extension background page, which might not have a web contents? It would short > circuit in AddCaptureDevices and not show the tray icon in that case? I'm not sure. I'll have to try this out to see whether Chrome is producing the desired behavior (infobar for user permissions, etc.). In any case, without a WebContents instance, the AddCaptureDevices() would result in a no-op (i.e., no system tray icon *and* no UI favicon animation). https://codereview.chromium.org/12035046/diff/8001/chrome/browser/media/media... File chrome/browser/media/media_stream_capture_indicator.cc (left): https://codereview.chromium.org/12035046/diff/8001/chrome/browser/media/media... chrome/browser/media/media_stream_capture_indicator.cc:137: DCHECK(tabs_.empty()); On 2013/01/24 14:09:06, xians1 wrote: > please add back DCHECKs for aliases_ and usage_map. If they are not empty, it > might mean the UI is leaking. We prefer having the browser crash rather than > showing a wrong UI. Done. Originally, I thought this was an issue only upon shutdown (where MediaStreamCaptureIndicator is unhooked and destroyed before all "close" events are sent to it). Seems to be all fixed now. :) https://codereview.chromium.org/12035046/diff/8001/chrome/browser/media/media... File chrome/browser/media/media_stream_capture_indicator.cc (right): https://codereview.chromium.org/12035046/diff/8001/chrome/browser/media/media... chrome/browser/media/media_stream_capture_indicator.cc:186: const int index = On 2013/01/24 14:09:06, xians1 wrote: > nit, you don't need const for int. Ack. IMHO, it's a good habit to use const wherever possible to catch bugs. This is one of those things the style guide says to do, but many people forget (see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Use_of_const). Also, if you have a few minutes, here's a very funny (and educational) video clip: http://gamesfromwithin.com/the-const-nazi Granted, it's use here is a bit pedantic, but why take it out? https://codereview.chromium.org/12035046/diff/8001/chrome/browser/media/media... chrome/browser/media/media_stream_capture_indicator.cc:434: CountDevices(devices, &num_audio, &num_video, &num_mirroring); On 2013/01/24 14:09:06, xians1 wrote: > On 2013/01/24 07:04:44, justinlin wrote: > > Could the CountDevices function just be moved into > > CaptureDeviceUsage::TallyUsage? We can expose something like a > > "ShouldShowDesktopNotifications" method there too. > > +1 Done. Good call! :) BTW--It seemed cleanest to have TallyUsage() just return the appropriate balloon body message ID when the balloon is to be shown. https://codereview.chromium.org/12035046/diff/8001/chrome/browser/media/media... chrome/browser/media/media_stream_capture_indicator.cc:494: web_contents->NotifyNavigationStateChanged(content::INVALIDATE_TYPE_TAB); On 2013/01/24 14:09:06, xians1 wrote: > what if the page has gone and the web_contents is from aliases_, will it be a > problem for using the cached web_contents here? With all my testing, I never saw this happen. However, after using xians's test strategy (which involves using WebRTC functionality), it turns out that a WebContents *can* be destroyed before the "close" method is called. I'm guessing there's some extra thread trampolining causing the out-of-order calls to MediaStreamCaptureIndicator. This was easy to cleanly fix, however. I just use WebContentsObserver to observe the destroy event, and make sure I only deref the pointer if destruction has not yet occurred. > Besides, please do a test to verify the UI works in the following case: > Open two tabs with http://apprtc.appspot.com and > https://webrtc-demos.appspot.com/html/pc1.html; allow the access; open the third > tab, right click and close other tabs. Good idea! This caught the WebContents destroy-before-use bug here in MediaStreamCaptureIndicator. I fixed the problem. Confirmed many tab open/close, start/stop, WebRTC/PPAPI/etc., combinations all do the right thing. https://codereview.chromium.org/12035046/diff/8001/chrome/browser/media/media... chrome/browser/media/media_stream_capture_indicator.cc:494: web_contents->NotifyNavigationStateChanged(content::INVALIDATE_TYPE_TAB); On 2013/01/24 14:45:55, xians1 wrote: > Note, you should do the test on Mac or windows 7, since the system tray is not > available on the gprecise and windows 8. Actually, gprecise has the system tray and it was working for me. I also plan on testing all this thoroughly on my Win7 desktop before I commit. https://codereview.chromium.org/12035046/diff/8001/chrome/browser/ui/tabs/tab... File chrome/browser/ui/tabs/tab_utils.cc (right): https://codereview.chromium.org/12035046/diff/8001/chrome/browser/ui/tabs/tab... chrome/browser/ui/tabs/tab_utils.cc:32: indicator->IsSupplyingMirroredMedia(render_process_id, render_view_id); On 2013/01/24 07:04:44, justinlin wrote: > nit: I think I kind of prefer just something "IsMirroringTab" , but I don't feel > strongly about it :). The "supplying" word in the function name is what sounds > weird to me. Okay. How about "IsBeingMirrored?" I want to leave the word "tab" out since the IDs are referencing a RenderView/WebContents.
mostly nits, thanks for the fix and cleanup! lgtm for the tab capture parts and general code org/style https://codereview.chromium.org/12035046/diff/1/chrome/browser/media/media_st... File chrome/browser/media/media_stream_capture_indicator.cc (left): https://codereview.chromium.org/12035046/diff/1/chrome/browser/media/media_st... chrome/browser/media/media_stream_capture_indicator.cc:205: CreateStatusTray(); On 2013/01/28 19:36:51, Yuri wrote: > On 2013/01/24 07:04:44, justinlin wrote: > > OK. I'm probably nitpicking too much, but could webcam/mic be requested from > an > > extension background page, which might not have a web contents? It would short > > circuit in AddCaptureDevices and not show the tray icon in that case? > > I'm not sure. I'll have to try this out to see whether Chrome is producing the > desired behavior (infobar for user permissions, etc.). In any case, without a > WebContents instance, the AddCaptureDevices() would result in a no-op (i.e., no > system tray icon *and* no UI favicon animation). I meant in that case, we don't have a web_contents (if we called getUserMedia() an extension background page), but we still do want to show the tray icon and such because the background page is capturing webcam/mic. I'm not sure if that's possible, maybe xians@ knows. https://codereview.chromium.org/12035046/diff/8001/chrome/browser/ui/tabs/tab... File chrome/browser/ui/tabs/tab_utils.cc (right): https://codereview.chromium.org/12035046/diff/8001/chrome/browser/ui/tabs/tab... chrome/browser/ui/tabs/tab_utils.cc:32: indicator->IsSupplyingMirroredMedia(render_process_id, render_view_id); On 2013/01/28 19:36:52, Yuri wrote: > On 2013/01/24 07:04:44, justinlin wrote: > > nit: I think I kind of prefer just something "IsMirroringTab" , but I don't > feel > > strongly about it :). The "supplying" word in the function name is what sounds > > weird to me. > > Okay. How about "IsBeingMirrored?" I want to leave the word "tab" out since > the IDs are referencing a RenderView/WebContents. SGTM https://codereview.chromium.org/12035046/diff/14002/chrome/browser/media/medi... File chrome/browser/media/media_stream_capture_indicator.cc (right): https://codereview.chromium.org/12035046/diff/14002/chrome/browser/media/medi... chrome/browser/media/media_stream_capture_indicator.cc:115: bool IsWebContentsDestroyed() const { return web_contents() == NULL; } Hmm, is this really needed? I think callers just checking web_contents() themselves directly is fine. https://codereview.chromium.org/12035046/diff/14002/chrome/browser/media/medi... chrome/browser/media/media_stream_capture_indicator.cc:141: int MediaStreamCaptureIndicator::WebContentsDeviceUsage::TallyUsage( This could probably use a better name since it returns something now. https://codereview.chromium.org/12035046/diff/14002/chrome/browser/media/medi... chrome/browser/media/media_stream_capture_indicator.cc:142: const content::MediaStreamDevices& devices, int inc_amount) { nit: Make inc_amount a bool? https://codereview.chromium.org/12035046/diff/14002/chrome/browser/media/medi... chrome/browser/media/media_stream_capture_indicator.cc:433: AliasMap::const_iterator it = aliases_.find(key); nit: make_pair? your preference. https://codereview.chromium.org/12035046/diff/14002/chrome/browser/media/medi... chrome/browser/media/media_stream_capture_indicator.cc:493: if (alias_it->second == web_contents) { nit: no braces https://codereview.chromium.org/12035046/diff/14002/chrome/browser/media/medi... File chrome/browser/media/media_stream_capture_indicator.h (right): https://codereview.chromium.org/12035046/diff/14002/chrome/browser/media/medi... chrome/browser/media/media_stream_capture_indicator.h:59: // media for remoting). s/remoting/remote broadcast ? https://codereview.chromium.org/12035046/diff/14002/chrome/browser/media/medi... chrome/browser/media/media_stream_capture_indicator.h:93: content::WebContents* LookUpByKnownAlias(int render_process_id, I see why it's currently like this, but I wish there was a way to make this safer. Could it return a WebContentsDeviceUsage* instead somehow? Might need a few more changes. Also, the comment helps but maybe a better name could help too. Maybe FindCachedWebContentsByIDs? Cached suggests a little bit more that it's unsafe to directly deref. https://codereview.chromium.org/12035046/diff/14002/chrome/browser/media/medi... chrome/browser/media/media_stream_capture_indicator.h:138: // A map of known render_process_id/render_view_id pairs that have been maybe "/original/ known [id-pairs]" to clarify that as the render views change, the ID's won't look up the correct web_contents anymore. https://codereview.chromium.org/12035046/diff/14002/chrome/browser/media/medi... chrome/browser/media/media_stream_capture_indicator.h:144: // A map from command_ids to their associated WebContents instance. This is s/map/vector indexed by command_id? And, maybe this typedef and variable should be renamed?
https://codereview.chromium.org/12035046/diff/14002/chrome/browser/media/medi... File chrome/browser/media/media_stream_capture_indicator.cc (right): https://codereview.chromium.org/12035046/diff/14002/chrome/browser/media/medi... chrome/browser/media/media_stream_capture_indicator.cc:115: bool IsWebContentsDestroyed() const { return web_contents() == NULL; } On 2013/01/28 22:31:21, justinlin wrote: > Hmm, is this really needed? I think callers just checking web_contents() > themselves directly is fine. Seemed cleaner. Some reasons: 1) Callers already have the WebContents*, so it's a bit kludgy to make a method call to get it again. 2) web_contents() is a protected method, and I'd rather not make it public because of #1. https://codereview.chromium.org/12035046/diff/14002/chrome/browser/media/medi... chrome/browser/media/media_stream_capture_indicator.cc:142: const content::MediaStreamDevices& devices, int inc_amount) { On 2013/01/28 22:31:21, justinlin wrote: > nit: Make inc_amount a bool? Done. https://codereview.chromium.org/12035046/diff/14002/chrome/browser/media/medi... chrome/browser/media/media_stream_capture_indicator.cc:493: if (alias_it->second == web_contents) { On 2013/01/28 22:31:21, justinlin wrote: > nit: no braces Done. https://codereview.chromium.org/12035046/diff/14002/chrome/browser/media/medi... File chrome/browser/media/media_stream_capture_indicator.h (right): https://codereview.chromium.org/12035046/diff/14002/chrome/browser/media/medi... chrome/browser/media/media_stream_capture_indicator.h:59: // media for remoting). On 2013/01/28 22:31:21, justinlin wrote: > s/remoting/remote broadcast ? Done. https://codereview.chromium.org/12035046/diff/14002/chrome/browser/media/medi... chrome/browser/media/media_stream_capture_indicator.h:93: content::WebContents* LookUpByKnownAlias(int render_process_id, On 2013/01/28 22:31:21, justinlin wrote: > I see why it's currently like this, but I wish there was a way to make this > safer. Could it return a WebContentsDeviceUsage* instead somehow? Might need a > few more changes. I liked this idea, and tried working the code around it. Unfortunately, a lot of things became a bit messy. For example: 1) For the AddCaptureDevices() use case, I had to add "auto create" functionality to the LookUp method with an extra bool argument. 2) When we remove entries from the maps in RemoveCaptureDevices(), I had to have separate "look up the WebContents pointer again" logic, which just duplicates most of the look-up method. > Also, the comment helps but maybe a better name could help > too. Maybe FindCachedWebContentsByIDs? Cached suggests a little bit more that > it's unsafe to directly deref. IMHO, the verb "LookUp" on a method returning a pointer implies a search which can fail. It's not caching that we're doing here; it's more like weak references. https://codereview.chromium.org/12035046/diff/14002/chrome/browser/media/medi... chrome/browser/media/media_stream_capture_indicator.h:138: // A map of known render_process_id/render_view_id pairs that have been On 2013/01/28 22:31:21, justinlin wrote: > maybe "/original/ known [id-pairs]" to clarify that as the render views change, > the ID's won't look up the correct web_contents anymore. Done. https://codereview.chromium.org/12035046/diff/14002/chrome/browser/media/medi... chrome/browser/media/media_stream_capture_indicator.h:144: // A map from command_ids to their associated WebContents instance. This is On 2013/01/28 22:31:21, justinlin wrote: > s/map/vector indexed by command_id? And, maybe this typedef and variable should > be renamed? Done.
one more nit. lgtm if you also test the CL on Mac and Windows. https://codereview.chromium.org/12035046/diff/16002/chrome/browser/media/medi... File chrome/browser/media/media_stream_capture_indicator.cc (right): https://codereview.chromium.org/12035046/diff/16002/chrome/browser/media/medi... chrome/browser/media/media_stream_capture_indicator.cc:502: web_contents->NotifyNavigationStateChanged(content::INVALIDATE_TYPE_TAB); nit, please move this line of code up to the declaration, and you might not need a local bool
shutdown change lgtm https://codereview.chromium.org/12035046/diff/22002/chrome/browser/media/medi... File chrome/browser/media/media_stream_capture_indicator.h (right): https://codereview.chromium.org/12035046/diff/22002/chrome/browser/media/medi... chrome/browser/media/media_stream_capture_indicator.h:135: typedef std::map<content::WebContents*, WebContentsDeviceUsage*> UsageMap; would it be possible to make this a map of objects instead of pointers?
Thanks for the review, everyone. :) https://codereview.chromium.org/12035046/diff/16002/chrome/browser/media/medi... File chrome/browser/media/media_stream_capture_indicator.cc (right): https://codereview.chromium.org/12035046/diff/16002/chrome/browser/media/medi... chrome/browser/media/media_stream_capture_indicator.cc:502: web_contents->NotifyNavigationStateChanged(content::INVALIDATE_TYPE_TAB); On 2013/01/29 10:07:24, xians1 wrote: > nit, please move this line of code up to the declaration, and you might not need > a local bool Done. https://codereview.chromium.org/12035046/diff/22002/chrome/browser/media/medi... File chrome/browser/media/media_stream_capture_indicator.h (right): https://codereview.chromium.org/12035046/diff/22002/chrome/browser/media/medi... chrome/browser/media/media_stream_capture_indicator.h:135: typedef std::map<content::WebContents*, WebContentsDeviceUsage*> UsageMap; On 2013/01/29 21:32:07, justinlin wrote: > would it be possible to make this a map of objects instead of pointers? I would have liked to do this, but WebContentsDeviceUsage would need a default ctor. It's possible to change things around, but a lot of other things would have to move around (e.g., class defns in header file, a separate Init() method, and additional logic in AddCaptureDevices()).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miu@chromium.org/12035046/22002
lgtm
Message was sent while issue was closed.
Change committed as 179490 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
