[Extensions] Add tests for call order of manifest handlers [chromium/src : main]
Anton Bershanskyi (Gerrit)
Anton Bershanskyi added 2 comments![]( "Open in Gerrit")
Hi Devlin,
this CL adds a few tests validating the call order of `extensions::ParseAppURLs()` and `AppLaunchInfo::Parse()`. I would like to land it before landing CL 7863620 to ensure that CL 7863620 has no behavioral changes.
Also, I would like to delete `AppLaunchInfo::OverrideLaunchURL()` which is not used any more. If you prefer, we can remove `AppLaunchInfo::OverrideLaunchURL()` in a separate CL 7879908 or together within this CL.
void AppLaunchInfo::OverrideLaunchURL(Extension* extension,This CL is a super-set of CL 7879908. Devlin, if you are fine with removing `AppLaunchInfo::OverrideLaunchURL()` in this CL together with adding these tests, I can close CL 7879908 in favor of this CL.
Related details
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 2 comments![]( "Open in Gerrit")
Votes added by Devlin Cronin
| Code-Review | +1 |
2 comments
Thanks, Anton! LGTM, and I'm fine removing the unused method in this CL.
URLPattern pattern(Extension::kValidWebExtentSchemes);
EXPECT_TRUE(pattern.SetScheme("*"));
pattern.SetHost("www.google.com");
pattern.SetPath("/*");nit: since this is a test, maybe just inline the construction:
```
URLPattern pattern(kValidWebExtentSchemes, "*://www.google.com/*")
```
Ditto below
Related details
- Anton Bershanskyi
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. |
Anton Bershanskyi (Gerrit)
Anton Bershanskyi added 1 comment![]( "Open in Gerrit")
URLPattern pattern(Extension::kValidWebExtentSchemes);
EXPECT_TRUE(pattern.SetScheme("*"));
pattern.SetHost("www.google.com");
pattern.SetPath("/*");nit: since this is a test, maybe just inline the construction:
```
URLPattern pattern(kValidWebExtentSchemes, "*://www.google.com/*")
```Ditto below
I updated two `URLPattern`s to use inline constructor, but the third one does not serialize to anything meaningful (it becomes `":/*"`). Please let me know if there is a better way to write it.
Related details
- Devlin Cronin
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. |
Devlin Cronin (Gerrit)
Devlin Cronin added 1 comment![]( "Open in Gerrit")
URLPattern pattern(Extension::kValidWebExtentSchemes);
EXPECT_TRUE(pattern.SetScheme("*"));
pattern.SetHost("www.google.com");
pattern.SetPath("/*");Anton Bershanskyinit: since this is a test, maybe just inline the construction:
```
URLPattern pattern(kValidWebExtentSchemes, "*://www.google.com/*")
```Ditto below
I updated two `URLPattern`s to use inline constructor, but the third one does not serialize to anything meaningful (it becomes `":/*"`). Please let me know if there is a better way to write it.
Hmm... How were you trying to serialize it?
Right now, it's missing a SetScheme() call, which isn't really valid in practice (production callsites pretty much all parse URLPatterns directly), and I'd expect this to be `URLPattern(kValidWebExtentSchemes, "*://gmail.com/*")`. Does that not work, or does that not work with the scheme set for some reason?
Related details
- Anton Bershanskyi
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. |
Anton Bershanskyi (Gerrit)
Anton Bershanskyi added 1 comment![]( "Open in Gerrit")
URLPattern pattern(Extension::kValidWebExtentSchemes);
EXPECT_TRUE(pattern.SetScheme("*"));
pattern.SetHost("www.google.com");
pattern.SetPath("/*");Anton Bershanskyinit: since this is a test, maybe just inline the construction:
```
URLPattern pattern(kValidWebExtentSchemes, "*://www.google.com/*")
```Ditto below
Devlin CroninI updated two `URLPattern`s to use inline constructor, but the third one does not serialize to anything meaningful (it becomes `":/*"`). Please let me know if there is a better way to write it.
Hmm... How were you trying to serialize it?
Right now, it's missing a SetScheme() call, which isn't really valid in practice (production callsites pretty much all parse URLPatterns directly), and I'd expect this to be `URLPattern(kValidWebExtentSchemes, "*://gmail.com/*")`. Does that not work, or does that not work with the scheme set for some reason?
I checked again and `URLPattern(Extension::kValidWebExtentSchemes, "https://www.gmail.com/*")` worked.
Related details
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Devlin Cronin
| Code-Review | +1 |
- Anton Bershanskyi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anton Bershanskyi (Gerrit)
Anton Bershanskyi added 1 comment![]( "Open in Gerrit")
Hi Justin, would you be interested in reviewing these new tests? This CL should not have any behavioral changes. Thanks in advance.
Related details
- Justin Lulejian
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Lulejian (Gerrit)
Justin Lulejian voted and added 7 comments![]( "Open in Gerrit")
Votes added by Justin Lulejian
| Code-Review | +1 |
7 comments
lgtm, thanks Anton! Just a few minor comments.
handler refactoring.Probably want to mention the removal of `OverrideLaunchURL` as well since it expands what this commit is doing.
// If launch_web_url_ is not empty, then it was set in kLaunchWebURL path```suggestion
// If launch_web_url_ is not empty, then it was set in `kLaunchWebURL` path
```
// Ensure consistency of extension->web_extent().is_empty() with actual```suggestion
// Ensure consistency of `extension->web_extent().is_empty()` with actual
```
// Ensure consistency of extension->web_extent().is_empty() with actual
// Extension origins.```suggestion
// Ensure consistency of `extension->web_extent().is_empty()` with actual
// `Extension` origins.
```
DCHECK(URLOverrides::GetChromeURLOverrides(extension).empty());Consider adding a comment here mentioning the dependency on ChromeURLOverridesHandler running before this handler, similar to the explanation in the new test AppURLsAndAppLaunchWebURL.
// extent_ should be immutable after manifest parsing finishes.```suggestion
// `extent_` should be immutable after manifest parsing finishes.
```
Related details
- Anton Bershanskyi
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. |
Anton Bershanskyi (Gerrit)
Anton Bershanskyi added 7 comments![]( "Open in Gerrit")
Hi Justin, thanks for review. I incorporated changes, please take another look and feel free to mark discussions "resolved" if changes are OK.
handler refactoring.Probably want to mention the removal of `OverrideLaunchURL` as well since it expands what this commit is doing.
Added
> Remove unused private method `AppLaunchInfo::OverrideLaunchURL()`.
// If launch_web_url_ is not empty, then it was set in kLaunchWebURL path```suggestion
// If launch_web_url_ is not empty, then it was set in `kLaunchWebURL` path
```
Fix applied.
// Ensure consistency of extension->web_extent().is_empty() with actual```suggestion
// Ensure consistency of `extension->web_extent().is_empty()` with actual
```
Fix applied.
// Ensure consistency of extension->web_extent().is_empty() with actual
// Extension origins.```suggestion
// Ensure consistency of `extension->web_extent().is_empty()` with actual
// `Extension` origins.
```
Done
DCHECK(URLOverrides::GetChromeURLOverrides(extension).empty());Consider adding a comment here mentioning the dependency on ChromeURLOverridesHandler running before this handler, similar to the explanation in the new test AppURLsAndAppLaunchWebURL.
Follow-up CL 7863620 will make this dependency explicit and add comments where appropriate.
// extent_ should be immutable after manifest parsing finishes.```suggestion
// `extent_` should be immutable after manifest parsing finishes.
```
- Justin Lulejian
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. |