Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Formatting
  • Loading branch information
yaakovschectman committed Jan 11, 2023
commit 5aa4c778a3256bf8bb040b1bcb2292aea7b88cd8
6 changes: 4 additions & 2 deletions shell/platform/common/alert_platform_node_delegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,13 @@ namespace flutter {

class AlertPlatformNodeDelegate : public ui::AXPlatformNodeDelegateBase {
Copy link
Member

Choose a reason for hiding this comment

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

Add a doc comment. (True, we lack a lot in existing code but good to remedy in new code)

Copy link
Member

Choose a reason for hiding this comment

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

Please do a pass, there's several other fields that are also missing comments

public:
explicit AlertPlatformNodeDelegate(ui::AXPlatformNodeDelegate* parent_delegate);
explicit AlertPlatformNodeDelegate(
ui::AXPlatformNodeDelegate* parent_delegate);
~AlertPlatformNodeDelegate();

AlertPlatformNodeDelegate(const AlertPlatformNodeDelegate& other) = delete;
AlertPlatformNodeDelegate operator=(const AlertPlatformNodeDelegate& other) = delete;
AlertPlatformNodeDelegate operator=(const AlertPlatformNodeDelegate& other) =
delete;

void SetText(const std::u16string& text);
Copy link
Member

Choose a reason for hiding this comment

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

Consider void SetText(std::u16string_view text);. It's super lightweight, sometimes better performance, and allows for passing a char16_t* string as well (unlikely as that is).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The AXNodeData methods that this method calls take string& parameters. Am I mistaken that constructing a string from a string_view effectively copies it?

Copy link
Member

Choose a reason for hiding this comment

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

I'm also leaning towards keeping this a string reference, but, the string reference is also copied when AXNodeData stores it as an attribute.

Copy link
Member

Choose a reason for hiding this comment

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

The AXNodeData methods that this method calls take string& parameters. Am I mistaken that constructing a string from a string_view effectively copies it?

Yes - to benefit from this, you'd need any methods this gets passed to to also take a view rather than a string ref. If the current implementation is doing so, then leave as-is; at some point we can do a sweep of the codebase to clean these up across the board.


Expand Down