Bug 44256 – [EFL] Private Browsing feature for WebKit-Efl

• Component: WebKit EFL
• Status: RESOLVED INVALID

[Grzegorz Czajkowski]

This feature implement private browsing feature for WebKit-Efl. 

It allows to disable cache during private browsing mode. Feature creates new cookie jar for cookies when user turns on private browsing mode. When user turns off it default cookie jar is restored.

[Grzegorz Czajkowski]

Created attachment 64841 [details]
Private browsing feature for WebKit-Efl

[Gyuyoung Kim]

It seems to me that methods regarding cookie need to be moved to ewk_cookies.cpp.

Hello Antognolli,

How do you think about it ?

[Gyuyoung Kim]

Hello Grzegorz,

Could you please add "[EFL]" prefix to the title of this bug ?

[Rafael Antognolli]

Hi Grzegorz, just some comments about your patch:

First, I agree with Gyuyoung, I'd put the cookies part inside ewk_cookies.cpp. Another comment:

+Eina_Bool _ewk_replace_cookie_jar(Eina_Bool enable)
+{
+#if USE(SOUP)
+    static Eina_Bool current_status_of_pb = EINA_FALSE;
+
+    if (current_status_of_pb == enable)
+        return EINA_FALSE;

What does it mean if you return false here? I think in cases where the setting you are trying to put is valid, but is already set, you should don't do nothing and just return true, instead of false. If you want a different behavior, document it on the doxygen string of the function, since this is the default one for most of our functions.

And I also don't like the idea of these static variables inside the function. They seem a little lost inside it. A better approach would be to add it to a struct somewhere.

Another question is: does this conflict with the bug 44239 ? I think if you are in private browsing mode, maybe you shouldn't store any page for offline use. What do you think about this?

Regards,
Rafael

[Adam Barth]

Comment on attachment 64841 [details]
Private browsing feature for WebKit-Efl

Looks reasonable.  This review is more of a rubber-stamp.

> WebKit/efl/ChangeLog:7
> +        [Efl] Private browsing feature.
> +        https://bugs.webkit.org/show_bug.cgi?id=44256
> +
Would be nice to have a high-level summary here...

[Adam Barth]

Comment on attachment 64841 [details]
Private browsing feature for WebKit-Efl

Actually, it's probably better to answer Rafael's questions before landing this patch.

[Grzegorz Czajkowski]

Created attachment 73065 [details]
Incognito feature for WebKit-Efl

[Grzegorz Czajkowski]

1) I moved a function _ewk_cookie_jar_replace to ewk_cookies.h

2) I changed the name of feature to Incognito Browsing Feature because WebKit already has private browsing for the Pages.

3) I changed condition - if we alredy are in incognito mode we return EINA_TRUE.

4) Needed information regarding to cookies are stored in Ewk_Incognito_Cookies struct. Definition of this struct is puted to ewk_cookies.cpp. I think it's not necessary to include it to ewk_cookies.h because only _ewk_jookie_jar_replace uses it.

5) Static variables are moved to _ewk_incognito_cookie_get() - they are needed in this case - I think.

6) Added docs.

7) Changelog has high-level summary.

I would like to know your opinion on that.
Thanks.

[Grzegorz Czajkowski]

1) I moved a function _ewk_cookie_jar_replace to ewk_cookies.h

2) I changed the name of feature to Incognito Browsing Feature because WebKit already has private browsing for the Pages.

3) I changed condition - if we alredy are in incognito mode we return EINA_TRUE.

4) Needed information regarding to cookies are stored in Ewk_Incognito_Cookies struct. Definition of this struct is puted to ewk_cookies.cpp, only _ewk_jookie_jar_replace uses it.

5) A new function _ewk_incognito_cookies_get() has been added.

5) Static variables are moved to _ewk_incognito_cookie_get() - they are needed in this case - I think.

6) Added docs.

7) Changelog has high-level summary.

I would like to know your opinion on that.
Thanks.

[Grzegorz Czajkowski]

Created attachment 73066 [details]
Incognito feature for WebKit-Efl

[WebKit Review Bot]

Comment on attachment 73066 [details]
Incognito feature for WebKit-Efl

