Bug 138534 - Implement action menu support for videos
Summary: Implement action menu support for videos
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-07 22:01 PST by Beth Dakin
Modified: 2014-11-08 11:08 PST (History)
6 users (show)

See Also:


Attachments
Patch (14.37 KB, patch)
2014-11-07 22:14 PST, Beth Dakin
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2014-11-07 22:01:42 PST
Implement action menu support for videos

rdar://problem/18742164
Comment 1 Beth Dakin 2014-11-07 22:14:54 PST
Created attachment 241226 [details]
Patch
Comment 2 WebKit Commit Bot 2014-11-07 23:29:59 PST
Attachment 241226 [details] did not pass style-queue:


ERROR: Source/WebCore/WebCore.exp.in:0:  Source/WebCore/WebCore.exp.in should be sorted, use Tools/Scripts/sort-export-file script  [list/order] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Tim Horton 2014-11-07 23:33:14 PST
Comment on attachment 241226 [details]
Patch

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

> Source/WebCore/rendering/HitTestResult.cpp:508
> +bool HitTestResult::isMediaThatCanBeDownloaded() const

isDownloadableMedia?

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:401
> +    RefPtr<WebHitTestResult> hitTestResult = WebHitTestResult::create(_hitTestResult.hitTestResult);

Shouldn't we just use hitTestResultForStage?
Comment 4 Beth Dakin 2014-11-08 10:40:55 PST
(In reply to comment #3)
> Comment on attachment 241226 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=241226&action=review
> 
> > Source/WebCore/rendering/HitTestResult.cpp:508
> > +bool HitTestResult::isMediaThatCanBeDownloaded() const
> 
> isDownloadableMedia?
> 

I like that better! Will use.

> > Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:401
> > +    RefPtr<WebHitTestResult> hitTestResult = WebHitTestResult::create(_hitTestResult.hitTestResult);
> 
> Shouldn't we just use hitTestResultForStage?

UGH, I hate that enum. We should use that, except we don't know the stage here unless we pass it in, and doing that would be ridiculous. Because really, you just want the hitTestResultForStage method NOT to take an enum, and to return whichever hitTestResult it can access, favoring the new one. We have a lot of spots in this file that still choose the wrong hit test result (like all of the URL methods which all favor the old one!), I think I will fix this everywhere in a follow-up patch.
Comment 5 Beth Dakin 2014-11-08 11:08:27 PST
Thanks! http://trac.webkit.org/changeset/175779