Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1805583002:
For Desktop tab sharing, activate selected inactive tab (Closed)
DescriptionFor Desktop tab sharing, activate selected inactive tab
BUG=594722
Committed: https://crrev.com/4bfd41b23017c287cf269f34ca563ad4072a6bf4
Cr-Commit-Position: refs/heads/master@{#381473}
Patch Set 1 #
Total comments: 6
Patch Set 2 : #
Total comments: 6
Patch Set 3 : #
Total comments: 1
Patch Set 4 : #Messages
Total messages: 13 (4 generated)
gyzhou@chromium.org changed reviewers: + msw@chromium.org
Mike, Would you please take a look of this CL. Thanks, George
https://codereview.chromium.org/1805583002/diff/1/chrome/browser/ui/views/des... File chrome/browser/ui/views/desktop_media_picker_views.cc (right): https://codereview.chromium.org/1805583002/diff/1/chrome/browser/ui/views/des... chrome/browser/ui/views/desktop_media_picker_views.cc:528: content::WebContents* tab = content::WebContents::FromRenderFrameHost( Is this really the best way to get the WebContents* from the source? (if this is done elsewhere, a helper function might be nice) https://codereview.chromium.org/1805583002/diff/1/chrome/browser/ui/views/des... chrome/browser/ui/views/desktop_media_picker_views.cc:533: for (auto* browser : *BrowserList::GetInstance()) { Use chrome::FindBrowserWithWebContents() instead. https://codereview.chromium.org/1805583002/diff/1/chrome/browser/ui/views/des... chrome/browser/ui/views/desktop_media_picker_views.cc:542: tab_strip_model->ActivateTabAt(tab_index, true); Use Browser::ActivateContents() instead.
Mike, I updated the code based on your comments Thanks https://codereview.chromium.org/1805583002/diff/1/chrome/browser/ui/views/des... File chrome/browser/ui/views/desktop_media_picker_views.cc (right): https://codereview.chromium.org/1805583002/diff/1/chrome/browser/ui/views/des... chrome/browser/ui/views/desktop_media_picker_views.cc:528: content::WebContents* tab = content::WebContents::FromRenderFrameHost( On 2016/03/15 21:33:47, msw wrote: > Is this really the best way to get the WebContents* from the source? > (if this is done elsewhere, a helper function might be nice) DesktopMediaID doesn't have a function to get webcontents directly and no other place in the UI needs webcontents. If you have a better alternative way, please let me know. https://codereview.chromium.org/1805583002/diff/1/chrome/browser/ui/views/des... chrome/browser/ui/views/desktop_media_picker_views.cc:533: for (auto* browser : *BrowserList::GetInstance()) { On 2016/03/15 21:33:47, msw wrote: > Use chrome::FindBrowserWithWebContents() instead. Done. Good suggestion. https://codereview.chromium.org/1805583002/diff/1/chrome/browser/ui/views/des... chrome/browser/ui/views/desktop_media_picker_views.cc:542: tab_strip_model->ActivateTabAt(tab_index, true); On 2016/03/15 21:33:47, msw wrote: > Use Browser::ActivateContents() instead. ActivateTabAt() is a private member of browser, so I use tab->GetDelegate()->ActivateContents(tab) here
https://codereview.chromium.org/1805583002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_media_picker_views.cc (right): https://codereview.chromium.org/1805583002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_media_picker_views.cc:533: if (browser) { When would browser be null? Do we need to check this? https://codereview.chromium.org/1805583002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_media_picker_views.cc:535: DCHECK(tab_strip_model); nit: dcheck before dereference is unnecessary. https://codereview.chromium.org/1805583002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_media_picker_views.cc:538: if (tab_index != tab_strip_model->active_index()) Must we check if the tab is inactive? Otherwise, this whole block becomes: tab->GetDelegate()->ActivateContents(tab); If necessary, use 'tab != browser->tab_strip_model()->GetActiveWebContents()'
Mike, A update based on your comments. You comments make the code much more concise. Thanks, https://codereview.chromium.org/1805583002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_media_picker_views.cc (right): https://codereview.chromium.org/1805583002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_media_picker_views.cc:533: if (browser) { On 2016/03/15 23:58:19, msw wrote: > When would browser be null? Do we need to check this? You are right, as long as tab is valid, browser should be valid. https://codereview.chromium.org/1805583002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_media_picker_views.cc:535: DCHECK(tab_strip_model); On 2016/03/15 23:58:19, msw wrote: > nit: dcheck before dereference is unnecessary. Done. https://codereview.chromium.org/1805583002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_media_picker_views.cc:538: if (tab_index != tab_strip_model->active_index()) On 2016/03/15 23:58:19, msw wrote: > Must we check if the tab is inactive? Otherwise, this whole block becomes: > tab->GetDelegate()->ActivateContents(tab); > If necessary, use 'tab != browser->tab_strip_model()->GetActiveWebContents()' A check probably is not necessary.
lgtm with a nit https://codereview.chromium.org/1805583002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_media_picker_views.cc (right): https://codereview.chromium.org/1805583002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_media_picker_views.cc:529: if (render_frame_host) { nit: FromRenderFrameHost can take a nullptr, inline the content::RenderFrameHost::FromID call, as you had in the prior patch set, or at least remove this check.
The CQ bit was checked by gyzhou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/1805583002/#ps60001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1805583002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1805583002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== For Desktop tab sharing, activate selected inactive tab BUG=594722 ========== to ========== For Desktop tab sharing, activate selected inactive tab BUG=594722 Committed: https://crrev.com/4bfd41b23017c287cf269f34ca563ad4072a6bf4 Cr-Commit-Position: refs/heads/master@{#381473} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/4bfd41b23017c287cf269f34ca563ad4072a6bf4 Cr-Commit-Position: refs/heads/master@{#381473} |
