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

8275138: WebView: UserAgent string is empty for first request #643

Closed

Conversation

kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Oct 14, 2021

The failure was caused by a change that was done in connection with the WebKit 610.2 update, JDK-8259635. The FrameLoaderClient::userAgent function was changed in WebKit itself to be a const method in WebKit 610.2. We override that method in FrameLoaderClientJava to return the user agent string, which is done by reading the user agent from an instance of the Page class. FrameLoaderClientJava has a m_page field that is lazily initialized whenever it is needed. This lazy initialization can no longer be done from the userAgent method, since it is const, as can be seen from this change that was done as part of WebKit 610.2:

--- a/modules/javafx.web/src/main/native/Source/WebKitLegacy/java/WebCoreSupport/FrameLoaderClientJava.cpp
+++ b/modules/javafx.web/src/main/native/Source/WebKitLegacy/java/WebCoreSupport/FrameLoaderClientJava.cpp
-String FrameLoaderClientJava::userAgent(const URL&)
+String FrameLoaderClientJava::userAgent(const URL&) const
 {
-    return page()->settings().userAgent();
+    if (!m_page)
+        return emptyString();
+    return m_page->settings().userAgent();
 }

Formerly, if the m_page field was uninitialized, FrameLoaderClient::userAgent would have initialized it by the call to the page() function, but since it is now const we can't do that. This means that the user agent string will be empty until some other method is called that initializes the m_page field.

The fix is to initialize that field when the WebPage is created. We can't do it in the constructor, since the needed reference to the Page class isn't yet available, so I added an init method that is called from WebPage::twkInit.

I added a new unit test that fails without the fix and passes with the fix.


Progress

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

Issue

  • JDK-8275138: WebView: UserAgent string is empty for first request

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/643/head:pull/643
$ git checkout pull/643

Update a local copy of the PR:
$ git checkout pull/643
$ git pull https://git.openjdk.java.net/jfx pull/643/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 643

View PR using the GUI difftool:
$ git pr show -t 643

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/643.diff

@kevinrushforth
Copy link
Member Author

/reviewers 2

@kevinrushforth kevinrushforth requested a review from arapte October 14, 2021 12:33
@bridgekeeper
Copy link

bridgekeeper bot commented Oct 14, 2021

👋 Welcome back kcr! 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.

@openjdk openjdk bot added the rfr Ready for review label Oct 14, 2021
@openjdk
Copy link

openjdk bot commented Oct 14, 2021

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

@mlbridge
Copy link

mlbridge bot commented Oct 14, 2021

Webrevs

@aghaisas
Copy link
Collaborator

The fix looks fine. I built and tested on mac.
The new test fails without the fix and passes with it.

Copy link
Member

@arapte arapte 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 too.

@openjdk
Copy link

openjdk bot commented Oct 20, 2021

@kevinrushforth 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:

8275138: WebView: UserAgent string is empty for first request

Reviewed-by: aghaisas, arapte

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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 Oct 20, 2021
@kevinrushforth
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Oct 20, 2021

Going to push as commit d6f78e2.

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

openjdk bot commented Oct 20, 2021

@kevinrushforth Pushed as commit d6f78e2.

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

@kevinrushforth kevinrushforth deleted the 8275138-user-agent branch October 20, 2021 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

3 participants