[WK2] REGRESSION(r180465): WebKit::WebPage::editorState() triggers a layout

• Component: WebKit2
• Status: RESOLVED FIXED

[Ryosuke Niwa]

After http://trac.webkit.org/changeset/180465/, we syncrhnously trigger layout upon every selection change.

[Radar WebKit Bug Importer]

<rdar://problem/19951047>

[Sam Weinig]

From the patch (and I might be reading it wrong), it looks like we had this behavior already on iOS, so it is regression only on Mac. Is that correct?

[Ryosuke Niwa]

(In reply to comment #2)
> From the patch (and I might be reading it wrong), it looks like we had this
> behavior already on iOS, so it is regression only on Mac. Is that correct?

Yes but that code was added in http://trac.webkit.org/changeset/171015 so this could still be a regression since our last release...

[Enrica Casucci]

(In reply to comment #3)
> (In reply to comment #2)
> > From the patch (and I might be reading it wrong), it looks like we had this
> > behavior already on iOS, so it is regression only on Mac. Is that correct?
> 
> Yes but that code was added in http://trac.webkit.org/changeset/171015 so
> this could still be a regression since our last release...

How can we return this type of information without layout?

[Sam Weinig]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > From the patch (and I might be reading it wrong), it looks like we had this
> > > behavior already on iOS, so it is regression only on Mac. Is that correct?
> > 
> > Yes but that code was added in http://trac.webkit.org/changeset/171015 so
> > this could still be a regression since our last release...
> 
> How can we return this type of information without layout?

I am not sure that is necessarily the right question. We might want to look at this a bit more removed, and consider if we really need to grab this information everytime the selection changes, or whether we can wait until the next style recalc/layout happens to tell the UIProcess that the editor state has changed.

[Ryosuke Niwa]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > From the patch (and I might be reading it wrong), it looks like we had this
> > > behavior already on iOS, so it is regression only on Mac. Is that correct?
> > 
> > Yes but that code was added in http://trac.webkit.org/changeset/171015 so
> > this could still be a regression since our last release...
> 
> How can we return this type of information without layout?

I don't think we can.  I think we should instead delay when the font information is fetched. Right now, we seem to be updating it each time selection changes within a single task (e.g. when JS modifies it multiple times) but we shouldn't have to do that because font panel, keyboard, etc... won't be interacting with users faster than 60fps.

[Chris Dumez]

Created attachment 247349 [details]
WIP Patch

[Build Bot]

Comment on attachment 247349 [details]
WIP Patch

Attachment 247349 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5953856059998208

New failing tests:
platform/mac/editing/input/selection-change-closes-typing-2.html
platform/mac/editing/input/selection-change-closes-typing.html
editing/secure-input/password-input-focusing.html

[Build Bot]

Created attachment 247351 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Chris Dumez]

I see a 31% progression on Speedometer locally with my WIP patch.

[Chris Dumez]

Created attachment 247353 [details]
WIP Patch

[Chris Dumez]

Created attachment 247357 [details]
WIP Patch

[Build Bot]

Comment on attachment 247357 [details]
WIP Patch

Attachment 247357 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5103672115593216

New failing tests:
editing/secure-input/password-input-focusing.html

[Build Bot]

Created attachment 247362 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Chris Dumez]

Created attachment 247366 [details]
WIP Patch

[Build Bot]

Comment on attachment 247366 [details]
WIP Patch

Attachment 247366 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6710393611223040

New failing tests:
platform/mac/editing/input/selection-change-closes-typing-2.html
platform/mac/editing/input/selection-change-closes-typing.html
editing/secure-input/password-input-focusing.html

[Build Bot]

Created attachment 247375 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Enrica Casucci]

Created attachment 247469 [details]
Patch3

[WebKit Commit Bot]

Attachment 247469 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:333:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebPageProxy.h:543:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/API/mac/WKView.mm:2522:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/mac/WKView.mm:2523:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/mac/WKView.mm:2524:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 247469 [details]
Patch3

Looks good and I have confirmed that it fixes the perf regression locally. I'll let a WK2 Owner review this though.

[Alexey Proskuryakov]

Comment on attachment 247469 [details]
Patch3

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

> Source/WebKit2/UIProcess/API/mac/WKView.mm:748
> +        _data->_page->fontAtSelection([self](const String& fontName, double fontSize, bool selectionHasMultipleFonts, CallbackBase::Error error) {

Do we actually need to capture self here?

> Source/WebKit2/UIProcess/API/mac/WKView.mm:750
> +            NSFont *font = [NSFont fontWithName:fontName size:fontSize];
> +            [[NSFontManager sharedFontManager] setSelectedFont:font isMultiple:selectionHasMultipleFonts];

Should we bail out if there was no font found (e.g. an SVG font or another loadable font is in use)?

[Enrica Casucci]

(In reply to comment #21)
> Comment on attachment 247469 [details]
> Patch3
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247469&action=review
> 
> > Source/WebKit2/UIProcess/API/mac/WKView.mm:748
> > +        _data->_page->fontAtSelection([self](const String& fontName, double fontSize, bool selectionHasMultipleFonts, CallbackBase::Error error) {
> 
> Do we actually need to capture self here?
No, I'll remove it.
> 
> > Source/WebKit2/UIProcess/API/mac/WKView.mm:750
> > +            NSFont *font = [NSFont fontWithName:fontName size:fontSize];
> > +            [[NSFontManager sharedFontManager] setSelectedFont:font isMultiple:selectionHasMultipleFonts];
> 
> Should we bail out if there was no font found (e.g. an SVG font or another
> loadable font is in use)?
Yes, there is no point in calling the font manager.
thanks for the review.

[Enrica Casucci]

Committed revision 180768.