[WebIntents, Views] First version of "Waiting" dialog for views.

First version of "Waiting" dialog for views.

BUG=137281
R=sky@chromium.org, gbillock@chromium.org

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

[groby-ooo-7-16, 2012-08-25 03:08:31]

sky: please review views
gbillock: please review (trivial) intents

[tfarina, 2012-08-26 12:45:53]

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

http://codereview.chromium.org/10870092/diff/1/chrome/browser/ui/intents/web_...
chrome/browser/ui/intents/web_intent_picker_controller.cc:891: 
is necessary to remove this empty line? I think it might help in readability.

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

http://codereview.chromium.org/10870092/diff/1/chrome/browser/ui/intents/web_...
chrome/browser/ui/intents/web_intent_picker_controller.cc:890:
DCHECK(dialog_state_ == kPickerHidden);
nit:
DCHECK_EQ(expected, actual);

so
DCHECK_EQ(kPickerHidden, dialog_state_);

?

http://codereview.chromium.org/10870092/diff/1/chrome/browser/ui/views/web_in...
File chrome/browser/ui/views/web_intent_picker_views.cc (right):

http://codereview.chromium.org/10870092/diff/1/chrome/browser/ui/views/web_in...
chrome/browser/ui/views/web_intent_picker_views.cc:83: // Create a "close"
button.
the function name already says this ;) so it seems redundant, I'd just leave
this comment out.

http://codereview.chromium.org/10870092/diff/1/chrome/browser/ui/views/web_in...
chrome/browser/ui/views/web_intent_picker_views.cc:88:
rb.GetImageSkiaNamed(IDR_SHARED_IMAGES_X));
indentation is wrong here, align with v.

http://codereview.chromium.org/10870092/diff/1/chrome/browser/ui/views/web_in...
chrome/browser/ui/views/web_intent_picker_views.cc:257: views::ButtonListener*
listener, bool use_close_button) {
can you write one argument per line? I guess listener fits on the above line,
and you can write use_close_button below it.

http://codereview.chromium.org/10870092/diff/1/chrome/browser/ui/views/web_in...
chrome/browser/ui/views/web_intent_picker_views.cc:287: // Create text
end with a period.

[sky, 2012-08-27 14:27:28]

http://codereview.chromium.org/10870092/diff/1/chrome/browser/ui/views/web_in...
File chrome/browser/ui/views/web_intent_picker_views.cc (right):

http://codereview.chromium.org/10870092/diff/1/chrome/browser/ui/views/web_in...
chrome/browser/ui/views/web_intent_picker_views.cc:1181:
contents_->RemoveAllChildViews(contents_);
I think you mean RemoveAllChildViews(true) here.

[groby-ooo-7-16, 2012-08-28 00:08:17]

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

http://codereview.chromium.org/10870092/diff/1/chrome/browser/ui/intents/web_...
chrome/browser/ui/intents/web_intent_picker_controller.cc:891: 
It doesn't add much clarity, either.
On 2012/08/26 12:45:54, tfarina wrote:
> is necessary to remove this empty line? I think it might help in readability.

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

http://codereview.chromium.org/10870092/diff/1/chrome/browser/ui/intents/web_...
chrome/browser/ui/intents/web_intent_picker_controller.cc:890:
DCHECK(dialog_state_ == kPickerHidden);
On 2012/08/26 12:45:54, tfarina wrote:
> nit:
> DCHECK_EQ(expected, actual);
> 
> so
> DCHECK_EQ(kPickerHidden, dialog_state_);
> 
> ?

Done.

http://codereview.chromium.org/10870092/diff/1/chrome/browser/ui/views/web_in...
File chrome/browser/ui/views/web_intent_picker_views.cc (right):

http://codereview.chromium.org/10870092/diff/1/chrome/browser/ui/views/web_in...
chrome/browser/ui/views/web_intent_picker_views.cc:83: // Create a "close"
button.
It does, but I like having a comment for functions :) (Preserves the idea across
renames, for one)
On 2012/08/26 12:45:54, tfarina wrote:
> the function name already says this ;) so it seems redundant, I'd just leave
> this comment out.

http://codereview.chromium.org/10870092/diff/1/chrome/browser/ui/views/web_in...
chrome/browser/ui/views/web_intent_picker_views.cc:88:
rb.GetImageSkiaNamed(IDR_SHARED_IMAGES_X));
On 2012/08/26 12:45:54, tfarina wrote:
> indentation is wrong here, align with v.

Done.

http://codereview.chromium.org/10870092/diff/1/chrome/browser/ui/views/web_in...
chrome/browser/ui/views/web_intent_picker_views.cc:257: views::ButtonListener*
listener, bool use_close_button) {
On 2012/08/26 12:45:54, tfarina wrote:
> can you write one argument per line? I guess listener fits on the above line,
> and you can write use_close_button below it.

Done.

http://codereview.chromium.org/10870092/diff/1/chrome/browser/ui/views/web_in...
chrome/browser/ui/views/web_intent_picker_views.cc:287: // Create text
On 2012/08/26 12:45:54, tfarina wrote:
> end with a period.

Done.

http://codereview.chromium.org/10870092/diff/1/chrome/browser/ui/views/web_in...
chrome/browser/ui/views/web_intent_picker_views.cc:1181:
contents_->RemoveAllChildViews(contents_);
I certainly do, thanks for catching this. (Go implicit casts, though! ;)
Done.
On 2012/08/27 14:27:28, sky wrote:
> I think you mean RemoveAllChildViews(true) here.

[sky, 2012-08-28 03:00:16]

LGTM

[Greg Billock, 2012-08-28 17:23:37]

On 2012/08/28 03:00:16, sky wrote:
> LGTM

lgtm

[groby-ooo-7-16, 2012-08-28 17:25:12]

tfarina: Hopefully addressed your concerns. Since I can't reach you directly,
landing this w/ owners LGTM, but happy to address any further comments you have.

[commit-bot: I haz the power, 2012-08-28 17:25:20]

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

[commit-bot: I haz the power, 2012-08-28 19:10:42]

Try job failure for 10870092-4 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-08-28 20:44:58]

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

[commit-bot: I haz the power, 2012-08-28 23:31:03]

Change committed as 153787

[tfarina, 2012-08-29 01:04:01]

https://chromiumcodereview.appspot.com/10870092/diff/15002/chrome/browser/ui/...
File chrome/browser/ui/views/web_intent_picker_views.cc (right):

https://chromiumcodereview.appspot.com/10870092/diff/15002/chrome/browser/ui/...
chrome/browser/ui/views/web_intent_picker_views.cc:267: views::ColumnSet*
header_cs = NULL;
looks like this could be moved inside the if clause below.

https://chromiumcodereview.appspot.com/10870092/diff/15002/chrome/browser/ui/...
chrome/browser/ui/views/web_intent_picker_views.cc:1084: if (waiting_view_ &&
!model->IsWaitingForSuggestions()) {
no need of { } for single line statements.

https://chromiumcodereview.appspot.com/10870092/diff/15002/chrome/browser/ui/...
chrome/browser/ui/views/web_intent_picker_views.cc:1300: 
please, remove these two empty lines.