Summary: | [EFL] Add setting api for enabling encoding detector | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryuan Choi <ryuan.choi> | ||||||||||||
Component: | WebKit EFL | Assignee: | 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
Ryuan Choi
2010-09-08 17:45:13 PDT
Created attachment 66974 [details]
Patch
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. (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. 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.
Created attachment 67359 [details]
Patch
(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. (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 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. (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. Created attachment 67410 [details]
Patch
(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. 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). (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. Created attachment 67560 [details]
patch with Rafael suggested
(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 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 on attachment 67560 [details] patch with Rafael suggested Clearing flags on attachment: 67560 Committed r69783: <http://trac.webkit.org/changeset/69783> All reviewed patches have been landed. Closing bug. |