Check if drawFocusRingMaskWithFrame:inView: exists when drawing focus ring

Check if drawFocusRingMaskWithFrame:inView: exists when drawing focus ring

drawFocusRingMaskWithFrame:inView: only exists for Mac OS X 10.7 and later.

BUG=328814, 425274
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196915

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197740

[keishi, 2014-07-02 14:17:28]

drawFocusRingMaskWithFrame:inView: is only available for 10.7 and later so check
if it exists.

[Robert Sesek, 2014-07-02 14:45:42]

https://codereview.chromium.org/368003002/diff/1/Source/platform/mac/ThemeMac.mm
File Source/platform/mac/ThemeMac.mm (left):

https://codereview.chromium.org/368003002/diff/1/Source/platform/mac/ThemeMac...
Source/platform/mac/ThemeMac.mm:171: #if
BUTTON_CELL_DRAW_WITH_FRAME_DRAWS_FOCUS_RING
Why is it safe to remove this?

[keishi, 2014-07-02 15:54:05]

https://codereview.chromium.org/368003002/diff/1/Source/platform/mac/ThemeMac.mm
File Source/platform/mac/ThemeMac.mm (left):

https://codereview.chromium.org/368003002/diff/1/Source/platform/mac/ThemeMac...
Source/platform/mac/ThemeMac.mm:171: #if
BUTTON_CELL_DRAW_WITH_FRAME_DRAWS_FOCUS_RING
On 2014/07/02 14:45:42, rsesek wrote:
> Why is it safe to remove this?

Hmm.
setShowsFirstResponder draws the focus ring on <=10.7
and drawFocusRingMaskWithFrame exists for >=10.7
so on 10.7 two focus rings might be drawn.

Maybe I need to test if drawFocusRingMaskWithFrame exists here too.

[Jiang Jiang, 2014-07-04 15:39:39]

On 2014/07/02 15:54:05, keishi wrote:
>
https://codereview.chromium.org/368003002/diff/1/Source/platform/mac/ThemeMac.mm
> File Source/platform/mac/ThemeMac.mm (left):
> 
>
https://codereview.chromium.org/368003002/diff/1/Source/platform/mac/ThemeMac...
> Source/platform/mac/ThemeMac.mm:171: #if
> BUTTON_CELL_DRAW_WITH_FRAME_DRAWS_FOCUS_RING
> On 2014/07/02 14:45:42, rsesek wrote:
> > Why is it safe to remove this?
> 
> Hmm.
> setShowsFirstResponder draws the focus ring on <=10.7
> and drawFocusRingMaskWithFrame exists for >=10.7
> so on 10.7 two focus rings might be drawn.
> 
> Maybe I need to test if drawFocusRingMaskWithFrame exists here too.

Yes. What I did in Opera's local patch is to completely remove the
BUTTON_CELL_DRAW_WITH_FRAME_DRAWS_FOCUS_RING compile time macro, and test in
runtime for [cell respondsToSelector:@selector(drawFocusRingWithFrame:inView:)]
instead.

[keishi, 2014-07-07 09:16:07]

https://codereview.chromium.org/368003002/diff/1/Source/platform/mac/ThemeMac.mm
File Source/platform/mac/ThemeMac.mm (left):

https://codereview.chromium.org/368003002/diff/1/Source/platform/mac/ThemeMac...
Source/platform/mac/ThemeMac.mm:171: #if
BUTTON_CELL_DRAW_WITH_FRAME_DRAWS_FOCUS_RING
On 2014/07/02 15:54:05, keishi wrote:
> On 2014/07/02 14:45:42, rsesek wrote:
> > Why is it safe to remove this?
> 
> Hmm.
> setShowsFirstResponder draws the focus ring on <=10.7
> and drawFocusRingMaskWithFrame exists for >=10.7
> so on 10.7 two focus rings might be drawn.
> 
> Maybe I need to test if drawFocusRingMaskWithFrame exists here too.

I confirmed by building using 10.9 SDK and running on 10.7,
setShowsFirstResponder: draws the focus ring and drawFocusRingMaskWithFrame does
nothing. So removing this #if seems to be ok

[keishi, 2014-07-17 08:18:41]

On 2014/07/07 09:16:07, keishi wrote:
>
https://codereview.chromium.org/368003002/diff/1/Source/platform/mac/ThemeMac.mm
> File Source/platform/mac/ThemeMac.mm (left):
> 
>
https://codereview.chromium.org/368003002/diff/1/Source/platform/mac/ThemeMac...
> Source/platform/mac/ThemeMac.mm:171: #if
> BUTTON_CELL_DRAW_WITH_FRAME_DRAWS_FOCUS_RING
> On 2014/07/02 15:54:05, keishi wrote:
> > On 2014/07/02 14:45:42, rsesek wrote:
> > > Why is it safe to remove this?
> > 
> > Hmm.
> > setShowsFirstResponder draws the focus ring on <=10.7
> > and drawFocusRingMaskWithFrame exists for >=10.7
> > so on 10.7 two focus rings might be drawn.
> > 
> > Maybe I need to test if drawFocusRingMaskWithFrame exists here too.
> 
> I confirmed by building using 10.9 SDK and running on 10.7,
> setShowsFirstResponder: draws the focus ring and drawFocusRingMaskWithFrame
does
> nothing. So removing this #if seems to be ok

