VR: System indicator rework.

chrome/browser/android/vr_shell/ui_scene_manager.cc

Index: chrome/browser/android/vr_shell/ui_scene_manager.cc
diff --git a/chrome/browser/android/vr_shell/ui_scene_manager.cc b/chrome/browser/android/vr_shell/ui_scene_manager.cc
index 69ffdd55c3c942698bd693095fdfeb7639c8e0ab..e9bae22b41d29edd667f2205c565a64cf41d7e3e 100644
--- a/chrome/browser/android/vr_shell/ui_scene_manager.cc
+++ b/chrome/browser/android/vr_shell/ui_scene_manager.cc
@@ -9,23 +9,23 @@
 #include "chrome/browser/android/vr_shell/textures/close_button_texture.h"
 #include "chrome/browser/android/vr_shell/textures/ui_texture.h"
 #include "chrome/browser/android/vr_shell/ui_browser_interface.h"
-#include "chrome/browser/android/vr_shell/ui_elements/audio_capture_indicator.h"
 #include "chrome/browser/android/vr_shell/ui_elements/button.h"
 #include "chrome/browser/android/vr_shell/ui_elements/exit_prompt.h"
 #include "chrome/browser/android/vr_shell/ui_elements/exit_prompt_backplane.h"
 #include "chrome/browser/android/vr_shell/ui_elements/exit_warning.h"
 #include "chrome/browser/android/vr_shell/ui_elements/loading_indicator.h"
-#include "chrome/browser/android/vr_shell/ui_elements/location_access_indicator.h"
 #include "chrome/browser/android/vr_shell/ui_elements/permanent_security_warning.h"
-#include "chrome/browser/android/vr_shell/ui_elements/screen_capture_indicator.h"
 #include "chrome/browser/android/vr_shell/ui_elements/screen_dimmer.h"
+#include "chrome/browser/android/vr_shell/ui_elements/system_indicator.h"
 #include "chrome/browser/android/vr_shell/ui_elements/transient_security_warning.h"
 #include "chrome/browser/android/vr_shell/ui_elements/transient_url_bar.h"
 #include "chrome/browser/android/vr_shell/ui_elements/ui_element.h"
 #include "chrome/browser/android/vr_shell/ui_elements/ui_element_debug_id.h"
 #include "chrome/browser/android/vr_shell/ui_elements/url_bar.h"
-#include "chrome/browser/android/vr_shell/ui_elements/video_capture_indicator.h"
 #include "chrome/browser/android/vr_shell/ui_scene.h"
+#include "chrome/grit/generated_resources.h"
+#include "components/vector_icons/vector_icons.h"
+#include "ui/vector_icons/vector_icons.h"
 
 namespace vr_shell {
 
@@ -69,17 +69,10 @@ static constexpr float kUrlBarHeight = kUrlBarHeightDMM * kUrlBarDistance;
 static constexpr float kUrlBarVerticalOffset = -0.516 * kUrlBarDistance;
 static constexpr float kUrlBarRotationRad = -0.175;
 
-static constexpr float kIndicatorContentDistance = 0.1;
-static constexpr float kAudioCaptureIndicatorWidth = 0.5;
-static constexpr float kVideoCaptureIndicatorWidth = 0.5;
-static constexpr float kScreenCaptureIndicatorWidth = 0.4;
-static constexpr float kLocationIndicatorWidth = 0.088;
-
-static constexpr float kCaptureIndicatorsVerticalOffset = 0.1;
-static constexpr float kAudioCaptureHorizontalOffset = -0.6;
-static constexpr float kVideoCaptureHorizontalOffset = 0;
-static constexpr float kScreenCaptureHorizontalOffset = 0.6;
-static constexpr float kLocationAccessHorizontalOffset = 1.0;
+static constexpr float kIndicatorHeight = 0.0625;

[amp, 2017/06/22 18:34:43]

Does this include an implicit distance in it already?  Kind of related to my
question about ZOffset below.

+static constexpr float kIndicatorGap = 0.05;
+static constexpr float kIndicatorYOffset = 0.1;
+static constexpr float kIndicatorZOffset = 0.1;

[amp, 2017/06/22 18:34:43]

We call it Distance everywhere else.  Switching to ZOffset threw me off.

If it varies depending on number of elements I guess it's fine, but it looks
like all of them use the same value.

I like the changes to the other values though as it makes it clearer that they
are relative rather than absolute values.

[cjgrant, 2017/06/22 19:16:33]

I'm not sure what actions are being suggested here. :)  Our distance constants
specify absolute Z distance, but these offsets are relative to the parent object
(the content quad).  It seems like calling them distance would be misleading.  

What do you mean about varying with the number of elements?  The Y and Z apply
to all the indicators, and X position gets computed based on the number of
indicators, and the gap between them.

Let me know if I'm missing something!

[amp, 2017/06/22 20:36:22]

Thanks for the explanation.  I was used to seeing 'distance' and other word
based dimensions, and Z offset didn't immediately make sense.  Using it as an
offset from the anchor element sounds like a good use, although maybe having
'anchor[dimension]Offset' as the name would have prevented my confusion.  Still,
we don't use Z anywhere else for the constants.  It's not a big deal, but maybe
we should use distanceOffset (or convert the 'distance' and 'width' etc names to
use something like Z, X, etc).  

My varying by number comment was just referring to that computation of the X
position that you mention.

For the height comment, how do dmm values connect in with dynamically (anchored
to a different element) calculated distances?  When the content gets moved back
for fullscreen mode should the height/size of the indicators change as well? 
(I'm not sure if we have decided if the indicators show up in fullscreen, but
asking the question assuming they do).

[cjgrant, 2017/06/22 20:58:11]

- I'll update the naming to Distance and Vertical to match others. SGTM.
- On DMMs, bottom line, we are broken.  Same as we assign static texture sizes
to all these elements instead of computing what the actual texture size should
be.  This is one big ball of post-BBB fun to solve. :)

 
 static constexpr float kTransientUrlBarDistance = 1.4;
 static constexpr float kTransientUrlBarWidth =
