Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
294123002:
Make the cocoa permission bubble match mocks. (Closed)
|
Created:
6 years, 7 months ago by leng Modified:
6 years, 7 months ago Reviewers:
groby-ooo-7-16 CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionMake the cocoa permission bubble match mocks.
Specifically, edits font size and padding, and makes calculation of the close
button's origin clearer.
BUG=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272313
Patch Set 1 #
Total comments: 6
Patch Set 2 : moved constant #Patch Set 3 : revert constant relocation #Messages
Total messages: 17 (0 generated)
lgtm https://codereview.chromium.org/294123002/diff/1/chrome/browser/ui/cocoa/webs... File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm (right): https://codereview.chromium.org/294123002/diff/1/chrome/browser/ui/cocoa/webs... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:51: const NSSize kPermissionIconSize = {18, 18}; General question: Are some of those constants shared across UIs? https://codereview.chromium.org/294123002/diff/1/chrome/browser/ui/cocoa/webs... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:388: [permissionIcon setFrameSize:kPermissionIconSize]; Are you sure you want scaling if the icon is not 18x18? Do you want to DCHECK icon size?
PTAL - I'm not sure I moved the constant to the right place, or even if it's necessary to move it as it's not currently shared. It just might be. https://codereview.chromium.org/294123002/diff/1/chrome/browser/ui/cocoa/webs... File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm (right): https://codereview.chromium.org/294123002/diff/1/chrome/browser/ui/cocoa/webs... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:51: const NSSize kPermissionIconSize = {18, 18}; On 2014/05/21 18:28:37, groby wrote: > General question: Are some of those constants shared across UIs? This one currently is not, but may be used by the views code. I moved it into a chrome_style namespace in permission_bubble_view.h. Is that the right place? https://codereview.chromium.org/294123002/diff/1/chrome/browser/ui/cocoa/webs... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:388: [permissionIcon setFrameSize:kPermissionIconSize]; On 2014/05/21 18:28:37, groby wrote: > Are you sure you want scaling if the icon is not 18x18? Do you want to DCHECK > icon size? The scaling is deliberate - I didn't want to recreate all of the icons, since they are only rough versions anyhow.
SLGTM https://codereview.chromium.org/294123002/diff/1/chrome/browser/ui/cocoa/webs... File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm (right): https://codereview.chromium.org/294123002/diff/1/chrome/browser/ui/cocoa/webs... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:51: const NSSize kPermissionIconSize = {18, 18}; On 2014/05/21 22:33:51, leng wrote: > On 2014/05/21 18:28:37, groby wrote: > > General question: Are some of those constants shared across UIs? > > This one currently is not, but may be used by the views code. I moved it into a > chrome_style namespace in permission_bubble_view.h. Is that the right place? Let's not add another namespace. If there's no natural place to keep things, it's fine in here for now. https://codereview.chromium.org/294123002/diff/1/chrome/browser/ui/cocoa/webs... chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:388: [permissionIcon setFrameSize:kPermissionIconSize]; On 2014/05/21 22:33:51, leng wrote: > On 2014/05/21 18:28:37, groby wrote: > > Are you sure you want scaling if the icon is not 18x18? Do you want to DCHECK > > icon size? > > The scaling is deliberate - I didn't want to recreate all of the icons, since > they are only rough versions anyhow. Done.
The CQ bit was checked by leng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leng@chromium.org/294123002/40001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by leng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leng@chromium.org/294123002/40001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by leng@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/leng@chromium.org/294123002/40001
Message was sent while issue was closed.
Change committed as 272313 |
