Show error messages in speech bubble allowing user to retry as well.

We used to show error messages as info bars earlier. Based on UX feedback, we now show the messages within the speech input UI bubble. We also let the user retry recognition without returning the error to the web page unless the user clicked the cancel button or closed the bubble.

For this to work, we now keep the request alive in the SpeechInputManager map until it completes successfully or user explicitly cancelled it. We also keep the SpeechInputBubble object alive, even when the actual bubble window is not visible on screen, and construct the window if required whenever we want to show the recording status or error message on screen.

BUG=53598
TEST=unit_tests --gtest_filter=SpeechInputBubbleControllerTest.*

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

[joth, 2010-09-10 14:11:03]

I'm not entirely confident about reviewing the Views changes, not sure who the
original reviewer was?

http://codereview.chromium.org/3352018/diff/5001/6004
File chrome/browser/speech/speech_input_bubble.h (right):

http://codereview.chromium.org/3352018/diff/5001/6004#newcode94
chrome/browser/speech/speech_input_bubble.h:94: void SetMessage(const string16&
text);
slight shame how the public interface is now a mix of concrete and pure virtual
methods, but some abstract methods are protected. (the so-called template
pattern) -- would be nice if the all followed one style or the other.

http://codereview.chromium.org/3352018/diff/5001/6004#newcode111
chrome/browser/speech/speech_input_bubble.h:111: DisplayMode display_mode_;
protected data members not allowed by style guide, use accessors or push down to
child classes.

Alternatively, remove altogether and instead have methods on the Delegate to
supply these when needed. (InfoBarDelegate does it this way, for example)

http://codereview.chromium.org/3352018/diff/5001/6005
File chrome/browser/speech/speech_input_bubble_controller.cc (right):

http://codereview.chromium.org/3352018/diff/5001/6005#newcode39
chrome/browser/speech/speech_input_bubble_controller.cc:39:
DCHECK(bubbles_.find(caller_id) == bubbles_.end());
I've had this style recommended before:
DCHECK_EQ(0u, bubbles_.count(caller_id));

http://codereview.chromium.org/3352018/diff/5001/6005#newcode74
chrome/browser/speech/speech_input_bubble_controller.cc:74: if
(bubbles_.find(caller_id) == bubbles_.end())
ditto
if (!bubbles_.count(caller_id))

or store the iterator in a local, and reuse below

http://codereview.chromium.org/3352018/diff/5001/6005#newcode82
chrome/browser/speech/speech_input_bubble_controller.cc:82:
bubbles_[caller_id]->Show();
nit: this does the map lookup twice.

http://codereview.chromium.org/3352018/diff/5001/6005#newcode98
chrome/browser/speech/speech_input_bubble_controller.cc:98:
bubbles_[caller_id]->SetRecognizingMode();
ditto (and next fn)

http://codereview.chromium.org/3352018/diff/5001/6005#newcode120
chrome/browser/speech/speech_input_bubble_controller.cc:120:
bubbles_[caller_id]->Show();
could we factor most of this out and share with SetBubbleRecordingMode?

http://codereview.chromium.org/3352018/diff/5001/6006
File chrome/browser/speech/speech_input_bubble_controller.h (right):

http://codereview.chromium.org/3352018/diff/5001/6006#newcode23
chrome/browser/speech/speech_input_bubble_controller.h:23: // those requests in
the UI thread. There is only 1 speech input bubble shown to
>There is only 1 speech input bubble
the map of Bubble* seems to contradict this now. comment here or there to
resolve this?

http://codereview.chromium.org/3352018/diff/5001/6006#newcode45
chrome/browser/speech/speech_input_bubble_controller.h:45:
~SpeechInputBubbleController();
virtual

http://codereview.chromium.org/3352018/diff/5001/6006#newcode82
chrome/browser/speech/speech_input_bubble_controller.h:82: std::map<int,
SpeechInputBubble*> bubbles_;
comment what the ints are. also, ownership / lifetime of the pointers.

http://codereview.chromium.org/3352018/diff/5001/6008
File chrome/browser/speech/speech_input_bubble_gtk.cc (right):

http://codereview.chromium.org/3352018/diff/5001/6008#newcode128
chrome/browser/speech/speech_input_bubble_gtk.cc:128: // TODO(satish):
Implement.
could use NOTREACHED();

http://codereview.chromium.org/3352018/diff/5001/6009
File chrome/browser/speech/speech_input_bubble_mac.mm (right):

http://codereview.chromium.org/3352018/diff/5001/6009#newcode59
chrome/browser/speech/speech_input_bubble_mac.mm:59: // TODO(satish): Implement.
ditto

