[Part 2] Hook up back button to back-to-opener functionality [chromium/src : main]
Diana Qu (Gerrit)
Diana Qu added 1 comment![]( "Open in Gerrit")
Sending this one out~ I had to do some refactor in browser_command.cc. Let me know if you think that’s okay.
Related details
- Darryl James
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darryl James (Gerrit)
Darryl James voted and added 4 comments![]( "Open in Gerrit")
Votes added by Darryl James
| Code-Review | +1 |
4 comments
lgtm % small comments; Nice work!
if (CanGoBackToOpener(web_contents)) {
tabs::TabInterface* tab =
tabs::TabInterface::MaybeGetFromContents(web_contents);optional: It might be worth finding a way to only query for the tab and controller once. I noticed that CanGoBackToOpener and the code within this block re-query for the tab / controller.
I don't believe this is an expensive operation so I am fine with this as is 👍
static_cast<BrowserWindowInterface*>(browser()));Is the cast needed here? I would assume no since `Browser` extends `BrowserWindowInterface` already 👍
}super nit: new line after this one to separate the test below
Related details
- Diana Qu
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. |
Diana Qu (Gerrit)
Diana Qu added 4 comments![]( "Open in Gerrit")
Hi Darryl! I updated a bit base on your comment! Could you take another look when you get a chance?
Also do you mind add another reviewer that you think would be good to take a look at this?
(Since part 1 is still pending Takashi's review. So no rush on this one.)
if (CanGoBackToOpener(web_contents)) {
tabs::TabInterface* tab =
tabs::TabInterface::MaybeGetFromContents(web_contents);optional: It might be worth finding a way to only query for the tab and controller once. I noticed that CanGoBackToOpener and the code within this block re-query for the tab / controller.
I don't believe this is an expensive operation so I am fine with this as is 👍
I created another helper with different parameter!
static_cast<BrowserWindowInterface*>(browser()));Is the cast needed here? I would assume no since `Browser` extends `BrowserWindowInterface` already 👍
Ah that make sense. Updated all test around cast.
super nit: new line after this one to separate the test below
- Darryl James
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. |
Darryl James (Gerrit)
Darryl James voted and added 4 comments![]( "Open in Gerrit")
Votes added by Darryl James
| Code-Review | +1 |
4 comments
if (CanGoBackToOpener(web_contents)) {
tabs::TabInterface* tab =
tabs::TabInterface::MaybeGetFromContents(web_contents);Diana Quoptional: It might be worth finding a way to only query for the tab and controller once. I noticed that CanGoBackToOpener and the code within this block re-query for the tab / controller.
I don't believe this is an expensive operation so I am fine with this as is 👍
I created another helper with different parameter!
Sweet!
Because you have inverted the condition by checking the tab first you can also consider getting rid of the new function and inline the `controller && controller->CanGoBackToOpener` at the callsites if you wanted.
Marking as resolved though since that is ultimately preference, nice work!
Diana QuIs the cast needed here? I would assume no since `Browser` extends `BrowserWindowInterface` already 👍
Ah that make sense. Updated all test around cast.
Done
Diana Qusuper nit: new line after this one to separate the test below
Thanks! Updated.
- Diana Qu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darryl James (Gerrit)
Darryl James added 1 comment![]( "Open in Gerrit")
Hi Darryl! I updated a bit base on your comment! Could you take another look when you get a chance?
Also do you mind add another reviewer that you think would be good to take a look at this?(Since part 1 is still pending Takashi's review. So no rush on this one.)
Great work! I can poke around and find someone to help with this review for you!
Related details
- David Pennington
- Diana Qu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darryl James (Gerrit)
Darryl James voted and added 1 comment![]( "Open in Gerrit")
Votes added by Darryl James
| Code-Review | +0 |
1 comment
Adding @dpenning as a secondary reviewer - feel free to pass along if you are aware of someone else with experience in this area
Design document can be found here: https://docs.google.com/document/d/1P02yjuD-mbUNzFXZYJnMqhJM_ZuivHFVbvRPIB8rX-o/edit?tab=t.0#heading=h.7nki9mck5t64
Related details
- David Pennington
- Diana Qu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darryl James (Gerrit)
Darryl James voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- David Pennington
- Diana Qu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Diana Qu (Gerrit)
Diana Qu added 3 comments![]( "Open in Gerrit")
Hi Liang~ Add you for a second eye on this as well.
Adding @dpenning as a secondary reviewer - feel free to pass along if you are aware of someone else with experience in this area
Design document can be found here: https://docs.google.com/document/d/1P02yjuD-mbUNzFXZYJnMqhJM_ZuivHFVbvRPIB8rX-o/edit?tab=t.0#heading=h.7nki9mck5t64
Thanks! I also add Liang from my team for another eye.
if (CanGoBackToOpener(web_contents)) {
tabs::TabInterface* tab =
tabs::TabInterface::MaybeGetFromContents(web_contents);Diana Quoptional: It might be worth finding a way to only query for the tab and controller once. I noticed that CanGoBackToOpener and the code within this block re-query for the tab / controller.
I don't believe this is an expensive operation so I am fine with this as is 👍
Darryl JamesI created another helper with different parameter!
Sweet!
Because you have inverted the condition by checking the tab first you can also consider getting rid of the new function and inline the `controller && controller->CanGoBackToOpener` at the callsites if you wanted.
Marking as resolved though since that is ultimately preference, nice work!
Good note. I will make a note to myself and probably try to update this in part 4
Related details
- David Pennington
- Liang Zhao
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
GoBack(GetTabAndRevertIfNecessary(browser, disposition));The existing code checks CanGoBack(browser) before calling GetTabAndRevertIfNecessary. If CanGoBack is false, the new code will revert omnibox edits while the old code does not change anything. Is it possible for this function to be called with CanGoBack as false?
Related details
- David Pennington
- Diana Qu
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. |
David Pennington (Gerrit)
David Pennington voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Pennington
| Code-Review | +1 |
1 comment
adding creis@ to review navigation code in the back_to_opener_controller.cc
Related details
- Diana Qu
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. |
Diana Qu (Gerrit)
Diana Qu added 1 comment![]( "Open in Gerrit")
GoBack(GetTabAndRevertIfNecessary(browser, disposition));The existing code checks CanGoBack(browser) before calling GetTabAndRevertIfNecessary. If CanGoBack is false, the new code will revert omnibox edits while the old code does not change anything. Is it possible for this function to be called with CanGoBack as false?
The new code won't get called when CanGoBack is false. It will check on the normal CanGoBack first and then CanGoBackToOpener before trying to goback. If you think its a concern. I can add another test around omnibox cases.
Related details
- Liang Zhao
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. |
Liang Zhao (Gerrit)
Liang Zhao voted and added 1 comment![]( "Open in Gerrit")
Votes added by Liang Zhao
| Code-Review | +1 |
1 comment
GoBack(GetTabAndRevertIfNecessary(browser, disposition));Diana QuThe existing code checks CanGoBack(browser) before calling GetTabAndRevertIfNecessary. If CanGoBack is false, the new code will revert omnibox edits while the old code does not change anything. Is it possible for this function to be called with CanGoBack as false?
The new code won't get called when CanGoBack is false. It will check on the normal CanGoBack first and then CanGoBackToOpener before trying to goback. If you think its a concern. I can add another test around omnibox cases.
Looks like this is related to IDC_BACK, and there are code that update the command's enable state based on ShouldEnableBackButton(). So, it should not be called when CanGoBack is false. You could do some manual tests to confirm though: open a tab, type something in address bar and press Alt+<left-arrow> to see whether the address bar changes with your change.
Related details
- Diana Qu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |