Bug 138432 - Action menu URL preview should "peek," i.e. appear when the menu item is highlighted
Summary: Action menu URL preview should "peek," i.e. appear when the menu item is high...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-11-05 11:48 PST by Beth Dakin
Modified: 2014-11-05 12:17 PST (History)
4 users (show)

See Also:

Attachments
Patch (7.15 KB, patch)
2014-11-05 11:53 PST, Beth Dakin 		andersca: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2014-11-05 11:48:07 PST 
Action menu URL preview should "peek," i.e. appear when the menu item is highlighted

rdar://problem/18774264
Comment 1 Beth Dakin 2014-11-05 11:53:50 PST 
Created attachment 241046 [details]
Patch
Comment 2 Conrad Shultz 2014-11-05 12:05:05 PST 
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.
Comment 3 Beth Dakin 2014-11-05 12:11:59 PST 
(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.
Comment 4 Beth Dakin 2014-11-05 12:17:24 PST 
http://trac.webkit.org/changeset/175632

Thanks!