Revert "Make PWA titlebar and icons flex and animate properly."

This reverts commit 8f2f4243.

Reason for revert: speculative revert for test failures on Mac
for WebAppFrameToolbarBrowserTest.SpaceConstrained, see e.g.
https://ci.chromium.org/p/chromium/builders/ci/Mac10.12%20Tests/31700

Original change's description:
> Make PWA titlebar and icons flex and animate properly.
> 
> Nested layouts now allocate available space properly to their
> descendents. Also, we consistently use flex layout in the PWA titlebar.
> 
> Removed a couple of invalid assumptions that were encoded as DCHECKs.
> 
> There are still some inkdrop issues but they are purely cosmetic, see
> crbug.com/1006162 for more detail.
> 
> Bug: 1048061
> Change-Id: I403b71b5e2bbcb2d461e1ed6c543c76f91d4c652
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2036264
> Commit-Queue: Dana Fried <dfried@chromium.org>
> Reviewed-by: Peter Boström <pbos@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#738874}

TBR=pbos@chromium.org,dfried@chromium.org

Change-Id: I731bad118043652a6dbd082c6099ebbd0c697835
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1048061
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2041615Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#738907}

chrome/browser/ui/views/extensions/extensions_toolbar_container.cc

@@ -36,41 +36,26 @@ ExtensionsToolbarContainer::DropInfo::DropInfo(
     size_t index)
     : action_id(action_id), index(index) {}
 
-ExtensionsToolbarContainer::ExtensionsToolbarContainer(Browser* browser,
-                                                       DisplayMode display_mode)
+ExtensionsToolbarContainer::ExtensionsToolbarContainer(Browser* browser)
     : ToolbarIconContainerView(/*uses_highlight=*/true),
       browser_(browser),
       model_(ToolbarActionsModel::Get(browser_->profile())),
       model_observer_(this),
