Permissions: Show the reason for permission decisions made on the user's behalf.

chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.h"
 
 #import <AppKit/AppKit.h>
 
 #include <cmath>
 
@@ skipping 76 matching lines @@
 
 // Minimum distance between the label and its corresponding menu.
 const CGFloat kMinSeparationBetweenLabelAndMenu = 16;
 
 // Square size of the permission delete button image.
 const CGFloat kPermissionDeleteImageSize = 16;
 
 // The spacing between individual permissions.
 const CGFloat kPermissionsVerticalSpacing = 16;
 
+// The spacing between permissions and their decision description labels.
+const CGFloat kPermissionsDecisionVerticalSpacing = 4;
+
 // Amount to lower each permission icon to align the icon baseline with the
 // label text.
 const CGFloat kPermissionIconYAdjustment = 1;
 
 // Amount to lower each permission popup button to make its text align with the
 // permission label.
 const CGFloat kPermissionPopupButtonYAdjustment = 3;
 
 // Internal Page Bubble --------------------------------------------------------
 
@@ skipping 786 matching lines @@
 
   // Align the icon with the text.
   [self alignPermissionIcon:imageView withTextField:label];
 
   // Permissions specified by policy or an extension cannot be changed.
   if (permissionInfo.source == content_settings::SETTING_SOURCE_EXTENSION ||
       permissionInfo.source == content_settings::SETTING_SOURCE_POLICY) {
     [button setEnabled:NO];
   }
 
+  // Show the reason for the permission decision in a new row if it did not come
+  // from the user.
   NSRect buttonFrame = [button frame];
-  return NSMakePoint(NSMaxX(buttonFrame), NSMaxY(buttonFrame));
+  int maxY = NSMaxY(buttonFrame);

[lgarron, 2017/03/29 05:49:26]

I know the existing code uses maxY, but mainly to mean "the very bottom", and
but I'm finding it hard to follow the calculations in here.  Is it possible to
keep using point.y?

That is, something like:

point.y = NSHeight(label.frame);
if (...) {
  point.y += kPermissionsDecisionVerticalSpacing;
  ...
  point.y += label.frame.size.height;
}

return NSMakePoint(NSMaxX(buttonFrame), point.y);

[Patti Lor, 2017/03/30 03:27:29]

Done, thanks for the suggestions. I added a comment too to try and make it
clearer.

+  base::string16 reason = PageInfoUI::PermissionDecisionReasonToUIString(
+      [self profile], permissionInfo, url_);
+  if (!reason.empty()) {
+    point.y = position.y + kPermissionsDecisionVerticalSpacing +
+              label.frame.size.height;
+    label = [self addText:reason
+                 withSize:[NSFont systemFontSize]

[lgarron, 2017/03/29 05:49:26]

Looking at the mocks, it seems to be this should be smallSystemFontSize.

[Patti Lor, 2017/03/30 03:27:29]

I think there was a misunderstanding here - by mocks, do you mean the
screenshots that were shared with the permissions team? Those are taken after
having this CL patched in. The only other mocks I know of are "fully-MDified",
and since the rest of the UI is not at that stage yet, it was agreed by Dom & UX
that we would have an interim state which looked like the screenshots in the
relevant email. I'm leaving the font size as it currently is now since I am
assuming you would like page info to look like those screenshots, but here is a
screenshot of what it would look like if the font was smallSystemFontSize.
https://drive.google.com/open?id=0BzEa5HU1aAqBYkRRaE1oNHN0V1U

Let me know if that's what you meant.

[lgarron, 2017/04/05 00:37:46]

Ah. So, this is partially my fault.
Per the specs I was using [1], the permission label should use a larger font,
while the dropdown is a smaller font. For some reason, the Views implementation
doesn't have that.

The screenshot uses the smaller font for the small part, but it's MacViews,
which uses a smaller font for the permission label, which I must have missed
changing to a larger label for https://crbug.com/512442
I've filed https://crbug.com/708358 for that.

My expectation is that decision text would be small, matching the dropdown. (If
not, then the Views permission label and decision string should both be made
larger.) It would be nice to confirm with a designer, though. Feel free to pick
one or the other for now.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=512442&desc=2#c16

[Patti Lor, 2017/04/05 01:02:43]

Thanks for clarifying & following up on the font sizes :) I'll leave things as
they are now since it sounds like there will need to be a change later either
way.

+                     bold:NO
+                   toView:view
+                  atPoint:point];
+    label.textColor = skia::SkColorToSRGBNSColor(SK_ColorGRAY);

[lgarron, 2017/03/29 05:49:26]

Could you share the color across platforms by e.g. creating a
GetPermissionDecisionTextColor() function in PageInfoUI?

[Patti Lor, 2017/03/30 03:27:29]

Done.

+    maxY += label.frame.size.height;
+  }
+
+  return NSMakePoint(NSMaxX(buttonFrame), maxY);
 }
 
 // Adds a new row to the UI listing the permissions. Returns the NSPoint of the
 // last UI element added (either the permission button, in LTR, or the text
 // label, in RTL).
 - (NSPoint)addChosenObject:(ChosenObjectInfoPtr)objectInfo
                     toView:(NSView*)view
                    atPoint:(NSPoint)point {
   base::string16 labelText = l10n_util::GetStringFUTF16(
       objectInfo->ui_info.label_string_id,
@@ skipping 280 matching lines @@
 void PageInfoUIBridge::SetCookieInfo(const CookieInfoList& cookie_info_list) {
   [bubble_controller_ setCookieInfo:cookie_info_list];
 }
 
 void PageInfoUIBridge::SetPermissionInfo(
     const PermissionInfoList& permission_info_list,
     ChosenObjectInfoList chosen_object_info_list) {
   [bubble_controller_ setPermissionInfo:permission_info_list
                        andChosenObjects:std::move(chosen_object_info_list)];
 }