Bug 39854 - Refactor platform dependent editing behavior code out of Settings
Summary: Refactor platform dependent editing behavior code out of Settings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Antonio Gomes
URL:
Keywords:
Depends on:
Blocks: 36627 41975 41976 41977 41989
  Show dependency treegraph
 
Reported: 2010-05-27 14:19 PDT by Antonio Gomes
Modified: 2010-07-09 14:39 PDT (History)
14 users (show)

See Also:


Attachments
wip - first materialization of the idea (14.09 KB, patch)
2010-05-27 14:39 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
wip - second prototype of the idea (6.30 KB, patch)
2010-05-28 08:38 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
3rd simple design - feedback? (21.22 KB, patch)
2010-05-30 20:56 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
3rd simple design (right file) - feedback? (13.19 KB, patch)
2010-05-30 21:02 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
4th try - feedback? (24.85 KB, patch)
2010-05-31 08:17 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
patch v1 - functional (36.09 KB, patch)
2010-05-31 14:33 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
patch v2 (36.03 KB, patch)
2010-05-31 17:49 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
patch v2.1 - same as v2, but builds on Mac (39.99 KB, patch)
2010-06-01 07:04 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
patch v2.2 - same as v2, but really builds on Mac (40.07 KB, patch)
2010-06-01 08:56 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
patch v2.3 - part I (37.26 KB, patch)
2010-06-04 14:37 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
patch v2.3 - part I (right file) (36.86 KB, patch)
2010-06-04 14:44 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
patch v2.3 - part I (agaisnt ToT) (36.86 KB, patch)
2010-06-07 12:14 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
(committed with r60841, reviewed by Ojan and Darin) patch v2.3 - part I (agaisnt ToT and builds) (39.51 KB, patch)
2010-06-07 13:55 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
patch v3 - part II (4.79 KB, patch)
2010-06-28 05:35 PDT, Antonio Gomes
tonikitoo: commit-queue-
Details | Formatted Diff | Diff
patch v3.1 - part II (4.73 KB, patch)
2010-06-30 20:01 PDT, Antonio Gomes
ojan: review-
Details | Formatted Diff | Diff
(r62815,r=ojan) patch v3.2 - part II (3.86 KB, patch)
2010-07-08 11:54 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antonio Gomes 2010-05-27 14:19:59 PDT
... as discussed suggested by Darin and Ojan in bug 36627

"Once we have a third behavior or fourth, I think we need some functions that translate the overall behavior policy into specific behaviors rather than directly saying "behavior == Mac" or "behavior == Windows". Helper functions that expression the different rules in plain language.

I think that should be straightforward and we can do that refactoring before adding a new behavior value."


+++ This bug was initially created as a clone of Bug #36627 +++
Comment 1 Antonio Gomes 2010-05-27 14:39:56 PDT
Created attachment 57277 [details]
wip - first materialization of the idea

A quick prototype of the idea we discussed.

Basically, it adds a EditorSettings class, also owned by Page. The idea is to gradually move all EditingBehavior stuff in Settings to this new EditorSettings class, and change callsites of Settings::editorBehavior() to use the methods we add into this class.

I've showed how it is going to happen in the RenderBlock case, for now. (please ignore the method name)

Darin, Ojan, could you please validate that before I go on?
Comment 2 Ojan Vafai 2010-05-27 15:27:42 PDT
Comment on attachment 57277 [details]
wip - first materialization of the idea

This is close to what I had in mind. Except I was picturing EditingBehavior that is a member of Settings. There's a lot of plumbing to get at Settings (e.g. Frame and Document already have settings methods). Actually, I was originally picturing something more like WebCore/editing/htmlediting.h, but the fact that the behavior is runtime settable means we can't just use a bunch of helper functions like that. I think we probably don't need the cpp file though. These methods should all be really dumb if-statements that check the editing behavior and return booleans.

That's all nit-picking though. What you have seems fine to me. I defer to darin here though.
Comment 3 Darin Adler 2010-05-27 15:29:22 PDT
I originally had imagined an enum and functions that took that enum as an argument.

