Migrate print preview handler methods to ash::LocalPrinter [chromium/src : main]
Qijiang Fan (Gerrit)
Qijiang Fan abandoned this change.
Qijiang Fan abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Qijiang Fan (Gerrit)
Qijiang Fan added 1 comment![]( "Open in Gerrit")
amd64-generic failures are unrelated (another wip bug by gardeners)
Related details
- Georg Neis
- Hidehiko Abe
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Georg Neis (Gerrit)
Georg Neis added 16 comments![]( "Open in Gerrit")
Haven't looked at the tests yet.
`ash::LocalPrinter` and implementing them in `ash::LocalPrinterImpl`.You're also adding `GetPrinter` there, but it doesn't directly come from crosapi. Could you explain in the commit message?
// Guest users are not supported for all functions.This comment doesn't make much sense here.
On the other hand, why not mention that all methods just crash immediately? Maybe as a comment on the class in line 16.
GetPrinterCallback callback) override;Please return the result directly (removing the callback parameter).
ash::CupsPrintersManagerFactory::GetForBrowserContext(context);In some functions you have a DCHECK(printers_manager) at this point. In some you have a CHECK. Here you don't have either. Let's be consistent.
ash::BrowserContextHelper::Get()->GetBrowserContextByAccountId(
accountId));Let's do this only once (line 279).
ipp_client_info_calculator_;How important is it that this gets created lazily?
void GetUsernamePerPolicy(AshJobSettingsCallback callback,At least the first part of the comment is no longer true for GetUsernamePerPolicy, and also not for GetIppClientInfoOnPrinterFetched I believe.
bool IsActiveUserAffiliated() {Is the TODO from the original function not relevant here?
const std::optional<std::string>& oauth_token) {Prefer base/types/optional_ref.h.
cros_local_printer_->GetUsernamePerPolicy(Can we delete this function from crosapi now?
user_manager::UserManager::Get()->GetPrimaryUser()->display_email();Deprecated, see components/user_manager/user_manager.h
std::move(add_profile_username_callback).Run(std::make_optional(username));nit: redundant
Related details
- Hidehiko Abe
- Qijiang Fan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Qijiang Fan (Gerrit)
Qijiang Fan added 15 comments![]( "Open in Gerrit")
`ash::LocalPrinter` and implementing them in `ash::LocalPrinterImpl`.You're also adding `GetPrinter` there, but it doesn't directly come from crosapi. Could you explain in the commit message?
Done
This comment doesn't make much sense here.
On the other hand, why not mention that all methods just crash immediately? Maybe as a comment on the class in line 16.
deleted in fake. account to be revisited like all other functions.
You can delete these includes.
done
Please return the result directly (removing the callback parameter).
done
ash::CupsPrintersManagerFactory::GetForBrowserContext(context);In some functions you have a DCHECK(printers_manager) at this point. In some you have a CHECK. Here you don't have either. Let's be consistent.
Done
ash::BrowserContextHelper::Get()->GetBrowserContextByAccountId(
accountId));Let's do this only once (line 279).
done
How important is it that this gets created lazily?
the key is actually to workaround interactive_ui_tests and browser_tests.
IppClientInfoCalculator::Create() will crash on DCHECK for not being running on ChromeOS. (and the browser_tests that uses extension to print pdf didn't goes to the path that really needs ippclientinfo either)
crosapi did the same thing
void GetUsernamePerPolicy(AshJobSettingsCallback callback,At least the first part of the comment is no longer true for GetUsernamePerPolicy, and also not for GetIppClientInfoOnPrinterFetched I believe.
done
// connection is unavailable.Qijiang FanPlease update.
done
Is the TODO from the original function not relevant here?
user_manager should always be there at this step.
(previous steps calls to ash::LocalPrinter used account too)
const std::optional<std::string>& oauth_token) {Qijiang FanPrefer base/types/optional_ref.h.
done
cros_local_printer_->GetUsernamePerPolicy(Can we delete this function from crosapi now?
there are more to delete (funcs in this CL, some observer stuff), will be deleted together at next CL
user_manager::UserManager::Get()->GetPrimaryUser()->display_email();Qijiang FanDeprecated, see components/user_manager/user_manager.h
session does not contain information for display_email
changed to session_manager for other callers that only needs account id
std::move(add_profile_username_callback).Run(std::make_optional(username));Qijiang Fannit: redundant
done
Related details
- Georg Neis
- Hidehiko Abe
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Georg Neis (Gerrit)
Georg Neis voted and added 5 comments![]( "Open in Gerrit")
Votes added by Georg Neis
| Code-Review | +1 |
5 comments
`ash::LocalPrinter` and implementing them in `ash::LocalPrinterImpl`.Qijiang FanYou're also adding `GetPrinter` there, but it doesn't directly come from crosapi. Could you explain in the commit message?
Done
I meant explaining where it comes from, as it was not in the original interface.
// Guest users are not supported for all functions.Qijiang FanThis comment doesn't make much sense here.
On the other hand, why not mention that all methods just crash immediately? Maybe as a comment on the class in line 16.
deleted in fake. account to be revisited like all other functions.
I still think there should be a sentence in this header saying what FakeLocalPrinter does, namely just crash in each method.
ipp_client_info_calculator_;Qijiang FanHow important is it that this gets created lazily?
the key is actually to workaround interactive_ui_tests and browser_tests.
IppClientInfoCalculator::Create() will crash on DCHECK for not being running on ChromeOS. (and the browser_tests that uses extension to print pdf didn't goes to the path that really needs ippclientinfo either)
crosapi did the same thing
I see, please add a code comment about that.
user_manager::UserManager::Get()->GetPrimaryUser()->display_email();Qijiang FanDeprecated, see components/user_manager/user_manager.h
session does not contain information for display_email
changed to session_manager for other callers that only needs account id
First get the session, then get the user via `user_manager::UserManager::Get()->FindUser(session->account_id())`.
Related details
- Hidehiko Abe
- Qijiang Fan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hidehiko Abe (Gerrit)
Hidehiko Abe added 12 comments![]( "Open in Gerrit")
content::BrowserContext* context =could you avoid duplicating the code? Instead, could you move the code, i.e., at least remove the original function?
// These functions chain together, potentially calling the corresponding
// `LocalPrinter` function, `IppClientInfoCalculator` function,
// `user_manager`, etc., and then convert the result to a job setting, add itcould you keep the definition of the function behavior more specific and concrete?
// Creates an instance suitable for testing with the `ash::LocalPrinter` andcould you document the lifetime requirement of local_printer?
}
}nit:
```
} // namespace printing
} // namespace ash
```
ipp_client_info_calculator_;Qijiang FanHow important is it that this gets created lazily?
Georg Neisthe key is actually to workaround interactive_ui_tests and browser_tests.
IppClientInfoCalculator::Create() will crash on DCHECK for not being running on ChromeOS. (and the browser_tests that uses extension to print pdf didn't goes to the path that really needs ippclientinfo either)
crosapi did the same thing
I see, please add a code comment about that.
I'm confused. Should this class be instantiated only on ChromeOS platform...?
if (!user_manager::UserManager::IsInitialized() ||
!user_manager::UserManager::Get()->IsUserLoggedIn()) {please check by using session_manager::SessionManager, instead.
Profile* profile = ProfileManager::GetPrimaryUserProfile();this should be covered by the GetProfilePrefs() to be used below.
const std::string username =could you move this into if block below?
if (profile->GetPrefs()->GetBoolean(please use user_manager::User::GetProfilePrefs()
IsActiveUserAffiliated()) {this checking session may be different from primary one used above at least for this code.
Could you leave TODO to revisit here to fix the twisted code?
class TestLocalPrinter : public ash::FakeLocalPrinter {this test impl should be embedded in your new FakeLocalPrinter?
In general, the behavior of the fake should be implemented in the fake class put next to the original class to avoid divergence of the behavior for variation of test cases.
class MockIppClientInfoCalculatorcan we avoid new gmock use?
Related details
- Qijiang Fan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Qijiang Fan (Gerrit)
Qijiang Fan added 17 comments![]( "Open in Gerrit")
`ash::LocalPrinter` and implementing them in `ash::LocalPrinterImpl`.Qijiang FanYou're also adding `GetPrinter` there, but it doesn't directly come from crosapi. Could you explain in the commit message?
Georg NeisDone
I meant explaining where it comes from, as it was not in the original interface.
Done
// Guest users are not supported for all functions.Qijiang FanThis comment doesn't make much sense here.
On the other hand, why not mention that all methods just crash immediately? Maybe as a comment on the class in line 16.
Georg Neisdeleted in fake. account to be revisited like all other functions.
I still think there should be a sentence in this header saying what FakeLocalPrinter does, namely just crash in each method.
done
// Guest users are not supported for all functions.Qijiang FanThis comment doesn't make much sense here.
On the other hand, why not mention that all methods just crash immediately? Maybe as a comment on the class in line 16.
Georg Neisdeleted in fake. account to be revisited like all other functions.
I still think there should be a sentence in this header saying what FakeLocalPrinter does, namely just crash in each method.
Done
could you avoid duplicating the code? Instead, could you move the code, i.e., at least remove the original function?
like other migrations before, removal will be done at separate CLs soon confirmed without regression for a few days.
// These functions chain together, potentially calling the corresponding
// `LocalPrinter` function, `IppClientInfoCalculator` function,
// `user_manager`, etc., and then convert the result to a job setting, add itcould you keep the definition of the function behavior more specific and concrete?
Done
// Creates an instance suitable for testing with the `ash::LocalPrinter` andcould you document the lifetime requirement of local_printer?
what do you mean by document lifetime requiremnt of local_printer?
local_printer is always there (almost persistent) after ash initialization, while LocalPrinterHandlerChromeos is only at print-ui lifecycle (very short)
also CreateForTesting is for testing where SetUp setsup everything in correct order, and TearDown deletes them.
nit:
```
} // namespace printing
} // namespace ash
```
Done
ipp_client_info_calculator_;Qijiang FanHow important is it that this gets created lazily?
Georg Neisthe key is actually to workaround interactive_ui_tests and browser_tests.
IppClientInfoCalculator::Create() will crash on DCHECK for not being running on ChromeOS. (and the browser_tests that uses extension to print pdf didn't goes to the path that really needs ippclientinfo either)
crosapi did the same thing
Hidehiko AbeI see, please add a code comment about that.
I'm confused. Should this class be instantiated only on ChromeOS platform...?
BUILDFLAG(OS_CHROMEOS) does not equal to IsRunningOnChromeOS()
the later checks it is really running on ChromeOS. since IppClientInfoCalculator needs the ChromeOS version info
if (!user_manager::UserManager::IsInitialized() ||
!user_manager::UserManager::Get()->IsUserLoggedIn()) {please check by using session_manager::SessionManager, instead.
Done
Profile* profile = ProfileManager::GetPrimaryUserProfile();this should be covered by the GetProfilePrefs() to be used below.
done
could you move this into if block below?
done
user_manager::UserManager::Get()->GetPrimaryUser()->display_email();Qijiang FanDeprecated, see components/user_manager/user_manager.h
Georg Neissession does not contain information for display_email
changed to session_manager for other callers that only needs account id
First get the session, then get the user via `user_manager::UserManager::Get()->FindUser(session->account_id())`.
done
please use user_manager::User::GetProfilePrefs()
done
this checking session may be different from primary one used above at least for this code.
Could you leave TODO to revisit here to fix the twisted code?
Done
this checking session may be different from primary one used above at least for this code.
Could you leave TODO to revisit here to fix the twisted code?
done
this test impl should be embedded in your new FakeLocalPrinter?
In general, the behavior of the fake should be implemented in the fake class put next to the original class to avoid divergence of the behavior for variation of test cases.
i would leave fake a NOTREACHED() for now (the same as what crosapi does) the reason is:
- there is indeed difference of fake implementations for different tests. (some just return a hardcoded values, some have extra helper to set values, some throw errors)
- the "parent" fake is just a stub that overrides all virtual functions so we don't need to add override for (each test's) unused functions.
can we avoid new gmock use?
mock is cleaner than fake with initialized return values + call counts members.
is mock now banned?
Related details
- Georg Neis
- Hidehiko Abe
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Georg Neis (Gerrit)
Georg Neis voted Code-Review+1![]( "Open in Gerrit")
- Hidehiko Abe
- Qijiang Fan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hidehiko Abe (Gerrit)
Hidehiko Abe added 6 comments![]( "Open in Gerrit")
content::BrowserContext* context =Qijiang Fancould you avoid duplicating the code? Instead, could you move the code, i.e., at least remove the original function?
like other migrations before, removal will be done at separate CLs soon confirmed without regression for a few days.
Marked as unresolved.
So, it's difficult to see what are carried...?
// These functions chain together, potentially calling the corresponding
// `LocalPrinter` function, `IppClientInfoCalculator` function,
// `user_manager`, etc., and then convert the result to a job setting, add itQijiang Fancould you keep the definition of the function behavior more specific and concrete?
Done
I'd be more concrete.
What do you mean by "potentially"? What is corresponding ... function? E.g. for GetUsernamePerPolicy, there's no trivially corresponding methods. Also, these are more likely implementation details. Could you comment "what is the semantics" of each function?
// Creates an instance suitable for testing with the `ash::LocalPrinter` andQijiang Fancould you document the lifetime requirement of local_printer?
what do you mean by document lifetime requiremnt of local_printer?
local_printer is always there (almost persistent) after ash initialization, while LocalPrinterHandlerChromeos is only at print-ui lifecycle (very short)
also CreateForTesting is for testing where SetUp setsup everything in correct order, and TearDown deletes them.
Regardless of the current use, local_printer must be non-null and outlive the returned instance. Could you explicitly document in the comment?
user_manager::UserManager::Get()->FindUser(account)->display_email();could you avoid searching twice?
class TestLocalPrinter : public ash::FakeLocalPrinter {Qijiang Fanthis test impl should be embedded in your new FakeLocalPrinter?
In general, the behavior of the fake should be implemented in the fake class put next to the original class to avoid divergence of the behavior for variation of test cases.
i would leave fake a NOTREACHED() for now (the same as what crosapi does) the reason is:
- there is indeed difference of fake implementations for different tests. (some just return a hardcoded values, some have extra helper to set values, some throw errors)
- the "parent" fake is just a stub that overrides all virtual functions so we don't need to add override for (each test's) unused functions.
Marked as unresolved.
there is indeed difference of fake implementations for different tests.
definitely this is the reason we shouldn't have overriding in each test. The fake should "fake" the behavior of the real instance, and it shouldn't depend on the test cases. If some test would like to exercise the error situation, then fake should have an ability to mimic the "service error".
class MockIppClientInfoCalculatorQijiang Fancan we avoid new gmock use?
mock is cleaner than fake with initialized return values + call counts members.
is mock now banned?
Marked as unresolved.
As described above, mock describes the service behavior in test, which increases maintenance cost unnecessarily. Let's avoid such.
Related details
- Qijiang Fan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Qijiang Fan (Gerrit)
Qijiang Fan added 5 comments![]( "Open in Gerrit")
content::BrowserContext* context =Qijiang Fancould you avoid duplicating the code? Instead, could you move the code, i.e., at least remove the original function?
Hidehiko Abelike other migrations before, removal will be done at separate CLs soon confirmed without regression for a few days.
Marked as unresolved.
So, it's difficult to see what are carried...?
Done
// These functions chain together, potentially calling the corresponding
// `LocalPrinter` function, `IppClientInfoCalculator` function,
// `user_manager`, etc., and then convert the result to a job setting, add itQijiang Fancould you keep the definition of the function behavior more specific and concrete?
Hidehiko AbeDone
I'd be more concrete.
What do you mean by "potentially"? What is corresponding ... function? E.g. for GetUsernamePerPolicy, there's no trivially corresponding methods. Also, these are more likely implementation details. Could you comment "what is the semantics" of each function?
done
user_manager::UserManager::Get()->FindUser(account)->display_email();could you avoid searching twice?
Done
class TestLocalPrinter : public ash::FakeLocalPrinter {Qijiang Fanthis test impl should be embedded in your new FakeLocalPrinter?
In general, the behavior of the fake should be implemented in the fake class put next to the original class to avoid divergence of the behavior for variation of test cases.
Hidehiko Abei would leave fake a NOTREACHED() for now (the same as what crosapi does) the reason is:
- there is indeed difference of fake implementations for different tests. (some just return a hardcoded values, some have extra helper to set values, some throw errors)
- the "parent" fake is just a stub that overrides all virtual functions so we don't need to add override for (each test's) unused functions.
Marked as unresolved.
there is indeed difference of fake implementations for different tests.
definitely this is the reason we shouldn't have overriding in each test. The fake should "fake" the behavior of the real instance, and it shouldn't depend on the test cases. If some test would like to exercise the error situation, then fake should have an ability to mimic the "service error".
how about leaving it to the next cl. considering the size of current CL and works are needed to unify the behavior.
TODO added in parent FakeLocalPrinter class.
class MockIppClientInfoCalculatorQijiang Fancan we avoid new gmock use?
Hidehiko Abemock is cleaner than fake with initialized return values + call counts members.
is mock now banned?
Marked as unresolved.
As described above, mock describes the service behavior in test, which increases maintenance cost unnecessarily. Let's avoid such.
- Georg Neis
- Hidehiko Abe
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Qijiang Fan (Gerrit)
Qijiang Fan added 1 comment![]( "Open in Gerrit")
// Creates an instance suitable for testing with the `ash::LocalPrinter` andcould you document the lifetime requirement of local_printer?
Hidehiko Abewhat do you mean by document lifetime requiremnt of local_printer?
local_printer is always there (almost persistent) after ash initialization, while LocalPrinterHandlerChromeos is only at print-ui lifecycle (very short)
also CreateForTesting is for testing where SetUp setsup everything in correct order, and TearDown deletes them.
Regardless of the current use, local_printer must be non-null and outlive the returned instance. Could you explicitly document in the comment?
Done
Related details
- Georg Neis
- Hidehiko Abe
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Qijiang Fan (Gerrit)
Qijiang Fan voted![]( "Open in Gerrit")
| Auto-Submit | +1 |
| Commit-Queue | +1 |
Related details
- Georg Neis
- Hidehiko Abe
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Qijiang Fan (Gerrit)
Qijiang Fan voted Auto-Submit+0![]( "Open in Gerrit")
| Auto-Submit | +0 |
Georg Neis (Gerrit)
Georg Neis voted Code-Review+1![]( "Open in Gerrit")
- Hidehiko Abe
- Qijiang Fan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hidehiko Abe (Gerrit)
Hidehiko Abe voted and added 4 comments![]( "Open in Gerrit")
Votes added by Hidehiko Abe
| Code-Review | +1 |
4 comments
result =
token.has_value() ? std::make_optional(*token) : std::nullopt;using base::test::TestFuture will simplify the code structure, IIUC.
auto* profile_prefs = user ? user->GetProfilePrefs() : nullptr;nit: 'user' should be always available. Maybe CHECK?
Related details
- Qijiang Fan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Qijiang Fan (Gerrit)
Qijiang Fan added 3 comments![]( "Open in Gerrit")
nit/style: just one empty line is enough.
Done
result =
token.has_value() ? std::make_optional(*token) : std::nullopt;using base::test::TestFuture will simplify the code structure, IIUC.
done and other usages of RunLoop too.
but we still need to copy the value since the underlying object in optional_ref is out of scope at Take() time.
while GetPrinters still uses std::optional&, future receiving std::optional will just have a copied value.
auto* profile_prefs = user ? user->GetProfilePrefs() : nullptr;nit: 'user' should be always available. Maybe CHECK?
- Georg Neis
- Hidehiko Abe
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Georg Neis (Gerrit)
Georg Neis voted Code-Review+1![]( "Open in Gerrit")
- Hidehiko Abe
- Qijiang Fan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |