Summary: | [EFL][WK2] Use C API inside ewk_file_chooser_request | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||
Component: | WebKit EFL | Assignee: | Chris Dumez <cdumez> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | andersca, ap, benjamin, gyuyoung.kim, kenneth, kling, lucas.de.marchi, mikhail.pozdnyakov, naginenis, rakuco, sam, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 107657 | ||||||||||||||
Attachments: |
|
Description
Chris Dumez
2013-01-24 05:09:54 PST
Created attachment 184475 [details]
Patch
Created attachment 185406 [details]
Patch
Rebased on master.
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 (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). > > (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.
(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. Created attachment 185446 [details]
Patch
Do not construct a ASCIILiteral anymore.
(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. Created attachment 185490 [details]
Patch
Use ASCIILiteral in String concatenations.
Created attachment 185491 [details]
Patch
Make use of toCopiedURLAPI().
Comment on attachment 185491 [details] Patch Attachment 185491 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16217923 Comment on attachment 185491 [details]
Patch
win-ews seems to be in a bad state. False alarm.
Comment on attachment 185491 [details]
Patch
LGTM
Comment on attachment 185491 [details] Patch Clearing flags on attachment: 185491 Committed r141709: <http://trac.webkit.org/changeset/141709> All reviewed patches have been landed. Closing bug. |