Restrict width of the Subresource Filtering bubble to 320px.

Restrict width of the Subresource Filtering bubble to 320px.

BUG=647248
Committed: https://crrev.com/39e03e7ea798b1ea83bc0b87c57bba62a0ddb24e
Cr-Commit-Position: refs/heads/master@{#420719}

[melandory, 2016-09-15 14:19:25]

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

[commit-bot: I haz the power, 2016-09-15 14:19:35]

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

[melandory, 2016-09-15 14:52:38]

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

[melandory, 2016-09-15 14:52:38]

PTAL, screenshot attached to the bug.

[commit-bot: I haz the power, 2016-09-15 15:30:44]

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

[commit-bot: I haz the power, 2016-09-15 15:30:45]

Dry run: This issue passed the CQ dry run.

[msw, 2016-09-15 18:29:04]

https://codereview.chromium.org/2344663003/diff/1/chrome/browser/ui/views/con...
File chrome/browser/ui/views/content_setting_bubble_contents.cc (right):

https://codereview.chromium.org/2344663003/diff/1/chrome/browser/ui/views/con...
chrome/browser/ui/views/content_setting_bubble_contents.cc:50: const int
kMaxContentsWidth = 500;
Should we just change the existing maximum width? Please work with someone to
consider the behavior of this bubble in the context of all the other content
setting bubbles. This bubble probably shouldn't get a lot of unique treatments
(link->button, special width, etc.).

https://codereview.chromium.org/2344663003/diff/1/chrome/browser/ui/views/con...
chrome/browser/ui/views/content_setting_bubble_contents.cc:56: // Subresource
filter bubble has ling explanation string, so the width should be
nit: 'a long' consider: 'Maximum width of the subresource filter prompt, which
has a long string.'

https://codereview.chromium.org/2344663003/diff/1/chrome/browser/ui/views/con...
chrome/browser/ui/views/content_setting_bubble_contents.cc:189:
preferred_size.set_width(
nit: consider:
  preferred_size.set_width(std::min(preferred_width, kMaxContentsWidth));
  // Clamp the subresource filter prompt even further, because ... <insert text
explaining why this bubble should be less wide than other content bubbles, which
isn't really obvious>
  if (content_setting_bubble_model_->AsSubresourceFilterBubbleModel())
    preferred_size.set_width(std::min(preferred_width,
kMaxSubresourceFilterPromptWidth));

[melandory, 2016-09-15 19:47:11]

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

[melandory, 2016-09-15 19:47:44]

> Should we just change the existing maximum width? Please work with someone to
> consider the behavior of this bubble in the context of all the other content
> setting bubbles. This bubble probably shouldn't get a lot of unique treatments
> (link->button, special width, etc.).
I think this part of UI is active redesign. Adding Max, since he is more
knowledgeable.

https://codereview.chromium.org/2344663003/diff/1/chrome/browser/ui/views/con...
File chrome/browser/ui/views/content_setting_bubble_contents.cc (right):

https://codereview.chromium.org/2344663003/diff/1/chrome/browser/ui/views/con...
chrome/browser/ui/views/content_setting_bubble_contents.cc:56: // Subresource
filter bubble has ling explanation string, so the width should be
On 2016/09/15 18:29:03, msw wrote:
> nit: 'a long' consider: 'Maximum width of the subresource filter prompt, which
> has a long string.'

Done.

https://codereview.chromium.org/2344663003/diff/1/chrome/browser/ui/views/con...
chrome/browser/ui/views/content_setting_bubble_contents.cc:189:
preferred_size.set_width(
On 2016/09/15 18:29:03, msw wrote:
> nit: consider:
>   preferred_size.set_width(std::min(preferred_width, kMaxContentsWidth));
>   // Clamp the subresource filter prompt even further, because ... <insert
text
> explaining why this bubble should be less wide than other content bubbles,
which
> isn't really obvious>
>   if (content_setting_bubble_model_->AsSubresourceFilterBubbleModel())
>     preferred_size.set_width(std::min(preferred_width,
> kMaxSubresourceFilterPromptWidth));

Done.

[commit-bot: I haz the power, 2016-09-15 19:47:45]

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, 2016-09-15 20:29:08]

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

[commit-bot: I haz the power, 2016-09-15 20:29:09]

Dry run: This issue passed the CQ dry run.

[maxwalker, 2016-09-16 08:32:39]

> I think this part of UI is active redesign. Adding Max, since he is more
> knowledgeable.

Project Harmony aims to make dialogs more consistent. A part of this is to size
dialogs to 320px by default (hey may use the 448px or 512px sizes as needed).
https://docs.google.com/presentation/d/17CDjsa8u0rhgjV0zTCJMwY1z5BbaaXKiJqBL_...

[msw, 2016-09-16 16:59:44]

On 2016/09/16 08:32:39, maxwalker wrote:
> > I think this part of UI is active redesign. Adding Max, since he is more
> > knowledgeable.
> 
> Project Harmony aims to make dialogs more consistent. A part of this is to
size
> dialogs to 320px by default (hey may use the 448px or 512px sizes as needed).
>
https://docs.google.com/presentation/d/17CDjsa8u0rhgjV0zTCJMwY1z5BbaaXKiJqBL_...

Ok, please either make all content settings bubbles 320px wide, or audit them to
see if any need to be 448/512px. That's a much better fix than addressing each
of these bubbles  one-by-one over time.

[melandory, 2016-09-21 15:11:57]

On 2016/09/16 16:59:44, msw wrote:
> On 2016/09/16 08:32:39, maxwalker wrote:
> > > I think this part of UI is active redesign. Adding Max, since he is more
> > > knowledgeable.
> > 
> > Project Harmony aims to make dialogs more consistent. A part of this is to
> size
> > dialogs to 320px by default (hey may use the 448px or 512px sizes as
needed).
> >
>
https://docs.google.com/presentation/d/17CDjsa8u0rhgjV0zTCJMwY1z5BbaaXKiJqBL_...
> 
> Ok, please either make all content settings bubbles 320px wide, or audit them
to
> see if any need to be 448/512px. That's a much better fix than addressing each
> of these bubbles  one-by-one over time.
It seems that auditing all content setting bubbles is long process (and
potentially unnecessary, because Harmony will eventually change them).
On the other hand our feature is blocked on the length of the bubble, because UI
review thinks that long bubble in this case is unacceptable.
What about following: we proceed with this solution and I'll raise the question
about auditing content setting bubble length with UX team?

[msw, 2016-09-21 15:46:31]

On 2016/09/21 15:11:57, melandory wrote:
> On 2016/09/16 16:59:44, msw wrote:
> > On 2016/09/16 08:32:39, maxwalker wrote:
> > > > I think this part of UI is active redesign. Adding Max, since he is more
> > > > knowledgeable.
> > > 
> > > Project Harmony aims to make dialogs more consistent. A part of this is to
> > size
> > > dialogs to 320px by default (hey may use the 448px or 512px sizes as
> needed).
> > >
> >
>
https://docs.google.com/presentation/d/17CDjsa8u0rhgjV0zTCJMwY1z5BbaaXKiJqBL_...
> > 
> > Ok, please either make all content settings bubbles 320px wide, or audit
them
> to
> > see if any need to be 448/512px. That's a much better fix than addressing
each
> > of these bubbles  one-by-one over time.
> It seems that auditing all content setting bubbles is long process (and
> potentially unnecessary, because Harmony will eventually change them).
> On the other hand our feature is blocked on the length of the bubble, because
UI
> review thinks that long bubble in this case is unacceptable.
> What about following: we proceed with this solution and I'll raise the
question
> about auditing content setting bubble length with UX team?

Rename the variable and update the comments to reflect that this is simply the
"new default kMaxContentsWidth", not some special value for the subresource
filter prompt; reorder the constant directly below kMaxContentsWidth. Please
also file a bug and add a TODO to audit/update other bubbles.

[melandory, 2016-09-23 11:11:26]

On 2016/09/21 15:46:31, msw on vacation 9-22 wrote:
> On 2016/09/21 15:11:57, melandory wrote:
> > On 2016/09/16 16:59:44, msw wrote:
> > > On 2016/09/16 08:32:39, maxwalker wrote:
> > > > > I think this part of UI is active redesign. Adding Max, since he is
more
> > > > > knowledgeable.
> > > > 
> > > > Project Harmony aims to make dialogs more consistent. A part of this is
to
> > > size
> > > > dialogs to 320px by default (hey may use the 448px or 512px sizes as
> > needed).
> > > >
> > >
> >
>
https://docs.google.com/presentation/d/17CDjsa8u0rhgjV0zTCJMwY1z5BbaaXKiJqBL_...
> > > 
> > > Ok, please either make all content settings bubbles 320px wide, or audit
> them
> > to
> > > see if any need to be 448/512px. That's a much better fix than addressing
> each
> > > of these bubbles  one-by-one over time.
> > It seems that auditing all content setting bubbles is long process (and
> > potentially unnecessary, because Harmony will eventually change them).
> > On the other hand our feature is blocked on the length of the bubble,
because
> UI
> > review thinks that long bubble in this case is unacceptable.
> > What about following: we proceed with this solution and I'll raise the
> question
> > about auditing content setting bubble length with UX team?
> 
> Rename the variable and update the comments to reflect that this is simply the
> "new default kMaxContentsWidth", not some special value for the subresource
> filter prompt; reorder the constant directly below kMaxContentsWidth. Please
> also file a bug and add a TODO to audit/update other bubbles.

Done.

[melandory, 2016-09-23 11:12:11]

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

[commit-bot: I haz the power, 2016-09-23 11:12:26]

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, 2016-09-23 11:27:12]

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

[commit-bot: I haz the power, 2016-09-23 11:27:13]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[msw, 2016-09-23 14:09:28]

lgtm

[melandory, 2016-09-23 19:15:08]

The CQ bit was checked by melandory@chromium.org

[commit-bot: I haz the power, 2016-09-23 19:15:46]

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, 2016-09-23 20:38:21]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-23 20:40:33]

Description was changed from

==========
Restrict width of the Subresource Filtering bubble to 320px.

BUG=647248
==========

to

==========
Restrict width of the Subresource Filtering bubble to 320px.

BUG=647248

Committed: https://crrev.com/39e03e7ea798b1ea83bc0b87c57bba62a0ddb24e
Cr-Commit-Position: refs/heads/master@{#420719}
==========

[commit-bot: I haz the power, 2016-09-23 20:40:35]

Patchset 3 (id:??) landed as
https://crrev.com/39e03e7ea798b1ea83bc0b87c57bba62a0ddb24e
Cr-Commit-Position: refs/heads/master@{#420719}