Add a Certificate Viewer link to the Page Info dropdown

Add a Certificate Viewer link to the Page Info dropdown

In Chrome 55, we removed the easy path from Page Info to the
Certificate Viewer dialog. This change puts it back in a new
Certificate view just above the Cookie view.

BUG=663971

[elawrence, 2017-03-10 19:56:57]

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

[commit-bot: I haz the power, 2017-03-10 19:57:44]

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-03-10 20:41:21]

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

[commit-bot: I haz the power, 2017-03-10 20:41:22]

Dry run: This issue passed the CQ dry run.

[elawrence, 2017-03-31 20:50:53]

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

[elawrence, 2017-03-31 20:51:23]

Patchset #3 (id:40001) has been deleted

[elawrence, 2017-03-31 20:51:31]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-03-31 20:51:46]

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

[elawrence, 2017-03-31 20:52:47]

elawrence@chromium.org changed reviewers:
+ lgarron@chromium.org

[elawrence, 2017-03-31 20:52:48]

PTAL at the PageInfo code?

[commit-bot: I haz the power, 2017-03-31 22:05:02]

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

[commit-bot: I haz the power, 2017-03-31 22:05:02]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[lgarron, 2017-04-05 02:06:26]

Sorry I didn't get to this yet.
I left some comments, but looks on the right track.

My main concerns is that this CL currently copies the Cookies code manually for
Cocoa/Views even though they have the same layout, and I'd really like to avoid
adding technical debt like that.
Do you think you'd be able to unify them on each platform be factoring out a
class for it?

https://codereview.chromium.org/2737413004/diff/60001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2737413004/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7131: <message
name="IDS_PAGE_INFO_CERTIFICATE" desc="Title of the certificate area in the Page
Information Window, shown when a HTTPS site is loaded.">
Could you add these to pageinfo_strings.grdp instead?
https://crbug.com/663975

https://codereview.chromium.org/2737413004/diff/60001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2737413004/diff/60001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2360: #endif  // defined(OS_CHROMEOS)
Nit: don't change indentation of unrelated code (maybe this is due to some rogue
`git cl format`ing?)

https://codereview.chromium.org/2737413004/diff/60001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2493: {"show-cert-button",
flag_descriptions::kShowCertButtonOnPageInfoName,
Nit: If it's going to be a link in the foreseeable future, could we call this
kShowCertLinkOnPageInfoName?

https://codereview.chromium.org/2737413004/diff/60001/chrome/browser/flag_des...
File chrome/browser/flag_descriptions.cc (right):

https://codereview.chromium.org/2737413004/diff/60001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.cc:1528: const char
kShowCertButtonOnPageInfoName[] = "Show certificate button";
ditto

https://codereview.chromium.org/2737413004/diff/60001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.cc:1531: "Shows a button that launches the
certificate viewer from the Page "
I would write: Add a link from the Page Info bubble to the certificate viewer
for HTTPS sites.

https://codereview.chromium.org/2737413004/diff/60001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.cc:1532: "Information dropdown window on HTTPS
sites.";
I don't think we call it "dropdown window" anywhere? I would prefer to avoid
introducing any new names for Page Info if possible, so I'd prefer "Page
Info[rmation] bubble".

https://codereview.chromium.org/2737413004/diff/60001/chrome/browser/flag_des...
File chrome/browser/flag_descriptions.h (right):

https://codereview.chromium.org/2737413004/diff/60001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.h:1670: extern const char
kShowCertButtonOnPageInfoName[];
ditto

https://codereview.chromium.org/2737413004/diff/60001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm (right):

https://codereview.chromium.org/2737413004/diff/60001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm:727: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(
Could you move this check into a platform-independent helper in page_info_ui.h?

Similar to GetPermissionDecisionTextColor() in
https://codereview.chromium.org/2743423004; I want to unify cross-platform
calculations as much as possible:
https://docs.google.com/document/d/12YnhxsNd-o82DfsyPpHRKM2yuXx7XOoKqaPt94CL9...

[elawrence, 2017-04-05 15:26:19]

Thanks for having a look.

> My main concerns is that this CL currently copies the Cookies code manually
for
> Cocoa/Views even though they have the same layout, and I'd really like to
avoid
> adding technical debt like that.
> Do you think you'd be able to unify them on each platform be factoring out a
> class for it?

Do you know the timeline for when Cocoa is slated to be removed? Does it make
sense to do a refactoring there, or would it make more sense do it only in
Views, which we expect to live on?

It's not quite correct to say the current iteration "copies the Cookies code
manually"-- while there is some duplicated layout goo (creating the row and
columns), the code for computing the strings and icon is meaningfully different.


