Bug 97176 – Add setUsesEncodingDetector function in InternalSettings.

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Tools / Tests

[Kangil Han]

To enable encoding detector, a setting function would be needed.

[Kangil Han]

Created attachment 164848 [details]
patch

[Kangil Han]

Created attachment 164860 [details]
patch

rebase!

[kov's GTK+ EWS bot]

Comment on attachment 164860 [details]
patch

Attachment 164860 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13959002

Comment on attachment 164860 [details]
patch

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

> Source/WebCore/testing/InternalSettings.cpp:662
> +void InternalSettings::setUsesEncodingDetector(bool enabled, ExceptionCode& ec)

It is better to omit unused parameter "ec" to make some compilers not to issue unused parameter warning.

[Kangil Han]

(In reply to comment #4)
> (From update of attachment 164860 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164860&action=review
> 
> > Source/WebCore/testing/InternalSettings.cpp:662
> > +void InternalSettings::setUsesEncodingDetector(bool enabled, ExceptionCode& ec)
> 
> It is better to omit unused parameter "ec" to make some compilers not to issue unused parameter warning.

Actually, it is used in macro. :-)

#define InternalSettingsGuardForSettings()  \
    if (!settings()) { \
        ec = INVALID_ACCESS_ERR; \
        return; \
    }

[Kangil Han]

Created attachment 164890 [details]
patch

[Alexey Proskuryakov]

Comment on attachment 164890 [details]
patch

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

> Source/WebCore/testing/InternalSettings.idl:85
> +        void setUsesEncodingDetector(in boolean enabled) raises(DOMException);

"Internal settings set uses encoding detector" is unnecessarily bad grammar. Please improve it.

The function never raises exceptions, so it should not be marked as such, even if other functions in this file are.

[Alexey Proskuryakov]

> The function never raises exceptions, so it should not be marked as such, even if other functions in this file are.

It does, surprisingly, so please ignore this comment.

[Darin Adler]

(In reply to comment #8)
> > The function never raises exceptions, so it should not be marked as such, even if other functions in this file are.
> 
> It does, surprisingly, so please ignore this comment.

Huh? I don’t see any code that raises an exception.

[Kangil Han]

(In reply to comment #9)
> (In reply to comment #8)
> > > The function never raises exceptions, so it should not be marked as such, even if other functions in this file are.
> > 
> > It does, surprisingly, so please ignore this comment.
> 
> Huh? I don’t see any code that raises an exception.

I guess ap found 'INVALID_ACCESS_ERR' line below. :-)

#define InternalSettingsGuardForSettings()  \
    if (!settings()) { \
        ec = INVALID_ACCESS_ERR; \
        return; \
    }

[Kangil Han]

Created attachment 165035 [details]
patch

I hope @ap would like updated function name. :-)

[Kangil Han]

I would like to have review for landing this patch.

[Andreas Kling]

Comment on attachment 165035 [details]
patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.

[Sam Sneddon [:gsnedders]]

The setting itself landed in bug 16482; we already have a generic API to mutate settings, so I don't see what's needed (plus this is ancient!).