extensions: Enable chrome.tabGroups.update() API on desktop Android [chromium/src : main]
James Cook (Gerrit)
James Cook added 1 comment![]( "Open in Gerrit")
Related details
- Zoraiz Naeem
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zoraiz Naeem (Gerrit)
Zoraiz Naeem added 3 comments![]( "Open in Gerrit")
// Android does not have a SupportsTabGroups() method.Even if its not supported, we can still use the new interfaces right (Just for consistency) or the new interfaces are yet fully back compatible?
ASSERT_TRUE(group.has_value());nit: ASSERT_TRUE(group);
`ASSERT_TRUE(std::optional<T>);` directly checks for the value. So a bit less code.
ASSERT_TRUE(new_visual_data.has_value());nit: `ASSERT_TRUE(new_visual_data)`
Related details
- James Cook
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. |
James Cook (Gerrit)
James Cook added 4 comments![]( "Open in Gerrit")
Even if its not supported, we can still use the new interfaces right (Just for consistency) or the new interfaces are yet fully back compatible?
In this case, the new interfaces aren't fully backwards compatible. :-( Fortunately the things that don't support tab groups are things like Chrome Apps, which are not supported on Android.
nit: ASSERT_TRUE(group);
`ASSERT_TRUE(std::optional<T>);` directly checks for the value. So a bit less code.
Done.
FWIW I like to use has_value() so I don't have to think about cases like this:
```
std::optional<bool> foo = false;
ASSERT_TRUE(foo);
```
Is naked `foo` true-ish or false-ish? It's true-ish, but I have to think about it. :-)
ASSERT_TRUE(new_visual_data.has_value());James Cooknit: `ASSERT_TRUE(new_visual_data)`
Done
Related details
- Zoraiz Naeem
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zoraiz Naeem (Gerrit)
Zoraiz Naeem voted and added 2 comments![]( "Open in Gerrit")
Votes added by Zoraiz Naeem
| Code-Review | +1 |
2 comments
lgtm!
ASSERT_TRUE(group.has_value());James Cooknit: ASSERT_TRUE(group);
`ASSERT_TRUE(std::optional<T>);` directly checks for the value. So a bit less code.
Done.
FWIW I like to use has_value() so I don't have to think about cases like this:
```
std::optional<bool> foo = false;
ASSERT_TRUE(foo);
```
Is naked `foo` true-ish or false-ish? It's true-ish, but I have to think about it. :-)
From syntax point I think of optionals as unique_ptr/ptrs. But I must agree that in the examples as above, it makes sense to use `.value()`. I tend to not to use `value()` unless it provides clarity.
Related details
- James Cook
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zoraiz Naeem (Gerrit)
Zoraiz Naeem added 1 comment![]( "Open in Gerrit")
// Android does not have a SupportsTabGroups() method.James CookEven if its not supported, we can still use the new interfaces right (Just for consistency) or the new interfaces are yet fully back compatible?
In this case, the new interfaces aren't fully backwards compatible. :-( Fortunately the things that don't support tab groups are things like Chrome Apps, which are not supported on Android.
out of curiosity, do we plan to make them compatibly? (So that we just use these interfaces across the codebase without thinking)
Related details
- James Cook
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Cook (Gerrit)
James Cook added 2 comments![]( "Open in Gerrit")
Thanks for the review!
// Android does not have a SupportsTabGroups() method.James CookEven if its not supported, we can still use the new interfaces right (Just for consistency) or the new interfaces are yet fully back compatible?
Zoraiz NaeemIn this case, the new interfaces aren't fully backwards compatible. :-( Fortunately the things that don't support tab groups are things like Chrome Apps, which are not supported on Android.
out of curiosity, do we plan to make them compatibly? (So that we just use these interfaces across the codebase without thinking)
Yeah, I think the plan is to move them slowly towards being more compatible. I've done a little bit of that with the tabGroups API changes. But there's a lot left to do. I think there are a couple folks on the Clank team looking at it.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
James Cook (Gerrit)
James Cook voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
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
extensions: Enable chrome.tabGroups.update() API on desktop Android
Rewrite the API implementation to use the cross-platform
BrowserWindowInterface and TabListInterface. Do the same for the
browser tests.
- M chrome/browser/extensions/api/tab_groups/BUILD.gn
- M chrome/browser/extensions/api/tab_groups/tab_groups_api.cc
- M chrome/browser/extensions/api/tab_groups/tab_groups_api_browsertest.cc
- M chrome/browser/extensions/extension_tab_util.h
Code-Review: +1 by Zoraiz Naeem
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |