Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
309483009:
Remaining bits to get views_examples_with_content_exe to work on Mac (Closed)
|
Created:
6 years, 6 months ago by tapted Modified:
6 years, 5 months ago CC:
chromium-reviews, ben+aura_chromium.org, sievers+watch_chromium.org, rsesek+watch_chromium.org, dmazzoni+watch_chromium.org, sadrul, tdanderson+views_chromium.org, aboxhall+watch_chromium.org, jam, jbauman+watch_chromium.org, darin-cc_chromium.org, kalyank, tdresser+watch_chromium.org, yuzo+watch_chromium.org, piman+watch_chromium.org, ben+views_chromium.org, danakj+watch_chromium.org, chrome-apps-syd-reviews_chromium.org, plundblad+watch_chromium.org, Ian Vollick, tfarina, dtseng+watch_chromium.org, cc-bugs_chromium.org, Andre, mac-views-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRemaining bits to get views_examples_with_content_exe to work on Mac
The example window currently doesn't activate itself properly because it
lacks an activation policy and a menubar.
This CL sets the activation policy, adds a menubar and defers window
creation until the applicationDidFinishLoading notification. Creating a
window before this can activate and receive key events, but the menubar
will not switch properly.
With this, views_examples_with_content_exe compiles and runs nicely on a
Mac build with `GYP_DEFINES=toolkit-views=1`. Note the `WebView` example
will crash for now because views::NativeViewHost is not yet implemented
for mac.
BUG=395507
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284883
Patch Set 1 #Patch Set 2 : Big trim #Patch Set 3 : more non-critical stuff trimmed #Patch Set 4 : neater still #
Total comments: 4
Patch Set 5 : Updated to handle key events #Patch Set 6 : rebase to master@r275900 #
Total comments: 10
Patch Set 7 : rebase onto crrev/309483009. Still needs polish. #Patch Set 8 : rebased. Trimmed. #Patch Set 9 : Fix activation #Patch Set 10 : rebase to master! #
Total comments: 8
Patch Set 11 : respond to commments #Patch Set 12 : fix stray space #Patch Set 13 : Quit literal + TODO #Messages
Total messages: 13 (0 generated)
https://codereview.chromium.org/309483009/diff/70001/ui/gfx/mac/point_utils.mm File ui/gfx/mac/point_utils.mm (right): https://codereview.chromium.org/309483009/diff/70001/ui/gfx/mac/point_utils.m... ui/gfx/mac/point_utils.mm:17: return NSMaxY([[[NSScreen screens] objectAtIndex:0] frame]); How about multiple screens? https://codereview.chromium.org/309483009/diff/70001/ui/views/cocoa/bridged_c... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/309483009/diff/70001/ui/views/cocoa/bridged_c... ui/views/cocoa/bridged_content_view.mm:53: - (void)setFrame:(NSRect)newFrame { Probably better to override the more primitive -setFrameSize: instead. setFrame's implementation calls setFrameOrigin and setFrameSize.
Thanks for taking a look. This now has some polish and unit tests in https://codereview.chromium.org/329743002/ where I'll pick this up from. https://codereview.chromium.org/309483009/diff/70001/ui/gfx/mac/point_utils.mm File ui/gfx/mac/point_utils.mm (right): https://codereview.chromium.org/309483009/diff/70001/ui/gfx/mac/point_utils.m... ui/gfx/mac/point_utils.mm:17: return NSMaxY([[[NSScreen screens] objectAtIndex:0] frame]); On 2014/06/04 23:55:08, Andre wrote: > How about multiple screens? screen coordinates are always relative to the bottom left of the screen[0], which is the screen with the menu bar on it. screen_mac.mm actually has something like this (ConvertCoordinateSystem) in a private namespace. It's possible we won't need point_utils.mm .... I keep finding reasons to say "don't really need to use it here" ;) https://codereview.chromium.org/309483009/diff/70001/ui/views/cocoa/bridged_c... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/309483009/diff/70001/ui/views/cocoa/bridged_c... ui/views/cocoa/bridged_content_view.mm:53: - (void)setFrame:(NSRect)newFrame { On 2014/06/04 23:55:08, Andre wrote: > Probably better to override the more primitive -setFrameSize: instead. > setFrame's implementation calls setFrameOrigin and setFrameSize. > Done.
https://codereview.chromium.org/309483009/diff/100001/ui/gfx/mac/point_utils.h File ui/gfx/mac/point_utils.h (right): https://codereview.chromium.org/309483009/diff/100001/ui/gfx/mac/point_utils.... ui/gfx/mac/point_utils.h:18: GFX_EXPORT NSPoint ScreenPointToNSPoint(const gfx::Point& point); Document each of these. https://codereview.chromium.org/309483009/diff/100001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/309483009/diff/100001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:36: - (views::View*)view { Use @synthesize instead? https://codereview.chromium.org/309483009/diff/100001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:77: hostedView_->Paint(&canvas, views::CullSet()); It would be great to optimize this to use dirtyRect. https://codereview.chromium.org/309483009/diff/100001/ui/views_content_client... File ui/views_content_client/views_content_client_main_parts_mac.mm (right): https://codereview.chromium.org/309483009/diff/100001/ui/views_content_client... ui/views_content_client/views_content_client_main_parts_mac.mm:86: NSString* quitTitle = [@"Quit " stringByAppendingString:appName]; I assume this is all temporary? This would need to be localized.
Thanks for the review! I've incorporated some stuff into https://codereview.chromium.org/329743002/ and rebased this upon that CL to better reflect the subject -- the CL here still needs some polish though. I'll add you to the review in https://codereview.chromium.org/329743002/ -- it's mostly a subset of this, with some extra polish (and tests \o/!). https://codereview.chromium.org/309483009/diff/100001/ui/gfx/mac/point_utils.h File ui/gfx/mac/point_utils.h (right): https://codereview.chromium.org/309483009/diff/100001/ui/gfx/mac/point_utils.... ui/gfx/mac/point_utils.h:18: GFX_EXPORT NSPoint ScreenPointToNSPoint(const gfx::Point& point); On 2014/06/11 14:53:48, rsesek wrote: > Document each of these. Will do this in a follow-up (although I'm still trying to convince myself we need this file ;). https://codereview.chromium.org/309483009/diff/100001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/309483009/diff/100001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:36: - (views::View*)view { On 2014/06/11 14:53:48, rsesek wrote: > Use @synthesize instead? Done. (in https://codereview.chromium.org/329743002/) https://codereview.chromium.org/309483009/diff/100001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:77: hostedView_->Paint(&canvas, views::CullSet()); On 2014/06/11 14:53:48, rsesek wrote: > It would be great to optimize this to use dirtyRect. At this call site, it should be clipped by the canvas in View::Paint - but you're quite right that I'll need to do this at the other side of the bridge; in NativeWidgetMac (which also needs a coordiate transform) (see https://codereview.chromium.org/329743002/diff/70001/ui/views/widget/native_w... ). https://codereview.chromium.org/309483009/diff/100001/ui/views_content_client... File ui/views_content_client/views_content_client_main_parts_mac.mm (right): https://codereview.chromium.org/309483009/diff/100001/ui/views_content_client... ui/views_content_client/views_content_client_main_parts_mac.mm:86: NSString* quitTitle = [@"Quit " stringByAppendingString:appName]; On 2014/06/11 14:53:48, rsesek wrote: > I assume this is all temporary? This would need to be localized. Yep - this example code is not shipped, but it might actually be easy to get a localized string here anyway. I'll investigate.
Hi Robert PTAL. This is now ready (the extra widget stuff it needed has now all landed with tests). https://codereview.chromium.org/309483009/diff/100001/ui/gfx/mac/point_utils.h File ui/gfx/mac/point_utils.h (right): https://codereview.chromium.org/309483009/diff/100001/ui/gfx/mac/point_utils.... ui/gfx/mac/point_utils.h:18: GFX_EXPORT NSPoint ScreenPointToNSPoint(const gfx::Point& point); On 2014/06/12 09:09:38, tapted wrote: > On 2014/06/11 14:53:48, rsesek wrote: > > Document each of these. > > Will do this in a follow-up (although I'm still trying to convince myself we > need this file ;). Done - this bit now landed (with documentation) in r279975. https://codereview.chromium.org/309483009/diff/100001/ui/views_content_client... File ui/views_content_client/views_content_client_main_parts_mac.mm (right): https://codereview.chromium.org/309483009/diff/100001/ui/views_content_client... ui/views_content_client/views_content_client_main_parts_mac.mm:86: NSString* quitTitle = [@"Quit " stringByAppendingString:appName]; On 2014/06/12 09:09:38, tapted wrote: > On 2014/06/11 14:53:48, rsesek wrote: > > I assume this is all temporary? This would need to be localized. > > Yep - this example code is not shipped, but it might actually be easy to get a > localized string here anyway. I'll investigate. Sadly, no easy way to do this without pulling in a dependency we don't want, or adding a string to the build that isn't used in anything that we ship. Added a note.
https://codereview.chromium.org/309483009/diff/320001/ui/views_content_client... File ui/views_content_client/views_content_client_main_parts_mac.mm (right): https://codereview.chromium.org/309483009/diff/320001/ui/views_content_client... ui/views_content_client/views_content_client_main_parts_mac.mm:17: @interface ContentClientAppController : NSObject<NSApplicationDelegate> { naming: ViewContentClientAppController, to distinguish it from say content_shell. https://codereview.chromium.org/309483009/diff/320001/ui/views_content_client... ui/views_content_client/views_content_client_main_parts_mac.mm:56: [[NSApplication sharedApplication] setDelegate:app_controller_]; This is only for demo applications, right? Otherwise you'd need some form of extensibility here. https://codereview.chromium.org/309483009/diff/320001/ui/views_content_client... ui/views_content_client/views_content_client_main_parts_mac.mm:107: // TODO(tapted): Localize "Quit" if this is ever used for a released binary. I wonder if you can use the AppKit string for this, if there's a way to get that…
https://codereview.chromium.org/309483009/diff/320001/ui/views_content_client... File ui/views_content_client/views_content_client_main_parts_mac.mm (right): https://codereview.chromium.org/309483009/diff/320001/ui/views_content_client... ui/views_content_client/views_content_client_main_parts_mac.mm:17: @interface ContentClientAppController : NSObject<NSApplicationDelegate> { On 2014/07/21 17:53:54, rsesek wrote: > naming: ViewContentClientAppController, to distinguish it from say > content_shell. Done. https://codereview.chromium.org/309483009/diff/320001/ui/views_content_client... ui/views_content_client/views_content_client_main_parts_mac.mm:56: [[NSApplication sharedApplication] setDelegate:app_controller_]; On 2014/07/21 17:53:54, rsesek wrote: > This is only for demo applications, right? Otherwise you'd need some form of > extensibility here. Yep - just demo apps. Currently views_examples_with_content_exe and app_list_demo (and probably ash_shell in future too). https://codereview.chromium.org/309483009/diff/320001/ui/views_content_client... ui/views_content_client/views_content_client_main_parts_mac.mm:107: // TODO(tapted): Localize "Quit" if this is ever used for a released binary. On 2014/07/21 17:53:54, rsesek wrote: > I wonder if you can use the AppKit string for this, if there's a way to get > that… Found a way to do this using info gathered here: http://www.alexcurylo.com/blog/tag/localization/ Hunting around in /System/Library/Frameworks/AppKit.framework/Versions/C/Resources/English.lproj it turns out `Document.strings` contains Quit (first you need to copy it somewhere and do `plutil -convert xml1 Document.strings` to make it readable). See what you think... it's not exactly pretty, but I don't think there's a nicer way to get it from AppKit.
LGTM https://codereview.chromium.org/309483009/diff/320001/ui/views_content_client... File ui/views_content_client/views_content_client_main_parts_mac.mm (right): https://codereview.chromium.org/309483009/diff/320001/ui/views_content_client... ui/views_content_client/views_content_client_main_parts_mac.mm:107: // TODO(tapted): Localize "Quit" if this is ever used for a released binary. On 2014/07/22 00:06:51, tapted wrote: > On 2014/07/21 17:53:54, rsesek wrote: > > I wonder if you can use the AppKit string for this, if there's a way to get > > that… > > Found a way to do this using info gathered here: > http://www.alexcurylo.com/blog/tag/localization/ > > Hunting around in > /System/Library/Frameworks/AppKit.framework/Versions/C/Resources/English.lproj > it turns out `Document.strings` contains Quit (first you need to copy it > somewhere and do `plutil -convert xml1 Document.strings` to make it readable). > > See what you think... it's not exactly pretty, but I don't think there's a nicer > way to get it from AppKit. Hm yeah, definitely not as pretty. I think unlocalized is fine and probably better than using this undocumented bit. Interesting find, though!
+sky for OWNERS in src/ui https://codereview.chromium.org/309483009/diff/320001/ui/views_content_client... File ui/views_content_client/views_content_client_main_parts_mac.mm (right): https://codereview.chromium.org/309483009/diff/320001/ui/views_content_client... ui/views_content_client/views_content_client_main_parts_mac.mm:107: // TODO(tapted): Localize "Quit" if this is ever used for a released binary. On 2014/07/22 00:39:12, rsesek wrote: > On 2014/07/22 00:06:51, tapted wrote: > > On 2014/07/21 17:53:54, rsesek wrote: > > > I wonder if you can use the AppKit string for this, if there's a way to get > > > that… > > > > Found a way to do this using info gathered here: > > http://www.alexcurylo.com/blog/tag/localization/ > > > > Hunting around in > > /System/Library/Frameworks/AppKit.framework/Versions/C/Resources/English.lproj > > it turns out `Document.strings` contains Quit (first you need to copy it > > somewhere and do `plutil -convert xml1 Document.strings` to make it readable). > > > > See what you think... it's not exactly pretty, but I don't think there's a > nicer > > way to get it from AppKit. > > Hm yeah, definitely not as pretty. I think unlocalized is fine and probably > better than using this undocumented bit. Interesting find, though! Done - (rolled it back to unlocalized).
LGTM
The CQ bit was checked by tapted@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tapted@chromium.org/309483009/380001
Message was sent while issue was closed.
Change committed as 284883 |
