Skip to content

[google_maps_flutter] Refactor the JavaScript interaction and verify integration tests#1066

Open
seungsoo47 wants to merge 2 commits into
flutter-tizen:masterfrom
seungsoo47:google_maps_flutter-add-integration-tests
Open

[google_maps_flutter] Refactor the JavaScript interaction and verify integration tests#1066
seungsoo47 wants to merge 2 commits into
flutter-tizen:masterfrom
seungsoo47:google_maps_flutter-add-integration-tests

Conversation

@seungsoo47

Copy link
Copy Markdown
Contributor
  • Refactor the JavaScript interaction layer by introducing GoogleMapsJsBridge, replacing direct WebViewController calls.
  • Update webview_flutter to ^4.13.1 and webview_flutter_lwe to ^0.5.0.
  • Verify integration tests pass against upstream google_maps_flutter v2.17.0.

@seungsoo47
seungsoo47 force-pushed the google_maps_flutter-add-integration-tests branch from 370807c to 39802fe Compare July 15, 2026 12:09

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the JavaScript interaction layer of the google_maps_flutter_tizen plugin by introducing a GoogleMapsJsBridge to replace direct WebViewController calls. The review feedback highlights a critical serialization issue in the bridge where arguments are directly interpolated, potentially causing runtime crashes and security vulnerabilities. To resolve this, the reviewer suggests introducing a JsExpression class to handle raw JS code and properly encoding other arguments. Additionally, the feedback recommends marking MapsJsEvent as sealed for exhaustiveness, utilizing the new bridge.addListener abstraction across controllers instead of executing raw JavaScript, and wrapping JS references in JsExpression to prevent them from being treated as string literals.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines 381 to 383
Future<void> _setMap(Object? /*GMap?|StreetViewPanorama?*/ map) async {
await callMethod(this, 'setMap', <Object?>[map]);
await _bridge.callMethod(JsRef(toString()), 'setMap', <Object?>[map]);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Convert map to JsExpression if it is passed as a String (e.g., 'map'), so it refers to the JS variable rather than being treated as a string literal.

Suggested change
Future<void> _setMap(Object? /*GMap?|StreetViewPanorama?*/ map) async {
await callMethod(this, 'setMap', <Object?>[map]);
await _bridge.callMethod(JsRef(toString()), 'setMap', <Object?>[map]);
}
Future<void> _setMap(Object? /*GMap?|StreetViewPanorama?*/ map) async {
final Object? mapArg = map is String ? JsExpression(map) : map;
await _bridge.callMethod(JsRef(toString()), 'setMap', <Object?>[mapArg]);
}

Comment on lines 823 to 825
Future<void> _setMap(Object? map) async {
await callMethod(this, 'setMap', <Object?>[map]);
await _bridge.callMethod(JsRef(toString()), 'setMap', <Object?>[map]);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Convert map to JsExpression if it is passed as a String (e.g., 'map'), so it refers to the JS variable rather than being treated as a string literal.

Suggested change
Future<void> _setMap(Object? map) async {
await callMethod(this, 'setMap', <Object?>[map]);
await _bridge.callMethod(JsRef(toString()), 'setMap', <Object?>[map]);
}
Future<void> _setMap(Object? map) async {
final Object? mapArg = map is String ? JsExpression(map) : map;
await _bridge.callMethod(JsRef(toString()), 'setMap', <Object?>[mapArg]);
}

Comment on lines 479 to 481
Future<void> _setMap(Object? /*GMap?|StreetViewPanorama?*/ map) async {
await callMethod(this, 'setMap', <Object?>[map]);
await _bridge.callMethod(JsRef(toString()), 'setMap', <Object?>[map]);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Convert map to JsExpression if it is passed as a String (e.g., 'map'), so it refers to the JS variable rather than being treated as a string literal.

Suggested change
Future<void> _setMap(Object? /*GMap?|StreetViewPanorama?*/ map) async {
await callMethod(this, 'setMap', <Object?>[map]);
await _bridge.callMethod(JsRef(toString()), 'setMap', <Object?>[map]);
}
Future<void> _setMap(Object? /*GMap?|StreetViewPanorama?*/ map) async {
final Object? mapArg = map is String ? JsExpression(map) : map;
await _bridge.callMethod(JsRef(toString()), 'setMap', <Object?>[mapArg]);
}

Comment on lines +364 to +399
Future<void> setProperty(
JsRef ref,
String property,
Object? value,
) async {
await controller.runJavaScript(
"JSON.stringify($ref['$property'] = $value)",
);
}

@override
Future<Object?> getProperty(JsRef ref, String property) async {
return controller.runJavaScriptReturningResult('$ref.$property');
}

@override
Future<void> callMethod(
JsRef ref,
String method,
List<Object?> args,
) async {
await controller.runJavaScript(
'JSON.stringify($ref.$method.apply($ref, $args))',
);
}

@override
Future<Object?> callMethodReturning(
JsRef ref,
String method,
List<Object?> args,
) async {
return controller.runJavaScriptReturningResult(
'$ref.$method.apply($ref, $args)',
);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

There is a critical correctness and security issue in how arguments are serialized and passed to the WebView. Currently, args are interpolated directly into the JavaScript string using $args (which calls List.toString()). In Dart, ['My String'].toString() produces [My String]. When evaluated in JavaScript, this becomes [My String], which treats My String as raw JavaScript code (referencing undefined variables or causing syntax errors) rather than a string literal. This leads to runtime crashes (e.g., in _setTitle or _setContent) and poses a potential JavaScript injection vulnerability if user-provided strings are passed. To fix this, we should introduce a JsExpression class to explicitly wrap raw JavaScript code/expressions, and update _serializeArg to automatically jsonEncode plain String arguments so they are safely quoted and escaped as JS string literals, while allowing JsRef and JsExpression to be evaluated literally.

  String _serializeArg(Object? arg) {
    if (arg == null) {
      return 'null';
    }
    if (arg is JsRef || arg is JsExpression) {
      return arg.toString();
    }
    if (arg is String) {
      return jsonEncode(arg);
    }
    if (arg is bool || arg is num) {
      return arg.toString();
    }
    return arg.toString();
  }

  @override
  Future<void> setProperty(
    JsRef ref,
    String property,
    Object? value,
  ) async {
    await controller.runJavaScript(
      "JSON.stringify($ref['$property'] = ${_serializeArg(value)})",
    );
  }

  @override
  Future<Object?> getProperty(JsRef ref, String property) async {
    return controller.runJavaScriptReturningResult('$ref.$property');
  }

  @override
  Future<void> callMethod(
    JsRef ref,
    String method,
    List<Object?> args,
  ) async {
    final String serializedArgs = '[' + args.map(_serializeArg).join(', ') + ']';
    await controller.runJavaScript(
      'JSON.stringify($ref.$method.apply($ref, $serializedArgs))',
    );
  }

  @override
  Future<Object?> callMethodReturning(
    JsRef ref,
    String method,
    List<Object?> args,
  ) async {
    final String serializedArgs = '[' + args.map(_serializeArg).join(', ') + ']';
    return controller.runJavaScriptReturningResult(
      '$ref.$method.apply($ref, $serializedArgs)',
    );
  }

Comment on lines 199 to 211
Future<void> _setContent(Object? /*String?|Node?*/ content) async {
await callMethod(this, 'setContent', <Object?>[content]);
await _bridge.callMethod(JsRef(toString()), 'setContent', <Object?>[
content,
]);
}

Future<void> _setPixelOffset(GSize? size) async {
await setProperty(this, 'pixelOffset', size?.toValue());
await _bridge.setProperty(
JsRef(toString()),
'pixelOffset',
size?.toValue(),
);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Wrap size?.toValue() in JsExpression so that it is evaluated as a raw JS object literal rather than being treated as a plain string literal.

Suggested change
Future<void> _setContent(Object? /*String?|Node?*/ content) async {
await callMethod(this, 'setContent', <Object?>[content]);
await _bridge.callMethod(JsRef(toString()), 'setContent', <Object?>[
content,
]);
}
Future<void> _setPixelOffset(GSize? size) async {
await setProperty(this, 'pixelOffset', size?.toValue());
await _bridge.setProperty(
JsRef(toString()),
'pixelOffset',
size?.toValue(),
);
}
Future<void> _setContent(Object? /*String?|Node?*/ content) async {
await _bridge.callMethod(JsRef(toString()), 'setContent', <Object?>[
content,
]);
}
Future<void> _setPixelOffset(GSize? size) async {
await _bridge.setProperty(
JsRef(toString()),
'pixelOffset',
size != null ? JsExpression(size.toValue()) : null,
);
}

Comment on lines +6 to +9
import 'dart:async';
import 'dart:io';

import 'package:webview_flutter/webview_flutter.dart';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Import dart:convert to support jsonEncode for safe argument serialization.

Suggested change
import 'dart:async';
import 'dart:io';
import 'package:webview_flutter/webview_flutter.dart';
import 'dart:async';
import 'dart:convert';
import 'dart:io';
import 'package:webview_flutter/webview_flutter.dart';

Comment on lines +28 to 32
Future<void> _addCircleEvent(GoogleMapsJsBridge bridge) async {
final String command =
"$_circle.addListener('click', (event) => CircleClick.postMessage(JSON.stringify(${_circle?.id})));";
await controller!.runJavaScript(command);
await bridge.runJavaScript(command);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Use the newly introduced bridge.addListener abstraction instead of directly running raw JavaScript. This improves consistency and maintainability.

Suggested change
Future<void> _addCircleEvent(GoogleMapsJsBridge bridge) async {
final String command =
"$_circle.addListener('click', (event) => CircleClick.postMessage(JSON.stringify(${_circle?.id})));";
await controller!.runJavaScript(command);
await bridge.runJavaScript(command);
}
Future<void> _addCircleEvent(GoogleMapsJsBridge bridge) async {
await bridge.addListener(
JsRef(_circle.toString()),
'click',
'CircleClick',
'JSON.stringify(${_circle?.id})',
);
}

Comment on lines +28 to 32
Future<void> _addPolygonEvent(GoogleMapsJsBridge bridge) async {
final String command =
"$_polygon.addListener('click', (event) => PolygonClick.postMessage(JSON.stringify(${_polygon?.id})));";
await controller!.runJavaScript(command);
await bridge.runJavaScript(command);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Use the newly introduced bridge.addListener abstraction instead of directly running raw JavaScript. This improves consistency and maintainability.

Suggested change
Future<void> _addPolygonEvent(GoogleMapsJsBridge bridge) async {
final String command =
"$_polygon.addListener('click', (event) => PolygonClick.postMessage(JSON.stringify(${_polygon?.id})));";
await controller!.runJavaScript(command);
await bridge.runJavaScript(command);
}
Future<void> _addPolygonEvent(GoogleMapsJsBridge bridge) async {
await bridge.addListener(
JsRef(_polygon.toString()),
'click',
'PolygonClick',
'JSON.stringify(${_polygon?.id})',
);
}

Comment on lines +33 to +36
abstract class MapsJsEvent {
/// Const constructor for subclasses.
const MapsJsEvent();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using a sealed class for MapsJsEvent is highly recommended here. Since the _onJsEvent method in GoogleMapsController performs a pattern-matching switch over all subclasses of MapsJsEvent, marking MapsJsEvent as sealed allows the Dart compiler to guarantee exhaustiveness at compile time. This prevents future bugs if new event types are added but not handled in the switch.

Suggested change
abstract class MapsJsEvent {
/// Const constructor for subclasses.
const MapsJsEvent();
}
sealed class MapsJsEvent {
/// Const constructor for subclasses.
const MapsJsEvent();
}

Comment on lines +27 to 31
Future<void> _addGroundOverlayEvent(GoogleMapsJsBridge bridge) async {
final String command =
"$_groundOverlay.addListener('click', (event) => GroundOverlayClick.postMessage(JSON.stringify(${_groundOverlay?.id})));";
await controller!.runJavaScript(command);
await bridge.runJavaScript(command);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Use the newly introduced bridge.addListener abstraction instead of directly running raw JavaScript. This improves consistency and maintainability.

Suggested change
Future<void> _addGroundOverlayEvent(GoogleMapsJsBridge bridge) async {
final String command =
"$_groundOverlay.addListener('click', (event) => GroundOverlayClick.postMessage(JSON.stringify(${_groundOverlay?.id})));";
await controller!.runJavaScript(command);
await bridge.runJavaScript(command);
}
Future<void> _addGroundOverlayEvent(GoogleMapsJsBridge bridge) async {
await bridge.addListener(
JsRef(_groundOverlay.toString()),
'click',
'GroundOverlayClick',
'JSON.stringify(${_groundOverlay?.id})',
);
}

@seungsoo47 seungsoo47 changed the title Google maps flutter add integration tests [google_maps_flutter] Refactor the JavaScript interaction and verify integration tests Jul 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant