Bug 218879 – Add menu support for app highlights for books

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Add menu support for app highlights for books

[Megan Gardner]

Created attachment 413988 [details]
Patch

[Alex Christensen]

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*

[Megan Gardner]

Created attachment 414051 [details]
Patch

[Megan Gardner]

Created attachment 414057 [details]
Patch

[Megan Gardner]

Created attachment 414062 [details]
Patch

[Megan Gardner]

Created attachment 414069 [details]
Patch

[Tim Horton]

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

[Tim Horton]

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

[Megan Gardner]

Created attachment 414110 [details]
Patch

[Megan Gardner]

Created attachment 414111 [details]
Patch

[Alex Christensen]

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.

[Megan Gardner]

Created attachment 414244 [details]
Patch

[Megan Gardner]

Created attachment 414248 [details]
Patch for landing

[Megan Gardner]

Created attachment 414257 [details]
Patch for landing

[EWS]

Committed r269865: <https://trac.webkit.org/changeset/269865>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 414257 [details].

[Radar WebKit Bug Importer]

<rdar://problem/71450870>

[Megan Gardner]

<rdar://problem/71352113>

[Simon Fraser (smfr)]

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.