Conversation
ARTI1208
force-pushed
the
jcef
branch
2 times, most recently
from
1178d53 to
2a6ef9c
Compare
Codecov Report
@@ Coverage Diff @@
## master #130 +/- ##
============================================
- Coverage 25.17% 22.28% -2.90%
Complexity 63 63
============================================
Files 132 141 +9
Lines 3893 4434 +541
Branches 407 470 +63
============================================
+ Hits 980 988 +8
- Misses 2892 3425 +533
Partials 21 21
Continue to review full report at Codecov.
|
|
nice！ |
SerVB
left a comment
SerVB
left a comment
There was a problem hiding this comment.
Wow, that was big and I expect this will be difficult to support... Probably we could make the logic much concise if there was a JCEF factory that we can just inject? We could ask for that the platform team to simplify our code in the future...
| * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE | ||
| * SOFTWARE. | ||
| */ | ||
| package org.jetbrains.projector.common |
There was a problem hiding this comment.
Looks like this is no a right place to add these 3 files. projector-common is meant for code that is shared between the server and the client.
| @UseProjectorLoader | ||
| public object CefHandlers { | ||
|
|
||
| @JvmStatic |
There was a problem hiding this comment.
Why do we use JvmStatic for private members of this class?
And for public ones?
We should add a comment on that.
| ) : ClientEvent() | ||
|
|
||
| @OptIn(ExperimentalJsExport::class) | ||
| @JsExport |
There was a problem hiding this comment.
| @JsExport | |
| @JsExport // used from JS code |
There was a problem hiding this comment.
However, fmpov, it's pretty scary dependency. Commented suggestions in ProjectorJcefBrowser.kt.
| const prjWebPackage = prjWebModule.org.jetbrains.projector.client.web; | ||
| const addEvent = prjWebPackage.state.ClientAction.AddEvent; | ||
| const jcefEvent = prjWebModule.${ClientJcefEvent::class.java.name}; | ||
| prjWebPackage.Application.fireAction(new addEvent(new jcefEvent($id, '$jsQueryFunctionName', obj.request))); |
There was a problem hiding this comment.
So because of that, we:
- Make
Applicationsingleton; - Add
JsExporttoAddEventandClientJcefEvent?
It's quite a scary dependency to me. Moreover, there is still many hard code like packages that isn't checked at compile time.
Can we somehow apply these changes just from the web client side? That could be cleaner...
| ) | ||
| ) { | ||
|
|
||
| constructor(x: Int, y: Int) : this(x.toDouble(), y.toDouble()) |
There was a problem hiding this comment.
In many other places we do Point(x.toDouble(), y.toDouble()), so probably we should adjust those places as well or avoid adding one more constructor...
4 participants
Server changes: JetBrains/projector-server#111