85209 2012-04-30 10:50:23 -0700 [EFL] Remove unnecessary extra call to set developer extras setting on the test startup 2012-05-24 22:00:48 -0700 1 1 1 Unclassified WebKit WebKit EFL 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 naginenis naginenis gyuyoung.kim gyuyoung.kim lucas.de.marchi rakuco tonikitoo webkit.review.bot oldest_to_newest 612747 0 naginenis 2012-04-30 10:50:23 -0700 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(). 612792 1 139489 naginenis 2012-04-30 11:31:24 -0700 Created attachment 139489 Patch Remove an unnecessary extra call enable_developer_extras_set(). 614002 2 139489 gyuyoung.kim 2012-05-01 22:56:48 -0700 Comment on attachment 139489 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. 614172 3 gyuyoung.kim 2012-05-02 04:15:18 -0700 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. 614220 4 naginenis 2012-05-02 06:03:32 -0700 (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. 614296 5 rakuco 2012-05-02 07:45:52 -0700 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. 628875 6 gyuyoung.kim 2012-05-21 01:39:33 -0700 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 ? 631738 7 rakuco 2012-05-23 09:48:46 -0700 (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. 633536 8 139489 tonikitoo 2012-05-24 21:52:39 -0700 Comment on attachment 139489 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 633544 9 139489 webkit.review.bot 2012-05-24 22:00:36 -0700 Comment on attachment 139489 Patch Clearing flags on attachment: 139489 Committed r118473: <http://trac.webkit.org/changeset/118473> 633546 10 webkit.review.bot 2012-05-24 22:00:48 -0700 All reviewed patches have been landed. Closing bug. 139489 2012-04-30 11:31:24 -0700 2012-05-24 22:00:35 -0700 Patch 85209.patch text/plain 1650 naginenis ZGlmZiAtLWdpdCBhL1Rvb2xzL0NoYW5nZUxvZyBiL1Rvb2xzL0NoYW5nZUxvZwppbmRleCAyZTY5 OTQ4Li4yYjg4ZTFmIDEwMDY0NAotLS0gYS9Ub29scy9DaGFuZ2VMb2cKKysrIGIvVG9vbHMvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTYgQEAKKzIwMTItMDQtMzAgIFN1ZGFyc2FuYSBOYWdpbmVuaSAg PHN1ZGFyc2FuYS5uYWdpbmVuaUBsaW51eC5pbnRlbC5jb20+CisKKyAgICAgICAgW0VGTF0gUmVt b3ZlIHVubmVjZXNzYXJ5IGV4dHJhIGNhbGwgdG8gc2V0IGRldmVsb3BlciBleHRyYXMgc2V0dGlu ZyBvbiB0aGUgdGVzdCBzdGFydHVwCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3No b3dfYnVnLmNnaT9pZD04NTIwOQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEp LgorCisgICAgICAgIFNldCBkZXZlbG9wZXIgZXh0cmFzIHNldHRpbmcgY2FsbGVkIHR3aWNlIG9u IHRoZSB0ZXN0IHN0YXJ0dXAuIEhlbmNlLCByZW1vdmluZworICAgICAgICBhbiB1bm5lY2Vzc2Fy eSBleHRyYSBjYWxsIHdoaWNoIHNldCB3cm9uZyB2YWx1ZS4gCisKKyAgICAgICAgKiBEdW1wUmVu ZGVyVHJlZS9lZmwvRHVtcFJlbmRlclRyZWVDaHJvbWUuY3BwOgorICAgICAgICAoRHVtcFJlbmRl clRyZWVDaHJvbWU6OnJlc2V0RGVmYXVsdHNUb0NvbnNpc3RlbnRWYWx1ZXMpOgorCiAyMDEyLTA0 LTI5ICBNYWNpZWogU3RhY2hvd2lhayAgPG1qc0BhcHBsZS5jb20+CiAKICAgICAgICAgUkVHUkVT U0lPTjogT24gTGlvbiwgcnVuLXdlYmtpdC10ZXN0cyBjaGFuZ2VzIHRoZSBkaXNwbGF5IGNvbG9y IHByb2ZpbGUgZXZlbiB3aGVuIG5vdCBydW5uaW5nIHBpeGVsIHRlc3RzCmRpZmYgLS1naXQgYS9U b29scy9EdW1wUmVuZGVyVHJlZS9lZmwvRHVtcFJlbmRlclRyZWVDaHJvbWUuY3BwIGIvVG9vbHMv RHVtcFJlbmRlclRyZWUvZWZsL0R1bXBSZW5kZXJUcmVlQ2hyb21lLmNwcAppbmRleCA2MzAwNDEw Li5kNDkwMDVmIDEwMDY0NAotLS0gYS9Ub29scy9EdW1wUmVuZGVyVHJlZS9lZmwvRHVtcFJlbmRl clRyZWVDaHJvbWUuY3BwCisrKyBiL1Rvb2xzL0R1bXBSZW5kZXJUcmVlL2VmbC9EdW1wUmVuZGVy VHJlZUNocm9tZS5jcHAKQEAgLTIyNSw3ICsyMjUsNiBAQCB2b2lkIER1bXBSZW5kZXJUcmVlQ2hy b21lOjpyZXNldERlZmF1bHRzVG9Db25zaXN0ZW50VmFsdWVzKCkKICAgICBld2tfdmlld19zZXR0 aW5nX2F1dG9fbG9hZF9pbWFnZXNfc2V0KG1haW5WaWV3KCksIEVJTkFfVFJVRSk7CiAgICAgZXdr X3ZpZXdfc2V0dGluZ191c2VyX3N0eWxlc2hlZXRfc2V0KG1haW5WaWV3KCksIDApOwogICAgIGV3 a192aWV3X3NldHRpbmdfZW5hYmxlX3hzc19hdWRpdG9yX3NldChicm93c2VyLT5tYWluVmlldygp LCBFSU5BX1RSVUUpOwotICAgIGV3a192aWV3X3NldHRpbmdfZW5hYmxlX2RldmVsb3Blcl9leHRy YXNfc2V0KGJyb3dzZXItPm1haW5WaWV3KCksIEVJTkFfRkFMU0UpOwogICAgIGV3a192aWV3X3Nl dHRpbmdfbWluaW11bV90aW1lcl9pbnRlcnZhbF9zZXQoYnJvd3Nlci0+bWFpblZpZXcoKSwgMC4w MTApOyAvLyAxMCBtaWxsaXNlY29uZHMgKERPTVRpbWVyOjpzX21pbkRlZmF1bHRUaW1lckludGVy dmFsKQogCiAgICAgZXdrX3ZpZXdfem9vbV9zZXQobWFpblZpZXcoKSwgMS4wLCAwLCAwKTsK