Cordova-plugin-googlemaps: Refactor www/googlemaps-cdv-plugin.js

[stalniy]

I'm submitting a ... (check one with "x")

• [ ] question
• [ ] any problem or bug report
• [ ] feature request
• [x] contribution

If you choose 'problem or bug report', please select OS: (check one with "x")

• [x] Android
• [x] iOS

I'd like to refactor www/googlemaps-cdv-plugin.js as it contains a lot of complicated logic. That logic is really hard to understand, also I see there are some code duplication which adds in size in resulting app bundle (js evaluation take time especially at 1st boot).

Why?

• this will improve codebase, thus it will be easier to make contributions
• this will decrease amount of js code, so app will load faster
• this will allow to remove redundant/dead code
• this will allow to write unit tests for almost every file in the plugin
• this will allow to refactor some recursive calls with loops (loops are faster than recursion in JS)

What I eventually plan to do is to split that logic between 3 objects:

• MapFactory - responsible for Map creation, will be used instead of massive logic in getMap function
• MapsApi - easy interface to make native API calls to GoogleMaps related plugins (this will encapsulate Command queue mechanism). This interface I plan to build based on promises
• DomObserver - object which will walk over DOM elements and collect their rects, zIndex, etc and watch when they changed

I started to do this in my fork, so you are welcome to check the first results here. This is the initial refactoring.

Currently what I ended up with is in https://github.com/stalniy/cordova-plugin-googlemaps/tree/refactor/map . This is still not final version but very close to that. Currently I'm testing every usecase from v2.0 demo repo.

Let me know whether you interested in merging this into core project!

Thanks for awesome plugin!

P.S.: also I'd like to remove existing polyfills from the project and just add a note that this project requires that polyfills on older webviews. This will allow people to choose whatever polyfill library they like :) Also this will decrease the resulting js size of the plugin. Obviously this may be discussed in a separate ticket

[wf9a5m75]

Thank you for offering the cleanup code.
I understand what your point is.
The reason of the messy code is I created is through tons of experiments.
I guess the current code is stable.

As far as I tested on bunch of projects, bunch of frameworks, the code is fine.
(Well, you have it that the code has problem with ionic tab on another ticket. Please share an example project on github)

Let's do clean up. I will check your current code, let me take time.

[wf9a5m75]

I will update the things I noticed.

---

case: background

location: https://github.com/mapsplugin/cordova-plugin-googlemaps/compare/660b7a3...stalniy:refactor/map-pr?expand=1#diff-48ad1d52e6acef24cc4b997847d030b1L124

html, body, ._gmaps_cdv_, ._gmaps_cdv_ .nav-decor {
   background: transparent !important;
}

description
This is not enough. In order to display the maps, the parent HTML elements must be transparent.
There was a case, a HTML element has background-color and background-image for both to the one html element.
For that case background: transparent !important; did not work.

Specifying three ways override the background property mandatory.

background-image: url() !important;
background: rgba(0,0,0,0) url() !important;
background-color: rgba(0,0,0,0) !important;

---

case : document.body.offsetHeight;

location: https://github.com/mapsplugin/cordova-plugin-googlemaps/compare/660b7a3...stalniy:refactor/map-pr?expand=1#diff-48ad1d52e6acef24cc4b997847d030b1L146

//setTimeout(function() {
  // Webkit redraw mandatory
  // http://stackoverflow.com/a/3485654/697856
  document.body.style.backgroundColor = "rgba(0,0,0,0)";
    //document.body.style.display='none';
    document.body.offsetHeight;
  //document.body.style.display='';
//}, 0);

description :

This is necessary code for UIWebView on iOS10

---

case : null pointer error

location
https://github.com/mapsplugin/cordova-plugin-googlemaps/compare/660b7a3...stalniy:refactor/map-pr?expand=1#diff-48ad1d52e6acef24cc4b997847d030b1L219

