Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1585013002:
MacViews: Use Cocoa folder icons in tree views. (Closed)
|
Created:
4 years, 11 months ago by karandeepb Modified:
4 years, 11 months ago CC:
chromium-reviews, rsesek+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: Use Cocoa folder icons in tree views.
Chrome's Cocoa UI retrieves some image assets from the System using APIs such
as +[NSImage imageNamed:key]. To support this under toolkit-views, provide a
ui::ResourceDelegate on Mac that can transparently map some IDR_ resource IDs
to a system-provided asset. Initially do this for folder icons in
views::TreeView to provide a folder icon that is consistent with the running
OSX version.
The following resource ids are currently intercepted - IDR_FOLDER_OPEN,
IDR_FOLDER_CLOSED, IDR_FOLDER_OPEN_RTL and IDR_FOLDER_CLOSED_RTL. Although
IDR_FOLDER_CLOSED is in ui/resources (and not ui/views/resources), it is
currently unused on Mac (chrome://syncfs-internals/ refers to it in its CSS,
but doesn't actually use that style anywhere in its html or js). Hence this
CL, does not alter any icons on Chrome Mac, except under Views.
BUG=543674
Committed: https://crrev.com/e60fc3cf40cbc732e5ccaf6ff3785cc1351fbe03
Cr-Commit-Position: refs/heads/master@{#370819}
Patch Set 1 : #
Total comments: 29
Patch Set 2 : Removed folder icon from devtools and rtl support, addressed review comments. #
Total comments: 17
Patch Set 3 : Addressed review comments. #
Total comments: 12
Patch Set 4 : Rebased #Patch Set 5 : Addressed review comments. #
Messages
Total messages: 26 (12 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== MacViews: Use Cocoa folder icons. ========== to ========== MacViews: Use Cocoa folder icons in tree views and Developer Tools panel. This CL causes Tree views and Developer tools panel to use Mac- specific folder icons on Mac. It does this by creating a MacResourceDelegate, which intercepts certain resource ids and provides Mac specific assets for them at runtime. Currently it overrides the following resource ids - IDR_FOLDER_OPEN, IDR_FOLDER_CLOSED, IDR_FOLDER_OPEN_RTL, IDR_FOLDER_CLOSED_RTL, IMAGES_FRAME_PNG. BUG=543674 ==========
karandeepb@chromium.org changed reviewers: + tapted@chromium.org
PTAL Trent. This is a preliminary CL. Let me know if the approach looks feasible or if there is a better way to do things. Thanks. https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/resource... File chrome/browser/resource_delegate_mac.mm (right): https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:104: // Currently ignoring scale factor. I am not quite sure how to handle scale factor. Generally, its just used to define scale with respect to the 1X resource. https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:138: const gfx::Image MacResourceDelegate::InterceptedIcon::GetImage() const { This can also use chrome/browser/icon_manager.h, with a few caveats - 1) Will need change to IconManager's public api. 2) Will involve async calls. Should it be preferred? https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:158: image_skia = gfx::ImageSkiaOperations::CreateMirroredImage(image_skia); I couldn't find a simple enough way to mirror an image with the current APIs. Any other easier way?
How much do we get by just implementing GetNativeImageNamed? i.e. Do the particular assets we are interested in ever get delivered by other means (can we fall back to the raster assets in those cases?) I think we should also defer RTL stuff to a follow-up CL. I'm not sure whether it even makes sense to flip these particular assets in RTL (e.g. does Safari?). We can just deliver un-flipped assets for the initial CL. https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/DEPS File chrome/browser/DEPS (right): https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/DEPS#new... chrome/browser/DEPS:4: "+blink/grit/devtools_resources.h", I think we'll need to avoid this somehow.. https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main_mac.h (right): https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main_mac.h:10: #include "chrome/browser/resource_delegate_mac.h" nit: forward declare [or possibly we don't even need to make it scoped_ptr - just have a value and pass &resource_delegate_] https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/resource... File chrome/browser/resource_delegate_mac.h (right): https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/resource... chrome/browser/resource_delegate_mac.h:5: #ifndef UI_BASE_RESOURCE_RESOURCE_DELEGATE_MAC_H_ include guard isn't right https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/resource... chrome/browser/resource_delegate_mac.h:52: class InterceptedIcon { Skia does some crazy stuff around image loading. Typically an image is only fully loaded (automatically) when it is first drawn to screen, since that determines whether it needs to load a hi-dpi asset or a regular asset. This includes fetching the appropriate representation from a wrapped NSImage if that's required. So, I don't think this is needed. https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/resource... chrome/browser/resource_delegate_mac.h:80: InterceptedIconsMap intercepted_icons_map_; I don't think you need this https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/resource... File chrome/browser/resource_delegate_mac.mm (right): https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:21: MacResourceDelegate* g_instance = nullptr; I don't think you need this https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:27: intercepted_icons_map_.insert(InterceptedIconsMap::value_type( We should try not to do anything in the constructor, since this is on the Chrome startup path, and it's super important that that is fast. https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:68: return this->GetNativeImageNamed(resource_id, no need for `this->` but also when does this get used? (can it just return gfx::Image unconditionally?) https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:75: DCHECK_EQ(rtl, ui::ResourceBundle::RTL_DISABLED); comment why this is safe? (do we need to worry about the mirroring if this is the case?) https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:76: InterceptedIconsMap::const_iterator it = I would just put a switch statement here for now, like switch (resource_id) { case IDR_FOLDER_OPEN: return ImageForHFSType(kOpenFolderIcon); case IDR_FOLDER_CLOSED: return ImageForHFSType(kGenericFolderIcon); .. } with gfx::Image ImageForHFSType(.. hfs_type) { return gfx::Image([[[NSWorkspace sharedWorkspace] iconForFileType:hfs_type]); } https://codereview.chromium.org/1585013002/diff/40001/ui/gfx/image/image_skia... File ui/gfx/image/image_skia_operations.cc (right): https://codereview.chromium.org/1585013002/diff/40001/ui/gfx/image/image_skia... ui/gfx/image/image_skia_operations.cc:427: class MirroredSource : public ImageSkiaSource { For RTL, another approach is to flip the canvas when drawing rather than flipping the image asset. See e.g. ImageButton::OnPaint(gfx::Canvas* canvas) {}
Description was changed from ========== MacViews: Use Cocoa folder icons in tree views and Developer Tools panel. This CL causes Tree views and Developer tools panel to use Mac- specific folder icons on Mac. It does this by creating a MacResourceDelegate, which intercepts certain resource ids and provides Mac specific assets for them at runtime. Currently it overrides the following resource ids - IDR_FOLDER_OPEN, IDR_FOLDER_CLOSED, IDR_FOLDER_OPEN_RTL, IDR_FOLDER_CLOSED_RTL, IMAGES_FRAME_PNG. BUG=543674 ========== to ========== MacViews: Use Cocoa folder icons in tree views. This CL causes Tree views panel to use Mac-specific folder icons on Mac. It does this by creating a MacResourceDelegate, which intercepts certain resource ids and provides Mac specific assets for them at runtime. Currently it overrides the following resource ids - IDR_FOLDER_OPEN, IDR_FOLDER_CLOSED, IDR_FOLDER_OPEN_RTL, IDR_FOLDER_CLOSED_RTL. BUG=543674 ==========
PTAL Trent. Thanks. https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/DEPS File chrome/browser/DEPS (right): https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/DEPS#new... chrome/browser/DEPS:4: "+blink/grit/devtools_resources.h", On 2016/01/14 23:43:50, tapted wrote: > I think we'll need to avoid this somehow.. Done. https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main_mac.h (right): https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main_mac.h:10: #include "chrome/browser/resource_delegate_mac.h" On 2016/01/14 23:43:50, tapted wrote: > nit: forward declare [or possibly we don't even need to make it scoped_ptr - > just have a value and pass &resource_delegate_] Have changed to a value type, but we can't forward declare for a value type, right? https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/resource... File chrome/browser/resource_delegate_mac.h (right): https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/resource... chrome/browser/resource_delegate_mac.h:5: #ifndef UI_BASE_RESOURCE_RESOURCE_DELEGATE_MAC_H_ On 2016/01/14 23:43:50, tapted wrote: > include guard isn't right Done. https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/resource... chrome/browser/resource_delegate_mac.h:52: class InterceptedIcon { On 2016/01/14 23:43:50, tapted wrote: > Skia does some crazy stuff around image loading. Typically an image is only > fully loaded (automatically) when it is first drawn to screen, since that > determines whether it needs to load a hi-dpi asset or a regular asset. This > includes fetching the appropriate representation from a wrapped NSImage if > that's required. > > So, I don't think this is needed. Done. https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/resource... chrome/browser/resource_delegate_mac.h:80: InterceptedIconsMap intercepted_icons_map_; On 2016/01/14 23:43:50, tapted wrote: > I don't think you need this Done. https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/resource... File chrome/browser/resource_delegate_mac.mm (right): https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:21: MacResourceDelegate* g_instance = nullptr; On 2016/01/14 23:43:50, tapted wrote: > I don't think you need this Done. https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:27: intercepted_icons_map_.insert(InterceptedIconsMap::value_type( On 2016/01/14 23:43:50, tapted wrote: > We should try not to do anything in the constructor, since this is on the Chrome > startup path, and it's super important that that is fast. Done. https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:68: return this->GetNativeImageNamed(resource_id, On 2016/01/14 23:43:50, tapted wrote: > no need for `this->` but also when does this get used? (can it just return > gfx::Image unconditionally?) tree_view.cc uses the GetImageNamed(..) call. Should I change it there? https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:75: DCHECK_EQ(rtl, ui::ResourceBundle::RTL_DISABLED); On 2016/01/14 23:43:50, tapted wrote: > comment why this is safe? (do we need to worry about the mirroring if this is > the case?) Done. This is because on Mac and mostly other platforms as well, there are separate mirrored assets. Hence ImageRTL::RTL_ENABLED is not used. Same is done in resource_bundle_mac.mm - https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/resource/r.... https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:76: InterceptedIconsMap::const_iterator it = On 2016/01/14 23:43:50, tapted wrote: > I would just put a switch statement here for now, like > > switch (resource_id) { > case IDR_FOLDER_OPEN: > return ImageForHFSType(kOpenFolderIcon); > case IDR_FOLDER_CLOSED: > return ImageForHFSType(kGenericFolderIcon); > .. > } > > > with > > gfx::Image ImageForHFSType(.. hfs_type) { > return gfx::Image([[[NSWorkspace sharedWorkspace] iconForFileType:hfs_type]); > } Done. The default return size is 32X32, which is big for the Edit Bookmark dialog, hence have provided an IconSize option as well. https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:104: // Currently ignoring scale factor. On 2016/01/14 04:21:33, karandeepb wrote: > I am not quite sure how to handle scale factor. Generally, its just used to > define scale with respect to the 1X resource. Done. https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:158: image_skia = gfx::ImageSkiaOperations::CreateMirroredImage(image_skia); On 2016/01/14 04:21:33, karandeepb wrote: > I couldn't find a simple enough way to mirror an image with the current APIs. > Any other easier way? Done. https://codereview.chromium.org/1585013002/diff/40001/ui/gfx/image/image_skia... File ui/gfx/image/image_skia_operations.cc (right): https://codereview.chromium.org/1585013002/diff/40001/ui/gfx/image/image_skia... ui/gfx/image/image_skia_operations.cc:427: class MirroredSource : public ImageSkiaSource { On 2016/01/14 23:43:50, tapted wrote: > For RTL, another approach is to flip the canvas when drawing rather than > flipping the image asset. See e.g. ImageButton::OnPaint(gfx::Canvas* canvas) {} Done. https://codereview.chromium.org/1585013002/diff/60001/chrome/browser/resource... File chrome/browser/resource_delegate_mac.mm (right): https://codereview.chromium.org/1585013002/diff/60001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:25: iconForFileType:NSFileTypeForHFSTypeCode(hfs_type)] retain]; Can you check if calling retain here is ok? gfx::Image(NSImage *) expects to take ownership of the NSImage but it doesn't call retain on it.
https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main_mac.h (right): https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main_mac.h:10: #include "chrome/browser/resource_delegate_mac.h" On 2016/01/18 00:31:17, karandeepb wrote: > On 2016/01/14 23:43:50, tapted wrote: > > nit: forward declare [or possibly we don't even need to make it scoped_ptr - > > just have a value and pass &resource_delegate_] > > Have changed to a value type, but we can't forward declare for a value type, > right? yup - exactly :) https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/resource... File chrome/browser/resource_delegate_mac.mm (right): https://codereview.chromium.org/1585013002/diff/40001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:68: return this->GetNativeImageNamed(resource_id, On 2016/01/18 00:31:17, karandeepb wrote: > On 2016/01/14 23:43:50, tapted wrote: > > no need for `this->` but also when does this get used? (can it just return > > gfx::Image unconditionally?) > > tree_view.cc uses the GetImageNamed(..) call. Should I change it there? So, I *think* what you have is correct. A ResourceBundle OWNER may know for sure. I'm not certain that it is always OK to return an NSImage-backed resource from GetImageNamed, but it seems to be supported and I don't think it would work otherwise. https://codereview.chromium.org/1585013002/diff/60001/chrome/browser/resource... File chrome/browser/resource_delegate_mac.mm (right): https://codereview.chromium.org/1585013002/diff/60001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:18: BIG // 32X32. `BIG` is unused. I think we want const int kFolderIconReportedSizeDIP = 16; But only if 16 is correct. E.g. the raster asset is 19x18 pixels. Ideally, the size on Mac would "match", but we want ensure there is no distortion. Can you post some screenshots on the bug? https://codereview.chromium.org/1585013002/diff/60001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:22: // gfx::Image. In case the icon can't be retreived, an empty image is returned. I don't think the "In case the icon..." part is right. Perhaps remove that part of the comment and DCHECK(icon); after retrieving it from NSWorkspace, since it should "always" exist. https://codereview.chromium.org/1585013002/diff/60001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:23: gfx::Image ImageForHFSType(int hfs_type, IconSize icon_size) { IconSize -> int https://codereview.chromium.org/1585013002/diff/60001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:25: iconForFileType:NSFileTypeForHFSTypeCode(hfs_type)] retain]; On 2016/01/18 00:31:18, karandeepb wrote: > Can you check if calling retain here is ok? gfx::Image(NSImage *) expects to > take ownership of the NSImage but it doesn't call retain on it. ooh - yeah gfx::Image is weird. the return from iconForFileType will be autoreleased, so it's weird to retain it here. I think we can instead do `return gfx::Image([icon retain]);` to keep the weirdness together https://codereview.chromium.org/1585013002/diff/60001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:64: // Flipped images are not used on Mac. I think it was only used for GTK and we can remove it -- https://codereview.chromium.org/1601583002/ https://codereview.chromium.org/1585013002/diff/60001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:67: switch (resource_id) { Needs a comment about why this is correct re: the _RTL assets. E.g. "On Mac, return the same assets for the _RTL resources. Consistent with, e.g., Safari." https://codereview.chromium.org/1585013002/diff/60001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:71: break; nit: no break required https://codereview.chromium.org/1585013002/diff/60001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:75: break; nit: no break
PTAL Trent. Thanks. https://codereview.chromium.org/1585013002/diff/60001/chrome/browser/resource... File chrome/browser/resource_delegate_mac.mm (right): https://codereview.chromium.org/1585013002/diff/60001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:18: BIG // 32X32. On 2016/01/18 04:15:42, tapted wrote: > `BIG` is unused. I think we want > > const int kFolderIconReportedSizeDIP = 16; > > But only if 16 is correct. E.g. the raster asset is 19x18 pixels. Ideally, the > size on Mac would "match", but we want ensure there is no distortion. Can you > post some screenshots on the bug? Done. Have attached screenshots on the bug report. https://codereview.chromium.org/1585013002/diff/60001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:22: // gfx::Image. In case the icon can't be retreived, an empty image is returned. On 2016/01/18 04:15:42, tapted wrote: > I don't think the "In case the icon..." part is right. Perhaps remove that part > of the comment and DCHECK(icon); after retrieving it from NSWorkspace, since it > should "always" exist. Done. https://codereview.chromium.org/1585013002/diff/60001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:23: gfx::Image ImageForHFSType(int hfs_type, IconSize icon_size) { On 2016/01/18 04:15:42, tapted wrote: > IconSize -> int Done. https://codereview.chromium.org/1585013002/diff/60001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:25: iconForFileType:NSFileTypeForHFSTypeCode(hfs_type)] retain]; On 2016/01/18 04:15:42, tapted wrote: > On 2016/01/18 00:31:18, karandeepb wrote: > > Can you check if calling retain here is ok? gfx::Image(NSImage *) expects to > > take ownership of the NSImage but it doesn't call retain on it. > > ooh - yeah gfx::Image is weird. the return from iconForFileType will be > autoreleased, so it's weird to retain it here. I think we can instead do `return > gfx::Image([icon retain]);` to keep the weirdness together Done. https://codereview.chromium.org/1585013002/diff/60001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:64: // Flipped images are not used on Mac. On 2016/01/18 04:15:42, tapted wrote: > I think it was only used for GTK and we can remove it -- > https://codereview.chromium.org/1601583002/ Will remove once the CL lands. https://codereview.chromium.org/1585013002/diff/60001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:67: switch (resource_id) { On 2016/01/18 04:15:42, tapted wrote: > Needs a comment about why this is correct re: the _RTL assets. E.g. "On Mac, > return the same assets for the _RTL resources. Consistent with, e.g., Safari." Done. https://codereview.chromium.org/1585013002/diff/60001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:71: break; On 2016/01/18 04:15:42, tapted wrote: > nit: no break required Done. https://codereview.chromium.org/1585013002/diff/60001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:75: break; On 2016/01/18 04:15:42, tapted wrote: > nit: no break Done.
looking good! I think this is worth going ahead with. I would just add some "rationale" to the CL description, since what we're doing is a bit unconventional. E.g. a first paragraph like "Chrome's Cocoa UI retrieves some image assets from the System using APIs such as +[NSImage imageNamed:key]. To support this under toolkit-views, provide a ui::ResourceDelegate on Mac that can transparently map some IDR_ resource IDs to a system-provided asset. Initially do this for folder icons in views::TreeView to provide a folder icon that is consistent with the running OSX version." I think it also needs a note that although IDR_FOLDER_CLOSED appears in ui_resources (and not views_resources) it's currently unused on Mac. (chrome://syncfs-internals/ refers to it in its CSS, but doesn't seem to actually use that style anywhere in its html or js). That is, by remapping these IDs, we are not changing any icon on Chrome Mac, except under Views. https://codereview.chromium.org/1585013002/diff/80001/chrome/browser/resource... File chrome/browser/resource_delegate_mac.h (right): https://codereview.chromium.org/1585013002/diff/80001/chrome/browser/resource... chrome/browser/resource_delegate_mac.h:11: namespace ui { whoops - stuff under chrome/* doesn't (usually) belong in any namespace (sometimes it geta chrome:: but that's rare and for specific scenarios) https://codereview.chromium.org/1585013002/diff/80001/chrome/browser/resource... chrome/browser/resource_delegate_mac.h:13: // A resource bundle delegate. Primary purpose is to intercept certain resource A resource bundle delegate -> A resource bundle delegate for Mac https://codereview.chromium.org/1585013002/diff/80001/chrome/browser/resource... chrome/browser/resource_delegate_mac.h:14: // ids and to provide Mac specific assets for them at runtime. There are other ways to provide Mac-specific assets, I think the distinguishing characteristic is that it facilitates a way to provide assets retrieved from system APIs rather than the resource bundle. https://codereview.chromium.org/1585013002/diff/80001/chrome/browser/resource... chrome/browser/resource_delegate_mac.h:15: class UI_BASE_EXPORT MacResourceDelegate : public ui::ResourceBundle::Delegate { remove UI_BASE_EXPORT https://codereview.chromium.org/1585013002/diff/80001/chrome/browser/resource... File chrome/browser/resource_delegate_mac.mm (right): https://codereview.chromium.org/1585013002/diff/80001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:90: return scoped_ptr<gfx::Font>(); I think `return nullptr` should work now that we have C++11
https://codereview.chromium.org/1585013002/diff/80001/chrome/browser/resource... File chrome/browser/resource_delegate_mac.mm (right): https://codereview.chromium.org/1585013002/diff/80001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:38: }; oh and there shouldn't be semicolons after all these methods -- there's a linting script that picks this up, but it's only run on presubmit in certain subdirectories of chrome
Description was changed from ========== MacViews: Use Cocoa folder icons in tree views. This CL causes Tree views panel to use Mac-specific folder icons on Mac. It does this by creating a MacResourceDelegate, which intercepts certain resource ids and provides Mac specific assets for them at runtime. Currently it overrides the following resource ids - IDR_FOLDER_OPEN, IDR_FOLDER_CLOSED, IDR_FOLDER_OPEN_RTL, IDR_FOLDER_CLOSED_RTL. BUG=543674 ========== to ========== MacViews: Use Cocoa folder icons in tree views. Chrome's Cocoa UI retrieves some image assets from the System using APIs such as +[NSImage imageNamed:key]. To support this under toolkit-views, provide a ui::ResourceDelegate on Mac that can transparently map some IDR_ resource IDs to a system-provided asset. Initially do this for folder icons in views::TreeView to provide a folder icon that is consistent with the running OSX version. The following resource ids are currently intercepted - IDR_FOLDER_OPEN, IDR_FOLDER_CLOSED, IDR_FOLDER_OPEN_RTL and IDR_FOLDER_CLOSED_RTL. Although IDR_FOLDER_CLOSED is in ui/resources (and not ui/views/resources), it is currently unused on Mac. (chrome://syncfs-internals/ refers to it in its CSS, but doesn't actually use that style anywhere in its html or js). Hence this CL, does not alter any icons on Chrome Mac, except under Views. BUG=543674 ==========
Description was changed from ========== MacViews: Use Cocoa folder icons in tree views. Chrome's Cocoa UI retrieves some image assets from the System using APIs such as +[NSImage imageNamed:key]. To support this under toolkit-views, provide a ui::ResourceDelegate on Mac that can transparently map some IDR_ resource IDs to a system-provided asset. Initially do this for folder icons in views::TreeView to provide a folder icon that is consistent with the running OSX version. The following resource ids are currently intercepted - IDR_FOLDER_OPEN, IDR_FOLDER_CLOSED, IDR_FOLDER_OPEN_RTL and IDR_FOLDER_CLOSED_RTL. Although IDR_FOLDER_CLOSED is in ui/resources (and not ui/views/resources), it is currently unused on Mac. (chrome://syncfs-internals/ refers to it in its CSS, but doesn't actually use that style anywhere in its html or js). Hence this CL, does not alter any icons on Chrome Mac, except under Views. BUG=543674 ========== to ========== MacViews: Use Cocoa folder icons in tree views. Chrome's Cocoa UI retrieves some image assets from the System using APIs such as +[NSImage imageNamed:key]. To support this under toolkit-views, provide a ui::ResourceDelegate on Mac that can transparently map some IDR_ resource IDs to a system-provided asset. Initially do this for folder icons in views::TreeView to provide a folder icon that is consistent with the running OSX version. The following resource ids are currently intercepted - IDR_FOLDER_OPEN, IDR_FOLDER_CLOSED, IDR_FOLDER_OPEN_RTL and IDR_FOLDER_CLOSED_RTL. Although IDR_FOLDER_CLOSED is in ui/resources (and not ui/views/resources), it is currently unused on Mac. (chrome://syncfs-internals/ refers to it in its CSS, but doesn't actually use that style anywhere in its html or js). Hence this CL, does not alter any icons on Chrome Mac, except under Views. BUG=543674 ==========
Description was changed from ========== MacViews: Use Cocoa folder icons in tree views. Chrome's Cocoa UI retrieves some image assets from the System using APIs such as +[NSImage imageNamed:key]. To support this under toolkit-views, provide a ui::ResourceDelegate on Mac that can transparently map some IDR_ resource IDs to a system-provided asset. Initially do this for folder icons in views::TreeView to provide a folder icon that is consistent with the running OSX version. The following resource ids are currently intercepted - IDR_FOLDER_OPEN, IDR_FOLDER_CLOSED, IDR_FOLDER_OPEN_RTL and IDR_FOLDER_CLOSED_RTL. Although IDR_FOLDER_CLOSED is in ui/resources (and not ui/views/resources), it is currently unused on Mac. (chrome://syncfs-internals/ refers to it in its CSS, but doesn't actually use that style anywhere in its html or js). Hence this CL, does not alter any icons on Chrome Mac, except under Views. BUG=543674 ========== to ========== MacViews: Use Cocoa folder icons in tree views. Chrome's Cocoa UI retrieves some image assets from the System using APIs such as +[NSImage imageNamed:key]. To support this under toolkit-views, provide a ui::ResourceDelegate on Mac that can transparently map some IDR_ resource IDs to a system-provided asset. Initially do this for folder icons in views::TreeView to provide a folder icon that is consistent with the running OSX version. The following resource ids are currently intercepted - IDR_FOLDER_OPEN, IDR_FOLDER_CLOSED, IDR_FOLDER_OPEN_RTL and IDR_FOLDER_CLOSED_RTL. Although IDR_FOLDER_CLOSED is in ui/resources (and not ui/views/resources), it is currently unused on Mac (chrome://syncfs-internals/ refers to it in its CSS, but doesn't actually use that style anywhere in its html or js). Hence this CL, does not alter any icons on Chrome Mac, except under Views. BUG=543674 ==========
PTAL Trent. Thanks. https://codereview.chromium.org/1585013002/diff/80001/chrome/browser/resource... File chrome/browser/resource_delegate_mac.h (right): https://codereview.chromium.org/1585013002/diff/80001/chrome/browser/resource... chrome/browser/resource_delegate_mac.h:11: namespace ui { On 2016/01/19 00:36:52, tapted wrote: > whoops - stuff under chrome/* doesn't (usually) belong in any namespace > (sometimes it geta chrome:: but that's rare and for specific scenarios) Done. https://codereview.chromium.org/1585013002/diff/80001/chrome/browser/resource... chrome/browser/resource_delegate_mac.h:13: // A resource bundle delegate. Primary purpose is to intercept certain resource On 2016/01/19 00:36:52, tapted wrote: > A resource bundle delegate -> A resource bundle delegate for Mac Done. https://codereview.chromium.org/1585013002/diff/80001/chrome/browser/resource... chrome/browser/resource_delegate_mac.h:14: // ids and to provide Mac specific assets for them at runtime. On 2016/01/19 00:36:52, tapted wrote: > There are other ways to provide Mac-specific assets, I think the distinguishing > characteristic is that it facilitates a way to provide assets retrieved from > system APIs rather than the resource bundle. Done. https://codereview.chromium.org/1585013002/diff/80001/chrome/browser/resource... chrome/browser/resource_delegate_mac.h:15: class UI_BASE_EXPORT MacResourceDelegate : public ui::ResourceBundle::Delegate { On 2016/01/19 00:36:52, tapted wrote: > remove UI_BASE_EXPORT Done. https://codereview.chromium.org/1585013002/diff/80001/chrome/browser/resource... File chrome/browser/resource_delegate_mac.mm (right): https://codereview.chromium.org/1585013002/diff/80001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:38: }; On 2016/01/19 00:38:47, tapted wrote: > oh and there shouldn't be semicolons after all these methods -- there's a > linting script that picks this up, but it's only run on presubmit in certain > subdirectories of chrome Done. https://codereview.chromium.org/1585013002/diff/80001/chrome/browser/resource... chrome/browser/resource_delegate_mac.mm:90: return scoped_ptr<gfx::Font>(); On 2016/01/19 00:36:52, tapted wrote: > I think `return nullptr` should work now that we have C++11 Done.
lgtm
karandeepb@chromium.org changed reviewers: + rsesek@chromium.org
PTAL @rsesek. Thanks.
LGTM
The CQ bit was checked by karandeepb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1585013002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1585013002/120001
Message was sent while issue was closed.
Description was changed from ========== MacViews: Use Cocoa folder icons in tree views. Chrome's Cocoa UI retrieves some image assets from the System using APIs such as +[NSImage imageNamed:key]. To support this under toolkit-views, provide a ui::ResourceDelegate on Mac that can transparently map some IDR_ resource IDs to a system-provided asset. Initially do this for folder icons in views::TreeView to provide a folder icon that is consistent with the running OSX version. The following resource ids are currently intercepted - IDR_FOLDER_OPEN, IDR_FOLDER_CLOSED, IDR_FOLDER_OPEN_RTL and IDR_FOLDER_CLOSED_RTL. Although IDR_FOLDER_CLOSED is in ui/resources (and not ui/views/resources), it is currently unused on Mac (chrome://syncfs-internals/ refers to it in its CSS, but doesn't actually use that style anywhere in its html or js). Hence this CL, does not alter any icons on Chrome Mac, except under Views. BUG=543674 ========== to ========== MacViews: Use Cocoa folder icons in tree views. Chrome's Cocoa UI retrieves some image assets from the System using APIs such as +[NSImage imageNamed:key]. To support this under toolkit-views, provide a ui::ResourceDelegate on Mac that can transparently map some IDR_ resource IDs to a system-provided asset. Initially do this for folder icons in views::TreeView to provide a folder icon that is consistent with the running OSX version. The following resource ids are currently intercepted - IDR_FOLDER_OPEN, IDR_FOLDER_CLOSED, IDR_FOLDER_OPEN_RTL and IDR_FOLDER_CLOSED_RTL. Although IDR_FOLDER_CLOSED is in ui/resources (and not ui/views/resources), it is currently unused on Mac (chrome://syncfs-internals/ refers to it in its CSS, but doesn't actually use that style anywhere in its html or js). Hence this CL, does not alter any icons on Chrome Mac, except under Views. BUG=543674 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Use Cocoa folder icons in tree views. Chrome's Cocoa UI retrieves some image assets from the System using APIs such as +[NSImage imageNamed:key]. To support this under toolkit-views, provide a ui::ResourceDelegate on Mac that can transparently map some IDR_ resource IDs to a system-provided asset. Initially do this for folder icons in views::TreeView to provide a folder icon that is consistent with the running OSX version. The following resource ids are currently intercepted - IDR_FOLDER_OPEN, IDR_FOLDER_CLOSED, IDR_FOLDER_OPEN_RTL and IDR_FOLDER_CLOSED_RTL. Although IDR_FOLDER_CLOSED is in ui/resources (and not ui/views/resources), it is currently unused on Mac (chrome://syncfs-internals/ refers to it in its CSS, but doesn't actually use that style anywhere in its html or js). Hence this CL, does not alter any icons on Chrome Mac, except under Views. BUG=543674 ========== to ========== MacViews: Use Cocoa folder icons in tree views. Chrome's Cocoa UI retrieves some image assets from the System using APIs such as +[NSImage imageNamed:key]. To support this under toolkit-views, provide a ui::ResourceDelegate on Mac that can transparently map some IDR_ resource IDs to a system-provided asset. Initially do this for folder icons in views::TreeView to provide a folder icon that is consistent with the running OSX version. The following resource ids are currently intercepted - IDR_FOLDER_OPEN, IDR_FOLDER_CLOSED, IDR_FOLDER_OPEN_RTL and IDR_FOLDER_CLOSED_RTL. Although IDR_FOLDER_CLOSED is in ui/resources (and not ui/views/resources), it is currently unused on Mac (chrome://syncfs-internals/ refers to it in its CSS, but doesn't actually use that style anywhere in its html or js). Hence this CL, does not alter any icons on Chrome Mac, except under Views. BUG=543674 Committed: https://crrev.com/e60fc3cf40cbc732e5ccaf6ff3785cc1351fbe03 Cr-Commit-Position: refs/heads/master@{#370819} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e60fc3cf40cbc732e5ccaf6ff3785cc1351fbe03 Cr-Commit-Position: refs/heads/master@{#370819} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
