Make ARC's intent picker use the buttons insets.

Make ARC's intent picker use the buttons insets.

Need to call SetupLayout via UpdateDialogButtons so the buttons make
proper use of the established insets.

Bug: 678141
Test: Try.
Review-Url: https://codereview.chromium.org/2874493002
Cr-Commit-Position: refs/heads/master@{#472534}
Committed: https://chromium.googlesource.com/chromium/src/+/15bdd62c05472adc642dcf0982fe4662502ac957

[djacobo, 2017-05-09 20:41:57]

The CQ bit was checked by djacobo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 20:42:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[djacobo, 2017-05-09 21:13:03]

djacobo@chromium.org changed reviewers:
+ sky@chromium.org

[djacobo, 2017-05-09 21:13:03]

PTAL

[commit-bot: I haz the power, 2017-05-09 22:00:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 22:00:36]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[djacobo, 2017-05-09 22:20:56]

On 2017/05/09 21:13:03, djacobo wrote:
> PTAL

Already tested on M59, this fixes the insets problem there too (no need to
cherry pick it before M59)

[djacobo, 2017-05-10 01:05:05]

The CQ bit was checked by djacobo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 01:06:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 03:10:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 03:10:24]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet;
builder either lacks capacity or does not exist (misspelled?))
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build
has not started yet; builder either lacks capacity or does not exist
(misspelled?))
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does
not exist (misspelled?))

[sky, 2017-05-10 15:27:02]

https://codereview.chromium.org/2874493002/diff/1/chrome/browser/ui/views/int...
File chrome/browser/ui/views/intent_picker_bubble_view.cc (right):