-      extensions_button_(new ExtensionsToolbarButton(browser_, this)),
-      display_mode_(display_mode) {
+      extensions_button_(new ExtensionsToolbarButton(browser_, this)) {
   model_observer_.Add(model_);
   // Do not flip the Extensions icon in RTL.
   extensions_button_->EnableCanvasFlippingForRTLUI(false);
-
-  const views::FlexSpecification hide_icon_flex_specification =
-      views::FlexSpecification(views::MinimumFlexSizeRule::kPreferredSnapToZero,
-                               views::MaximumFlexSizeRule::kPreferred)
-          .WithWeight(0);
-  switch (display_mode) {
-    case DisplayMode::kNormal:
-      // In normal mode, the menu icon is always shown.
-      extensions_button_->SetProperty(views::kFlexBehaviorKey,
-                                      views::FlexSpecification());
-      break;
-    case DisplayMode::kCompact:
-      // In compact mode, the menu icon can be hidden but has the highest
-      // priority.
-      extensions_button_->SetProperty(
-          views::kFlexBehaviorKey, hide_icon_flex_specification.WithOrder(1));
-      break;
-  }
+  extensions_button_->SetProperty(views::kFlexBehaviorKey,
+                                  views::FlexSpecification());
   extensions_button_->SetID(VIEW_ID_EXTENSIONS_MENU_BUTTON);
   AddMainButton(extensions_button_);
   target_layout_manager()
       ->SetFlexAllocationOrder(views::FlexAllocationOrder::kReverse)
       .SetDefault(views::kFlexBehaviorKey,
-                  hide_icon_flex_specification.WithOrder(3));
+                  views::FlexSpecification(
+                      views::MinimumFlexSizeRule::kPreferredSnapToZero,
+                      views::MaximumFlexSizeRule::kPreferred)
+                      .WithWeight(0));
   CreateActions();
 
   // TODO(pbos): Consider splitting out tab-strip observing into another class.
@@ -151,28 +136,11 @@ void ExtensionsToolbarContainer::UpdateIconVisibility(
   // Popped out action uses a flex rule that causes it to always be visible
   // regardless of space; default for actions is to drop out when there is
   // insufficient space. So if an action is being forced visible, it should have
-  // a rule that gives it higher priority, and if it does not, it should use the
-  // default.
+  // the always-show flex rule, and if it not, it should not.
   const bool must_show = ShouldForceVisibility(extension_id);
   if (must_show) {
-    switch (display_mode_) {
-      case DisplayMode::kNormal:
-        // In normal mode, the icon's visibility is forced.
-        action_view->SetProperty(views::kFlexBehaviorKey,
-                                 views::FlexSpecification());
-        break;
-      case DisplayMode::kCompact:
-        // In compact mode, the icon can still drop out, but receives precedence
-        // over other actions.
-        action_view->SetProperty(
-            views::kFlexBehaviorKey,
-            views::FlexSpecification(
-                views::MinimumFlexSizeRule::kPreferredSnapToZero,
-                views::MaximumFlexSizeRule::kPreferred)
-                .WithWeight(0)
-                .WithOrder(2));
-        break;
-    }
+    action_view->SetProperty(views::kFlexBehaviorKey,
+                             views::FlexSpecification());
   } else {
     action_view->ClearProperty(views::kFlexBehaviorKey);
   }

chrome/browser/ui/views/extensions/extensions_toolbar_container.h

@@ -38,22 +38,7 @@ class ExtensionsToolbarContainer : public ToolbarIconContainerView,
   using ToolbarIconMap = std::map<ToolbarActionsModel::ActionId,
                                   std::unique_ptr<ToolbarActionView>>;
 
-  // Determines how the container displays - specifically whether the menu and
-  // popped out action can be hidden.
-  enum class DisplayMode {
-    // In normal mode, the menu icon and popped-out action is always visible.
-    // Normal mode is used for the main toolbar and in windows where there is
-    // always enough space to show at least two icons.
-    kNormal,
-    // In compact mode, one or both of the menu icon and popped-out action may
-    // be hidden. Compact mode is used in smaller windows (e.g. webapps) where
-    // there may not be enough space to display the buttons.
-    kCompact,
-  };
-
-  explicit ExtensionsToolbarContainer(
-      Browser* browser,
-      DisplayMode display_mode = DisplayMode::kNormal);
+  explicit ExtensionsToolbarContainer(Browser* browser);
   ~ExtensionsToolbarContainer() override;
 
   ExtensionsToolbarButton* extensions_button() const {
@@ -200,7 +185,6 @@ class ExtensionsToolbarContainer : public ToolbarIconContainerView,
   ScopedObserver<ToolbarActionsModel, ToolbarActionsModel::Observer>
       model_observer_;
   ExtensionsToolbarButton* const extensions_button_;
-  DisplayMode display_mode_;
 
   // TODO(pbos): Create actions and icons only for pinned pinned / popped out
   // actions (lazily). Currently code expects GetActionForId() to return

chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc

@@ -841,8 +841,8 @@ class WebAppNonClientFrameViewAshTest
   }
 
   PageActionIconView* GetPageActionIcon(PageActionIconType type) {
-    return browser_view_->toolbar_button_provider()->GetPageActionIconView(
-        type);
+    return browser_view_->toolbar_button_provider()
+        ->GetPageActionIconView(type);
   }
 
   ContentSettingImageView* GrantGeolocationPermission() {
@@ -1044,10 +1044,6 @@ IN_PROC_BROWSER_TEST_P(WebAppNonClientFrameViewAshTest,
   SetUpWebApp();
   ContentSettingImageView* geolocation_icon = GrantGeolocationPermission();
 
-  // In order to receive focus, the geo icon must be laid out (and be both
-  // visible and nonzero size).
-  web_app_frame_toolbar_->Layout();
-
   EXPECT_FALSE(web_app_menu_button_->HasFocus());
   EXPECT_FALSE(geolocation_icon->HasFocus());
 

chrome/browser/ui/views/toolbar/browser_actions_container.cc

@@ -437,6 +437,7 @@ int BrowserActionsContainer::GetHeightForWidth(int width) const {
 }
 
 gfx::Size BrowserActionsContainer::GetMinimumSize() const {
+  DCHECK(interactive_);
   return gfx::Size(GetResizeAreaWidth(),
                    toolbar_actions_bar_->GetViewSize().height());
 }

chrome/browser/ui/views/web_apps/web_app_frame_toolbar_browsertest.cc

@@ -127,16 +127,14 @@ IN_PROC_BROWSER_TEST_F(WebAppFrameToolbarBrowserTest, SpaceConstrained) {
   // Resize the WebAppFrameToolbarView just enough to clip out the page action
   // icons (and toolbar contents left of them).
   const int original_toolbar_width = web_app_frame_toolbar()->width();
-  const int new_toolbar_width = toolbar_right_container->width() -
-                                page_action_icon_container->bounds().right();
-  const int new_frame_width =
-      frame_view()->width() - original_toolbar_width + new_toolbar_width;
-
-  // The best cross-platform way to resize the app window is to set the size
-  // directly on the widget.
-  views::Widget* const widget = frame_view()->GetWidget();
-  widget->SetSize(gfx::Size(new_frame_width, frame_view()->height()));
-  widget->GetRootView()->Layout();
+  web_app_frame_toolbar()->SetSize(
+      gfx::Size(toolbar_right_container->width() -
+                    page_action_icon_container->bounds().right(),
+                web_app_frame_toolbar()->height()));
+  frame_view()->SetSize(gfx::Size(frame_view()->width() -
+                                      original_toolbar_width +
+                                      web_app_frame_toolbar()->width(),
+                                  frame_view()->height()));
 
   // The left container (containing Back and Reload) should be hidden.
   EXPECT_FALSE(toolbar_left_container->GetVisible());
@@ -146,9 +144,9 @@ IN_PROC_BROWSER_TEST_F(WebAppFrameToolbarBrowserTest, SpaceConstrained) {
   EXPECT_EQ(window_title->width(), 0);
 #endif
 
-  // The page action icons should be hidden while the app menu button retains
-  // its full width.
-  EXPECT_FALSE(page_action_icon_container->GetVisible());
+  // The page action icons should be clipped to 0 width while the app menu
+  // button retains its full width.
+  EXPECT_EQ(page_action_icon_container->width(), 0);
   EXPECT_EQ(menu_button->width(), original_menu_button_width);
 }
 

chrome/browser/ui/views/web_apps/web_app_frame_toolbar_view.cc

@@ -548,22 +548,15 @@ WebAppFrameToolbarView::ToolbarButtonContainer::ToolbarButtonContainer(
     views::Widget* widget,
     BrowserView* browser_view)
     : browser_view_(browser_view) {
-  views::FlexLayout* const layout =
-      SetLayoutManager(std::make_unique<views::FlexLayout>());
-  layout->SetOrientation(views::LayoutOrientation::kHorizontal)
-      .SetInteriorMargin(gfx::Insets(0, WebAppFrameRightMargin()))
-      .SetDefault(
-          views::kMarginsKey,
-          gfx::Insets(0,
-                      HorizontalPaddingBetweenPageActionsAndAppMenuButtons()))
-      .SetCollapseMargins(true)
-      .SetCrossAxisAlignment(views::LayoutAlignment::kCenter)
-      .SetDefault(views::kFlexBehaviorKey,
-                  views::FlexSpecification(
-                      views::MinimumFlexSizeRule::kPreferredSnapToZero,
-                      views::MaximumFlexSizeRule::kPreferred)
-                      .WithWeight(0))
-      .SetFlexAllocationOrder(views::FlexAllocationOrder::kReverse);
+  views::BoxLayout& layout =
+      *SetLayoutManager(std::make_unique<views::BoxLayout>(
+          views::BoxLayout::Orientation::kHorizontal,
+          gfx::Insets(0, WebAppFrameRightMargin()),
+          HorizontalPaddingBetweenPageActionsAndAppMenuButtons()));
+  // Right align to clip the leftmost items first when not enough space.
+  layout.set_main_axis_alignment(views::BoxLayout::MainAxisAlignment::kEnd);
+  layout.set_cross_axis_alignment(
+      views::BoxLayout::CrossAxisAlignment::kCenter);
 
   const auto* app_controller = browser_view_->browser()->app_controller();
 
@@ -603,20 +596,8 @@ WebAppFrameToolbarView::ToolbarButtonContainer::ToolbarButtonContainer(
                              static_cast<int>(HTCLIENT));
 
   if (base::FeatureList::IsEnabled(features::kExtensionsToolbarMenu)) {
-    // Extensions toolbar area with pinned extensions is lower priority than,
-    // for example, the menu button or other toolbar buttons, and pinned
-    // extensions should hide before other toolbar buttons.
-    constexpr int kLowPriorityFlexOrder = 2;
-    extensions_container_ =
-        AddChildView(std::make_unique<ExtensionsToolbarContainer>(
-            browser_view_->browser(),
-            ExtensionsToolbarContainer::DisplayMode::kCompact));
-    extensions_container_->SetProperty(
-        views::kFlexBehaviorKey,
-        views::FlexSpecification(
-            extensions_container_->animating_layout_manager()
-                ->GetDefaultFlexRule())
-            .WithOrder(kLowPriorityFlexOrder));
+    extensions_container_ = AddChildView(
+        std::make_unique<ExtensionsToolbarContainer>(browser_view_->browser()));
     views::SetHitTestComponent(extensions_container_,
                                static_cast<int>(HTCLIENT));
   } else {

ui/views/layout/animating_layout_manager.cc

@@ -468,6 +468,7 @@ void AnimatingLayoutManager::LayoutImpl() {
         GetMainAxis(orientation(), current_layout_.host_size);
     if (current_main > host_main ||
         (bounds_main && current_main > *bounds_main)) {
+      DCHECK(!bounds_main || *bounds_main >= host_main);
       last_available_host_size_ = available_size;
       ResetLayoutToSize(host_size);
     } else if (available_size != last_available_host_size_) {

ui/views/layout/fill_layout.cc

@@ -42,8 +42,7 @@ ProposedLayout FillLayout::CalculateProposedLayout(
   for (View* child : host_view()->children()) {
     if (ShouldIncludeChild(child)) {
       layout.child_layouts.push_back(
-          ChildLayout{child, child->GetVisible(), contents_bounds,
-                      SizeBounds(contents_bounds.size())});
+          {child, child->GetVisible(), contents_bounds});
     }
   }
 

ui/views/layout/layout_manager_base.cc

@@ -12,29 +12,6 @@
 
 namespace views {
 
-namespace {
-
-// Adjusts |child_available_size| by adding the difference between the host
-// view's size and the size available to it.
-SizeBounds AdjustAvailableSizeForParentAvailableSize(
-    const View* host,
-    const SizeBounds& child_available_size) {
-  if (!host || !host->parent() || child_available_size == SizeBounds())
-    return child_available_size;
-
-  SizeBounds host_additional_size = host->parent()->GetAvailableSize(host);
-  host_additional_size.Enlarge(-host->width(), -host->height());
-  return SizeBounds(
-      child_available_size.width() && host_additional_size.width()
-          ? *child_available_size.width() + *host_additional_size.width()
-          : child_available_size.width(),
-      child_available_size.height() && host_additional_size.height()
-          ? *child_available_size.height() + *host_additional_size.height()
-          : child_available_size.height());
-}
-
-}  // anonymous namespace
-
 LayoutManagerBase::~LayoutManagerBase() = default;
 
 gfx::Size LayoutManagerBase::GetPreferredSize(const View* host) const {
@@ -67,10 +44,8 @@ SizeBounds LayoutManagerBase::GetAvailableSize(const View* host,
   DCHECK_EQ(host_view_, host);
   if (cached_layout_size_) {
     for (const auto& child_layout : cached_layout_.child_layouts)
-      if (child_layout.child_view == view) {
-        return AdjustAvailableSizeForParentAvailableSize(
-            host, child_layout.available_size);
-      }
+      if (child_layout.child_view == view)
+        return child_layout.available_size;
   }
   return SizeBounds();
 }

ui/views/layout/layout_manager_base_unittest.cc

@@ -36,17 +36,9 @@ class TestLayoutManagerBase : public LayoutManagerBase {
     return included;
   }
 
-  void OverrideProposedLayout(const ProposedLayout& forced_layout) {
-    forced_layout_ = forced_layout;
-    InvalidateHost(true);
-  }
-
   // LayoutManagerBase:
   ProposedLayout CalculateProposedLayout(
       const SizeBounds& size_bounds) const override {
-    if (forced_layout_)
-      return *forced_layout_;
-
     ProposedLayout layout;
     layout.host_size.set_width(
         std::max(kMinimumSize.width(),
@@ -56,10 +48,6 @@ class TestLayoutManagerBase : public LayoutManagerBase {
                  size_bounds.height().value_or(kPreferredSize.height())));
     return layout;
   }
-
- private:
-  // If specified, will always return this layout.
-  base::Optional<ProposedLayout> forced_layout_;
 };
 
 // This layout layout lays out included child views in the upper-left of the
@@ -644,12 +632,17 @@ class LayoutManagerBaseAvailableSizeTest : public testing::Test {
         view_->SetLayoutManager(std::make_unique<TestLayoutManagerBase>());
   }
 
+  void SetCachedLayout(const ProposedLayout& layout) {
+    layout_->set_cached_layout_size(layout.host_size);
+    layout_->set_cached_layout(layout);
+  }
+
   View* view() { return view_.get(); }
-  TestLayoutManagerBase* layout() { return layout_; }
+  LayoutManagerBase* layout() { return layout_; }
 
  private:
   std::unique_ptr<View> view_;
-  TestLayoutManagerBase* layout_;
+  LayoutManagerBase* layout_;
 };
 
 TEST_F(LayoutManagerBaseAvailableSizeTest, ReturnsCorrectValues) {
@@ -659,98 +652,13 @@ TEST_F(LayoutManagerBaseAvailableSizeTest, ReturnsCorrectValues) {
   View* const child2 = view()->AddChildView(std::make_unique<View>());
   View not_a_child;
 
-  layout()->OverrideProposedLayout(
-      {{10, 10},
-       {{child1, true, {1, 1, 1, 1}, kChild1Bounds},
-        {child2, true, {2, 2, 2, 2}, kChild2Bounds}}});
-  view()->SizeToPreferredSize();
+  SetCachedLayout({{10, 10},
+                   {{child1, true, {1, 1, 1, 1}, kChild1Bounds},
+                    {child2, true, {2, 2, 2, 2}, kChild2Bounds}}});
 
   EXPECT_EQ(kChild1Bounds, view()->GetAvailableSize(child1));
   EXPECT_EQ(kChild2Bounds, view()->GetAvailableSize(child2));
   EXPECT_EQ(SizeBounds(), view()->GetAvailableSize(&not_a_child));
 }
 
-TEST_F(LayoutManagerBaseAvailableSizeTest, AvailableSizesInNestedValuesAdd) {
-  View* const child = view()->AddChildView(std::make_unique<View>());
-  View* const grandchild = child->AddChildView(std::make_unique<View>());
-  auto* const child_layout =
-      child->SetLayoutManager(std::make_unique<TestLayoutManagerBase>());
-
-  constexpr gfx::Size kViewSize(18, 17);
-  constexpr SizeBounds kChildAvailableSize(16, 15);
-  constexpr gfx::Size kChildSize(13, 12);
-  constexpr SizeBounds kGrandchildAvailableSize(10, 9);
-  constexpr gfx::Size kGrandchildSize(3, 2);
-  layout()->OverrideProposedLayout(
-      {kViewSize, {{child, true, {{3, 3}, kChildSize}, kChildAvailableSize}}});
-  child_layout->OverrideProposedLayout({kChildSize,
-                                        {{grandchild,
-                                          true,
-                                          {{2, 2}, kGrandchildSize},
-                                          kGrandchildAvailableSize}}});
-  view()->SizeToPreferredSize();
-
-  EXPECT_EQ(kChildAvailableSize, view()->GetAvailableSize(child));
-  SizeBounds expected;
-  expected.set_width(*kGrandchildAvailableSize.width() +
-                     *kChildAvailableSize.width() - kChildSize.width());
-  expected.set_height(*kGrandchildAvailableSize.height() +
-                      *kChildAvailableSize.height() - kChildSize.height());
-  EXPECT_EQ(expected, child->GetAvailableSize(grandchild));
-}
-
-TEST_F(LayoutManagerBaseAvailableSizeTest,
-       PartiallySpecifiedAvailableSizesInNestedLayoutsAddPartially) {
-  View* const child = view()->AddChildView(std::make_unique<View>());
-  View* const grandchild = child->AddChildView(std::make_unique<View>());
-  auto* const child_layout =
-      child->SetLayoutManager(std::make_unique<TestLayoutManagerBase>());
-
-  constexpr gfx::Size kViewSize(18, 17);
-  constexpr SizeBounds kChildAvailableSize(16, base::nullopt);
-  constexpr gfx::Size kChildSize(13, 12);
-  constexpr SizeBounds kGrandchildAvailableSize(10, 9);
-  constexpr gfx::Size kGrandchildSize(3, 2);
-  layout()->OverrideProposedLayout(
-      {kViewSize, {{child, true, {{3, 3}, kChildSize}, kChildAvailableSize}}});
-  child_layout->OverrideProposedLayout({kChildSize,
-                                        {{grandchild,
-                                          true,
-                                          {{2, 2}, kGrandchildSize},
-                                          kGrandchildAvailableSize}}});
-  view()->SizeToPreferredSize();
-
-  EXPECT_EQ(kChildAvailableSize, view()->GetAvailableSize(child));
-  SizeBounds expected;
-  expected.set_width(*kGrandchildAvailableSize.width() +
-                     *kChildAvailableSize.width() - kChildSize.width());
-  expected.set_height(*kGrandchildAvailableSize.height());
-  EXPECT_EQ(expected, child->GetAvailableSize(grandchild));
-}
-
-TEST_F(LayoutManagerBaseAvailableSizeTest,
-       MismatchedAvailableSizesInNestedLayoutsDoNotAdd) {
-  View* const child = view()->AddChildView(std::make_unique<View>());
-  View* const grandchild = child->AddChildView(std::make_unique<View>());
-  auto* const child_layout =
-      child->SetLayoutManager(std::make_unique<TestLayoutManagerBase>());
-
-  constexpr gfx::Size kViewSize(18, 17);
-  constexpr SizeBounds kChildAvailableSize(16, base::nullopt);
-  constexpr gfx::Size kChildSize(13, 12);
-  constexpr SizeBounds kGrandchildAvailableSize(base::nullopt, 9);
-  constexpr gfx::Size kGrandchildSize(3, 2);
-  layout()->OverrideProposedLayout(
-      {kViewSize, {{child, true, {{3, 3}, kChildSize}, kChildAvailableSize}}});
-  child_layout->OverrideProposedLayout({kChildSize,
-                                        {{grandchild,
-                                          true,
-                                          {{2, 2}, kGrandchildSize},
-                                          kGrandchildAvailableSize}}});
-  view()->SizeToPreferredSize();
-
-  EXPECT_EQ(kChildAvailableSize, view()->GetAvailableSize(child));
-  EXPECT_EQ(kGrandchildAvailableSize, child->GetAvailableSize(grandchild));
-}
-
 }  // namespace views