rsesek@, could you take a look? Thanks.

[Robert Sesek, 2014-07-17 15:12:32]

I apologize. I didn't realize you were waiting on me.

On 2014/07/04 15:39:39, Jiang wrote:
> On 2014/07/02 15:54:05, keishi wrote:
> >
>
https://codereview.chromium.org/368003002/diff/1/Source/platform/mac/ThemeMac.mm
> > File Source/platform/mac/ThemeMac.mm (left):
> > 
> >
>
https://codereview.chromium.org/368003002/diff/1/Source/platform/mac/ThemeMac...
> > Source/platform/mac/ThemeMac.mm:171: #if
> > BUTTON_CELL_DRAW_WITH_FRAME_DRAWS_FOCUS_RING
> > On 2014/07/02 14:45:42, rsesek wrote:
> > > Why is it safe to remove this?
> > 
> > Hmm.
> > setShowsFirstResponder draws the focus ring on <=10.7
> > and drawFocusRingMaskWithFrame exists for >=10.7
> > so on 10.7 two focus rings might be drawn.
> > 
> > Maybe I need to test if drawFocusRingMaskWithFrame exists here too.
> 
> Yes. What I did in Opera's local patch is to completely remove the
> BUTTON_CELL_DRAW_WITH_FRAME_DRAWS_FOCUS_RING compile time macro, and test in
> runtime for [cell
respondsToSelector:@selector(drawFocusRingWithFrame:inView:)]
> instead.

I think we should take this approach globally, rather than just here. Jiang:
maybe you can upload the Opera patch? I find it really hard to reason about the
BUTTON_CELL_DRAW_WITH_FRAME_DRAWS_FOCUS_RING macro, and it seems generally
unnecessary given that runtime checks are available.

https://codereview.chromium.org/368003002/diff/20001/Source/platform/mac/WebC...
File Source/platform/mac/WebCoreNSCellExtras.mm (right):

https://codereview.chromium.org/368003002/diff/20001/Source/platform/mac/WebC...
Source/platform/mac/WebCoreNSCellExtras.mm:33: {return;
Spurious return.

[keishi, 2014-07-18 13:26:55]

Opera's already known to be working patch would be better but I've updated the
patch to remove BUTTON_CELL_DRAW_WITH_FRAME_DRAWS_FOCUS_RING just in case.

https://codereview.chromium.org/368003002/diff/20001/Source/platform/mac/WebC...
File Source/platform/mac/WebCoreNSCellExtras.mm (right):

https://codereview.chromium.org/368003002/diff/20001/Source/platform/mac/WebC...
Source/platform/mac/WebCoreNSCellExtras.mm:33: {return;
On 2014/07/17 15:12:32, rsesek wrote:
> Spurious return.

Done.

[Robert Sesek, 2014-07-21 21:14:38]

https://codereview.chromium.org/368003002/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderThemeChromiumMac.mm (right):

https://codereview.chromium.org/368003002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderThemeChromiumMac.mm:629: if ([cell
respondsToSelector:@selector(drawFocusRingMaskWithFrame:inView:)])
Add a comment as to why you need to do this? You stated the reason in the review
comments, but it won't be obvious to posterity.

https://codereview.chromium.org/368003002/diff/40001/Source/platform/mac/Them...
File Source/platform/mac/ThemeMac.mm (right):