@@ -212,57 +205,37 @@ void UiSceneManager::CreateSecurityWarnings() {
 void UiSceneManager::CreateSystemIndicators() {
   std::unique_ptr<UiElement> element;
 
-  element = base::MakeUnique<AudioCaptureIndicator>(512);
-  element->set_debug_id(kAudioCaptureIndicator);
-  element->set_id(AllocateId());
-  element->set_translation({kAudioCaptureHorizontalOffset,
-                            kCaptureIndicatorsVerticalOffset,
-                            kIndicatorContentDistance});
-  element->set_parent_id(main_content_->id());
-  element->set_y_anchoring(YAnchoring::YTOP);
-  element->set_size({kAudioCaptureIndicatorWidth, 0, 1});
-  element->set_visible(false);
-  audio_capture_indicator_ = element.get();
-  scene_->AddUiElement(std::move(element));
-
-  element = base::MakeUnique<LocationAccessIndicator>(250);
-  element->set_debug_id(kLocationAccessIndicator);
-  element->set_id(AllocateId());
-  element->set_translation({kLocationAccessHorizontalOffset,
-                            kCaptureIndicatorsVerticalOffset,
-                            kIndicatorContentDistance});
-  element->set_size({kLocationIndicatorWidth, 0, 1});
-  element->set_parent_id(main_content_->id());
-  element->set_y_anchoring(YAnchoring::YTOP);
-  element->set_visible(false);
-  location_access_indicator_ = element.get();
-  scene_->AddUiElement(std::move(element));
-
-  element = base::MakeUnique<VideoCaptureIndicator>(512);
-  element->set_debug_id(kVideoCaptureIndicator);
-  element->set_id(AllocateId());
-  element->set_translation({kVideoCaptureHorizontalOffset,
-                            kCaptureIndicatorsVerticalOffset,
-                            kIndicatorContentDistance});
-  element->set_parent_id(main_content_->id());
-  element->set_y_anchoring(YAnchoring::YTOP);
-  element->set_size({kVideoCaptureIndicatorWidth, 0, 1});
-  element->set_visible(false);
-  video_capture_indicator_ = element.get();
-  scene_->AddUiElement(std::move(element));
+  struct Indicator {
+    UiElement** element;
+    UiElementDebugId debug_id;
+    const gfx::VectorIcon& icon;
+    int resource_string;
+  };
+  const std::vector<Indicator> indicators = {
+      {&audio_capture_indicator_, kAudioCaptureIndicator, ui::kMicrophoneIcon,
+       IDS_AUDIO_CALL_NOTIFICATION_TEXT_2},
+      {&video_capture_indicator_, kVideoCaptureIndicator, ui::kVideocamIcon,
+       IDS_VIDEO_CALL_NOTIFICATION_TEXT_2},
+      {&screen_capture_indicator_, kScreenCaptureIndicator,
+       vector_icons::kScreenShareIcon, IDS_SCREEN_CAPTURE_NOTIFICATION_TEXT_2},
+      {&location_access_indicator_, kLocationAccessIndicator,
+       ui::kLocationOnIcon, 0},
+  };
+
+  for (const auto& indicator : indicators) {
+    element = base::MakeUnique<SystemIndicator>(
+        512, kIndicatorHeight, indicator.icon, indicator.resource_string);

[asimjour1, 2017/06/22 14:46:00]

I'm not sure if I get this part. Do we use 512 for all textures? I think we
should add this number to struct Indicator.

[cjgrant, 2017/06/22 15:51:39]

We use 512 for many textures, but the number should be based on how large the
element will actually appear in the scene.  For these indicators however, the
texture actually specifies narrower textures for icon-only elements.  This lets
each indicator choose its ideal size, based on a consistent maximum size.  If
anything, I would change the meaning to be the height in pixels, rather than
width, and replace the meters-based height I'm passing in.  What do you think of
that?

[asimjour1, 2017/06/22 19:18:35]

Please use a constant for 512.

[cjgrant, 2017/06/22 20:58:11]

As mentioned offline, all our sizes are hard-coded, because they're wrong and
need fixing.  Making this a constant furthers the illusion that it's okay. :)

+    element->set_debug_id(indicator.debug_id);
+    element->set_id(AllocateId());
+    element->set_parent_id(main_content_->id());
+    element->set_y_anchoring(YAnchoring::YTOP);
+    element->set_visible(false);
+    *(indicator.element) = element.get();
+    system_indicators_.push_back(element.get());
+    scene_->AddUiElement(std::move(element));
+  }
 
-  element = base::MakeUnique<ScreenCaptureIndicator>(512);
-  element->set_debug_id(kScreenCaptureIndicator);
-  element->set_id(AllocateId());
-  element->set_translation({kScreenCaptureHorizontalOffset,
-                            kCaptureIndicatorsVerticalOffset,
-                            kIndicatorContentDistance});
-  element->set_parent_id(main_content_->id());
-  element->set_y_anchoring(YAnchoring::YTOP);
-  element->set_size({kScreenCaptureIndicatorWidth, 0, 1});
-  element->set_visible(false);
-  screen_capture_indicator_ = element.get();
-  scene_->AddUiElement(std::move(element));
+  ConfigureIndicators();
 }
 
 void UiSceneManager::CreateContentQuad() {
@@ -542,6 +515,13 @@ void UiSceneManager::SetIncognito(bool incognito) {
   ConfigureScene();
 }
 
+void UiSceneManager::OnGLInitialized() {
+  scene_->OnGLInitialized();
+
+  // Indicators don't know their position until they've rendered themselves.
+  ConfigureIndicators();
+}
+
 void UiSceneManager::OnAppButtonClicked() {
   // App button click exits the WebVR presentation and fullscreen.
   browser_->ExitPresent();
@@ -576,6 +556,23 @@ void UiSceneManager::ConfigureIndicators() {
   video_capture_indicator_->set_visible(!web_vr_mode_ && video_capturing_);
   screen_capture_indicator_->set_visible(!web_vr_mode_ && screen_capturing_);
   location_access_indicator_->set_visible(!web_vr_mode_ && location_access_);
+
+  // Position elements dynamically relative to each other, based on which
+  // indicators are showing, and how big each one is.
+  float total_width = kIndicatorGap * (system_indicators_.size() - 1);
+  for (const UiElement* indicator : system_indicators_) {
+    if (indicator->visible())
+      total_width += indicator->size().x();
+  }
+  float x_position = -total_width / 2;
+  for (UiElement* indicator : system_indicators_) {
+    if (!indicator->visible())
+      continue;
+    float width = indicator->size().x();
+    indicator->set_translation(
+        {x_position + width / 2, kIndicatorYOffset, kIndicatorZOffset});
+    x_position += width + kIndicatorGap;
+  }
 }
 
 void UiSceneManager::OnSecurityWarningTimer() {