[WebIntents, Gtk] "Waiting for Suggestion" dialog

chrome/browser/ui/intents/web_intent_picker_controller.cc

Index: chrome/browser/ui/intents/web_intent_picker_controller.cc
diff --git a/chrome/browser/ui/intents/web_intent_picker_controller.cc b/chrome/browser/ui/intents/web_intent_picker_controller.cc
index 38eb5f2e0019f76a62e0aa1adc8e8e561609b313..ceefd93a57fb32328ce39c1481281e4a19f50a79 100644
--- a/chrome/browser/ui/intents/web_intent_picker_controller.cc
+++ b/chrome/browser/ui/intents/web_intent_picker_controller.cc
@@ -24,6 +24,7 @@
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_navigator.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
+#include "chrome/browser/ui/constrained_window_tab_helper.h"
 #include "chrome/browser/ui/intents/web_intent_picker.h"
 #include "chrome/browser/ui/intents/web_intent_picker_model.h"
 #include "chrome/browser/ui/tab_contents/tab_contents.h"
@@ -60,6 +61,13 @@ const char kPickActionURL[] = "http://webintents.org/pick";
 const char kSubscribeActionURL[] = "http://webintents.org/subscribe";
 const char kSaveActionURL[] = "http://webintents.org/save";
 
+// Maximum amount of time to delay displaying dialog while waiting for data.
+const int kMaxHiddenSetupTimeMs = 200;
+
+// Minimum amount of time to show waiting dialog, if it is shown.
+const int kMinThrobberDisplayTimeMs = 2000;
+
+
 // Gets the favicon service for the profile in |tab_contents|.
 FaviconService* GetFaviconService(TabContents* tab_contents) {
   return tab_contents->profile()->GetFaviconService(Profile::EXPLICIT_ACCESS);
@@ -161,17 +169,20 @@ class SourceWindowObserver : content::WebContentsObserver {
 
 WebIntentPickerController::WebIntentPickerController(
     TabContents* tab_contents)
-    : tab_contents_(tab_contents),
+    : dialog_state_(kPickerHidden),
+      tab_contents_(tab_contents),
       picker_(NULL),
       picker_model_(new WebIntentPickerModel()),
       pending_async_count_(0),
       pending_registry_calls_count_(0),
+      pending_cws_request_(false),
       picker_shown_(false),
       window_disposition_source_(NULL),
       source_intents_dispatcher_(NULL),
       intents_dispatcher_(NULL),
       service_tab_(NULL),
-      weak_ptr_factory_(this) {
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)),
+      ALLOW_THIS_IN_INITIALIZER_LIST(timer_factory_(this)) {
   content::NavigationController* controller =
       &tab_contents->web_contents()->GetController();
   registrar_.Add(this, content::NOTIFICATION_LOAD_START,
@@ -191,9 +202,13 @@ void WebIntentPickerController::SetIntentsDispatcher(
       base::Bind(&WebIntentPickerController::OnSendReturnMessage,
                  weak_ptr_factory_.GetWeakPtr()));
 }
-
 void WebIntentPickerController::ShowDialog(const string16& action,
                                            const string16& type) {
+
+  // As soon as the dialog is requested, block all input events
+  // on the original tab.
+  tab_contents_->constrained_window_tab_helper()->BlockTabContent(true);
+
   // Only show a picker once.
   // TODO(gbillock): There's a hole potentially admitting multiple
   // in-flight dispatches since we don't create the picker
@@ -247,8 +262,12 @@ void WebIntentPickerController::ShowDialog(const string16& action,
     }
   }
 
-  pending_async_count_ += 2;
-  pending_registry_calls_count_ += 1;
+  SetDialogState(kPickerSetup);
+
+  pending_async_count_++;
+  pending_registry_calls_count_++;

[Greg Billock, 2012/08/10 19:39:50]

move these into same para as GetIntentServices

+
+  pending_cws_request_ = true;

[Greg Billock, 2012/08/10 19:39:50]

move this right by cws call

[groby-ooo-7-16, 2012/08/10 22:55:10]

Can't. We're already waiting for CWS data at this point. It is theoretically
possible (if GetIntentServices were ever synchronous) that the OnDataAvailable
callback is fired _before_ we requested CWS data.

If pending_cws_request were false at that point, we'd erroneously jump past the
waiting state.
On 2012/08/10 19:39:50, Greg Billock wrote:

[Greg Billock, 2012/08/13 15:17:24]

I'm not seeing that. Isn't the CWS request the
"GetCWSIntentsRegistry->GetIntentServices" below? If not, this is really
confusing. The above GetIntentServices call is covered by the attending
pending_async_count_++ right above it. What I'm seeing here is that the
pending_async_count_++ is for the GetIntentServices call below, there's another
one for GetDefaultIntentService, but the pending_cws_request_ = true is
dislocated up here instead of right by the call.

[groby-ooo-7-16, 2012/08/13 22:03:33]

It works currently, because WebIntentsRegistry->GetIntentServices and
GetDefaultIntentService work asynchronously and just post a task. If they _were_
to work synchronously, they both invoke their callback immediately and decrement
async_count to 0. 

The callback will then see that pending_cws_request is false, and conclude we
have _all_ the data. All before CWSIntentRegistry->GetIntentServices is called.

Your change theoretically works, but it is dependent on the implementation of
the registry staying the same. I don't like that on principle :)

Given that the issue will go away with a pending refactor (and WI Registry
implementation hopefully doesn't change _that_ quickly ;) moved it anyways...




On 2012/08/13 15:17:24, Greg Billock wrote:

[Greg Billock, 2012/08/14 05:22:00]

I see what you mean, but this means the logic is path-dependent, not state
dependent. If there's something going to fix it, it probably doesn't matter for
now, as I can't see these methods becoming sync.

On 2012/08/13 22:03:33, groby wrote:

 
   GetWebIntentsRegistry(tab_contents_)->GetIntentServices(
       action, type,
@@ -265,10 +284,11 @@ void WebIntentPickerController::ShowDialog(const string16& action,
                    weak_ptr_factory_.GetWeakPtr()));
   }
 
-    GetCWSIntentsRegistry(tab_contents_)->GetIntentServices(
-        action, type,
-        base::Bind(&WebIntentPickerController::OnCWSIntentServicesAvailable,
-                   weak_ptr_factory_.GetWeakPtr()));
+  pending_async_count_++;
+  GetCWSIntentsRegistry(tab_contents_)->GetIntentServices(
+      picker_model_->action(), picker_model_->mimetype(),
+      base::Bind(&WebIntentPickerController::OnCWSIntentServicesAvailable,
+                 weak_ptr_factory_.GetWeakPtr()));
 }
 
 void WebIntentPickerController::Observe(
@@ -515,11 +535,7 @@ void WebIntentPickerController::WebIntentServicesForExplicitIntent(
         favicon_service, handle,
         picker_model_->GetInstalledServiceCount() - 1);
 
-    if (services[i].disposition ==
-        webkit_glue::WebIntentServiceData::DISPOSITION_INLINE)
-      CreatePicker();
-    OnServiceChosen(services[i].service_url,
-                    ConvertDisposition(services[i].disposition));
+    InvokeService(picker_model_->GetInstalledServiceAt(i));
     AsyncOperationFinished();
     return;
   }
@@ -555,18 +571,13 @@ void WebIntentPickerController::RegistryCallsCompleted() {
             GURL(picker_model_->default_service_url()));
 
     if (default_service != NULL) {
-      if (default_service->disposition ==
-          WebIntentPickerModel::DISPOSITION_INLINE)
-        CreatePicker();
-
-      OnServiceChosen(default_service->url, default_service->disposition);
+      InvokeService(*default_service);
       return;
     }
   }
 
