Bug 45427

Summary: [EFL] Add setting api for enabling encoding detector
Product: WebKit Reporter: Ryuan Choi <ryuan.choi>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: antognolli+webkit, commit-queue, lucas.de.marchi, tonikitoo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch
none
testpage
none
Patch
none
Patch
none
patch with Rafael suggested none

Description Ryuan Choi 2010-09-08 17:45:13 PDT
contents was broken in some of korean site like http://www.zdnet.co.kr
It's because those site doesn't specified encoding.
Comment 1 Ryuan Choi 2010-09-08 17:49:47 PDT
Created attachment 66974 [details]
Patch
Comment 2 chris fleizach 2010-09-09 00:48:27 PDT
Comment on attachment 66974 [details]
Patch

> Index: WebKit/efl/ChangeLog
> ===================================================================
> --- WebKit/efl/ChangeLog	(revision 67042)
> +++ WebKit/efl/ChangeLog	(working copy)
> @@ -1,3 +1,15 @@
> +2010-09-08  Ryuan Choi  <ryuan.choi@samsung.com>
> +
> +        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.
Comment 3 Ryuan Choi 2010-09-10 02:03:35 PDT
(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 @@
> > +2010-09-08  Ryuan Choi  <ryuan.choi@samsung.com>
> > +
> > +        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.
Comment 4 Ryuan Choi 2010-09-12 20:15:30 PDT
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.
Comment 5 Ryuan Choi 2010-09-12 23:01:40 PDT
Created attachment 67359 [details]
Patch
Comment 6 Ryuan Choi 2010-09-12 23:16:50 PDT
(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.
Comment 7 Rafael Antognolli 2010-09-13 06:38:48 PDT
(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.
Comment 8 Lucas De Marchi 2010-09-13 06:53:34 PDT
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.
Comment 9 Ryuan Choi 2010-09-13 08:01:16 PDT
(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.
Comment 10 Ryuan Choi 2010-09-13 08:10:30 PDT
Created attachment 67410 [details]
Patch
Comment 11 Ryuan Choi 2010-09-13 08:50:52 PDT
(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.
Comment 12 Rafael Antognolli 2010-09-13 10:04:57 PDT
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).
Comment 13 Antonio Gomes 2010-09-14 08:00:55 PDT
(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.
Comment 14 Ryuan Choi 2010-09-14 09:02:36 PDT
Created attachment 67560 [details]
patch with Rafael suggested
Comment 15 Ryuan Choi 2010-10-11 23:56:26 PDT
(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.
Comment 16 Eric Seidel (no email) 2010-10-14 10:59:42 PDT
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.
Comment 17 WebKit Commit Bot 2010-10-14 11:37:25 PDT
Comment on attachment 67560 [details]
patch with Rafael suggested

Clearing flags on attachment: 67560

Committed r69783: <http://trac.webkit.org/changeset/69783>
Comment 18 WebKit Commit Bot 2010-10-14 11:37:30 PDT
All reviewed patches have been landed.  Closing bug.