Bug 151846 - [EFL] test_ewk2_context failed after r192808
Summary: [EFL] test_ewk2_context failed after r192808
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hunseop Jeong
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-12-03 20:25 PST by Hunseop Jeong
Modified: 2015-12-21 21:51 PST (History)
5 users (show)

See Also:


Attachments
Patch (4.27 KB, patch)
2015-12-07 02:00 PST, Hunseop Jeong
no flags Details | Formatted Diff | Diff
Patch (13.14 KB, patch)
2015-12-07 05:50 PST, Hunseop Jeong
no flags Details | Formatted Diff | Diff
Patch (12.59 KB, patch)
2015-12-21 17:51 PST, Hunseop Jeong
no flags Details | Formatted Diff | Diff
Patch (12.67 KB, patch)
2015-12-21 21:02 PST, Hunseop Jeong
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hunseop Jeong 2015-12-03 20:25:15 PST
https://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2/builds/25771/steps/API%20tests/logs/stdio

../../Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:178: Failure
Value of: ewk_context_process_model_get(context)
  Actual: 1
Expected: EWK_PROCESS_MODEL_SHARED_SECONDARY
Which is: 0

../../Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:216: Failure
Value of: ewk_context_process_model_get(context)
  Actual: 1
Expected: EWK_PROCESS_MODEL_SHARED_SECONDARY
Which is: 0

../../Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:209: Failure
Expected: (webView1WebProcessID) != (webView2WebProcessID), actual: 16158 vs 16158
[  FAILED  ] EWK2ContextTestMultipleProcesses.ewk_context_web_process_model (11 ms)

