Bug 85209 – [EFL] Remove unnecessary extra call to set developer extras setting on the test startup

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Sudarsana Nagineni (babu)]

enable_developer_extras_set() called twice on the test startup. Calling this function with wrong value from resetDefaultsToConsistentValues()is unnecessary because the same function with the correct value called again from createLayoutTestController().

[Sudarsana Nagineni (babu)]

Created attachment 139489 [details]
Patch

Remove an unnecessary extra call enable_developer_extras_set().

[Gyuyoung Kim]

Comment on attachment 139489 [details]
Patch

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

> Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:-228
> -    ewk_view_setting_enable_developer_extras_set(browser->mainView(), EINA_FALSE);

I think we need to keep this developer extras setting in this function. Although resetDefaultsToConsistentValues() is only called by runTest in EFL DRT, other functions is able to call this function. I think we need to modify why we set developer_extras_set as true regardless of inspector tests. I think we can only enable this option when test is for inspector.

[Gyuyoung Kim]

In GTK port case, they have a function to check if developer extras option is enabled. But, they don't check it as below,

static bool shouldEnableDeveloperExtras(const string& pathOrURL)
{
    return true;
}

I take a look what is developer_extra option. It seems the option is used by inspector and to support additional features. For example, message is shown on console or support worker thread on inspector and so on. I'm still thinking we can enable this feature when we run test regarding inspector. It look other ports  just enable this feature regardless of inspector.

By the way, I think resetDefaultsToConsistentValues() is to reset all options. So, IMO, developerExtras option needs to be set as false by default. If other places wanna reset all options by this function, we can't set it as false by this patch.

[Sudarsana Nagineni (babu)]

(In reply to comment #3)
> In GTK port case, they have a function to check if developer extras option is enabled. But, they don't check it as below,
> 
> static bool shouldEnableDeveloperExtras(const string& pathOrURL)
> {
>     return true;
> }
> 
> I take a look what is developer_extra option. It seems the option is used by inspector and to support additional features. For example, message is shown on console or support worker thread on inspector and so on. I'm still thinking we can enable this feature when we run test regarding inspector. It look other ports  just enable this feature regardless of inspector.

Initially, other ports had a check that enables developer extras only for tests that have inspector-enabled/ in their path. But, the following commit removed the condition and enabled the setting always in DRT.
http://trac.webkit.org/changeset/72366

> 
> By the way, I think resetDefaultsToConsistentValues() is to reset all options. So, IMO, developerExtras option needs to be set as false by default. If other places wanna reset all options by this function, we can't set it as false by this patch.

Currently resetDefaultsToConsistentValues() is getting called only on the test startup. So, I thought it's an unnecessary to set the setting FALSE first and change to TRUE again on the test startup. As this is a trivial case, I can keep this as it is, if you think we might use reset*() function in other places to reset the values.

[Raphael Kubo da Costa (:rakuco)]

Interesting, I had not noticed we sort of reset the settings in two different places (createLTC and resetDefaultsToConsistentValues). I understand Gyuyoung's concern, however the way I see it is that after the commit Sudarsana pointed out we always need to enable developer extras and we currently effectively have two contradicting calls immediately following each other, so we either just leave the call in createLTC as it is or remove it and pass EINA_TRUE to the ewk_view_setting call in resetDefaults. I tend to favour the former just to make the API more similar to the one used by other ports so changes such as the very one cited by Sudarsana can be made more easily in the future.

[Gyuyoung Kim]

If DRT needs to enable/disable developer_extras_set during the layout test, I'm still thinking we need to keep ewk_view_setting_enable_developer_extras_set(browser->mainView(), EINA_FALSE); in resetDefaultsToConsistentValues(). But, as Sudarsana said, if this option needs to be turned on always, I agree with submitting patch for now.

Kubo, is this similar your opinion ?

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #6)
> If DRT needs to enable/disable developer_extras_set during the layout test, I'm still thinking we need to keep ewk_view_setting_enable_developer_extras_set(browser->mainView(), EINA_FALSE); in resetDefaultsToConsistentValues(). But, as Sudarsana said, if this option needs to be turned on always, I agree with submitting patch for now.
> 
> Kubo, is this similar your opinion ?

Yes. Right now we're duplicating calls already made in createLTC(). If we remove that, we can even get rid of a FIXME I added in r117540.

[Antonio Gomes]

Comment on attachment 139489 [details]
Patch

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

> Tools/ChangeLog:9
> +        an unnecessary extra call which set wrong value. 

sets

[WebKit Review Bot]

Comment on attachment 139489 [details]
Patch

Clearing flags on attachment: 139489

Committed r118473: <http://trac.webkit.org/changeset/118473>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.