Skip to content

Conversation

@cmeng-git
Copy link
Contributor

@cmeng-git cmeng-git commented Mar 1, 2023

Add XEP-0070: Verifying HTTP Requests via XMPP support

@cmeng-git cmeng-git changed the base branch from master to 4.4 March 1, 2023 00:04
Copy link
Member

@Flowdalic Flowdalic left a comment

Choose a reason for hiding this comment

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

I usually do not add new features to stable Smack branches, so please adjust the target branch to master.

Copy link
Member

@Flowdalic Flowdalic left a comment

Choose a reason for hiding this comment

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

First round of review. I think especially the manager design needs to be changed.


private final String id;
private final String method;
private final String url;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private final String url;
private final URI url;

I believe this member should be of type URI.

Copy link
Contributor Author

@cmeng-git cmeng-git Mar 25, 2023

Choose a reason for hiding this comment

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

Can this variable be handled as a string all the ways. Otherwise need to do extract steps during xml phrase() and in toXML() method.
In aTalk app implementation, the value is used as string in prompting user to accept or reject the auth request. Do not see the need for app to access the URL directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

The URI class is not intended to be in fields. So please keep as a string

Copy link
Member

Choose a reason for hiding this comment

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

The URI class is not intended to be in fields.

Care to elaborate on that?

Copy link
Contributor

Choose a reason for hiding this comment

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

The URI class has a lot of internal fields, the object size itself is 80 bytes and each internal string adds more and more e.g. URI.create("https://github.com/igniterealtime/Smack/pull/557") took 400 bytes in memory (checked with JOL).
And the parsing itself also needs for CPU.
But there is no any logic that needs the parsed URI. We just need to show the link to a user.

@cmeng-git cmeng-git changed the base branch from 4.4 to master March 27, 2023 02:16

private final String id;
private final String method;
private final String url;
Copy link
Contributor

Choose a reason for hiding this comment

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

The URI class is not intended to be in fields. So please keep as a string

@cmeng-git cmeng-git changed the base branch from master to 4.4 September 26, 2025 02:48
@cmeng-git cmeng-git changed the base branch from 4.4 to master September 26, 2025 02:51
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.

3 participants