Bug 107811 – [EFL][WK2] Use C API inside ewk_file_chooser_request

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Chris Dumez]

As per Bug 107657, we should start using the C API internally to avoid violating layering.

[Chris Dumez]

Created attachment 184475 [details]
Patch

[Chris Dumez]

Created attachment 185406 [details]
Patch

Rebased on master.

[Gyuyoung Kim]

Comment on attachment 185406 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_file_chooser_request.cpp:128
> +        String fileURL = ASCIILiteral("file://") + String::fromUTF8(url);

According to Benjamin's comment, it seems to me String Operators is enough in this case.
https://bugs.webkit.org/show_bug.cgi?id=107363#c13

[Chris Dumez]

(In reply to comment #3)
> (From update of attachment 185406 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185406&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_file_chooser_request.cpp:128
> > +        String fileURL = ASCIILiteral("file://") + String::fromUTF8(url);
> 
> According to Benjamin's comment, it seems to me String Operators is enough in this case.
> https://bugs.webkit.org/show_bug.cgi?id=107363#c13

Gyuyoung, I do not understand your comment. I'm using operator+ from String. Benjamin's comment is about not using StringBuilder (which I'm not using).

[Benjamin Poulain]

> > (From update of attachment 185406 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=185406&action=review
> > 
> > > Source/WebKit2/UIProcess/API/efl/ewk_file_chooser_request.cpp:128
> > > +        String fileURL = ASCIILiteral("file://") + String::fromUTF8(url);
> > 
> > According to Benjamin's comment, it seems to me String Operators is enough in this case.
> > https://bugs.webkit.org/show_bug.cgi?id=107363#c13
> 
> Gyuyoung, I do not understand your comment. I'm using operator+ from String. Benjamin's comment is about not using StringBuilder (which I'm not using).

Quick comment before going to bed :)

Gyuyoung is right
    String fileURL = ASCIILiteral("file://") + String::fromUTF8(url);
is worse than
    String fileURL = "file://" + String::fromUTF8(url);
but that's a bug and I am probably to blame for that.

There is no StringTypeAdapter for ASCIILiteral. So your code will generate:
   String fileURL = StringAppend(String(ASCIILiteral("file://")), String::fromUTF8(url));

If you have 5 minutes, it would be great to make another patch adding a StringTypeAdapter for ASCIILiteral.

[Chris Dumez]

(In reply to comment #5)
> > > (From update of attachment 185406 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=185406&action=review
> > > 
> > > > Source/WebKit2/UIProcess/API/efl/ewk_file_chooser_request.cpp:128
> > > > +        String fileURL = ASCIILiteral("file://") + String::fromUTF8(url);
> > > 
> > > According to Benjamin's comment, it seems to me String Operators is enough in this case.
> > > https://bugs.webkit.org/show_bug.cgi?id=107363#c13
> > 
> > Gyuyoung, I do not understand your comment. I'm using operator+ from String. Benjamin's comment is about not using StringBuilder (which I'm not using).
> 
> Quick comment before going to bed :)
> 
> Gyuyoung is right
>     String fileURL = ASCIILiteral("file://") + String::fromUTF8(url);
> is worse than
>     String fileURL = "file://" + String::fromUTF8(url);
> but that's a bug and I am probably to blame for that.
> 
> There is no StringTypeAdapter for ASCIILiteral. So your code will generate:
>    String fileURL = StringAppend(String(ASCIILiteral("file://")), String::fromUTF8(url));
> 
> If you have 5 minutes, it would be great to make another patch adding a StringTypeAdapter for ASCIILiteral.

Yes. It sounds like a good idea to add a StringTypeAdapter for ASCIILiteral. I'll do that, thanks.

[Chris Dumez]

Created attachment 185446 [details]
Patch

Do not construct a ASCIILiteral anymore.

[Gyuyoung Kim]

(In reply to comment #6)
> Yes. It sounds like a good idea to add a StringTypeAdapter for ASCIILiteral. I'll do that, thanks.

+1, Good idea to me as well.

[Chris Dumez]

Created attachment 185490 [details]
Patch

Use ASCIILiteral in String concatenations.

[Chris Dumez]

Created attachment 185491 [details]
Patch

Make use of toCopiedURLAPI().

[Build Bot]

Comment on attachment 185491 [details]
Patch

Attachment 185491 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16217923

[Chris Dumez]

Comment on attachment 185491 [details]
Patch

win-ews seems to be in a bad state. False alarm.

[Kenneth Rohde Christiansen]

Comment on attachment 185491 [details]
Patch

LGTM

[WebKit Review Bot]

Comment on attachment 185491 [details]
Patch

Clearing flags on attachment: 185491

Committed r141709: <http://trac.webkit.org/changeset/141709>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.