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

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

Index: chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm
diff --git a/chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm b/chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm
index a86a1934953eee48a40d820913670921ea481193..a3bf3771017e0388fc4125a7c9ddfdf958b660ee 100644
--- a/chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm
+++ b/chrome/browser/ui/cocoa/page_info/page_info_bubble_controller.mm
@@ -94,6 +94,9 @@ 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;
@@ -900,8 +903,25 @@ bool IsInternalURL(const GURL& url) {
     [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