Action menu URL preview should "peek," i.e. appear when the menu item is highlighted

• Component: WebKit2
• Status: RESOLVED FIXED

[Beth Dakin]

Action menu URL preview should "peek," i.e. appear when the menu item is highlighted

rdar://problem/18774264

[Beth Dakin]

Created attachment 241046 [details]
Patch

[Conrad Shultz]

Comment on attachment 241046 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241046&action=review

> Source/WebKit2/UIProcess/mac/WKActionMenuController.h:59
> +    RetainPtr<NSPopover> _previewPopover;

I wonder if we should close the popover and nil out its delegate when the action menu controller is deallocated just to be on the safe side.

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:180
> +    if (_previewPopover && !_shouldKeepPreviewPopoverOpen) {

The _previewPopover check is unnecessary.

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:182
> +        _previewPopover = nil;

It would probably be a good idea to nil out the popover's delegate as part of tear-down.

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:258
> +- (void)_keepPreviewOpen:(id)sender

Should this be -_keepPreviewOpenFromActionMenu: for consistency?

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:267
> +        return;

Is there any way for this to be received with a different URL from the one being shown? If there is, I'm not sure this will do the right thing.

[Beth Dakin]

(In reply to comment #2)
> Comment on attachment 241046 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=241046&action=review
> 
> > Source/WebKit2/UIProcess/mac/WKActionMenuController.h:59
> > +    RetainPtr<NSPopover> _previewPopover;
> 
> I wonder if we should close the popover and nil out its delegate when the
> action menu controller is deallocated just to be on the safe side.
> 

Sounds like a good idea. I'll put it in willDestroyView for now, but maybe we should have a dealloc too?

> > Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:180
> > +    if (_previewPopover && !_shouldKeepPreviewPopoverOpen) {
> 
> The _previewPopover check is unnecessary.
> 

Removed.

> > Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:182
> > +        _previewPopover = nil;
> 
> It would probably be a good idea to nil out the popover's delegate as part
> of tear-down.
> 

Will do.

> > Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:258
> > +- (void)_keepPreviewOpen:(id)sender
> 
> Should this be -_keepPreviewOpenFromActionMenu: for consistency?
> 

It should. Fixed.

> > Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:267
> > +        return;
> 
> Is there any way for this to be received with a different URL from the one
> being shown? If there is, I'm not sure this will do the right thing.

Good point. This should not be possible… I'll give some thought to whether it deserves some sort of belt-and-suspenders.

[Beth Dakin]

http://trac.webkit.org/changeset/175632

Thanks!