Disable history sync buttons after click and close dialog optionally [chromium/src : main]
Ryan Sultanem (Gerrit)
Ryan Sultanem added 5 comments![]( "Open in Gerrit")
state == DialogButtonEnableState::kDisabled ? true : false;nit: this can be removed.
return GetParam() == ButtonToClick::kAcceptButtonnit: exhaustive switch statement is clearer here in my opinion.
DialogButtonEnableState::kDisabled));Do you think it is worth attempting to `ClickButton()` again, and expecting nothing. Maybe we could also link the original bug that we had.
{{.test_suffix = "RegularPostClick"}, /*.after_button_click=*/true},Anthi OrfanouI would not have expected a pixel test for that.
Would adding a typscript test be better?
Anthi OrfanouRight now we don't have any such tests, I will try to produce one.
Ryan SultanemI have added a new ui test after making the closing of the dialog optional. I've filed another bug (457370781) for js tests, but so far I fail to instantiate correctly the page under test.
Should I remove the pixel test case?
Thank you for tackling this!
Yes I would say removing the pixel tests is better, as it is not really a "screenshot" like test, and the interactive ui test that you added cover it well enough!
Related details
- Anthi Orfanou
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. |
Anthi Orfanou (Gerrit)
Anthi Orfanou voted and added 1 comment![]( "Open in Gerrit")
Votes added by Anthi Orfanou
| Commit-Queue | +1 |
1 comment
{{.test_suffix = "RegularPostClick"}, /*.after_button_click=*/true},Anthi OrfanouI would not have expected a pixel test for that.
Would adding a typscript test be better?
Anthi OrfanouRight now we don't have any such tests, I will try to produce one.
I have added a new ui test after making the closing of the dialog optional. I've filed another bug (457370781) for js tests, but so far I fail to instantiate correctly the page under test.
Should I remove the pixel test case?
test is added in chrome/browser/ui/signin/signin_view_controller_interactive_uitest.cc
Related details
- Anthi Orfanou
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. |
Anthi Orfanou (Gerrit)
Anthi Orfanou added 4 comments![]( "Open in Gerrit")
state == DialogButtonEnableState::kDisabled ? true : false;nit: this can be removed.
Done
return GetParam() == ButtonToClick::kAcceptButtonnit: exhaustive switch statement is clearer here in my opinion.
Done
DialogButtonEnableState::kDisabled));Do you think it is worth attempting to `ClickButton()` again, and expecting nothing. Maybe we could also link the original bug that we had.
Done
{{.test_suffix = "RegularPostClick"}, /*.after_button_click=*/true},Anthi OrfanouI would not have expected a pixel test for that.
Would adding a typscript test be better?
Anthi OrfanouRight now we don't have any such tests, I will try to produce one.
Anthi OrfanouI have added a new ui test after making the closing of the dialog optional. I've filed another bug (457370781) for js tests, but so far I fail to instantiate correctly the page under test.
Should I remove the pixel test case?
test is added in chrome/browser/ui/signin/signin_view_controller_interactive_uitest.cc
- Ryan Sultanem
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anthi Orfanou (Gerrit)
Anthi Orfanou voted Auto-Submit+1![]( "Open in Gerrit")
| Auto-Submit | +1 |
Related details
- Ryan Sultanem
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Sultanem (Gerrit)
Ryan Sultanem voted and added 5 comments![]( "Open in Gerrit")
Votes added by Ryan Sultanem
| Code-Review | +1 |
5 comments
LGTM % question/confirmation.
Thanks!
history sync optin screen, conditional (thought a dedicatednit: `through`
FRIEND_TEST_ALL_PREFIXES(HistorySyncOptinUIWindowPixelTest, InvokeUi_default);nit: this can be removed, right?
/*browser=*/nullptr, /*should_close_modal_dialog=*/false,Just confirming: Is this expected?
My understanding from the commit message is that this param should be used only for testing for now, but in a later change it will actually be used in production.
2 points:
- If it will not be used in production, maybe we can think of a cleaner solution? Please ignore if it is actually needed.
- Is this changing the behavior for the Profile Picker flow? Or is it ignored for this flow as there are no browsers?
Maybe an idea could be to reuse `HistorySyncOptinLaunchContext` somehow? E.g. for test add a new value maybe? This could be more explicit. Just an idea, I am not fully aware on how this could be used later on.
Assume this as a question/non blocking comment - just sharing ideas.
// possible. Covert back to a check after we verify we no longer hit this.nit: `Convert` :)
Related details
- Anthi Orfanou
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. |
Anthi Orfanou (Gerrit)
Anthi Orfanou voted and added 4 comments![]( "Open in Gerrit")
Votes added by Anthi Orfanou
| Auto-Submit | +1 |
4 comments
history sync optin screen, conditional (thought a dedicatedAnthi Orfanounit: `through`
Done
FRIEND_TEST_ALL_PREFIXES(HistorySyncOptinUIWindowPixelTest, InvokeUi_default);nit: this can be removed, right?
Done
/*browser=*/nullptr, /*should_close_modal_dialog=*/false,Just confirming: Is this expected?
My understanding from the commit message is that this param should be used only for testing for now, but in a later change it will actually be used in production.
2 points:
- If it will not be used in production, maybe we can think of a cleaner solution? Please ignore if it is actually needed.
- Is this changing the behavior for the Profile Picker flow? Or is it ignored for this flow as there are no browsers?
Maybe an idea could be to reuse `HistorySyncOptinLaunchContext` somehow? E.g. for test add a new value maybe? This could be more explicit. Just an idea, I am not fully aware on how this could be used later on.
Assume this as a question/non blocking comment - just sharing ideas.
Yes, the value will be used in production for the case of the signin interception which should not close the dialog.
In the profile picker the value does not matter at all:
If there is a null browser there is no modal dialog attached to the browser to close.
For consistency I can change this to true (previous behaviour) and add a comment that the value is ignored if browser is null. My suggestion would be to turn this to an optional:
- the picker flows would use an nullopt
- the browser flows would have to set it to true, except for the upcoming signin in interception. We can even CHECK in the browser that if the browser is set then the optional must have a value.
What do you think? Maybe I can re-work this with the interception bubble CL?
// possible. Covert back to a check after we verify we no longer hit this.Anthi Orfanounit: `Convert` :)
Done
Related details
- Ryan Sultanem
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. |
Anthi Orfanou (Gerrit)
Anthi Orfanou voted and added 1 comment![]( "Open in Gerrit")
Votes added by Anthi Orfanou
| Auto-Submit | +1 |
1 comment
/*browser=*/nullptr, /*should_close_modal_dialog=*/false,Anthi OrfanouJust confirming: Is this expected?
My understanding from the commit message is that this param should be used only for testing for now, but in a later change it will actually be used in production.
2 points:
- If it will not be used in production, maybe we can think of a cleaner solution? Please ignore if it is actually needed.
- Is this changing the behavior for the Profile Picker flow? Or is it ignored for this flow as there are no browsers?
Maybe an idea could be to reuse `HistorySyncOptinLaunchContext` somehow? E.g. for test add a new value maybe? This could be more explicit. Just an idea, I am not fully aware on how this could be used later on.
Assume this as a question/non blocking comment - just sharing ideas.
Yes, the value will be used in production for the case of the signin interception which should not close the dialog.
In the profile picker the value does not matter at all:
If there is a null browser there is no modal dialog attached to the browser to close.For consistency I can change this to true (previous behaviour) and add a comment that the value is ignored if browser is null. My suggestion would be to turn this to an optional:
- the picker flows would use an nullopt
- the browser flows would have to set it to true, except for the upcoming signin in interception. We can even CHECK in the browser that if the browser is set then the optional must have a value.
What do you think? Maybe I can re-work this with the interception bubble CL?
Converted to an optional, let's revisit when we consume this for a valid use case where we want to skip.
Related details
- Ryan Sultanem
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. |
Anthi Orfanou (Gerrit)
Anthi Orfanou added 1 comment![]( "Open in Gerrit")
/*browser=*/nullptr, /*should_close_modal_dialog=*/false,Anthi OrfanouJust confirming: Is this expected?
My understanding from the commit message is that this param should be used only for testing for now, but in a later change it will actually be used in production.
2 points:
- If it will not be used in production, maybe we can think of a cleaner solution? Please ignore if it is actually needed.
- Is this changing the behavior for the Profile Picker flow? Or is it ignored for this flow as there are no browsers?
Maybe an idea could be to reuse `HistorySyncOptinLaunchContext` somehow? E.g. for test add a new value maybe? This could be more explicit. Just an idea, I am not fully aware on how this could be used later on.
Assume this as a question/non blocking comment - just sharing ideas.
Anthi OrfanouYes, the value will be used in production for the case of the signin interception which should not close the dialog.
In the profile picker the value does not matter at all:
If there is a null browser there is no modal dialog attached to the browser to close.For consistency I can change this to true (previous behaviour) and add a comment that the value is ignored if browser is null. My suggestion would be to turn this to an optional:
- the picker flows would use an nullopt
- the browser flows would have to set it to true, except for the upcoming signin in interception. We can even CHECK in the browser that if the browser is set then the optional must have a value.
What do you think? Maybe I can re-work this with the interception bubble CL?
Converted to an optional, let's revisit when we consume this for a valid use case where we want to skip.
Done
Related details
- Ryan Sultanem
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anthi Orfanou (Gerrit)
Anthi Orfanou voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Ryan Sultanem
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Sultanem (Gerrit)
Ryan Sultanem voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ryan Sultanem
| Code-Review | +1 |
1 comment
/*browser=*/nullptr, /*should_close_modal_dialog=*/false,Anthi OrfanouJust confirming: Is this expected?
My understanding from the commit message is that this param should be used only for testing for now, but in a later change it will actually be used in production.
2 points:
- If it will not be used in production, maybe we can think of a cleaner solution? Please ignore if it is actually needed.
- Is this changing the behavior for the Profile Picker flow? Or is it ignored for this flow as there are no browsers?
Maybe an idea could be to reuse `HistorySyncOptinLaunchContext` somehow? E.g. for test add a new value maybe? This could be more explicit. Just an idea, I am not fully aware on how this could be used later on.
Assume this as a question/non blocking comment - just sharing ideas.
Anthi OrfanouYes, the value will be used in production for the case of the signin interception which should not close the dialog.
In the profile picker the value does not matter at all:
If there is a null browser there is no modal dialog attached to the browser to close.For consistency I can change this to true (previous behaviour) and add a comment that the value is ignored if browser is null. My suggestion would be to turn this to an optional:
- the picker flows would use an nullopt
- the browser flows would have to set it to true, except for the upcoming signin in interception. We can even CHECK in the browser that if the browser is set then the optional must have a value.
What do you think? Maybe I can re-work this with the interception bubble CL?
Anthi OrfanouConverted to an optional, let's revisit when we consume this for a valid use case where we want to skip.
Done
- Anthi Orfanou
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Disable history sync buttons after click and close dialog optionally
This CL makes the closure of the modal dialog, displaying the
history sync optin screen, conditional (through a dedicated
argument). This a a pre-requisite of 418143300, where interacting
with the dialog button should not close the hosting dialog. Here
it facilitates easier testing for the button disabling.
The main functional change is button disabling, after the first click, to prevent double-clicking.
The first click triggers an one-off history sync optin flow.
- M chrome/browser/resources/signin/history_sync_optin/history_sync_optin_app.html.ts
- M chrome/browser/resources/signin/history_sync_optin/history_sync_optin_app.ts
- M chrome/browser/ui/signin/signin_view_controller.cc
- M chrome/browser/ui/signin/signin_view_controller.h
- M chrome/browser/ui/signin/signin_view_controller_delegate.h
- M chrome/browser/ui/signin/signin_view_controller_interactive_uitest.cc
- M chrome/browser/ui/views/profiles/history_sync_optin_ui_browsertest.cc
- M chrome/browser/ui/views/profiles/profile_picker_post_sign_in_adapter.cc
- M chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc
- M chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.h
- M chrome/browser/ui/webui/signin/history_sync_optin/history_sync_optin_handler.cc
- M chrome/browser/ui/webui/signin/history_sync_optin/history_sync_optin_handler.h
- M chrome/browser/ui/webui/signin/history_sync_optin/history_sync_optin_handler_unittest.cc
- M chrome/browser/ui/webui/signin/history_sync_optin/history_sync_optin_ui.cc
- M chrome/browser/ui/webui/signin/history_sync_optin/history_sync_optin_ui.h
- M chrome/browser/ui/webui/signin/history_sync_optin_service.cc
Code-Review: +1 by Ryan Sultanem
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |