Bug 45427 – [EFL] Add setting api for enabling encoding detector

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Ryuan Choi]

contents was broken in some of korean site like http://www.zdnet.co.kr
It's because those site doesn't specified encoding.

[Ryuan Choi]

Created attachment 66974 [details]
Patch

[chris fleizach]

Comment on attachment 66974 [details]
Patch

> Index: WebKit/efl/ChangeLog
> ===================================================================
> --- WebKit/efl/ChangeLog	(revision 67042)
> +++ WebKit/efl/ChangeLog	(working copy)
> @@ -1,3 +1,15 @@
> [phone]  Ryuan Choi  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [EFL] enable UsesEncodingDetector
> +        https://bugs.webkit.org/show_bug.cgi?id=45427
> +
> +        Enable encodingDetector.
> +

please add a more informative comment as to why you want to enable here. 

and please add a layout test to confirm the desired behavior is now present.

[Ryuan Choi]

(In reply to comment #2)
> (From update of attachment 66974 [details])
> > Index: WebKit/efl/ChangeLog
> > ===================================================================
> > --- WebKit/efl/ChangeLog	(revision 67042)
> > +++ WebKit/efl/ChangeLog	(working copy)
> > @@ -1,3 +1,15 @@
> > [phone]  Ryuan Choi  <[e-mail]>
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        [EFL] enable UsesEncodingDetector
> > +        https://bugs.webkit.org/show_bug.cgi?id=45427
> > +
> > +        Enable encodingDetector.
> > +
> 
> please add a more informative comment as to why you want to enable here. 
> 
> and please add a layout test to confirm the desired behavior is now present.

Hi, Thank you for your reviewing.
Unfortunately EFL doesn't have layout test yet.

but, while analyzing more, I found that It's not real solution for this bug.
Although applying this patch, I got a broken content sometimes.

after analyzed more, I'll prepare.

[Ryuan Choi]

Created attachment 67355 [details]
testpage

This page contains korean letter.
Unless enabling EncodingDetector, this page will not be rendered properly.

we need setting APIs to enable encodingDetector like other ports.

[Ryuan Choi]

Created attachment 67359 [details]
Patch

[Ryuan Choi]

(In reply to comment #5)
> Created an attachment (id=67359) [details]
> Patch

I changed approach to add setting api instead of changing encoding detector directly.

[Rafael Antognolli]

(In reply to comment #6)
> (In reply to comment #5)
> > Created an attachment (id=67359) [details] [details]
> > Patch
> 
> I changed approach to add setting api instead of changing encoding detector directly.

The patch is nice IMHO. Do you still have the problem that you described in comment #3? Although the new approach is better, I don't think it will solve that.

[Lucas De Marchi]

Comment on attachment 67359 [details]
Patch

> Index: WebKit/efl/ewk/ewk_view.cpp
> ===================================================================
[...]
> +/**
> + * Sets the encoding detector.
> + *
> + * @param o view object to set if encoding detector is enabled.
> + * @return @c EINA_TRUE on success and @c EINA_FALSE on failure
> + */
> +Eina_Bool ewk_view_setting_encoding_detector_set(Evas_Object* o, Eina_Bool enable)
> +{
> +    EWK_VIEW_SD_GET_OR_RETURN(o, sd, EINA_FALSE);
> +    EWK_VIEW_PRIV_GET_OR_RETURN(sd, priv, EINA_FALSE);
> +    enable = !!enable;
> +    if (priv->settings.local_storage != enable) {
s/local_storage/encoding_detector/

[...]
> +/**
> + * Gets if the encoding detector is enabled.
> + *
> + * @param o view object to set if encoding detector is enabled.
s/set/get

> + * @return @c EINA_TRUE if encoding detector is enabled, @c EINA_FALSE if not.
It returns EINA_FALSE on errors, too.

[Ryuan Choi]

(In reply to comment #8)
> (From update of attachment 67359 [details])
> > Index: WebKit/efl/ewk/ewk_view.cpp
> > ===================================================================
> [...]
> > +/**
> > + * Sets the encoding detector.
> > + *
> > + * @param o view object to set if encoding detector is enabled.
> > + * @return @c EINA_TRUE on success and @c EINA_FALSE on failure
> > + */
> > +Eina_Bool ewk_view_setting_encoding_detector_set(Evas_Object* o, Eina_Bool enable)
> > +{
> > +    EWK_VIEW_SD_GET_OR_RETURN(o, sd, EINA_FALSE);
> > +    EWK_VIEW_PRIV_GET_OR_RETURN(sd, priv, EINA_FALSE);
> > +    enable = !!enable;
> > +    if (priv->settings.local_storage != enable) {
> s/local_storage/encoding_detector/
> 
> [...]
> > +/**
> > + * Gets if the encoding detector is enabled.
> > + *
> > + * @param o view object to set if encoding detector is enabled.
> s/set/get
> 
> > + * @return @c EINA_TRUE if encoding detector is enabled, @c EINA_FALSE if not.
> It returns EINA_FALSE on errors, too.

oops, sorry I mistake.

[Ryuan Choi]

Created attachment 67410 [details]
Patch

[Ryuan Choi]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Created an attachment (id=67359) [details] [details] [details]
> > > Patch
> > 
> > I changed approach to add setting api instead of changing encoding detector directly.
> 
> The patch is nice IMHO. Do you still have the problem that you described in comment #3? Although the new approach is better, I don't think it will solve that.

right, this is only for fixing general case like testpage I attached.

to solve completely, I think that we need to improve Encoding Detection algorithm.
Current TextEncodingDetectorICU algorithm sometimes failed to get proper encoding for this site (zdnet.co.kr)
This is not for EFL, I reproduced this in Gtklauncher, epiphany, Chrome in linux.

now I don't have idea to improve encoding detector itself
and I think that adding encoding detector api and improvement can be separated.

[Rafael Antognolli]

Ok, then this implementation is nice.

Another thing that I forgot before, it would be good to add the default setting in _ewk_view_priv_new() (ok, I know that it's off by default, but having it explicit would be better).

[Antonio Gomes]

(In reply to comment #12)
> Ok, then this implementation is nice.
> 
> Another thing that I forgot before, it would be good to add the default setting in _ewk_view_priv_new() (ok, I know that it's off by default, but having it explicit would be better).

Please address that.

[Ryuan Choi]

Created attachment 67560 [details]
patch with Rafael suggested

[Ryuan Choi]

(In reply to comment #13)
> (In reply to comment #12)
> > Ok, then this implementation is nice.
> > 
> > Another thing that I forgot before, it would be good to add the default setting in _ewk_view_priv_new() (ok, I know that it's off by default, but having it explicit would be better).
> 
> Please address that.

could you review this? I applied this as default setting as Rafael suggested.

[Eric Seidel (no email)]

Comment on attachment 67560 [details]
patch with Rafael suggested

rs=me?  Would be helpful if other EFL contributors could do informal reviews.  This looks sane to me, but I don't know much EFL. I don't think EFL has any official reviewers yet so I'm attempting to help move things along.  I'm happy to not rubber-stamp EFL patches if that's preferred.

[WebKit Commit Bot]

Comment on attachment 67560 [details]
patch with Rafael suggested

Clearing flags on attachment: 67560

Committed r69783: <http://trac.webkit.org/changeset/69783>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.