-      if (scrollEndTimer) {
-        clearTimeout(scrollEndTimer);
-      }
+    document.body.addEventListener("scroll", function() {
+      clearTimeout(scrollEndTimer);

description

Removing if statement may cause null pointer error or undefined error.
I don't remember exact case, but I faced the error when I tested.

---

case undefined error on Android 4.4

location https://github.com/mapsplugin/cordova-plugin-googlemaps/compare/660b7a3...stalniy:refactor/map-pr?expand=1#diff-48ad1d52e6acef24cc4b997847d030b1L288

-              if (mutation.addedNodes) {

https://github.com/mapsplugin/cordova-plugin-googlemaps/compare/660b7a3...stalniy:refactor/map-pr?expand=1#diff-48ad1d52e6acef24cc4b997847d030b1L298

-              if (mutation.removedNodes) {

description
Android 4.4 has kind of tricky. mutation.addedNodes does not exist always.

---

case : android back button

location
https://github.com/mapsplugin/cordova-plugin-googlemaps/compare/660b7a3...stalniy:refactor/map-pr?expand=1#diff-48ad1d52e6acef24cc4b997847d030b1L605

-    //--------------------------------------------
-    // Hook the backbutton of Android action
-    //--------------------------------------------
-    var anotherBackbuttonHandler = null;
-    function onBackButton(e) {
-      common.nextTick(putHtmlElements);
-      if (anotherBackbuttonHandler) {
-        // anotherBackbuttonHandler must handle the page moving transaction.
-        // The plugin does not take care anymore if another callback is registered.
-        anotherBackbuttonHandler(e);
-      } else {
-        cordova_exec(null, null, 'CordovaGoogleMaps', 'backHistory', []);
-      }
-    }
-    document.addEventListener("backbutton", onBackButton);
-
-    var _org_addEventListener = document.addEventListener;
-    var _org_removeEventListener = document.removeEventListener;
-    document.addEventListener = function(eventName, callback) {
-      var args = Array.prototype.slice.call(arguments, 0);
-      if (eventName.toLowerCase() !== "backbutton") {
-        _org_addEventListener.apply(this, args);
-        return;
-      }
-      if (!anotherBackbuttonHandler) {
-        anotherBackbuttonHandler = callback;
-      }
-    };
-    document.removeEventListener = function(eventName, callback) {
-      var args = Array.prototype.slice.call(arguments, 0);
-      if (eventName.toLowerCase() !== "backbutton") {
-        _org_removeEventListener.apply(this, args);
-        return;
-      }
-      if (anotherBackbuttonHandler === callback) {
-        anotherBackbuttonHandler = null;
-      }
-    };
-

description

If remove this code, the app crashes when you move to another page during the plugin processing something (such as map.animateCamera()).

---

case : code flow is changed

location https://github.com/mapsplugin/cordova-plugin-googlemaps/compare/660b7a3...stalniy:refactor/map-pr?expand=1#diff-48ad1d52e6acef24cc4b997847d030b1L751

-                    domPositions[elemId] = {
-                      pointerEvents: common.getStyle(elem, 'pointer-events'),
-                      isMap: false,
-                      size: common.getDivRect(elem),
-                      zIndex: common.getZIndex(elem),
-                      children: (elemId in domPositions ? domPositions[elemId].children : []),
-                      overflowX: common.getStyle(elem, "overflow-x"),
-                      overflowY: common.getStyle(elem, "overflow-y"),
-                      containMapIDs: (isCached ? domPositions[elemId].containMapIDs : {})
-                    };
-                    domPositions[elemId].containMapIDs[mapId] = 1;
+                    var position = DOM_OBSERVER.addPosition(elem);
+                    position.containMapIDs[mapId] = 1;

description

Where did you write this code?

children: (elemId in domPositions ? domPositions[elemId].children : []),

At this point, the maps plugin needs to keepdomPositions[elemId].children.
Please do not drop any code.

---

case : var options = options || {}; is always {}

location: https://github.com/mapsplugin/cordova-plugin-googlemaps/compare/660b7a3...stalniy:refactor/map-pr?expand=1#diff-293844d2cb003f5cd238a4a4f205358cR39

+DomObserver.prototype.traceDomTree = function(element, params) {
+  if (canSkip(element)) {
+    this.removeDomTree(element);
+    return;
+  }
+
+  var position = this.addPosition(element);
+  var hasMaps = Object.keys(position.containMapIDs).length > 0;
+  var options = options || {};
+  var isForce = options['if'] && this[options['if']](element);
+

---

case : code is changed

location: https://github.com/mapsplugin/cordova-plugin-googlemaps/compare/660b7a3...stalniy:refactor/map-pr?expand=1#diff-293844d2cb003f5cd238a4a4f205358cR42

-  if ((containMapCnt > 0 || isMapChild || domPositions[elemId].pointerEvents === "none") && element.children.length > 0) {
+  if (hasMaps || isForce || position.isMap || position.pointerEvents === "none") {

description

Please do not change the code meaning. Your change kills HTMLInfoWindow.

---

case : var children = element.getElementsByTagName('*');

location: https://github.com/mapsplugin/cordova-plugin-googlemaps/compare/660b7a3...stalniy:refactor/map-pr?expand=1#diff-293844d2cb003f5cd238a4a4f205358cR43

description:

dom.children property contains only DOM elements.

element.getElementsByTagName('*') returns everything, including not necessary tags such as <svg>, and it includes all child elements. This breaks the DOM hierarchy tree of the native side.
(I guess you don't understand how the maps plugin detect the touch currently)

[stalniy]

Thanks for review!

I will go through each case and will try to explain why I did this:

case: background

location: https://github.com/mapsplugin/cordova-plugin-googlemaps/compare/660b7a3...stalniy:refactor/map-pr?expand=1#diff-48ad1d52e6acef24cc4b997847d030b1L124

Description:

The background-image and background-color are more specific versions of background, using background you can specify everything, and by specifying background css property you reset background-image and background-color as well. This behavior works at least in desktop browsers and should work in mobile browsers as well. Check this fiddle to test
Also transparent is the same as rgba(0,0,0,0) but more ancient :) In the current codebase you specified black color with zero transparency, so it's transparent.

---

case: document.body.offsetHeight;

location: https://github.com/mapsplugin/cordova-plugin-googlemaps/compare/660b7a3...stalniy:refactor/map-pr?expand=1#diff-48ad1d52e6acef24cc4b997847d030b1L146

//setTimeout(function() {
  // Webkit redraw mandatory
  // http://stackoverflow.com/a/3485654/697856
  document.body.style.backgroundColor = "rgba(0,0,0,0)";
    //document.body.style.display='none';
    document.body.offsetHeight;
  //document.body.style.display='';
//}, 0);

Description

I removed this just because in stylesheet we already have a CSS rule which sets transparent background to body. That's why I removed it

---

case: null pointer error

location
https://github.com/mapsplugin/cordova-plugin-googlemaps/compare/660b7a3...stalniy:refactor/map-pr?expand=1#diff-48ad1d52e6acef24cc4b997847d030b1L219

-      if (scrollEndTimer) {
-        clearTimeout(scrollEndTimer);
-      }
+    document.body.addEventListener("scroll", function() {
+      clearTimeout(scrollEndTimer);

Description:

Could you please try to remember the use case and I will test it. According to my experience in web development it's safe to pass non-existing timerId in clearTimeout and it should just ignore the call. Also this is what MDN says - https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/clearTimeout#Notes

---

case undefined error on Android 4.4

location https://github.com/mapsplugin/cordova-plugin-googlemaps/compare/660b7a3...stalniy:refactor/map-pr?expand=1#diff-48ad1d52e6acef24cc4b997847d030b1L288

Description:
Fair enough, will revert. Thanks for pointing out!

---

case : android back button

location
https://github.com/mapsplugin/cordova-plugin-googlemaps/compare/660b7a3...stalniy:refactor/map-pr?expand=1#diff-48ad1d52e6acef24cc4b997847d030b1L605

Description
Fair enough. I suggest to move this into separate plugin function and ask people to call that function if they don't use Ionic or don't register backbutton event handler. I think this kind of overrides may be tricky and it would be good not to do this if possible.
So, eventually users of the plugin should run this code, if they don't use ionic

document.addEventListener('backbutton', plugin.google.maps.handleHistory);

What do you think? Cons of this is that it will be a breaking change, but I guess it should not affect majority of users (suspecting that almost everybody uses Ionic). PROS are less magic and webview will execute only what application needs

---

case: code flow is changed

location https://github.com/mapsplugin/cordova-plugin-googlemaps/compare/660b7a3...stalniy:refactor/map-pr?expand=1#diff-48ad1d52e6acef24cc4b997847d030b1L751

Description:
Strange, probably I started refactoring before the change was introduced for children. Anyway will revert.

---

case: var options = options || {}; is always {}

location: https://github.com/mapsplugin/cordova-plugin-googlemaps/compare/660b7a3...stalniy:refactor/map-pr?expand=1#diff-293844d2cb003f5cd238a4a4f205358cR39

Description:
Fair enough, will fix it. I believe that reassigning function arguments leads to confusion and it's harder to follow logic, thus I created a separate variable (it should be var options = params || {}).

---

* case*: code is changed

location: https://github.com/mapsplugin/cordova-plugin-googlemaps/compare/660b7a3...stalniy:refactor/map-pr?expand=1#diff-293844d2cb003f5cd238a4a4f205358cR42

-  if ((containMapCnt > 0 || isMapChild || domPositions[elemId].pointerEvents === "none") && element.children.length > 0) {
+  if (hasMaps || isForce || position.isMap || position.pointerEvents === "none") {

Description:

In my testing from refactor/map branch HTMLInfoWindow worked ok. I will double check this and will explain this change in the next case, as they related.

---

case: var children = element.getElementsByTagName('*');

location: https://github.com/mapsplugin/cordova-plugin-googlemaps/compare/660b7a3...stalniy:refactor/map-pr?expand=1#diff-293844d2cb003f5cd238a4a4f205358cR43

description:

dom.children property contains only DOM elements.
element.getElementsByTagName('*') returns everything, including not necessary tags such as
(I guess you don't understand how the maps plugin detect the touch currently)

Both contains only DOM elements, also both contains SVG elements (created a small fiddle to prove that - https://jsfiddle.net/qxskzc68/6/). element.getElementsByTagName('*') does not return comments, text nodes, and other non-HTMLElement nodes. So, it works the same way as children but returns all children even nested.

This allows to get rid of recursion and speed things up a bit. The refactored code does exactly the same thing as in your codebase. The codepen which proves that - https://codepen.io/Stotska/pen/XZVpgM . I'm a bit simplified the task in codepen (just gather element ids) but you can see that the eventual result is the same.

Let me know if you have other questions!

[wf9a5m75]

I will reply each ones tomorrow, but I reply the last one.

The v2.x needs to keep element orders and hierarchy. element.getElementsByTagName('*') does not keep them.

Did you read the native code?

[stalniy]

hierarchy is kept by the function and according to this SO answer the order is also kept - https://stackoverflow.com/questions/4954003/order-of-the-elements-returned-using-getelementsbytagname

Otherwise codepen wouldn't work because I compare 2 structures using JSON.stringify, if they produced different order then JSON structures would be different.

Update: I saw some native code but I'm not so good in Native development comparing to web

Update 2: link to W3C - https://www.w3.org/TR/REC-DOM-Level-1/level-one-core.html#method-getElementsByTagName

[wf9a5m75]

So, do you see the same result for both $0.children and $0.getElementsByTagName("*")?

<ul id="list">
  <li id="menu1"><a>menu1</a></li>
  <li id="menu2"><a>menu2</a></li>
  <li id="menu3"><a>menu3</a></li>
  <li id="menu4"><a>menu4</a></li>
  <li id="menu5"><a>menu5</a></li>
  <li id="menu6"><a>menu6</a></li>
  <li id="menu7"><a>menu7</a></li>
  <li id="menu8"><a>menu8</a></li>
  <li id="menu9"><a>menu9</a></li>
  <li id="menu10"><a>menu10</a></li>
  <li id="menu11"><a>menu11</a></li>
  <li id="menu12"><a>menu12</a></li>
  <li id="menu13"><a>menu13</a></li>
  <li id="menu14"><a>menu14</a></li>
  <li id="menu15"><a>menu15</a></li>
  <div style="clear:both"></div>
</ul>

var $0=document.getElementById("list");
console.log($0.children);
console.log($0.getElementsByTagName("*"));

[wf9a5m75]

As I promise you, I describe how this plugin inspects DOM tree.

Until the plugin v2.1.x, the maps plugin checks the position, size and z-index (etc) of all html elements.
However this approach has some problems.
I re-studied the z-index property mechanism and implement it with MutationObserver.

---

javascript side

From the maps plugin v2.2.x, the plugin checks the HTML elements that are highest z-index values at the local.

For example, this is the DOM tree of ionic framework that has multiple pages.

This plugin just needs to detect the click point is on map div or not.
Thus, plugin checks only the html elements that contain maps in tree hierarchy.
Other elements are ignored.

The red areas are omitted because no map contains.

---

native side

In the native side, the maps plugin needs to find the elements that highest z-index value at the time.

Since overflow and pointer-events properties are affect to touch action, the native code also needs to consider these values.

The z-index values are depends on the hierarchy from <body>, so the native code searches from <body>.

That's why traceDomTree needs to inspect recursively. getElementsByTagName(*) returns all html elements that child elements.

[stalniy]

Thanks a lot! Now I see where I was wrong. Great :) will change that code

[wf9a5m75]

Ok, next.

The background property.

The webview must be transparent anytime.

Anytime means anycase.
Even if the developer specified background: red !important in other place.

So, how about this case?
This was actual example in the past.

(plugin css  **faster added styles**)
.ion-content {
  background: transparent !important
}

(developer css  **later added styles**)
.ion-content {
  background: red !important
}

---

You need to consider failover when you create your code for public code.

If you just create your code for yourself, background: transparent !important is enough.
But the other people might do different approach for various ways.

So the above case, the people who can not see the map, they report it's bug.
(I say again, this was actual case in the past)

That's why you need to consider failover anytime.

---

So, if the plugin defines three ways, even the another style overrides the background property, the background-color and the background-image properties take over.

(plugin css  **faster added styles**)
.ion-content {
  background: transparent !important
  background-color: transparent !important
  background-image: url()  !important
}

(developer css  **later added styles**)
.ion-content {
  background: red !important
}

---

And reason of rgba(0,0,0,0) instead of transparent is some old Android webview did not recognize the transparent.
I don't remember which one , and I think this one is older than Android 4.4, but it was a bug of webview.

[wf9a5m75]

case: document.body.offsetHeight;

I removed this just because in stylesheet we already have a CSS rule which sets transparent background to body. That's why I removed it

Did you read this url? http://stackoverflow.com/a/3485654/697856

[wf9a5m75]

Could you tell me what this code is for?

var SUPPORTS_PASSIVE = false;
document.createElement("div").addEventListener("test", function() {}, {
  get passive() {
    SUPPORTS_PASSIVE = true;
    return false;
  }
});

[wf9a5m75]

I have been reading your code, but your code is too optimized from the current master code.
(I agree the current master code is messy)

In order to understand your code, I will merge your code little by little.

[wf9a5m75]

Could you tell me what the reason?

- if (!cordova) {
+ if (typeof cordova === 'undefined') {

[wf9a5m75]

And it seems you remove this code.
https://github.com/mapsplugin/cordova-plugin-googlemaps/blob/a0e3dbb74d16e43784c7d8c8756ae3a569429f41/www/googlemaps-cdv-plugin.js#L87-L116

Could you tell me the reason for removing the code, or just you haven't implemented yet?

The reason I implement this code is to prevent changing the zoom scale in browser side.
If user change the zoom scale, the plugin positioning becomes too difficult.
That's why I stick the zoom scale.

If you have your opinion, please tell me it. If no reason, please keep it.

[wf9a5m75]

Regarding of the MapsApi.js, it seems you don't understand how queue work....

[wf9a5m75]

Umm. I can't merge your code at this time, because you don't understand how does the maps plugin and change the code a lot.

[stalniy]

I know, that's why I started this dialog. I will recheck all your comments and will reply on weekend. Currently a bit busy at office.

Will try to fix and correct stuff to work properly :)

[wf9a5m75]

I will split the code of googlemaps-cdv-plugin.js, and add some comments to understand.

[cvaliere]

@stalniy if you're interested, I'm currently also working on rewriting this plugin: https://github.com/BeMyEye/cordova-plugin-googlemaps

my goal is to remove everything that is done by this plugin automatically, because this plugin is sooooooo CPU and memory-intensive that it's totally unusable

it's still work in progress, but if you're interested, let me know and I can give more information

[wf9a5m75]

@cvaliere I can not merge your code, because you remove necessary code a lot.

[stalniy]

@cvaliere I'm very interested in :) I actually would suggest to work together on this and create refactoring plan which needs to be approved by @wf9a5m75

[wf9a5m75]

The most problem of you guys is that you guys only test for simple case.
But more complex cases, such as multiple pages using ionic or framework7, or the apps that have lots of elements, you guys code do not work.
(If you tested, you can't remove the important code)

[cvaliere]

@wf9a5m75 I don't expect you to merge my code. I will maintain my own fork, because your goal is to do everything automagically, whereas mine is to keep high performance in my app.

@stalniy my fork is now fully functional. I've written a README that explains how the official plugin works, what I removed from it, and what are the differences. If you're interested or have any question about it, I'm happy to talk about it. I'm convinced that we can still improve a lot the performance of this plugin, and deduplicate a lot of code.

[wf9a5m75]

Ok, please talk on the forked repo, not here.

[battika]

And please open the possibility for talking there as the issues section is not present there :)

[wf9a5m75]

Sorry for other people. I post a little noisy comment.

---

@cvaliere I decided to prohibit to use this plugin from the next version, because you flame this plugin with fake information.
https://github.com/BeMyEye/cordova-plugin-googlemaps#what-the-official-plugin-does-automatically-and-is-really-not-ok

As I wrote above, the map plugin does not check all html elements anymore from v2.2.0.

resize the Maps every 250ms (Android, ResizeTask) or 100ms (iOS, redrawTimer)

Can you read the code flow? The maps plugin stops the timer thread when no changes are occurred in WebView (you remove the code though).

From my view, you don't really understand how this plugin work. And you also blamed me a lot in the past.

That's why I added an condition in the license file of this plugin.
https://github.com/mapsplugin/cordova-plugin-googlemaps/blob/ef954fa22b74434226acf8b9312b2ec34bfbb4e4/LICENSE#L203-L206

Since you already have a forked repo, you can modify or maintain the code.
However, you can not copy the code from this repo anymore.

I recommend you should create new plugin from scratch.

[wf9a5m75]

@stalniy Regarding of the back button, you overlook the code of this line.

    function onBackButton(e) {
      common.nextTick(putHtmlElements);    <------ here
      if (anotherBackbuttonHandler) {
        // anotherBackbuttonHandler must handle the page moving transaction.
        // The plugin does not take care anymore if another callback is registered.
        anotherBackbuttonHandler(e);
      } else {
        cordova_exec(null, null, 'CordovaGoogleMaps', 'backHistory', []);
      }
    }
    document.addEventListener("backbutton", onBackButton);

Since the maps plugin does not have the all HTML element position and size, the maps plugin needs to check quickly when you press the Android back button.

For example, this is good example to understand.
https://github.com/battika/ionic-gmaps-test

---

When you launch the application, the plugin checks only necessary html elements.

From JS side to the native side, send these information. pgm994053879960 is <page-home> element.

When you open the second page,
although the DOM elements in the first page keeps the __plugindomid attributes, these elements are not checked, because <page-home> is display: none.

Thus, the plugin checks only under the elements of <page-second>.

---

If you tap the Android back button with this status, what the plugin should next?

Of course, the maps plugin needs to recognize the <page-home> again.

But how? When?

Cordova notifies the event only with backbutton event. But here is the point.

Cordova allows only one event listener for the backbutton event.
If you doubt me, try this code without this plugin.

document.addEventListener('backbutton', function() {
  alert("listener1");
});

document.addEventListener('backbutton', function() {
  alert("listener2");
});

I guess the reason is the developer want to display an alert before existing the app sometimes, such as Exit? [yes] | [no]

What would be occurred if the multiple event listener receive the event? Probably lots of troubles.

---

How about if the maps plugin does not receive the event, when should the plugin recheck the DOM tree?

Since the ionic will delete <page-second> element, the MutableObserver will be receive the event.

But if the maps plugin checks the DOM tree every time when remove-nodes event occurred? It's bad performance.

The maps plugin should do removing the cache information only when the remove-nodes event is occurred.

Thus, the backbutton is the perfect timing.

---

So, the app developer wants to use the backbutton event, but this plugin also needs to know the event, but Cordova allows only one event listener.

The solution I figured out is to wrap the addEventListener.
(Well, Cordova itself takes the same approach)

    var anotherBackbuttonHandler = null;
    function onBackButton(e) {
      common.nextTick(putHtmlElements);    // <--- super important!
      if (anotherBackbuttonHandler) {
        // anotherBackbuttonHandler must handle the page moving transaction.
        // The plugin does not take care anymore if another callback is registered.
        anotherBackbuttonHandler(e);
      } else {
        cordova_exec(null, null, 'CordovaGoogleMaps', 'backHistory', []);
      }
    }
    document.addEventListener("backbutton", onBackButton);

    var _org_addEventListener = document.addEventListener;
    document.addEventListener = function(eventName, callback) {
      var args = Array.prototype.slice.call(arguments, 0);
      if (eventName.toLowerCase() !== "backbutton") {
        _org_addEventListener.apply(this, args);
        return;
      }
      if (!anotherBackbuttonHandler) {
        anotherBackbuttonHandler = callback;
      }
    };

Then when you tap the Android backbutton, the maps plugin re-check the DOM tree quickly.

---

Okay, well, what would be occurred if you remove the event handler code?

Well, it's no problem if ionic app.

What?

Because the maps plugin observes class and style __attributes__.

      observer.observe(document.body.parentElement, {
        attributes : true,
        childList: true,
        subtree: true,
        attributeFilter: ['style', 'class']
      });

The example case, @battika executes map.setDiv('map_canvas') in the home page when ionic notifies the ionViewDidEnter event.

  ionViewDidEnter() {
    console.log('HomePage: ionViewDidEnter()');
    if (!this.firstLoad) {
      this.map.setDiv('map_canvas');
    } else {
      this.firstLoad = false;
    }
  }

In that case, map.setDiv() statement adds the _gmaps_cdv_ class to the parent elements of the map div.

But how about if no attributes change, no elements are removed, but change the zIndex of the elements like this?

Well, the framework7 takes this approach.

document.getElementsByTagName("page-second")[0].style.zIndex = 100;
document.getElementsByTagName("page-home")[0].style.display = 1;

In that case, MutableObserver does not tell the event to the plugin code.
Thus, the plugin does not do anything when you tap on the Android backbutton, then tap on the app might not work.

---

That's why this plugin needs to handle the backbutton event.

[wf9a5m75]

Next, parseBoolean.

Regarding of this comment, the parseBoolean is a monkey code.

A lot of useless "utility" methods. For instance, parseBoolean, which transforms "true" and 1 to true and is called every time you use a method which has a boolean parameter. Just in case you code like a monkey.

https://github.com/BeMyEye/cordova-plugin-googlemaps/blob/225ddee9ba2148896526e10991e9a3a9ebce2cc5/README.md#other-small-things

Well, I don't create this function if all JS developers put the correct value.

Some JS developers create overlays with the values from their database directly.

For example, I see like a below code for many times.

getRestaurants().then(function(locationList) {
   for (var i = 0; i < locationList.length; i++) {
      map.addMarker(locationList[i]);
   }
});

Well, if locationList contains like this, what would be occurred?

locationList = [
  {
     "position": {"lat": 35.11, "lng": 137.12},
     "draggable": "true"
  },
 ...
];

In that case, "draggable" is string, but the native code expects boolean, then Android app would be crashed.

    if (opts.has("draggable")) {
      markerOptions.draggable(opts.getBoolean("draggable"));
    }

After that, the JS developer tells me My android app crashes!

Am I kidding you? No, it's serious. I saw many times like a this case.

---

I don't matter to spend my time if the issue is really of this plugin, but spending for my private time for debugging other people problem is not welcome.

However I can not detect what the problem is, without reproduce the issue.
(That's why I ask to share the code on git repo)

That's why I created the parseBoolean function even it's monkey code.

[wf9a5m75]

Of course, if there is no parseBoolean, the browser saves the cpu a little.
But is that really big change?

Technology is faster.
You know what iPhoneX is almost the same as MacMini.

How about older Android? How many percentage of Android 4.3 (no longer supported though)?
Almost gone.

https://developer.android.com/about/dashboards/index.html

You won't feel the gap except if you want to try markers tons of markers.

---

So, my highest priority is to work the plugin safely as much as possible.

In order to archive that, I prefer small monkey code than spending my private time for other people debugging.

[stalniy]

Hi @wf9a5m75

I think that you did is awesome! Despite the fact that there is a room for improvements, I wouldn't be able to suggest them if you didn't create this plugin.

Also I know that I don't understand the whole picture. That's why I stopped my refactoring and decided to ask for your help (guidance) in this job.

@cvaliere, I don't think that blaming somebody will help you to create a better software. And I'd recommend to work In cooperation with others to create awesome things. Isn't this how Open Source was created or how vendors of browsers decided to standardize JS and web?

Let's be polite guys as we are from different cultures and may do mistakes in phrases, words or meanings when we talk using not our native language.

---

@wf9a5m75 I plan to spend some time today looking into your comments and fixing them. Thanks for the guidance, your work and patience!

[cvaliere]

@stalniy I opened the "Issues" section on my fork

I totally agree about the Open Source spirit. As a matter of fact, if I'm talking about my fork, it's precisely because I spent literally weeks reverse-engineering this plugin and finding ways to improve it, and I want to give this time back to the open-source community for those who are interested with performance.

Though, it doesn't mean that we have to blindly say that this plugin is "awesome", because the truth is, it really isn't. The problem is, almost no one knows how bad this plugin is, because at first glance, it just works! It's only when you realize that your app has become really slow and CPU-intensive, and you start to wonder why, that you figure out this is because of this plugin.

If you don't believe me, despite my explanations (https://github.com/BeMyEye/cordova-plugin-googlemaps#why-the-official-plugin-is-not-good), you can make the test yourself: launch your app in a simulator and run a Performance test. You'll see how much CPU is taken by the plugin, it's almost crazy.

I don't blame @wf9a5m75, and I don't think I'm being impolite with him. I have just documented everything that this plugin does behind your back, that is documented nowhere else, and which explains why this plugin is, sorry for the word, bad.

I would be happy to cooperate on this main repository, but @wf9a5m75 and I have already a long history (several years!) of me proposing my help and him constantly refusing. That's why I decided to have my own fork. But @wf9a5m75, if you're willing to work together on a performant version of your plugin, I'm always open to it :)

[wf9a5m75]

@stalniy Thank you for waiting long days.
I refactored the most spaghetti file googlemaps-cdv-plugin.js.

The main part of it is now CordovaGoogleMaps.js.
https://github.com/mapsplugin/cordova-plugin-googlemaps/blob/multiple_maps/www/CordovaGoogleMaps.js

In order to help understanding the code flow, I added lots of comments.
If you are still interested for refactoring, please try it.

[wf9a5m75]

Since the refactoring is done at once, I close this thread. But please send a new pul request with improvement. I really appreciate for your help.