../../Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:257: Failure
Expected: (webView1WebProcessID) != (webView2WebProcessID), actual: 16162 vs 16162
[  FAILED  ] EWK2ContextTestMultipleProcesses.ewk_context_network_process_model (9 ms)
Comment 1 Hunseop Jeong 2015-12-03 20:39:32 PST
As ProcessModel always be the MultipleSecondaryProcesses, EFL also need to deprecate the ewk_context_process_model_set/get apis and change the tests. Do I remove the it and add the some apis that set the process count limit?
Comment 2 Gyuyoung Kim 2015-12-03 20:49:35 PST
(In reply to comment #1)
> As ProcessModel always be the MultipleSecondaryProcesses, EFL also need to
> deprecate the ewk_context_process_model_set/get apis and change the tests.
> Do I remove the it and add the some apis that set the process count limit?

Because http://trac.webkit.org/changeset/192808 always made processmodel MultipleSecondaryProcesses, I agree to modify our test.
Comment 3 Hunseop Jeong 2015-12-07 02:00:42 PST
Created attachment 266756 [details]
Patch
Comment 4 Hunseop Jeong 2015-12-07 02:08:44 PST
I just fix the test_ewk2_context API, we have to add the APIs for setting the webprocess count like as GTK+ port. I will add the new APIs and tests in new bug.
Comment 5 Ryuan Choi 2015-12-07 03:40:57 PST
(In reply to comment #4)
> I just fix the test_ewk2_context API, we have to add the APIs for setting
> the webprocess count like as GTK+ port. I will add the new APIs and tests in
> new bug.

I agree that we should support the legacy behaviour although we add new API.
In addition, I think that we should deprecate test_ewk2_context_process_model_{get|set} with new API to set the count of web process.
Comment 6 Gyuyoung Kim 2015-12-07 03:57:07 PST
Comment on attachment 266756 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:86
> +    setProcessModel(EWK_PROCESS_MODEL_SHARED_SECONDARY);

I don't understand yet why we need to keep this legacy option. I think we just need to remove this option in EWK APIs and API test, isn't it ?
Comment 7 Hunseop Jeong 2015-12-07 05:50:41 PST
Created attachment 266771 [details]
Patch
Comment 8 Hunseop Jeong 2015-12-07 05:57:11 PST
I removed the deprecated ewk_context_process_model_{set|get} apis and added the ewk_context_web_process_count_limit_{set|get} apis for setting the webprocess count. we can control the process behaviors by setting the webprocess count.
Comment 9 Gyuyoung Kim 2015-12-07 07:31:44 PST
Comment on attachment 266771 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:264
> +    m_processCountLimit = count;

Should we update the process count even when new count is same with m_processCountLimit ? Can't we just return when value is same ?

> Source/WebKit2/UIProcess/API/efl/ewk_context.h:504
> + * @return the maximum number of web processes, or 0 if there isn't a limit.

Do not add a period to be sync with existing style.
Comment 10 Hunseop Jeong 2015-12-07 17:53:06 PST
I just fix the test_ewk2_context API, we have to add the APIs for setting the webprocess count like as GTK+ port. I will add the new APIs and tests in new bug.(In reply to comment #9)
> Comment on attachment 266771 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=266771&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:264
> > +    m_processCountLimit = count;
> 
> Should we update the process count even when new count is same with
> m_processCountLimit ? Can't we just return when value is same ?
Okay, I will add it.

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_context.h:504
> > + * @return the maximum number of web processes, or 0 if there isn't a limit.
> 
> Do not add a period to be sync with existing style.
I don't understand the 'period' meaning,, could you tell me the meaning of 'period'?
Comment 11 Gyuyoung Kim 2015-12-09 01:19:52 PST
(In reply to comment #10)
> > > Source/WebKit2/UIProcess/API/efl/ewk_context.h:504
> > > + * @return the maximum number of web processes, or 0 if there isn't a limit.
> > 
> > Do not add a period to be sync with existing style.
> I don't understand the 'period' meaning,, could you tell me the meaning of
> 'period'?

"."(a period) at the end of line.
Comment 12 Gyuyoung Kim 2015-12-17 21:39:13 PST
(In reply to comment #11)
> (In reply to comment #10)
> > > > Source/WebKit2/UIProcess/API/efl/ewk_context.h:504
> > > > + * @return the maximum number of web processes, or 0 if there isn't a limit.
> > > 
> > > Do not add a period to be sync with existing style.
> > I don't understand the 'period' meaning,, could you tell me the meaning of
> > 'period'?
> 
> "."(a period) at the end of line.

Any update ? I wish ewk_context API test is fine again.
Comment 13 Hunseop Jeong 2015-12-18 00:16:30 PST
(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > > > Source/WebKit2/UIProcess/API/efl/ewk_context.h:504
> > > > > + * @return the maximum number of web processes, or 0 if there isn't a limit.
> > > > 
> > > > Do not add a period to be sync with existing style.
> > > I don't understand the 'period' meaning,, could you tell me the meaning of
> > > 'period'?
> > 
> > "."(a period) at the end of line.
> 
> Any update ? I wish ewk_context API test is fine again.
Oh,, Sorry,, I'll take care of it.
This patch has some problems to fix this issue.
So I need more time. I'll fix it ASAP.
Comment 14 Hunseop Jeong 2015-12-21 17:51:52 PST
Created attachment 267773 [details]
Patch
Comment 15 Gyuyoung Kim 2015-12-21 18:23:48 PST
Comment on attachment 267773 [details]
Patch

LGTM.
Comment 16 WebKit Commit Bot 2015-12-21 20:49:26 PST
Comment on attachment 267773 [details]
Patch

Rejecting attachment 267773 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 267773, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WebKit2/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.webkit.org/results/591796
Comment 17 Hunseop Jeong 2015-12-21 20:54:15 PST
(In reply to comment #15)
> Comment on attachment 267773 [details]
> Patch
> 
> LGTM.

Oops,, I missed the 'Reviewed by' comment in ChangeLog.
I will upload new patch.
Comment 18 Hunseop Jeong 2015-12-21 21:02:11 PST
Created attachment 267778 [details]
Patch
Comment 19 WebKit Commit Bot 2015-12-21 21:51:24 PST
Comment on attachment 267778 [details]
Patch

Clearing flags on attachment: 267778

Committed r194359: <http://trac.webkit.org/changeset/194359>
Comment 20 WebKit Commit Bot 2015-12-21 21:51:29 PST
All reviewed patches have been landed.  Closing bug.