[bedrock] Migrate BrowserList begin/end Part 12. [chromium/src : main]
Qikai Zhong (Gerrit)
Qikai Zhong added 1 comment![]( "Open in Gerrit")
Hi, this CL is ready, please help review, thanks~
Related details
- Alex Carutasu
- Tom Lukaszewicz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Qikai Zhong (Gerrit)
Qikai Zhong added 1 comment![]( "Open in Gerrit")
// Note: When invoking BrowserController::ForEachBrowser in
// OnBrowserCreated, the new browser will show up for
// kAscendingCreationTime but not yet for kAscendingActivationTime.
// TODO(crbug.com/369688254): Revisit this behavior.Do not have permission to check this bug. But I think it should gone after https://chromium-review.googlesource.com/c/chromium/src/+/7080234
Related details
- Alex Carutasu
- Tom Lukaszewicz
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. |
Alex Carutasu (Gerrit)
Alex Carutasu added 3 comments![]( "Open in Gerrit")
Thanks for the changes, left some comments.
ash::BrowserController::BrowserOrder::kAscendingCreationTime,Curious: how do we know that this ordering isn't an important assumption in any of the touched components?
if (callback(*GetDelegate(browser)) == kBreakIteration) {
return false; // stop iterating
}
return true; // continue iteratingnit, a bit shorter:
```suggestion
return callback(*GetDelegate(browser)) != kBreakIteration;
```
Related details
- Qikai Zhong
- Tom Lukaszewicz
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. |
Qikai Zhong (Gerrit)
Qikai Zhong added 2 comments![]( "Open in Gerrit")
ash::BrowserController::BrowserOrder::kAscendingCreationTime,Curious: how do we know that this ordering isn't an important assumption in any of the touched components?
Our current plan is to remove the dependency on the browser creation order. Therefore, I'll first try proposing a CL and running a CQ dry run to see the results. As far as I know, this CL will definitely change some shelf behaviors, so this CL will definitely need owner review.
if (callback(*GetDelegate(browser)) == kBreakIteration) {
return false; // stop iterating
}
return true; // continue iteratingnit, a bit shorter:
```suggestion
return callback(*GetDelegate(browser)) != kBreakIteration;
```
- Alex Carutasu
- Tom Lukaszewicz
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. |
Tom Lukaszewicz (Gerrit)
Tom Lukaszewicz voted and added 3 comments![]( "Open in Gerrit")
Votes added by Tom Lukaszewicz
| Code-Review | +1 |
3 comments
Yury could you ptal for ash shelf?
ash::BrowserController::BrowserOrder::kAscendingCreationTime,Curious: how do we know that this ordering isn't an important assumption in any of the touched components?
Qikai is right and we shouldn't be exposing browser ordering directly to clients.
It looks like the shelf context menu is the only client that is specifically interested in this.
Looking over the expectations it's not clear that ordering is strictly important - the comments explicitly mention testing for the presence of the browser (not its position), and the positional assertions may just have just been a convenient way to test for this.
@kh...@chromium.org do you happen to know if creation order is important on the ash shelf context menu?
// Note: When invoking BrowserController::ForEachBrowser in
// OnBrowserCreated, the new browser will show up for
// kAscendingCreationTime but not yet for kAscendingActivationTime.
// TODO(crbug.com/369688254): Revisit this behavior.Do not have permission to check this bug. But I think it should gone after https://chromium-review.googlesource.com/c/chromium/src/+/7080234
Yep! The issue the comment mentions should have been resolved by your CL (the bug itself tracks a more general issue so we can leave that open).
Related details
- Alex Carutasu
- Qikai Zhong
- Yury Khmel
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. |