extensions: Add tab groups API browser tests to desktop Android [chromium/src : main]
James Cook (Gerrit)
James Cook voted and added 3 comments![]( "Open in Gerrit")
Votes added by James Cook
| Commit-Queue | +1 |
3 comments
Zoraiz, another one for you. Thanks again.
#if BUILDFLAG(ENABLE_EXTENSIONS)ENABLE_EXTENSIONS is an extensions convention that means "all platforms except desktop Android for now, but we'll add Android later."
static_assert(BUILDFLAG(ENABLE_EXTENSIONS_CORE));ENABLE_EXTENSIONS_CORE includes desktop Android.
Related details
- Zoraiz Naeem
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. |
Zoraiz Naeem (Gerrit)
Zoraiz Naeem added 5 comments![]( "Open in Gerrit")
#if BUILDFLAG(ENABLE_EXTENSIONS)ENABLE_EXTENSIONS is an extensions convention that means "all platforms except desktop Android for now, but we'll add Android later."
Acknowledged
#endifplease add `// BUILDFLAG(ENABLE_EXTENSIONS)`. My bad but I spent sometime visually to see where it was ending XD
static_assert(BUILDFLAG(ENABLE_EXTENSIONS_CORE));ENABLE_EXTENSIONS_CORE includes desktop Android.
Would this check fail? Since it being complied outside of if-def block? Whats the purpose?
#if BUILDFLAG(ENABLE_EXTENSIONS)Do added this if-def section to later enable it for Android?
// don't support tab groups, so tab groups are always supported.`apps that have tab strips don't support tab group` -> `apps that have tab strips that don't support tab group` ->
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 5 comments![]( "Open in Gerrit")
please add `// BUILDFLAG(ENABLE_EXTENSIONS)`. My bad but I spent sometime visually to see where it was ending XD
Done
Zoraiz NaeemENABLE_EXTENSIONS_CORE includes desktop Android.
Would this check fail? Since it being complied outside of if-def block? Whats the purpose?
This is asserting that the whole extension system is enabled, which is true on Win/Mac/Linux/ChromeOS/desktop Android. We use it as a clue that a file has been ported to desktop Android. Otherwise it's not obvious (when opening the file, or looking at it in code search). We used to have to dig through BUILD.gn files to tell which files have been converted.
Do added this if-def section to later enable it for Android?
Yes, this `#if BUILDFLAG(ENABLE_EXTENSIONS)` directive is how we indicate that something doesn't work yet on desktop Android, but we want to port it in the future. It's a convention we use extensively throughout the extensions codebase.
We don't use `#if !BUIIDFLAG(IS_ANDROID)` because we use that to mean "this is a platform difference on Android that will stay this way permanently".
// don't support tab groups, so tab groups are always supported.`apps that have tab strips don't support tab group` -> `apps that have tab strips that don't support tab group` ->
- 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 & nit
ASSERT_TRUE(group.has_value());nit: ASSERT_TRUE(group);
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 2 comments![]( "Open in Gerrit")
Thanks for the review!
ASSERT_TRUE(group.has_value());James Cooknit: ASSERT_TRUE(group);
This is fixed in the next CL in the chain. Hopefully that's OK.
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: Add tab groups API browser tests to desktop Android
Now that we've converted some of the unit tests that used Browser and
BrowserWindow to browser tests, we can start enabling them on Android.
Add the test to the build. Enable tests for the get() API, which is
the only one we support on Android right now. Rewrite the test suite
to use cross-platform tab list and tab concepts, so it runs on
Android.
Comment out the remaining tests with BUILDFLAG(ENABLE_EXTENSIONS),
which is our convention for extensions code to be ported to Android
later.
- M chrome/browser/extensions/api/tab_groups/tab_groups_api_browsertest.cc
- M chrome/test/BUILD.gn
Code-Review: +1 by Zoraiz Naeem
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |