Bug 86195 – [EFL][DRT] LayoutTestController does not implement clearApplicationCacheForOrigin

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Jussi Kukkonen (jku)]

EFL's LayoutTestController should implement clearApplicationCacheForOrigin so http/tests/appcache/origin-delete.html can be unskipped.

[Jussi Kukkonen (jku)]

Created attachment 141388 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 141388 [details]
Patch

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

I wonder if we should use the word "origin" for the ewk_settings function -- I would expect it to take a Ewk_Security_Origin, not a const char*.

> Source/WebKit/efl/ewk/ewk_settings.h:256
> + * Removes the HTML5 application cache for a security origin.

Nit: "remove" may be too strong a verb here; "clear" sounds better.

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:553
> +void LayoutTestController::clearApplicationCacheForOrigin(OpaqueJSString *origin)

Nit: wrong placement of the asterisk.

[Chris Dumez]

(In reply to comment #2)
> (From update of attachment 141388 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141388&action=review
> 
> I wonder if we should use the word "origin" for the ewk_settings function -- I would expect it to take a Ewk_Security_Origin, not a const char*.

He followed the style of an existing similar function:
void ewk_settings_local_storage_database_origin_clear(const char* url);

At least he is consistent.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 141388 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=141388&action=review
> > 
> > I wonder if we should use the word "origin" for the ewk_settings function -- I would expect it to take a Ewk_Security_Origin, not a const char*.
> 
> He followed the style of an existing similar function:
> void ewk_settings_local_storage_database_origin_clear(const char* url);
> 
> At least he is consistent.

That's right, but that function was introduced before tmpsantos committed his Ewk_Security_Origin stuff :-)

Wouldn't it make sense to move this new one into ewk_security_origin.cpp itself?

[Jussi Kukkonen (jku)]

(In reply to comment #2)
> (From update of attachment 141388 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141388&action=review
> 
> I wonder if we should use the word "origin" for the ewk_settings function -- I would expect it to take a Ewk_Security_Origin, not a const char*.

Ah, sorry for not bringing this up when sending the patch... I initially put the code in ewk_security_origin.cpp to accompany ewk_security_origin_application_cache_quota_set()) but the reality is that the caller would use the string to build a Ewk_Security_Origin only for E_S_O to then retrieve the original string from itself... I also found ewk_settings_local_storage_database_origin_clear() which did take a const char* so went with that approach.

Unless I hear otherwise, I'll move the function back to ewk_security_origin.cpp based on your comments so far.

> > Source/WebKit/efl/ewk/ewk_settings.h:256
> > + * Removes the HTML5 application cache for a security origin.
> 
> Nit: "remove" may be too strong a verb here; "clear" sounds better.
> 
> > Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:553
> > +void LayoutTestController::clearApplicationCacheForOrigin(OpaqueJSString *origin)
> 
> Nit: wrong placement of the asterisk.

Thanks, will fix these.

[Jussi Kukkonen (jku)]

Created attachment 141759 [details]
Patch

[Jussi Kukkonen (jku)]

Note that this still depends on chris' patch from bug 85585. Feel free to remove review flag here if it isn't appropriate until the blocker lands.


After talking with tmpsantos this is what I ended up adding:

 void ewk_security_origin_application_cache_clear
     (const Ewk_Security_Origin *origin)
 Ewk_Security_Origin *ewk_security_origin_new_from_string
     (const char *url)

The latter because it wasn't possible to create a new E_S_O otherwise. I think the end result still looks cleaner: E_S_O is a better place for this.

[Jussi Kukkonen (jku)]

Created attachment 142311 [details]
Patch

Rebase patch now that blocker landed

[Chris Dumez]

Comment on attachment 142311 [details]
Patch

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

> Source/WebKit/efl/ewk/ewk_security_origin.cpp:105
> +    return ewk_security_origin_new(WebCore::SecurityOrigin::createFromString(String (url)).get());

extra space here QString(url). Also you should probably use QString::fromUTF8(url) instead.

> Tools/DumpRenderTree/efl/LayoutTestControllerEfl.cpp:553
> +void LayoutTestController::clearApplicationCacheForOrigin(OpaqueJSString* origin)

Calling it "url" instead of "origin" would be less confusing.

[Gyuyoung Kim]

Comment on attachment 142311 [details]
Patch

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

>> Source/WebKit/efl/ewk/ewk_security_origin.cpp:105
>> +    return ewk_security_origin_new(WebCore::SecurityOrigin::createFromString(String (url)).get());
> 
> extra space here QString(url). Also you should probably use QString::fromUTF8(url) instead.

QString is for QT port. We just enough to use String::fromUTF8().

> Source/WebKit/efl/ewk/ewk_security_origin.h:99
> +EAPI uint64_t             ewk_security_origin_web_database_quota_get(const Ewk_Security_Origin *o);

As far as I know, webkit reviewer don't like to mix patch. You don't mention to fix indentation. I would recommend to file new bug to fix indentation. I think indentation patch is able to be landed without review.

[Chris Dumez]

Comment on attachment 142311 [details]
Patch

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

>>> Source/WebKit/efl/ewk/ewk_security_origin.cpp:105
>>> +    return ewk_security_origin_new(WebCore::SecurityOrigin::createFromString(String (url)).get());
>> 
>> extra space here QString(url). Also you should probably use QString::fromUTF8(url) instead.
> 
> QString is for QT port. We just enough to use String::fromUTF8().

Sorry for the typo, I obviously meant String::fromUTF8().

[Jussi Kukkonen (jku)]

(In reply to comment #10)

Thanks for review Chris & Gyuyoung Kim. I'll fix the string issues.

> > Source/WebKit/efl/ewk/ewk_security_origin.h:99
> > +EAPI uint64_t             ewk_security_origin_web_database_quota_get(const Ewk_Security_Origin *o);
> 
> As far as I know, webkit reviewer don't like to mix patch. You don't mention to fix indentation. I would recommend to file new bug to fix indentation. I think indentation patch is able to be landed without review.

This was not just a random indentation fix: the indentation was fine until I added a function with a long return signature. I can separate the re-indent of course but it seems a little odd to first commit code that makes other code wrongly indented and then fix the indent in another commit...

[Jussi Kukkonen (jku)]

Created attachment 142969 [details]
Patch

[Jussi Kukkonen (jku)]

(In reply to comment #13)
> Created an attachment (id=142969) [details]

Fixes the string issue, adds a changelog comment about keeping indentation correct and rebases on master.

Let me know if you think the indent work really should go in a separate patch and I will do it.

[Jussi Kukkonen (jku)]

Add some reviewer CCs from 'webkit-patch suggest-reviewers'

[Gyuyoung Kim]

Comment on attachment 142969 [details]
Patch

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

If reviewer don't mind to fix indentation as well, looks good to me except for trivial style nit.

> Source/WebKit/efl/ewk/ewk_security_origin.cpp:93
> +void ewk_security_origin_application_cache_clear(const Ewk_Security_Origin *origin)

Style nit : Move '*' to data type side.

[Jussi Kukkonen (jku)]

Created attachment 145055 [details]
Fix style nit from comment 16

[Gustavo Noronha (kov)]

Comment on attachment 145055 [details]
Fix style nit from comment 16

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

> Source/WebKit/efl/ewk/ewk_security_origin.h:54
> -EAPI const char  *ewk_security_origin_protocol_get(Ewk_Security_Origin *o);
> +EAPI const char          *ewk_security_origin_protocol_get(Ewk_Security_Origin *o);

Fixing misc style issues along with functionality addition may cause some trouble with bisecting, so I'd try to avoid it.

[Gyuyoung Kim]

(In reply to comment #18)
> (From update of attachment 145055 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145055&action=review
> 
> > Source/WebKit/efl/ewk/ewk_security_origin.h:54
> > -EAPI const char  *ewk_security_origin_protocol_get(Ewk_Security_Origin *o);
> > +EAPI const char          *ewk_security_origin_protocol_get(Ewk_Security_Origin *o);
> 
> Fixing misc style issues along with functionality addition may cause some trouble with bisecting, so I'd try to avoid it.

Yes, I also don't like to mix functionality with style nits. I will have a care further when I review patch informally.

[WebKit Review Bot]

Comment on attachment 145055 [details]
Fix style nit from comment 16

Clearing flags on attachment: 145055

Committed r119115: <http://trac.webkit.org/changeset/119115>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.