Revert "Set window properties when creating widget, not after."

This reverts commit f93b22c3.

Reason for revert: It causes crashes. Please see crbug.com/1160507
Bug: 1160507

Original change's description:
> Set window properties when creating widget, not after.
>
> Optimize ArcOverlayManager so that it observe arc windows only.
>
> Bug: None
>
> Change-Id: Ic6c5f43fea647369eac092c1c4c4d68aed7d8ce9
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2578377
> Commit-Queue: Mitsuru Oshima (slow: gardener) <oshima@chromium.org>
> Reviewed-by: Yuichiro Hanada <yhanada@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#838281}

TBR=oshima@chromium.org,lpique@chromium.org,yhanada@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: None
Change-Id: I11338ea9ab9fd72aa3d0b0a14b0b47a51edb1425
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2600417Reviewed-by: Yuichiro Hanada <yhanada@chromium.org>
Commit-Queue: Yuichiro Hanada <yhanada@chromium.org>
Cr-Commit-Position: refs/heads/master@{#838772}

ash/public/cpp/external_arc/BUILD.gn

@@ -43,7 +43,6 @@ static_library("external_arc") {
     "//base",
     "//chromeos/constants",
     "//components/account_id",
-    "//components/arc:arc_base_utils",
     "//components/arc:arc_metrics_constants",
     "//components/arc:connection_holder",
     "//components/arc/mojom:notifications",

ash/public/cpp/external_arc/overlay/DEPS

-include_rules = [
-  "+components/arc/arc_util.h",
-]

ash/public/cpp/external_arc/overlay/arc_overlay_manager.cc

@@ -6,7 +6,6 @@
 
 #include "ash/public/cpp/external_arc/overlay/arc_overlay_controller_impl.h"
 #include "base/logging.h"
-#include "components/arc/arc_util.h"
 #include "components/exo/shell_surface_base.h"
 #include "components/exo/shell_surface_util.h"
 #include "components/exo/surface.h"
@@ -67,38 +66,78 @@ void ArcOverlayManager::DeregisterHostWindow(const std::string& overlay_token) {
 }
 
 void ArcOverlayManager::OnWindowInitialized(aura::Window* window) {
-  // Ignore windows that are container (no delegate), or non arc window.
-  if (!window->delegate() || !arc::IsArcAppWindow(window))
+  // Ignore windows that do not have a delegate set.
+  if (!window->delegate())
     return;
 
-  window_observations_.AddObservation(window);
+  // We only ever observe the most recent window being created
+  unknown_window_observation_.Reset();
+  unknown_window_observation_.Observe(window);
 }
 
 void ArcOverlayManager::OnWindowDestroying(aura::Window* window) {
-  window_observations_.RemoveObservation(window);
+  if (unknown_window_observation_.IsObservingSource(window))
+    unknown_window_observation_.Reset();
+
+  if (overlay_window_observations_.IsObservingSource(window))
+    overlay_window_observations_.RemoveObservation(window);
+}
+
+void ArcOverlayManager::OnWindowPropertyChanged(aura::Window* window,
+                                                const void* key,
+                                                intptr_t old) {
+  // We only care about property changes on the single unknown window.
+  // (We also are observing other windows via overlay_window_observations_)
+  if (!unknown_window_observation_.IsObservingSource(window))
+    return;
+
+  // exo::ShellSurfaceBase sets this key soon after creating the window
+  if (!exo::IsShellMainSurfaceKey(key))
+    return;
+
+  // It may still be of interest as an overlay, but we don't need to observe it
+  // as an unknown window.
+  unknown_window_observation_.Reset();
+
+  // If this isn't actually a variant of a exo::ShellSurfaceBase, it is not an
+  // overlay candidate.
+  auto* shell_surface_base = exo::GetShellSurfaceBaseForWindow(window);
+  if (!shell_surface_base)
+    return;
+
+  auto* shell_root_surface = shell_surface_base->root_surface();
+  DCHECK(shell_root_surface);
+
+  // If the client_surface_id doesn't have a particular prefix, it is not an
+  // overlay candidate.
+  std::string client_surface_id = shell_root_surface->GetClientSurfaceId();
+  if (!base::StartsWith(client_surface_id, kBillingIdPrefix))
+    return;
+
+  // This window seems to be an overlay candidate. Continue observing it as one
+  // until it is ready. exo::ShellSurfaceBase is still setting it up.
+  overlay_window_observations_.AddObservation(window);
 }
 
 void ArcOverlayManager::OnWindowVisibilityChanged(aura::Window* window,
                                                   bool visible) {
+  // For this event, we only care about windows that are potential overlays.
+  if (!overlay_window_observations_.IsObservingSource(window))
+    return;
+
   // We only care about windows that are now visible.
   if (!visible)
     return;
 
   // We do not need to keep observing the window.
-  DCHECK(window_observations_.IsObservingSource(window));
-  window_observations_.RemoveObservation(window);
+  overlay_window_observations_.RemoveObservation(window);
 
   auto* shell_surface_base = exo::GetShellSurfaceBaseForWindow(window);
   DCHECK(shell_surface_base);
   auto* shell_root_surface = shell_surface_base->root_surface();
   DCHECK(shell_root_surface);
 
-  // If the client_surface_id doesn't have a particular prefix, it is not an
-  // overlay candidate.
   std::string client_surface_id = shell_root_surface->GetClientSurfaceId();
-  if (!base::StartsWith(client_surface_id, kBillingIdPrefix))
-    return;
-
   std::string overlay_token =
       client_surface_id.substr(strlen(kBillingIdPrefix));
 

ash/public/cpp/external_arc/overlay/arc_overlay_manager.h

@@ -63,6 +63,9 @@ class ASH_PUBLIC_EXPORT ArcOverlayManager : public aura::EnvObserver,
 
   // aura::WindowObserver:
   void OnWindowDestroying(aura::Window* window) override;
+  void OnWindowPropertyChanged(aura::Window* window,
+                               const void* key,
+                               intptr_t old) override;
   void OnWindowVisibilityChanged(aura::Window* window, bool visible) override;
 
  private:
@@ -75,10 +78,16 @@ class ASH_PUBLIC_EXPORT ArcOverlayManager : public aura::EnvObserver,
 
   base::ScopedObservation<aura::Env, aura::EnvObserver> env_observer_{this};
 
-  // This tracks newly created arc windows until they're being shown, or
-  // destoryed.
+  // This tracks a single newly created window until we get a confirmation that
+  // it is an exo::ShellSurfaceBase with the right settings to be an overlay
+  // (which should happen immediately after creation), or until another new
+  // window is created.
+  base::ScopedObservation<aura::Window, aura::WindowObserver>
+      unknown_window_observation_{this};
+
+  // This tracks all the overlay candidates until they are actually ready
   base::ScopedMultiSourceObservation<aura::Window, aura::WindowObserver>
-      window_observations_{this};
+      overlay_window_observations_{this};
 };
 
 }  // namespace ash

components/exo/shell_surface_base.cc

@@ -917,11 +917,9 @@ void ShellSurfaceBase::CreateShellSurfaceWidget(
       startup_id_ ? *startup_id_ : std::string(),
       /*for_creation=*/true, params.init_properties_container);
 
-  SetShellApplicationId(&params.init_properties_container, application_id_);
-  SetShellMainSurface(&params.init_properties_container, root_surface());
-  SetShellStartupId(&params.init_properties_container, startup_id_);
-
   bool activatable = activatable_;
+  if (container_ == ash::kShellWindowId_SystemModalContainer)
+    activatable &= HasHitTestRegion();
 
   // ShellSurfaces in system modal container are only activatable if input
   // region is non-empty. See OnCommitSurface() for more details.
@@ -944,6 +942,9 @@ void ShellSurfaceBase::CreateShellSurfaceWidget(
   window->SetEventTargetingPolicy(
       aura::EventTargetingPolicy::kTargetAndDescendants);
   InstallCustomWindowTargeter();
+  SetShellApplicationId(window, application_id_);
+  SetShellStartupId(window, startup_id_);
+  SetShellMainSurface(window, root_surface());
 
   // Start tracking changes to window bounds and window state.
   window->AddObserver(this);

components/exo/shell_surface_util.cc

@@ -78,28 +78,28 @@ aura::WindowTargeter* FindTargeter(ui::EventTarget* target) {
 
 }  // namespace
 
-void SetShellApplicationId(ui::PropertyHandler* property_handler,
+void SetShellApplicationId(aura::Window* window,
                            const base::Optional<std::string>& id) {
   TRACE_EVENT1("exo", "SetApplicationId", "application_id", id ? *id : "null");
 
   if (id)
-    property_handler->SetProperty(kApplicationIdKey, *id);
+    window->SetProperty(kApplicationIdKey, *id);
   else
-    property_handler->ClearProperty(kApplicationIdKey);
+    window->ClearProperty(kApplicationIdKey);
 }
 
-const std::string* GetShellApplicationId(const aura::Window* property_handler) {
-  return property_handler->GetProperty(kApplicationIdKey);
+const std::string* GetShellApplicationId(const aura::Window* window) {
+  return window->GetProperty(kApplicationIdKey);
 }
 
-void SetShellStartupId(ui::PropertyHandler* property_handler,
+void SetShellStartupId(aura::Window* window,
                        const base::Optional<std::string>& id) {
   TRACE_EVENT1("exo", "SetStartupId", "startup_id", id ? *id : "null");
 
   if (id)
-    property_handler->SetProperty(kStartupIdKey, *id);
+    window->SetProperty(kStartupIdKey, *id);
   else
-    property_handler->ClearProperty(kStartupIdKey);
+    window->ClearProperty(kStartupIdKey);
 }
 
 const std::string* GetShellStartupId(aura::Window* window) {
@@ -140,9 +140,8 @@ bool IsShellMainSurfaceKey(const void* key) {
   return kMainSurfaceKey == key;
 }
 
-void SetShellMainSurface(ui::PropertyHandler* property_handler,
-                         Surface* surface) {
-  property_handler->SetProperty(kMainSurfaceKey, surface);
+void SetShellMainSurface(aura::Window* window, Surface* surface) {
+  window->SetProperty(kMainSurfaceKey, surface);
 }
 
 Surface* GetShellMainSurface(const aura::Window* window) {

components/exo/shell_surface_util.h

@@ -10,10 +10,6 @@
 
 #include "base/optional.h"
 
-namespace ui {
-class PropertyHandler;
-}
-
 namespace aura {
 class Window;
 }
@@ -33,15 +29,21 @@ class Permission;
 class Surface;
 class ShellSurfaceBase;
 
-// Sets the application ID to the property_handler. The application ID
-// identifies the general class of applications to which the window belongs.
-void SetShellApplicationId(ui::PropertyHandler* property_handler,
+// Sets the application ID for the window. The application ID identifies the
+// general class of applications to which the window belongs.
+void SetShellApplicationId(aura::Window* window,
                            const base::Optional<std::string>& id);
 const std::string* GetShellApplicationId(const aura::Window* window);
 
-// Sets the startup ID to the property handler. The startup ID identifies the
+// Sets ARC app type for the provided |window|.
+void SetArcAppType(aura::Window* window);
+
+// Sets Lacros app type for the provided |window|.
+void SetLacrosAppType(aura::Window* window);
+
+// Sets the startup ID for the window. The startup ID identifies the
 // application using startup notification protocol.
-void SetShellStartupId(ui::PropertyHandler* property_handler,
+void SetShellStartupId(aura::Window* window,
                        const base::Optional<std::string>& id);
 const std::string* GetShellStartupId(aura::Window* window);
 
@@ -60,9 +62,8 @@ const base::Optional<int32_t> GetShellClientAccessibilityId(
 // Returns true if the given key is the shell main surface key
 bool IsShellMainSurfaceKey(const void* key);
 
-// Sets the main surface to the property handler.
-void SetShellMainSurface(ui::PropertyHandler* property_handler,
-                         Surface* surface);
+// Sets the main surface for the window.
+void SetShellMainSurface(aura::Window* window, Surface* surface);
 
 // Returns the main Surface instance or nullptr if it is not set.
 // |window| must not be nullptr.