A class is OK too, but I don't think it needs to be mutable. Instead the constructor can take an argument of the behavior, and to change it you can just replace the object with an entirely new one. There's no need to ever take a pointer to one of these.
Comment 4 Ojan Vafai 2010-05-27 15:32:11 PDT
(In reply to comment #3)
> A class is OK too, but I don't think it needs to be mutable. Instead the constructor can take an argument of the behavior, and to change it you can just replace the object with an entirely new one. There's no need to ever take a pointer to one of these.

I don't feel strongly, but this sounds best to me.
Comment 5 Antonio Gomes 2010-05-28 08:38:00 PDT
Created attachment 57331 [details]
wip - second prototype of the idea

> I think we probably don't need the cpp file though. These methods should all > be really dumb if-statements that check the editing behavior and return
> booleans.

Not using dedicated .cpp/.h files for the class anymore. Instead it is not a private class of Settings and it is now called EditingBehaviorController

> A class is OK too, but I don't think it needs to be mutable. Instead the constructor can take an argument of the behavior, and to change it you can just replace the object with an entirely new one. There's no need to ever take a pointer to one of these.

Ok, also made it not mutable: wheneven Settings::setEditingBehavior(EditingBehavior) is called, a new Object is created on the stack.

In this design, for each specific editing behavior there would be a method in Settings and a correspondent method in EditingBehaviorController. The former returns a forward the call of the latter.

The changes in RenderBlock is still shown as an example call site.

What do you think?
Comment 6 Darin Adler 2010-05-28 08:51:22 PDT
(In reply to comment #5)
> In this design, for each specific editing behavior there would be a method in Settings and a correspondent method in EditingBehaviorController. The former returns a forward the call of the latter.

General idea looks good, but I think this is something to be avoided. We don't want to have to add two functions every time we add a new editing behavior.

I suggest making the "controller" class public, and in fact the class itself could be in a separate header file and not be a member of settings. There should be a getter on the Settings class that returns it with a short name. Then you can say:

    settings->editing().shouldMoveCaretToBoundary()

The only part I'm not sure of is what the optimal name for EditingBehaviorController and Settings::editing() are. I think the object represents editing behavior details, interpretation, or policy. Controller might be an OK term too. For the Settings member, the key is brevity.
Comment 7 Darin Adler 2010-05-28 08:59:25 PDT
Comment on attachment 57331 [details]
wip - second prototype of the idea

> +    bool moveCaretToBoundary = settings && settings->shouldMoveCaretToBoundary();

This will need to have a longer name like:

    shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom

Maybe not that exact name, but one more like it.

> -        // We hit this case for Mac behavior when the Y coordinate is below the last box.
> -        ASSERT(!useWindowsBehavior);
> +        // We hit this case for non Windows behavior when the Y coordinate is below the last box.
> +        ASSERT(moveCaretToBoundary);

I think it's OK, and clearer, for the comment to call this "Mac behavior".

It's so annoying that Document::settings() can return 0. Maybe we should have a getter on Document that returns the editing behavior directly instead of always having to go via settings.
Comment 8 Ojan Vafai 2010-05-28 09:55:14 PDT
(In reply to comment #7)
> > -        // We hit this case for Mac behavior when the Y coordinate is below the last box.
> > -        ASSERT(!useWindowsBehavior);
> > +        // We hit this case for non Windows behavior when the Y coordinate is below the last box.
> > +        ASSERT(moveCaretToBoundary);
> 
> I think it's OK, and clearer, for the comment to call this "Mac behavior".

In this specific case, once we add Linux behavior in, it will apply to Mac and Linux. But in that case we could just change "Mac" to "Mac/Linux".

> It's so annoying that Document::settings() can return 0. Maybe we should have a getter on Document that returns the editing behavior directly instead of always having to go via settings.

This sounds great! The null-checks are super-annoying.
Comment 9 Darin Adler 2010-05-28 10:12:50 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > > -        // We hit this case for Mac behavior when the Y coordinate is below the last box.
> > > -        ASSERT(!useWindowsBehavior);
> > > +        // We hit this case for non Windows behavior when the Y coordinate is below the last box.
> > > +        ASSERT(moveCaretToBoundary);
> > 
> > I think it's OK, and clearer, for the comment to call this "Mac behavior".
> 
> In this specific case, once we add Linux behavior in, it will apply to Mac and Linux. But in that case we could just change "Mac" to "Mac/Linux".

Right, but I think even if the comment says "the Mac behavior" it's OK if it's actually used on more platforms than just Mac.

> > It's so annoying that Document::settings() can return 0. Maybe we should have a getter on Document that returns the editing behavior directly instead of always having to go via settings.
> 
> This sounds great! The null-checks are super-annoying.

Maybe Editor is the right object to put this on, rather than Document, though.
Comment 10 Antonio Gomes 2010-05-28 11:00:57 PDT
maybe EditingBehaviorController naming the class and EditingBehaviorPolicy naming the enum?
Comment 11 Antonio Gomes 2010-05-28 12:41:23 PDT
Eric suggested something else, which sounds reasonable to me: EditingBehavior (the class) to be very self contained and not even know about Settings and all.

<eseidel> anyway if you have EditingBehavior(settings->editingBehavior()) then you can ask nice questions like .shouldIDoAwesomeEditingStuff()
<eseidel> EditingBehavior is simply about trying to make the individual code lines not have to make policy decisions about mac vs. linux
<eseidel> they ask the question they want, in "plain english" as functions on the EditingBehavior, adn that class maps from the policy enum to a bool for that behavior
<eseidel> tonikitoo: so I don't think that SEttings would even know about the new EditingBehavior
<eseidel> tonikitoo: it could, but if it does, only to just call EditingBehavior(editingBehavior())

... we would end up with call sites doing

EditingBehavior foo(settings->editingBehavior());
if (foo.shouldXXXYYYZZZ()) {
   // cool stuff
}

of course, enum would be renamed to something else to avoid collision.

thoughts?

ps: it is good that we are putting cards over table, and I am sure we will find out the solution that all would be happy in the end
Comment 12 Darin Adler 2010-05-28 12:47:41 PDT
> <eseidel> anyway if you have EditingBehavior(settings->editingBehavior()) then you can ask nice questions like .shouldIDoAwesomeEditingStuff()
> <eseidel> EditingBehavior is simply about trying to make the individual code lines not have to make policy decisions about mac vs. linux
> <eseidel> they ask the question they want, in "plain english" as functions on the EditingBehavior, adn that class maps from the policy enum to a bool for that behavior

Sure that's the right concept. I agree with Eric.

> <eseidel> tonikitoo: so I don't think that SEttings would even know about the new EditingBehavior
> <eseidel> tonikitoo: it could, but if it does, only to just call EditingBehavior(editingBehavior())
> 
> ... we would end up with call sites doing
> 
> EditingBehavior foo(settings->editingBehavior());
> if (foo.shouldXXXYYYZZZ()) {
>    // cool stuff
> }
> 
> of course, enum would be renamed to something else to avoid collision.
> 
> thoughts?

I think if taken literally it will make the code at the call site too wordy. I want something more like this:

    if (frame->editor()->behavior().shouldFlashCaretNeonWhenTypingOjansName())

And less like this:

    Settings* settings = document->settings();
    if (settings && EditingBehavior(settings->editingBehaviorType()).shouldFlashCaretNeonWhenTypingOjansName())

We could have Settings only know about the enum if we like, but that's not critical. What is critical is that the mapping from enum values to all the functions be in its own header and not mixed in with other stuff.

I don't even think all those functions need to be in a class. That style could be like this:

    Settings* settings = document->settings();
    if (settings && shouldFlashCaretNeonWhenTypingOjansName(settings->editingBehavior()))

Or this:

    if (shouldFlashCaretNeonWhenTypingOjansName(frame->editor()->behavior()))
Comment 13 Ojan Vafai 2010-05-28 13:17:27 PDT
(In reply to comment #12)
>     if (frame->editor()->behavior().shouldFlashCaretNeonWhenTypingOjansName())
>     if (shouldFlashCaretNeonWhenTypingOjansName(frame->editor()->behavior()))

Either one of these is fine with me. I prefer to avoid the other ones where we need to null-check settings. The latter seems simplest to implement, but I don't really care which of these two we do.

Also, can I add this as my own personal easter egg? :)
Comment 14 Antonio Gomes 2010-05-30 20:56:12 PDT
Created attachment 57430 [details]
3rd simple design - feedback?

Summary:

* Implemented it so that call sites will look like the quoted code below:
>     if (frame->editor()->behavior().shouldFlashCaretNeonWhenTypingOjansName())
>     if (shouldFlashCaretNeonWhenTypingOjansName(frame->editor()->behavior()))
* Added WebCore/editing/EditingBehavior.{cpp,h} so all editing behavior methods can will be grouped together on their own header.
* Implemented it so that Settings class only knows about the enum.
* Renamed the enum in Settings from EditingBehavior to EditingBehaviorType, and changed places in WebKit referring to it.
Comment 15 Antonio Gomes 2010-05-30 21:01:09 PDT
Comment on attachment 57430 [details]
3rd simple design - feedback?

err ... wrong file :(, sorry for bug spamming
Comment 16 Antonio Gomes 2010-05-30 21:02:05 PDT
Created attachment 57431 [details]
3rd simple design (right file) - feedback?
Comment 17 Darin Adler 2010-05-30 21:02:28 PDT
Comment on attachment 57430 [details]
3rd simple design - feedback?

> +bool EditingBehavior::shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom() const
> +{
> +    if (Settings* settings = settingsForEditor())
> +        return settings->editingBehavior() != EditingWindowsBehavior;
> +
> +    return false;
> +}

We need to write a lot of these functions, so I'd like these to be as streamlined as possible. I suggest a helper function named type() that returns the editing behavior type and then:

    bool shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom() const { return type() != EditingWindowsBehavior; }

And I'd like to see these in the header if possible, since we want these to be easy to read.

But I think what we actually want is this:

    class EditingBehavior {
    public:
        EditingBehavior(EditingBehaviorType type) : m_type(type) { }

        bool shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom() const { return m_type != EditingWindowsBehavior; }
 
    private:
        EditingBehaviorType m_type;
    }

There's no reason for EditingBehavior to have an Editor* in it.
Comment 18 Antonio Gomes 2010-05-30 21:05:46 PDT
(In reply to comment #17)
> (From update of attachment 57430 [details])
> > +bool EditingBehavior::shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom() const
> > +{
> > +    if (Settings* settings = settingsForEditor())
> > +        return settings->editingBehavior() != EditingWindowsBehavior;
> > +
> > +    return false;
> > +}
> 
> We need to write a lot of these functions, so I'd like these to be as streamlined as possible. I suggest a helper function named type() that returns the editing behavior type and then:
> 
>     bool shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom() const { return type() != EditingWindowsBehavior; }
> 
> And I'd like to see these in the header if possible, since we want these to be easy to read.
> 
> But I think what we actually want is this:
> 
>     class EditingBehavior {
>     public:
>         EditingBehavior(EditingBehaviorType type) : m_type(type) { }
> 
>         bool shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom() const { return m_type != EditingWindowsBehavior; }
> 
>     private:
>         EditingBehaviorType m_type;
>     }
> 
> There's no reason for EditingBehavior to have an Editor* in it.

darin, I uploaded a wrong file. could you re-check the later, please? Or your comments remain the same for that new one?
Comment 19 Darin Adler 2010-05-30 21:19:38 PDT
(In reply to comment #18)
> darin, I uploaded a wrong file. could you re-check the later, please? Or your comments remain the same for that new one?

The same comments apply to the new one, but about Settings* rather than Editor*.
Comment 20 Darin Adler 2010-05-30 21:21:09 PDT
In Settings:

    void setEditingBehaviorType(EditingBehaviorType type) { m_editingBehaviorType = type; }
    EditingBehavior editingBehavior() const { return static_cast<EditingBehaviorType>(m_editingBehavior); }

Note that the getter returns EditingBehavior, not EditingBehaviorType. That's where the "magic" happens.
Comment 21 Antonio Gomes 2010-05-31 08:17:24 PDT
Created attachment 57468 [details]
4th try - feedback?

Summary:
    * Renamed EditingBehavior enum in Settings to EditingBehaviorType
    * Added EditingBehavior class, living in WebCore/editing/EditingBehavior.h . It does not know about external classes but EditingBehaviorType
    * Settings now has:
        - a setter for the EditingBehaviorType: setEditingBehaviorType(EditingBehaviorType)
        - a getter for the EditinbeBehavior, based on the type: EditingBehavior editingBehavior()
    * Call sites in WebKit/ adjusted to work with the above changes.
    * Editor.cpp has a behavior() getter, relying on Settings' editingBehavior method.
    * Call sites will look like:
        if (frame->editor()->behavior().shouldThisDesignBeUsed_questionMark()))
            /// do xxx
Comment 22 Darin Adler 2010-05-31 09:36:23 PDT
Comment on attachment 57468 [details]
4th try - feedback?

> +#ifndef EditingBehavior_h
> +#define EditingBehavior_h
> +
> +#include "Settings.h"

It seems  shame to have to have EditingBehavior.h include the whole Settings.h file just to get the definition of the EditingBehaviorType enum. I would put that enum into its own file.

> +    // Platform variant editing behavior methods.

 I would not call these "methods" and nor would I use the phrase them "platform variant". I would say something more like:

    // Individual functions for each case where we have more than one style of editing behavior.
    // Create a new function for any platform difference so we can control it here.

My comment isn't perfect either, but I think it may be a bit easier to understand.

> +} // namspace WebCore

Typo "namspace".

> +        EditingBehavior editingBehavior() const;

Since we have an Editor function that returns EditingBehavior, we could make this Settings function just return EditingBehaviorType and make it so that Settings doesn't know anything about EditingBehavior at all.

> --- a/WebCore/rendering/RenderBlock.cpp
> +++ b/WebCore/rendering/RenderBlock.cpp
> @@ -19,16 +19,17 @@
>   * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
>   * Boston, MA 02110-1301, USA.
>   */
>  
>  #include "config.h"
>  #include "RenderBlock.h"
>  
>  #include "Document.h"
> +#include "EditingBehavior.h"

This include is not needed because Editor.h takes care of it already.

Otherwise this looks great to me.
Comment 23 Antonio Gomes 2010-05-31 14:33:36 PDT
Created attachment 57495 [details]
patch v1 - functional

* Addresses Darin's previously requests for feedback{1,2,3,4}

Summary:

* EditingBehavior enum renamed to EditingBehaviorTypes and move out to editing/EditingBehaviorTypes.h
- Also done with changes needed in WebKit/ due to the enum change.

* EditingBehavior class added. It is not to be directly instantiated, but rather accessed via Editor::behavior() getter.
- Settings class will only know about the EditingBehaviorTypes enum, not about EditingBehavior class
- EditingBehavior will only know about the EditingBehaviorTypes enum, not about Settings or Editor.

* All existing Settings::defaultBehavior() == Editing{Mac,Windows}Behavior changed to call methods in EditingBehavior class - (please review the methods names, I am open to suggestions).
* Added new files to the build various sytems (hope I did not miss any).
Comment 24 WebKit Review Bot 2010-05-31 14:38:41 PDT
Attachment 57495 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/editing/EditingBehaviorTypes.h:39:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 25 WebKit Review Bot 2010-05-31 15:25:25 PDT
Attachment 57495 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2805024
Comment 26 Antonio Gomes 2010-05-31 17:49:00 PDT
Created attachment 57502 [details]
patch v2

Same patch as patch 57495 (see comment #23 for a summary), but includes a fix for Chromium-linux build (in comment #25) and style nit in comment #24.

As for the Mac build, I can not see the error log, and it gets hard to fix, but I will fix at office tomorrow.

Apart from that, patch is ready for review.
Comment 27 Antonio Gomes 2010-06-01 07:04:22 PDT
Created attachment 57540 [details]
patch v2.1 - same as v2, but builds on Mac

Same as patch 57502, but builds on Mac. See comment #26
Comment 28 Antonio Gomes 2010-06-01 08:56:12 PDT
Created attachment 57550 [details]
patch v2.2 - same as v2, but really builds on Mac

err. I missed settings it as private header.

see comment #23 for summary.
Comment 29 Darin Adler 2010-06-01 11:27:50 PDT
Comment on attachment 57550 [details]
patch v2.2 - same as v2, but really builds on Mac

Patch is great. What we need to work on are the names of the behaviors. For each one, we should write a plain language sentence or paragraph describing the behavior and then summarize that for the name of the function.

> +    bool shouldConsiderStylePresentOnlyIfThroughoutTheSelection() const { return m_type != EditingMacBehavior; }

I think this name is a little vague. What this controls is style-toggling behavior. It doesn't affect other "is style present" situations, such as when to check the "Bold" menu item in an application. Does it?

> +    bool shouldExtendSelectionFromEndPointAlways() const { return m_type != EditingMacBehavior; }

This name is too broad. It controls only shift-click behavior, not selection extension that's done with shift-arrow-keys for example.

> +    bool shouldKeybardSelectionExtendForHorizontalNavigation() const { return m_type == EditingMacBehavior; }

What does "extend for horizontal navigation" mean? This one needs work.

> +    bool shouldGrowSelectionWhenExtendingToBoundary() const { return m_type == EditingMacBehavior; }

If you don't grow the selection when extending to a boundary, what do you do? Shrink the selection?
Comment 30 Ojan Vafai 2010-06-01 15:45:33 PDT
Comment on attachment 57550 [details]
patch v2.2 - same as v2, but really builds on Mac

WebCore/editing/Editor.cpp:104
 +      if (!m_frame || !m_frame->page() || !m_frame->settings())
Do you need to null check page here? Looks like m_frame and settings should be sufficient.

WebCore/editing/EditingBehavior.h:43
 +      bool shouldKeybardSelectionExtendForHorizontalNavigation() const { return m_type == EditingMacBehavior; }
typo: Keybard
Comment 31 Antonio Gomes 2010-06-01 18:34:53 PDT
Comment on attachment 57550 [details]
patch v2.2 - same as v2, but really builds on Mac

Removing the review flag for now based on current not-so-good names patch comes up with.

I will work on each method naming by deeply understanding what exactly it tries to work around by looking at the original commits that introduced it.

help welcome.
Comment 32 Darin Adler 2010-06-02 19:02:00 PDT
We can land the patch with names for the ones we have good names for, and then add more incrementally. They don't all have to change at once.
Comment 33 Antonio Gomes 2010-06-03 08:57:13 PDT
(In reply to comment #32)
> We can land the patch with names for the ones we have good names for, and then add more incrementally. They don't all have to change at once.

Ok, Darin. I can prepare it later today (today and tomorrow are holidays in Brazil)
Comment 34 Antonio Gomes 2010-06-04 13:08:09 PDT
> > We can land the patch with names for the ones we have good names for, and then add more incrementally. They don't all have to change at once.
> 
> Ok, Darin. I can prepare it later today (today and tomorrow are holidays in Brazil)

... finally back on it.

@darin: you did not commented about these ones:

bool shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom() const { ... }
bool shouldCenterAlignWhenSelectionIsRevealed() const { ... }
bool shouldConsiderMouseSelectionAsDirectional() const { ... }

Do you these are good to go as-are? If so, we could start with them, and incrementally working on the missing ones.
Comment 35 Antonio Gomes 2010-06-04 14:37:33 PDT
Created attachment 57916 [details]
patch v2.3 - part I

As per suggestion in comment #32, patch "v2.3 - part I" introduces the following methods to EditingBehavior.h:

bool shouldMoveCaretToHorizontalBoundaryWhenPastTopOrBottom() const { ... }
bool shouldCenterAlignWhenSelectionIsRevealed() const { ... }
bool shouldConsiderMouseSelectionAsDirectional() const { ... }

Follow up work will replace all remaining calls to Editing{Mac,Windows}Behavior by new functions in EditingBehavior class.

remaing ones, currently bad named are:

(these are close imo)
bool shouldConsiderStylePresentOnlyIfThroughoutTheSelection() const { ... }
bool shouldExtendSelectionFromEndPointAlways() const { ... }

(these need love)
bool shouldKeybardSelectionExtendForHorizontalNavigation() const { ... }
bool shouldGrowSelectionWhenExtendingToBoundary() const { ...}
Comment 36 Antonio Gomes 2010-06-04 14:42:20 PDT
Comment on attachment 57916 [details]
patch v2.3 - part I

wrong patch
Comment 37 Antonio Gomes 2010-06-04 14:44:58 PDT
Created attachment 57919 [details]
 patch v2.3 - part I   (right file) 

see comment #35 for summary
Comment 38 Antonio Gomes 2010-06-07 12:14:45 PDT
Created attachment 58070 [details]
patch v2.3 - part I (agaisnt ToT)

Same as patch 57919 (patch v2.3 - part I (right file)), but rebased against ToT for the bots to build it.

Details: Patch did not build on EWS's bot because it had a conflit in WebCore/WebCore.xcodeproj/project.pbxproj

For summary see comment #35.
Comment 39 Early Warning System Bot 2010-06-07 12:25:06 PDT
Attachment 58070 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3160141
Comment 40 Antonio Gomes 2010-06-07 13:55:09 PDT
Created attachment 58081 [details]
(committed with r60841, reviewed by Ojan and Darin) patch v2.3 - part I (agaisnt ToT and builds)

Same as patch v2.3 (see comment #35), but now against ToT and fixed build issue in comment #39.
Comment 41 Ojan Vafai 2010-06-07 18:28:20 PDT
Comment on attachment 58081 [details]
(committed with r60841, reviewed by Ojan and Darin) patch v2.3 - part I (agaisnt ToT and builds)

This looks good to me with the nit below fixed.

WebCore/editing/Editor.cpp:104
 +      if (!m_frame || !m_frame->page() || !m_frame->settings())
Do you need to null-check page here? m_frame and settings should be sufficient.
Comment 42 WebKit Review Bot 2010-06-07 20:24:33 PDT
http://trac.webkit.org/changeset/60816 might have broken GTK Linux 64-bit Debug
Comment 43 Antonio Gomes 2010-06-08 06:49:04 PDT
Comment on attachment 58081 [details]
(committed with r60841, reviewed by Ojan and Darin) patch v2.3 - part I (agaisnt ToT and builds)

Clearing flags on attachment: 58081

Committed r60841: <http://trac.webkit.org/changeset/60841>
Comment 44 Antonio Gomes 2010-06-28 05:17:59 PDT
Finally some time to back work on this! I will follow Darin's suggestion to follow up incrementally here, case by case.

> > +    bool shouldConsiderStylePresentOnlyIfThroughoutTheSelection() const { return m_type != EditingMacBehavior; }
> 
> I think this name is a little vague. What this controls is style-toggling behavior. It doesn't affect other "is style present" situations, such as when to check the "Bold" menu item in an application. Does it?

In fact, it is a possible situation, yes.

In QtWebKit we have something called 'WebAction' (see [1]), accessible through the QWebPage::WebAction enum. It does include some text specific styles including bold, italic, underline , subscript, superscriput, strikethrough.

[1] http://doc.qt.nokia.com/4.7-snapshot/qwebpage.html#WebAction-enum

It is possible app developers to connect a menu item to, say for instance, QWebPage::ToggleBold, like in the snippet below:

void MainWindow::editMenu()
{
  QToolBar* bar = addToolBar("Edit");
  bar->addAction(page()->action(QWebPage::ToggleBold));
  ...
}

It enables QtWebKit to enable/disable and to check/uncheck menu items in the application chrome side, according to the WebCore state. Same mechanism is applied to Edit->Paste|Copy|Cut menu items. If no text is in clipboard then Paste menu item will be disable. If no text selected in content, Copy will be disabled, and so on ...

Specifically about ToggleBold, suppose user makes a text selection with mouse. At some point, EditorClient::respondToChangedSelection will be called.

void EditorClientQt::respondToChangedSelection()
{
  ...
  m_page->d->updateEditorActions();
  ...
}

which will end up calling

QWebPagePrivate::updateAction(QWebPage::WebAction action)
void QWebPagePrivate::updateEditorActions()
{
  updateAction(QWebPage::Cut);
  updateAction(QWebPage::Copy);
  updateAction(QWebPage::Paste);
  ...
  updateAction(QWebPage::ToggleBold);
  ...
}

void QWebPagePrivate::updateAction(QWebPage::WebAction action)
{
 ...
 WebCore::FrameLoader *loader = mainFrame->d->frame->loader();
 WebCore::Editor *editor = page->focusController()->focusedOrMainFrame()->editor();

 bool enabled = a->isEnabled();
 bool checked = a->isChecked();

 switch (action) {
   case QWebPage::Back:
   ...
   default: {
     // see if it's an editor command
     const char* commandName = editorCommandForWebActions(action);

     // if it's an editor command, let it's logic determine state
     if (commandName) {
     Editor::Command command = editor->command(commandName);
     enabled = command.isEnabled();
     if (enabled)
       checked = command.state() != FalseTriState;
     else
       checked = false;
   }
   break;
 }

If the user open up the Edit menu item, "command.state()" will do the trick of saying if the menu item has to be enabled, checked, or not for the ToggleBold action.
Comment 45 Antonio Gomes 2010-06-28 05:35:40 PDT
Created attachment 59894 [details]
patch v3 - part II

Patch addresses the situation described in comment #44.
Comment 46 Antonio Gomes 2010-06-28 08:10:31 PDT
Comment on attachment 59894 [details]
patch v3 - part II

cq- so I can fix some bad English in "Patch makes situations that depend on this check like this to be controlled by the EditingBehavior class." in the ChangeLog.

supposing it gets r+, of course.
Comment 47 Antonio Gomes 2010-06-30 20:01:31 PDT
Created attachment 60186 [details]
patch v3.1 - part II

Same as patch v3 - part II (attachment 59894 [details]), but fixed ChangeLog entry.

@Darin or Ojan, could you please review? (please read comment #44)
Comment 48 Antonio Gomes 2010-07-05 15:25:05 PDT
(In reply to comment #47)
> Created an attachment (id=60186) [details]
> patch v3.1 - part II
> 
> Same as patch v3 - part II (attachment 59894 [details]), but fixed ChangeLog entry.
> 
> @Darin or Ojan, could you please review? (please read comment #44)

Ping review(?)
Comment 49 Ojan Vafai 2010-07-07 14:21:51 PDT
Comment on attachment 60186 [details]
patch v3.1 - part II

>  static TriState stateStyle(Frame* frame, int propertyID, const char* desiredValue)
>  {
>      RefPtr<CSSMutableStyleDeclaration> style = CSSMutableStyleDeclaration::create();
> -    style->setProperty(propertyID, desiredValue);
> -    return frame->editor()->selectionHasStyle(style.get());
> +    style->setProperty(propertyID, desiredValue); // We need to add this style to pass it to selectionStartHasStyle / selectionHasStyle
> +
> +    TriState styleIsPresent;
> +    if (!frame->editor()->behavior().shouldConsiderStylePresentOnlyIfThroughoutTheSelection())
> +        styleIsPresent = frame->editor()->selectionStartHasStyle(style.get()) ? TrueTriState : FalseTriState;
> +    else
> +        styleIsPresent = frame->editor()->selectionHasStyle(style.get());
> +
> +    return styleIsPresent;

This is a change in behavior. If we keep it, it should have a layout test and should maybe be a separate patch. In either case, I'm not sure this change is correct. I think Darin was just asking that the name of the EditingBehavior method be more specific. How about something like shouldToggleStyleBasedOnStartOfSelection?
Comment 50 Antonio Gomes 2010-07-08 08:57:23 PDT
(In reply to comment #49)
> I think Darin was just asking that the name of the EditingBehavior method be more specific. How about something like shouldToggleStyleBasedOnStartOfSelection?

Thank you for the comments, Ojan!

What I tried to explain in comment #44 is the situation "consider text as styled if style is present in all text nodes of a selection" encloses the following cases:

1) the toggle style behavior. For that situation, shouldToggleStyleBasedOnStartOfSelection as a name it great.

- example: the BOLD button of a rich text editor.

2) the "what is style state" situation? For that one, the method name you suggested does not work very well.

- example: an application menu item that has its checked/unchecked status relying on the style considered as present or not.

What do you think?


> This is a change in behavior. If we keep it, it should have a layout test and should maybe be a separate patch. In either case, I'm not sure this change is correct. 

Could you tell why you think it is wrong?
Comment 51 Darin Adler 2010-07-08 10:48:06 PDT
I think Ojanā€™s main point is that first we want to refactor these settings and give them good names without changing behavior. Changing behavior should be handled separately.
Comment 52 Ojan Vafai 2010-07-08 11:13:20 PDT
(In reply to comment #50)
> What I tried to explain in comment #44 is the situation "consider text as styled if style is present in all text nodes of a selection" encloses the following cases:

Doh. Sorry, I missed that whole set of comments.

On a side note, in the future, followup patches for a given issue should be done in new bugs. They new bug can point to the original, but it makes it easier to follow the relevant bits for the patch in question. In general, it should be one patch per bug.

> > This is a change in behavior. If we keep it, it should have a layout test and should maybe be a separate patch. In either case, I'm not sure this change is correct. 
> 
> Could you tell why you think it is wrong?

When shouldConsiderStylePresentOnlyIfThroughoutTheSelection is true it's taking what used to be a tri-state and making it a boolean. Specifically, I think we'd return the wrong value in the following case:
<div id=foo contentEditable>foo<b>bar</b></div>
<script>
window.getSelection().selectAllChildren(foo);
console.log(document.queryCommandIndeterm('bold'));
</script>

That should log "true" to the console. With your patch, I think it would log "false". On the other hand, this patch fixes queryCommandState to return the correct value in this case. Currently it would return true for the above and should return false.

It would be unfortunate to fix one case and break the other. Can we do this as two separate patches?

1. Just change the current editingBehavior call. Call the method shouldToggleStyleBasedOnStartOfSelection, but also put a FIXME to use that method in stateStyle for the cases where it's used for queryCommandState.
2. Rename shouldToggleStyleBasedOnStartOfSelection to shouldConsiderStylePresentOnlyIfThroughoutTheSelection and use it in stateStyle, but not if it's being used to get a return value for queryCommandIndeterm. This is a bit complicated because right now both queryCommandState and queryCommandIndeterm end up calling the same method. So it would require a few more changes to EditorCommand.cpp.
Comment 53 Antonio Gomes 2010-07-08 11:54:24 PDT
Created attachment 60921 [details]
(r62815,r=ojan) patch v3.2 - part II

As suggested by Ojan and Darin, keep only refactor work happening in this bug. No behavioral should happen here.
Comment 54 Antonio Gomes 2010-07-08 12:13:14 PDT
Comment on attachment 60921 [details]
(r62815,r=ojan) patch v3.2 - part II

Clearing flags on attachment: 60921

Committed r62815: <http://trac.webkit.org/changeset/62815>
Comment 55 WebKit Review Bot 2010-07-08 12:55:38 PDT
http://trac.webkit.org/changeset/62815 might have broken Qt Linux Release
Comment 56 Antonio Gomes 2010-07-08 13:00:09 PDT
(In reply to comment #55)
> http://trac.webkit.org/changeset/62815 might have broken Qt Linux Release

Fixed in r62815 . Bogus ! in the IF statement.
Comment 57 Robert Hogan 2010-07-08 13:42:48 PDT
This broke editing/5195166-1.html on the Qt bot.
Comment 58 Antonio Gomes 2010-07-08 14:06:04 PDT
(In reply to comment #57)
> This broke editing/5195166-1.html on the Qt bot.

robert, this breakage is related to http://trac.webkit.org/changeset/62816 not this bug.
Comment 59 Antonio Gomes 2010-07-09 11:39:22 PDT
Given the amount of comments in this bug and that its main goal is also accomplished, lets consider it as FIXED, and move the work involving specific missing bits in follow up bugs.

I will file all of them now, and link to this one for the record.

Thank you Darin and Ojan.
Comment 60 Antonio Gomes 2010-07-09 14:37:59 PDT
(In reply to comment #29)
> > +    bool shouldExtendSelectionFromEndPointAlways() const { return m_type != EditingMacBehavior; }
> 
> This name is too broad. It controls only shift-click behavior, not selection extension that's done with shift-arrow-keys for example.

bug 41977.

> > +    bool shouldKeybardSelectionExtendForHorizontalNavigation() const { return m_type == EditingMacBehavior; }
> 
> What does "extend for horizontal navigation" mean? This one needs work.

bug 41976.
 
> > +    bool shouldGrowSelectionWhenExtendingToBoundary() const { return m_type == EditingMacBehavior; }
> 
> If you don't grow the selection when extending to a boundary, what do you do? Shrink the selection?

bug 41975.
Comment 61 Antonio Gomes 2010-07-09 14:39:05 PDT
(In reply to comment #52)
> 2. Rename shouldToggleStyleBasedOnStartOfSelection to shouldConsiderStylePresentOnlyIfThroughoutTheSelection and use it in stateStyle, but not if it's being used to get a return value for queryCommandIndeterm. This is a bit complicated because right now both queryCommandState and queryCommandIndeterm end up calling the same method. So it would require a few more changes to EditorCommand.cpp.

bug 41989.

And we're done here.