-  CreatePicker();
-  picker_->SetActionString(GetIntentActionString(
-      UTF16ToUTF8(picker_model_->action())));
+  OnPickerEvent(kPickerEventRegistryData);

[Greg Billock, 2012/08/10 19:39:50]

...RegistryDataComplete

In refactors, it'd be nice to get rid of the logic here and just signal "got
more data" to the state transitioner, though.

[groby-ooo-7-16, 2012/08/10 22:55:10]

Done.

+  OnIntentDataArrived();
 }
 
 void WebIntentPickerController::OnFaviconDataAvailable(
@@ -621,6 +632,8 @@ void WebIntentPickerController::OnCWSIntentServicesAvailable(
   }
 
   AsyncOperationFinished();
+  pending_cws_request_ = false;
+  OnIntentDataArrived();
 }
 
 void WebIntentPickerController::OnExtensionIconURLFetchComplete(
@@ -668,6 +681,14 @@ void WebIntentPickerController::OnExtensionIconURLFetchComplete(
                  unavailable_callback));
 }
 
+void WebIntentPickerController::OnIntentDataArrived() {
+  DCHECK(picker_model_.get());
+
+  if (!pending_cws_request_ &&
+      pending_registry_calls_count_ == 0)
+    OnPickerEvent(kPickerEventDataComplete);

[Greg Billock, 2012/08/10 19:39:50]

How about ...AsyncDataComplete

again, probably better to put all this transition logic into OnPickerEvent

[groby-ooo-7-16, 2012/08/10 22:55:10]

Changed the name. As for the remaining refactor you suggested, I'm not seeing
this making things much cleaner - we need the transition !hasRegistry ->
hasRegistry and !(hasRegistry && hasCWS) -> (hasRegistry && hasCWS). All other
ones are completely uninteresting.

(I've attempted the refactor, and it's more messy than before. I'll take CLs ;)


On 2012/08/10 19:39:50, Greg Billock wrote:

[Greg Billock, 2012/08/13 15:17:24]

OK, I believe you. :-) It's nice to have all the transition logic in one place,
and have the handlers be isolated from any state variables, let's try it later.

On 2012/08/10 22:55:10, groby wrote:

+}
+
 // static
 void WebIntentPickerController::DecodeExtensionIconAndResize(
     scoped_ptr<std::string> icon_response,
@@ -756,6 +777,48 @@ void WebIntentPickerController::OnExtensionInstallServiceAvailable(
   AsyncOperationFinished();
 }
 
+void WebIntentPickerController::OnPickerEvent(WebIntentPickerEvent event) {
+  switch (event) {

[Greg Billock, 2012/08/10 19:39:50]

Would this be easier to read if it switched on current state, and then figured
out the new state? That's a more canonical FSM organization, but perhaps this
saves effort if we want to do common operations on transitions into a state? In
control flows, that gets handled by the machinery providing a callback for every
state, but here you'd just call SetDialogState with the new one and consolidate
it there.

[groby-ooo-7-16, 2012/08/10 22:55:10]

I'm not convinced. 

The important part, at least for the picker, is what event happened, not what
state we're in. 

Ideally, we'd switch on a (state, event) tuple :)


On 2012/08/10 19:39:50, Greg Billock wrote:

[Greg Billock, 2012/08/13 15:17:24]

Sounds good.

+    case kPickerEventHiddenSetupTimeout:
+      DCHECK(dialog_state_ == kPickerSetup);
+      SetDialogState(kPickerWaiting);
+      break;
+
+    case kPickerEventMaxWaitTimeExceeded:
+      DCHECK(dialog_state_ == kPickerWaiting);
+
+      // If registry data is complete, go to main dialog. Otherwise, wait.
+      if (pending_registry_calls_count_ == 0)
+        SetDialogState(kPickerMain);
+      else
+        SetDialogState(kPickerWaitLong);
+      break;
+
+    case kPickerEventRegistryData:
+      DCHECK(dialog_state_ == kPickerSetup ||
+             dialog_state_ == kPickerWaiting ||
+             dialog_state_ == kPickerWaitLong);
+
+      // If minimum wait dialog time is exceeded, display main dialog.
+      // Either way, we don't do a thing.
+      break;
+
+    case kPickerEventDataComplete:
+      DCHECK(dialog_state_ == kPickerSetup ||
+              dialog_state_ == kPickerWaiting);
+
+      // In setup state, transition to main dialog. In waiting state, let
+      // timer expire.
+      if (dialog_state_ == kPickerSetup)
+        SetDialogState(kPickerMain);
+      break;
+
+    default:
+      NOTREACHED();
+      break;
+  }
+}
+
 void WebIntentPickerController::AsyncOperationFinished() {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   if (--pending_async_count_ == 0) {
@@ -764,14 +827,82 @@ void WebIntentPickerController::AsyncOperationFinished() {
   }
 }
 
+void WebIntentPickerController::InvokeService(
+    const WebIntentPickerModel::InstalledService& service) {
+  if (service.disposition == WebIntentPickerModel::DISPOSITION_INLINE) {
+    SetDialogState(kPickerMain);
+  }
+  OnServiceChosen(service.url, service.disposition);
+}
+
+void WebIntentPickerController::SetDialogState(WebIntentPickerState state) {
+  // Ignore events that don't change state.
+  if (state == dialog_state_)
+    return;
+
+  // Any pending timers are abandoned on state changes.
+  timer_factory_.InvalidateWeakPtrs();
+
+  switch (state) {
+    case kPickerSetup:
+      DCHECK(dialog_state_ == kPickerHidden);
+
+      // Post timer CWS pending
+      MessageLoop::current()->PostDelayedTask(FROM_HERE,
+          base::Bind(&WebIntentPickerController::OnPickerEvent,
+                     timer_factory_.GetWeakPtr(),
+                     kPickerEventHiddenSetupTimeout),
+          base::TimeDelta::FromMilliseconds(kMaxHiddenSetupTimeMs));
+      break;
+
+    case kPickerWaiting:
+      DCHECK(dialog_state_ == kPickerSetup);
+      // Waiting dialog can be dismissed after minimum wait time.
+      MessageLoop::current()->PostDelayedTask(FROM_HERE,
+          base::Bind(&WebIntentPickerController::OnPickerEvent,
+                     timer_factory_.GetWeakPtr(),
+                     kPickerEventMaxWaitTimeExceeded),
+          base::TimeDelta::FromMilliseconds(kMinThrobberDisplayTimeMs));
+      break;
+
+    case kPickerWaitLong:
+      DCHECK(dialog_state_ == kPickerWaiting);
+      break;
+
+    case kPickerMain:
+      // No DCHECK - main state can be reached from any state.
+      // Ready to display data.
+      picker_model_->SetWaitingForSuggestions(false);
+      break;
+
+    case kPickerHidden:
+      break;
+
+    default:
+      NOTREACHED();
+      break;
+
+  }
+
+  dialog_state_ = state;
+
+  // Create picker dialog when changing away from hidden state.
+  if (dialog_state_ != kPickerHidden && dialog_state_ != kPickerSetup)
+    CreatePicker();
+}
+
+
 void WebIntentPickerController::CreatePicker() {
   // If picker is non-NULL, it was set by a test.
   if (picker_ == NULL)
     picker_ = WebIntentPicker::Create(tab_contents_, this, picker_model_.get());
+  picker_->SetActionString(GetIntentActionString(
+      UTF16ToUTF8(picker_model_->action())));
   picker_shown_ = true;
 }
 
 void WebIntentPickerController::ClosePicker() {
+  SetDialogState(kPickerHidden);
   if (picker_)
     picker_->Close();
 }