| Summary: | [EFL][WK2] Crash when "Download Linked File" from MiniBrowser context menu is clicked | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Maciej Florek <m.florek> | ||||||||||
| Component: | WebKit EFL | Assignee: | Nobody <webkit-unassigned> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | commit-queue, g.czajkowski, gyuyoung.kim, hh.kaka, l.bialek, lucas.de.marchi, ossy, ryuan.choi | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Maciej Florek
2014-04-03 06:53:15 PDT
Created attachment 246314 [details]
Patch
Comment on attachment 246314 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246314&action=review I like the idea of detaching DownloadManagerEFL from EwkView! Generally looks promising, added few minor comments. I'd like to listen Gyuyoung, Ryuan comments on that. This patch lacks of API tests but I believe they could be done in a separate patch. > Source/WebKit2/UIProcess/API/efl/ewk_download_job.h:155 > + * will be called on the Ewk_Download_Job. Ewk_Download_Job ? I think you meant "Ewk_Context" > Source/WebKit2/UIProcess/efl/DownloadManagerEfl.cpp:138 > + clientCallbacks.m_requested = nullptr; > + clientCallbacks.m_failed = nullptr; > + clientCallbacks.m_cancelled = nullptr; > + clientCallbacks.m_finished = nullptr; > + clientCallbacks.m_userData = nullptr; Please define ClientDownloadCallbacks' constructor or use memset(&clientCallbacks, 0, sizeof(ClientDownloadCallbacks)) for that. > Source/WebKit2/UIProcess/efl/DownloadManagerEfl.h:37 > +struct ClientDownloadCallbacks { Since we don't expose those callbacks outside DownloadManagerEfl.h/cpp I'd prefer to move the declaration to the cpp file and use forward declaration (struct ClientDownloadsCallbacks;) > Source/WebKit2/UIProcess/efl/DownloadManagerEfl.h:51 > + void setCallbacks(Ewk_Download_Requested_Cb, Ewk_Download_Failed_Cb, Ewk_Download_Cancelled_Cb, Ewk_Download_Finished_Cb, void* userData); setClientCallbacks() ? > Source/WebKit2/UIProcess/efl/DownloadManagerEfl.h:69 > + ClientDownloadCallbacks clientCallbacks; m_clientCallbacks as it's class member. > Tools/ChangeLog:13 > + Adapt download caalbcks to new callback mechanism typo: caalbcks -> callbacks > Tools/MiniBrowser/efl/main.c:850 > +on_download_failed(Ewk_Download_Job_Error *downloadError, void *user_data) MiniBrowser follows C coding style (downloadError -> download_error). > Tools/MiniBrowser/efl/main.c:852 > + const Ewk_Error *error = downloadError->error; I don't think "const" modifier brings benefits here. > Tools/MiniBrowser/efl/main.c:2302 > + // Set callbacks for download events. > + ewk_context_download_callbacks_set(context, on_download_request, on_download_failed, 0, on_download_finished, window); I'd rather move them to context related code in elm_main(), there we already have ewk_context_process_model_set(context, EWK_PROCESS_MODEL_MULTIPLE_SECONDARY); ewk_context_favicon_database_directory_set(context, NULL); Comment on attachment 246314 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246314&action=review Thanks for the patch. I leave some comments. > Source/WebKit2/ChangeLog:15 > + Propose a new callbacks which are view independent and detach EFL's DownloadMaager > + from EwkView. It makes DownloadManagerEFL consistent with WKContextDownload. > + Fix the crash and restore download functionality. I am not sure. Doesn't we have any use cases which need to distinguish which ewk_view(tab) requests this download? > Source/WebKit2/ChangeLog:32 > + Old tests do not match new feature. Separate bug will be introduced. Bug ID? > Source/WebKit2/UIProcess/API/efl/ewk_context.h:374 > +EAPI void ewk_context_download_callbacks_set(Ewk_Context* context, > + Ewk_Download_Requested_Cb download_requested_func, > + Ewk_Download_Failed_Cb download_failed_func, > + Ewk_Download_Cancelled_Cb download_cancelled_func, > + Ewk_Download_Finished_Cb download_finished_func, > + void* data); EFL looks prefer the spaces to align first letter of first parameter with others if we define API using multiple line. For example, in Ecore_File.h EAPI Eina_Bool ecore_file_download(const char *url, const char *dst, ...); And style of * is wrong for the first parameter. > Source/WebKit2/UIProcess/API/efl/ewk_download_job_private.h:74 > + EwkDownloadJob(WKDownloadRef download); Let's add "explicit" >> Tools/MiniBrowser/efl/main.c:852 >> + const Ewk_Error *error = downloadError->error; > > I don't think "const" modifier brings benefits here. Instead, how about define `error` field of Ewk_Download_Job_Error with "const" modifier? >> Tools/MiniBrowser/efl/main.c:2302 >> + ewk_context_download_callbacks_set(context, on_download_request, on_download_failed, 0, on_download_finished, window); > > I'd rather move them to context related code in elm_main(), there we already have > > ewk_context_process_model_set(context, EWK_PROCESS_MODEL_MULTIPLE_SECONDARY); > ewk_context_favicon_database_directory_set(context, NULL); +1 Comment on attachment 246314 [details]
Patch
r- now based on previous comments, it isn't ready for landing yet.
Created attachment 247847 [details]
Patch
Attachment 247847 [details] did not pass style-queue:
ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:369: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:370: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:371: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:372: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:373: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 5 in 11 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Hello, I have uploaded new patch with (almost) all comments included. To answer your questions: > I am not sure. > Doesn't we have any use cases which need to distinguish which ewk_view(tab) > requests this download? As far as I know, all modern browsers have general download manager common for all tabs/windows. When user starts to download a file, dialog bog appears and then download is conducted by an agent located next to address bar. When it is finished, agent's icon is blinking, not a tab. > Bug ID? I will introduce new bug, when approach from this bug will be accepted. Now there is no point for this. > ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:369: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] This is caused by space aligment in ewk_context.h (that is why I used only 4 spaces there in initial patch). As far as I know checking style in *.h files should be disabled so probably those messages can be ignored. Best regards, Lukasz Bialek (In reply to comment #7) > Hello, > > I have uploaded new patch with (almost) all comments included. To answer > your questions: > > > I am not sure. > > Doesn't we have any use cases which need to distinguish which ewk_view(tab) > > requests this download? > > As far as I know, all modern browsers have general download manager common > for all tabs/windows. When user starts to download a file, dialog bog > appears and then download is conducted by an agent located next to address > bar. When it is finished, agent's icon is blinking, not a tab. > OK. > > Bug ID? > > I will introduce new bug, when approach from this bug will be accepted. Now > there is no point for this. > > > ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:369: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] > > This is caused by space aligment in ewk_context.h (that is why I used only 4 > spaces there in initial patch). As far as I know checking style in *.h files > should be disabled so probably those messages can be ignored. > Hmm, we can consider to add exception for check-webkit-style. But I am not sure which one is better for us. gyuyoung, what do you think about it? > Best regards, > Lukasz Bialek Comment on attachment 247847 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247847&action=review > Source/WebKit2/ChangeLog:13 > + Propose a new callbacks which are view independent and detach EFL's DownloadMaager Typo: DownloadMaager -> DownloadManager > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_download_job.cpp:-44 > -class EWK2DownloadJobTest : public EWK2UnitTestBase { Why should we remove this unit test ? I think we need to modify this unit test based on this change. > Source/WebKit2/UIProcess/efl/DownloadManagerEfl.cpp:164 > + RefPtr<EwkDownloadJob> ewkDownload = EwkDownloadJob::create(download); I think EwkDownloadJob::create() is not necessary. We can make an instance with new EwkDownloadJob if we move its constructor from private to public. (In reply to comment #8) > (In reply to comment #7) > > Hello, > > > > I have uploaded new patch with (almost) all comments included. To answer > > your questions: > > > > > I am not sure. > > > Doesn't we have any use cases which need to distinguish which ewk_view(tab) > > > requests this download? > > > > As far as I know, all modern browsers have general download manager common > > for all tabs/windows. When user starts to download a file, dialog bog > > appears and then download is conducted by an agent located next to address > > bar. When it is finished, agent's icon is blinking, not a tab. > > > OK. > > > > Bug ID? > > > > I will introduce new bug, when approach from this bug will be accepted. Now > > there is no point for this. > > > > > ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:369: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] > > > > This is caused by space aligment in ewk_context.h (that is why I used only 4 > > spaces there in initial patch). As far as I know checking style in *.h files > > should be disabled so probably those messages can be ignored. > > > > Hmm, we can consider to add exception for check-webkit-style. > But I am not sure which one is better for us. > > gyuyoung, what do you think about it? I think we can land this patch without check-webkit-style change. Created attachment 247943 [details]
Patch
Attachment 247943 [details] did not pass style-queue:
ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:369: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:370: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:371: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:372: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:373: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 5 in 13 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Hello, New patch uploaded. > Typo: DownloadMaager -> DownloadManager Fixed. > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_download_job.cpp:-44 > > -class EWK2DownloadJobTest : public EWK2UnitTestBase { > > Why should we remove this unit test ? I think we need to modify this unit > test based on this change. Tests are modified and enabled. They pass. > > Source/WebKit2/UIProcess/efl/DownloadManagerEfl.cpp:164 > > + RefPtr<EwkDownloadJob> ewkDownload = EwkDownloadJob::create(download); > > I think EwkDownloadJob::create() is not necessary. We can make an instance > with new EwkDownloadJob if we move its constructor from private to public. Unfortunately, when I tried to convert it to unique_ptr approach, I encountered a problem with HashMap add operation ("use of deleted function 'std::unique_ptr<_Tp, _Dp>& std::unique_ptr<_Tp, _Dp>::operator=(...)". I would leave it for now because it will need some further investigation about HashMap and this problem is not connected with subject of this patch. It can be easily fixed in separete bug. Best regards, Lukasz Bialek Comment on attachment 247943 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247943&action=review LGTM except for my small comment. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_download_job.cpp:123 > + static void on_download_cancelled(EwkObject* eventInfo, void* userData) eventInfo is not used inside this function. It can cause unused param build warning. Created attachment 247946 [details]
Patch
Attachment 247946 [details] did not pass style-queue:
ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:369: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:370: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:371: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:372: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKit2/UIProcess/API/efl/ewk_context.h:373: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Total errors found: 5 in 13 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 247946 [details] Patch Clearing flags on attachment: 247946 Committed r181076: <http://trac.webkit.org/changeset/181076> All reviewed patches have been landed. Closing bug. |