[WebIntents, Gtk] "Waiting for Suggestion" dialog

[WebIntents, Gtk] "Waiting for Suggestion" dialog

Mostly a refactor of the picker controller, plus an initial (not complete - missing throbber image support) version of the Gtk dialog

BUG=137281
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151598

[groby-ooo-7-16, 2012-08-08 23:17:51]

This is the initial logic for the "waiting" dialog. Still pending some cleanup,
re-layouting for Gtk, and tests :)

Please treat as an RFC for the non-Gtk code.

[Greg Billock, 2012-08-08 23:47:11]

http://codereview.chromium.org/10827238/diff/1/chrome/browser/ui/intents/web_...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

http://codereview.chromium.org/10827238/diff/1/chrome/browser/ui/intents/web_...
chrome/browser/ui/intents/web_intent_picker_controller.cc:268:
pending_registry_calls_count_ += 1;
There are two of these -- services and defaults. Better to track them separately
with their own flags/events. Then we can figure out what's going on more easily.

http://codereview.chromium.org/10827238/diff/1/chrome/browser/ui/intents/web_...
chrome/browser/ui/intents/web_intent_picker_controller.cc:633:
AsyncOperationFinished();
It'd be nice to use the state machine to kill this off -- just notify the state
machine on events, and let it figure out what state to go into.

http://codereview.chromium.org/10827238/diff/1/chrome/browser/ui/intents/web_...
chrome/browser/ui/intents/web_intent_picker_controller.cc:687:
pending_registry_calls_count_ == 0)
Merge this into the state machine logic.

http://codereview.chromium.org/10827238/diff/1/chrome/browser/ui/intents/web_...
File chrome/browser/ui/intents/web_intent_picker_controller.h (right):

http://codereview.chromium.org/10827238/diff/1/chrome/browser/ui/intents/web_...
chrome/browser/ui/intents/web_intent_picker_controller.h:63:
kPickerEventDataComplete,  // Data from registry and CWS has arrived.
Better to just have discrete events and figure out what they mean for the FSM in
the code, so have kPickerEventDataCWS.

http://codereview.chromium.org/10827238/diff/1/chrome/browser/ui/intents/web_...
chrome/browser/ui/intents/web_intent_picker_controller.h:196: void
OnPickerEvent(WebIntentPickerEvent event);
Why do we need this? Shouldn't the callbacks themselves update the state?

[groby-ooo-7-16, 2012-08-09 21:59:56]

https://chromiumcodereview.appspot.com/10827238/diff/1/chrome/browser/ui/inte...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

https://chromiumcodereview.appspot.com/10827238/diff/1/chrome/browser/ui/inte...
chrome/browser/ui/intents/web_intent_picker_controller.cc:268:
pending_registry_calls_count_ += 1;
See discussion below - tempted to move to bitfield.

I might reserve that for a second refactoring, though. I really want to land
this functionality :)
On 2012/08/08 23:47:11, Greg Billock wrote:
> There are two of these -- services and defaults. Better to track them
separately
> with their own flags/events. Then we can figure out what's going on more
easily.

https://chromiumcodereview.appspot.com/10827238/diff/1/chrome/browser/ui/inte...
chrome/browser/ui/intents/web_intent_picker_controller.cc:633:
AsyncOperationFinished();
I'm trying to have the states correspond to actual action taken - this would
introduce a dummy state. 

And it'd be a _real_ pain once I moved the pending_registry_calls_count_ over -
we have three pieces of data coming in, so a total of 8 states. Only 3 of which
we're interested in.


On 2012/08/08 23:47:11, Greg Billock wrote:
> It'd be nice to use the state machine to kill this off -- just notify the
state
> machine on events, and let it figure out what state to go into.

https://chromiumcodereview.appspot.com/10827238/diff/1/chrome/browser/ui/inte...
chrome/browser/ui/intents/web_intent_picker_controller.cc:687:
pending_registry_calls_count_ == 0)
See above. I'm treating this as a mini state machine. :)

Would using a bit field instead of a count make it more obvious?

On 2012/08/08 23:47:11, Greg Billock wrote:
> Merge this into the state machine logic.

