Bug 218879

Summary: Add menu support for app highlights for books
Product: WebKit Reporter: Megan Gardner <megan_gardner>
Component: New BugsAssignee: Megan Gardner <megan_gardner>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 232642    
Attachments:
Description Flags
Patch
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing
none
Patch for landing none

Description Megan Gardner 2020-11-12 17:25:06 PST
Add menu support for app highlights for books
Comment 1 Megan Gardner 2020-11-12 18:13:49 PST
Created attachment 413988 [details]
Patch
Comment 2 Alex Christensen 2020-11-13 07:43:55 PST
Comment on attachment 413988 [details]
Patch

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

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:35321
> +		446EE4E7255DF00D00454463 /* Concatenate Localizable.strings */ = {

Did you mean to add this?  It doesn't say what this is doing in the ChangeLog.

> Source/WebCore/page/ContextMenuController.cpp:411
> +    case ContextMenuItemTagAddHighlightToCurrentGroup:

#if PLATFORM(COCOA)

> Source/WebCore/page/ContextMenuController.cpp:931
> +                if (Page* page = frame->page()) {

auto*
Comment 3 Megan Gardner 2020-11-13 09:21:07 PST
Created attachment 414051 [details]
Patch
Comment 4 Megan Gardner 2020-11-13 09:49:47 PST
Created attachment 414057 [details]
Patch
Comment 5 Megan Gardner 2020-11-13 10:54:05 PST
Created attachment 414062 [details]
Patch
Comment 6 Megan Gardner 2020-11-13 11:19:04 PST
Created attachment 414069 [details]
Patch
Comment 7 Tim Horton 2020-11-13 15:35:09 PST
Comment on attachment 414069 [details]
Patch

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

> Source/WebKit/FeatureFlags/WebKit.plist:2
>  <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">

If you could avoid sorting and reindenting this, that would be grand for blaming/merging purposes.

> Source/WebKit/Shared/WebPreferencesDefaultValues.cpp:113
> +#if PLATFORM(IOS_FAMILY) || PLATFORM(MAC)

This should be an ENABLE
Comment 8 Tim Horton 2020-11-13 15:36:24 PST
Comment on attachment 414069 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Add menu items and associated plubling for support for books highlights in modern webkit.

plubling
WebKit
Comment 9 Megan Gardner 2020-11-13 17:09:34 PST
Created attachment 414110 [details]
Patch
Comment 10 Megan Gardner 2020-11-13 17:14:46 PST
Created attachment 414111 [details]
Patch
Comment 11 Alex Christensen 2020-11-14 16:02:58 PST
Comment on attachment 414111 [details]
Patch

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

> Source/WTF/Scripts/Preferences/WebPreferencesInternal.yaml:60
> +    WebKitLegacy:

This and WebCore: are probably not necessary, since it's only exposed in WebKit.

> Source/WebCore/page/ContextMenuController.cpp:465
> +        // TODO: Add Highlight Logic

I think WebKit style prefers FIXME over TODO.
Comment 12 Megan Gardner 2020-11-16 09:50:56 PST
Created attachment 414244 [details]
Patch
Comment 13 Megan Gardner 2020-11-16 10:16:47 PST
Created attachment 414248 [details]
Patch for landing
Comment 14 Megan Gardner 2020-11-16 10:57:03 PST
Created attachment 414257 [details]
Patch for landing
Comment 15 EWS 2020-11-16 11:33:12 PST
Committed r269865: <https://trac.webkit.org/changeset/269865>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 414257 [details].
Comment 16 Radar WebKit Bug Importer 2020-11-16 11:34:18 PST
<rdar://problem/71450870>
Comment 17 Megan Gardner 2020-11-16 11:47:31 PST
<rdar://problem/71352113>
Comment 18 Simon Fraser (smfr) 2020-11-16 12:02:49 PST
Comment on attachment 414257 [details]
Patch for landing

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

> Source/WTF/ChangeLog:3
> +        Add menu support for app highlights for books

menu -> context menu, but too late now.