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.

Sign up for GitHub

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

Jump to bottom

8254049: Update WebView to public suffix list 2020-04-24 #324

Closed
wants to merge 5 commits into from
Closed

8254049: Update WebView to public suffix list 2020-04-24 #324

wants to merge 5 commits into from

Conversation

arun-joseph
Copy link
Member

@arun-joseph arun-joseph commented Oct 19, 2020 •
edited by openjdk bot
Loading

We should use the public_suffix_list.dat file in the JDK instead. Reading the public_suffix_list.dat file is modified to be similar to DomainName.java. If the file is not present, isPublicSuffix() returns false, which is similar to how WebKit ignores the public suffix check when it is disabled.

Test: Run PublicSuffixesTest.java

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8254049: Update WebView to public suffix list 2020-04-24

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jfx pull/324/head:pull/324
$ git checkout pull/324

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 19, 2020

👋 Welcome back ajoseph! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@arun-joseph arun-joseph marked this pull request as ready for review October 19, 2020 17:26
@openjdk openjdk bot added the rfr Ready for review label Oct 19, 2020
@mlbridge
Copy link

mlbridge bot commented Oct 19, 2020 •
edited
Loading

Webrevs

@kevinrushforth
Copy link
Member

/reviewers 2

@openjdk
Copy link

openjdk bot commented Oct 19, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

kevinrushforth
kevinrushforth reviewed Oct 27, 2020
View reviewed changes
Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

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

I haven't done an exhaustive review yet, but it generally seems to work. I left a few questions below.

modules/javafx.web/src/main/java/com/sun/webkit/network/PublicSuffixes.java Show resolved Hide resolved
modules/javafx.web/src/main/java/com/sun/webkit/network/PublicSuffixes.java Show resolved Hide resolved
modules/javafx.web/src/main/java/com/sun/webkit/network/PublicSuffixes.java Show resolved Hide resolved
kevinrushforth
kevinrushforth reviewed Nov 24, 2020
View reviewed changes
Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

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

I tested it both with and without the public_suffix_list.dat file in the JDK. I left a few inline comments, but this is almost ready to go.

modules/javafx.web/src/main/java/com/sun/webkit/network/PublicSuffixes.java Outdated Show resolved Hide resolved
modules/javafx.web/src/main/java/com/sun/webkit/network/PublicSuffixes.java Outdated Show resolved Hide resolved
modules/javafx.web/src/main/java/com/sun/webkit/network/PublicSuffixes.java Outdated Show resolved Hide resolved
kevinrushforth
kevinrushforth reviewed Nov 30, 2020
View reviewed changes
Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

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

I verified that the doPrivileged change allows it to work with a security manager. The only issue I now see is related to the log warning.

modules/javafx.web/src/main/java/com/sun/webkit/network/PublicSuffixes.java Outdated Show resolved Hide resolved
kevinrushforth
kevinrushforth approved these changes Dec 1, 2020
View reviewed changes
Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

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

Looks good.

guruhb
guruhb approved these changes Dec 3, 2020
View reviewed changes
Copy link
Contributor

@guruhb guruhb left a comment

Choose a reason for hiding this comment

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

Looks good to me

@openjdk
Copy link

openjdk bot commented Dec 3, 2020

@arun-joseph This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8254049: Update WebView to public suffix list 2020-04-24

Reviewed-by: kcr, ghb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 36 new commits pushed to the master branch:

  • bd51089: 8256978: GitHub actions: build fails on Linux due to missing package
  • a394fab: 8199592: Control labels truncated at certain DPI scaling levels
  • 2fe8677: 8256184: Openjfx build broken (Eclipse)
  • 8e03018: 8256649: Parameterized tests must not use instances as parameters
  • 767a1fd: 8256686: GitHub actions: build fails due to upgraded MSVC compiler
  • a128952: 8177945: Single cell selection flickers when adding data to TableView
  • dd22cd2: 8242861: Update ImagePattern to apply SVG pattern transforms
  • ade3227: 8254569: Remove hard dependency on Dispman in Monocle fb rendering
  • 08d8a98: 8237491: [Linux] Undecorated stage cannot be maximized
  • 92804f3: 8255723: Gtk glass backend should run with Gtk+ 3.8 (minimum)
  • ... and 26 more: https://git.openjdk.java.net/jfx/compare/e856a58fbbd6104d768c54063a9dba897ea0e5e0...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Ready to be integrated label Dec 3, 2020
@arun-joseph
Copy link
Member Author

/integrate

@openjdk openjdk bot closed this Dec 3, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Ready to be integrated rfr Ready for review labels Dec 3, 2020
@openjdk
Copy link

openjdk bot commented Dec 3, 2020

@arun-joseph Since your change was applied there have been 36 commits pushed to the master branch:

  • bd51089: 8256978: GitHub actions: build fails on Linux due to missing package
  • a394fab: 8199592: Control labels truncated at certain DPI scaling levels
  • 2fe8677: 8256184: Openjfx build broken (Eclipse)
  • 8e03018: 8256649: Parameterized tests must not use instances as parameters
  • 767a1fd: 8256686: GitHub actions: build fails due to upgraded MSVC compiler
  • a128952: 8177945: Single cell selection flickers when adding data to TableView
  • dd22cd2: 8242861: Update ImagePattern to apply SVG pattern transforms
  • ade3227: 8254569: Remove hard dependency on Dispman in Monocle fb rendering
  • 08d8a98: 8237491: [Linux] Undecorated stage cannot be maximized
  • 92804f3: 8255723: Gtk glass backend should run with Gtk+ 3.8 (minimum)
  • ... and 26 more: https://git.openjdk.java.net/jfx/compare/e856a58fbbd6104d768c54063a9dba897ea0e5e0...master

Your commit was automatically rebased without conflicts.

Pushed as commit c8d9c94.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@arun-joseph arun-joseph deleted the 8254049 branch December 3, 2020 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guruhb guruhb guruhb approved these changes

@kevinrushforth kevinrushforth kevinrushforth approved these changes

Assignees
No one assigned
Labels
integrated Pull request has been integrated
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@arun-joseph @kevinrushforth @guruhb