[Webkit-unassigned] [Bug 109404] Add selectTrailingWhitespaceEnabled setting to WebCore::Page

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=109404





--- Comment #30 from Manuel Rego Casasnovas <rego at igalia.com>  2013-03-07 01:11:24 PST ---
(In reply to comment #29)
> (From update of attachment 191692 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191692&action=review
> 
> > Source/WebCore/page/Settings.in:199
> > +# smartInsertDeleteEnabled and selectTrailingWhitespaceEnabled are mutually
> > +# exclusive, meaning that enabling one will disable the other.
> 
> This seems like a new behavior and I'm not sure how you would enforce this for all ports.  For example, in Chromium, the current behavior is:
> 
> bool RenderViewImpl::isSmartInsertDeleteEnabled() {
> #if defined(OS_MACOSX)
>   return true;
> #else
>   return false;
> #endif
> }
> 
> bool RenderViewImpl::isSelectTrailingWhitespaceEnabled() {
> #if defined(OS_WIN)
>   return true;
> #else
>   return false;
> #endif
> }
> 
> Which means on Chromium Linux, both are false.

Maybe in Chromium is different, but I included the comment because of it's what is explained in WebCore::Editor:
    // smartInsertDeleteEnabled and selectTrailingWhitespaceEnabled are 
    // mutually exclusive, meaning that enabling one will disable the other.
    bool smartInsertDeleteEnabled();
    bool isSelectTrailingWhitespaceEnabled();

Besides, the ports allowing to change these settings (like mac or win) where changing the other setting to the opposite value when a setting was established. Anyway I can remove the comment here if this is not true for all the ports.

> > Source/WebKit/chromium/public/WebSettings.h:150
> > +    virtual void setSelectTrailingWhitespaceEnabled(bool) = 0;
> 
> I don't think we need to add an API for this since I don't think we plan on changing these during runtime.  The exception is for DRT, but that can use the WebCore Internals API.

Yes for DRT I'm already using the internals.

> > Source/WebKit/chromium/public/WebViewClient.h:-190
> > -    virtual bool isSmartInsertDeleteEnabled() { return true; }
> > -    virtual bool isSelectTrailingWhitespaceEnabled() { return true; }
> 
> The problem with removing this is that we'll no longer get the correct behavior in Chromium, which overrides these virtual methods-- see the RenderViewImpl snippet above.  As long as we retain the correct behavior, it's OK to remove this.
> 
> There are a couple ways you could do this, (1) Find a good time to set this (maybe WebViewImpl's constructor?) or (2) set the right default value in Settings.cpp (e.g., see defaultUnifiedTextCheckerEnabled in Settings.cpp and Settings.in).

I'll provide a patch following the 2nd approach. Thanks for the feedback.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.