https://chromiumcodereview.appspot.com/10827238/diff/1/chrome/browser/ui/inte...
File chrome/browser/ui/intents/web_intent_picker_controller.h (right):

https://chromiumcodereview.appspot.com/10827238/diff/1/chrome/browser/ui/inte...
chrome/browser/ui/intents/web_intent_picker_controller.h:63:
kPickerEventDataComplete,  // Data from registry and CWS has arrived.
CWS data by itself does _never_ cause any transition, though. That's why it's
just a bool.

On 2012/08/08 23:47:11, Greg Billock wrote:
> Better to just have discrete events and figure out what they mean for the FSM
in
> the code, so have kPickerEventDataCWS.

https://chromiumcodereview.appspot.com/10827238/diff/1/chrome/browser/ui/inte...
chrome/browser/ui/intents/web_intent_picker_controller.h:196: void
OnPickerEvent(WebIntentPickerEvent event);
On 2012/08/08 23:47:11, Greg Billock wrote:
> Why do we need this? Shouldn't the callbacks themselves update the state?

OnPickerEvent handles edges of the graph, SetDialogState does the work
associated with activating nodes.

[groby-ooo-7-16, 2012-08-09 23:55:16]

This is in a state that should be (closer) to ready. Major refactorings as
suggested in first patch review postponed for round 2.

[Elliot Glaysher, 2012-08-10 16:46:00]

gtk lgtm

[Greg Billock, 2012-08-10 18:46:57]

http://codereview.chromium.org/10827238/diff/2002/chrome/browser/ui/gtk/web_i...
File chrome/browser/ui/gtk/web_intent_picker_gtk.cc (right):