Rejecting patch 73066 from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[WebKit Review Bot]

Comment on attachment 73066 [details]
Incognito feature for WebKit-Efl

Rejecting patch 73066 from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[WebKit Review Bot]

Comment on attachment 73066 [details]
Incognito feature for WebKit-Efl

Rejecting patch 73066 from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Grzegorz Czajkowski]

Created attachment 73067 [details]
Incognito feature for WebKit-Efl

[Grzegorz Czajkowski]

Created attachment 73068 [details]
Incognito feature for WebKit-Efl

[Grzegorz Czajkowski]

Created attachment 73069 [details]
Incognito feature for WebKit-Efl

[WebKit Review Bot]

Attachment 73068 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebKit/efl/ChangeLog', u'WebKit/efl/ewk/ewk_cookies.cpp', u'WebKit/efl/ewk/ewk_cookies.h', u'WebKit/efl/ewk/ewk_main.cpp', u'WebKit/efl/ewk/ewk_main.h', u'WebKit/efl/ewk/ewk_view.cpp', u'WebKit/efl/ewk/ewk_view.h']" exit_code: 1
WebKit/efl/ewk/ewk_main.cpp:25:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 73069 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebKit/efl/ChangeLog', u'WebKit/efl/ewk/ewk_cookies.cpp', u'WebKit/efl/ewk/ewk_cookies.h', u'WebKit/efl/ewk/ewk_main.cpp', u'WebKit/efl/ewk/ewk_main.h', u'WebKit/efl/ewk/ewk_view.cpp', u'WebKit/efl/ewk/ewk_view.h']" exit_code: 1
WebKit/efl/ewk/ewk_main.cpp:25:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Grzegorz Czajkowski]

Created attachment 73220 [details]
Incognito feature for WebKit-Efl

Alphabetical sorting problem has been resolved.
I am sorry for the previous comments :)

[Rafael Antognolli]

(In reply to comment #19)
> Created an attachment (id=73220) [details]
> Incognito feature for WebKit-Efl
> 
> Alphabetical sorting problem has been resolved.
> I am sorry for the previous comments :)

About the last patch:

+        Saves cookies to a new cookie jar, disables cache of WebCore and doesn't strore any pages for offline use.

Please, fix the typo ("strore").

+ * @param enable if @c EINA_TRUE creates a new cookie jar (all cookies will be saved to this one), c@ EINA_FALSE restores default cookie jar.

Another typo ("c@" instead of "@c").

+ * @return @ EINA_TRUE if cookie jar has been replaced/restored. @c EINA_FALSE otherwise or other error occured.

