After http://trac.webkit.org/changeset/180465/, we syncrhnously trigger layout upon every selection change.
<rdar://problem/19951047>
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?
(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...
(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?
(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.
(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.
Created attachment 247349 [details] WIP Patch
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
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
I see a 31% progression on Speedometer locally with my WIP patch.
Created attachment 247353 [details] WIP Patch
Created attachment 247357 [details] WIP Patch
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
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
Created attachment 247366 [details] WIP Patch
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
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
Created attachment 247469 [details] Patch3
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.
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.
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)?
(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.
Committed revision 180768.