[WebIntents, Gtk] "Waiting for Suggestion" dialog

chrome/browser/ui/intents/web_intent_picker_controller.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/intents/web_intent_picker_controller.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/platform_app_launcher.h"
 #include "chrome/browser/extensions/webstore_installer.h"
 #include "chrome/browser/favicon/favicon_service.h"
 #include "chrome/browser/intents/cws_intents_registry_factory.h"
 #include "chrome/browser/intents/default_web_intent_service.h"
 #include "chrome/browser/intents/web_intents_registry_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/tab_contents/tab_util.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #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"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/webdata/web_data_service.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/url_constants.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/notification_source.h"
@@ skipping 16 matching lines @@
 
 namespace {
 
 const char kShareActionURL[] = "http://webintents.org/share";
 const char kEditActionURL[] = "http://webintents.org/edit";
 const char kViewActionURL[] = "http://webintents.org/view";
 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);
 }
 
 // Gets the web intents registry for the profile in |tab_contents|.
 WebIntentsRegistry* GetWebIntentsRegistry(TabContents* tab_contents) {
   return WebIntentsRegistryFactory::GetForProfile(tab_contents->profile());
 }
 
@@ skipping 81 matching lines @@
   }
 
  private:
   base::WeakPtr<WebIntentPickerController> controller_;
 };
 
 }  // namespace
 
 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,
                  content::Source<content::NavigationController>(controller));
   registrar_.Add(this, chrome::NOTIFICATION_TAB_CLOSING,
                  content::Source<content::NavigationController>(controller));
 }
 
 WebIntentPickerController::~WebIntentPickerController() {
 }
 
 // TODO(gbillock): combine this with ShowDialog.
 void WebIntentPickerController::SetIntentsDispatcher(
     content::WebIntentsDispatcher* intents_dispatcher) {
   intents_dispatcher_ = intents_dispatcher;
   intents_dispatcher_->RegisterReplyNotification(
       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
   // in this method, but only after calling the registry.
   if (picker_shown_) {
     if (intents_dispatcher_) {
       intents_dispatcher_->SendReplyMessage(
           webkit_glue::WEB_INTENT_REPLY_FAILURE,
           ASCIIToUTF16("Simultaneous intent invocation."));
     }
@@ skipping 33 matching lines @@
       // required to find disposition set by service.)
       pending_async_count_++;
       GetWebIntentsRegistry(tab_contents_)->GetIntentServices(
           action, type, base::Bind(
               &WebIntentPickerController::WebIntentServicesForExplicitIntent,
               weak_ptr_factory_.GetWeakPtr()));
       return;
     }
   }
 
-  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,
           base::Bind(&WebIntentPickerController::OnWebIntentServicesAvailable,
               weak_ptr_factory_.GetWeakPtr()));
 
   GURL invoking_url = tab_contents_->web_contents()->GetURL();
   if (invoking_url.is_valid()) {
     pending_async_count_++;
     pending_registry_calls_count_++;
     GetWebIntentsRegistry(tab_contents_)->GetDefaultIntentService(
         action, type, invoking_url,
         base::Bind(&WebIntentPickerController::OnWebIntentDefaultsAvailable,
                    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(
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
   DCHECK(type == content::NOTIFICATION_LOAD_START ||
          type == chrome::NOTIFICATION_TAB_CLOSING);
   ClosePicker();
 }
@@ skipping 226 matching lines @@
         services[i].service_url,
         history::FAVICON,
         &favicon_consumer_,
         base::Bind(
             &WebIntentPickerController::OnFaviconDataAvailable,
             weak_ptr_factory_.GetWeakPtr()));
     favicon_consumer_.SetClientData(
         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;
   }
 
   // No acceptable extension. The intent cannot be dispatched.
   intents_dispatcher_->SendReplyMessage(
       webkit_glue::WEB_INTENT_REPLY_FAILURE,  ASCIIToUTF16(
           "Explicit extension URL is not available."));
 
   AsyncOperationFinished();
@@ skipping 15 matching lines @@
   if (pending_registry_calls_count_ != 0) return;
 
   if (picker_model_->default_service_url().is_valid()) {
     // If there's a default service, dispatch to it immediately
     // without showing the picker.
     const WebIntentPickerModel::InstalledService* default_service =
         picker_model_->GetInstalledServiceWithURL(
             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(
     FaviconService::Handle handle, history::FaviconData favicon_data) {
   size_t index = favicon_consumer_.GetClientDataForCurrentRequest();
   if (favicon_data.is_valid()) {
     SkBitmap icon_bitmap;
 
     if (gfx::PNGCodec::Decode(favicon_data.image_data->front(),
                               favicon_data.image_data->size(),
@@ skipping 34 matching lines @@
                 weak_ptr_factory_.GetWeakPtr(), info.id)));
 
     icon_url_fetcher->SetLoadFlags(
         net::LOAD_DO_NOT_SEND_COOKIES | net::LOAD_DO_NOT_SAVE_COOKIES);
     icon_url_fetcher->SetRequestContext(
         tab_contents_->profile()->GetRequestContext());
     icon_url_fetcher->Start();
   }
 
   AsyncOperationFinished();
+  pending_cws_request_ = false;
+  OnIntentDataArrived();
 }
 
 void WebIntentPickerController::OnExtensionIconURLFetchComplete(
     const string16& extension_id, const net::URLFetcher* source) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   if (source->GetResponseCode() != 200) {
     AsyncOperationFinished();
     return;
   }
 
@@ skipping 27 matching lines @@
 
   // Decode PNG and resize on worker thread.
   content::BrowserThread::PostBlockingPoolTask(
       FROM_HERE,
       base::Bind(&DecodeExtensionIconAndResize,
                  base::Passed(&response),
                  available_callback,
                  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,
     const ExtensionIconAvailableCallback& callback,
     const base::Closure& unavailable_callback) {
   SkBitmap icon_bitmap;
   if (gfx::PNGCodec::Decode(
         reinterpret_cast<const unsigned char*>(icon_response->data()),
         icon_response->length(),
         &icon_bitmap)) {
@@ skipping 68 matching lines @@
   const webkit_glue::WebIntentServiceData& service_data = services[0];
   picker_model_->AddInstalledService(
       service_data.title, service_data.service_url,
       ConvertDisposition(service_data.disposition));
   OnServiceChosen(
       service_data.service_url,
       ConvertDisposition(service_data.disposition));
   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) {
     if (picker_)
       picker_->OnPendingAsyncCompleted();
   }
 }
 
+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();
 }