Bug 218879 - Add menu support for app highlights for books
Summary: Add menu support for app highlights for books
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Megan Gardner
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-11-12 17:25 PST by Megan Gardner
Modified: 2020-11-16 12:02 PST (History)
4 users (show)

See Also:


Attachments
Patch (29.05 KB, patch)
2020-11-12 18:13 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (28.85 KB, patch)
2020-11-13 09:21 PST, Megan Gardner
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (28.82 KB, patch)
2020-11-13 09:49 PST, Megan Gardner
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (29.93 KB, patch)
2020-11-13 10:54 PST, Megan Gardner
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (28.78 KB, patch)
2020-11-13 11:19 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (26.24 KB, patch)
2020-11-13 17:09 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (26.24 KB, patch)
2020-11-13 17:14 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (26.19 KB, patch)
2020-11-16 09:50 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch for landing (26.20 KB, patch)
2020-11-16 10:16 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch for landing (26.22 KB, patch)
2020-11-16 10:57 PST, Megan Gardner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.