Make 'Load full site' a button instead of link.

Make 'Load full site' a button instead of link.

BUG=646762
Committed: https://crrev.com/2c2451438542a03100d0f9b56d0397a49421b4fd
Cr-Commit-Position: refs/heads/master@{#418697}

[melandory, 2016-09-14 10:54:13]

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

[commit-bot: I haz the power, 2016-09-14 10:54:21]

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-14 10:55:25]

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

[commit-bot: I haz the power, 2016-09-14 10:55:33]

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-14 11:09:21]

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

[commit-bot: I haz the power, 2016-09-14 11:09: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-14 11:10:59]

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

[melandory, 2016-09-14 11:11:00]

PTAL, screenshot in is attachment to the bug.

[melandory, 2016-09-14 11:38:18]

Patchset #1 (id:1) has been deleted

[melandory, 2016-09-14 11:38:27]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-09-14 12:08:09]

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

[commit-bot: I haz the power, 2016-09-14 12:08:10]

Dry run: This issue passed the CQ dry run.

[msw, 2016-09-14 17:01:34]

Please fix your CL title and description... button/link is inverted.

https://codereview.chromium.org/2339783003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/content_setting_bubble_contents.cc (right):

https://codereview.chromium.org/2339783003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/content_setting_bubble_contents.cc:32: #include
"ui/views/controls/button/label_button.h"
nit: This include is redundant

https://codereview.chromium.org/2339783003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/content_setting_bubble_contents.cc:166:
manage_button_(NULL),
nit: nullptr here and elsewhere

https://codereview.chromium.org/2339783003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/content_setting_bubble_contents.cc:387: if
(content_setting_bubble_model_->bubble_content().manage_link_as_button) {
Why make this an option? Why not just always use a button?

https://codereview.chromium.org/2339783003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/content_setting_bubble_contents.cc:390:
content_setting_bubble_model_->bubble_content().manage_link));
nit: rename |BubbleContent::manage_link| to |manage_text| or similar.

[melandory, 2016-09-14 19:30:12]

https://codereview.chromium.org/2339783003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/content_setting_bubble_contents.cc (right):

https://codereview.chromium.org/2339783003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/content_setting_bubble_contents.cc:32: #include
"ui/views/controls/button/label_button.h"
On 2016/09/14 17:01:34, msw wrote:
> nit: This include is redundant

Done.

https://codereview.chromium.org/2339783003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/content_setting_bubble_contents.cc:166:
manage_button_(NULL),
On 2016/09/14 17:01:34, msw wrote:
> nit: nullptr here and elsewhere

Done.

https://codereview.chromium.org/2339783003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/content_setting_bubble_contents.cc:387: if
(content_setting_bubble_model_->bubble_content().manage_link_as_button) {
On 2016/09/14 17:01:34, msw wrote:
> Why make this an option? Why not just always use a button?

It was set as a requirement for our particular bubble, that it "manage link"
should be button, because it effectively reloads the page. For other content
setting the link might be more appropriate, because clicking on it opens setting
page.
Although it looks like button on OSX. So I don't really have strong opinion. I
can ask our UX designer to raise this point and maybe change them all to be
buttons.
For now I prefer to distinguish link and button and do not wait until decision
about all bubbles is made.

https://codereview.chromium.org/2339783003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/content_setting_bubble_contents.cc:390:
content_setting_bubble_model_->bubble_content().manage_link));
On 2016/09/14 17:01:34, msw wrote:
> nit: rename |BubbleContent::manage_link| to |manage_text| or similar.

Done.

[melandory, 2016-09-14 19:31:54]

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

[commit-bot: I haz the power, 2016-09-14 19:33:30]

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

[msw, 2016-09-14 19:50:24]

Again: please fix your CL title and description...

https://codereview.chromium.org/2339783003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/content_setting_bubble_contents.cc (right):

