Bug 86195 - [EFL][DRT] LayoutTestController does not implement clearApplicationCacheForOrigin
Summary: [EFL][DRT] LayoutTestController does not implement clearApplicationCacheForOr...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 85585
Blocks: 86460
  Show dependency treegraph
 
Reported: 2012-05-11 04:52 PDT by Jussi Kukkonen (jku)
Modified: 2012-05-31 10:00 PDT (History)
11 users (show)

See Also:


Attachments
Patch (5.70 KB, patch)
2012-05-11 05:44 PDT, Jussi Kukkonen (jku)
no flags Details | Formatted Diff | Diff
Patch (9.23 KB, patch)
2012-05-14 11:22 PDT, Jussi Kukkonen (jku)
no flags Details | Formatted Diff | Diff
Patch (9.34 KB, patch)
2012-05-16 11:40 PDT, Jussi Kukkonen (jku)
no flags Details | Formatted Diff | Diff
Patch (9.71 KB, patch)
2012-05-21 01:49 PDT, Jussi Kukkonen (jku)
no flags Details | Formatted Diff | Diff
Fix style nit from comment 16 (9.75 KB, patch)
2012-05-31 05:20 PDT, Jussi Kukkonen (jku)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jussi Kukkonen (jku) 2012-05-11 04:52:06 PDT
EFL's LayoutTestController should implement clearApplicationCacheForOrigin so http/tests/appcache/origin-delete.html can be unskipped.
Comment 1 Jussi Kukkonen (jku) 2012-05-11 05:44:32 PDT
Created attachment 141388 [details]
Patch
Comment 2 Raphael Kubo da Costa (:rakuco) 2012-05-11 06:58:34 PDT
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.
Comment 3 Chris Dumez 2012-05-11 07:01:36 PDT
(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.
Comment 4 Raphael Kubo da Costa (:rakuco) 2012-05-11 07:20:51 PDT
(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?
Comment 5 Jussi Kukkonen (jku) 2012-05-14 01:04:46 PDT
(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.
Comment 6 Jussi Kukkonen (jku) 2012-05-14 11:22:49 PDT
Created attachment 141759 [details]
Patch
Comment 7 Jussi Kukkonen (jku) 2012-05-14 11:25:57 PDT
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.
Comment 8 Jussi Kukkonen (jku) 2012-05-16 11:40:50 PDT
Created attachment 142311 [details]
Patch

Rebase patch now that blocker landed
Comment 9 Chris Dumez 2012-05-16 11:59:52 PDT
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.
Comment 10 Gyuyoung Kim 2012-05-16 18:05:43 PDT
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.
Comment 11 Chris Dumez 2012-05-17 00:07:10 PDT
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().
Comment 12 Jussi Kukkonen (jku) 2012-05-21 00:27:14 PDT
(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...
Comment 13 Jussi Kukkonen (jku) 2012-05-21 01:49:14 PDT
Created attachment 142969 [details]
Patch
Comment 14 Jussi Kukkonen (jku) 2012-05-21 01:51:51 PDT
(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.
Comment 15 Jussi Kukkonen (jku) 2012-05-29 02:33:08 PDT
Add some reviewer CCs from 'webkit-patch suggest-reviewers'
Comment 16 Gyuyoung Kim 2012-05-29 02:45:27 PDT
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.
Comment 17 Jussi Kukkonen (jku) 2012-05-31 05:20:21 PDT
Created attachment 145055 [details]
Fix style nit from comment 16
Comment 18 Gustavo Noronha (kov) 2012-05-31 06:00:55 PDT
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.
Comment 19 Gyuyoung Kim 2012-05-31 06:55:21 PDT
(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.
Comment 20 WebKit Review Bot 2012-05-31 10:00:19 PDT
Comment on attachment 145055 [details]
Fix style nit from comment 16

Clearing flags on attachment: 145055

Committed r119115: <http://trac.webkit.org/changeset/119115>
Comment 21 WebKit Review Bot 2012-05-31 10:00:26 PDT
All reviewed patches have been landed.  Closing bug.