Summary: | [EFL] Implement downloadURL in ContextMenuClientEfl | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gyuyoung Kim <gyuyoung.kim> | ||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, hyuki.kim, joone.hur, kenneth, leandro, lucas.de.marchi | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | Linux | ||||||||||
Bug Depends on: | 40967 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Gyuyoung Kim
2010-06-24 04:29:26 PDT
Created attachment 59636 [details]
Patch
Patch.
Comment on attachment 59636 [details]
Patch
Just an informal review: looks good.
Dear kenneth, Please review this bug as well. Thank you, Gyuyoung Kim Comment on attachment 59636 [details]
Patch
WebKit/ChangeLog:7
+ to application.
That is one long bug title!
WebKit/efl/WebCoreSupport/ContextMenuClientEfl.cpp:72
+ download.url = url.string().utf8().data();
shouldn't you copy it?
(In reply to comment #4) > WebKit/efl/WebCoreSupport/ContextMenuClientEfl.cpp:72 > + download.url = url.string().utf8().data(); > shouldn't you copy it? No, but this is indeed wrong. Gyuyoung Kim, you should do as in FrameLoaderClientEfl::download(ResourceHandle*, const ResourceRequest& request, const ResourceRequest&, const ResourceResponse&). That is: CString url = request.url().prettyURL().utf8(); download.url = url.data(); This way you will keep the CString object in stack until ewk_view_download_request returns. Otherwise the CString object returned by utf8() will be destroyed as soon as its .data() returns. Created attachment 60574 [details]
Patch
Kenneth and Lucas, thank you for your review.
I didn't think about it. If the download url is not copied, the url can be removed. This is a bug.
I modify this patch according to your guidance.
Comment on attachment 60574 [details]
Patch
WebKit/efl/WebCoreSupport/ContextMenuClientEfl.h:46
+ virtual void downloadURL(const KURL& url);
don't add url here, according to style guide
Should I remove url argument in virtual void downloadURL(const KURL& url); ? I referenced to gtk / qt port. ContextMenuClientGtk.h => virtual void downloadURL(const WebCore::KURL& url); ContextMenuClientQt.h => virtual void downloadURL(const KURL& url); (In reply to comment #8) > Should I remove url argument in virtual void downloadURL(const KURL& url); ? > > I referenced to gtk / qt port. > > ContextMenuClientGtk.h > => virtual void downloadURL(const WebCore::KURL& url); > > ContextMenuClientQt.h > => virtual void downloadURL(const KURL& url); Don't trust old code :-) For method definitions you should leave our duplicate information. I already know it is an url as the type is KURL, same think with setText(const String&) is better than setText(const String& string) as string doesn't add additional info. On the other hand, if it is not duplicating info feel free to add it, like setLabel(const String& label, const String& tooltip). Got it? Qt specific code used to follow Qt style, but we will clean that up over time. Created attachment 60576 [details]
Patch
If parameter's type already means parameter's name, I shouldn't define parameter, right?
I keep in mind this guidance.
Thank you Keneeth.
Comment on attachment 60576 [details]
Patch
Yes, you got it
Comment on attachment 60576 [details] Patch Clearing flags on attachment: 60576 Committed r62515: <http://trac.webkit.org/changeset/62515> Hello Leandro, Could you please change status with Resolved Fixed ? (In reply to comment #13) > Hello Leandro, > > Could you please change status with Resolved Fixed ? This should be done automatically by Commit Bot. I don't know why it didn't happen. (In reply to comment #14) > (In reply to comment #13) > > Hello Leandro, > > > > Could you please change status with Resolved Fixed ? > > This should be done automatically by Commit Bot. I don't know why it didn't happen. Because there is still one non-obsolete bug. I'm clearing the flags and closing the bug in a minute. Comment on attachment 60574 [details]
Patch
Clearing obsolete patch flags.
|