[webkit-dev] Should we move methods from layoutTestController/etc... to internals?

[Hajime Morrita]

On Wed, Feb 22, 2012 at 10:48 AM, Maciej Stachowiak <mjs at apple.com> wrote:
>
> On Feb 21, 2012, at 5:40 PM, Hajime Morrita wrote:
>
>> Hi, Thanks for starting this discussion!
>>
>> To summarize, there is a rough consensus which is like:
>>
>> * A. LayoutTestController (LTC) is for control test harness.
>> dumpAsText() and waitUntilDone() is great example of this.
>>
>> * B. EventSender and some other stuff are for emulating external
>> events like user input.
>> keyDown() is good example.
>>
>> * C. Internals is for inspecting WebCore internal state which isn't
>> available from WebKit API.
>> Inspecting spellcheck markers is one typical example.
>>
>> But I found that there are some test-specific APIs which don't fit
>> well in any of listed categories:
>>
>> * D. Changing Settings and RuntimeFeatures values.
>> Inspecting flags in these objects is a good fit for Internals. But how
>> about changing these values? I feel that it's no longer "inspecting".
>> On the other hand, we know that adding new WebKit and LTC API for each
>> these flags is certainly tedious work especially the flag is just for
>> disabling/enabling half-baked features. Currently we have several flag
>> setters on Internals object but I worry a bit this as a start of
>> abusing Internals.
>>
>> My feeling is that it's OK to use Internals family assuming that
>> getting/setting flags are "trivial" enough so that we don't need to
>> worry about unexpected side-effects. We already have InternalsSettings
>> for this purpose.
>>
>> * E. Invoking some WebCore API which implements "portable" WebKit API.
>> This is one step forward from D. There are some WebKit APIs which just
>> call corresponding WebCore methods. A good example of WebCore side is
>> WebCore::Page::setDeviceScaleFactor(): There is a discussion to
>> whether we should add a wrapper API for this to Internals:
>> http://wkb.ug/74014.
>>
>> This API is less trivial than changing settings since this isn't just
>> a parameter setting but has a side effect. On the other hand, some API
>> like this has no platform-specific part. WebKit API just calls this.
>> It could be felt as a waste of abstraction if we add LTC API for that
>> - it will just stack a few more call frames.
>>
>> I think we could cover this usecase by some Internals-like object. But
>> it should never be Internals itself and ideally a wrapper of WebCore
>> object. For example, we could have something like PageWrapper for
>> WebCore::Page which could be implemented in the same way as Internals.
>>
>> Or, we could employ the controller-client pattern to have IDL
>> generated class in shared WebCoreTestSupport and let each DRT to
>> implement the client. This could eliminate the need for hand-written
>> bindings even though we need to have some per-port stuff.
>>
>> * F. Purely for testing, but "portable" mechanism.
>> Mocking some clients are primary example of this. Mock objects are
>> rarely platform specific and implementing it in each platform is
>> nothing more than coding practice IMHO. I understand that there are
>> some controversy around mock itself. But we have some features which
>> are hard to test in automated manner without mock.
>>
>> For this, we could do same as E.
>>
>> I think LTC isn't good place for neither D, E or F. But Internals also isn't.
>> What do you think?
>
> For D, E and F, I think the methods should be split up into object by purpose rather than by how they'd be implemented. I agree that these methods shouldn't be piled onto a single LayoutTestController object.

That's right.

>
> One concern I have about settings: poking through the WebKit API layer to set settings directly in WebCore may not necessarily work as expected in all WebKit ports, or at least it will violate implementation invariants. For example, in WebKit2 (currently using the WebKitTestRunner tool which is separate from DumpRenderTree at present), both the WebProcess and the UI process need to know about settings. This makes me think that settings (at least ones surfaced at the API level) should actually go through the WebKit API rather than poking directly into WebCore via the internals object.
>

Good point. I wasn't aware of this.

In my understanding, the problem here is that these states can be
out-of-sync. Considering so, we can move a part of Settings parameters
to RuntimeFeatures, which doesn't need to be updated dynamically. I
guess there is a certain numer of flags in Settings which are't
changed from the default outside the test.

Thanks for pointing this out.

Bests,
--
morrita

> Regards,
> Maciej
>
>
>
>
>