[subresource_filter] Ensure desktop "Reload" button is big enough

[subresource_filter] Ensure desktop "Reload" button is big enough

BUG=689992
Review-Url: https://codereview.chromium.org/2804113003
Cr-Commit-Position: refs/heads/master@{#463416}
Committed: https://chromium.googlesource.com/chromium/src/+/a3504f98584ce3e25570b963121fd51c7748b5b3

[Charlie Harrison, 2017-04-06 21:04:37]

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

[commit-bot: I haz the power, 2017-04-06 21:05:36]

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-04-06 21:55:20]

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

[commit-bot: I haz the power, 2017-04-06 21:55:20]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-04-07 00:52:47]

csharrison@chromium.org changed reviewers:
+ msw@chromium.org

[Charlie Harrison, 2017-04-07 00:52:48]

msw: Does this seem like a reasonable solution to you? It is a little bit hacky
and doesn't sit right with me.

[msw, 2017-04-07 10:03:22]

How about instead adding something like this: 
  // Return all strings potentially used for the done button text.
  // Used to ensure the button is adequately wide for all possible strings.
  std::vector<base::string16>BubbleModel::GetDoneButtonStrings();
The subresource-filter subclass would have an override, and
ContentSettingBubbleContents would init the button like this:
  // Ensure the button is adequately wide for all possible strings.
  for (const auto& text : model_->GetDoneButtonStrings()) {
    button->SetText(text);
    ... is the manual sizing really needed now? bummer :(
  }
  button->SetText(/*actual string desired...*/);
It's still kinda lame, but less hacky.

[Charlie Harrison, 2017-04-08 00:44:41]

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

[commit-bot: I haz the power, 2017-04-08 00:45:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2017-04-08 00:50:28]

msw: It looks like we needed a call to Layout() the DialogClientView. The
current CL fixes the issue in the linked bug where even "Reload" does not fit in
the bubble on some configurations.

The current CL does *not* initialize size the button to the max it needs. I got
that working, but it was quite ugly and involved double-layout for every call to
DialogClientView::ViewHierarchyChanged :/

Working on a unit test atm.

[commit-bot: I haz the power, 2017-04-08 01:35:54]

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

[commit-bot: I haz the power, 2017-04-08 01:35:55]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-04-08 15:28:14]

Looks like all the existing unit tests are testing the content setting data
model, not the UI attributes (e.g. actual width).

Is there an easy way to get at the specific button from a browser test or
interactive UI test?

[msw, 2017-04-10 17:43:00]

I guess this is fine if we want to let the button size change when the checkbox
is toggled. You might be able to write a new views-specific unit test for the
bubble to check the actual button size changes, but maybe a browser test is
needed?

[Charlie Harrison, 2017-04-10 17:49:30]

On 2017/04/10 17:43:00, msw wrote:
> I guess this is fine if we want to let the button size change when the
checkbox
> is toggled. You might be able to write a new views-specific unit test for the
> bubble to check the actual button size changes, but maybe a browser test is
> needed?

I was thinking of just landing this quick fix for now (to address the problems
in the linked bug) so we can get it in before branch point.

I can talk to UI folks but I imagine I'll have to come back and do the sizing
properly in M60.

I'll try to work on a test right now but I may need to do it in a follow up if
it ends up being non-trivial.

Does this sound OK to you?

[msw, 2017-04-10 17:54:40]

On 2017/04/10 17:49:30, Charlie Harrison wrote:
> On 2017/04/10 17:43:00, msw wrote:
> > I guess this is fine if we want to let the button size change when the
> checkbox
> > is toggled. You might be able to write a new views-specific unit test for
the
> > bubble to check the actual button size changes, but maybe a browser test is
> > needed?
> 
> I was thinking of just landing this quick fix for now (to address the problems
> in the linked bug) so we can get it in before branch point.
> 
> I can talk to UI folks but I imagine I'll have to come back and do the sizing
> properly in M60.
> 
> I'll try to work on a test right now but I may need to do it in a follow up if
> it ends up being non-trivial.
> 
> Does this sound OK to you?

yup, sgtm and the patch lgtm as-is for now.

[Charlie Harrison, 2017-04-10 21:17:17]

I'll go ahead and land this now. I have a unit test that works, but it's complex
enough that I suspect it'll take a day or two to land. Will likely send that out
for review today if I can get it presentable :)

[Charlie Harrison, 2017-04-10 21:17:25]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2017-04-10 21:17:56]

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-04-10 22:10:48]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491859045468130,
"parent_rev": "33c5ac234dc93a3cfc31fda63fe716d8d2a40410", "commit_rev":
"a3504f98584ce3e25570b963121fd51c7748b5b3"}

[commit-bot: I haz the power, 2017-04-10 22:11:48]

Description was changed from

==========
[subresource_filter] Ensure desktop "Reload" button is big enough

BUG=689992
==========

to

==========
[subresource_filter] Ensure desktop "Reload" button is big enough

BUG=689992

Review-Url: https://codereview.chromium.org/2804113003
Cr-Commit-Position: refs/heads/master@{#463416}
Committed:
https://chromium.googlesource.com/chromium/src/+/a3504f98584ce3e25570b963121f...
==========

[commit-bot: I haz the power, 2017-04-10 22:11:52]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/a3504f98584ce3e25570b963121f...