Another typo ("@" between return and EINA_TRUE shouldn't exist).

Other comments:

Could you change the name _ewk_cookie_jar_replace() to _ewk_cookie_jar_replace_set()? Or even something like _ewk_cookie_jar_fake_set(), or _ewk_cookie_jar_tmp_set()? Maybe it's just my opinion, but I believe that this is a little more descriptive...

I also believe that a better place for the function _ewk_cache_disable would be inside ewk_settings.cpp. What do you think about it?

Regards,

Rafael

[Grzegorz Czajkowski]

Created attachment 73368 [details]
Incognito feature for WebKit-Efl

Rafael's suggestions were applied

[Grzegorz Czajkowski]

(In reply to comment #20)
> (In reply to comment #19)
> > Created an attachment (id=73220) [details] [details]
> > Incognito feature for WebKit-Efl
> > 
> > Alphabetical sorting problem has been resolved.
> > I am sorry for the previous comments :)
> 
> About the last patch:
> 
> +        Saves cookies to a new cookie jar, disables cache of WebCore and doesn't strore any pages for offline use.
> 
> Please, fix the typo ("strore").

Fixed. I am sorry for the misprints.

> 
> + * @param enable if @c EINA_TRUE creates a new cookie jar (all cookies will be saved to this one), c@ EINA_FALSE restores default cookie jar.
> 
> Another typo ("c@" instead of "@c").

Fixed

> 
> + * @return @ EINA_TRUE if cookie jar has been replaced/restored. @c EINA_FALSE otherwise or other error occured.
> 
> Another typo ("@" between return and EINA_TRUE shouldn't exist).

Fixed

> 
> Other comments:
> 
> Could you change the name _ewk_cookie_jar_replace() to _ewk_cookie_jar_replace_set()? Or even something like _ewk_cookie_jar_fake_set(), or _ewk_cookie_jar_tmp_set()? Maybe it's just my opinion, but I believe that this is a little more descriptive...
> 

That's a good idea. I changed it to _ewk_cookie_jar_fake_set().

> I also believe that a better place for the function _ewk_cache_disable would be inside ewk_settings.cpp. What do you think about it?
> 

Ok. I moved it and changed name to ewk_setting_disable_cache_set and exported it to public API.

>
> Regards,
> 
> Rafael

I added comments to functions _ewk_incognito_cookie_get and _ewk_cookie_jar_fake_set that they aren't exported to public API.

I've fixed the build break because a file Cache.h has been renamed to MemoryCache.h.

I did a small changes in ewk_setting.cpp regarding to sorting problems of includes files. I hope that you don't mind on that :)

[Rafael Antognolli]

Sure, the patch looks good to me now :-)

[Grzegorz Czajkowski]

Is there any chance to land this patch into WebKit? :)

[WebKit Commit Bot]

Comment on attachment 73368 [details]
Incognito feature for WebKit-Efl

Rejecting attachment 73368 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-attachment', '--no-update', '--non-interactive', 73368]" exit_code: 2
Last 500 characters of output:
gs.cpp
Hunk #1 FAILED at 29.
Hunk #2 FAILED at 261.
2 out of 2 hunks FAILED -- saving rejects to file WebKit/efl/ewk/ewk_settings.cpp.rej
patching file WebKit/efl/ewk/ewk_settings.h
patching file WebKit/efl/ewk/ewk_view.cpp
Hunk #1 succeeded at 4516 (offset 162 lines).
patching file WebKit/efl/ewk/ewk_view.h
Hunk #1 succeeded at 535 (offset 34 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Kenneth Rohde Christiansen', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/7277162

[Grzegorz Czajkowski]

Created attachment 77985 [details]
Incognito feature for WebKit-Efl

Patch fixes merge conflict.

[WebKit Review Bot]

Attachment 77985 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebKit/efl/ChangeLog', u'WebKit/efl/ewk/ewk_cookies.cpp', u'WebKit/efl/ewk/ewk_cookies.h', u'WebKit/efl/ewk/ewk_settings.cpp', u'WebKit/efl/ewk/ewk_settings.h', u'WebKit/efl/ewk/ewk_view.cpp', u'WebKit/efl/ewk/ewk_view.h']" exit_code: 1
WebKit/efl/ewk/ewk_view.h:538:  The parameter name "o" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Lucas De Marchi]

Comment on attachment 77985 [details]
Incognito feature for WebKit-Efl

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

> WebKit/efl/ewk/ewk_settings.cpp:300
> + * Disables/enables memory cache of WebCore.
> + *
> + * @param @c EINA_TRUE if we want to disable WebCore's cache, @c EINA_FALSE if we want to enable.
> + * @return @c EINA_TRUE if cache has been disabled/enabled, @c EINA_FALSE if function can't disable/enable cache.
> + */
> +Eina_Bool ewk_settings_disable_cache_set(Eina_Bool set)

This name is really weird because there are 2 verbs "disable" and "set". Could you rename it to ewk_settings_cache_disabled_set?

> WebKit/efl/ewk/ewk_settings.cpp:307
> +    if (cache->disabled() != set) 

Since this is EAPI, you can't really trust user passed EINA_TRUE or EINA_FALSE. In some places we use to use "set = !!set;". Please either do this or check by "!set"

> WebKit/efl/ewk/ewk_view.h:538
> +EAPI Eina_Bool ewk_view_incognito_browsing_set(Evas_Object *o, Eina_Bool enable);

Follow the other cases on this header: it's Evas_Object* o, not Evas_Object *o

[Grzegorz Czajkowski]

Ok, I will fix this.
What about declaration of _ewk_cookie_jar_fake_set. This is internal method. Does it should be moved to ewk_private.h? If yes I suggest to remove prefix "_".

[Lucas De Marchi]