https://codereview.chromium.org/368003002/diff/40001/Source/platform/mac/Them...
Source/platform/mac/ThemeMac.mm:171: if (![cell
respondsToSelector:@selector(drawFocusRingMaskWithFrame:inView:)]) {
… and copy that comment here

https://codereview.chromium.org/368003002/diff/40001/Source/platform/mac/WebC...
File Source/platform/mac/WebCoreNSCellExtras.h (right):

https://codereview.chromium.org/368003002/diff/40001/Source/platform/mac/WebC...
Source/platform/mac/WebCoreNSCellExtras.h:29: // FIXME: Might want to use this
on Mac once we only support OS X 10.8+
I think you've done this.

[tkent, 2015-06-04 09:06:56]

I confirmed this CL resolved crbug.com/425274, Yosemite focus ring issue.

Keishi, can you proceed this?

https://codereview.chromium.org/368003002/diff/60001/Source/platform/mac/WebC...
File Source/platform/mac/WebCoreNSCellExtras.mm (right):

https://codereview.chromium.org/368003002/diff/60001/Source/platform/mac/WebC...
Source/platform/mac/WebCoreNSCellExtras.mm:39: [self
drawFocusRingMaskWithFrame:cellFrame inView:controlView];
Need to use NSInvocation so that this is compilable with 10.6 SDK.
Like:

    SEL sel = @selector(drawFocusRingMaskWithFrame:inView:);
    if (![self respondsToSelector:sel])
        return;
    ...
    NSMethodSignature* signature = [NSCell
instanceMethodSignatureForSelector:sel];
    NSInvocation* invocation = [NSInvocation
invocationWithMethodSignature:signature];
    [invocation setTarget:self];
    [invocation setSelector:sel];
    [invocation setArgument:&cellFrame atIndex:2];
    [invocation setArgument:&controlView atIndex:3];
    [invocation invoke];

[Robert Sesek, 2015-06-04 11:21:04]

https://codereview.chromium.org/368003002/diff/60001/Source/platform/mac/WebC...
File Source/platform/mac/WebCoreNSCellExtras.mm (right):

https://codereview.chromium.org/368003002/diff/60001/Source/platform/mac/WebC...
Source/platform/mac/WebCoreNSCellExtras.mm:39: [self
drawFocusRingMaskWithFrame:cellFrame inView:controlView];
On 2015/06/04 09:06:56, tkent wrote:
> Need to use NSInvocation so that this is compilable with 10.6 SDK.
> Like:
> 
>     SEL sel = @selector(drawFocusRingMaskWithFrame:inView:);
>     if (![self respondsToSelector:sel])
>         return;
>     ...
>     NSMethodSignature* signature = [NSCell
> instanceMethodSignatureForSelector:sel];
>     NSInvocation* invocation = [NSInvocation
> invocationWithMethodSignature:signature];
>     [invocation setTarget:self];
>     [invocation setSelector:sel];
>     [invocation setArgument:&cellFrame atIndex:2];
>     [invocation setArgument:&controlView atIndex:3];
>     [invocation invoke];

No, the respondsToSelector: check is sufficient, if the method is also
forward-declared.

[keishi, 2015-06-08 01:38:13]

https://codereview.chromium.org/368003002/diff/60001/Source/platform/mac/WebC...
File Source/platform/mac/WebCoreNSCellExtras.mm (right):

https://codereview.chromium.org/368003002/diff/60001/Source/platform/mac/WebC...
Source/platform/mac/WebCoreNSCellExtras.mm:39: [self
drawFocusRingMaskWithFrame:cellFrame inView:controlView];
On 2015/06/04 11:21:04, Robert Sesek wrote:
> On 2015/06/04 09:06:56, tkent wrote:
> > Need to use NSInvocation so that this is compilable with 10.6 SDK.
> > Like:
> > 
> >     SEL sel = @selector(drawFocusRingMaskWithFrame:inView:);
> >     if (![self respondsToSelector:sel])
> >         return;
> >     ...
> >     NSMethodSignature* signature = [NSCell
> > instanceMethodSignatureForSelector:sel];
> >     NSInvocation* invocation = [NSInvocation
> > invocationWithMethodSignature:signature];
> >     [invocation setTarget:self];
> >     [invocation setSelector:sel];
> >     [invocation setArgument:&cellFrame atIndex:2];
> >     [invocation setArgument:&controlView atIndex:3];
> >     [invocation invoke];
> 
> No, the respondsToSelector: check is sufficient, if the method is also
> forward-declared.

I'm building on Yosemite with the 10.6 sdk and I get the error below when I use
respondsToSelector: (like this diff). Am I doing something wrong?

ninja: Entering directory `out/Debug'
[1/118] OBJCXX
obj/third_party/WebKit/Source/platform/mac/blink_platform.WebCoreNSCellExtras.o
FAILED: /Users/keishi/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF
obj/third_party/WebKit/Source/platform/mac/blink_platform.WebCoreNSCellExtras.o.d
-DV8_DEPRECATION_WARNINGS -D__ASSERT_MACROS_DEFINE_VERSIONS_WITHOUT_UNDERSCORE=0
-DCHROMIUM_BUILD -DCR_CLANG_REVISION=238013-3 -DCOMPONENT_BUILD
-DTOOLKIT_VIEWS=1 -DUSE_LIBJPEG_TURBO=1 -DENABLE_ONE_CLICK_SIGNIN
-DENABLE_PRE_SYNC_BACKUP -DENABLE_REMOTING=1 -DENABLE_WEBRTC=1
-DENABLE_MEDIA_ROUTER=1 -DENABLE_PEPPER_CDMS -DENABLE_CONFIGURATION_POLICY
-DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1
-DSYSTEM_NATIVELY_SIGNALS_MEMORY_PRESSURE -DDONT_EMBED_BUILD_METADATA
-DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGIN_INSTALLATION=1
-DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1
-DENABLE_AUTOFILL_DIALOG=1 -DENABLE_BACKGROUND=1 -DENABLE_GOOGLE_NOW=1
-DCLD_VERSION=2 -DENABLE_PRINTING=1 -DENABLE_BASIC_PRINTING=1
-DENABLE_PRINT_PREVIEW=1 -DENABLE_SPELLCHECK=1
-DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1
-DENABLE_SUPERVISED_USERS=1 -DENABLE_SERVICE_DISCOVERY=1
-DENABLE_WIFI_BOOTSTRAPPING=1 -DV8_USE_EXTERNAL_STARTUP_DATA
-DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL
-DSAFE_BROWSING_SERVICE -DBLINK_PLATFORM_IMPLEMENTATION=1 -DINSIDE_BLINK
-DENABLE_LAYOUT_UNIT_IN_INLINE_BOXES=0
-DWTF_USE_CONCATENATED_IMPULSE_RESPONSES=1 -DENABLE_INPUT_MULTIPLE_FIELDS_UI=1
-DENABLE_WEB_AUDIO=1 -DWebCascadeList=ChromiumWebCoreObjCWebCascadeList
-DWebScrollAnimationHelperDelegate=ChromiumWebCoreObjCWebScrollAnimationHelperDelegate
-DWebScrollbarPainterControllerDelegate=ChromiumWebCoreObjCWebScrollbarPainterControllerDelegate
-DWebScrollbarPainterDelegate=ChromiumWebCoreObjCWebScrollbarPainterDelegate
-DWebScrollbarPartAnimation=ChromiumWebCoreObjCWebScrollbarPartAnimation
-DWebCoreFlippedView=ChromiumWebCoreObjCWebCoreFlippedView
-DWebCoreTextFieldCell=ChromiumWebCoreObjCWebCoreTextFieldCell -DSKIA_DLL
-DGR_GL_IGNORE_ES3_MSAA=0 -DSK_SUPPORT_GPU=1
-DSK_SUPPORT_LEGACY_OPTIONLESS_GET_PIXELS
-DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS -DU_USING_ICU_NAMESPACE=0
-DU_ENABLE_DYLOAD=0 -DCHROME_PNG_WRITE_SUPPORT -DPNG_USER_CONFIG -DV8_SHARED
-DUSING_V8_SHARED -DUSE_LIBPCI=1 -DUSE_OPENSSL=1 -D__STDC_CONSTANT_MACROS
-D__STDC_FORMAT_MACROS -DDYNAMIC_ANNOTATIONS_ENABLED=1
-DWTF_USE_DYNAMIC_ANNOTATIONS=1 -Igen -I../../third_party/angle/include
-Igen/blink -I../../third_party/WebKit/Source -I../.. -I../../skia/config
-I../../third_party/khronos -I../../gpu -I../../third_party/WebKit
-I../../third_party/skia/include/core -I../../third_party/skia/include/effects
-I../../third_party/skia/include/pdf -I../../third_party/skia/include/gpu
-I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops
-I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports
-I../../third_party/skia/include/utils -I../../skia/ext
-I../../third_party/skia/include/utils/mac -I../../third_party/icu/source/i18n
-I../../third_party/icu/source/common -I../../third_party/libpng
-I../../third_party/zlib -I../../third_party/libwebp
-I../../third_party/ots/include -I../../third_party/qcms/src -I../../v8/include
-I../../third_party/iccjpeg -I../../third_party/libjpeg_turbo
-I../../third_party/harfbuzz-ng/src -isysroot
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.6.sdk
-O0 -gdwarf-2 -fvisibility=hidden -Werror -Wnewline-eof
-mmacosx-version-min=10.6 -arch x86_64 -Wall -Wendif-labels -Wextra
-Wno-unused-parameter -Wno-missing-field-initializers
-Wno-selector-type-mismatch -Wpartial-availability -Wheader-hygiene
-Wno-char-subscripts -Wno-unneeded-internal-declaration
-Wno-covered-switch-default -Wstring-conversion -Wno-c++11-narrowing
-Wno-deprecated-register -Wno-inconsistent-missing-override
-Wglobal-constructors -std=c++11 -fno-rtti -fno-exceptions
-fvisibility-inlines-hidden -fno-threadsafe-statics -Xclang -load -Xclang
/Users/keishi/chromium/src/third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.dylib
-Xclang -add-plugin -Xclang find-bad-constructs -Xclang
-plugin-arg-find-bad-constructs -Xclang check-templates -fcolor-diagnostics
-fno-strict-aliasing -fstack-protector-all -Wno-undefined-bool-conversion
-Wno-tautological-undefined-compare -Wobjc-missing-property-synthesis
-fobjc-call-cxx-cdtors -include
obj/third_party/WebKit/Source/build/mac/blink_platform.Prefix.h-mm -c
../../third_party/WebKit/Source/platform/mac/WebCoreNSCellExtras.mm -o
obj/third_party/WebKit/Source/platform/mac/blink_platform.WebCoreNSCellExtras.o
../../third_party/WebKit/Source/platform/mac/WebCoreNSCellExtras.mm:40:11:
error: instance method '-drawFocusRingMaskWithFrame:inView:' not found (return
type defaults to 'id') [-Werror,-Wobjc-method-access]
    [self drawFocusRingMaskWithFrame:cellFrame inView:controlView];
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSCell.h:165:12:
note: receiver is instance of class declared here
@interface NSCell : NSObject <NSCopying, NSCoding>
           ^
1 error generated.
ninja: build stopped: subcommand failed.

[tkent, 2015-06-08 06:07:41]

https://codereview.chromium.org/368003002/diff/60001/Source/platform/mac/WebC...
File Source/platform/mac/WebCoreNSCellExtras.mm (right):

https://codereview.chromium.org/368003002/diff/60001/Source/platform/mac/WebC...
Source/platform/mac/WebCoreNSCellExtras.mm:39: [self
drawFocusRingMaskWithFrame:cellFrame inView:controlView];
On 2015/06/08 01:38:12, keishi wrote:
> On 2015/06/04 11:21:04, Robert Sesek wrote:
> > On 2015/06/04 09:06:56, tkent wrote:
> > > Need to use NSInvocation so that this is compilable with 10.6 SDK.
> > > Like:
> > > 
> > >     SEL sel = @selector(drawFocusRingMaskWithFrame:inView:);
> > >     if (![self respondsToSelector:sel])
> > >         return;
> > >     ...
> > >     NSMethodSignature* signature = [NSCell
> > > instanceMethodSignatureForSelector:sel];
> > >     NSInvocation* invocation = [NSInvocation
> > > invocationWithMethodSignature:signature];
> > >     [invocation setTarget:self];
> > >     [invocation setSelector:sel];
> > >     [invocation setArgument:&cellFrame atIndex:2];
> > >     [invocation setArgument:&controlView atIndex:3];
> > >     [invocation invoke];
> > 
> > No, the respondsToSelector: check is sufficient, if the method is also
> > forward-declared.
> 
> I'm building on Yosemite with the 10.6 sdk and I get the error below when I
use
> respondsToSelector: (like this diff). Am I doing something wrong?


I think we need to add our own declarations like [1].
[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[keishi, 2015-06-08 10:40:18]

On 2015/06/08 06:07:41, tkent wrote:
>
https://codereview.chromium.org/368003002/diff/60001/Source/platform/mac/WebC...
> File Source/platform/mac/WebCoreNSCellExtras.mm (right):
> 
>
https://codereview.chromium.org/368003002/diff/60001/Source/platform/mac/WebC...
> Source/platform/mac/WebCoreNSCellExtras.mm:39: [self
> drawFocusRingMaskWithFrame:cellFrame inView:controlView];
> On 2015/06/08 01:38:12, keishi wrote:
> > On 2015/06/04 11:21:04, Robert Sesek wrote:
> > > On 2015/06/04 09:06:56, tkent wrote:
> > > > Need to use NSInvocation so that this is compilable with 10.6 SDK.
> > > > Like:
> > > > 
> > > >     SEL sel = @selector(drawFocusRingMaskWithFrame:inView:);
> > > >     if (![self respondsToSelector:sel])
> > > >         return;
> > > >     ...
> > > >     NSMethodSignature* signature = [NSCell
> > > > instanceMethodSignatureForSelector:sel];
> > > >     NSInvocation* invocation = [NSInvocation
> > > > invocationWithMethodSignature:signature];
> > > >     [invocation setTarget:self];
> > > >     [invocation setSelector:sel];
> > > >     [invocation setArgument:&cellFrame atIndex:2];
> > > >     [invocation setArgument:&controlView atIndex:3];
> > > >     [invocation invoke];
> > > 
> > > No, the respondsToSelector: check is sufficient, if the method is also
> > > forward-declared.
> > 
> > I'm building on Yosemite with the 10.6 sdk and I get the error below when I
> use
> > respondsToSelector: (like this diff). Am I doing something wrong?
> 
> 
> I think we need to add our own declarations like [1].
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Oh I see. Done.

[Robert Sesek, 2015-06-08 18:54:28]

Overall LG. Only one more comment, which just occurred to me.

https://codereview.chromium.org/368003002/diff/120001/Source/platform/mac/Web...
File Source/platform/mac/WebCoreNSCellExtras.mm (right):

https://codereview.chromium.org/368003002/diff/120001/Source/platform/mac/Web...
Source/platform/mac/WebCoreNSCellExtras.mm:38: -
(void)_web_drawFocusRingWithFrame:(NSRect)cellFrame inView:(NSView *)controlView
We should rename this. Apple still ship WebCore (which does get transitively
linked into Chromium) with this exact same symbol name -[NSCell
_web_drawFocusRingWithFrame:inView:]. Because ObjC has a flat namespace, this
could conflict with that symbol. I suggest renaming this to:

- (void)cr_drawFocusRingWithFrame:inView:

That way the symbols won't ever collide.

[keishi, 2015-06-09 11:34:17]

On 2015/06/08 18:54:28, Robert Sesek wrote:
> Overall LG. Only one more comment, which just occurred to me.
> 
>
https://codereview.chromium.org/368003002/diff/120001/Source/platform/mac/Web...
> File Source/platform/mac/WebCoreNSCellExtras.mm (right):
> 
>
https://codereview.chromium.org/368003002/diff/120001/Source/platform/mac/Web...
> Source/platform/mac/WebCoreNSCellExtras.mm:38: -
> (void)_web_drawFocusRingWithFrame:(NSRect)cellFrame inView:(NSView
*)controlView
> We should rename this. Apple still ship WebCore (which does get transitively
> linked into Chromium) with this exact same symbol name -[NSCell
> _web_drawFocusRingWithFrame:inView:]. Because ObjC has a flat namespace, this
> could conflict with that symbol. I suggest renaming this to:
> 
> - (void)cr_drawFocusRingWithFrame:inView:
> 
> That way the symbols won't ever collide.

Done.

[Robert Sesek, 2015-06-09 13:14:35]

LGTM

[keishi, 2015-06-11 00:44:36]

Wrapped setting focus on the NSCell with respondsToSelector so we don't draw the
focus ring twice. PTAL

[tkent, 2015-06-11 00:50:24]

lgtm

[keishi, 2015-06-11 00:52:35]

The CQ bit was checked by keishi@chromium.org

[keishi, 2015-06-11 00:52:36]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/368003002/#ps200001
(title: " ")

[commit-bot: I haz the power, 2015-06-11 00:53:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/368003002/200001

[commit-bot: I haz the power, 2015-06-11 01:43:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-11 01:43:10]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)

[keishi, 2015-06-11 04:43:02]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2015-06-11 04:43:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/368003002/200001

[commit-bot: I haz the power, 2015-06-11 05:54:28]

Committed patchset #11 (id:200001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196915

[keishi, 2015-06-15 20:51:05]

A revert of this CL (patchset #11 id:200001) has been created in
https://codereview.chromium.org/1184073004/ by keishi@chromium.org.

The reason for reverting is: Tetsts failed on 10.7 because focus ring was not
drawn..

[keishi, 2015-06-16 07:33:43]

After trying many builds I think this is what's happening:
We only need to support building with 10.6 sdk and 10.9+ sdk. (building with
10.7 sdk results in errors and isn't supported)

The original problem was that building with 10.9 sdk doesn't draw the focus
rings.
The problem was that BUTTON_CELL_DRAW_WITH_FRAME_DRAWS_FOCUS_RING was true even
when building with 10.9 sdk.
MAC_OS_X_VERSION_MIN_REQUIRED is the 10.6 even when building with the 10.9 sdk
(because it is the deployment target).
Changing it to MAC_OS_X_VERSION_MAX_ALLOWED solves this problem.

The new problem is that building with 10.6 sdk and running on 10.10 os doesn't
draw the focus rings.
I think the problem is that [NSCell drawWithFrame:inView:] is broken in AppKit
10.10 and it doesn't draw focus rings.

What this new patch set does is basically, if we build with 10.6 sdk we should
use [NSCell drawWithFrame:inView:] and if we build with 10.9 sdk we should use
[NSCell drawFocusRingMaskWithFrame:inView:].
However if we build with 10.6 sdk and is running on 10.10 we should use [NSCell
drawFocusRingMaskWithFrame:inView:].

[keishi, 2015-06-16 08:44:59]

Removed use of macros.

[tkent, 2015-06-16 08:49:14]

lgtm

[keishi, 2015-06-16 10:12:01]

The CQ bit was checked by keishi@chromium.org

[keishi, 2015-06-16 10:12:02]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/368003002/#ps260001
(title: " ")

[commit-bot: I haz the power, 2015-06-16 10:12:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/368003002/260001

[commit-bot: I haz the power, 2015-06-16 11:59:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-16 11:59:36]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/59060)

[Robert Sesek, 2015-06-16 23:58:33]

On 2015/06/16 07:33:43, keishi wrote:
> The new problem is that building with 10.6 sdk and running on 10.10 os doesn't
> draw the focus rings.
> I think the problem is that [NSCell drawWithFrame:inView:] is broken in AppKit
> 10.10 and it doesn't draw focus rings.

I don't think it's broken. I think the method changed to support a focus ring
animation. See https://code.google.com/p/chromium/issues/detail?id=425274#c12

[keishi, 2015-06-19 03:43:58]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2015-06-19 03:44:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/368003002/260001

[commit-bot: I haz the power, 2015-06-19 05:19:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-19 05:19:33]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/59690)

[keishi, 2015-06-22 14:20:09]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2015-06-22 14:20:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/368003002/260001

[commit-bot: I haz the power, 2015-06-22 15:53:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-22 15:53:49]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/60083)

[Robert Sesek, 2015-06-22 16:02:23]

This isn't ready for commit yet, look at the crahses from the tryjobs:

2015-06-22 08:12:37.792 webkit_unit_tests[15335:507] +[NSCell
cr_drawWithFrameDrawsFocusRing]: unrecognized selector sent to class
0x7fff7f680478
ASSERTION FAILED: Uncaught exception - +[NSCell cr_drawWithFrameDrawsFocusRing]:
unrecognized selector sent to class 0x7fff7f680478
0
../../third_party/WebKit/Source/platform/mac/BlockExceptions.mm(34) : void
ReportBlockedObjCException(NSException *)
1   0x10fc4088b blink::ThemeMac::addVisualOverflow(blink::ControlPart, unsigned
int, float, blink::IntRect&) const
2   0x111451fd0 blink::LayoutBlock::addVisualOverflowFromTheme()
3   0x111451e78 blink::LayoutBlock::computeOverflow(blink::LayoutUnit, bool)
4   0x1114a5d1f blink::LayoutFlexibleBox::layoutBlock(bool)
5   0x1114517b2 blink::LayoutBlock::layout()
6   0x111479596 blink::LayoutBlockFlow::layoutInlineChildren(bool,
blink::LayoutUnit&, blink::LayoutUnit&, blink::LayoutUnit)
7   0x11146f823 blink::LayoutBlockFlow::layoutBlockFlow(bool,
blink::LayoutUnit&, blink::SubtreeLayoutScope&)
8   0x111463290 blink::LayoutBlockFlow::layoutBlock(bool)
9   0x1114517b2 blink::LayoutBlock::layout()
10  0x111463dd7 blink::LayoutBlockFlow::layoutBlockChild(blink::LayoutBox&,
blink::MarginInfo&, blink::LayoutUnit&)
11  0x111467cf1 blink::LayoutBlockFlow::layoutBlockChildren(bool,
blink::SubtreeLayoutScope&, blink::LayoutUnit, blink::LayoutUnit)
12  0x11146f843 blink::LayoutBlockFlow::layoutBlockFlow(bool,
blink::LayoutUnit&, blink::SubtreeLayoutScope&)
13  0x111463290 blink::LayoutBlockFlow::layoutBlock(bool)
14  0x1114517b2 blink::LayoutBlock::layout()
15  0x111463dd7 blink::LayoutBlockFlow::layoutBlockChild(blink::LayoutBox&,
blink::MarginInfo&, blink::LayoutUnit&)
16  0x111467cf1 blink::LayoutBlockFlow::layoutBlockChildren(bool,
blink::SubtreeLayoutScope&, blink::LayoutUnit, blink::LayoutUnit)
17  0x11146f843 blink::LayoutBlockFlow::layoutBlockFlow(bool,
blink::LayoutUnit&, blink::SubtreeLayoutScope&)
18  0x111463290 blink::LayoutBlockFlow::layoutBlock(bool)
19  0x1114517b2 blink::LayoutBlock::layout()
20  0x111541df1 blink::LayoutView::layoutContent()
21  0x1115422b3 blink::LayoutView::layout()
22  0x11121f972 blink::FrameView::performLayout(bool)
23  0x11122084a blink::FrameView::layout()
24  0x110c8ee35 blink::Document::implicitClose()
25  0x111339675 blink::FrameLoader::checkCompleted()
26  0x111339516 blink::FrameLoader::finishedParsing()
27  0x110c9963f blink::Document::finishedParsing()
28  0x110f44b93 blink::HTMLDocumentParser::end()
29  0x110f40ebf blink::HTMLDocumentParser::prepareToStopParsing()
30  0x110f44e64 blink::HTMLDocumentParser::finish()
31  0x110c88801 blink::Document::close(blink::ExceptionState&)

[keishi, 2015-06-23 11:05:22]

On 2015/06/22 16:02:23, Robert Sesek wrote:
> This isn't ready for commit yet, look at the crahses from the tryjobs:
> 
> 2015-06-22 08:12:37.792 webkit_unit_tests[15335:507] +[NSCell
> cr_drawWithFrameDrawsFocusRing]: unrecognized selector sent to class
> 0x7fff7f680478
> ASSERTION FAILED: Uncaught exception - +[NSCell
cr_drawWithFrameDrawsFocusRing]:
> unrecognized selector sent to class 0x7fff7f680478
> 0
> ../../third_party/WebKit/Source/platform/mac/BlockExceptions.mm(34) : void
> ReportBlockedObjCException(NSException *)
> 1   0x10fc4088b blink::ThemeMac::addVisualOverflow(blink::ControlPart,
unsigned
> int, float, blink::IntRect&) const
> 2   0x111451fd0 blink::LayoutBlock::addVisualOverflowFromTheme()
> 3   0x111451e78 blink::LayoutBlock::computeOverflow(blink::LayoutUnit, bool)
> 4   0x1114a5d1f blink::LayoutFlexibleBox::layoutBlock(bool)
> 5   0x1114517b2 blink::LayoutBlock::layout()
> 6   0x111479596 blink::LayoutBlockFlow::layoutInlineChildren(bool,
> blink::LayoutUnit&, blink::LayoutUnit&, blink::LayoutUnit)
> 7   0x11146f823 blink::LayoutBlockFlow::layoutBlockFlow(bool,
> blink::LayoutUnit&, blink::SubtreeLayoutScope&)
> 8   0x111463290 blink::LayoutBlockFlow::layoutBlock(bool)
> 9   0x1114517b2 blink::LayoutBlock::layout()
> 10  0x111463dd7 blink::LayoutBlockFlow::layoutBlockChild(blink::LayoutBox&,
> blink::MarginInfo&, blink::LayoutUnit&)
> 11  0x111467cf1 blink::LayoutBlockFlow::layoutBlockChildren(bool,
> blink::SubtreeLayoutScope&, blink::LayoutUnit, blink::LayoutUnit)
> 12  0x11146f843 blink::LayoutBlockFlow::layoutBlockFlow(bool,
> blink::LayoutUnit&, blink::SubtreeLayoutScope&)
> 13  0x111463290 blink::LayoutBlockFlow::layoutBlock(bool)
> 14  0x1114517b2 blink::LayoutBlock::layout()
> 15  0x111463dd7 blink::LayoutBlockFlow::layoutBlockChild(blink::LayoutBox&,
> blink::MarginInfo&, blink::LayoutUnit&)
> 16  0x111467cf1 blink::LayoutBlockFlow::layoutBlockChildren(bool,
> blink::SubtreeLayoutScope&, blink::LayoutUnit, blink::LayoutUnit)
> 17  0x11146f843 blink::LayoutBlockFlow::layoutBlockFlow(bool,
> blink::LayoutUnit&, blink::SubtreeLayoutScope&)
> 18  0x111463290 blink::LayoutBlockFlow::layoutBlock(bool)
> 19  0x1114517b2 blink::LayoutBlock::layout()
> 20  0x111541df1 blink::LayoutView::layoutContent()
> 21  0x1115422b3 blink::LayoutView::layout()
> 22  0x11121f972 blink::FrameView::performLayout(bool)
> 23  0x11122084a blink::FrameView::layout()
> 24  0x110c8ee35 blink::Document::implicitClose()
> 25  0x111339675 blink::FrameLoader::checkCompleted()
> 26  0x111339516 blink::FrameLoader::finishedParsing()
> 27  0x110c9963f blink::Document::finishedParsing()
> 28  0x110f44b93 blink::HTMLDocumentParser::end()
> 29  0x110f40ebf blink::HTMLDocumentParser::prepareToStopParsing()
> 30  0x110f44e64 blink::HTMLDocumentParser::finish()
> 31  0x110c88801 blink::Document::close(blink::ExceptionState&)

Thanks! I changed drawWithFrameDrawsFocusRing to a C++ method to make it work.
PTAL

[Robert Sesek, 2015-06-23 15:11:01]

LGTM

https://codereview.chromium.org/368003002/diff/280001/Source/platform/mac/Web...
File Source/platform/mac/WebCoreNSCellExtras.h (right):

https://codereview.chromium.org/368003002/diff/280001/Source/platform/mac/Web...
Source/platform/mac/WebCoreNSCellExtras.h:27: #include
"platform/PlatformExport.h"
Unused?

https://codereview.chromium.org/368003002/diff/280001/Source/platform/mac/Web...
Source/platform/mac/WebCoreNSCellExtras.h:29: @interface NSCell
(LionSDKDeclarations)
You can put this in the .mm file instead.

[keishi, 2015-06-24 02:36:00]

https://codereview.chromium.org/368003002/diff/280001/Source/platform/mac/Web...
File Source/platform/mac/WebCoreNSCellExtras.h (right):

https://codereview.chromium.org/368003002/diff/280001/Source/platform/mac/Web...
Source/platform/mac/WebCoreNSCellExtras.h:27: #include
"platform/PlatformExport.h"
On 2015/06/23 15:11:01, Robert Sesek wrote:
> Unused?

Done.

https://codereview.chromium.org/368003002/diff/280001/Source/platform/mac/Web...
Source/platform/mac/WebCoreNSCellExtras.h:29: @interface NSCell
(LionSDKDeclarations)
On 2015/06/23 15:11:01, Robert Sesek wrote:
> You can put this in the .mm file instead.

Done.

[keishi, 2015-06-24 02:36:05]

The CQ bit was checked by keishi@chromium.org

[keishi, 2015-06-24 02:36:05]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/368003002/#ps300001
(title: " ")

[commit-bot: I haz the power, 2015-06-24 02:36:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/368003002/300001

[commit-bot: I haz the power, 2015-06-24 02:47:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-24 02:47:38]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[keishi, 2015-06-24 11:34:20]

The CQ bit was checked by keishi@chromium.org

[keishi, 2015-06-24 11:34:20]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/368003002/#ps320001
(title: " ")

[commit-bot: I haz the power, 2015-06-24 11:34:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/368003002/320001

[commit-bot: I haz the power, 2015-06-24 13:55:13]

Committed patchset #17 (id:320001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197740