Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Element.executeJavaScript #4539

Merged
merged 1 commit into from Aug 31, 2018
Merged

Add Element.executeJavaScript #4539

merged 1 commit into from Aug 31, 2018

Conversation

Legioth
Copy link
Member

@Legioth Legioth commented Aug 17, 2018

Fixes #4538


This change is Reviewable

Copy link
Contributor

@denis-anisimov denis-anisimov left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 8 files at r1.
Reviewable status: all discussions resolved, 0 of 1 LGTMs obtained


flow-server/src/main/java/com/vaadin/flow/dom/Element.java, line 1474 at r1 (raw file):

public void executeJavaScript(

public ?
Without any javadocs?

Copy link
Contributor

@caalador caalador left a comment

Choose a reason for hiding this comment

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

Reviewable status: all discussions resolved, 0 of 1 LGTMs obtained


flow-server/src/main/java/com/vaadin/flow/dom/Element.java, line 1474 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…
public void executeJavaScript(

public ?
Without any javadocs?

Also it should be added to an exception list as ElementTest.publicElementMethodsShouldReturnElement test fails.

@Legioth Legioth force-pushed the element_executeJavaScript branch 3 times, most recently from 89f0682 to 695d459 Compare August 22, 2018 09:28
Copy link
Contributor

@caalador caalador left a comment

Choose a reason for hiding this comment

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

2 JS tests have failed in the build.

  • TimingInfoReporedViwe has a (TypeError) : (intermediate value).apply(... and a Script error: (:0)
  • JsApiGetByIdView has a (TypeError) : Cannot read property 'getByNodeId' of undefined

Reviewable status: all discussions resolved, 0 of 1 LGTMs obtained

@@ -1338,12 +1341,13 @@ public Registration addAttachListener(
// This explicit class instantiation is the workaround
// which fixes a JVM optimization+serialization bug.
// Do not convert to lambda
// Detected under Win7_64 /JDK 1.8.0_152, 1.8.0_172
// Detected under Win7_64 /JDK 1.8.0_152, 1.8.0_172
// see ElementAttributeMap#deferRegistration
new Command() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAJOR Make this anonymous inner class a lambda rule

@@ -1370,12 +1374,13 @@ public Registration addDetachListener(
// This explicit class instantiation is the workaround
// which fixes a JVM optimization+serialization bug.
// Do not convert to lambda
// Detected under Win7_64 /JDK 1.8.0_152, 1.8.0_172
// Detected under Win7_64 /JDK 1.8.0_152, 1.8.0_172
// see ElementAttributeMap#deferRegistration
new Command() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAJOR Make this anonymous inner class a lambda rule

@vaadin-bot
Copy link
Collaborator

SonarQube analysis reported 4 issues

  • MAJOR 2 major
  • INFO 2 info

Watch the comments in this conversation to review them.

2 extra issues

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

  1. INFO Element.java#L121: Do not forget to remove this deprecated code someday. rule
  2. INFO Element.java#L544: Do not forget to remove this deprecated code someday. rule

Copy link
Contributor

@caalador caalador left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 5 of 8 files at r1, 4 of 5 files at r2.
Dismissed @vaadin-bot from 2 discussions.
Reviewable status: all discussions resolved, 0 of 1 LGTMs obtained, and 1 stale

@caalador caalador merged commit 56f2ef2 into master Aug 31, 2018
@caalador caalador deleted the element_executeJavaScript branch August 31, 2018 05:50
@pekam pekam added this to the 1.1.0.beta3 milestone Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants