Cache the mimeTypes and plugins DOM objects.

Cache the mimeTypes and plugins DOM objects.

At the moment the mimeTypes and plugins objects on navigator return new objects
when indexed by the same ordinal.

This patch adds caching of the mimeTypes and plugins script wrappable DOM objects
DOMPlugin/DOMMimeType in DOMPluginArray/DOMMimeTypeArray.

BUG=616662

[Anton Obzhirov, 2016-07-17 23:10:19]

Description was changed from

==========
Cache the mimeTypes and plugins DOM objects.

At the moment the mimeTypes and plugins objects on navigator return new objects
when indexed by the same ordinal.

This patch adds caching of the mimeTypes and plugins script wrappable DOM
objects
DOMPlugin/DOMMimeType in DOMPluginArray/DOMMimeTypeArray.

BUG=616662
==========

to

==========
Cache the mimeTypes and plugins DOM objects.

At the moment the mimeTypes and plugins objects on navigator return new objects
when indexed by the same ordinal.

This patch adds caching of the mimeTypes and plugins script wrappable DOM
objects
DOMPlugin/DOMMimeType in DOMPluginArray/DOMMimeTypeArray.

BUG=616662
==========

[Anton Obzhirov, 2016-07-17 23:10:19]

a.obzhirov@samsung.com changed reviewers:
+ dominicc@chromium.org, pfeldman@chromium.org

[PhistucK, 2016-07-18 05:28:26]

phistuck@gmail.com changed reviewers:
+ phistuck@gmail.com

[PhistucK, 2016-07-18 05:28:26]

Does disabling a plugin using about:plugins without a page refresh behave as
expected with this patch?

[Anton Obzhirov, 2016-07-18 14:44:37]

On 2016/07/18 05:28:26, PhistucK wrote:
> Does disabling a plugin using about:plugins without a page refresh behave as
> expected with this patch?

Seems so, I've tested it few minutes ago.

[dominicc (has gone to gerrit), 2016-07-19 01:24:17]

As always, please add tests.

[Anton Obzhirov, 2016-07-19 10:43:25]

On 2016/07/19 01:24:17, dominicc wrote:
> As always, please add tests.

Will do.

[pfeldman, 2016-07-22 18:03:49]

pfeldman@chromium.org changed reviewers:
- pfeldman@chromium.org

[Anton Obzhirov, 2016-07-24 15:39:19]

On 2016/07/19 10:43:25, Anton Obzhirov wrote:
> On 2016/07/19 01:24:17, dominicc wrote:
> > As always, please add tests.
> 
> Will do.

Hi guys, I've added a new test. Also updated the code to handle refresh plugins
case, have a look.

[dominicc (has gone to gerrit), 2016-07-25 08:02:37]

Thank you for working on this. A couple of comments inline.

https://codereview.chromium.org/2157883002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/plugins/script-tests/navigator-plugins-mimeTypes-comparison.js
(right):

https://codereview.chromium.org/2157883002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/plugins/script-tests/navigator-plugins-mimeTypes-comparison.js:1:
description(
Could you use testharness.js? It's a better test harness. For example, you don't
need to check in expectations files if all tests in the file pass; you can use
Promises; it can run tests interleaved (async_test); etc.

https://codereview.chromium.org/2157883002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/plugins/DOMMimeType.cpp (right):

https://codereview.chromium.org/2157883002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/plugins/DOMMimeType.cpp:79: return
NavigatorPlugins::plugins(*frame()->domWindow()->navigator())->getPlugin(m_pluginData->mimePluginIndices()[m_index]);
Can you add a LayoutTest test for what happens when you get a MimeType, then
detach the iframe of that document, then call enabledPlugin?

[Anton Obzhirov, 2016-07-27 19:41:56]

On 2016/07/25 08:02:37, dominicc wrote:
> Thank you for working on this. A couple of comments inline.
> 
>
https://codereview.chromium.org/2157883002/diff/20001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/plugins/script-tests/navigator-plugins-mimeTypes-comparison.js
> (right):
> 
>
https://codereview.chromium.org/2157883002/diff/20001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/plugins/script-tests/navigator-plugins-mimeTypes-comparison.js:1:
> description(
> Could you use testharness.js? It's a better test harness. For example, you
don't
> need to check in expectations files if all tests in the file pass; you can use
> Promises; it can run tests interleaved (async_test); etc.
> 
>
https://codereview.chromium.org/2157883002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/plugins/DOMMimeType.cpp (right):
> 
>
https://codereview.chromium.org/2157883002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/plugins/DOMMimeType.cpp:79: return
>
NavigatorPlugins::plugins(*frame()->domWindow()->navigator())->getPlugin(m_pluginData->mimePluginIndices()[m_index]);
> Can you add a LayoutTest test for what happens when you get a MimeType, then
> detach the iframe of that document, then call enabledPlugin?

Hi, thanks for the review, will address both comments. I am on holiday till 5th
of August so will submit the update after 5th.

[Anton Obzhirov, 2016-07-27 19:43:04]

https://codereview.chromium.org/2157883002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/plugins/script-tests/navigator-plugins-mimeTypes-comparison.js
(right):

https://codereview.chromium.org/2157883002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/plugins/script-tests/navigator-plugins-mimeTypes-comparison.js:1:
description(
On 2016/07/25 08:02:37, dominicc wrote:
> Could you use testharness.js? It's a better test harness. For example, you
don't
> need to check in expectations files if all tests in the file pass; you can use
> Promises; it can run tests interleaved (async_test); etc.
Will do, I got also warning when I was submitting cl :)

https://codereview.chromium.org/2157883002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/plugins/DOMMimeType.cpp (right):

https://codereview.chromium.org/2157883002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/plugins/DOMMimeType.cpp:79: return
NavigatorPlugins::plugins(*frame()->domWindow()->navigator())->getPlugin(m_pluginData->mimePluginIndices()[m_index]);
On 2016/07/25 08:02:37, dominicc wrote:
> Can you add a LayoutTest test for what happens when you get a MimeType, then
> detach the iframe of that document, then call enabledPlugin?
Yes, forgot about this API, thanks.

[Anton Obzhirov, 2016-08-14 20:41:52]

On 2016/07/27 19:43:04, Anton Obzhirov wrote:
>
https://codereview.chromium.org/2157883002/diff/20001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/plugins/script-tests/navigator-plugins-mimeTypes-comparison.js
> (right):
> 
>
https://codereview.chromium.org/2157883002/diff/20001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/plugins/script-tests/navigator-plugins-mimeTypes-comparison.js:1:
> description(
> On 2016/07/25 08:02:37, dominicc wrote:
> > Could you use testharness.js? It's a better test harness. For example, you
> don't
> > need to check in expectations files if all tests in the file pass; you can
use
> > Promises; it can run tests interleaved (async_test); etc.
> Will do, I got also warning when I was submitting cl :)
> 
>
https://codereview.chromium.org/2157883002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/plugins/DOMMimeType.cpp (right):
> 
>
https://codereview.chromium.org/2157883002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/plugins/DOMMimeType.cpp:79: return
>
NavigatorPlugins::plugins(*frame()->domWindow()->navigator())->getPlugin(m_pluginData->mimePluginIndices()[m_index]);
> On 2016/07/25 08:02:37, dominicc wrote:
> > Can you add a LayoutTest test for what happens when you get a MimeType, then
> > detach the iframe of that document, then call enabledPlugin?
> Yes, forgot about this API, thanks.

Done, plz check. I've added the new test, it seems to behave well.

[dominicc (has gone to gerrit), 2016-08-30 02:46:29]

Sorry for taking so long to get back to you on this!

Thank you for writing tests. Comments inline.

https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/plugins/enabledPlugin-after-iframe-detached.html
(right):

https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/plugins/enabledPlugin-after-iframe-detached.html:5:
<iframe id='test-plugin-iframe-id'
src="resources/x-blink-test-plugin-iframe.html"></iframe>
Be consistent about types of quotes for attributes--prefer double quotes.

https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/plugins/enabledPlugin-after-iframe-detached.html:8:
async_test(function(t)
Consider using arrow functions; they're slightly more succinct and readable.
Below too.

http://es6-features.org/#StatementBodies

https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/plugins/enabledPlugin-after-iframe-detached.html:12:
{
Put { on the previous line. A bit of Blink C++ style is bleeding into your
JavaScript.

https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/plugins/enabledPlugin-after-iframe-detached.html:15:
document.getElementById("d").innerHTML = "";
Be consistent with single and double quotes for string literals. Prefer single
quotes.

https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/plugins/enabledPlugin-after-iframe-detached.html:19:
}, 50));
Is there an event you can listen for, instead of a timeout? If you just need to
spin the event loop, use requestAnimationFrame. It's way faster.

https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/plugins/navigator-plugins-mimeTypes-comparison.html
(right):

https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/plugins/navigator-plugins-mimeTypes-comparison.html:8:
assert_true(navigator.mimeTypes.length != 0);
Asserting ... > 0 might make the intent clearer.

https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/plugins/navigator-plugins-mimeTypes-comparison.html:11:
for (var i = 0; i < navigator.plugins.length; ++i) {
Maybe i++ is more idiomatic JavaScript?

https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/plugins/navigator-plugins-mimeTypes-comparison.html:13:
if (navigator.plugins[i].name != "Blink Test Plugin")
There's no particular reason you need the Blink test plugin, right? Maybe just
run these assertions for all the plugins.

https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/plugins/navigator-plugins-mimeTypes-comparison.html:15:
assert_true(navigator.mimeTypes[i] === navigator.mimeTypes[i]);
Maybe write a helper, assert_same_object? This would make the intent clearer.

[Anton Obzhirov, 2016-08-30 20:00:29]

On 2016/08/30 02:46:29, dominicc wrote:
> Sorry for taking so long to get back to you on this!
> 
> Thank you for writing tests. Comments inline.
> 
>
https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/plugins/enabledPlugin-after-iframe-detached.html
> (right):
> 
>
https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/plugins/enabledPlugin-after-iframe-detached.html:5:
> <iframe id='test-plugin-iframe-id'
> src="resources/x-blink-test-plugin-iframe.html"></iframe>
> Be consistent about types of quotes for attributes--prefer double quotes.
> 
>
https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/plugins/enabledPlugin-after-iframe-detached.html:8:
> async_test(function(t)
> Consider using arrow functions; they're slightly more succinct and readable.
> Below too.
> 
> http://es6-features.org/#StatementBodies
> 
>
https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/plugins/enabledPlugin-after-iframe-detached.html:12:
> {
> Put { on the previous line. A bit of Blink C++ style is bleeding into your
> JavaScript.
> 
>
https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/plugins/enabledPlugin-after-iframe-detached.html:15:
> document.getElementById("d").innerHTML = "";
> Be consistent with single and double quotes for string literals. Prefer single
> quotes.
> 
>
https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/plugins/enabledPlugin-after-iframe-detached.html:19:
> }, 50));
> Is there an event you can listen for, instead of a timeout? If you just need
to
> spin the event loop, use requestAnimationFrame. It's way faster.
> 
>
https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/plugins/navigator-plugins-mimeTypes-comparison.html
> (right):
> 
>
https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/plugins/navigator-plugins-mimeTypes-comparison.html:8:
> assert_true(navigator.mimeTypes.length != 0);
> Asserting ... > 0 might make the intent clearer.
> 
>
https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/plugins/navigator-plugins-mimeTypes-comparison.html:11:
> for (var i = 0; i < navigator.plugins.length; ++i) {
> Maybe i++ is more idiomatic JavaScript?
> 
>
https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/plugins/navigator-plugins-mimeTypes-comparison.html:13:
> if (navigator.plugins[i].name != "Blink Test Plugin")
> There's no particular reason you need the Blink test plugin, right? Maybe just
> run these assertions for all the plugins.
> 
>
https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/plugins/navigator-plugins-mimeTypes-comparison.html:15:
> assert_true(navigator.mimeTypes[i] === navigator.mimeTypes[i]);
> Maybe write a helper, assert_same_object? This would make the intent clearer.

NP, welcome back :) Will address all issue and submit new one.

[Anton Obzhirov, 2016-09-01 10:46:37]

https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/plugins/enabledPlugin-after-iframe-detached.html
(right):

https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/plugins/enabledPlugin-after-iframe-detached.html:19:
}, 50));
On 2016/08/30 02:46:28, dominicc wrote:
> Is there an event you can listen for, instead of a timeout? If you just need
to
> spin the event loop, use requestAnimationFrame. It's way faster.
Yes, it is just to get out of this loop iteration.
I could also use requestIdleCallback but requestAnimationFrame will be faster.

https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/plugins/navigator-plugins-mimeTypes-comparison.html
(right):

https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/plugins/navigator-plugins-mimeTypes-comparison.html:15:
assert_true(navigator.mimeTypes[i] === navigator.mimeTypes[i]);
On 2016/08/30 02:46:29, dominicc wrote:
> Maybe write a helper, assert_same_object? This would make the intent clearer.
Actually assert_equals could be used. It additionally checks type of the
objects.

[Anton Obzhirov, 2016-09-01 17:30:42]

On 2016/09/01 10:46:37, Anton Obzhirov wrote:
>
https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/plugins/enabledPlugin-after-iframe-detached.html
> (right):
> 
>
https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/plugins/enabledPlugin-after-iframe-detached.html:19:
> }, 50));
> On 2016/08/30 02:46:28, dominicc wrote:
> > Is there an event you can listen for, instead of a timeout? If you just need
> to
> > spin the event loop, use requestAnimationFrame. It's way faster.
> Yes, it is just to get out of this loop iteration.
> I could also use requestIdleCallback but requestAnimationFrame will be faster.
> 
>
https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/plugins/navigator-plugins-mimeTypes-comparison.html
> (right):
> 
>
https://codereview.chromium.org/2157883002/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/plugins/navigator-plugins-mimeTypes-comparison.html:15:
> assert_true(navigator.mimeTypes[i] === navigator.mimeTypes[i]);
> On 2016/08/30 02:46:29, dominicc wrote:
> > Maybe write a helper, assert_same_object? This would make the intent
clearer.
> Actually assert_equals could be used. It additionally checks type of the
> objects.

Updated, plz have a look.

[dominicc (has gone to gerrit), 2016-09-02 00:57:31]

The CQ bit was checked by dominicc@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 00:57:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dominicc (has gone to gerrit), 2016-09-02 01:24:36]

lgtm

[commit-bot: I haz the power, 2016-09-02 02:09:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-02 02:09:27]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Anton Obzhirov, 2016-09-02 08:34:38]

Will check the failing tests, seems to be the real one.

[Anton Obzhirov, 2016-09-05 21:14:09]

The CQ bit was checked by a.obzhirov@samsung.com

[Anton Obzhirov, 2016-09-05 21:14:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominicc@chromium.org
Link to the patchset: https://codereview.chromium.org/2157883002/#ps80001
(title: "Updated after dry run.")

[commit-bot: I haz the power, 2016-09-05 21:14:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Anton Obzhirov, 2016-09-05 21:16:33]

On 2016/09/02 08:34:38, Anton Obzhirov wrote:
> Will check the failing tests, seems to be the real one.

Done, seemed to be an issue with 0 index which was considered an empty key in
the hashmap and failed one of the asserts.
In this case it is the valid key so updated the patch.

[commit-bot: I haz the power, 2016-09-05 21:17:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-05 21:17:09]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Anton Obzhirov, 2016-09-06 07:38:58]

a.obzhirov@samsung.com changed reviewers:
+ tkent@chromium.org

[Anton Obzhirov, 2016-09-06 07:40:45]

On 2016/09/05 21:17:09, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

I've added one of the owner of the updated files.
Plz have a look.

[tkent, 2016-09-06 08:00:00]

This doesn't look a right approach.
Can we merge PluginData into DOMPlugin, and merge MimeClassInfo into
DOMMIMEType?

[Anton Obzhirov, 2016-09-07 14:14:26]

On 2016/09/06 08:00:00, tkent wrote:
> This doesn't look a right approach.
> Can we merge PluginData into DOMPlugin, and merge MimeClassInfo into
> DOMMIMEType?

You mean merge PluginInfo into DOMPlugin?
If we merge PluginInfo into DOMPlugin and MimeClassInfo into DOMMIMEType then
PluginData is not needed and is removed so Page
will not have the direct ownership of the plugins data.
So it will be via Page -> LocalFrame -> LocalDOMWindow -> Navigator ->
NavigatorPlugins -> DOMMimeTypeArray/DOMPluginArray.
I wonder if there will be some issues with that. Refactoring will be quite big,
may be in this case the patch should be split into the tests part and the
implementation.

[tkent, 2016-09-08 05:41:48]

On 2016/09/07 at 14:14:26, a.obzhirov wrote:
> You mean merge PluginInfo into DOMPlugin?

Oh, it's right.

> If we merge PluginInfo into DOMPlugin and MimeClassInfo into DOMMIMEType then
PluginData is not needed and is removed so Page
> will not have the direct ownership of the plugins data.
> So it will be via Page -> LocalFrame -> LocalDOMWindow -> Navigator ->
NavigatorPlugins -> DOMMimeTypeArray/DOMPluginArray.
> I wonder if there will be some issues with that. Refactoring will be quite
big,
> may be in this case the patch should be split into the tests part and the
implementation.

Yeah, it would be a large refactoring, and need multiple CLs.  But it would be
far better than introducing weird HashMaps like Patch Set 5.
The first step would be to investigate whether we can move PluginData ownership
from Page to LocalFrame.

[Anton Obzhirov, 2016-09-08 12:29:58]

On 2016/09/08 05:41:48, tkent wrote:
> On 2016/09/07 at 14:14:26, a.obzhirov wrote:
> > You mean merge PluginInfo into DOMPlugin?
> 
> Oh, it's right.
> 
> > If we merge PluginInfo into DOMPlugin and MimeClassInfo into DOMMIMEType
then
> PluginData is not needed and is removed so Page
> > will not have the direct ownership of the plugins data.
> > So it will be via Page -> LocalFrame -> LocalDOMWindow -> Navigator ->
> NavigatorPlugins -> DOMMimeTypeArray/DOMPluginArray.
> > I wonder if there will be some issues with that. Refactoring will be quite
> big,
> > may be in this case the patch should be split into the tests part and the
> implementation.
> 
> Yeah, it would be a large refactoring, and need multiple CLs.  But it would be
> far better than introducing weird HashMaps like Patch Set 5.
> The first step would be to investigate whether we can move PluginData
ownership
> from Page to LocalFrame.

I will investigate. But since the patchset does fix the interoperability issue
with Firefox and other vendors
without regressions maybe it can be still landed for now with the tests while I
am trying another approach.
We could have a new bug for the refactoring which I would continue with.

[tkent, 2016-09-09 02:57:17]

not lgtm.  We should not introduce HashSets to resolve the issue.

We should fix the interoperability bug, but the bug is not urgent, and not worth
to add ugly hack.

[Anton Obzhirov, 2016-09-09 10:24:53]

On 2016/09/09 02:57:17, tkent wrote:
> not lgtm.  We should not introduce HashSets to resolve the issue.
> 
> We should fix the interoperability bug, but the bug is not urgent, and not
worth
> to add ugly hack.

OK, I will do the investigation and will update about its results.

[dominicc (has gone to gerrit), 2016-10-14 08:23:05]

On 2016/09/09 at 10:24:53, a.obzhirov wrote:
> On 2016/09/09 02:57:17, tkent wrote:
> > not lgtm.  We should not introduce HashSets to resolve the issue.
> > 
> > We should fix the interoperability bug, but the bug is not urgent, and not
worth
> > to add ugly hack.
> 
> OK, I will do the investigation and will update about its results.

I'm going to close this for now, but feel free to reopen it with investigation
results. Or we can discuss on the bug.

[Anton Obzhirov, 2016-10-14 21:57:52]

On 2016/10/14 08:23:05, dominicc wrote:
> On 2016/09/09 at 10:24:53, a.obzhirov wrote:
> > On 2016/09/09 02:57:17, tkent wrote:
> > > not lgtm.  We should not introduce HashSets to resolve the issue.
> > > 
> > > We should fix the interoperability bug, but the bug is not urgent, and not
> worth
> > > to add ugly hack.
> > 
> > OK, I will do the investigation and will update about its results.
> 
> I'm going to close this for now, but feel free to reopen it with investigation
> results. Or we can discuss on the bug.

OK, I will try to find sometime soon to investigate.