Bug 151846 – [EFL] test_ewk2_context failed after r192808

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Hunseop Jeong]

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)

[Hunseop Jeong]

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?

[Gyuyoung Kim]

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

[Hunseop Jeong]

Created attachment 266756 [details]
Patch

[Hunseop Jeong]

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.

[Ryuan Choi]

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

[Gyuyoung Kim]

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 ?

[Hunseop Jeong]

Created attachment 266771 [details]
Patch

[Hunseop Jeong]

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.

[Gyuyoung Kim]

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.

[Hunseop Jeong]

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'?

[Gyuyoung Kim]

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

[Gyuyoung Kim]

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

[Hunseop Jeong]

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

[Hunseop Jeong]

Created attachment 267773 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 267773 [details]
Patch

LGTM.

[WebKit Commit Bot]

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

[Hunseop Jeong]

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

[Hunseop Jeong]

Created attachment 267778 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 267778 [details]
Patch

Clearing flags on attachment: 267778

Committed r194359: <http://trac.webkit.org/changeset/194359>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.