Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2640433004:
ChromeOS MD: Fix cannot drag tab onto immersive fullscreen window (Closed)
|
Created:
3 years, 11 months ago by Qiang(Joe) Xu Modified:
3 years, 10 months ago CC:
chromium-reviews, tfarina, dcheng Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChromeOS MD: Fix cannot drag tab onto immersive fullscreen window
changes:
This CL is a follow-up fix for https://codereview.chromium.org/2326703002/ MD change.
The new MD design completely hides tabstrip without light bar anymore. In this case, we could consider tabstrip as visible.
BUG=676698
TEST=emulator test saw bug fixed.
Review-Url: https://codereview.chromium.org/2640433004
Cr-Commit-Position: refs/heads/master@{#447658}
Committed: https://chromium.googlesource.com/chromium/src/+/a4cc8622a6cca4971414a46acf812c7ea4e410eb
Patch Set 1 #Patch Set 2 : modify test coverage, but it is a disabled test #
Total comments: 12
Patch Set 3 : clean the code based on ps2's comments #
Total comments: 10
Patch Set 4 : GetBoundsForTabStripInBrowserView and fix test failures #
Total comments: 3
Patch Set 5 : comments #
Total comments: 5
Patch Set 6 : TabStrip::GetIdealWidth #Patch Set 7 : do not set tabstrip bounds empty when unrevealed for md #Patch Set 8 : immersive fullscreen tabstrip visible #Patch Set 9 : fix browser test failure #Patch Set 10 : nits for ps9 #
Total comments: 4
Patch Set 11 : comments from ps10 #
Total comments: 4
Patch Set 12 : comments from ps11 #
Messages
Total messages: 82 (57 generated)
The CQ bit was checked by warx@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...
Patchset #2 (id:20001) has been deleted
Description was changed from ========== ChromeOS MD: Fix cannot drag tab onto immersive fullscreen window BUG=676698 ========== to ========== ChromeOS MD: Fix cannot drag tab onto immersive fullscreen window changes: ChromeOS MD makes tabstrip invisible when browser window enters immersive fullscreen mode, which makes the logic in tab_drag_controller broken. (1) When browser window is immersive non-revealed mode, i.e. tabstrip bounds is zero, get tabstrip_bounds from non-client view coordinates where tabstrip should be laid when revealed for checking DoesTabStripContain. (2) BrowserViewLayout::LayoutTabStripRegion lacks the procedure to restore tabstrip bounds for this case. Do it in DoesTabStripContain. BUG=676698 TEST=emulator test saw bug fixed. ==========
warx@chromium.org changed reviewers: + sky@chromium.org, tdanderson@chromium.org, yiyix@chromium.org
Description was changed from ========== ChromeOS MD: Fix cannot drag tab onto immersive fullscreen window changes: ChromeOS MD makes tabstrip invisible when browser window enters immersive fullscreen mode, which makes the logic in tab_drag_controller broken. (1) When browser window is immersive non-revealed mode, i.e. tabstrip bounds is zero, get tabstrip_bounds from non-client view coordinates where tabstrip should be laid when revealed for checking DoesTabStripContain. (2) BrowserViewLayout::LayoutTabStripRegion lacks the procedure to restore tabstrip bounds for this case. Do it in DoesTabStripContain. BUG=676698 TEST=emulator test saw bug fixed. ========== to ========== ChromeOS MD: Fix cannot drag tab onto immersive fullscreen window changes: This CL is a follow-up fix for https://codereview.chromium.org/2326703002/ MD change. ChromeOS MD makes tabstrip invisible when browser window enters immersive fullscreen mode, which makes the logic in tab_drag_controller broken. (1) When browser window is immersive non-revealed mode, i.e. tabstrip bounds is zero, get tabstrip_bounds from non-client view coordinates where tabstrip should be laid when revealed for checking DoesTabStripContain. (2) BrowserViewLayout::LayoutTabStripRegion lacks the procedure to restore tabstrip bounds for this case. Do it in DoesTabStripContain. BUG=676698 TEST=emulator test saw bug fixed. ==========
sky@ for owner review, tdanderson@, yiyi@ for md. Thanks!
https://codereview.chromium.org/2640433004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/2640433004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:862: #if defined(OS_CHROMEOS) Why does this need to be chromeos specific? ImmersiveModeController isn't chromeos specific. Also, rather than calling ShouldHideTopViews() can you check BrowserView::IsTabStripVisible? https://codereview.chromium.org/2640433004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:885: tabstrip->SetVisible(true); This function is meant to check whether the tabstrip contains a point. It shouldn't have side effects. If you need to change visibility that should happen else where. https://codereview.chromium.org/2640433004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (right): https://codereview.chromium.org/2640433004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:2108: if (is_using_material_design) { Is this test run in both md and non-md?
On 2017/01/17 18:07:01, sky wrote: > https://codereview.chromium.org/2640433004/diff/40001/chrome/browser/ui/views... > File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): > > https://codereview.chromium.org/2640433004/diff/40001/chrome/browser/ui/views... > chrome/browser/ui/views/tabs/tab_drag_controller.cc:862: #if > defined(OS_CHROMEOS) > Why does this need to be chromeos specific? ImmersiveModeController isn't > chromeos specific. Also, rather than calling ShouldHideTopViews() can you check > BrowserView::IsTabStripVisible? > > https://codereview.chromium.org/2640433004/diff/40001/chrome/browser/ui/views... > chrome/browser/ui/views/tabs/tab_drag_controller.cc:885: > tabstrip->SetVisible(true); > This function is meant to check whether the tabstrip contains a point. It > shouldn't have side effects. If you need to change visibility that should happen > else where. > > https://codereview.chromium.org/2640433004/diff/40001/chrome/browser/ui/views... > File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc > (right): > > https://codereview.chromium.org/2640433004/diff/40001/chrome/browser/ui/views... > chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:2108: if > (is_using_material_design) { > Is this test run in both md and non-md? Please see the note I left on the bug regarding the milestone to target this CL. I will wait until sky's comments are addressed before reviewing in more detail.
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by warx@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 ========== ChromeOS MD: Fix cannot drag tab onto immersive fullscreen window changes: This CL is a follow-up fix for https://codereview.chromium.org/2326703002/ MD change. ChromeOS MD makes tabstrip invisible when browser window enters immersive fullscreen mode, which makes the logic in tab_drag_controller broken. (1) When browser window is immersive non-revealed mode, i.e. tabstrip bounds is zero, get tabstrip_bounds from non-client view coordinates where tabstrip should be laid when revealed for checking DoesTabStripContain. (2) BrowserViewLayout::LayoutTabStripRegion lacks the procedure to restore tabstrip bounds for this case. Do it in DoesTabStripContain. BUG=676698 TEST=emulator test saw bug fixed. ========== to ========== ChromeOS MD: Fix cannot drag tab onto immersive fullscreen window changes: This CL is a follow-up fix for https://codereview.chromium.org/2326703002/ MD change. ChromeOS MD makes tabstrip invisible when browser window enters immersive fullscreen mode, which makes the logic in tab_drag_controller broken. (1) Using tabstrip bounds where it should be laid out in browser view instead of local bounds. (2) If target_tabstrip is invisible, show it before attaching. BUG=676698 TEST=emulator test saw bug fixed. ==========
Hi all, there is an update, ptal, thanks! Changes are also updated in issue description. https://codereview.chromium.org/2640433004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/2640433004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:862: #if defined(OS_CHROMEOS) On 2017/01/17 18:07:01, sky wrote: > Why does this need to be chromeos specific? ImmersiveModeController isn't > chromeos specific. Also, rather than calling ShouldHideTopViews() can you check > BrowserView::IsTabStripVisible? done https://codereview.chromium.org/2640433004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:885: tabstrip->SetVisible(true); On 2017/01/17 18:07:01, sky wrote: > This function is meant to check whether the tabstrip contains a point. It > shouldn't have side effects. If you need to change visibility that should happen > else where. done by moving to DragBrowserToNewTabStrip https://codereview.chromium.org/2640433004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (right): https://codereview.chromium.org/2640433004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:2108: if (is_using_material_design) { On 2017/01/17 18:07:01, sky wrote: > Is this test run in both md and non-md? I don't know.. Do you mean it may not actually run for MD since this test doesn't setup ash?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2640433004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (right): https://codereview.chromium.org/2640433004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:11: #include "ash/common/material_design/material_design_controller.h" Move to USE_ASH ifdef below. https://codereview.chromium.org/2640433004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:2108: if (is_using_material_design) { On 2017/01/18 01:13:52, Qiang(Joe) Xu wrote: > On 2017/01/17 18:07:01, sky wrote: > > Is this test run in both md and non-md? > > I don't know.. Do you mean it may not actually run for MD since this test > doesn't setup ash? You have two code paths here, one md and one non-md. Are they both exercised? How? It looks like the test is disabled. Can you fix it, otherwise we're getting no coverage. https://codereview.chromium.org/2640433004/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/2640433004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:114: // Get tabstrip bounds from non-client view coordinates where tabstrip should be I think you mean, "Returns the bounds of the tabstrip in screen coordinates in |rect|." I'm a bit confused on how this fails, comments below. https://codereview.chromium.org/2640433004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:119: return false; I think callers should check for this case. https://codereview.chromium.org/2640433004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:121: if (!window) Can this case really happen? https://codereview.chromium.org/2640433004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:125: if (!browser_view) Same comment here. Can this case really happen? https://codereview.chromium.org/2640433004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:127: *rect = browser_view->frame()->GetBoundsForTabStrip(tabstrip); Why do you need to use GetBoundsForTabStrip rather than the actual bounds?
https://codereview.chromium.org/2640433004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (right): https://codereview.chromium.org/2640433004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:2108: if (is_using_material_design) { On 2017/01/18 16:40:45, sky wrote: > On 2017/01/18 01:13:52, Qiang(Joe) Xu wrote: > > On 2017/01/17 18:07:01, sky wrote: > > > Is this test run in both md and non-md? > > > > I don't know.. Do you mean it may not actually run for MD since this test > > doesn't setup ash? > > You have two code paths here, one md and one non-md. Are they both exercised? > How? > It looks like the test is disabled. Can you fix it, otherwise we're getting no > coverage. I think the test is running with the default value of ash-md flag, with which function IsImersiveModeMaterial() always return true. Regarding comment "completely hidden in immersive fullscreen mode.", both immersive mode and fullscreen mode exist in chromeos and they are different. Could you clarify which mode you refer to? I think you mean immersive mode.
The CQ bit was checked by warx@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 ========== ChromeOS MD: Fix cannot drag tab onto immersive fullscreen window changes: This CL is a follow-up fix for https://codereview.chromium.org/2326703002/ MD change. ChromeOS MD makes tabstrip invisible when browser window enters immersive fullscreen mode, which makes the logic in tab_drag_controller broken. (1) Using tabstrip bounds where it should be laid out in browser view instead of local bounds. (2) If target_tabstrip is invisible, show it before attaching. BUG=676698 TEST=emulator test saw bug fixed. ========== to ========== ChromeOS MD: Fix cannot drag tab onto immersive fullscreen window changes: This CL is a follow-up fix for https://codereview.chromium.org/2326703002/ MD change. ChromeOS MD makes tabstrip invisible when browser window enters immersive fullscreen mode, which makes the logic in tab_drag_controller broken. (1) Using tabstrip bounds where it should be laid out in browser view instead of actual bounds as they are (0, 0, 0x0). (2) If target_tabstrip is invisible, show it before attaching. BUG=676698 TEST=emulator test saw bug fixed. ==========
Patchset #4 (id:100001) has been deleted
The CQ bit was checked by warx@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: This issue passed the CQ dry run.
Hi all, ptal on new patch. Changes: (1) Using GetBoundsForTabStripInBrowserView, same as https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi... (2) Make DoesTabStripContain make in browser view coordinates I still don't know how to enable the disabled test, but don't want it block the other code review right now, so I am sending it out. Thanks! https://codereview.chromium.org/2640433004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc (right): https://codereview.chromium.org/2640433004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:11: #include "ash/common/material_design/material_design_controller.h" On 2017/01/18 16:40:45, sky wrote: > Move to USE_ASH ifdef below. Done. https://codereview.chromium.org/2640433004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:2108: if (is_using_material_design) { On 2017/01/18 16:40:45, sky wrote: > On 2017/01/18 01:13:52, Qiang(Joe) Xu wrote: > > On 2017/01/17 18:07:01, sky wrote: > > > Is this test run in both md and non-md? > > > > I don't know.. Do you mean it may not actually run for MD since this test > > doesn't setup ash? > > You have two code paths here, one md and one non-md. Are they both exercised? > How? > It looks like the test is disabled. Can you fix it, otherwise we're getting no > coverage. It seems is_using_material_deisn is exercised right now. Currently I don't know how to fix the test yet. It waits forever on DragTabToWindowInSeparateDisplayStep2 and constatnly fails on chromeos. https://codereview.chromium.org/2640433004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:2108: if (is_using_material_design) { On 2017/01/18 20:41:50, yiyix wrote: > On 2017/01/18 16:40:45, sky wrote: > > On 2017/01/18 01:13:52, Qiang(Joe) Xu wrote: > > > On 2017/01/17 18:07:01, sky wrote: > > > > Is this test run in both md and non-md? > > > > > > I don't know.. Do you mean it may not actually run for MD since this test > > > doesn't setup ash? > > > > You have two code paths here, one md and one non-md. Are they both exercised? > > How? > > It looks like the test is disabled. Can you fix it, otherwise we're getting > no > > coverage. > > I think the test is running with the default value of ash-md flag, with which > function IsImersiveModeMaterial() always return true. > > Regarding comment "completely hidden in immersive fullscreen mode.", both > immersive mode and fullscreen mode exist in chromeos and they are different. > Could you clarify which mode you refer to? I think you mean immersive mode. I think "immersive fullscreen" means "immersive unrevealed". It is used in many places in the code base. https://codereview.chromium.org/2640433004/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/2640433004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:114: // Get tabstrip bounds from non-client view coordinates where tabstrip should be On 2017/01/18 16:40:46, sky wrote: > I think you mean, "Returns the bounds of the tabstrip in screen coordinates in > |rect|." I'm a bit confused on how this fails, comments below. changed to GetBoundsForTabStripInBrowserView, so change the comments. https://codereview.chromium.org/2640433004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:119: return false; On 2017/01/18 16:40:46, sky wrote: > I think callers should check for this case. caller will make sure it https://codereview.chromium.org/2640433004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:121: if (!window) On 2017/01/18 16:40:46, sky wrote: > Can this case really happen? seems cannot happen. remove it. https://codereview.chromium.org/2640433004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:125: if (!browser_view) On 2017/01/18 16:40:46, sky wrote: > Same comment here. Can this case really happen? Done. https://codereview.chromium.org/2640433004/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_drag_controller.cc:127: *rect = browser_view->frame()->GetBoundsForTabStrip(tabstrip); On 2017/01/18 16:40:46, sky wrote: > Why do you need to use GetBoundsForTabStrip rather than the actual bounds? the actual bounds will return (0, 0, 0x0) for this case.
Based on the description it seems to me the reveal is not being shown when the tab is dragged to the top of the window. Is that what is happening? If that is the case the immersive code should trigger the reveal, and then I think it'll all work. https://codereview.chromium.org/2640433004/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/2640433004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller.cc:116: aura::Window* window = tabstrip->GetWidget()->GetNativeWindow(); gfx::NativeView (so that this works in non-aura builds). https://codereview.chromium.org/2640433004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller.cc:590: if (!target_tabstrip->visible()) { You should not be changing the visibility of the tabstrip. That indicates something else has gone wrong.
Patchset #5 (id:140001) has been deleted
The CQ bit was checked by warx@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: This issue passed the CQ dry run.
https://codereview.chromium.org/2640433004/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/2640433004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller.cc:116: aura::Window* window = tabstrip->GetWidget()->GetNativeWindow(); On 2017/01/20 18:03:43, sky wrote: > gfx::NativeView (so that this works in non-aura builds). gfx::NativeWindow? https://codereview.chromium.org/2640433004/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/2640433004/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller.cc:903: attached_tabstrip_->StartedDraggingTabs(tabs); We should get ImmersiveModeCOntrollerAsh::OnImmersiveRevealedStarted called here (which will layout tabstrip region). A better way might be calling StartedDraggingTabs, to let TabStripController know that user started dragging tabs, which will call GetRevealedLock to start revealing. There is also a StartedDraggingTabs(tabs) call after InsertWebContentsAt call.
https://codereview.chromium.org/2640433004/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/2640433004/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller.cc:903: attached_tabstrip_->StartedDraggingTabs(tabs); On 2017/01/22 01:43:57, Qiang(Joe) Xu wrote: > We should get ImmersiveModeCOntrollerAsh::OnImmersiveRevealedStarted called here > (which will layout tabstrip region). A better way might be calling > StartedDraggingTabs, to let TabStripController know that user started dragging > tabs, which will call GetRevealedLock to start revealing. There is also a > StartedDraggingTabs(tabs) call after InsertWebContentsAt call. StartedDraggingTabs is called on line 949.
https://codereview.chromium.org/2640433004/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/2640433004/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller.cc:903: attached_tabstrip_->StartedDraggingTabs(tabs); On 2017/01/23 16:19:06, sky wrote: > On 2017/01/22 01:43:57, Qiang(Joe) Xu wrote: > > We should get ImmersiveModeCOntrollerAsh::OnImmersiveRevealedStarted called > here > > (which will layout tabstrip region). A better way might be calling > > StartedDraggingTabs, to let TabStripController know that user started dragging > > tabs, which will call GetRevealedLock to start revealing. There is also a > > StartedDraggingTabs(tabs) call after InsertWebContentsAt call. > > StartedDraggingTabs is called on line 949. Yes, I am aware of that. The problem is here: https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab_drag_co..., the attached_->ideal_bounds(i) will give the minimum tab size(32, 29), not the actual tab width, which makes the logic broken for GetInsertionIndexFrom. So we have to reveal the tabstrip before insertion happens. Equivalently, make this happen https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi... with reveal unlocked before insertion. I am calling StartedDraggingTabs here, but the only call I need is https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab_strip.c... (controller_->OnStartedDraggingTabs()). I think make a call of StartedDraggingTabs with |tabs| before insertion is OK. It is doing layouting/animation the UI work with the state just before attaching new tab https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab_strip.c...
https://codereview.chromium.org/2640433004/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/2640433004/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller.cc:903: attached_tabstrip_->StartedDraggingTabs(tabs); On 2017/01/23 17:50:37, Qiang(Joe) Xu wrote: > On 2017/01/23 16:19:06, sky wrote: > > On 2017/01/22 01:43:57, Qiang(Joe) Xu wrote: > > > We should get ImmersiveModeCOntrollerAsh::OnImmersiveRevealedStarted called > > here > > > (which will layout tabstrip region). A better way might be calling > > > StartedDraggingTabs, to let TabStripController know that user started > dragging > > > tabs, which will call GetRevealedLock to start revealing. There is also a > > > StartedDraggingTabs(tabs) call after InsertWebContentsAt call. > > > > StartedDraggingTabs is called on line 949. > > Yes, I am aware of that. The problem is here: > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab_drag_co..., > the attached_->ideal_bounds(i) will give the minimum tab size(32, 29), not the > actual tab width, which makes the logic broken for GetInsertionIndexFrom. So we > have to reveal the tabstrip before insertion happens. Equivalently, make this > happen > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi... > with reveal unlocked before insertion. I am calling StartedDraggingTabs here, > but the only call I need is > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab_strip.c... > (controller_->OnStartedDraggingTabs()). I think make a call of > StartedDraggingTabs with |tabs| before insertion is OK. It is doing > layouting/animation the UI work with the state just before attaching new tab > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab_strip.c... So, you're solution to this is to call the function twice? That doesn't strike you as a hack?
The CQ bit was checked by warx@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 ========== ChromeOS MD: Fix cannot drag tab onto immersive fullscreen window changes: This CL is a follow-up fix for https://codereview.chromium.org/2326703002/ MD change. ChromeOS MD makes tabstrip invisible when browser window enters immersive fullscreen mode, which makes the logic in tab_drag_controller broken. (1) Using tabstrip bounds where it should be laid out in browser view instead of actual bounds as they are (0, 0, 0x0). (2) If target_tabstrip is invisible, show it before attaching. BUG=676698 TEST=emulator test saw bug fixed. ========== to ========== ChromeOS MD: Fix cannot drag tab onto immersive fullscreen window changes: This CL is a follow-up fix for https://codereview.chromium.org/2326703002/ MD change. ChromeOS MD makes tabstrip invisible when browser window enters immersive fullscreen mode, which makes the logic in tab_drag_controller broken. (1) Using tabstrip bounds where it should be laid out in browser view instead of actual bounds as they are (0, 0, 0x0). (2) instead of using tabstrip::width(), which will be zero when unrevealed, using tabstrip::GetBoundsInBrowserView().width(), in GenerateIdealBounds(). BUG=676698 TEST=emulator test saw bug fixed. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by warx@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #7 (id:200001) has been deleted
Patchset #6 (id:180001) has been deleted
Description was changed from ========== ChromeOS MD: Fix cannot drag tab onto immersive fullscreen window changes: This CL is a follow-up fix for https://codereview.chromium.org/2326703002/ MD change. ChromeOS MD makes tabstrip invisible when browser window enters immersive fullscreen mode, which makes the logic in tab_drag_controller broken. (1) Using tabstrip bounds where it should be laid out in browser view instead of actual bounds as they are (0, 0, 0x0). (2) instead of using tabstrip::width(), which will be zero when unrevealed, using tabstrip::GetBoundsInBrowserView().width(), in GenerateIdealBounds(). BUG=676698 TEST=emulator test saw bug fixed. ========== to ========== ChromeOS MD: Fix cannot drag tab onto immersive fullscreen window changes: This CL is a follow-up fix for https://codereview.chromium.org/2326703002/ MD change. ChromeOS MD makes tabstrip invisible when browser window enters immersive fullscreen mode, which makes the logic in tab_drag_controller broken. (1) Using tabstrip bounds where it should be laid out in browser view instead of actual bounds as they are (0, 0, 0x0). (2) instead of using tabstrip::width(), which will be zero when unrevealed, create a new method GetIdealWidth(). BUG=676698 TEST=emulator test saw bug fixed. ==========
The CQ bit was checked by warx@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: This issue passed the CQ dry run.
Hi, new patch uploaded, ptal, thanks! https://codereview.chromium.org/2640433004/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/tab_drag_controller.cc (right): https://codereview.chromium.org/2640433004/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/tab_drag_controller.cc:903: attached_tabstrip_->StartedDraggingTabs(tabs); On 2017/01/23 20:07:26, sky wrote: > On 2017/01/23 17:50:37, Qiang(Joe) Xu wrote: > > On 2017/01/23 16:19:06, sky wrote: > > > On 2017/01/22 01:43:57, Qiang(Joe) Xu wrote: > > > > We should get ImmersiveModeCOntrollerAsh::OnImmersiveRevealedStarted > called > > > here > > > > (which will layout tabstrip region). A better way might be calling > > > > StartedDraggingTabs, to let TabStripController know that user started > > dragging > > > > tabs, which will call GetRevealedLock to start revealing. There is also a > > > > StartedDraggingTabs(tabs) call after InsertWebContentsAt call. > > > > > > StartedDraggingTabs is called on line 949. > > > > Yes, I am aware of that. The problem is here: > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab_drag_co..., > > the attached_->ideal_bounds(i) will give the minimum tab size(32, 29), not the > > actual tab width, which makes the logic broken for GetInsertionIndexFrom. So > we > > have to reveal the tabstrip before insertion happens. Equivalently, make this > > happen > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi... > > with reveal unlocked before insertion. I am calling StartedDraggingTabs here, > > but the only call I need is > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab_strip.c... > > (controller_->OnStartedDraggingTabs()). I think make a call of > > StartedDraggingTabs with |tabs| before insertion is OK. It is doing > > layouting/animation the UI work with the state just before attaching new tab > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab_strip.c... > > So, you're solution to this is to call the function twice? That doesn't strike > you as a hack? Hi, I am sorry I thought this could work. Another solution would be do the work for ideal bounds for tabs, which I updated in the new patch.
I think you're working around how the reveal is triggered. I think the immersive code should be responsible for observing when it should trigger, not the tab strip code.
Description was changed from ========== ChromeOS MD: Fix cannot drag tab onto immersive fullscreen window changes: This CL is a follow-up fix for https://codereview.chromium.org/2326703002/ MD change. ChromeOS MD makes tabstrip invisible when browser window enters immersive fullscreen mode, which makes the logic in tab_drag_controller broken. (1) Using tabstrip bounds where it should be laid out in browser view instead of actual bounds as they are (0, 0, 0x0). (2) instead of using tabstrip::width(), which will be zero when unrevealed, create a new method GetIdealWidth(). BUG=676698 TEST=emulator test saw bug fixed. ========== to ========== ChromeOS MD: Fix cannot drag tab onto immersive fullscreen window changes: This CL is a follow-up fix for https://codereview.chromium.org/2326703002/ MD change. ChromeOS MD makes tabstrip bounds empty when browser window enters immersive fullscreen mode, which makes the logic in tab_drag_controller broken. Solution is when layouting tabstrip region, do not set tabstrip bounds empty when unrevealed for MD. BUG=676698 TEST=emulator test saw bug fixed. ==========
Hi sky@, ptal, thanks! Instead of making tabstrip visible for immersive fullscreen in md, I am doing "do not set bounds empty". I think this way is better. Because anyway tabstrip is actually invisible. Also, if we made tabstrip visible (though it will not be shown to user), several tests need to be updated, like in https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/immersive_....
I prefer keeping the tabstrip visible, I think that has less side effects with other code.
The CQ bit was checked by warx@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_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #9 (id:280001) has been deleted
The CQ bit was checked by warx@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 ========== ChromeOS MD: Fix cannot drag tab onto immersive fullscreen window changes: This CL is a follow-up fix for https://codereview.chromium.org/2326703002/ MD change. ChromeOS MD makes tabstrip bounds empty when browser window enters immersive fullscreen mode, which makes the logic in tab_drag_controller broken. Solution is when layouting tabstrip region, do not set tabstrip bounds empty when unrevealed for MD. BUG=676698 TEST=emulator test saw bug fixed. ========== to ========== ChromeOS MD: Fix cannot drag tab onto immersive fullscreen window changes: This CL is a follow-up fix for https://codereview.chromium.org/2326703002/ MD change. The new MD design completely hides tabstrip without light bar anymore. In this case, we could consider tabstrip as visible. BUG=676698 TEST=emulator test saw bug fixed. ==========
Hi sky@, ptal at this patch, thanks! Change is to make tabstrip visible for immersive fullscreen md.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2640433004/diff/320001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2640433004/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:531: #if defined(USE_ASH) USE_ASH is now the same as OS_CHROMEOS and will eventually be replaced by it. Please use OS_CHROMEOS in new code (as your touching includes above, please merge the USE_ASH and OS_CHROMEOS sections. https://codereview.chromium.org/2640433004/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:534: if (ash::MaterialDesignController::IsImmersiveModeMaterial()) Does it really matter whether md or not? I'm wondering if we can remove the branch (lines 530-539) entirely?
Patchset #11 (id:330001) has been deleted
Hi sky@, ptal, thanks! https://codereview.chromium.org/2640433004/diff/320001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2640433004/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:531: #if defined(USE_ASH) On 2017/02/01 16:31:37, sky wrote: > USE_ASH is now the same as OS_CHROMEOS and will eventually be replaced by it. > Please use OS_CHROMEOS in new code (as your touching includes above, please > merge the USE_ASH and OS_CHROMEOS sections. Done. https://codereview.chromium.org/2640433004/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:534: if (ash::MaterialDesignController::IsImmersiveModeMaterial()) On 2017/02/01 16:31:37, sky wrote: > Does it really matter whether md or not? I'm wondering if we can remove the > branch (lines 530-539) entirely? done, and I find IsImmersiveModeMaterial is equivalent to ShouldHideTabIndicators. So we can completely remote this trunk of lines.
LGTM https://codereview.chromium.org/2640433004/diff/350001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2640433004/diff/350001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:154: #endif // !defined(OS_CHROMEOS) Conditional ifdefs are harder to read. Please start a new ifdef for the !chromeos case. https://codereview.chromium.org/2640433004/diff/350001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:1655: // For Ash only, trusted windows (apps and settings) do not show a title, Ash -> Chrome OS (I think I'm not sure what the right capitalization and all that is, derat@ could tell you for sure). Update this in all blocks you're changing.
tdanderson@, yiyix@, would you also take a look at this? Thanks! https://codereview.chromium.org/2640433004/diff/350001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2640433004/diff/350001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:154: #endif // !defined(OS_CHROMEOS) On 2017/02/01 21:36:12, sky wrote: > Conditional ifdefs are harder to read. Please start a new ifdef for the > !chromeos case. Done. https://codereview.chromium.org/2640433004/diff/350001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_view.cc:1655: // For Ash only, trusted windows (apps and settings) do not show a title, On 2017/02/01 21:36:12, sky wrote: > Ash -> Chrome OS (I think I'm not sure what the right capitalization and all > that is, derat@ could tell you for sure). Update this in all blocks you're > changing. Done.
Patch set 12 lgtm
On 2017/02/01 23:08:05, tdanderson wrote: > Patch set 12 lgtm I think I can land it now to not block https://codereview.chromium.org/2656983005/. Thanks for review!
The CQ bit was checked by warx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2640433004/#ps370001 (title: "comments from ps11")
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": 370001, "attempt_start_ts": 1485992092959750,
"parent_rev": "9370d9b03ca9cfaf4d7fbc69fec5f22786dd0241", "commit_rev":
"a4cc8622a6cca4971414a46acf812c7ea4e410eb"}
Message was sent while issue was closed.
Description was changed from ========== ChromeOS MD: Fix cannot drag tab onto immersive fullscreen window changes: This CL is a follow-up fix for https://codereview.chromium.org/2326703002/ MD change. The new MD design completely hides tabstrip without light bar anymore. In this case, we could consider tabstrip as visible. BUG=676698 TEST=emulator test saw bug fixed. ========== to ========== ChromeOS MD: Fix cannot drag tab onto immersive fullscreen window changes: This CL is a follow-up fix for https://codereview.chromium.org/2326703002/ MD change. The new MD design completely hides tabstrip without light bar anymore. In this case, we could consider tabstrip as visible. BUG=676698 TEST=emulator test saw bug fixed. Review-Url: https://codereview.chromium.org/2640433004 Cr-Commit-Position: refs/heads/master@{#447658} Committed: https://chromium.googlesource.com/chromium/src/+/a4cc8622a6cca4971414a46acf81... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:370001) as https://chromium.googlesource.com/chromium/src/+/a4cc8622a6cca4971414a46acf81... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