Is there a reason to create a class vs. just adding a helper function that takes
a Title, LinkText, LinkID, and Icon?

[elawrence, 2017-04-05 23:20:19]

Patchset #3 (id:80001) has been deleted

[elawrence, 2017-04-06 01:06:11]

https://codereview.chromium.org/2737413004/diff/60001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/2737413004/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7131: <message
name="IDS_PAGE_INFO_CERTIFICATE" desc="Title of the certificate area in the Page
Information Window, shown when a HTTPS site is loaded.">
On 2017/04/05 02:06:25, lgarron wrote:
> Could you add these to pageinfo_strings.grdp instead?
> https://crbug.com/663975

Done.

https://codereview.chromium.org/2737413004/diff/60001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/2737413004/diff/60001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2360: #endif  // defined(OS_CHROMEOS)
On 2017/04/05 02:06:25, lgarron wrote:
> Nit: don't change indentation of unrelated code (maybe this is due to some
rogue
> `git cl format`ing?)

This is from git cl lint. I wasn't able to quickly find the documented best
practice here? Is it to just do a different CL that fixes linting problems?

https://codereview.chromium.org/2737413004/diff/60001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:2493: {"show-cert-button",
flag_descriptions::kShowCertButtonOnPageInfoName,
On 2017/04/05 02:06:25, lgarron wrote:
> Nit: If it's going to be a link in the foreseeable future, could we call this
> kShowCertLinkOnPageInfoName?

I suppose it's a link today and likely to remain that way. I didn't think too
hard about the flag naming because the flag itself is slated to go away in M60.

https://codereview.chromium.org/2737413004/diff/60001/chrome/browser/flag_des...
File chrome/browser/flag_descriptions.cc (right):

https://codereview.chromium.org/2737413004/diff/60001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.cc:1528: const char
kShowCertButtonOnPageInfoName[] = "Show certificate button";
On 2017/04/05 02:06:25, lgarron wrote:
> ditto

Done.

https://codereview.chromium.org/2737413004/diff/60001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.cc:1531: "Shows a button that launches the
certificate viewer from the Page "
On 2017/04/05 02:06:25, lgarron wrote:
> I would write: Add a link from the Page Info bubble to the certificate viewer
> for HTTPS sites.

Done.

https://codereview.chromium.org/2737413004/diff/60001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.cc:1532: "Information dropdown window on HTTPS
sites.";
On 2017/04/05 02:06:25, lgarron wrote:
> I don't think we call it "dropdown window" anywhere? I would prefer to avoid
> introducing any new names for Page Info if possible, so I'd prefer "Page
> Info[rmation] bubble".

Done.

https://codereview.chromium.org/2737413004/diff/60001/chrome/browser/flag_des...
File chrome/browser/flag_descriptions.h (right):

https://codereview.chromium.org/2737413004/diff/60001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.h:1670: extern const char
kShowCertButtonOnPageInfoName[];
On 2017/04/05 02:06:25, lgarron wrote:
> ditto

Done.

[lgarron, 2017-04-06 19:33:59]

On 2017/04/05 at 15:26:19, elawrence wrote:
> Thanks for having a look.
> 
> > My main concerns is that this CL currently copies the Cookies code manually
for
> > Cocoa/Views even though they have the same layout, and I'd really like to
avoid
> > adding technical debt like that.
> > Do you think you'd be able to unify them on each platform be factoring out a
> > class for it?
> 
> Do you know the timeline for when Cocoa is slated to be removed? Does it make
sense to do a refactoring there, or would it make more sense do it only in
Views, which we expect to live on?

Unclear. 6 months ago it was "at least 6 months from now", and there are still a
bunch of bugs.

> 
> It's not quite correct to say the current iteration "copies the Cookies code
manually"-- while there is some duplicated layout goo (creating the row and
columns), the code for computing the strings and icon is meaningfully different.


Right, not a literal copy but a manual one? :-P

> 
> Is there a reason to create a class vs. just adding a helper function that
takes a Title, LinkText, LinkID, and Icon?

A helper function would be in line with the Cocoa code (but it's not exactly a
shining beacon of code organization) – I don't care too much there.

My impression of Views is that it's preferable to wrap anything that should be
isolated/reusable in its own view. Since that code will stick around
indefinitely, I think we should definitely factor it out.

[elawrence, 2017-04-19 20:59:58]

Patchset #4 (id:120001) has been deleted

[elawrence, 2017-04-19 21:08:50]

Please take another look? I've added Subviews for the Cookie/Certificate
subsections. 

The Views code now feels a lot cleaner. (Cocoa less so, because most of the UI
helpers live as instance members on PageInfoBubbleController and cleaning those
up into static helpers elsewhere seems like a larger refactor of dubious value
for code not expected to live indefinitely.)

thanks!

[lgarron, 2017-04-20 19:12:56]

I think the Cocoa approach in this CL is pretty good given the existing code!

I have a question about the tooltip, but the rest looks ready for review by
rsesek@ and msw@. Up to you if you want to do that all in one CL.

https://codereview.chromium.org/2737413004/diff/140001/chrome/browser/flag_de...
File chrome/browser/flag_descriptions.cc (right):

https://codereview.chromium.org/2737413004/diff/140001/chrome/browser/flag_de...
chrome/browser/flag_descriptions.cc:201: //  Flag and values for MHTML Generator
options lab.
Nit: I like to put conceptually unrelated fixes in a separate CL.

https://codereview.chromium.org/2737413004/diff/140001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.h (right):

https://codereview.chromium.org/2737413004/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.h:42: // are
displayed when the user clicks the Page Info icon.
Nit: I usually call this the "omnibox security indicator" (although admittedly
that's a bit vague).

https://codereview.chromium.org/2737413004/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/page_info/page_info_popup_view.cc (right):

https://codereview.chromium.org/2737413004/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/page_info/page_info_popup_view.cc:734:
certificate_view_->SetLinkTooltip(issuer);
Is it common to add information like this in a tooltip?
I would probably run it by UI if you haven't yet.

https://codereview.chromium.org/2737413004/diff/140001/components/pageinfo_st...
File components/pageinfo_strings.grdp (right):

https://codereview.chromium.org/2737413004/diff/140001/components/pageinfo_st...
components/pageinfo_strings.grdp:4: <!-- Certificate Viewer link -->
FYI: This file was renamed; you'll have to rebase at least once before landing.

[elawrence, 2017-04-27 14:56:34]

https://codereview.chromium.org/2737413004/diff/140001/chrome/browser/flag_de...
File chrome/browser/flag_descriptions.cc (right):

https://codereview.chromium.org/2737413004/diff/140001/chrome/browser/flag_de...
chrome/browser/flag_descriptions.cc:201: //  Flag and values for MHTML Generator
options lab.
On 2017/04/20 19:12:56, lgarron wrote:
> Nit: I like to put conceptually unrelated fixes in a separate CL.

Okay, I've re-misspelled this.

https://codereview.chromium.org/2737413004/diff/140001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.h (right):

https://codereview.chromium.org/2737413004/diff/140001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.h:42: // are
displayed when the user clicks the Page Info icon.
On 2017/04/20 19:12:56, lgarron wrote:
> Nit: I usually call this the "omnibox security indicator" (although admittedly
> that's a bit vague).

Done.

https://codereview.chromium.org/2737413004/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/page_info/page_info_popup_view.cc (right):

https://codereview.chromium.org/2737413004/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/page_info/page_info_popup_view.cc:734:
certificate_view_->SetLinkTooltip(issuer);
On 2017/04/20 19:12:56, lgarron wrote:
> Is it common to add information like this in a tooltip?
> I would probably run it by UI if you haven't yet.

Done.

https://codereview.chromium.org/2737413004/diff/140001/components/pageinfo_st...
File components/pageinfo_strings.grdp (right):

https://codereview.chromium.org/2737413004/diff/140001/components/pageinfo_st...
components/pageinfo_strings.grdp:4: <!-- Certificate Viewer link -->
On 2017/04/20 19:12:56, lgarron wrote:
> FYI: This file was renamed; you'll have to rebase at least once before
landing.

Acknowledged.

[elawrence, 2017-04-27 14:59:08]

Patchset #1 (id:1) has been deleted

[elawrence, 2017-04-27 14:59:36]

Patchset #2 (id:100001) has been deleted

[elawrence, 2017-04-27 15:00:14]

Description was changed from

==========
Add a Certificate Viewer link to the Page Info dropdown

In Chrome 55, we removed the easy path from Page Info to the
Certificate Viewer dialog. This change puts it back in a new
Certificate view just above the Cookie view.

BUG=663971
==========

to

==========
Add a Certificate Viewer link to the Page Info dropdown

In Chrome 55, we removed the easy path from Page Info to the
Certificate Viewer dialog. This change puts it back in a new
Certificate view just above the Cookie view.

BUG=663971
==========

[elawrence, 2017-04-27 15:00:14]

elawrence@chromium.org changed reviewers:
- lgarron@chromium.org