Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2846913002:
Add a Certificate Viewer link to the Page Info dropdown (Closed)
|
Created:
3 years, 7 months ago by elawrence Modified:
3 years, 7 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews, lgarron+watch_chromium.org, mac-reviews_chromium.org, oshima+watch_chromium.org, raymes+watch_chromium.org, tfarina Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd 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
Review-Url: https://codereview.chromium.org/2846913002
Cr-Commit-Position: refs/heads/master@{#468539}
Committed: https://chromium.googlesource.com/chromium/src/+/d9978fcc021c14d5260d1abffc2d0b39e41018db
Patch Set 1 #
Total comments: 21
Patch Set 2 : Address review feedback #
Total comments: 5
Patch Set 3 : Fix nit #Patch Set 4 : Replace View-derived class with factory function #Patch Set 5 : Omit Vector icons on Android #
Total comments: 10
Patch Set 6 : Address nits #
Total comments: 18
Patch Set 7 : Address feedback #
Total comments: 1
Patch Set 8 : Rename Views static function #Patch Set 9 : Fix merge conflict #Patch Set 10 : Fix merge conflict #Messages
Total messages: 55 (30 generated)
elawrence@chromium.org changed reviewers: + lgarron@chromium.org, mpearson@chromium.org, msw@chromium.org, oshima@chromium.org, rsesek@chromium.org
oshima@: Please review changes in chrome/app/theme/* rsesek@: Please review changes in chrome/browser/ui/cocoa/* msw@: Please review changes in chrome/browser/ui/page_info/* and chrome/browser/ui/views/* lgarron@: Please review components/page_info_strings.grdp, chrome/browser/*flag*, chrome/common/chrome_switches.* and re-review anything if you like. mpearson@: Please review the change in histograms.xml
On 2017/04/27 15:42:42, elawrence wrote: > oshima@: Please review changes in chrome/app/theme/* can you use vector icon instead? (https://www.chromium.org/developers/how-tos/vectorized-icons-in-native-chrome-ui) Note to self: I probably should add presubmit check. > rsesek@: Please review changes in chrome/browser/ui/cocoa/* > > msw@: Please review changes in chrome/browser/ui/page_info/* and > chrome/browser/ui/views/* > > lgarron@: Please review components/page_info_strings.grdp, > chrome/browser/*flag*, chrome/common/chrome_switches.* and re-review anything if > you like. > > mpearson@: Please review the change in histograms.xml
https://codereview.chromium.org/2846913002/diff/1/chrome/browser/ui/cocoa/pag... File chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.h (right): https://codereview.chromium.org/2846913002/diff/1/chrome/browser/ui/cocoa/pag... chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.h:33: @interface InspectLinkView : FlippedView { You can forward declare this with @class and put it entirely in the .mm file. https://codereview.chromium.org/2846913002/diff/1/chrome/browser/ui/cocoa/pag... chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.h:33: @interface InspectLinkView : FlippedView { Please give this a brief top-level comment. https://codereview.chromium.org/2846913002/diff/1/chrome/browser/ui/cocoa/pag... chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.h:34: @private After this moves into the .mm file, you can also declare the instance variables in the @implementation: @interface InspectLinkView : FlippedView @end @implementation InspectLinkView () { NSButton* actionLink_; } ... @end https://codereview.chromium.org/2846913002/diff/1/chrome/browser/ui/cocoa/pag... chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.h:38: - (id)initWithFrame:(NSRect)frame; You can omit common designated initializers for view subclasses. https://codereview.chromium.org/2846913002/diff/1/chrome/browser/ui/cocoa/pag... File chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm (right): https://codereview.chromium.org/2846913002/diff/1/chrome/browser/ui/cocoa/pag... chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm:146: [self setSubviews:[NSArray array]]; Is this necessary? https://codereview.chromium.org/2846913002/diff/1/chrome/browser/ui/cocoa/pag... chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm:851: if (isValid) nit: braces
https://codereview.chromium.org/2846913002/diff/1/chrome/browser/ui/views/pag... File chrome/browser/ui/views/page_info/page_info_bubble_view.cc (right): https://codereview.chromium.org/2846913002/diff/1/chrome/browser/ui/views/pag... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:309: // |InspectLinkView| is a UI element (view) that shows information and nit: remove copied comment, the class comment above is sufficient https://codereview.chromium.org/2846913002/diff/1/chrome/browser/ui/views/pag... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:356: InvalidateLayout(); q: Is this needed? Perhaps GridLayout just does the right thing without this? https://codereview.chromium.org/2846913002/diff/1/chrome/browser/ui/views/pag... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:739: site_settings_view_->AddChildViewAt(certificate_view_, 0); Is SetIdentityInfo potentially called more than once for an open bubble? If so you'll need to remove and delete the old |certificate_view_| before creating and adding a new one (or just update the old one?) I also wonder if you should re-use the prior index in that case (are you sure that |certificate_view_| was really at index 0?) Or perhaps this should just be done in CreateSiteSettingsView? https://codereview.chromium.org/2846913002/diff/1/chrome/browser/ui/views/pag... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:807: } break; nit: break on next line (git cl format?)
histograms.xml lgtm
elawrence@chromium.org changed reviewers: + estade@chromium.org
Thanks for your quick reviews! @estade: PTAL at the new vector icon. @oshima: I've replaced the PNG icons with the vector icon. https://codereview.chromium.org/2846913002/diff/1/chrome/browser/ui/cocoa/pag... File chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.h (right): https://codereview.chromium.org/2846913002/diff/1/chrome/browser/ui/cocoa/pag... chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.h:33: @interface InspectLinkView : FlippedView { On 2017/04/27 18:08:56, Robert Sesek wrote: > You can forward declare this with @class and put it entirely in the .mm file. Done. https://codereview.chromium.org/2846913002/diff/1/chrome/browser/ui/cocoa/pag... chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.h:33: @interface InspectLinkView : FlippedView { On 2017/04/27 18:08:56, Robert Sesek wrote: > Please give this a brief top-level comment. Done. https://codereview.chromium.org/2846913002/diff/1/chrome/browser/ui/cocoa/pag... chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.h:34: @private On 2017/04/27 18:08:56, Robert Sesek wrote: > After this moves into the .mm file, you can also declare the instance variables > in the @implementation: > > @interface InspectLinkView : FlippedView > @end > > @implementation InspectLinkView () { > NSButton* actionLink_; > } > ... > @end Done. https://codereview.chromium.org/2846913002/diff/1/chrome/browser/ui/cocoa/pag... chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.h:38: - (id)initWithFrame:(NSRect)frame; On 2017/04/27 18:08:56, Robert Sesek wrote: > You can omit common designated initializers for view subclasses. Done. https://codereview.chromium.org/2846913002/diff/1/chrome/browser/ui/cocoa/pag... File chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm (right): https://codereview.chromium.org/2846913002/diff/1/chrome/browser/ui/cocoa/pag... chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm:146: [self setSubviews:[NSArray array]]; On 2017/04/27 18:08:56, Robert Sesek wrote: > Is this necessary? Apparently, not. Removing setSubviews has no discernible effect. https://codereview.chromium.org/2846913002/diff/1/chrome/browser/ui/cocoa/pag... chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm:851: if (isValid) On 2017/04/27 18:08:56, Robert Sesek wrote: > nit: braces Done. https://codereview.chromium.org/2846913002/diff/1/chrome/browser/ui/views/pag... File chrome/browser/ui/views/page_info/page_info_bubble_view.cc (right): https://codereview.chromium.org/2846913002/diff/1/chrome/browser/ui/views/pag... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:309: // |InspectLinkView| is a UI element (view) that shows information and On 2017/04/27 19:47:55, msw wrote: > nit: remove copied comment, the class comment above is sufficient Done. https://codereview.chromium.org/2846913002/diff/1/chrome/browser/ui/views/pag... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:356: InvalidateLayout(); On 2017/04/27 19:47:55, msw wrote: > q: Is this needed? Perhaps GridLayout just does the right thing without this? Apparently it is not needed. https://codereview.chromium.org/2846913002/diff/1/chrome/browser/ui/views/pag... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:739: site_settings_view_->AddChildViewAt(certificate_view_, 0); On 2017/04/27 19:47:55, msw wrote: > Is SetIdentityInfo potentially called more than once for an open bubble? If so > you'll need to remove and delete the old |certificate_view_| before creating and > adding a new one (or just update the old one?) I also wonder if you should > re-use the prior index in that case (are you sure that |certificate_view_| was > really at index 0?) Or perhaps this should just be done in > CreateSiteSettingsView? As far as I can tell, no, SetIdentityInfo() is only ever called once per Page Info bubble. lgarron@ may know something I don't? Unlike the Cookie subview created in CreateSiteSettingsView, we don't show the certificate subview on all pages, only those that have a certificate. https://codereview.chromium.org/2846913002/diff/1/chrome/browser/ui/views/pag... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:807: } break; On 2017/04/27 19:47:55, msw wrote: > nit: break on next line (git cl format?) Yes, running "git cl format" moves the break to this point. Apparently, our coding style is to put the break inside the brace (https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements), so I've made that change.
chrome/browser/ui/[views|page_info] lgtm
vector icon lgtm https://codereview.chromium.org/2846913002/diff/20001/chrome/browser/ui/page_... File chrome/browser/ui/page_info/page_info_ui.cc (right): https://codereview.chromium.org/2846913002/diff/20001/chrome/browser/ui/page_... chrome/browser/ui/page_info/page_info_ui.cc:463: return image; nit: return gfx::Image(gfx::Create... https://codereview.chromium.org/2846913002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/page_info/page_info_bubble_view.cc (right): https://codereview.chromium.org/2846913002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:309: InspectLinkView::InspectLinkView(const gfx::Image& image_icon, I would suggest not creating this new class. It doesn't (nor does it need to) override any methods of View. Instead most of it can be accomplished via a factory function. You can get rid of several parameters and both methods by creating, storing a reference to, and passing in the link separately.
On 2017/04/27 at 21:58:05, estade wrote: > vector icon lgtm > > https://codereview.chromium.org/2846913002/diff/20001/chrome/browser/ui/page_... > File chrome/browser/ui/page_info/page_info_ui.cc (right): > > https://codereview.chromium.org/2846913002/diff/20001/chrome/browser/ui/page_... > chrome/browser/ui/page_info/page_info_ui.cc:463: return image; > nit: return gfx::Image(gfx::Create... > > https://codereview.chromium.org/2846913002/diff/20001/chrome/browser/ui/views... > File chrome/browser/ui/views/page_info/page_info_bubble_view.cc (right): > > https://codereview.chromium.org/2846913002/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/page_info/page_info_bubble_view.cc:309: InspectLinkView::InspectLinkView(const gfx::Image& image_icon, > I would suggest not creating this new class. It doesn't (nor does it need to) override any methods of View. Instead most of it can be accomplished via a factory function. You can get rid of several parameters and both methods by creating, storing a reference to, and passing in the link separately. Regarding vector icons: the rest of Page Info doesn't use them yet (https://crbug.com/647551). We should fix that bug, but I'm worried about intermediate inconsistency. estade@:Is there any chance this will cause the icon to render "differently" from the others under any circumstances? Eric: Can you confirm that the icon intentionally has the same vector definitions at 1x and 2x?
LGTM
> Eric: Can you confirm that the icon intentionally has > the same vector definitions at 1x and 2x? My understanding is that the entire point of the vector architecture is that the 1x scales up nicely to 2x, and we only need a special "1x" version if we need to simplify a complicated icon when the DPR is 1.0. https://codereview.chromium.org/2846913002/diff/20001/chrome/browser/ui/page_... File chrome/browser/ui/page_info/page_info_ui.cc (right): https://codereview.chromium.org/2846913002/diff/20001/chrome/browser/ui/page_... chrome/browser/ui/page_info/page_info_ui.cc:463: return image; On 2017/04/27 21:58:04, Evan Stade wrote: > nit: return gfx::Image(gfx::Create... Done. https://codereview.chromium.org/2846913002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/page_info/page_info_bubble_view.cc (right): https://codereview.chromium.org/2846913002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:309: InspectLinkView::InspectLinkView(const gfx::Image& image_icon, On 2017/04/27 21:58:04, Evan Stade wrote: > I would suggest not creating this new class. It doesn't (nor does it need to) > override any methods of View. Instead most of it can be accomplished via a > factory function. You can get rid of several parameters and both methods by > creating, storing a reference to, and passing in the link separately. I may not understand exactly what sort of factory function you have in mind. Would it just return a regular View which doesn't contain a link as a member field? And then the link would be manipulated separately? The new subview was requested by lgarron@ in his original review as a means of cleaning up the code and providing some level of organization. As someone not very familiar with the Views architecture, the result of its cleanup seemed pretty good to me.
On 2017/04/27 22:12:49, elawrence wrote: > > Eric: Can you confirm that the icon intentionally has > > the same vector definitions at 1x and 2x? > > My understanding is that the entire point of the vector architecture is that the > 1x scales up nicely to 2x, and we only need a special "1x" version if we need to > simplify a complicated icon when the DPR is 1.0. Well, it's less about simplification (i.e. removing parts of the icon) and more about making sure it aligns to pixels nicely. This is more applicable at smaller sizes like 16dip, but if the designer is happy with how this one scales down, it should be ok. > > https://codereview.chromium.org/2846913002/diff/20001/chrome/browser/ui/page_... > File chrome/browser/ui/page_info/page_info_ui.cc (right): > > https://codereview.chromium.org/2846913002/diff/20001/chrome/browser/ui/page_... > chrome/browser/ui/page_info/page_info_ui.cc:463: return image; > On 2017/04/27 21:58:04, Evan Stade wrote: > > nit: return gfx::Image(gfx::Create... > > Done. > > https://codereview.chromium.org/2846913002/diff/20001/chrome/browser/ui/views... > File chrome/browser/ui/views/page_info/page_info_bubble_view.cc (right): > > https://codereview.chromium.org/2846913002/diff/20001/chrome/browser/ui/views... > chrome/browser/ui/views/page_info/page_info_bubble_view.cc:309: > InspectLinkView::InspectLinkView(const gfx::Image& image_icon, > On 2017/04/27 21:58:04, Evan Stade wrote: > > I would suggest not creating this new class. It doesn't (nor does it need to) > > override any methods of View. Instead most of it can be accomplished via a > > factory function. You can get rid of several parameters and both methods by > > creating, storing a reference to, and passing in the link separately. > > I may not understand exactly what sort of factory function you have in mind. > Would it just return a regular View which doesn't contain a link as a member > field? And then the link would be manipulated separately? Basically, the contents of the constructor becomes instead a static factory function. You can retain a pointer to the link separately so that you can change its text and tooltip instead of providing pass-throughs. > > The new subview was requested by lgarron@ in his original review as a means of > cleaning up the code and providing some level of organization. As someone not > very familiar with the Views architecture, the result of its cleanup seemed > pretty good to me. There's too much inheritance used in Views. There is currently an effort to reduce that. This is an example where we do not need inheritance. We are not overriding anything.
The CQ bit was checked by elawrence@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:50001) has been deleted
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...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by elawrence@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks again for your reviews! estade@: PTAL-- as requested, I've removed the Views-derived class and replaced it with a simple function. msw@: I changed the Views code non-trivially if you'd like another look. lgarron@: I've verified that the vector icon looks as expected on MacBook in Cocoa and MacViews and on Windows Views. oshima@: I don't know if you want to review anything here as I've removed the theme PNG files. https://codereview.chromium.org/2846913002/diff/1/chrome/browser/ui/views/pag... File chrome/browser/ui/views/page_info/page_info_bubble_view.cc (right): https://codereview.chromium.org/2846913002/diff/1/chrome/browser/ui/views/pag... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:356: InvalidateLayout(); On 2017/04/27 21:44:25, elawrence wrote: > On 2017/04/27 19:47:55, msw wrote: > > q: Is this needed? Perhaps GridLayout just does the right thing without this? > > Apparently it is not needed. Upon retesting on Windows Views, we actually do need to trigger a layout here. Without one, the Cookies section shows "44 in ..." because the label isn't made wider as the number increments. Calling Layout() before SizeToContents() is the pattern used elsewhere, so we'll use that. https://codereview.chromium.org/2846913002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/page_info/page_info_bubble_view.cc (right): https://codereview.chromium.org/2846913002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:309: InspectLinkView::InspectLinkView(const gfx::Image& image_icon, On 2017/04/27 22:12:49, elawrence wrote: > On 2017/04/27 21:58:04, Evan Stade wrote: > > I would suggest not creating this new class. It doesn't (nor does it need to) > > override any methods of View. Instead most of it can be accomplished via a > > factory function. You can get rid of several parameters and both methods by > > creating, storing a reference to, and passing in the link separately. > > I may not understand exactly what sort of factory function you have in mind. > Would it just return a regular View which doesn't contain a link as a member > field? And then the link would be manipulated separately? > > The new subview was requested by lgarron@ in his original review as a means of > cleaning up the code and providing some level of organization. As someone not > very familiar with the Views architecture, the result of its cleanup seemed > pretty good to me. Removed in the latest revision.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by elawrence@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 ==========
oshima@chromium.org changed reviewers: - oshima@chromium.org
oshima@chromium.org changed reviewers: + oshima@chromium.org
I don't think you need my review any longer. removing myself
oshima@chromium.org changed reviewers: - oshima@chromium.org
minor nits, I'll let msw review more thoroughly https://codereview.chromium.org/2846913002/diff/90001/chrome/browser/ui/views... File chrome/browser/ui/views/page_info/page_info_bubble_view.cc (right): https://codereview.chromium.org/2846913002/diff/90001/chrome/browser/ui/views... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:640: PageInfo::SITE_IDENTITY_STATUS_ERROR) nit: might be easier to read if you pulled this comparison out into a local var, since it's made twice. https://codereview.chromium.org/2846913002/diff/90001/chrome/browser/ui/views... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:691: PageInfoUI::GetPermissionIcon(info).ToImageSkia(); nit: AsImageSkia() https://codereview.chromium.org/2846913002/diff/90001/chrome/browser/ui/views... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:700: views::View* PageInfoBubbleView::CreateInspectSection( does this need to be a method? Can we make it a static function at the top of the file? https://codereview.chromium.org/2846913002/diff/90001/chrome/browser/ui/views... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:702: const base::string16& title, nit: if you make this take an int (IDS_ value) it will make callsites a little easier to read
The CQ bit was unchecked by commit-bot@chromium.org
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...)
Patchset #6 (id:110001) has been deleted
https://codereview.chromium.org/2846913002/diff/90001/chrome/browser/ui/views... File chrome/browser/ui/views/page_info/page_info_bubble_view.cc (right): https://codereview.chromium.org/2846913002/diff/90001/chrome/browser/ui/views... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:640: PageInfo::SITE_IDENTITY_STATUS_ERROR) On 2017/04/28 19:51:20, Evan Stade wrote: > nit: might be easier to read if you pulled this comparison out into a local var, > since it's made twice. Done. https://codereview.chromium.org/2846913002/diff/90001/chrome/browser/ui/views... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:691: PageInfoUI::GetPermissionIcon(info).ToImageSkia(); On 2017/04/28 19:51:21, Evan Stade wrote: > nit: AsImageSkia() Done. https://codereview.chromium.org/2846913002/diff/90001/chrome/browser/ui/views... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:700: views::View* PageInfoBubbleView::CreateInspectSection( On 2017/04/28 19:51:21, Evan Stade wrote: > does this need to be a method? Can we make it a static function at the top of > the file? It could be converted to a static method that either takes a views::LinkListener or both callers could take responsibility for setting link->set_listener(this) before calling the static method. I'll confess that it's unclear to me what benefits such a refactoring would provide? The current function is parallel to the existing CreateSiteSettingsView method. https://codereview.chromium.org/2846913002/diff/90001/chrome/browser/ui/views... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:702: const base::string16& title, On 2017/04/28 19:51:21, Evan Stade wrote: > nit: if you make this take an int (IDS_ value) it will make callsites a little > easier to read Good point! Done.
https://codereview.chromium.org/2846913002/diff/90001/chrome/browser/ui/views... File chrome/browser/ui/views/page_info/page_info_bubble_view.cc (right): https://codereview.chromium.org/2846913002/diff/90001/chrome/browser/ui/views... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:700: views::View* PageInfoBubbleView::CreateInspectSection( On 2017/04/28 20:36:51, elawrence wrote: > On 2017/04/28 19:51:21, Evan Stade wrote: > > does this need to be a method? Can we make it a static function at the top of > > the file? > > It could be converted to a static method that either takes a views::LinkListener > or both callers could take responsibility for setting link->set_listener(this) > before calling the static method. > > I'll confess that it's unclear to me what benefits such a refactoring would > provide? The current function is parallel to the existing CreateSiteSettingsView > method. static function in this file declutters the header file, and more importantly makes it evident what data it operates on (i.e. that the entire set of relevant inputs are in the parameter list)
LGTM with nits. https://codereview.chromium.org/2846913002/diff/130001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2846913002/diff/130001/chrome/browser/about_f... chrome/browser/about_flags.cc:2574: #endif // defined(OS_CHROMEOS) Nit: Still unrelated. (Same below.) https://codereview.chromium.org/2846913002/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/page_info/page_info_bubble_view.cc (right): https://codereview.chromium.org/2846913002/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:645: views::Link* link_inspect = new views::Link(link_title); Nit: `inspect_link` (You might have been trying to copy `link_section` above, but that actually describes a section for `site_settings_link`.) https://codereview.chromium.org/2846913002/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:654: site_settings_view_->AddChildViewAt( This doesn't seem to wrap (at least on MacViews) - a long string will make the dialog grow. We should probably use consistent wrapping/eliding, but that's outside the scope of this CL. Presumably, the translations for the single words won't be ridiculously log. https://codereview.chromium.org/2846913002/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/page_info/page_info_bubble_view.h (right): https://codereview.chromium.org/2846913002/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/page_info/page_info_bubble_view.h:166: // The view that contains the certificate, cookie, and permissions sections. That means "site settings" is no longer accurate, but I can't think of something better. Open to suggestions if you think of something. https://codereview.chromium.org/2846913002/diff/130001/components/page_info_s... File components/page_info_strings.grdp (right): https://codereview.chromium.org/2846913002/diff/130001/components/page_info_s... components/page_info_strings.grdp:4: <!-- Certificate Viewer link --> Nit: The strings in this file are in the order they appear in the UI. Could you place this section after "Detail strings"? (Not perfectly, but I'm working on it: https://codereview.chromium.org/2849713003) https://codereview.chromium.org/2846913002/diff/130001/components/page_info_s... components/page_info_strings.grdp:5: <message name="IDS_PAGE_INFO_CERTIFICATE" desc="Title of the certificate area in the Page Information Window, shown when a HTTPS site is loaded."> Nit: Strings descriptions are standardized to "Page Info bubble". https://codereview.chromium.org/2846913002/diff/130001/components/page_info_s... components/page_info_strings.grdp:8: <message name="IDS_PAGE_INFO_CERTIFICATE_VALID_LINK" desc="Text of the link that launches the Certificate Viewer. This appears in the Page Information Window when a HTTPS site is loaded with a valid certificate."> Nit: I would reverse the description sentences to emphasize what the string means, not what the UI uses it for. https://codereview.chromium.org/2846913002/diff/130001/components/page_info_s... components/page_info_strings.grdp:11: <message name="IDS_PAGE_INFO_CERTIFICATE_INVALID_LINK" desc="Text of the link that launches the Certificate Viewer. This appears in the Page Information Window when a HTTPS site is loaded with an invalid certificate, e.g. https://wrong.host.badssl.com/."> Same. https://codereview.chromium.org/2846913002/diff/130001/components/page_info_s... components/page_info_strings.grdp:15: Issued by <ph name="ISSUER">$1<ex>Let's Encrypt X3</ex></ph>. Nit: My instinct is that this string should not have a period at the end. Did UI weigh in?
Thanks for the reviews! msw@, would you like one final look at where we ended up? https://codereview.chromium.org/2846913002/diff/90001/chrome/browser/ui/views... File chrome/browser/ui/views/page_info/page_info_bubble_view.cc (right): https://codereview.chromium.org/2846913002/diff/90001/chrome/browser/ui/views... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:700: views::View* PageInfoBubbleView::CreateInspectSection( On 2017/04/28 20:56:55, Evan Stade wrote: > On 2017/04/28 20:36:51, elawrence wrote: > > On 2017/04/28 19:51:21, Evan Stade wrote: > > > does this need to be a method? Can we make it a static function at the top > of > > > the file? > > > > It could be converted to a static method that either takes a > views::LinkListener > > or both callers could take responsibility for setting link->set_listener(this) > > before calling the static method. > > > > I'll confess that it's unclear to me what benefits such a refactoring would > > provide? The current function is parallel to the existing > CreateSiteSettingsView > > method. > > static function in this file declutters the header file, and more importantly > makes it evident what data it operates on (i.e. that the entire set of relevant > inputs are in the parameter list) Done. https://codereview.chromium.org/2846913002/diff/130001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2846913002/diff/130001/chrome/browser/about_f... chrome/browser/about_flags.cc:2574: #endif // defined(OS_CHROMEOS) On 2017/04/28 23:16:43, lgarron wrote: > Nit: Still unrelated. (Same below.) I've malformed this again. https://codereview.chromium.org/2846913002/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/page_info/page_info_bubble_view.cc (right): https://codereview.chromium.org/2846913002/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:645: views::Link* link_inspect = new views::Link(link_title); On 2017/04/28 23:16:43, lgarron wrote: > Nit: `inspect_link` > > (You might have been trying to copy `link_section` above, but that actually > describes a section for `site_settings_link`.) Done. https://codereview.chromium.org/2846913002/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:654: site_settings_view_->AddChildViewAt( On 2017/04/28 23:16:43, lgarron wrote: > This doesn't seem to wrap (at least on MacViews) - a long string will make the > dialog grow. > > We should probably use consistent wrapping/eliding, but that's outside the scope > of this CL. Presumably, the translations for the single words won't be > ridiculously log. Acknowledged. https://codereview.chromium.org/2846913002/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/page_info/page_info_bubble_view.h (right): https://codereview.chromium.org/2846913002/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/page_info/page_info_bubble_view.h:166: // The view that contains the certificate, cookie, and permissions sections. On 2017/04/28 23:16:43, lgarron wrote: > That means "site settings" is no longer accurate, but I can't think of something > better. > Open to suggestions if you think of something. It arguably wasn't accurate before (cookies weren't a setting here) so I'll leave this alone for now. https://codereview.chromium.org/2846913002/diff/130001/components/page_info_s... File components/page_info_strings.grdp (right): https://codereview.chromium.org/2846913002/diff/130001/components/page_info_s... components/page_info_strings.grdp:4: <!-- Certificate Viewer link --> On 2017/04/28 23:16:43, lgarron wrote: > Nit: The strings in this file are in the order they appear in the UI. Could you > place this section after "Detail strings"? > (Not perfectly, but I'm working on it: > https://codereview.chromium.org/2849713003) Done. https://codereview.chromium.org/2846913002/diff/130001/components/page_info_s... components/page_info_strings.grdp:5: <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/28 23:16:43, lgarron wrote: > Nit: Strings descriptions are standardized to "Page Info bubble". Done. https://codereview.chromium.org/2846913002/diff/130001/components/page_info_s... components/page_info_strings.grdp:8: <message name="IDS_PAGE_INFO_CERTIFICATE_VALID_LINK" desc="Text of the link that launches the Certificate Viewer. This appears in the Page Information Window when a HTTPS site is loaded with a valid certificate."> On 2017/04/28 23:16:43, lgarron wrote: > Nit: I would reverse the description sentences to emphasize what the string > means, not what the UI uses it for. Done. https://codereview.chromium.org/2846913002/diff/130001/components/page_info_s... components/page_info_strings.grdp:11: <message name="IDS_PAGE_INFO_CERTIFICATE_INVALID_LINK" desc="Text of the link that launches the Certificate Viewer. This appears in the Page Information Window when a HTTPS site is loaded with an invalid certificate, e.g. https://wrong.host.badssl.com/."> On 2017/04/28 23:16:43, lgarron wrote: > Same. Done. https://codereview.chromium.org/2846913002/diff/130001/components/page_info_s... components/page_info_strings.grdp:15: Issued by <ph name="ISSUER">$1<ex>Let's Encrypt X3</ex></ph>. On 2017/04/28 23:16:43, lgarron wrote: > Nit: My instinct is that this string should not have a period at the end. Did UI > weigh in? Done. Terminal punctuation for Infotips which are full sentences. They declined using a full sentence, so no terminal punctuation.
The CQ bit was checked by elawrence@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Nice, this is even better; still lgtm with a minor nit. https://codereview.chromium.org/2846913002/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/page_info/page_info_bubble_view.cc (right): https://codereview.chromium.org/2846913002/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/page_info/page_info_bubble_view.cc:128: views::View* CreateInspectSection(const gfx::ImageSkia& image_icon, nit: I'm not sure "Inspect" conveys the purpose of these views, but I don't actually have a better suggestion.
On 2017/05/01 21:46:53, msw wrote: > Nice, this is even better; still lgtm with a minor nit. > > https://codereview.chromium.org/2846913002/diff/150001/chrome/browser/ui/view... > File chrome/browser/ui/views/page_info/page_info_bubble_view.cc (right): > > https://codereview.chromium.org/2846913002/diff/150001/chrome/browser/ui/view... > chrome/browser/ui/views/page_info/page_info_bubble_view.cc:128: views::View* > CreateInspectSection(const gfx::ImageSkia& image_icon, > nit: I'm not sure "Inspect" conveys the purpose of these views, but I don't > actually have a better suggestion. I picked "Inspect" because the only real commonality between the Cookies and Certificate sections is that they offer a link to allow the user to inspect the underlying data (Cookies or Certificate). I think CreateInspectLinkSection is a bit more descriptive, and it brings the Cocoa and Views code closer together. Thanks for the look!
The CQ bit was checked by elawrence@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org, estade@chromium.org, rsesek@chromium.org, lgarron@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2846913002/#ps190001 (title: "Fix merge conflict")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for chrome/browser/about_flags.cc:
While running git apply --index -3 -p1;
error: patch failed: chrome/browser/about_flags.cc:2759
Falling back to three-way merge...
Applied patch to 'chrome/browser/about_flags.cc' with conflicts.
U chrome/browser/about_flags.cc
Patch: chrome/browser/about_flags.cc
Index: chrome/browser/about_flags.cc
diff --git a/chrome/browser/about_flags.cc b/chrome/browser/about_flags.cc
index
5fcb4239f351221a9d1ed96adab5c4a3cc05bd52..9c24f7905756550a941b581c6810459774b55294
100644
--- a/chrome/browser/about_flags.cc
+++ b/chrome/browser/about_flags.cc
@@ -2759,6 +2759,12 @@ const FeatureEntry kFeatureEntries[] = {
flag_descriptions::kAutoplayPolicyDescription, kOsAll,
MULTI_VALUE_TYPE(kAutoplayPolicyChoices)},
+#if defined(TOOLKIT_VIEWS) || (defined(OS_MACOSX) && !defined(OS_IOS))
+ {"show-cert-link", flag_descriptions::kShowCertLinkOnPageInfoName,
+ flag_descriptions::kShowCertLinkOnPageInfoDescription, kOsDesktop,
+ SINGLE_VALUE_TYPE(switches::kShowCertLink)},
+#endif
+
// NOTE: Adding new command-line switches requires adding corresponding
// entries to enum "LoginCustomFlags" in histograms.xml. See note in
// histograms.xml and don't forget to run AboutFlagsHistogramTest unit
test.
The CQ bit was checked by elawrence@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, lgarron@chromium.org, estade@chromium.org, mpearson@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2846913002/#ps210001 (title: "Fix merge conflict")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 210001, "attempt_start_ts": 1493685837789960,
"parent_rev": "e84a36378d78316e10dd54398977d8f85a040a92", "commit_rev":
"d9978fcc021c14d5260d1abffc2d0b39e41018db"}
Message was sent while issue was closed.
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 Review-Url: https://codereview.chromium.org/2846913002 Cr-Commit-Position: refs/heads/master@{#468539} Committed: https://chromium.googlesource.com/chromium/src/+/d9978fcc021c14d5260d1abffc2d... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:210001) as https://chromium.googlesource.com/chromium/src/+/d9978fcc021c14d5260d1abffc2d... |
