Bug 127986

Summary: Remove inline member functions of FrameSelection that access m_selection
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, benjamin, darin, enrica, kling, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 127832    
Attachments:
Description Flags
Cleanup
none
Fixes GTK+ and EFL builds
none
Another build fix
none
One more build fix for EFL & GTK+
none
Fix EFL build enrica: review+

Description Ryosuke Niwa 2014-01-30 23:24:30 PST
In order to resolve the bug 127832, we need all call sites of FrameSelection's member functions
that depend on m_selection to decide whether they want the invalidated selection that's exposed to JS
or that the validated selection the editing & rendering code will use.

In order to simplify this transition, all such access to m_selection should be done though FrameSelection::selection().
Comment 1 Ryosuke Niwa 2014-01-30 23:48:26 PST
Created attachment 222798 [details]
Cleanup
Comment 2 Ryosuke Niwa 2014-01-31 00:22:48 PST
Created attachment 222802 [details]
Fixes GTK+ and EFL builds
Comment 3 Ryosuke Niwa 2014-01-31 00:39:15 PST
Created attachment 222805 [details]
Another build fix
Comment 4 Sergio Villar Senin 2014-01-31 03:50:08 PST
Comment on attachment 222805 [details]
Another build fix

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

Great to see how the FrameSelection interface gets smaller. Patch looks awesome to me. 

Just a minor nit, DumpRenderTreeSupportGtk also accesses rootEditableElement() so you'd have to add a similar fix to the one you did for EFL.

> Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:219
> +    const VisibleSelection& selection = frame.selection();

This should be frame.selection().selection() right?
Comment 5 Ryosuke Niwa 2014-01-31 14:39:55 PST
Created attachment 222855 [details]
One more build fix for EFL & GTK+
Comment 6 Ryosuke Niwa 2014-01-31 15:45:00 PST
Created attachment 222861 [details]
Fix EFL build
Comment 7 Enrica Casucci 2014-01-31 16:37:06 PST
Comment on attachment 222861 [details]
Fix EFL build

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

Looks good to me. Please fix FrameSelection.h before landing otherwise it will break iOS builds.

> Source/WebCore/editing/FrameSelection.h:343
> +    bool m_updateAppearanceEnabled : 1;DragController.cpp

This must be a typo. I'm sure it doesn't build like this for iOS :-)

> Source/WebCore/page/DragController.cpp:816
> +        const VisibleSelection& srcSelection = src.selection().selection();

I would call it sourceSelection.
Comment 8 Ryosuke Niwa 2014-01-31 20:06:05 PST
Committed r163232: <http://trac.webkit.org/changeset/163232>
Comment 9 Ryosuke Niwa 2014-01-31 20:53:45 PST
Debug tests fix landed in http://trac.webkit.org/changeset/163233.