Bug 129024

Summary: Changing selection shouldn't synchronously update editor UI components
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, benjamin, darin, enrica, g.czajkowski, ggaren
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 127832    
Attachments:
Description Flags
Fixes the bug
none
Fixes Windows build bfulgham: review+

Description Ryosuke Niwa 2014-02-18 20:57:46 PST
Right now, editor UI controller such as spellchecker, alternative text controller, and delete button controller
are all updated synchronously upon selection change.

We shouldn't do that at least for programatic selection changes.
Comment 1 Ryosuke Niwa 2014-02-18 21:50:56 PST
Created attachment 224592 [details]
Fixes the bug
Comment 2 Ryosuke Niwa 2014-02-18 22:37:00 PST
Created attachment 224595 [details]
Fixes Windows build
Comment 3 Brent Fulgham 2014-02-19 11:18:09 PST
Comment on attachment 224595 [details]
Fixes Windows build

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

A couple of minor nits to consider, but r=me.

> Source/WebCore/testing/Internals.cpp:1318
> +    // FIXME: We shouldn't pollute the id namespace with this name.

Should we file a bug to track this FIXME?

> LayoutTests/platform/mac/editing/deleting/id-in-deletebutton.html:14
> +    shouldBeNonNull('internals.findEditingDeleteButton(); document.getElementById("WebKit-Editing-Delete-Button")');

I don't feel like this is clearer than the original code.
Comment 4 Ryosuke Niwa 2014-02-19 16:10:01 PST
Committed r164401: <http://trac.webkit.org/changeset/164401>
Comment 5 Ryosuke Niwa 2014-02-19 16:12:12 PST
(In reply to comment #3)
> (From update of attachment 224595 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=224595&action=review
> 
> A couple of minor nits to consider, but r=me.
> 
> > Source/WebCore/testing/Internals.cpp:1318
> > +    // FIXME: We shouldn't pollute the id namespace with this name.
> 
> Should we file a bug to track this FIXME?

Filed https://bugs.webkit.org/show_bug.cgi?id=129072.

> 
> > LayoutTests/platform/mac/editing/deleting/id-in-deletebutton.html:14
> > +    shouldBeNonNull('internals.findEditingDeleteButton(); document.getElementById("WebKit-Editing-Delete-Button")');
> 
> I don't feel like this is clearer than the original code.

This is to make expected result more self explanatory.  The old expected result didn't tell us how we were finding the delete button controller at all.
Comment 6 Ryosuke Niwa 2014-02-19 17:12:03 PST
Removed a bogus assertion in http://trac.webkit.org/changeset/164404.
Comment 7 Ryosuke Niwa 2014-02-21 00:10:20 PST
Looks like this had ~1.5% performance impact on DoYouEvenBench:
https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-mavericks%22%2C%22DoYouEvenBench%2FFull%3ATime%3ATotal%22%5D%5D