Summary: | [WK2][WTR]Adding Layout test framework for MHTML page content | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Santosh Mahto <santosh.mahto> | ||||||||||||||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
Status: | UNCONFIRMED --- | ||||||||||||||||||||
Severity: | Normal | CC: | benjamin, commit-queue, darin, dpranke, glenn, gyuyoung.kim, kling, ngockhanhlam87, santosh.ma, vivekg, zan | ||||||||||||||||||
Priority: | P3 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||
OS: | All | ||||||||||||||||||||
Bug Depends on: | 116333 | ||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||
Attachments: |
|
Description
Santosh Mahto
2013-05-17 06:36:04 PDT
Created attachment 202079 [details]
Patch
@ Benjamin p Hi This is initial set of modification wk2. I am facing problem in modifying webkitpy/ related python code I followed the webarchive and added .mht in .webarvhive place but test still seems to not working. I want it to be a tex diff but as I see log --------------------------------------------------------------------- ./Tools/Scripts/run-webkit-tests --efl --debug -v LayoutTests/mhtml/simple.html -2 --details [1/1] mhtml/simple.html txt: <none> png: <none> wav: <none> webarchive: <none> mht: mhtml/simple-expected.mht ---> its recognising my expected file exp: PASS got: IMAGE ----> WHY IMAGE?? it seems it is going for image difftest took: 3.922 ----------------------------------------------------------------------- can you give me more clue(or some guy name) in webkitpy side modification @Benjamin It seems On above patch it is making a Image Diff as I could see it is making image diff in test result in bowser screen Instead It should make text diff Could you help me why it is going for Image diff The issue here is that the same list of extensions is used to find both all the test cases and the reference files for the reftests. In this case, the layout test that dumps the MHTML archive gets the corresponding .mht baseline recognized as a reference file. Hence the image mismatch. The lists should be split. I'll upload the patch soon, in a separate bug. (In reply to comment #4) > The issue here is that the same list of extensions is used to find both all the test cases and the reference files for the reftests. > > In this case, the layout test that dumps the MHTML archive gets the corresponding .mht baseline recognized as a reference file. Hence the image mismatch. > > The lists should be split. I'll upload the patch soon, in a separate bug. Thanks Zan Dobersek for taking this issue forward. The naming extesnion of baseline expected mhtml file depends on decision on this bug https://bugs.webkit.org/show_bug.cgi?id=116333 Created attachment 202228 [details]
Patch
simple.html --> test page simple-expected.txt --> basedline expected Created attachment 202253 [details]
Patch
Comment on attachment 202253 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=202253&action=review This is a start, but that is not the proper way to test code with an API on the UIProcess. Instead, what should happen is the InjectedBundle sends a message to the UIProcess asking for the data. The UIProcess uses the public API (WKPageGetContentsAsMHTMLData) and return the value to the injected bundle asynchronously. On a more generic standpoint. Why wouldn't the test always dump 2 expected results? One binary, one text? > Tools/WebKitTestRunner/InjectedBundle/TestRunner.h:325 > + bool m_useBinaryEncoding; Instead of this, I would just have two WhatToDump: MHTMLArchive && MHTMLBinaryArchive (In reply to comment #9) > (From update of attachment 202253 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=202253&action=review > > This is a start, but that is not the proper way to test code with an API on the UIProcess. I thought this is last :):) > Instead, what should happen is the InjectedBundle sends a message to the UIProcess asking for the data. The UIProcess uses the public API (WKPageGetContentsAsMHTMLData) and return the value to the injected bundle asynchronously. you seems right. > On a more generic standpoint. Why wouldn't the test always dump 2 expected results? One binary, one text? we don't need to dump two when layout test will just compare with one base line expected text file. and we are doing content test, to do encoding test(binary) we dont need to duplicate everything and adding differnt archive format binary specifies only that content(main resource & subresources) to be encoded in binary or not , MHTML archive format(headers) will still be in same format . MHTMLBinaryArchive is not any archive format. So there is one Archive format MHTML where data content(e.g image) are encoded with ascii(default) or Binary(if sspecified m_useBinaryEncoding = true) its like in webarchive <data> </data> encoding is decided by binary but archive format is always XML > > Tools/WebKitTestRunner/InjectedBundle/TestRunner.h:325 > > + bool m_useBinaryEncoding; This doesnot interfere with MHTML archive format, just changes the encoding on inside content resources > Instead of this, I would just have two WhatToDump: > MHTMLArchive && MHTMLBinaryArchive MHTMLArchive && MHTMLBinaryArchive are not different archive as I said above. specifiying two archive format name looks confusing to me and it will lead to code duplicate(redo everything again for MHTMLBinaryArchive. For MHTML layout tests means we are testing for content of MHTML dump.So its not must seems to mix up encoding test So I think one javascript api dumpAsMHTML(bool m_useBinaryEnCoding = false) is good enough (In reply to comment #10) > (In reply to comment #9) > > (From update of attachment 202253 [details] [details]) > > Instead, what should happen is the InjectedBundle sends a message to the UIProcess asking for the data. The UIProcess uses the public API (WKPageGetContentsAsMHTMLData) and return the value to the injected bundle asynchronously. it seems to me from code study that WebKitTestRunner injectedBundleClient (InjectedBundlePage.cpp) is meant to talk to webprocess injectedBundle only . It is not meant to access UIProcess directly Although I am not 100% sure. So please update your view (In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > (From update of attachment 202253 [details] [details] [details]) > > > > Instead, what should happen is the InjectedBundle sends a message to the UIProcess asking for the data. The UIProcess uses the public API (WKPageGetContentsAsMHTMLData) and return the value to the injected bundle asynchronously. > InjectedBundlePage.cpp is meant to access only WebProcess not UI process. The concept of injected bundle it to inject(use) the code of WebProcess without using UIprocess If I got through UI process it will clear violation of InjectedBundle concept. InjectedBundlePage.cpp is meant to call InjectedBundle of WebProcess. Thats why all kind of dump done by webkittestrunner (webarchive, text) diectly use WebProcess. For WebArchive There is wk2 api also WKFrameGetWebArchive(WKFrameRef in WkFrame.cpp, but injectedBundle use directly webprocess as per rules. Anyway UIprocess Api should be tested by unittest seperatly So I am confident of using webprocess directly Created attachment 203169 [details]
Patch
Comment on attachment 203169 [details]
Patch
Same reasons as before.
> InjectedBundlePage.cpp is meant to access only WebProcess not UI process.
> The concept of injected bundle it to inject(use) the code of WebProcess without using UIprocess
> If I got through UI process it will clear violation of InjectedBundle concept.
> InjectedBundlePage.cpp is meant to call InjectedBundle of WebProcess.
>
> Thats why all kind of dump done by webkittestrunner (webarchive, text) diectly
> use WebProcess.
> For WebArchive There is wk2 api also WKFrameGetWebArchive(WKFrameRef in WkFrame.cpp, but injectedBundle use directly webprocess as per rules.
>
> Anyway UIprocess Api should be tested by unittest seperatly
>
> So I am confident of using webprocess directly
See TestRunner::setPageVisibility(). That will give you an idea of how those stuff are run.
Created attachment 203485 [details]
Patch
>
> Instead, what should happen is the InjectedBundle sends a message to the UIProcess asking for the data. The UIProcess uses the public API (WKPageGetContentsAsMHTMLData) and return the value to the injected bundle asynchronously.
@Benjamin :
The patch is uploaded according to above requirement.
Comment on attachment 203485 [details] Patch Attachment 203485 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/717305 Created attachment 203486 [details]
Patch
Comment on attachment 203486 [details] Patch Attachment 203486 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/723522 Created attachment 203488 [details]
Patch
Created attachment 203914 [details]
Patch
Comment on attachment 203914 [details]
Patch
Assuming that patches for review since 2013 are stale, r-
|