http://codereview.chromium.org/3352018/diff/5001/6010
File chrome/browser/speech/speech_input_bubble_views.cc (right):

http://codereview.chromium.org/3352018/diff/5001/6010#newcode32
chrome/browser/speech/speech_input_bubble_views.cc:32: const int
kSpaceBetweenButtons = 10;
there's already a bunch of constants in
http://src.chromium.org/viewvc/chrome/trunk/src/views/standard_layout.h
should we be using any of them?

http://codereview.chromium.org/3352018/diff/5001/6010#newcode152
chrome/browser/speech/speech_input_bubble_views.cc:152: if
(message_->IsVisible()) {
DCHECK(try_again_->IsVisible());

http://codereview.chromium.org/3352018/diff/5001/6010#newcode168
chrome/browser/speech/speech_input_bubble_views.cc:168: } else {
DCHECK(heading_->IsVisible());
DCHECK(icon_->IsVisible());

http://codereview.chromium.org/3352018/diff/5001/6011
File chrome/browser/speech/speech_input_manager.cc (right):

http://codereview.chromium.org/3352018/diff/5001/6011#newcode114
chrome/browser/speech/speech_input_manager.cc:114: SpeechInputRequest request;
nit: can avoid a bunch of copying with:
SpeechInputRequest* request = &requests_[caller_id];
request->delegate = delegate;
...

http://codereview.chromium.org/3352018/diff/5001/6011#newcode119
chrome/browser/speech/speech_input_manager.cc:119: request.element_rect =
element_rect;
these three no longer need to be in the request, as we have already handed them
through to the bubble

http://codereview.chromium.org/3352018/diff/5001/6011#newcode199
chrome/browser/speech/speech_input_manager.cc:199: DCHECK(false);
NOTREACHED() << "unknown error " << error;
return;

[Satish, 2010-09-10 16:54:46]

Thanks for the quick review. I have addressed all the comments with no
exception. Some top level changes made:

- Factored SpeechInputBubble into Bubble (just an interface) and BubbleBase
(contains platform independent implementation)
- Removed UI code added to bubble_views.cc, instead just kept place holders as
in the _gtk and _mac files. I'll send a separate CL for these to the original
reviewers.
- The unit tests have got non-trivial changes to get working in the latest model
of bubble Create+SetXxx model to actually get them to active state and show on
screen.

[joth, 2010-09-10 17:08:40]

LGTM

Just 2 minor comments..

http://codereview.chromium.org/3352018/diff/16001/17005
File chrome/browser/speech/speech_input_bubble_controller.cc (right):

http://codereview.chromium.org/3352018/diff/16001/17005#newcode70
chrome/browser/speech/speech_input_bubble_controller.cc:70: }
you could factor this into SetBubbleRecordingModeOrMessage too

http://codereview.chromium.org/3352018/diff/16001/17006
File chrome/browser/speech/speech_input_bubble_controller.h (right):

http://codereview.chromium.org/3352018/diff/16001/17006#newcode88
chrome/browser/speech/speech_input_bubble_controller.h:88: // Map of caller-ids
to bubble objects. The bubbles are weak pointers owned by
s/weak pointer/raw pointer/ (or just say "The bubbles are owned by this
object....")

"weak pointer" makes me think of auto-self-nulling pointers like WeakPtr<> does.

[Satish, 2010-09-10 17:12:11]

http://codereview.chromium.org/3352018/diff/16001/17005
File chrome/browser/speech/speech_input_bubble_controller.cc (right):

http://codereview.chromium.org/3352018/diff/16001/17005#newcode70
chrome/browser/speech/speech_input_bubble_controller.cc:70: }
On 2010/09/10 17:08:40, joth wrote:
> you could factor this into SetBubbleRecordingModeOrMessage too

I want readers of this code to recognise that this method can be called from any
thread and does the right thing, hence did not want to move that check-and-post
to a sub function.

[joth, 2010-09-13 15:45:28]

On 10 September 2010 18:12, <satish@chromium.org> wrote:

>
> http://codereview.chromium.org/3352018/diff/16001/17005
> File chrome/browser/speech/speech_input_bubble_controller.cc (right):
>
> http://codereview.chromium.org/3352018/diff/16001/17005#newcode70
> chrome/browser/speech/speech_input_bubble_controller.cc:70: }
> On 2010/09/10 17:08:40, joth wrote:
>
>> you could factor this into SetBubbleRecordingModeOrMessage too
>>
>
> I want readers of this code to recognise that this method can be called
> from any thread and does the right thing, hence did not want to move
> that check-and-post to a sub function.
>
>
OK then.
Reason I mention this is constructing a member function pointer takes a
surprising amount of object code, so I always look for ways to share such
code.