Bug 41149 – [EFL] Implement downloadURL in ContextMenuClientEfl

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Gyuyoung Kim]

I implement the downloadURL method in ContextMenuClient as well as FrameLoaderClient.

BTW, in ContextMenuClient, there is a style error as below,

../WebKit-EWebLauncher/WebKit/efl/WebCoreSupport/ContextMenuClientEfl.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 1 files

I fix it to avoid style error as well.

[Gyuyoung Kim]

Created attachment 59636 [details]
Patch

Patch.

[Lucas De Marchi]

Comment on attachment 59636 [details]
Patch

Just an informal review: looks good.

[Gyuyoung Kim]

Dear kenneth,

Please review this bug as well. 

Thank you,
Gyuyoung Kim

[Kenneth Rohde Christiansen]

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?

[Lucas De Marchi]

(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.

[Gyuyoung Kim]

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.

[Kenneth Rohde Christiansen]

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

[Gyuyoung Kim]

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);

[Kenneth Rohde Christiansen]

(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.

[Gyuyoung Kim]

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.

[Kenneth Rohde Christiansen]

Comment on attachment 60576 [details]
Patch

Yes, you got it

[WebKit Commit Bot]

Comment on attachment 60576 [details]
Patch

Clearing flags on attachment: 60576

Committed r62515: <http://trac.webkit.org/changeset/62515>

[Gyuyoung Kim]

Hello Leandro,

Could you please change status with Resolved Fixed ?

[Lucas De Marchi]

(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.

[Leandro Pereira]

(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.

[Leandro Pereira]

Comment on attachment 60574 [details]
Patch

Clearing obsolete patch flags.