https://codereview.chromium.org/2339783003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/content_setting_bubble_contents.cc:387: if
(content_setting_bubble_model_->bubble_content().manage_link_as_button) {
On 2016/09/14 19:30:12, melandory wrote:
> On 2016/09/14 17:01:34, msw wrote:
> > Why make this an option? Why not just always use a button?
> 
> It was set as a requirement for our particular bubble, that it "manage link"
> should be button, because it effectively reloads the page. For other content
> setting the link might be more appropriate, because clicking on it opens
setting
> page.
> Although it looks like button on OSX. So I don't really have strong opinion. I
> can ask our UX designer to raise this point and maybe change them all to be
> buttons.
> For now I prefer to distinguish link and button and do not wait until decision
> about all bubbles is made. 

Acknowledged.

https://codereview.chromium.org/2339783003/diff/60001/chrome/browser/ui/conte...
File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right):

https://codereview.chromium.org/2339783003/diff/60001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model.h:124: bool
manage_text_as_button;
nit: show_manage_text_as_button (similar for setter)

https://codereview.chromium.org/2339783003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/content_setting_bubble_contents.cc (right):

https://codereview.chromium.org/2339783003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/content_setting_bubble_contents.cc:387: manage_button_ =
 views::MdTextButton::CreateSecondaryUiButton(
nit: remove extra space after '='; use "git cl format"

[commit-bot: I haz the power, 2016-09-14 19:52:12]

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

[commit-bot: I haz the power, 2016-09-14 19:52:13]

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

[melandory, 2016-09-14 19:57:44]

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

[commit-bot: I haz the power, 2016-09-14 19:59:06]

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-14 20:02:46]

Description was changed from

==========
Make 'Load full site' a link instead of button

BUG=646762
==========

to

==========
Make 'Load full site' a button instead of link.

BUG=646762
==========

[melandory, 2016-09-14 20:27:51]

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

[melandory, 2016-09-14 20:27:58]

https://codereview.chromium.org/2339783003/diff/60001/chrome/browser/ui/conte...
File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right):

https://codereview.chromium.org/2339783003/diff/60001/chrome/browser/ui/conte...
chrome/browser/ui/content_settings/content_setting_bubble_model.h:124: bool
manage_text_as_button;
On 2016/09/14 19:50:24, msw wrote:
> nit: show_manage_text_as_button (similar for setter)

Done.

https://codereview.chromium.org/2339783003/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/content_setting_bubble_contents.cc (right):

https://codereview.chromium.org/2339783003/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/content_setting_bubble_contents.cc:387: manage_button_ =
 views::MdTextButton::CreateSecondaryUiButton(
On 2016/09/14 19:50:24, msw wrote:
> nit: remove extra space after '='; use "git cl format"

Done.

[commit-bot: I haz the power, 2016-09-14 20:28:25]

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-14 20:50:08]

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

[commit-bot: I haz the power, 2016-09-14 20:50:09]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[msw, 2016-09-14 21:00:59]

lgtm

[melandory, 2016-09-14 21:27:21]

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

[commit-bot: I haz the power, 2016-09-14 21:27:59]

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-14 22:21:46]

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

[commit-bot: I haz the power, 2016-09-14 22:21:47]

Dry run: This issue passed the CQ dry run.

[melandory, 2016-09-14 22:24:32]

The CQ bit was checked by melandory@chromium.org

[melandory, 2016-09-14 22:24:32]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2339783003/#ps140001
(title: ".")

[commit-bot: I haz the power, 2016-09-14 22:25:00]

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-14 22:30:32]

Description was changed from

==========
Make 'Load full site' a button instead of link.

BUG=646762
==========

to

==========
Make 'Load full site' a button instead of link.

BUG=646762
==========

[commit-bot: I haz the power, 2016-09-14 22:30:33]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-09-14 22:32:10]

Description was changed from

==========
Make 'Load full site' a button instead of link.

BUG=646762
==========

to

==========
Make 'Load full site' a button instead of link.

BUG=646762

Committed: https://crrev.com/2c2451438542a03100d0f9b56d0397a49421b4fd
Cr-Commit-Position: refs/heads/master@{#418697}
==========

[commit-bot: I haz the power, 2016-09-14 22:32:11]

Patchset 6 (id:??) landed as
https://crrev.com/2c2451438542a03100d0f9b56d0397a49421b4fd
Cr-Commit-Position: refs/heads/master@{#418697}