Bug 199300 – [Curl] Fix CookieJarCurl::getRawCookie.

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Takashi Komori]

CookieJarCurl::getRawCookies is not working correctly because it uses CookieJarDB::searchCookies wrongly.

This function is used for displaying cookie data in the web inspector storage tab.

[Takashi Komori]

Created attachment 373072 [details]
Patch

[Basuke Suzuki]

This is informal review. Looks good to me.

[Fujii Hironori]

Comment on attachment 373072 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [Curl] Fix CookieJarCurl::getRawCookie.

Please be more informative. What is the problem, or how do you fix.

> LayoutTests/platform/wincairo-wk1/TestExpectations:301
> +http/tests/inspector/page/get-cookies.html [ Skip ]

Why is it skipped? This test is only for WK2? Then, does mac-wk1 also need to skip?

[Fujii Hironori]

Comment on attachment 373072 [details]
Patch

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

> LayoutTests/ChangeLog:9
> +        * http/tests/inspector/page/get-cookies.html: Added.

This test really should be in 'page' directory? There is LayoutTests/inspector/storage directory. Do you need to create LayoutTests/http/tests/inspector/storage directory?

[Fujii Hironori]

Comment on attachment 373072 [details]
Patch

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

> LayoutTests/http/tests/inspector/page/get-cookies.html:60
> +            InspectorTest.awaitEvent("LoadComplete").then((event) => {

Should you call InspectorTest.awaitEvent() before InspectorTest.evaluateInPage()? It seems that other test do so.

[Fujii Hironori]

Comment on attachment 373072 [details]
Patch

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

> LayoutTests/http/tests/inspector/page/get-cookies.html:85
> +                    InspectorTest.expectEqual(payload.cookies[2].domain, "127.0.0.1", "[Sub] Domain is '127.0.0.1'");

test value for 'value' should be other value than domain. For examle, "xyz".

[Fujii Hironori]

Comment on attachment 373072 [details]
Patch

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

> LayoutTests/http/tests/inspector/page/get-cookies.html:77
> +        description: "Get cookies on Main and SubResources (localhost / 127.0.0.1)",

Why do you need two tests, "Get cookies on Main and SubResource (localhost)" and "Get cookies on Main and SubResources (localhost / 127.0.0.1)"?

[Takashi Komori]

(In reply to Fujii Hironori from comment #5)
> Comment on attachment 373072 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373072&action=review
> 
> > LayoutTests/http/tests/inspector/page/get-cookies.html:60
> > +            InspectorTest.awaitEvent("LoadComplete").then((event) => {
> 
> Should you call InspectorTest.awaitEvent() before
> InspectorTest.evaluateInPage()? It seems that other test do so.

In this part we evaluate 'loadDocumentWithURL()' in test page and wait the results of the evaluation, and then check results.
So if we call evaluateInPage() before awaitEvent(), this test doesn't work.

[Fujii Hironori]

Comment on attachment 373072 [details]
Patch

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

>>> LayoutTests/http/tests/inspector/page/get-cookies.html:60
>>> +            InspectorTest.awaitEvent("LoadComplete").then((event) => {
>> 
>> Should you call InspectorTest.awaitEvent() before InspectorTest.evaluateInPage()? It seems that other test do so.
> 
> In this part we evaluate 'loadDocumentWithURL()' in test page and wait the results of the evaluation, and then check results.
> So if we call evaluateInPage() before awaitEvent(), this test doesn't work.

Do you mean "if we call awaitEvent() before evaluateInPage()"?
Why? I think awaitEvent should be called before evaluateInPage because it needs to register a event listener before start loading.

[Takashi Komori]

Created attachment 373294 [details]
Patch

[Takashi Komori]

(In reply to Fujii Hironori from comment #3)
> Comment on attachment 373072 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373072&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        [Curl] Fix CookieJarCurl::getRawCookie.
> 
> Please be more informative. What is the problem, or how do you fix.
> 
> > LayoutTests/platform/wincairo-wk1/TestExpectations:301
> > +http/tests/inspector/page/get-cookies.html [ Skip ]
> 
> Why is it skipped? This test is only for WK2? Then, does mac-wk1 also need
> to skip?

Because almost all inspector tests don't work on wincairo DRT.

I think mac-wk1 skips it because top level TestExpectations skips http/test/inspector/page/ directory and TestExpectations for mac-wk1 doesn't mention this test.
Is it correct?

[Takashi Komori]

(In reply to Fujii Hironori from comment #4)
> Comment on attachment 373072 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373072&action=review
> 
> > LayoutTests/ChangeLog:9
> > +        * http/tests/inspector/page/get-cookies.html: Added.
> 
> This test really should be in 'page' directory? There is
> LayoutTests/inspector/storage directory. Do you need to create
> LayoutTests/http/tests/inspector/storage directory?

If http/tests/inspector/storage/ is properer position than http/tests/inspector/page/ for this test, we will move it.
I would like to know the more detailed reasonings for changing the path.

[Takashi Komori]

(In reply to Fujii Hironori from comment #6)
> Comment on attachment 373072 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373072&action=review
> 
> > LayoutTests/http/tests/inspector/page/get-cookies.html:85
> > +                    InspectorTest.expectEqual(payload.cookies[2].domain, "127.0.0.1", "[Sub] Domain is '127.0.0.1'");
> 
> test value for 'value' should be other value than domain. For examle, "xyz".

Fixed.

[Takashi Komori]

(In reply to Fujii Hironori from comment #7)
> Comment on attachment 373072 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373072&action=review
> 
> > LayoutTests/http/tests/inspector/page/get-cookies.html:77
> > +        description: "Get cookies on Main and SubResources (localhost / 127.0.0.1)",
> 
> Why do you need two tests, "Get cookies on Main and SubResource (localhost)"
> and "Get cookies on Main and SubResources (localhost / 127.0.0.1)"?

Removed extra test.

[Takashi Komori]

(In reply to Fujii Hironori from comment #9)
> Comment on attachment 373072 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373072&action=review
> 
> >>> LayoutTests/http/tests/inspector/page/get-cookies.html:60
> >>> +            InspectorTest.awaitEvent("LoadComplete").then((event) => {
> >> 
> >> Should you call InspectorTest.awaitEvent() before InspectorTest.evaluateInPage()? It seems that other test do so.
> > 
> > In this part we evaluate 'loadDocumentWithURL()' in test page and wait the results of the evaluation, and then check results.
> > So if we call evaluateInPage() before awaitEvent(), this test doesn't work.
> 
> Do you mean "if we call awaitEvent() before evaluateInPage()"?
> Why? I think awaitEvent should be called before evaluateInPage because it
> needs to register a event listener before start loading.

I misunderstood. Fixed.

[Fujii Hironori]

Comment on attachment 373072 [details]
Patch

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

>>> LayoutTests/ChangeLog:9
>>> +        * http/tests/inspector/page/get-cookies.html: Added.
>> 
>> This test really should be in 'page' directory? There is LayoutTests/inspector/storage directory. Do you need to create LayoutTests/http/tests/inspector/storage directory?
> 
> If http/tests/inspector/storage/ is properer position than http/tests/inspector/page/ for this test, we will move it.
> I would like to know the more detailed reasonings for changing the path.

There is LayoutTests/inspector/page/filter-cookies-for-domain.html which is tesing "Page.getCookie".
Your test case is testing "Page.getCookies". http/tests/inspector/page seems appropriate.

[WebKit Commit Bot]

Comment on attachment 373294 [details]
Patch

Clearing flags on attachment: 373294

Committed r247064: <https://trac.webkit.org/changeset/247064>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/52535659>