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.
Created attachment 373072 [details] Patch
This is informal review. Looks good to me.
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?
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?
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.
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".
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)"?
(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.
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.
Created attachment 373294 [details] Patch
(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?
(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.
(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.
(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.
(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.
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.
Comment on attachment 373294 [details] Patch Clearing flags on attachment: 373294 Committed r247064: <https://trac.webkit.org/changeset/247064>
All reviewed patches have been landed. Closing bug.
<rdar://problem/52535659>