WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
83047
Mark text with text alternative with blue underline.
https://bugs.webkit.org/show_bug.cgi?id=83047
Summary
Mark text with text alternative with blue underline.
Jia Pu
Reported
2012-04-03 10:57:33 PDT
This is part of the changes for
bug 82503
. <
rdar://problem/11175647
>
Attachments
Patch (v1)
(95.62 KB, patch)
2012-04-16 17:13 PDT
,
Jia Pu
no flags
Details
Formatted Diff
Diff
Patch (v2)
(90.70 KB, patch)
2012-04-16 19:44 PDT
,
Jia Pu
no flags
Details
Formatted Diff
Diff
Patch (v3)
(90.70 KB, patch)
2012-04-16 20:48 PDT
,
Jia Pu
no flags
Details
Formatted Diff
Diff
Moved code shared by WebKit and WebKit2 into WebCore.
(75.97 KB, patch)
2012-04-18 19:38 PDT
,
Jia Pu
no flags
Details
Formatted Diff
Diff
Minor Adjustment
(76.13 KB, patch)
2012-04-18 20:04 PDT
,
Jia Pu
no flags
Details
Formatted Diff
Diff
Fix build.
(78.00 KB, patch)
2012-04-18 21:28 PDT
,
Jia Pu
no flags
Details
Formatted Diff
Diff
Patch (v1).
(128.25 KB, patch)
2012-06-11 17:42 PDT
,
Jia Pu
no flags
Details
Formatted Diff
Diff
Patch (v2).
(128.18 KB, patch)
2012-06-11 19:37 PDT
,
Jia Pu
no flags
Details
Formatted Diff
Diff
Fixing Lion build.
(128.26 KB, patch)
2012-06-13 00:27 PDT
,
Jia Pu
no flags
Details
Formatted Diff
Diff
Fixing Lion build.
(129.31 KB, patch)
2012-06-13 10:09 PDT
,
Jia Pu
no flags
Details
Formatted Diff
Diff
Remove block code to make gcc happy.
(128.99 KB, patch)
2012-06-13 18:44 PDT
,
Jia Pu
no flags
Details
Formatted Diff
Diff
Update to TOT.
(128.95 KB, patch)
2012-06-13 21:00 PDT
,
Jia Pu
enrica
: review+
Details
Formatted Diff
Diff
Show Obsolete
(11)
View All
Add attachment
proposed patch, testcase, etc.
Jia Pu
Comment 1
2012-04-16 17:13:18 PDT
Created
attachment 137438
[details]
Patch (v1)
Build Bot
Comment 2
2012-04-16 17:33:07 PDT
Comment on
attachment 137438
[details]
Patch (v1)
Attachment 137438
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/12415473
Early Warning System Bot
Comment 3
2012-04-16 17:45:50 PDT
Comment on
attachment 137438
[details]
Patch (v1)
Attachment 137438
[details]
did not pass qt-wk2-ews (qt): Output:
http://queues.webkit.org/results/12412656
Build Bot
Comment 4
2012-04-16 17:47:04 PDT
Comment on
attachment 137438
[details]
Patch (v1)
Attachment 137438
[details]
did not pass win-ews (win): Output:
http://queues.webkit.org/results/12410953
Philippe Normand
Comment 5
2012-04-16 18:00:12 PDT
Comment on
attachment 137438
[details]
Patch (v1)
Attachment 137438
[details]
did not pass gtk-ews (gtk): Output:
http://queues.webkit.org/results/12412668
Jia Pu
Comment 6
2012-04-16 19:44:44 PDT
Created
attachment 137464
[details]
Patch (v2)
Jia Pu
Comment 7
2012-04-16 20:48:55 PDT
Created
attachment 137472
[details]
Patch (v3)
Enrica Casucci
Comment 8
2012-04-18 11:50:49 PDT
Comment on
attachment 137472
[details]
Patch (v3) View in context:
https://bugs.webkit.org/attachment.cgi?id=137472&action=review
The patch seems correct. What I don't like is all the duplicated code between WebKit1 and WebKit2. You should move all the common code down to WebCore, this is the recommended approach and your patch will be a lot smaller too. R- because of all the duplicated code.
> Source/WebCore/ChangeLog:3 > + On OS X, Mark text with dictation alternative with blue underline.
lowercase m.
> Source/WebCore/ChangeLog:18 > + (WebCore):
Remove this line.
> Source/WebCore/ChangeLog:20 > + (AlternativeTextController):
ditto.
> Source/WebCore/ChangeLog:24 > + (AlternativeTextClient):
ditto,
> Source/WebKit/ChangeLog:3 > + On OS X, Mark text with dictation alternative with blue underline.
lower case m.
> Source/WebKit/mac/ChangeLog:3 > + On OS X, Mark text with dictation alternative with blue underline.
ditto.
> Source/WebKit2/ChangeLog:3 > + On OS X, Mark text with dictation alternative with blue underline.
lower case.
> Source/WebKit2/ChangeLog:15 > + (CoreIPC::::decode):
Remove 3 lines above.
> Source/WebKit2/ChangeLog:23 > + (WebKit):
Remove.
> Source/WebKit2/ChangeLog:33 > + (PageClient):
remove.
> Source/WebKit2/ChangeLog:35 > + (WebKit):
ditto.
> Source/WebKit2/ChangeLog:38 > + (WebPageProxy):
ditto.
> Source/WebKit2/ChangeLog:43 > + (WebKit):
ditto.
> Source/WebKit2/ChangeLog:49 > + (WebKit):
ditto.
> Source/WebKit2/ChangeLog:66 > + (WebKit):
ditto
> Source/WebKit2/ChangeLog:72 > + (WebKit):
ditto.
> Source/WebKit2/ChangeLog:74 > + (WebPage):
ditto.
> Source/WebKit2/ChangeLog:78 > + (WebKit):
ditto.
> Source/WebCore/editing/AlternativeTextController.cpp:629 > +#if USE(DICTATION_ALTERNATIVES)
Are you sure marker is always valid?
> Source/WebKit/mac/WebCoreSupport/WebAlternativeTextClient.mm:78 > + [m_webView _removeDictationAlternatives:dictationContext];
probably need an #else with UNUSED_PARAM(dictationContext)
> Source/WebKit2/UIProcess/API/mac/WKView.mm:1174 > +#endif
It would have been nice to share this code between WebKit1 and WebKit2. Could you move this down to WebCore and export it?
Jia Pu
Comment 9
2012-04-18 19:38:55 PDT
Created
attachment 137822
[details]
Moved code shared by WebKit and WebKit2 into WebCore.
Jia Pu
Comment 10
2012-04-18 19:40:30 PDT
(In reply to
comment #9
)
> Created an attachment (id=137822) [details] > Moved code shared by WebKit and WebKit2 into WebCore.
This patch avoids having duplicated DictationAlternativeController and TextAlternativeWithRange classes in WebKit and WebKit2.
Jia Pu
Comment 11
2012-04-18 20:04:13 PDT
Created
attachment 137825
[details]
Minor Adjustment
Build Bot
Comment 12
2012-04-18 20:13:17 PDT
Comment on
attachment 137825
[details]
Minor Adjustment
Attachment 137825
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/12297189
Jia Pu
Comment 13
2012-04-18 21:28:50 PDT
Created
attachment 137834
[details]
Fix build.
Jia Pu
Comment 14
2012-06-11 17:42:54 PDT
Created
attachment 146978
[details]
Patch (v1).
Jia Pu
Comment 15
2012-06-11 19:37:52 PDT
Created
attachment 146995
[details]
Patch (v2).
Build Bot
Comment 16
2012-06-11 20:03:49 PDT
Comment on
attachment 146995
[details]
Patch (v2).
Attachment 146995
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/12944397
Jia Pu
Comment 17
2012-06-13 00:27:51 PDT
Created
attachment 147251
[details]
Fixing Lion build.
Build Bot
Comment 18
2012-06-13 00:50:27 PDT
Comment on
attachment 147251
[details]
Fixing Lion build.
Attachment 147251
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/12943819
Jia Pu
Comment 19
2012-06-13 10:09:12 PDT
Created
attachment 147351
[details]
Fixing Lion build.
Build Bot
Comment 20
2012-06-13 10:33:38 PDT
Comment on
attachment 147351
[details]
Fixing Lion build.
Attachment 147351
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/12950404
Jia Pu
Comment 21
2012-06-13 18:44:25 PDT
Created
attachment 147463
[details]
Remove block code to make gcc happy.
Jia Pu
Comment 22
2012-06-13 21:00:27 PDT
Created
attachment 147479
[details]
Update to TOT.
Enrica Casucci
Comment 23
2012-06-14 12:13:35 PDT
Comment on
attachment 147479
[details]
Update to TOT. View in context:
https://bugs.webkit.org/attachment.cgi?id=147479&action=review
The patch looks good. I've added a couple of comments that I would like to see fixed before you land it. I'll trust you to fix them before landing.
> Source/WebCore/ChangeLog:11 > + This patch implements visual indication on dictated text with alternatives, and providing UI
providing-->provides
> Source/WebCore/editing/mac/AlternativeTextUIController.mm:95 > + NSSpellChecker* spellChecker = [NSSpellChecker sharedSpellChecker];
no need to use the local variable, use directly [NSSpellChecker sharedSpellChecker] on the line below.
Jia Pu
Comment 24
2012-06-14 13:25:06 PDT
Landed manually:
http://trac.webkit.org/changeset/120357
Alexey Proskuryakov
Comment 25
2012-06-15 12:13:09 PDT
Comment on
attachment 147479
[details]
Update to TOT. View in context:
https://bugs.webkit.org/attachment.cgi?id=147479&action=review
> Source/WebCore/editing/FrameSelection.h:121 > ClearTypingStyle = 1 << 2, > SpellCorrectionTriggered = 1 << 3, > DoNotSetFocus = 1 << 4, > + DictationTriggered = 1 << 5
I wish that we didn't have a mix of hight level and low level options here. It's not FrameSelection's job to make policy decisions on why to do when selection change is triggered by spelling or by dictation.
> Source/WebCore/page/ContextMenuController.cpp:903 > + bool haveContextMenuItemsForMisspellingOrGrammer = false;
"Grammer"?
> Source/WebKit2/UIProcess/WebPageProxy.messages.in:288 > + DictationAlternatives(uint64_t dictationContext) -> (Vector<String> alternatives)
It's a shame that we need a synchronous call from WebProcess to UIProcess to get data that's only really used in UIProcess to display a context menu.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug