Bug 129024 - Changing selection shouldn't synchronously update editor UI components
Summary: Changing selection shouldn't synchronously update editor UI components
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 127832
  Show dependency tree / graph
 
Reported: 2014-02-18 20:57 PST by Ryosuke Niwa
Modified: 2014-02-21 00:10 PST (History)
6 users (show)

See Also:

Attachments
Fixes the bug (27.73 KB, patch)
2014-02-18 21:50 PST, Ryosuke Niwa 		no flags Details | Formatted Diff | Diff
Fixes Windows build (30.71 KB, patch)
2014-02-18 22:37 PST, Ryosuke Niwa 		bfulgham: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

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