The NCN pointer is held in a static global currently.  Are you proposing getting rid of that global?  Without getting rid of the global, it may be tricky to ensure safe shutdown (e.g. avoid NCN functions executing on other threads when ~NCN is called).

The static global can still retain a raw pointer to the NCN, right? The callers of NetworkChangeNotifier::Create() are already wrapping the return value in a std::unique_ptr<>. I think we should make the method signature clearer that the ownership is transferred.

There's a global pointer, but that global pointer doesn't own the NCN.  The NCN is owned by the BrowserMainLoop, which keeps it in a unique_ptr.  https://cs.chromium.org/chromium/src/content/browser/browser_main_loop.h?type=cs&sq=package:chromium&l=265

Are you proposing getting rid of the global and switching all NCN references to go through the unique_ptr?

The proposal is just to make Create() return a unique_ptr, since the result of Create() is already expected to be put in a unique ptr.

I guess I'm fine with that.  It does worry me a bit that it's further hiding the fact that all accesses to the NCN are through the racy global pointer.  Perhaps we should add non-static NCN APIs and deprecate the static ones.

That may make sense, once we remove all consumer of NCN outside the network service.  Without static APIs, code also can't accidentally use a net API, rather than the new NetworkService API.

Hi, how can I assign to this issue (I'm new in chromium community)?

Until you're at least a provisional committer, issues can't be assigned to you, so until you become a committer, just say you'll take it (As you have), and that's enough.

If you're working with a committer, they can assign the issue to themselves, to make it clearer the issue is claimed, but that's not necessary (Regardless of whether you're working with someone or not).

Once you land a couple CLs, you can ask a committer you've worked with to request that you be given provisional committer status.

Thanks for quick replay. I'll git it a try.

I'm also new, is there anything left to do on this?

Sorry, this seems like a one-person-bug to me, and jacekskiba recently claimed it.  Unfortunately, I can't think a good starter bug to point you at right now.

Review: https://chromium-review.googlesource.com/c/chromium/src/+/749146

This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

I'd like to handle this issue if no one working.