Normalize the format of DVLOG(1) messages in window_tree.cc.

Normalize the format of DVLOG(1) messages in window_tree.cc.

BUG=none

Review-Url: https://codereview.chromium.org/2840413002
Cr-Commit-Position: refs/heads/master@{#468091}
Committed: https://chromium.googlesource.com/chromium/src/+/160584b5c1731c3c4f41183c94d5a7e725b99db0

[Elliot Glaysher, 2017-04-27 00:04:12]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 00:04:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 01:33:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-27 01:33:40]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Elliot Glaysher, 2017-04-27 17:11:24]

erg@chromium.org changed reviewers:
+ sky@chromium.org

[Elliot Glaysher, 2017-04-27 17:11:25]

One of the things that drives me batty about the current DVLOGs is that there's
no consistent format to their output. This normalizes the DVLOG(1)s in
window_tree.cc. (I'm not sure what to do about the DVLOG(3)s, but this is better
for the common case.

'[method name] "failed" ([short error string])'

[sky, 2017-04-27 22:11:36]

Yay for consistency. LGTM

[Elliot Glaysher, 2017-04-27 22:14:35]

The CQ bit was checked by erg@chromium.org

[commit-bot: I haz the power, 2017-04-27 22:30:21]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 03:00:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-28 03:00:57]

Failed to apply patch for services/ui/ws/window_tree.cc:
While running git apply --index -3 -p1;
  error: patch failed: services/ui/ws/window_tree.cc:2235
  Falling back to three-way merge...
  Applied patch to 'services/ui/ws/window_tree.cc' with conflicts.
  U services/ui/ws/window_tree.cc

Patch:       services/ui/ws/window_tree.cc
Index: services/ui/ws/window_tree.cc
diff --git a/services/ui/ws/window_tree.cc b/services/ui/ws/window_tree.cc
index
76cca8652d6419b7dd76004cafdf4c11c62443b7..daf5080a5f5edde8765fea7042335b8f460520d1
100644
--- a/services/ui/ws/window_tree.cc
+++ b/services/ui/ws/window_tree.cc
@@ -383,7 +383,7 @@ bool WindowTree::NewWindow(
   DVLOG(3) << "new window client=" << id_
            << " window_id=" << client_window_id.id;
   if (!IsValidIdForNewWindow(client_window_id)) {
-    DVLOG(1) << "new window failed, id is not valid for client";
+    DVLOG(1) << "NewWindow failed (id is not valid for client)";
     return false;
   }
   const WindowId window_id = GenerateNewWindowId();
@@ -407,23 +407,23 @@ bool WindowTree::AddWindow(const ClientWindowId&
parent_id,
            << " client child window_id= " << child_id.id << " global
window_id="
            << (child ? WindowIdToTransportId(child->id()) : 0);
   if (!parent) {
-    DVLOG(1) << "add failed, no parent";
+    DVLOG(1) << "AddWindow failed (no parent)";
     return false;
   }
   if (!child) {
-    DVLOG(1) << "add failed, no child";
+    DVLOG(1) << "AddWindow failed (no child)";
     return false;
   }
   if (child->parent() == parent) {
-    DVLOG(1) << "add failed, already has parent";
+    DVLOG(1) << "AddWindow failed (already has parent)";
     return false;
   }
   if (child->Contains(parent)) {
-    DVLOG(1) << "add failed, child contains parent";
+    DVLOG(1) << "AddWindow failed (child contains parent)";
     return false;
   }
   if (!access_policy_->CanAddWindow(parent, child)) {
-    DVLOG(1) << "add failed, access policy denied add";
+    DVLOG(1) << "AddWindow failed (access denied)";
     return false;
   }
   Operation op(this, window_server_, OperationType::ADD_WINDOW);
@@ -538,11 +538,11 @@ bool WindowTree::SetWindowVisibility(const ClientWindowId&
window_id,
            << " client window_id= " << window_id.id << " global window_id="
            << (window ? WindowIdToTransportId(window->id()) : 0);
   if (!window) {
-    DVLOG(1) << "SetWindowVisibility failure, no window";
+    DVLOG(1) << "SetWindowVisibility failed (no window)";
     return false;
   }
   if (!access_policy_->CanChangeWindowVisibility(window)) {
-    DVLOG(1) << "SetWindowVisibility failure, access policy denied change";
+    DVLOG(1) << "SetWindowVisibility failed (access policy denied change)";
     return false;
   }
   if (window->visible() == visible)
@@ -568,25 +568,25 @@ bool WindowTree::SetFocus(const ClientWindowId& window_id)
{
   ServerWindow* window = GetWindowByClientId(window_id);
   ServerWindow* currently_focused = window_server_->GetFocusedWindow();
   if (!currently_focused && !window) {
-    DVLOG(1) << "SetFocus failure, no focused window to clear.";
+    DVLOG(1) << "SetFocus failed (no focused window to clear)";
     return false;
   }
 
   Display* display = GetDisplay(window);
   if (window && (!display || !window->can_focus() || !window->IsDrawn())) {
-    DVLOG(1) << "SetFocus failure, window cannot be focused.";
+    DVLOG(1) << "SetFocus failed (window cannot be focused)";
     return false;
   }
 
   if (!access_policy_->CanSetFocus(window)) {
-    DVLOG(1) << "SetFocus failure, blocked by access policy.";
+    DVLOG(1) << "SetFocus failed (blocked by access policy)";
     return false;
   }
 
   Operation op(this, window_server_, OperationType::SET_FOCUS);
   bool success = window_server_->SetFocusedWindow(window);
   if (!success) {
-    DVLOG(1) << "SetFocus failure, could not SetFocusedWindow.";
+    DVLOG(1) << "SetFocus failed (could not SetFocusedWindow)";
   }
   return success;
 }
@@ -669,12 +669,14 @@ void WindowTree::AddActivationParent(const ClientWindowId&
window_id) {
   ServerWindow* window = GetWindowByClientId(window_id);
   if (window) {
     Display* display = GetDisplay(window);
-    if (display)
+    if (display) {
       display->AddActivationParent(window);
-    else
-      DVLOG(1) << "AddActivationParent window not associated with display";
+    } else {
+      DVLOG(1) << "AddActivationParent failed "
+               << "(window not associated with display)";
+    }
   } else {
-    DVLOG(1) << "AddActivationParent supplied invalid window id";
+    DVLOG(1) << "AddActivationParent failed (invalid window id)";
   }
 }
 
@@ -1046,26 +1048,26 @@ bool WindowTree::CanReorderWindow(const ServerWindow*
window,
                                   const ServerWindow* relative_window,
                                   mojom::OrderDirection direction) const {
   if (!window) {
-    DVLOG(1) << "reorder failing: invalid window";
+    DVLOG(1) << "CanReorderWindow failed (invalid window)";
     return false;
   }
   if (!relative_window) {
-    DVLOG(1) << "reorder failing: invalid relative window";
+    DVLOG(1) << "CanReorderWindow failed (invalid relative window)";
     return false;
   }
 
   if (!window->parent()) {
-    DVLOG(1) << "reorder failing: no parent";
+    DVLOG(1) << "CanReorderWindow failed (no parent)";
     return false;
   }
 
   if (window->parent() != relative_window->parent()) {
-    DVLOG(1) << "reorder failing: parents differ";
+    DVLOG(1) << "CanReorderWindow failed (parents differ)";
     return false;
   }
 
   if (!access_policy_->CanReorderWindow(window, relative_window, direction)) {
-    DVLOG(1) << "reorder failing: access policy denied";
+    DVLOG(1) << "CanReorderWindow failed (access policy denied)";
     return false;
   }
 
@@ -1077,7 +1079,7 @@ bool WindowTree::CanReorderWindow(const ServerWindow*
window,
       children.begin();
   if ((direction == mojom::OrderDirection::ABOVE && child_i == target_i + 1) ||
       (direction == mojom::OrderDirection::BELOW && child_i + 1 == target_i)) {
-    DVLOG(1) << "reorder failing: already in position";
+    DVLOG(1) << "CanReorderWindow failed (already in position)";
     return false;
   }
 
@@ -1423,11 +1425,14 @@ void WindowTree::RemoveWindowFromParent(uint32_t
change_id, Id window_id) {
            << " client window_id= " << window_id << " global window_id="
            << (window ? WindowIdToTransportId(window->id()) : 0);
   if (!window) {
-    DVLOG(1) << "remove failing, invalid window id=" << change_id;
+    DVLOG(1) << "RemoveWindowFromParent failed (invalid window id=" <<
change_id
+             << ")";
   } else if (!window->parent()) {
-    DVLOG(1) << "remove failing, no parent id=" << change_id;
+    DVLOG(1) << "RemoveWindowFromParent failed (no parent id=" << change_id
+             << ")";
   } else if (!access_policy_->CanRemoveWindowFromParent(window)) {
-    DVLOG(1) << "remove failing, access policy disallowed id=" << change_id;
+    DVLOG(1) << "RemoveWindowFromParent failed (access policy disallowed id="
+             << change_id << ")";
   } else {
     success = true;
     Operation op(this, window_server_,
@@ -1549,13 +1554,20 @@ void WindowTree::SetWindowBounds(
            << " global window_id="
            << (window ? WindowIdToTransportId(window->id()) : 0)
            << " bounds=" << bounds.ToString();
+
+  if (!window) {
+    DVLOG(1) << "SetWindowBounds failed (invalid window id)";
+    client()->OnChangeCompleted(change_id, false);
+    return;
+  }
+
   // Only the owner of the window can change the bounds.
-  bool success = window && access_policy_->CanSetWindowBounds(window);
+  bool success = access_policy_->CanSetWindowBounds(window);
   if (success) {
     Operation op(this, window_server_, OperationType::SET_WINDOW_BOUNDS);
     window->SetBounds(bounds, local_surface_id);
   } else {
-    DVLOG(1) << "Failed to set bounds on window.";
+    DVLOG(1) << "SetWindowBounds failed (access denied)";
   }
   client()->OnChangeCompleted(change_id, success);
 }
@@ -1610,10 +1622,14 @@ void WindowTree::AttachCompositorFrameSink(
     cc::mojom::MojoCompositorFrameSinkClientPtr client) {
   ServerWindow* window =
       GetWindowByClientId(ClientWindowId(transport_window_id));
-  const bool success =
-      window && access_policy_->CanSetWindowCompositorFrameSink(window);
+  if (!window) {
+    DVLOG(1) << "AttachCompositorFrameSink failed (invalid window id)";
+    return;
+  }
+
+  const bool success = access_policy_->CanSetWindowCompositorFrameSink(window);
   if (!success) {
-    DVLOG(1) << "request to AttachCompositorFrameSink failed";
+    DVLOG(1) << "AttachCompositorFrameSink failed (access denied)";
     return;
   }
   window->CreateCompositorFrameSink(std::move(compositor_frame_sink),
@@ -1694,11 +1710,11 @@ void WindowTree::SetClientArea(Id transport_window_id,
            << " insets=" << insets.top() << " " << insets.left() << " "
            << insets.bottom() << " " << insets.right();
   if (!window) {
-    DVLOG(1) << "SetClientArea failed, no window";
+    DVLOG(1) << "SetClientArea failed (invalid window id)";
     return;
   }
   if (!access_policy_->CanSetClientArea(window)) {
-    DVLOG(1) << "SetClientArea failed, access denied";
+    DVLOG(1) << "SetClientArea failed (access denied)";
     return;
   }
 
@@ -1711,8 +1727,13 @@ void WindowTree::SetHitTestMask(Id transport_window_id,
                                 const base::Optional<gfx::Rect>& mask) {
   ServerWindow* window =
       GetWindowByClientId(ClientWindowId(transport_window_id));
-  if (!window || !access_policy_->CanSetHitTestMask(window)) {
-    DVLOG(1) << "SetHitTestMask failed";
+  if (!window) {
+    DVLOG(1) << "SetHitTestMask failed (invalid window id)";
+    return;
+  }
+
+  if (!access_policy_->CanSetHitTestMask(window)) {
+    DVLOG(1) << "SetHitTestMask failed (access denied)";
     return;
   }
 
@@ -1724,8 +1745,13 @@ void WindowTree::SetHitTestMas…
(message too large)

[Elliot Glaysher, 2017-04-28 17:27:27]

The CQ bit was checked by erg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 17:28:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Elliot Glaysher, 2017-04-28 18:24:02]

The CQ bit was unchecked by erg@chromium.org

[Elliot Glaysher, 2017-04-28 18:24:07]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2017-04-28 18:24:08]

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/2840413002/#ps20001
(title: "Merge with tot")

[commit-bot: I haz the power, 2017-04-28 18:25:16]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 19:27:54]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493403847706160,
"parent_rev": "6fe9396d6935924e6bff4345269293ed3ffd430e", "commit_rev":
"160584b5c1731c3c4f41183c94d5a7e725b99db0"}

[commit-bot: I haz the power, 2017-04-28 19:28:31]

Description was changed from

==========
Normalize the format of DVLOG(1) messages in window_tree.cc.

BUG=none
==========

to

==========
Normalize the format of DVLOG(1) messages in window_tree.cc.

BUG=none

Review-Url: https://codereview.chromium.org/2840413002
Cr-Commit-Position: refs/heads/master@{#468091}
Committed:
https://chromium.googlesource.com/chromium/src/+/160584b5c1731c3c4f41183c94d5...
==========

[commit-bot: I haz the power, 2017-04-28 19:28:32]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/160584b5c1731c3c4f41183c94d5...