http://codereview.chromium.org/10827238/diff/2002/chrome/browser/ui/gtk/web_i...
chrome/browser/ui/gtk/web_intent_picker_gtk.cc:553: void
WebIntentPickerGtk::InitMainContents() {
What's the difference between InitContents and InitMainContents? How about
InitWaitContents() if IsWaitingForSuggestions?

Also, we're showing this if we can't load the DB? I don't think we should do
full contents replacement just if the CWS can't load and there are other
options. That is, it seems like there should be some more checks to see whether
the model has non-CWS stuff.

http://codereview.chromium.org/10827238/diff/2002/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_controller.h (right):

http://codereview.chromium.org/10827238/diff/2002/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.h:50: enum
WebIntentPickerState {
This is slop from the refactor, no?

[groby-ooo-7-16, 2012-08-10 18:57:26]

http://codereview.chromium.org/10827238/diff/2002/chrome/browser/ui/gtk/web_i...
File chrome/browser/ui/gtk/web_intent_picker_gtk.cc (right):

http://codereview.chromium.org/10827238/diff/2002/chrome/browser/ui/gtk/web_i...
chrome/browser/ui/gtk/web_intent_picker_gtk.cc:553: void
WebIntentPickerGtk::InitMainContents() {
InitContents: Just initializes contents_

It currently creates the waiting dialog since this means pickers _with_ the wait
dialog (Gtk) and _without_ (Views, Cocoa) have an identical API and can be
driven by the same controller. More refactoring to come on that ;)

InitMainContents: The "meat" of the picker dialog. Titles, buttons, inline dispo
surface, etc. Will be further split up into selection dialog and inline dispo.

On 2012/08/10 18:46:57, Greg Billock wrote:
> What's the difference between InitContents and InitMainContents? How about
> InitWaitContents() if IsWaitingForSuggestions?
> 
> Also, we're showing this if we can't load the DB? I don't think we should do
> full contents replacement just if the CWS can't load and there are other
> options. That is, it seems like there should be some more checks to see
whether
> the model has non-CWS stuff.

http://codereview.chromium.org/10827238/diff/2002/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_controller.h (right):

http://codereview.chromium.org/10827238/diff/2002/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.h:50: enum
WebIntentPickerState {
I'm not sure what you're asking :)

On 2012/08/10 18:46:57, Greg Billock wrote:
> This is slop from the refactor, no?

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

http://codereview.chromium.org/10827238/diff/2002/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

http://codereview.chromium.org/10827238/diff/2002/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.cc:268:
pending_registry_calls_count_++;
move these into same para as GetIntentServices

http://codereview.chromium.org/10827238/diff/2002/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.cc:270:
pending_cws_request_ = true;
move this right by cws call

http://codereview.chromium.org/10827238/diff/2002/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.cc:579:
OnPickerEvent(kPickerEventRegistryData);
...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.

http://codereview.chromium.org/10827238/diff/2002/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.cc:689:
OnPickerEvent(kPickerEventDataComplete);
How about ...AsyncDataComplete

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

http://codereview.chromium.org/10827238/diff/2002/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.cc:781: switch (event) {
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:09]

https://chromiumcodereview.appspot.com/10827238/diff/2002/chrome/browser/ui/i...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

https://chromiumcodereview.appspot.com/10827238/diff/2002/chrome/browser/ui/i...
chrome/browser/ui/intents/web_intent_picker_controller.cc:270:
pending_cws_request_ = true;
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:
> move this right by cws call

https://chromiumcodereview.appspot.com/10827238/diff/2002/chrome/browser/ui/i...
chrome/browser/ui/intents/web_intent_picker_controller.cc:579:
OnPickerEvent(kPickerEventRegistryData);
On 2012/08/10 19:39:50, Greg Billock wrote:
> ...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.

Done.

https://chromiumcodereview.appspot.com/10827238/diff/2002/chrome/browser/ui/i...
chrome/browser/ui/intents/web_intent_picker_controller.cc:689:
OnPickerEvent(kPickerEventDataComplete);
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:
> How about ...AsyncDataComplete
> 
> again, probably better to put all this transition logic into OnPickerEvent

https://chromiumcodereview.appspot.com/10827238/diff/2002/chrome/browser/ui/i...
chrome/browser/ui/intents/web_intent_picker_controller.cc:781: switch (event) {
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:
> 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.

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

https://chromiumcodereview.appspot.com/10827238/diff/2002/chrome/browser/ui/i...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

https://chromiumcodereview.appspot.com/10827238/diff/2002/chrome/browser/ui/i...
chrome/browser/ui/intents/web_intent_picker_controller.cc:270:
pending_cws_request_ = true;
On 2012/08/10 22:55:10, groby wrote:
> 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.

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.
> 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:
> > move this right by cws call
>

https://chromiumcodereview.appspot.com/10827238/diff/2002/chrome/browser/ui/i...
chrome/browser/ui/intents/web_intent_picker_controller.cc:689:
OnPickerEvent(kPickerEventDataComplete);
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:
> 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:
> > How about ...AsyncDataComplete
> > 
> > again, probably better to put all this transition logic into OnPickerEvent
>

https://chromiumcodereview.appspot.com/10827238/diff/2002/chrome/browser/ui/i...
chrome/browser/ui/intents/web_intent_picker_controller.cc:781: switch (event) {
On 2012/08/10 22:55:10, groby wrote:
> I'm not convinced. 
> 
> The important part, at least for the picker, is what event happened, not what
> state we're in. 

Sounds good.

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


> 
> 
> On 2012/08/10 19:39:50, Greg Billock wrote:
> > 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-13 22:03:33]

http://codereview.chromium.org/10827238/diff/2002/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

http://codereview.chromium.org/10827238/diff/2002/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.cc:270:
pending_cws_request_ = true;
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:
> On 2012/08/10 22:55:10, groby wrote:
> > 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.
> 
> 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.
> > 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:
> > > move this right by cws call
> > 
>

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

lgtm

http://codereview.chromium.org/10827238/diff/2002/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

http://codereview.chromium.org/10827238/diff/2002/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.cc:270:
pending_cws_request_ = true;
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:
> 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:
> > On 2012/08/10 22:55:10, groby wrote:
> > > 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.
> > 
> > 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.
> > > 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:
> > > > move this right by cws call
> > > 
> > 
>

[commit-bot: I haz the power, 2012-08-14 18:56:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/10827238/8002

[commit-bot: I haz the power, 2012-08-14 23:29:33]

Change committed as 151598