(In reply to comment #29)
> Ok, I will fix this.
> What about declaration of _ewk_cookie_jar_fake_set. This is internal method. Does it should be moved to ewk_private.h? If yes I suggest to remove prefix "_".

HUmn... you're right. Moreover, make _ewk_incognito_cookie_get() static.

[Grzegorz Czajkowski]

Created attachment 78384 [details]
Incognito feature for WebKit-Efl

[WebKit Review Bot]

Attachment 78384 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebKit/efl/ChangeLog', u'WebKit/efl/ewk/ewk_cookies.cpp', u'WebKit/efl/ewk/ewk_private.h', u'WebKit/efl/ewk/ewk_settings.cpp', u'WebKit/efl/ewk/ewk_settings.h', u'WebKit/efl/ewk/ewk_view.cpp', u'WebKit/efl/ewk/ewk_view.h']" exit_code: 1
WebKit/efl/ewk/ewk_view.h:538:  The parameter name "o" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Grzegorz Czajkowski]

(In reply to comment #28)
> (From update of attachment 77985 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77985&action=review
> 

> > +Eina_Bool ewk_settings_disable_cache_set(Eina_Bool set)
> 
> This name is really weird because there are 2 verbs "disable" and "set". Could you rename it to ewk_settings_cache_disabled_set?

renamed

> 
> > WebKit/efl/ewk/ewk_settings.cpp:307
> > +    if (cache->disabled() != set) 
> 
> Since this is EAPI, you can't really trust user passed EINA_TRUE or EINA_FALSE. In some places we use to use "set = !!set;". Please either do this or check by "!set"
> 

added set = !!set;

> > WebKit/efl/ewk/ewk_view.h:538
> > +EAPI Eina_Bool ewk_view_incognito_browsing_set(Evas_Object *o, Eina_Bool enable);
> 
> Follow the other cases on this header: it's Evas_Object* o, not Evas_Object *o

fixed

[Eric Seidel (no email)]

Comment on attachment 73368 [details]
Incognito feature for WebKit-Efl

Cleared Kenneth Rohde Christiansen's review+ from obsolete attachment 73368 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Grzegorz Czajkowski]

Created attachment 83806 [details]
Incognito browsing feature for WebKit-EFL

[Gyuyoung Kim]

Comment on attachment 83806 [details]
Incognito browsing feature for WebKit-EFL

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

> Source/WebKit/efl/ewk/ewk_cookies.cpp:40
> +#ifdef WTF_USE_SOUP

Please use #if USE(SOUP)

> Source/WebKit/efl/ewk/ewk_cookies.cpp:251
> +#ifdef WTF_USE_SOUP

Please use #if USE(SOUP) instead of #ifdef WTF_USE_SOUP

[Grzegorz Czajkowski]

Created attachment 84025 [details]
Incognito browsing feature for WebKit-EFL

[Grzegorz Czajkowski]

Created attachment 84026 [details]
Incognito browsing feature for WebKit-EFL

[Grzegorz Czajkowski]

(In reply to comment #36)
> (From update of attachment 83806 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83806&action=review
> 
> > Source/WebKit/efl/ewk/ewk_cookies.cpp:40
> > +#ifdef WTF_USE_SOUP
> 
> Please use #if USE(SOUP)

fixed

> 
> > Source/WebKit/efl/ewk/ewk_cookies.cpp:251
> > +#ifdef WTF_USE_SOUP
> 
> Please use #if USE(SOUP) instead of #ifdef WTF_USE_SOUP

fixed

[Gyuyoung Kim]

LGTM.

[Grzegorz Czajkowski]

(In reply to comment #40)
> LGTM.

Is any chance to add this to webkit?, The patch hangs a lot :)
Thanks

[Gyuyoung Kim]

(In reply to comment #41)
> (In reply to comment #40)
> > LGTM.
> 
> Is any chance to add this to webkit?, The patch hangs a lot :)
> Thanks

Yes, I know well. Some our patches are being requested to review. Yesterday, I requested reviewer that our patches need to be reviewed.  But, there is no response yet.

[Gyuyoung Kim]

Comment on attachment 84026 [details]
Incognito browsing feature for WebKit-EFL

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

> Source/WebKit/efl/ewk/ewk_cookies.cpp:266
> +    return EINA_TRUE;

Why should we return true unconditionally ?

> Source/WebKit/efl/ewk/ewk_cookies.cpp:268
> +    return EINA_FALSE;

Please change EINA_FALSE with  EINA_SAFETY_ON_TRUE_RETURN(1);

> Source/WebKit/efl/ewk/ewk_settings.cpp:341
> +    return EINA_TRUE;

This is same.

[Gyuyoung Kim]

Grzegorz, when do you upload new patch ?

[Daniel Bates]

Comment on attachment 84026 [details]
Incognito browsing feature for WebKit-EFL

r-'ing since some questions were raised by Gyuyoung Kim in comment 43.

[Antonio Gomes]

Comment on attachment 84026 [details]
Incognito browsing feature for WebKit-EFL

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

Please reply to Gyuyoung's questions, and mine as well. A few style issues to be fixed, and it does not apply to ToT anymore (rebase needed).

> Source/WebKit/efl/ewk/ewk_settings.cpp:332
> +    WebCore::MemoryCache *cache = WebCore::memoryCache();

"*" is on the left side.

> Source/WebKit/efl/ewk/ewk_settings.h:54
> +EAPI Eina_Bool       ewk_settings_cache_disable_set(Eina_Bool set);

Is it common to have "disable_set" APIs instead of "enable_set"?

> Source/WebKit/efl/ewk/ewk_view.cpp:4439
> +    return (result_cookies && result_cache && result_offline_pages);

no parentheses needed.

[Gyuyoung Kim]

Comment on attachment 84026 [details]
Incognito browsing feature for WebKit-EFL

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

> Source/WebKit/efl/ewk/ewk_settings.cpp:330
> +Eina_Bool ewk_settings_cache_disable_set(Eina_Bool set)

ewk_settings.cpp already has ewk_settings_cache_enable_set(). You need to remove this function from this patch.

ewk_settings.cpp.
355 void ewk_settings_cache_enable_set(Eina_Bool set)
356 {
357     WebCore::MemoryCache* cache = WebCore::memoryCache();
358     set = !set;
359     if (cache->disabled() != set)
360         cache->setDisabled(set);
361 }
362 

Grzegorz,
Are you preparing new patch for this bug ?

[Grzegorz Czajkowski]

(In reply to comment #47)
> (From update of attachment 84026 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84026&action=review
> 
> > Source/WebKit/efl/ewk/ewk_settings.cpp:330
> > +Eina_Bool ewk_settings_cache_disable_set(Eina_Bool set)
> 
> ewk_settings.cpp already has ewk_settings_cache_enable_set(). You need to remove this function from this patch.

Yes, I've already moved this function because it can be used in general mode too.

> 
> ewk_settings.cpp.
> 355 void ewk_settings_cache_enable_set(Eina_Bool set)
> 356 {
> 357     WebCore::MemoryCache* cache = WebCore::memoryCache();
> 358     set = !set;
> 359     if (cache->disabled() != set)
> 360         cache->setDisabled(set);
> 361 }
> 362 
> 
> Grzegorz,
> Are you preparing new patch for this bug ?

I don't think that patch is needed since WebKit's private browsing has been extended a lot.
One method that is worth to submit is:
ewk_cookie_jar_fake_set which replaces/restores default cookie jar of Soup session.
Do we need this? What do you think?

[Gyuyoung Kim]

> I don't think that patch is needed since WebKit's private browsing has been extended a lot.
> One method that is worth to submit is:
> ewk_cookie_jar_fake_set which replaces/restores default cookie jar of Soup session.
> Do we need this? What do you think?

Could you explain worth of the function ?

[Gyuyoung Kim]

(In reply to comment #49)
> > I don't think that patch is needed since WebKit's private browsing has been extended a lot.
> > One method that is worth to submit is:
> > ewk_cookie_jar_fake_set which replaces/restores default cookie jar of Soup session.
> > Do we need this? What do you think?
> 
> Could you explain worth of the function ?

Grzegorz, if you think we need to have the "ewk_cookie_jar_fake_set()", please file a new bug. This bug is too old. I close this bug as invalid.