Bug 116312 - [WK2][WTR]Adding Layout test framework for MHTML page content
Summary: [WK2][WTR]Adding Layout test framework for MHTML page content
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 116333
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-17 06:36 PDT by Santosh Mahto
Modified: 2019-06-09 08:57 PDT (History)
11 users (show)

See Also:


Attachments
Patch (13.23 KB, patch)
2013-05-17 06:41 PDT, Santosh Mahto
no flags Details | Formatted Diff | Diff
Patch (12.98 KB, patch)
2013-05-19 00:29 PDT, Santosh Mahto
no flags Details | Formatted Diff | Diff
Patch (12.95 KB, patch)
2013-05-19 21:15 PDT, Santosh Mahto
benjamin: review-
Details | Formatted Diff | Diff
Patch (15.24 KB, patch)
2013-05-29 04:45 PDT, Santosh Mahto
no flags Details | Formatted Diff | Diff
Patch (15.52 KB, patch)
2013-06-01 02:24 PDT, Santosh Mahto
no flags Details | Formatted Diff | Diff
Patch (15.39 KB, patch)
2013-06-01 02:36 PDT, Santosh Mahto
no flags Details | Formatted Diff | Diff
Patch (15.34 KB, patch)
2013-06-01 03:06 PDT, Santosh Mahto
no flags Details | Formatted Diff | Diff
Patch (15.71 KB, patch)
2013-06-06 02:13 PDT, Santosh Mahto
beidson: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Santosh Mahto 2013-05-17 06:36:04 PDT
Currently There is no support for layout test for page  content in MHTML format.
Currently  WKPageGetContentsAsMHTMLData has been exposed in WK2 which is used to get the
content in MHTML format.

This bug add  layout test framework  for testing page content in MHTMLArchive format.
it exposed testcase javascript API  testRunner.dumpAsMHTMLArchive. and compare with -expectd.mht file

Currently the Work is in process
Comment 1 Santosh Mahto 2013-05-17 06:41:02 PDT
Created attachment 202079 [details]
Patch
Comment 2 Santosh Mahto 2013-05-17 06:49:06 PDT
@ 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
Comment 3 Santosh Mahto 2013-05-17 08:59:57 PDT
@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
Comment 4 Zan Dobersek 2013-05-17 09:21:59 PDT
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.
Comment 5 Santosh Mahto 2013-05-17 19:02:24 PDT
(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
Comment 6 Santosh Mahto 2013-05-19 00:29:23 PDT
Created attachment 202228 [details]
Patch
Comment 7 Santosh Mahto 2013-05-19 00:48:18 PDT
simple.html --> test page
simple-expected.txt --> basedline expected
Comment 8 Santosh Mahto 2013-05-19 21:15:19 PDT
Created attachment 202253 [details]
Patch
Comment 9 Benjamin Poulain 2013-05-22 17:00:59 PDT
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
Comment 10 Santosh Mahto 2013-05-23 03:19:30 PDT
(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
Comment 11 Santosh Mahto 2013-05-26 02:04:25 PDT
(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
Comment 12 Santosh Mahto 2013-05-28 01:13:14 PDT
(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
Comment 13 Santosh Mahto 2013-05-29 04:45:44 PDT
Created attachment 203169 [details]
Patch
Comment 14 Benjamin Poulain 2013-05-29 22:45:32 PDT
Comment on attachment 203169 [details]
Patch

Same reasons as before.
Comment 15 Benjamin Poulain 2013-05-29 22:46:55 PDT
> 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.
Comment 16 Santosh Mahto 2013-06-01 02:24:33 PDT
Created attachment 203485 [details]
Patch
Comment 17 Santosh Mahto 2013-06-01 02:27:43 PDT
> 
> 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 18 Early Warning System Bot 2013-06-01 02:30:32 PDT
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
Comment 19 Santosh Mahto 2013-06-01 02:36:03 PDT
Created attachment 203486 [details]
Patch
Comment 20 Early Warning System Bot 2013-06-01 02:41:46 PDT
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
Comment 21 Santosh Mahto 2013-06-01 03:06:32 PDT
Created attachment 203488 [details]
Patch
Comment 22 Santosh Mahto 2013-06-06 02:13:18 PDT
Created attachment 203914 [details]
Patch
Comment 23 Brady Eidson 2016-05-24 22:00:30 PDT
Comment on attachment 203914 [details]
Patch

Assuming that patches for review since 2013 are stale, r-