https://codereview.chromium.org/2874493002/diff/1/chrome/browser/ui/views/int...
chrome/browser/ui/views/intent_picker_bubble_view.cc:111:
delegate->GetDialogClientView()->set_button_row_insets(
Can this be moved above line 109 such that 113 is not necessary? Otherwise I
think set_button_row_insets sure become SetButtonRowInsets() and it should force
a layout. In other words you shouldn't have to manually call
UpdateDialogButtons.

[djacobo, 2017-05-10 18:14:20]

https://codereview.chromium.org/2874493002/diff/1/chrome/browser/ui/views/int...
File chrome/browser/ui/views/intent_picker_bubble_view.cc (right):

https://codereview.chromium.org/2874493002/diff/1/chrome/browser/ui/views/int...
chrome/browser/ui/views/intent_picker_bubble_view.cc:111:
delegate->GetDialogClientView()->set_button_row_insets(
On 2017/05/10 15:27:02, sky wrote:
> Can this be moved above line 109 such that 113 is not necessary? Otherwise I
> think set_button_row_insets sure become SetButtonRowInsets() and it should
force
> a layout. In other words you shouldn't have to manually call
> UpdateDialogButtons.

I don't think that's possible since CreateBubble calls Init, so before that
everything GetDialogClientView is still not constructed, right? I did the
suggested experiment and the Browser crashes while trying to start the picker.

[sky, 2017-05-12 01:56:59]

Ok, in that case please rename set_button_row_insets and have it trigger a
layout.

On Wed, May 10, 2017 at 11:14 AM, <djacobo@chromium.org> wrote:

>
> https://codereview.chromium.org/2874493002/diff/1/chrome/
> browser/ui/views/intent_picker_bubble_view.cc
> File chrome/browser/ui/views/intent_picker_bubble_view.cc (right):
>
> https://codereview.chromium.org/2874493002/diff/1/chrome/
> browser/ui/views/intent_picker_bubble_view.cc#newcode111
> chrome/browser/ui/views/intent_picker_bubble_view.cc:111:
> delegate->GetDialogClientView()->set_button_row_insets(
> On 2017/05/10 15:27:02, sky wrote:
> > Can this be moved above line 109 such that 113 is not necessary?
> Otherwise I
> > think set_button_row_insets sure become SetButtonRowInsets() and it
> should force
> > a layout. In other words you shouldn't have to manually call
> > UpdateDialogButtons.
>
> I don't think that's possible since CreateBubble calls Init, so before
> that everything GetDialogClientView is still not constructed, right? I
> did the suggested experiment and the Browser crashes while trying to
> start the picker.
>
> https://codereview.chromium.org/2874493002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2017-05-12 01:57:23]

By rename I mean as set_button_row_insets will no longer be cheap it should
be changed to SetButtonRowInsets().

On Thu, May 11, 2017 at 6:56 PM, Scott Violet <sky@chromium.org> wrote:

> Ok, in that case please rename set_button_row_insets and have it trigger a
> layout.
>
> On Wed, May 10, 2017 at 11:14 AM, <djacobo@chromium.org> wrote:
>
>>
>> https://codereview.chromium.org/2874493002/diff/1/chrome/bro
>> wser/ui/views/intent_picker_bubble_view.cc
>> File chrome/browser/ui/views/intent_picker_bubble_view.cc (right):
>>
>> https://codereview.chromium.org/2874493002/diff/1/chrome/bro
>> wser/ui/views/intent_picker_bubble_view.cc#newcode111
>> chrome/browser/ui/views/intent_picker_bubble_view.cc:111:
>> delegate->GetDialogClientView()->set_button_row_insets(
>> On 2017/05/10 15:27:02, sky wrote:
>> > Can this be moved above line 109 such that 113 is not necessary?
>> Otherwise I
>> > think set_button_row_insets sure become SetButtonRowInsets() and it
>> should force
>> > a layout. In other words you shouldn't have to manually call
>> > UpdateDialogButtons.
>>
>> I don't think that's possible since CreateBubble calls Init, so before
>> that everything GetDialogClientView is still not constructed, right? I
>> did the suggested experiment and the Browser crashes while trying to
>> start the picker.
>>
>> https://codereview.chromium.org/2874493002/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[djacobo, 2017-05-12 21:08:39]

The CQ bit was checked by djacobo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 21:09:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[djacobo, 2017-05-12 21:13:29]

The CQ bit was checked by djacobo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 21:14:28]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[djacobo, 2017-05-12 21:15:27]

On 2017/05/12 01:57:23, sky wrote:
> By rename I mean as set_button_row_insets will no longer be cheap it should
> be changed to SetButtonRowInsets().
> 
> On Thu, May 11, 2017 at 6:56 PM, Scott Violet <mailto:sky@chromium.org> wrote:
> 
> > Ok, in that case please rename set_button_row_insets and have it trigger a
> > layout.
> >
> > On Wed, May 10, 2017 at 11:14 AM, <mailto:djacobo@chromium.org> wrote:
> >
> >>
> >> https://codereview.chromium.org/2874493002/diff/1/chrome/bro
> >> wser/ui/views/intent_picker_bubble_view.cc
> >> File chrome/browser/ui/views/intent_picker_bubble_view.cc (right):
> >>
> >> https://codereview.chromium.org/2874493002/diff/1/chrome/bro
> >> wser/ui/views/intent_picker_bubble_view.cc#newcode111
> >> chrome/browser/ui/views/intent_picker_bubble_view.cc:111:
> >> delegate->GetDialogClientView()->set_button_row_insets(
> >> On 2017/05/10 15:27:02, sky wrote:
> >> > Can this be moved above line 109 such that 113 is not necessary?
> >> Otherwise I
> >> > think set_button_row_insets sure become SetButtonRowInsets() and it
> >> should force
> >> > a layout. In other words you shouldn't have to manually call
> >> > UpdateDialogButtons.
> >>
> >> I don't think that's possible since CreateBubble calls Init, so before
> >> that everything GetDialogClientView is still not constructed, right? I
> >> did the suggested experiment and the Browser crashes while trying to
> >> start the picker.
> >>
> >> https://codereview.chromium.org/2874493002/
> >>
> >
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Alright, reading thru the test cases to see if something breaks.

[djacobo, 2017-05-12 21:15:28]

On 2017/05/12 01:57:23, sky wrote:
> By rename I mean as set_button_row_insets will no longer be cheap it should
> be changed to SetButtonRowInsets().
> 
> On Thu, May 11, 2017 at 6:56 PM, Scott Violet <mailto:sky@chromium.org> wrote:
> 
> > Ok, in that case please rename set_button_row_insets and have it trigger a
> > layout.
> >
> > On Wed, May 10, 2017 at 11:14 AM, <mailto:djacobo@chromium.org> wrote:
> >
> >>
> >> https://codereview.chromium.org/2874493002/diff/1/chrome/bro
> >> wser/ui/views/intent_picker_bubble_view.cc
> >> File chrome/browser/ui/views/intent_picker_bubble_view.cc (right):
> >>
> >> https://codereview.chromium.org/2874493002/diff/1/chrome/bro
> >> wser/ui/views/intent_picker_bubble_view.cc#newcode111
> >> chrome/browser/ui/views/intent_picker_bubble_view.cc:111:
> >> delegate->GetDialogClientView()->set_button_row_insets(
> >> On 2017/05/10 15:27:02, sky wrote:
> >> > Can this be moved above line 109 such that 113 is not necessary?
> >> Otherwise I
> >> > think set_button_row_insets sure become SetButtonRowInsets() and it
> >> should force
> >> > a layout. In other words you shouldn't have to manually call
> >> > UpdateDialogButtons.
> >>
> >> I don't think that's possible since CreateBubble calls Init, so before
> >> that everything GetDialogClientView is still not constructed, right? I
> >> did the suggested experiment and the Browser crashes while trying to
> >> start the picker.
> >>
> >> https://codereview.chromium.org/2874493002/
> >>
> >
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Alright, reading thru the test cases to see if something breaks.

[commit-bot: I haz the power, 2017-05-12 23:56:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-12 23:56:30]

Dry run: This issue passed the CQ dry run.

[sky, 2017-05-14 16:01:16]

https://codereview.chromium.org/2874493002/diff/40001/ui/views/window/dialog_...
File ui/views/window/dialog_client_view.cc (right):

https://codereview.chromium.org/2874493002/diff/40001/ui/views/window/dialog_...
ui/views/window/dialog_client_view.cc:113: if
(button_row_container_->has_children())
Why do you make this conditional on whether there are children? I could checking
if in a widget, but I'm not sure why the children matter.

[djacobo, 2017-05-16 01:10:03]

The CQ bit was checked by djacobo@chromium.org to run a CQ dry run

[djacobo, 2017-05-16 01:10:25]

https://codereview.chromium.org/2874493002/diff/40001/ui/views/window/dialog_...
File ui/views/window/dialog_client_view.cc (right):

https://codereview.chromium.org/2874493002/diff/40001/ui/views/window/dialog_...
ui/views/window/dialog_client_view.cc:113: if
(button_row_container_->has_children())
On 2017/05/14 16:01:16, sky wrote:
> Why do you make this conditional on whether there are children? I could
checking
> if in a widget, but I'm not sure why the children matter.

Initially I thought that not having an ok_button_ or cancel_button_ was the root
cause, (the observable behavior was the browser crashing right after logging
in), however, after some debugging I noticed that the real problem was
SetButtonRowInsets() being called from a view without a widget (which is a valid
case and that would still try to call
https://cs.chromium.org/chromium/src/ui/views/window/dialog_client_view.cc?dr...)

So I can replace line 113 with if (GetWidget()) and that should be enough,
replacing this line. Thanks for the pointer!

[commit-bot: I haz the power, 2017-05-16 01:11:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 02:09:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-16 02:09:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[djacobo, 2017-05-16 17:47:02]

The CQ bit was checked by djacobo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 17:48:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 19:30:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-16 19:30:49]

Dry run: This issue passed the CQ dry run.

[djacobo, 2017-05-17 01:03:35]

The CQ bit was checked by djacobo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 01:05:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 04:46:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-17 04:46:12]

Dry run: This issue passed the CQ dry run.

[sky, 2017-05-17 15:25:17]

LGTM

[djacobo, 2017-05-17 18:11:19]

The CQ bit was checked by djacobo@chromium.org

[commit-bot: I haz the power, 2017-05-17 18:12:45]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 19:35:50]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495044679376500,
"parent_rev": "476235bd668ef183b5e605cc5d0ceaed36586592", "commit_rev":
"15bdd62c05472adc642dcf0982fe4662502ac957"}

[commit-bot: I haz the power, 2017-05-17 19:36:00]

Description was changed from

==========
Make ARC's intent picker use the buttons insets.

Need to call SetupLayout via UpdateDialogButtons so the buttons make
proper use of the established insets.

Bug: 678141
Test: Try.
==========

to

==========
Make ARC's intent picker use the buttons insets.

Need to call SetupLayout via UpdateDialogButtons so the buttons make
proper use of the established insets.

Bug: 678141
Test: Try.
Review-Url: https://codereview.chromium.org/2874493002
Cr-Commit-Position: refs/heads/master@{#472534}
Committed:
https://chromium.googlesource.com/chromium/src/+/15bdd62c05472adc642dcf0982fe...
==========

[commit-bot: I haz the power, 2017-05-17 19:36:02]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/15bdd62c05472adc642dcf0982fe...